Re: [PATCH] [PATCH v2] gpg-interface.c: trim CR only before LF
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.
> 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 ..."
Show 18 quoted lines
> 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.
