gpg-interface: strip CR chars for Windows gpg2
PATCH v2 1/1 gpg-interface: strip CR chars for Windows gpg2
This fixes the issue with newlines being \r\n and not being displayed correctly when using gpg2 for Windows, as reported at
https://github.com/git-for-windows/git/issues/1249
Issues with non-ASCII characters remain for further investigation.
---
Addressed comments by Junio C Hamano (check for following \n, and updated the commit description).
gpg-interface.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)
gpg-interface.c +18 -9
@@ -145,6 +145,20 @@ const char *get_signing_key(void)return git_committer_info (IDENT_STRICT |IDENT_NO_DATE ); } +/* Strip CR from the CRLF line endings, in case we are on Windows. */ +static void strip_cr (struct strbuf *buffer,size_t bottom) { +size_t i, j; +for (i = j = bottom; i < buffer->len; i++) +if (!(i < buffer->len -1 && + buffer->buf[i] =='\r' && + buffer->buf[i +1 ] =='\n' )) { +if (i != j) + buffer->buf[j] = buffer->buf[i]; + j++; + } +strbuf_setlen (buffer, j); +} +/* * Create a detached signature for the contents of "buffer" and append * it after "signature"; "buffer" and "signature" can be the same
@@ -155,7 +169,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig {struct child_process gpg =CHILD_PROCESS_INIT ;int ret; -size_t i, j, bottom; +size_t bottom;struct strbuf gpg_status =STRBUF_INIT ; argv_array_pushl(&gpg.args,
@@ -180,14 +194,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sigif (ret)return error (_ ("gpg failed to sign the data" )); -/* Strip CR from the line endings, in case we are on Windows. */ -for (i = j = bottom; i < signature->len; i++) -if (signature->buf[i] !='\r' ) { -if (i != j) - signature->buf[j] = signature->buf[i]; - j++; - } -strbuf_setlen (signature, j); +strip_cr (signature, bottom);return 0 ; }
@@ -230,6 +237,8 @@ int verify_signed_buffer(const char *payload, size_t payload_size,sigchain_push (SIGPIPE ,SIG_IGN ); ret =pipe_command (&gpg, payload, payload_size, gpg_status,0 , gpg_output,0 ); +strip_cr (gpg_status,0 ); +strip_cr (gpg_output,0 );sigchain_pop (SIGPIPE );delete_tempfile (&temp);
Signature · 1 line
2.14.2.windows.3
Thanks for the re-roll...
Quoted 3 lines · Jerzy Kozera
This fixes the issue with newlines being \r\n and not being displayed correctly when using gpg2 for Windows, as reported at
https://github.com/git-for-windows/git/issues/1249
It's still not clear from this description what "not being displayed correctly" means. Ideally, the commit message should stand on its own, explaining exactly what problem the patch is solving, without the reader having to chase URLs to pages (which might disappear). If you could summarize the problem and solution in your own words in such a way that your description itself conveys enough information for someone not familiar with that problem report to understand the problem, then that would likely make a good commit message.
More below...
gpg-interface.c const char *get_signing_key(void) · line 146
Quoted 8 lines
Issues with non-ASCII characters remain for further investigation.
Signed-off-by Jerzy Kozera <[email protected]>---
gpg-interface.c +2
-0@@ -145,6 +145,20 @@ const char *get_signing_key(void) +/* Strip CR from the CRLF line endings, in case we are on Windows. */ +static void strip_cr (struct strbuf *buffer,size_t bottom) {
It's not at all clear what 'bottom' means. In the original, when the code was inline, the surrounding context would likely have given a good clue to the meaning of 'bottom', but here stand-alone, it conveys little or nothing. Perhaps a better name for this argument would be 'start_at' or 'from' or something.
gpg-interface.c const char *get_signing_key(void) · line 154
Quoted 5 lines
gpg-interface.c +5
-0+size_t i, j; +for (i = j = bottom; i < buffer->len; i++) +if (!(i < buffer->len -1 && + buffer->buf[i] =='\r' && + buffer->buf[i +1 ] =='\n' )) {
Hmm, was this tested? If I'm reading this correctly, this strips out the entire CRLF pair, whereas the original code only stripped the CR and left what followed it (typically LF) alone. Junio's suggestion was to enhance this to be more careful and strip CR only when followed immediately by LF (but to leave the LF intact). Therefore, this seems like a regression.
Quoted 46 lines
gpg-interface.c +11
-10+if (i != j) + buffer->buf[j] = buffer->buf[i]; + j++; + } +strbuf_setlen (buffer, j); +} +/* * Create a detached signature for the contents of "buffer" and append * it after "signature"; "buffer" and "signature" can be the same @@ -155,7 +169,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sig {struct child_process gpg =CHILD_PROCESS_INIT ;int ret; -size_t i, j, bottom; +size_t bottom;struct strbuf gpg_status =STRBUF_INIT ; argv_array_pushl(&gpg.args,@@ -180,14 +194,7 @@ int sign_buffer(struct strbuf *buffer, struct strbuf *signature, const char *sigif (ret)return error (_ ("gpg failed to sign the data" )); -/* Strip CR from the line endings, in case we are on Windows. */ -for (i = j = bottom; i < signature->len; i++) -if (signature->buf[i] !='\r' ) { -if (i != j) - signature->buf[j] = signature->buf[i]; - j++; - } -strbuf_setlen (signature, j); +strip_cr (signature, bottom);return 0 ; }@@ -230,6 +237,8 @@ int verify_signed_buffer(const char *payload, size_t payload_size,sigchain_push (SIGPIPE ,SIG_IGN ); ret =pipe_command (&gpg, payload, payload_size, gpg_status,0 , gpg_output,0 ); +strip_cr (gpg_status,0 ); +strip_cr (gpg_output,0 );sigchain_pop (SIGPIPE );delete_tempfile (&temp); --2.14.2.windows.3
Quoted 7 lines · Eric Sunshine
Thanks for the re-roll...
Quoted 3 lines · Jerzy Kozera
This fixes the issue with newlines being \r\n and not being displayed correctly when using gpg2 for Windows, as reported at
https://github.com/git-for-windows/git/issues/1249
Quoted 8 lines
It's still not clear from this description what "not being displayed correctly" means. Ideally, the commit message should stand on its own, explaining exactly what problem the patch is solving, without the reader having to chase URLs to pages (which might disappear). If you could summarize the problem and solution in your own words in such a way that your description itself conveys enough information for someone not familiar with that problem report to understand the problem, then that would likely make a good commit message.
Thanks. I was wondering if I am the only one who does not understand what the revised wording wants to say.
Quoted 9 lines
gpg-interface.c const char *get_signing_key(void) · line 146
Quoted 3 lines
gpg-interface.c +2
-0@@ -145,6 +145,20 @@ const char *get_signing_key(void) +/* Strip CR from the CRLF line endings, in case we are on Windows. */ +static void strip_cr (struct strbuf *buffer,size_t bottom) {It's not at all clear what 'bottom' means. In the original, when the code was inline, the surrounding context would likely have given a good clue to the meaning of 'bottom', but here stand-alone, it conveys little or nothing. Perhaps a better name for this argument would be '
start_at' or 'from' or something.
I personally do not mind 'bottom' (especially when it appears in contrast to 'top') too much, but start_at would be much clearer.
Quoted 6 lines
gpg-interface.c const char *get_signing_key(void) · line 154
Quoted 5 lines
gpg-interface.c +5
-0+size_t i, j; +for (i = j = bottom; i < buffer->len; i++) +if (!(i < buffer->len -1 && + buffer->buf[i] =='\r' && + buffer->buf[i +1 ] =='\n' )) {
Quoted 11 lines
Hmm, was this tested? If I'm reading this correctly, this strips out the entire CRLF pair, whereas the original code only stripped the CR and left what followed it (typically LF) alone. Junio's suggestion was to enhance this to be more careful and strip CR only when followed immediately by LF (but to leave the LF intact). Therefore, this seems like a regression.
Quoted 4 lines
gpg-interface.c +4
-0+if (i != j) + buffer->buf[j] = buffer->buf[i]; + j++; + }
I think the "negate the entire thing" condition confuses the readers. The negated condition is "Do we still have enough bytes to see if we are looking at CRLF? Are we at CR? Is the one byte beyond what we have a LF? Do all of these three conditions hold true?" If not, i.e. for all the bytes on the line except for that narrow "we are at CR of a CRLF sequence" case, the body of the loop makes a literal copy and advances the destination pointer 'j'. The only thing that is skipped is CR that comes at the beginning of a CRLF sequence.
So I think the loop does what it wants to do.
In any case, I think this should be a two patch series---one with the code as-is with a better explanation, but without "make sure only CR in CRLF and no other CR are stripped" improvement, and the other as a follow-up to it to make the improvement.
Thanks.
Quoted 29 lines · Junio C Hamano
Quoted 6 lines · Eric Sunshine
gpg-interface.c const char *get_signing_key(void) · line 154
Quoted 5 lines
gpg-interface.c +5
-0+size_t i, j; +for (i = j = bottom; i < buffer->len; i++) +if (!(i < buffer->len -1 && + buffer->buf[i] =='\r' && + buffer->buf[i +1 ] =='\n' )) {Quoted 11 lines
Hmm, was this tested? If I'm reading this correctly, this strips out the entire CRLF pair, whereas the original code only stripped the CR and left what followed it (typically LF) alone. Junio's suggestion was to enhance this to be more careful and strip CR only when followed immediately by LF (but to leave the LF intact). Therefore, this seems like a regression.
Quoted 4 lines
gpg-interface.c +4
-0+if (i != j) + buffer->buf[j] = buffer->buf[i]; + j++; + }I think the "negate the entire thing" condition confuses the readers. The negated condition is "Do we still have enough bytes to see if we are looking at CRLF? Are we at CR? Is the one byte beyond what we have a LF? Do all of these three conditions hold true?" If not, i.e. for all the bytes on the line except for that narrow "we are at CR of a CRLF sequence" case, the body of the loop makes a literal copy and advances the destination pointer 'j'. The only thing that is skipped is CR that comes at the beginning of a CRLF sequence.
Quoted 1 line
So I think the loop does what it wants to do.
You're right. I misunderstood it. Rephrasing the logic in a simpler fashion would help.
Quoted 4 lines
In any case, I think this should be a two patch series---one with the code as-is with a better explanation, but without "make sure only CR in CRLF and no other CR are stripped" improvement, and the other as a follow-up to it to make the improvement.
Agreed, a 2-patch series makes sense.
Quoted 20 lines · Jerzy Kozera
This fixes the issue with newlines being \r\n and not being displayed correctly when using gpg2 for Windows, as reported at
https://github.com/git-for-windows/git/issues/1249Issues with non-ASCII characters remain for further investigation.
Signed-off-by Jerzy Kozera <[email protected]>---
Addressed comments by Junio C Hamano (check for following \n, and updated the commit description).
gpg-interface.c | 27 ++++++++++++++++++--------- 1 file changed, 18 insertions(+), 9 deletions(-)gpg-interface.c +0
-0
gpg-interface.c const char *get_signing_key(void) · line 150
Quoted 6 lines
gpg-interface.c +3
-0@@ -145,6 +145,20 @@ const char *get_signing_key(void)return git_committer_info (IDENT_STRICT |IDENT_NO_DATE ); } +/* Strip CR from the CRLF line endings, in case we are on Windows. */ +static void strip_cr (struct strbuf *buffer,size_t bottom) { +size_t i, j;
It is not wrong to say "Strip CR from the CRLF". In Git we often talk about "convert CRLF into LF",
The comment somewhat different to the function name. The function namd and the name of the parameters can be more in in line with existing strbuf functions:
(And the opening '{' should go into it's own line:
static void convert_crlf_to_lf(struct strbuf *sb, size_t len)
{
size_t i, j;An even more generic approach (could be done in a seperate commit) would be to move the whole function into strbuf.c/strbuf.h, and it may be called like this.
void strbuf_crlf_to_lf(struct strbuf *sb, size_t len)
{
/* I would even avoid "i" and "j", and use src and dst or so) */ size_t src_pos, dst_idx;
}
Thanks for working on this.
[]