* [PATCH] [PATCH v2] gpg-interface.c: trim CR only before LF
@ 2025-10-16 20:03 Okhuomon Ajayi
2025-10-17 11:55 ` Christian Couder
0 siblings, 1 reply; 3+ messages in thread
From: Okhuomon Ajayi @ 2025-10-16 20:03 UTC (permalink / raw)
To: git; +Cc: Okhuomon Ajayi
Problem:
The function remove_cr_after() stripped CRs blindly. The comment suggested
NEEDSWORK: trim only CRs before LF. This caused potential confusion.
Solution:
Rename remove_cr_after() to trim_cr_before_lf() and update the comment:
"Trim CR characters only when they appear before LF (\r\n) line endings."
This keeps lone CRs intact and documents intent clearly.
Also improved formatting.
Signed-off-by: Okhuomon Ajayi <okhuomonajayi54@gmail•com>
---
gpg-interface.c | 34 ++++++++++++++++++++++++----------
1 file changed, 24 insertions(+), 10 deletions(-)
diff --git a/gpg-interface.c b/gpg-interface.c
index c961607444..2d114e05e8 100644
--- a/gpg-interface.c
+++ b/gpg-interface.c
@@ -964,23 +964,37 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
return use_format->sign_buffer(buffer, signature, signing_key);
}
-/*
- * Trim CR characters only when they appear before LF (\r\n) line endings.
- * This avoids removing legitimate lone CRs from teh content.
- */
+/* Convert CRLF to LF, in case we are on Windows */
static void trim_cr_before_lf(struct strbuf *buffer, size_t offset)
{
size_t i, j;
+ for (i = j = offset; i < buffer->len; i++) {
+ /* Skip CR only if it comes right before LF */
+ if (buffer->buf[i] == '\r' && i + 1 < buffer->len &&
+ buffer->buf[i + 1] == '\n')
+ continue;
+
+ if (i != j)
+ buffer->buf[j] = buffer->buf[i];
+ j++;
+ }
+ strbuf_setlen(buffer, j);
+}
+
+static void trim_cr_before_lf(struct strbuf *buffer, size_t offset)
+{
+ size_t i, j;
+
for (i = j = offset; i < buffer->len; i++) {
/* skip CR only if it comes right before LF */
- if (buffer->buf[i] == '\r' && i + 1 < buffer->len && buffer->buf[i+1] == '\n')
- continue;
+ if (buffer->buf[i] == '\r' && i + 1 < buffer->len &&
+ buffer->buf[i+1] == '\n')
+ continue;
- if (i != j)
- buffer->buf[j] = buffer->buf[i];
- j++;
-
+ if (i != j)
+ buffer->buf[j] = buffer->buf[i];
+ j++;
}
strbuf_setlen(buffer, j);
}
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH] [PATCH v2] gpg-interface.c: trim CR only before LF
2025-10-16 20:03 [PATCH] [PATCH v2] gpg-interface.c: trim CR only before LF Okhuomon Ajayi
@ 2025-10-17 11:55 ` Christian Couder
2025-10-17 19:12 ` Junio C Hamano
0 siblings, 1 reply; 3+ messages in thread
From: Christian Couder @ 2025-10-17 11:55 UTC (permalink / raw)
To: Okhuomon Ajayi; +Cc: git
On Thu, Oct 16, 2025 at 10:04 PM Okhuomon Ajayi
<okhuomonajayi54@gmail•com> wrote:
>
> Problem:
> The function remove_cr_after() stripped CRs blindly. The comment suggested
> NEEDSWORK: trim only CRs before LF.
We use the present tense to talk about the current situation. In
"Documentation/SubmittingPatches" there is:
"[[present-tense]]
The problem statement that describes the status quo is written in the
present tense. Write "The code does X when it is given input Y",
instead of "The code used to do Y when given input X". You do not
have to say "Currently"---the status quo in the problem statement is
about the code _without_ your change, by project convention."
Also you don't need to prefix this part with "Problem:". We should
understand from the description of the status quo that the situation
is not good and should be improved.
> This caused potential confusion.
It's not clear what caused potential confusion. Is it the "NEEDSWORK:
..." comment, or the fact that remove_cr_after() stripped CRs blindly,
or both?
Also it's not clear what the confusion is about. Is there confusion
because a reader can wonder if stripping CR blindly could be a bug?
What about something like:
"The remove_cr_after() function removes any CR it finds in a buffer
after an offset, but a 'NEEDSWORK' code comment in front of it says
that it should only remove a CR that is before an LF. This can make
readers wonder if stripping CRs blindly could result in bugs."
> Solution:
> Rename remove_cr_after() to trim_cr_before_lf() and update the comment:
Here also, from the description of what the patch does, we should
understand that it will improve things, so no need to prefix it with
"Solution:".
The issue is that to know what should be done about the current
situation, it would help to know if stripping CRs blindly could result
in bugs or not. So there should be an analysis part before the part
describing what the patch does. For example the analysis part could
say something like:
"As the remove_cr_after() function is only used to replace CR LF
sequences (generated by software on Windows) with a single LF, the
'NEEDSWORK' code comment seems to be correct. It seems safer to only
remove a CR when it is before an LF even if the buffer is not likely
to contain any other CR."
(Then you could even further clarify the goal of the patch when
starting to describe what the patch does with something like:
"To implement this safe solution suggested by the NEEDSWORK comment,
rename remove_cr_after() to trim_cr_before_lf() ..."
It might not be necessary here, but I mention it so that you can see
how to smoothly transition from the problem description.)
By the way you mention renaming remove_cr_after() to
trim_cr_before_lf() and updating the comment before it, but you don't
mention actually changing the implementation of the function so that
it only removes a CR when it's before a LF.
> "Trim CR characters only when they appear before LF (\r\n) line endings."
No need to duplicate the new code comment in the commit message. We
can see it in the patch.
> This keeps lone CRs intact and documents intent clearly.
This sentence is fine.
> Also improved formatting.
It's not clear what formatting is improved. And this should use an
imperative tone, like the above did with "Rename remove_cr_after() ...
and update ..."
> Signed-off-by: Okhuomon Ajayi <okhuomonajayi54@gmail•com>
> ---
> gpg-interface.c | 34 ++++++++++++++++++++++++----------
> 1 file changed, 24 insertions(+), 10 deletions(-)
>
> diff --git a/gpg-interface.c b/gpg-interface.c
> index c961607444..2d114e05e8 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -964,23 +964,37 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig
> return use_format->sign_buffer(buffer, signature, signing_key);
> }
>
> -/*
> - * Trim CR characters only when they appear before LF (\r\n) line endings.
> - * This avoids removing legitimate lone CRs from teh content.
> - */
> +/* Convert CRLF to LF, in case we are on Windows */
I don't see any "NEEDSWORKS" here. It looks like this is a patch that
was made against the version 1 of the patch you sent earlier. Instead,
all the versions of your patches should be made against a relatively
recent version of the 'master' branch.
This way if your patch is accepted, only your patch needs to be
merged. Also that makes it easier for reviewers to see that the commit
message (which starts by describing the current situation in 'master')
is correct.
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] [PATCH v2] gpg-interface.c: trim CR only before LF
2025-10-17 11:55 ` Christian Couder
@ 2025-10-17 19:12 ` Junio C Hamano
0 siblings, 0 replies; 3+ messages in thread
From: Junio C Hamano @ 2025-10-17 19:12 UTC (permalink / raw)
To: Christian Couder; +Cc: Okhuomon Ajayi, git
Christian Couder <christian.couder@gmail•com> writes:
> On Thu, Oct 16, 2025 at 10:04 PM Okhuomon Ajayi
> <okhuomonajayi54@gmail•com> wrote:
>>
>> Problem:
>> The function remove_cr_after() stripped CRs blindly. The comment suggested
>> NEEDSWORK: trim only CRs before LF.
>
> We use the present tense to talk about the current situation. In
> "Documentation/SubmittingPatches" there is:
>
> "[[present-tense]]
> The problem statement that describes the status quo is written in the
> present tense. Write "The code does X when it is given input Y",
> instead of "The code used to do Y when given input X". You do not
> have to say "Currently"---the status quo in the problem statement is
> about the code _without_ your change, by project convention."
>
> Also you don't need to prefix this part with "Problem:". We should
> understand from the description of the status quo that the situation
> is not good and should be improved.
Thanks for the above two pieces of advice. The latter follows if
messages of all commits follow a simple convention that we have been
following, which is that the usual way to compose a log message of
this project is to
- Give an observation on how the current system works in the
present tense (so no need to say "Currently X is Y", or
"Previously X was Y" to describe the state before your change;
just "X is Y" is enough), and discuss what you perceive as a
problem in it.
- Propose a solution (optional---often, problem description
trivially leads to an obvious solution in reader's minds).
- Give commands to somebody editing the codebase to "make it so",
instead of saying "This commit does X".
in this order.
Perhaps we should write it somewhere in the introductory text
designed to help applicants of mentoring programs like Outreachy and
GSoC?
Thanks.
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2025-10-17 19:13 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-10-16 20:03 [PATCH] [PATCH v2] gpg-interface.c: trim CR only before LF Okhuomon Ajayi
2025-10-17 11:55 ` Christian Couder
2025-10-17 19:12 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox