{"thread":{"id":"47178","subject":"[PATCH v2] gpg-interface: strip CR chars for Windows gpg2","startedAt":"2017-11-12T13:07:37Z","lastAt":"2017-11-13T10:24:21Z","messageCount":5,"participants":["Jerzy Kozera","Eric Sunshine","Torsten Bögershausen","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"332324","messageId":"20171112130710.16000-1-jerzy.kozera@gmail.com","threadId":"47178","inReplyTo":null,"subject":"[PATCH v2] gpg-interface: strip CR chars for Windows gpg2","fromName":"Jerzy Kozera","fromEmail":"jerzy.kozera@gmail.com","sentAt":"2017-11-12T13:07:10Z","receivedAt":"2017-11-12T13:07:37Z","isPatch":true,"sender":{"key":"jerzy.kozera@gmail.com","avatar":"https://gravatar.com/avatar/f9b73794f2e389bd9b490213b1aced0cfced339c940e2f657ab43fedb21291e8?d=mp&s=160"},"body":"This fixes the issue with newlines being \\r\\n and not being displayed\ncorrectly when using gpg2 for Windows, as reported at\nhttps://github.com/git-for-windows/git/issues/1249\n\nIssues with non-ASCII characters remain for further investigation.\n\nSigned-off-by: Jerzy Kozera \n---\n\nAddressed comments by Junio C Hamano (check for following \\n, and\nupdated the commit description).\n\n gpg-interface.c | 27 ++++++++++++++++++---------\n 1 file changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 4feacf16e5..ab592af7f2 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -145,6 +145,20 @@ const char *get_signing_key(void)\n \treturn git_committer_info(IDENT_STRICT|IDENT_NO_DATE);\n }\n \n+/* Strip CR from the CRLF line endings, in case we are on Windows. */\n+static void strip_cr(struct strbuf *buffer, size_t bottom) {\n+\tsize_t i, j;\n+\tfor (i = j = bottom; i < buffer->len; i++)\n+\t\tif (!(i < buffer->len - 1 &&\n+\t\t\t\tbuffer->buf[i] == '\\r' &&\n+\t\t\t\tbuffer->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+\t\t}\n+\tstrbuf_setlen(buffer, j);\n+}\n+\n /*\n * Create a detached signature for the contents of \"buffer\" and append\n * it after \"signature\"; \"buffer\" and \"signature\" can be the same\n@@ -155,7 +169,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n {\n \tstruct child_process gpg = CHILD_PROCESS_INIT;\n \tint ret;\n-\tsize_t i, j, bottom;\n+\tsize_t bottom;\n \tstruct strbuf gpg_status = STRBUF_INIT;\n \n \targv_array_pushl(&gpg.args,\n@@ -180,14 +194,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n \tif (ret)\n \t\treturn error(_(\"gpg failed to sign the data\"));\n \n-\t/* Strip CR from the line endings, in case we are on Windows. */\n-\tfor (i = j = bottom; i < signature->len; i++)\n-\t\tif (signature->buf[i] != '\\r') {\n-\t\t\tif (i != j)\n-\t\t\t\tsignature->buf[j] = signature->buf[i];\n-\t\t\tj++;\n-\t\t}\n-\tstrbuf_setlen(signature, j);\n+\tstrip_cr(signature, bottom);\n \n \treturn 0;\n }\n@@ -230,6 +237,8 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n \tsigchain_push(SIGPIPE, SIG_IGN);\n \tret = pipe_command(&gpg, payload, payload_size,\n \t\t\t gpg_status, 0, gpg_output, 0);\n+\tstrip_cr(gpg_status, 0);\n+\tstrip_cr(gpg_output, 0);\n \tsigchain_pop(SIGPIPE);\n \n \tdelete_tempfile(&temp);\n-- \n2.14.2.windows.3\n\n"},{"id":"332352","messageId":"CAPig+cSR_GGog2vATwGgMWDofVzkgDE7K=VvZ4GEyrzYC1t-GQ@mail.gmail.com","threadId":"47178","inReplyTo":"20171112130710.16000-1-jerzy.kozera@gmail.com","subject":"Re: [PATCH v2] gpg-interface: strip CR chars for Windows gpg2","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-12T17:32:43Z","receivedAt":"2017-11-12T17:32:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"Thanks for the re-roll...\n\nOn Sun, Nov 12, 2017 at 8:07 AM, Jerzy Kozera wrote:\n> This fixes the issue with newlines being \\r\\n and not being displayed\n> correctly when using gpg2 for Windows, as reported at\n> https://github.com/git-for-windows/git/issues/1249\n\nIt's still not clear from this description what \"not being displayed\ncorrectly\" means. Ideally, the commit message should stand on its own,\nexplaining exactly what problem the patch is solving, without the\nreader having to chase URLs to pages (which might disappear). If you\ncould summarize the problem and solution in your own words in such a\nway that your description itself conveys enough information for\nsomeone not familiar with that problem report to understand the\nproblem, then that would likely make a good commit message.\n\nMore below...\n\n> Issues with non-ASCII characters remain for further investigation.\n>\n> Signed-off-by: Jerzy Kozera \n> ---\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> @@ -145,6 +145,20 @@ const char *get_signing_key(void)\n> +/* Strip CR from the CRLF line endings, in case we are on Windows. */\n> +static void strip_cr(struct strbuf *buffer, size_t bottom) {\n\nIt's not at all clear what 'bottom' means. In the original, when the\ncode was inline, the surrounding context would likely have given a\ngood clue to the meaning of 'bottom', but here stand-alone, it conveys\nlittle or nothing. Perhaps a better name for this argument would be\n'start_at' or 'from' or something.\n\n> + size_t i, j;\n> + for (i = j = bottom; i < buffer->len; i++)\n> + if (!(i < buffer->len - 1 &&\n> + buffer->buf[i] == '\\r' &&\n> + buffer->buf[i + 1] == '\\n')) {\n\nHmm, was this tested? If I'm reading this correctly, this strips out\nthe entire CRLF pair, whereas the original code only stripped the CR\nand left what followed it (typically LF) alone. Junio's suggestion was\nto enhance this to be more careful and strip CR only when followed\nimmediately by LF (but to leave the LF intact). Therefore, this seems\nlike a regression.\n\n> + if (i != j)\n> + buffer->buf[j] = buffer->buf[i];\n> + j++;\n> + }\n> + strbuf_setlen(buffer, j);\n> +}\n> +\n> /*\n> * Create a detached signature for the contents of \"buffer\" and append\n> * it after \"signature\"; \"buffer\" and \"signature\" can be the same\n> @@ -155,7 +169,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n> {\n> struct child_process gpg = CHILD_PROCESS_INIT;\n> int ret;\n> - size_t i, j, bottom;\n> + size_t bottom;\n> struct strbuf gpg_status = STRBUF_INIT;\n>\n> argv_array_pushl(&gpg.args,\n> @@ -180,14 +194,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig\n> if (ret)\n> return error(_(\"gpg failed to sign the data\"));\n>\n> - /* Strip CR from the line endings, in case we are on Windows. */\n> - for (i = j = bottom; i < signature->len; i++)\n> - if (signature->buf[i] != '\\r') {\n> - if (i != j)\n> - signature->buf[j] = signature->buf[i];\n> - j++;\n> - }\n> - strbuf_setlen(signature, j);\n> + strip_cr(signature, bottom);\n>\n> return 0;\n> }\n> @@ -230,6 +237,8 @@ int verify_signed_buffer(const char *payload, size_t payload_size,\n> sigchain_push(SIGPIPE, SIG_IGN);\n> ret = pipe_command(&gpg, payload, payload_size,\n> gpg_status, 0, gpg_output, 0);\n> + strip_cr(gpg_status, 0);\n> + strip_cr(gpg_output, 0);\n> sigchain_pop(SIGPIPE);\n>\n> delete_tempfile(&temp);\n> --\n> 2.14.2.windows.3\n"},{"id":"332361","messageId":"20171112202505.GB2677@tor.lan","threadId":"47178","inReplyTo":"20171112130710.16000-1-jerzy.kozera@gmail.com","subject":"Re: [PATCH v2] gpg-interface: strip CR chars for Windows gpg2","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2017-11-12T20:25:05Z","receivedAt":"2017-11-12T20:25:15Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On Sun, Nov 12, 2017 at 01:07:10PM +0000, Jerzy Kozera wrote:\n> This fixes the issue with newlines being \\r\\n and not being displayed\n> correctly when using gpg2 for Windows, as reported at\n> https://github.com/git-for-windows/git/issues/1249\n> \n> Issues with non-ASCII characters remain for further investigation.\n> \n> Signed-off-by: Jerzy Kozera \n> ---\n> \n> Addressed comments by Junio C Hamano (check for following \\n, and\n> updated the commit description).\n> \n> gpg-interface.c | 27 ++++++++++++++++++---------\n> 1 file changed, 18 insertions(+), 9 deletions(-)\n> \n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index 4feacf16e5..ab592af7f2 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -145,6 +145,20 @@ const char *get_signing_key(void)\n> \treturn git_committer_info(IDENT_STRICT|IDENT_NO_DATE);\n> }\n> \n> +/* Strip CR from the CRLF line endings, in case we are on Windows. */\n> +static void strip_cr(struct strbuf *buffer, size_t bottom) {\n> +\tsize_t i, j;\n\nIt is not wrong to say \"Strip CR from the CRLF\".\nIn Git we often talk about \"convert CRLF into LF\",\n\nThe comment somewhat different to the function name.\nThe function namd and the name of the parameters can be more in\nin line with existing strbuf functions:\n(And the opening '{' should go into it's own line:\n\nstatic void convert_crlf_to_lf(struct strbuf *sb, size_t len)\n{\n\tsize_t i, j;\n\nAn even more generic approach (could be done in a seperate commit)\nwould be to move the whole function into strbuf.c/strbuf.h,\nand it may be called like this.\n\nvoid strbuf_crlf_to_lf(struct strbuf *sb, size_t len)\n{\n /* I would even avoid \"i\" and \"j\", and use src and dst or so) */\n size_t src_pos, dst_idx;\n}\n\nThanks for working on this.\n\n[]\n"},{"id":"332379","messageId":"xmqqk1yvaq2e.fsf@gitster.mtv.corp.google.com","threadId":"47178","inReplyTo":"CAPig+cSR_GGog2vATwGgMWDofVzkgDE7K=VvZ4GEyrzYC1t-GQ@mail.gmail.com","subject":"Re: [PATCH v2] gpg-interface: strip CR chars for Windows gpg2","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-13T02:08:09Z","receivedAt":"2017-11-13T02:08:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Thanks for the re-roll...\n>\n> On Sun, Nov 12, 2017 at 8:07 AM, Jerzy Kozera wrote:\n>> This fixes the issue with newlines being \\r\\n and not being displayed\n>> correctly when using gpg2 for Windows, as reported at\n>> https://github.com/git-for-windows/git/issues/1249\n>\n> It's still not clear from this description what \"not being displayed\n> correctly\" means. Ideally, the commit message should stand on its own,\n> explaining exactly what problem the patch is solving, without the\n> reader having to chase URLs to pages (which might disappear). If you\n> could summarize the problem and solution in your own words in such a\n> way that your description itself conveys enough information for\n> someone not familiar with that problem report to understand the\n> problem, then that would likely make a good commit message.\n\nThanks. I was wondering if I am the only one who does not\nunderstand what the revised wording wants to say.\n\n>> @@ -145,6 +145,20 @@ const char *get_signing_key(void)\n>> +/* Strip CR from the CRLF line endings, in case we are on Windows. */\n>> +static void strip_cr(struct strbuf *buffer, size_t bottom) {\n>\n> It's not at all clear what 'bottom' means. In the original, when the\n> code was inline, the surrounding context would likely have given a\n> good clue to the meaning of 'bottom', but here stand-alone, it conveys\n> little or nothing. Perhaps a better name for this argument would be\n> 'start_at' or 'from' or something.\n\nI personally do not mind 'bottom' (especially when it appears in\ncontrast to 'top') too much, but start_at would be much clearer.\n\n>> + size_t i, j;\n>> + for (i = j = bottom; i < buffer->len; i++)\n>> + if (!(i < buffer->len - 1 &&\n>> + buffer->buf[i] == '\\r' &&\n>> + buffer->buf[i + 1] == '\\n')) {\n>\n> Hmm, was this tested? If I'm reading this correctly, this strips out\n> the entire CRLF pair, whereas the original code only stripped the CR\n> and left what followed it (typically LF) alone. Junio's suggestion was\n> to enhance this to be more careful and strip CR only when followed\n> immediately by LF (but to leave the LF intact). Therefore, this seems\n> like a regression.\n>\n>> + if (i != j)\n>> + buffer->buf[j] = buffer->buf[i];\n>> + j++;\n>> + }\n\nI think the \"negate the entire thing\" condition confuses the\nreaders. The negated condition is \"Do we still have enough bytes to\nsee if we are looking at CRLF? Are we at CR? Is the one byte\nbeyond what we have a LF? Do all of these three conditions hold\ntrue?\" If not, i.e. for all the bytes on the line except for that\nnarrow \"we are at CR of a CRLF sequence\" case, the body of the loop\nmakes a literal copy and advances the destination pointer 'j'. The\nonly thing that is skipped is CR that comes at the beginning of a\nCRLF sequence.\n\nSo I think the loop does what it wants to do.\n\nIn any case, I think this should be a two patch series---one with\nthe code as-is with a better explanation, but without \"make sure\nonly CR in CRLF and no other CR are stripped\" improvement, and the\nother as a follow-up to it to make the improvement.\n\nThanks.\n"},{"id":"332404","messageId":"CAPig+cQqLJsBTSMSPCVJ+_2JxkO=i4v681rK=THoN7xQ+jB91g@mail.gmail.com","threadId":"47178","inReplyTo":"xmqqk1yvaq2e.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] gpg-interface: strip CR chars for Windows gpg2","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-11-13T10:24:14Z","receivedAt":"2017-11-13T10:24:21Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Nov 12, 2017 at 9:08 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>>> + size_t i, j;\n>>> + for (i = j = bottom; i < buffer->len; i++)\n>>> + if (!(i < buffer->len - 1 &&\n>>> + buffer->buf[i] == '\\r' &&\n>>> + buffer->buf[i + 1] == '\\n')) {\n>>\n>> Hmm, was this tested? If I'm reading this correctly, this strips out\n>> the entire CRLF pair, whereas the original code only stripped the CR\n>> and left what followed it (typically LF) alone. Junio's suggestion was\n>> to enhance this to be more careful and strip CR only when followed\n>> immediately by LF (but to leave the LF intact). Therefore, this seems\n>> like a regression.\n>>\n>>> + if (i != j)\n>>> + buffer->buf[j] = buffer->buf[i];\n>>> + j++;\n>>> + }\n>\n> I think the \"negate the entire thing\" condition confuses the\n> readers. The negated condition is \"Do we still have enough bytes to\n> see if we are looking at CRLF? Are we at CR? Is the one byte\n> beyond what we have a LF? Do all of these three conditions hold\n> true?\" If not, i.e. for all the bytes on the line except for that\n> narrow \"we are at CR of a CRLF sequence\" case, the body of the loop\n> makes a literal copy and advances the destination pointer 'j'. The\n> only thing that is skipped is CR that comes at the beginning of a\n> CRLF sequence.\n>\n> So I think the loop does what it wants to do.\n\nYou're right. I misunderstood it. Rephrasing the logic in a simpler\nfashion would help.\n\n> In any case, I think this should be a two patch series---one with\n> the code as-is with a better explanation, but without \"make sure\n> only CR in CRLF and no other CR are stripped\" improvement, and the\n> other as a follow-up to it to make the improvement.\n\nAgreed, a 2-patch series makes sense.\n"}]}