{"thread":{"id":"64338","subject":"[PATCH] [PATCH v2] gpg-interface.c: trim CR only before LF","startedAt":"2025-10-16T20:03:58Z","lastAt":"2025-10-17T19:13:00Z","messageCount":3,"participants":["Okhuomon Ajayi","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"528996","messageId":"20251016200344.43239-1-okhuomonajayi54@gmail.com","threadId":"64338","inReplyTo":null,"subject":"[PATCH] [PATCH v2] gpg-interface.c: trim CR only before LF","fromName":"Okhuomon Ajayi","fromEmail":"okhuomonajayi54@gmail.com","sentAt":"2025-10-16T20:03:44Z","receivedAt":"2025-10-16T20:03:58Z","isPatch":true,"sender":{"key":"okhuomonajayi54@gmail.com","avatar":null},"body":"Problem:\nThe function remove_cr_after() stripped CRs blindly. The comment suggested\nNEEDSWORK: trim only CRs before LF. This caused potential confusion.\n\nSolution:\nRename remove_cr_after() to trim_cr_before_lf() and update the comment:\n\"Trim CR characters only when they appear before LF (\\r\\n) line endings.\"\nThis keeps lone CRs intact and documents intent clearly.\n\nAlso improved formatting.\n\nSigned-off-by: Okhuomon Ajayi \n---\n gpg-interface.c | 34 ++++++++++++++++++++++++----------\n 1 file changed, 24 insertions(+), 10 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex c961607444..2d114e05e8 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -964,23 +964,37 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n \treturn use_format->sign_buffer(buffer, signature, signing_key);\n }\n \n-/*\n- * Trim CR characters only when they appear before LF (\\r\\n) line endings.\n- * This avoids removing legitimate lone CRs from teh content.\n- */\n+/* Convert CRLF to LF, in case we are on Windows */\n static void trim_cr_before_lf(struct strbuf *buffer, size_t offset)\n {\n \tsize_t i, j;\n \n+\tfor (i = j = offset; i < buffer->len; i++) {\n+\t\t/* Skip CR only if it comes right before LF */\n+\t\tif (buffer->buf[i] == '\\r' && i + 1 < buffer->len &&\n+\t\t buffer->buf[i + 1] == '\\n')\n+\t\t\tcontinue;\n+\n+\t\tif (i != j)\n+\t\t\tbuffer->buf[j] = buffer->buf[i];\n+\t\tj++;\n+\t}\n+\tstrbuf_setlen(buffer, j);\n+}\n+\n+static void trim_cr_before_lf(struct strbuf *buffer, size_t offset)\n+{\n+ size_t i, j;\n+\n \tfor (i = j = offset; i < buffer->len; i++) {\n \t /* skip CR only if it comes right before LF */\n-\t\tif (buffer->buf[i] == '\\r' && i + 1 < buffer->len && buffer->buf[i+1] == '\\n')\n-\t\t continue;\n+\t if (buffer->buf[i] == '\\r' && i + 1 < buffer->len &&\n+\t\t buffer->buf[i+1] == '\\n')\n+\t\t continue;\n \n-\t\t\tif (i != j)\n-\t\t\t\tbuffer->buf[j] = buffer->buf[i];\n-\t\t\tj++;\n-\t\t\n+ if (i != j)\n+\t\t buffer->buf[j] = buffer->buf[i];\n+\t j++;\n \t}\n \tstrbuf_setlen(buffer, j);\n }\n-- \n2.43.0\n\n"},{"id":"529062","messageId":"CAP8UFD2sdvkv_ZqiLZU9k5zF+tM3UTQ8+mJjziRZGzOra6dMFA@mail.gmail.com","threadId":"64338","inReplyTo":"20251016200344.43239-1-okhuomonajayi54@gmail.com","subject":"Re: [PATCH] [PATCH v2] gpg-interface.c: trim CR only before LF","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2025-10-17T11:55:02Z","receivedAt":"2025-10-17T11:55:16Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Thu, Oct 16, 2025 at 10:04 PM Okhuomon Ajayi\n wrote:\n>\n> Problem:\n> The function remove_cr_after() stripped CRs blindly. The comment suggested\n> NEEDSWORK: trim only CRs before LF.\n\nWe use the present tense to talk about the current situation. In\n\"Documentation/SubmittingPatches\" there is:\n\n\"[[present-tense]]\nThe problem statement that describes the status quo is written in the\npresent tense. Write \"The code does X when it is given input Y\",\ninstead of \"The code used to do Y when given input X\". You do not\nhave to say \"Currently\"---the status quo in the problem statement is\nabout the code _without_ your change, by project convention.\"\n\nAlso you don't need to prefix this part with \"Problem:\". We should\nunderstand from the description of the status quo that the situation\nis not good and should be improved.\n\n> This caused potential confusion.\n\nIt's not clear what caused potential confusion. Is it the \"NEEDSWORK:\n...\" comment, or the fact that remove_cr_after() stripped CRs blindly,\nor both?\n\nAlso it's not clear what the confusion is about. Is there confusion\nbecause a reader can wonder if stripping CR blindly could be a bug?\n\nWhat about something like:\n\n\"The remove_cr_after() function removes any CR it finds in a buffer\nafter an offset, but a 'NEEDSWORK' code comment in front of it says\nthat it should only remove a CR that is before an LF. This can make\nreaders wonder if stripping CRs blindly could result in bugs.\"\n\n> Solution:\n> Rename remove_cr_after() to trim_cr_before_lf() and update the comment:\n\nHere also, from the description of what the patch does, we should\nunderstand that it will improve things, so no need to prefix it with\n\"Solution:\".\n\nThe issue is that to know what should be done about the current\nsituation, it would help to know if stripping CRs blindly could result\nin bugs or not. So there should be an analysis part before the part\ndescribing what the patch does. For example the analysis part could\nsay something like:\n\n\"As the remove_cr_after() function is only used to replace CR LF\nsequences (generated by software on Windows) with a single LF, the\n'NEEDSWORK' code comment seems to be correct. It seems safer to only\nremove a CR when it is before an LF even if the buffer is not likely\nto contain any other CR.\"\n\n(Then you could even further clarify the goal of the patch when\nstarting to describe what the patch does with something like:\n\n\"To implement this safe solution suggested by the NEEDSWORK comment,\nrename remove_cr_after() to trim_cr_before_lf() ...\"\n\nIt might not be necessary here, but I mention it so that you can see\nhow to smoothly transition from the problem description.)\n\nBy the way you mention renaming remove_cr_after() to\ntrim_cr_before_lf() and updating the comment before it, but you don't\nmention actually changing the implementation of the function so that\nit only removes a CR when it's before a LF.\n\n> \"Trim CR characters only when they appear before LF (\\r\\n) line endings.\"\n\nNo need to duplicate the new code comment in the commit message. We\ncan see it in the patch.\n\n> This keeps lone CRs intact and documents intent clearly.\n\nThis sentence is fine.\n\n> Also improved formatting.\n\nIt's not clear what formatting is improved. And this should use an\nimperative tone, like the above did with \"Rename remove_cr_after() ...\nand update ...\"\n\n> Signed-off-by: Okhuomon Ajayi \n> ---\n> gpg-interface.c | 34 ++++++++++++++++++++++++----------\n> 1 file changed, 24 insertions(+), 10 deletions(-)\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index c961607444..2d114e05e8 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -964,23 +964,37 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n> return use_format->sign_buffer(buffer, signature, signing_key);\n> }\n>\n> -/*\n> - * Trim CR characters only when they appear before LF (\\r\\n) line endings.\n> - * This avoids removing legitimate lone CRs from teh content.\n> - */\n> +/* Convert CRLF to LF, in case we are on Windows */\n\nI don't see any \"NEEDSWORKS\" here. It looks like this is a patch that\nwas made against the version 1 of the patch you sent earlier. Instead,\nall the versions of your patches should be made against a relatively\nrecent version of the 'master' branch.\n\nThis way if your patch is accepted, only your patch needs to be\nmerged. Also that makes it easier for reviewers to see that the commit\nmessage (which starts by describing the current situation in 'master')\nis correct.\n"},{"id":"529089","messageId":"xmqqo6q5z6iu.fsf@gitster.g","threadId":"64338","inReplyTo":"CAP8UFD2sdvkv_ZqiLZU9k5zF+tM3UTQ8+mJjziRZGzOra6dMFA@mail.gmail.com","subject":"Re: [PATCH] [PATCH v2] gpg-interface.c: trim CR only before LF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-10-17T19:12:57Z","receivedAt":"2025-10-17T19:13:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> On Thu, Oct 16, 2025 at 10:04 PM Okhuomon Ajayi\n> wrote:\n>>\n>> Problem:\n>> The function remove_cr_after() stripped CRs blindly. The comment suggested\n>> NEEDSWORK: trim only CRs before LF.\n>\n> We use the present tense to talk about the current situation. In\n> \"Documentation/SubmittingPatches\" there is:\n>\n> \"[[present-tense]]\n> The problem statement that describes the status quo is written in the\n> present tense. Write \"The code does X when it is given input Y\",\n> instead of \"The code used to do Y when given input X\". You do not\n> have to say \"Currently\"---the status quo in the problem statement is\n> about the code _without_ your change, by project convention.\"\n>\n> Also you don't need to prefix this part with \"Problem:\". We should\n> understand from the description of the status quo that the situation\n> is not good and should be improved.\n\nThanks for the above two pieces of advice. The latter follows if\nmessages of all commits follow a simple convention that we have been\nfollowing, which is that the usual way to compose a log message of\nthis project is to\n\n - Give an observation on how the current system works in the\n present tense (so no need to say \"Currently X is Y\", or\n \"Previously X was Y\" to describe the state before your change;\n just \"X is Y\" is enough), and discuss what you perceive as a\n problem in it.\n\n - Propose a solution (optional---often, problem description\n trivially leads to an obvious solution in reader's minds).\n\n - Give commands to somebody editing the codebase to \"make it so\",\n instead of saying \"This commit does X\".\n\nin this order.\n\nPerhaps we should write it somewhere in the introductory text\ndesigned to help applicants of mentoring programs like Outreachy and\nGSoC?\n\nThanks.\n"}]}