{"thread":{"id":"65855","subject":"[PATCH] gpg-interface: fix strip_cr_before_lf to only remove CR before LF","startedAt":"2026-06-23T08:45:43Z","lastAt":"2026-06-23T15:09:33Z","messageCount":2,"participants":["DSAntonio08","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"546225","messageId":"20260623084520.9015-1-antonio.destefani08@gmail.com","threadId":"65855","inReplyTo":null,"subject":"[PATCH] gpg-interface: fix strip_cr_before_lf to only remove CR before LF","fromName":"DSAntonio08","fromEmail":"antonio.destefani08@gmail.com","sentAt":"2026-06-23T08:45:20Z","receivedAt":"2026-06-23T08:45:43Z","isPatch":true,"body":"The remove_cr_after() function was stripping all CR characters\nunconditionally, even lone \\r not followed by \\n. This is incorrect\nas only \\r\\n sequences (Windows line endings) should be normalized.\n\nFix the loop condition to skip \\r only when immediately followed by\n\\n, and rename the function to strip_cr_before_lf to reflect its\nactual behavior. Update both call sites and their comments accordingly.\n---\n gpg-interface.c | 18 +++++++-----------\n 1 file changed, 7 insertions(+), 11 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex dafd5371fa..87ae6503da 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -989,17 +989,13 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,\n \tfree(keyid_to_free);\n \treturn ret;\n }\n-\n-/*\n- * Strip CR from the line endings, in case we are on Windows.\n- * NEEDSWORK: make it trim only CRs before LFs and rename\n- */\n-static void remove_cr_after(struct strbuf *buffer, size_t offset)\n+/* Strip CR before LF from the line endings, in case we are on Windows. */\n+static void strip_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\tif (buffer->buf[i] != '\\r') {\n+\t\tif (buffer->buf[i] != '\\r' || (i + 1 < buffer->len && buffer->buf[i + 1] != '\\n')) {\n \t\t\tif (i != j)\n \t\t\t\tbuffer->buf[j] = buffer->buf[i];\n \t\t\tj++;\n@@ -1049,8 +1045,8 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature,\n \t}\n \tstrbuf_release(&gpg_status);\n \n-\t/* Strip CR from the line endings, in case we are on Windows. */\n-\tremove_cr_after(signature, bottom);\n+\t/* Strip CR before LF from the line endings, in case we are on Windows. */\n+\tstrip_cr_before_lf(signature, bottom);\n \n \treturn 0;\n }\n@@ -1136,8 +1132,8 @@ static int sign_buffer_ssh(struct strbuf *buffer, struct strbuf *signature,\n \t\t\tssh_signature_filename.buf);\n \t\tgoto out;\n \t}\n-\t/* Strip CR from the line endings, in case we are on Windows. */\n-\tremove_cr_after(signature, bottom);\n+\t/* Strip CR before LF from the line endings, in case we are on Windows. */\n+\tstrip_cr_before_lf(signature, bottom);\n \n out:\n \tif (key_file)\n-- \n2.54.0\n\n"},{"id":"546238","messageId":"xmqq8q85pa38.fsf@gitster.g","threadId":"65855","inReplyTo":"20260623084520.9015-1-antonio.destefani08@gmail.com","subject":"Re: [PATCH] gpg-interface: fix strip_cr_before_lf to only remove CR before LF","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-06-23T15:09:31Z","receivedAt":"2026-06-23T15:09:33Z","isPatch":true,"body":"DSAntonio08 <antonio.destefani08@gmail.com> writes:\n\n> The remove_cr_after() function was stripping all CR characters\n> unconditionally, even lone \\r not followed by \\n. This is incorrect\n> as only \\r\\n sequences (Windows line endings) should be normalized.\n>\n> Fix the loop condition to skip \\r only when immediately followed by\n> \\n, and rename the function to strip_cr_before_lf to reflect its\n> actual behavior. Update both call sites and their comments accordingly.\n> ---\n\nA few comments.\n\n - Documentation/SubmittingPatches[[sign-off]] wants you to certify\n   that this patch is something you have the right to submit to this\n   project.  Sign-off is missing at the end of the proposed log\n   message.\n\n - Documentation/SubmittingPatches[[real-name]] also prefers to see\n   us interacting with humans with real-sounding names, not handles.\n\n - When changing design decision made in a fairly ancient code, we\n   would prefer to see the proposed log message says why the code is\n   that way, and how and why it is safe to change it.\n\nAs to the last point, in this particular case, the NEEDSWORK comment\nsays \"only CRs before LFs\", but we must not blindly follow NEEDSWORK\ncomments.  They just mean \"somebody may want to rethink the issues\nin the future but right now we stop here and leave the code in this\nshape.\"\n\nSo, let's see if we can continue their thinking.\n\n  This \"We should normalize CR/LF into LF but we are too lazy to\n  bother\" originally came from c4adea82 (Convert CR/LF to LF in tag\n  signatures, 2008-07-11), and then 2f47eae2 (Split GPG interface into\n  its own helper library, 2011-09-07) moved the code around to make\n  the part interacting with GPG reusable.  Later when SSH signing was\n  introduced in 29b31577 (ssh signing: add ssh key format and signing\n  code, 2021-09-10), the \"remove CR\" was made into a helper function,\n  and that is what remains today.\n\nHaving something like the above paragraph in the proposed commit log\nmessage would have been very good.  Further, there might be old\ndiscussion that led to c4adea82 where people may have discussed if\nit is sensible to be lazy, which you may further want to dig down to\nthe root, to make sure that even back then people were aware that\ntouching only CRLF, not all CRs that appear at random places, was\nthe right thing and the code was done only due to laziness (rather\nthan, e.g., leaving lone CRs in the message somehow harms other\nparts of the system in a way we are not realizing in this\ndiscussion).\n\nThat would make a very good supporting material to convince readers\nwhy the change this patch is making a good idea.\n\n>  gpg-interface.c | 18 +++++++-----------\n>  1 file changed, 7 insertions(+), 11 deletions(-)\n>\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index dafd5371fa..87ae6503da 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -989,17 +989,13 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature,\n>  \tfree(keyid_to_free);\n>  \treturn ret;\n>  }\n> -\n> -/*\n> - * Strip CR from the line endings, in case we are on Windows.\n> - * NEEDSWORK: make it trim only CRs before LFs and rename\n> - */\n> -static void remove_cr_after(struct strbuf *buffer, size_t offset)\n> +/* Strip CR before LF from the line endings, in case we are on Windows. */\n> +static void strip_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\tif (buffer->buf[i] != '\\r') {\n> +\t\tif (buffer->buf[i] != '\\r' || (i + 1 < buffer->len && buffer->buf[i + 1] != '\\n')) {\n\nPlease avoid making lines overly long like this.  Wrapping at a\nlogical break in the expression like this:\n\n\t\tif (buffer->buf[i] != '\\r' ||\n\t\t    (i + 1 < buffer->len && buffer->buf[i + 1] != '\\n')) {\n\nwould not just make the line fit in a reasonable width limit, but\nmakes it easier to follow.\n\n>  \t\t\tif (i != j)\n>  \t\t\t\tbuffer->buf[j] = buffer->buf[i];\n>  \t\t\tj++;\n\nMore importantly, isn't the above slightly buggy?  If the buffer\nends with a lone CR (i.e. not followed by LF), your condition:\n\n\tif (buffer->buf[i] != '\\r' || (i + 1 < buffer->len && buffer->buf[i + 1] != '\\n'))\n\nwill evaluate to false because both operands of the OR are false;\nthe byte we are looking at is CR so the LHS off OR is false, and\nbecause we are at the end of the buffer, so we do not have the byte\nthat follows ours that is not LF, which makes RHS of OR also false.\nThe lone trailing CR will be skipped, instead of getting copied.\n\nIf I were writing this, I would have written it more like this:\n\n\tfor (i = j = offset; i < buffer->len; i++) {\n\t\tif (buffer->buf[i] == '\\r' &&\n\t\t    i + 1 < buffer->len && buffer->buf[i + 1] == '\\n')\n\t\t\tcontinue;\n\t\tbuffer->buf[j++] = buffer->buf[i];\n\t}\n\nThis not only avoids the bug by keeping lone trailing CRs, but\nalso makes what we are special-casing stand out more clearly\n(assignment is the norm, skipping is the exception), and avoids\npushing the assignment too deep in the conditional for readability.\n\nThe changes to the callers (due to function name update) below\n(ellided) both looked fine.\n\nThanks.\n"}]}