{"thread":{"id":"52278","subject":"[PATCH 0/1] Limit search for primary key fingerprint","startedAt":"2019-11-16T18:07:05Z","lastAt":"2019-11-23T00:22:26Z","messageCount":21,"participants":["Hans Jerry Illikainen","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"386374","messageId":"20191116180655.10988-1-hji@dyntopia.com","threadId":"52278","inReplyTo":null,"subject":"[PATCH 0/1] Limit search for primary key fingerprint","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-16T18:06:54Z","receivedAt":"2019-11-16T18:07:05Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"As part of implementing signature verification for git clone, I decided\nto refactor/unify the code for commit and merge verification to make it\nreusable during clones.\n\nThis lead me to discover that git requires merge signatures to be\ntrusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike\nthe behavior of verify-tag and verify-commit.\n\nSo, I figured that I'd make the minimum trust level configurable to make\nthe behavior of merge/commit/tag consistent.  And while doing so, I\nnoticed that parse_gpg_output() in gpg-interface.c assumes that the\nVALIDSIG status line has a field with a fingerprint for the primary key;\nbut that is only the case for OpenPGP signatures [1].\n\nThe consequence of that assumption is that the subsequent status line is\ninterpreted as the primary fingerprint for X509 signatures.  I'm not\nsure if the order is hardcoded in GnuPG, but in my testing the TRUST_\nstatus line always came after VALIDSIG -- and that breaks the config\noption to set a minimum trust level (not part of this patch):\n\n,----\n| $ git log -n1 --format=\"primary key: %GP\" signed-x509\n| gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65\n| gpgsm: Good signature from \"/CN=C O Mitter/O=Example/SN=C O/GN=Mitter\"\n| gpgsm:                 aka \"committer@example.com\"\n| primary key: TRUST_FULLY 0 shell\n`----\n\n[1]: https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS\n\n\nHans Jerry Illikainen (1):\n  gpg-interface: limit search for primary key fingerprint\n\n gpg-interface.c | 20 +++++++++++++++-----\n t/t4202-log.sh  |  6 ++++++\n 2 files changed, 21 insertions(+), 5 deletions(-)\n\n--\n2.24.0.156.g69483321b9.dirty\n"},{"id":"386375","messageId":"20191116180655.10988-2-hji@dyntopia.com","threadId":"52278","inReplyTo":"20191116180655.10988-1-hji@dyntopia.com","subject":"[PATCH 1/1] gpg-interface: limit search for primary key fingerprint","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-16T18:06:55Z","receivedAt":"2019-11-16T18:09:36Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"The VALIDSIG status line from GnuPG with --status-fd has a field that\nspecifies the fingerprint of the primary key that made the signature.\nHowever, that field is only available for OpenPGP signatures; not for\nCMS/X.509.\n\nAn unbounded search for a non-existent primary key fingerprint for X509\nsignatures results in the following status line being interpreted as the\nfingerprint.\n\nSigned-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n---\n gpg-interface.c | 20 +++++++++++++++-----\n t/t4202-log.sh  |  6 ++++++\n 2 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex d60115ca40..01c7ef42d4 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -148,21 +148,31 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\t\t}\n \t\t\t\t/* Do we have fingerprint? */\n \t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {\n+\t\t\t\t\tconst char *limit;\n+\n \t\t\t\t\tnext = strchrnul(line, ' ');\n \t\t\t\t\tfree(sigc->fingerprint);\n \t\t\t\t\tsigc->fingerprint = xmemdupz(line, next - line);\n \n-\t\t\t\t\t/* Skip interim fields */\n+\t\t\t\t\t/* Skip interim fields.  The search is\n+\t\t\t\t\t * limited to the same line since only\n+\t\t\t\t\t * OpenPGP signatures has a field with\n+\t\t\t\t\t * the primary fingerprint. */\n+\t\t\t\t\tlimit = strchrnul(line, '\\n');\n \t\t\t\t\tfor (j = 9; j > 0; j--) {\n-\t\t\t\t\t\tif (!*next)\n+\t\t\t\t\t\tif (!*next || next >= limit)\n \t\t\t\t\t\t\tbreak;\n \t\t\t\t\t\tline = next + 1;\n \t\t\t\t\t\tnext = strchrnul(line, ' ');\n \t\t\t\t\t}\n \n-\t\t\t\t\tnext = strchrnul(line, '\\n');\n-\t\t\t\t\tfree(sigc->primary_key_fingerprint);\n-\t\t\t\t\tsigc->primary_key_fingerprint = xmemdupz(line, next - line);\n+\t\t\t\t\tif (j == 0) {\n+\t\t\t\t\t\tnext = strchrnul(line, '\\n');\n+\t\t\t\t\t\tfree(sigc->primary_key_fingerprint);\n+\t\t\t\t\t\tsigc->primary_key_fingerprint =\n+\t\t\t\t\t\t\txmemdupz(line,\n+\t\t\t\t\t\t\t\t next - line);\n+\t\t\t\t\t}\n \t\t\t\t}\n \n \t\t\t\tbreak;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex e803ba402e..5d893b3137 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1580,6 +1580,12 @@ test_expect_success GPGSM 'setup signed branch x509' '\n \tgit commit -S -m signed_commit\n '\n \n+test_expect_success GPGSM 'log x509 fingerprint' '\n+\techo \"F8BF62E0693D0694816377099909C779FA23FD65 | \" >expect &&\n+\tgit log -n1 --format=\"%GF | %GP\" signed-x509 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success GPG 'log --graph --show-signature' '\n \tgit log --graph --show-signature -n1 signed >actual &&\n \tgrep \"^| gpg: Signature made\" actual &&\n-- \n2.24.0.156.g69483321b9.dirty\n\n"},{"id":"386377","messageId":"20191116194945.GA244853@google.com","threadId":"52278","inReplyTo":"20191116180655.10988-1-hji@dyntopia.com","subject":"Re: [PATCH 0/1] Limit search for primary key fingerprint","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2019-11-16T19:49:45Z","receivedAt":"2019-11-16T19:49:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nHans Jerry Illikainen wrote:\n\n> As part of implementing signature verification for git clone, I decided\n> to refactor/unify the code for commit and merge verification to make it\n> reusable during clones.\n\nThanks for writing this.\n\nMost of the text in this cover letter would be useful to have in the\ncommit message.  From the commit message alone, I could see that you\nwere fixing a bug, but I could not see the motivation or workflow it\nis part of.  If I were to later discover an issue triggered by this\ncommit, I wouldn't have enough information to weigh tradeoffs about\nthe right way to address such an issue.\n\nThanks and hope that helps,\nJonathan\n\n> This lead me to discover that git requires merge signatures to be\n> trusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike\n> the behavior of verify-tag and verify-commit.\n>\n> So, I figured that I'd make the minimum trust level configurable to make\n> the behavior of merge/commit/tag consistent.  And while doing so, I\n> noticed that parse_gpg_output() in gpg-interface.c assumes that the\n> VALIDSIG status line has a field with a fingerprint for the primary key;\n> but that is only the case for OpenPGP signatures [1].\n>\n> The consequence of that assumption is that the subsequent status line is\n> interpreted as the primary fingerprint for X509 signatures.  I'm not\n> sure if the order is hardcoded in GnuPG, but in my testing the TRUST_\n> status line always came after VALIDSIG -- and that breaks the config\n> option to set a minimum trust level (not part of this patch):\n>\n> ,----\n> | $ git log -n1 --format=\"primary key: %GP\" signed-x509\n> | gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65\n> | gpgsm: Good signature from \"/CN=C O Mitter/O=Example/SN=C O/GN=Mitter\"\n> | gpgsm:                 aka \"committer@example.com\"\n> | primary key: TRUST_FULLY 0 shell\n> `----\n>\n> [1]: https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS\n"},{"id":"386382","messageId":"20191116215850.3919-1-hji@dyntopia.com","threadId":"52278","inReplyTo":"20191116194945.GA244853@google.com","subject":"[PATCH v2 0/1] Limit search for primary key fingerprint","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-16T21:58:49Z","receivedAt":"2019-11-16T21:58:57Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"As part of implementing signature verification for git clone, I decided\nto refactor/unify the code for commit and merge verification to make it\nreusable during clones.\n\nThis lead me to discover that git requires merge signatures to be\ntrusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike\nthe behavior of verify-tag and verify-commit.\n\nSo, I figured that I'd make the minimum trust level configurable to make\nthe behavior of merge/commit/tag consistent.  And while doing so, I\nnoticed that parse_gpg_output() in gpg-interface.c assumes that the\nVALIDSIG status line has a field with a fingerprint for the primary key;\nbut that is only the case for OpenPGP signatures [1].\n\nThe consequence of that assumption is that the subsequent status line is\ninterpreted as the primary fingerprint for X509 signatures.  I'm not\nsure if the order is hardcoded in GnuPG, but in my testing the TRUST_\nstatus line always came after VALIDSIG -- and that breaks the config\noption to set a minimum trust level (not part of this patch):\n\n,----\n| $ git log -n1 --format=\"primary key: %GP\" signed-x509\n| gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65\n| gpgsm: Good signature from \"/CN=C O Mitter/O=Example/SN=C O/GN=Mitter\"\n| gpgsm:                 aka \"committer@example.com\"\n| primary key: TRUST_FULLY 0 shell\n`----\n\n[1]: https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS\n\n\nHans Jerry Illikainen (1):\n  gpg-interface: limit search for primary key fingerprint\n\n gpg-interface.c | 20 +++++++++++++++-----\n t/t4202-log.sh  |  6 ++++++\n 2 files changed, 21 insertions(+), 5 deletions(-)\n\n--\n2.24.GIT\n"},{"id":"386383","messageId":"20191116215850.3919-2-hji@dyntopia.com","threadId":"52278","inReplyTo":"20191116215850.3919-1-hji@dyntopia.com","subject":"[PATCH v2 1/1] gpg-interface: limit search for primary key fingerprint","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-16T21:58:50Z","receivedAt":"2019-11-16T21:58:58Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"As part of implementing signature verification for git clone, I decided\nto refactor/unify the code for commit and merge verification to make it\nreusable during clones.\n\nThis lead me to discover that git requires merge signatures to be\ntrusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike\nthe behavior of verify-tag and verify-commit.\n\nSo, I figured that I'd make the minimum trust level configurable to make\nthe behavior of merge/commit/tag consistent.  And while doing so, I\nnoticed that parse_gpg_output() in gpg-interface.c assumes that the\nVALIDSIG status line has a field with a fingerprint for the primary key;\nbut that is only the case for OpenPGP signatures.\n\nThe consequence of that assumption is that the subsequent status line is\ninterpreted as the primary fingerprint for X509 signatures.  I'm not\nsure if the order is hardcoded in GnuPG, but in my testing the TRUST_\nstatus line always came after VALIDSIG -- and that breaks the\npossibility of a configuration option to set a minimum trust level since\nthe TRUST_ line is consumed by VALIDSIG.\n\nSigned-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n---\n gpg-interface.c | 20 +++++++++++++++-----\n t/t4202-log.sh  |  6 ++++++\n 2 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex d60115ca40..01c7ef42d4 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -148,21 +148,31 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\t\t}\n \t\t\t\t/* Do we have fingerprint? */\n \t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {\n+\t\t\t\t\tconst char *limit;\n+\n \t\t\t\t\tnext = strchrnul(line, ' ');\n \t\t\t\t\tfree(sigc->fingerprint);\n \t\t\t\t\tsigc->fingerprint = xmemdupz(line, next - line);\n \n-\t\t\t\t\t/* Skip interim fields */\n+\t\t\t\t\t/* Skip interim fields.  The search is\n+\t\t\t\t\t * limited to the same line since only\n+\t\t\t\t\t * OpenPGP signatures has a field with\n+\t\t\t\t\t * the primary fingerprint. */\n+\t\t\t\t\tlimit = strchrnul(line, '\\n');\n \t\t\t\t\tfor (j = 9; j > 0; j--) {\n-\t\t\t\t\t\tif (!*next)\n+\t\t\t\t\t\tif (!*next || next >= limit)\n \t\t\t\t\t\t\tbreak;\n \t\t\t\t\t\tline = next + 1;\n \t\t\t\t\t\tnext = strchrnul(line, ' ');\n \t\t\t\t\t}\n \n-\t\t\t\t\tnext = strchrnul(line, '\\n');\n-\t\t\t\t\tfree(sigc->primary_key_fingerprint);\n-\t\t\t\t\tsigc->primary_key_fingerprint = xmemdupz(line, next - line);\n+\t\t\t\t\tif (j == 0) {\n+\t\t\t\t\t\tnext = strchrnul(line, '\\n');\n+\t\t\t\t\t\tfree(sigc->primary_key_fingerprint);\n+\t\t\t\t\t\tsigc->primary_key_fingerprint =\n+\t\t\t\t\t\t\txmemdupz(line,\n+\t\t\t\t\t\t\t\t next - line);\n+\t\t\t\t\t}\n \t\t\t\t}\n \n \t\t\t\tbreak;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex e803ba402e..5d893b3137 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1580,6 +1580,12 @@ test_expect_success GPGSM 'setup signed branch x509' '\n \tgit commit -S -m signed_commit\n '\n \n+test_expect_success GPGSM 'log x509 fingerprint' '\n+\techo \"F8BF62E0693D0694816377099909C779FA23FD65 | \" >expect &&\n+\tgit log -n1 --format=\"%GF | %GP\" signed-x509 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success GPG 'log --graph --show-signature' '\n \tgit log --graph --show-signature -n1 signed >actual &&\n \tgrep \"^| gpg: Signature made\" actual &&\n-- \n2.24.GIT\n\n"},{"id":"386437","messageId":"xmqqzhgt23nr.fsf@gitster-ct.c.googlers.com","threadId":"52278","inReplyTo":"20191116194945.GA244853@google.com","subject":"Re: [PATCH 0/1] Limit search for primary key fingerprint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-18T04:45:44Z","receivedAt":"2019-11-18T04:46:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Hans Jerry Illikainen wrote:\n>\n>> As part of implementing signature verification for git clone, I decided\n>> to refactor/unify the code for commit and merge verification to make it\n>> reusable during clones.\n>\n> Thanks for writing this.\n>\n> Most of the text in this cover letter would be useful to have in the\n> commit message.  From the commit message alone, I could see that you\n> were fixing a bug, but I could not see the motivation or workflow it\n> is part of.  If I were to later discover an issue triggered by this\n> commit, I wouldn't have enough information to weigh tradeoffs about\n> the right way to address such an issue.\n\nAfter reading the proposed log message of [PATCH v1 1/1], I have to\ndisagree.  It does not matter if we will later see new code in the\nclone codepath that would use the gpg-interface API.  Whether it\nhappens or not, this change to look for the key fingerprint only on\nthe same line is something we should consider independently.\n\nOn the other hand, why the author of this change thought that it may\nbe necessary thing to do is an excellent material to tell the story\nbehind the patch in the cover letter.\n\nThanks.\n"},{"id":"386438","messageId":"xmqqtv712145.fsf@gitster-ct.c.googlers.com","threadId":"52278","inReplyTo":"20191116180655.10988-2-hji@dyntopia.com","subject":"Re: [PATCH 1/1] gpg-interface: limit search for primary key fingerprint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-18T05:40:42Z","receivedAt":"2019-11-18T05:40:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hans Jerry Illikainen <hji@dyntopia.com> writes:\n\n> The VALIDSIG status line from GnuPG with --status-fd has a field that\n> specifies the fingerprint of the primary key that made the signature.\n> However, that field is only available for OpenPGP signatures; not for\n> CMS/X.509.\n>\n> An unbounded search for a non-existent primary key fingerprint for X509\n> signatures results in the following status line being interpreted as the\n> fingerprint.\n\nThe above two paragraphs give us an excellent explanation of what\nhappens in today's code.  What needs to follow is a description of\nthe approach taken to solve the problem in such a way that helps\nreaders to agree or disagree with the patch.  It needs to convince\nthem of at least two things:\n\n - The change is necessary to avoid a wrong line from getting\n   mistaken as the fingerprint with CMS/X.509 sigs, and instead we\n   say \"there is no fingerprint\" on X.509 sig (or whatever happens\n   with this change---I cannot tell what ramifications lack of the\n   fingerprint has to the callers of this function offhand, or how\n   the lack of fingerprint is reported back to the callers, but the\n   proposed log message must talk about it).\n\n - The change safely keeps identifying the right fingerprint with\n   OpenPGP sigs because the primary fingerprint, if shown, must be\n   on the same line (or whatever reason why it makes it safe---I do\n   not offhand know if this is guaranteed how and by whom [*1*], but\n   you must have researched it before sending this patch, so please\n   write it down to help readers).\n\n>  \t\t\t\t/* Do we have fingerprint? */\n>  \t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {\n> +\t\t\t\t\tconst char *limit;\n> +\n\nI wonder if it is simpler to define it next to 'next'.  Yes, this\nnew variable is used only within this block, but it also gets used\nonly in conjunction with that other variable.\n\n>  \t\t\t\t\tnext = strchrnul(line, ' ');\n>  \t\t\t\t\tfree(sigc->fingerprint);\n>  \t\t\t\t\tsigc->fingerprint = xmemdupz(line, next - line);\n\nSo, we skipped \"VALIDSIG \" and grabbed the first field <fingerprint>\nin sigc->fingerprint.  Then we used to unconditionally skip 9 SP\nseparated fields.  But there may only be 8 fields on the line, which\nis why this patch is needed.\n\n> -\t\t\t\t\t/* Skip interim fields */\n> +\t\t\t\t\t/* Skip interim fields.  The search is\n> +\t\t\t\t\t * limited to the same line since only\n> +\t\t\t\t\t * OpenPGP signatures has a field with\n> +\t\t\t\t\t * the primary fingerprint. */\n\n\t/*\n\t * A multi-line comment of ours looks like this; the\n\t * slash-asterisk that begins it, and the asterisk-slash\n\t * that ends it, are on their own lines, without anything\n\t * else but the indentation.\n\t */\n\n> +\t\t\t\t\tlimit = strchrnul(line, '\\n');\n>  \t\t\t\t\tfor (j = 9; j > 0; j--) {\n> -\t\t\t\t\t\tif (!*next)\n> +\t\t\t\t\t\tif (!*next || next >= limit)\n>  \t\t\t\t\t\t\tbreak;\n\nThis makes sure that a premature exit (i.e. \"0 < j\") means we ran\nout of the fields.  \n\nI'd prefer to write it \"limit <= next\" to help visualizing the two\nvalues (on a single line, lower values come left, higher ones come\nright), by the way.  That is a minor point.\n\nA bigger question is, when this happens, what value do we want to\nleave in sigc->primary_key_fingerprint?  As we can see from the\noriginal code that makes sure the old value in the field will not\nleak by first free()ing, it seems that it is possible in this code\nthat the field may not be NULL, but we just saw that on _our_\nsignature verification system, the primary key is not available.\nShouldn't we be nulling it out, after free()ing possibly leftover\nvalue in the field?\n\n>  \t\t\t\t\t\tline = next + 1;\n>  \t\t\t\t\t\tnext = strchrnul(line, ' ');\n>  \t\t\t\t\t}\n>  \n> -\t\t\t\t\tnext = strchrnul(line, '\\n');\n> -\t\t\t\t\tfree(sigc->primary_key_fingerprint);\n> -\t\t\t\t\tsigc->primary_key_fingerprint = xmemdupz(line, next - line);\n> +\t\t\t\t\tif (j == 0) {\n> +\t\t\t\t\t\tnext = strchrnul(line, '\\n');\n> +\t\t\t\t\t\tfree(sigc->primary_key_fingerprint);\n> +\t\t\t\t\t\tsigc->primary_key_fingerprint =\n> +\t\t\t\t\t\t\txmemdupz(line,\n> +\t\t\t\t\t\t\t\t next - line);\n> +\t\t\t\t\t}\n\nAvoid such an unnatural line-wrapping that makes the result harder\nto read.  A short helper\n\n\tstatic void replace_cstring(const char **field,\n\t\t\t\t    const char *line, const char *next)\n\t{\n\t\tfree(*field);\n\t\tif (line && next)\n\t\t\t*field = xmemdupz(line, next - line);\n\t\telse\n\t\t\t*field = NULL;\n\t}\n\nmay have quite a lot of uses in this function, not only for this\nfield.\n\nThis is a tangent, but I think \"skip 9 fields\" loop by itself\ndeserves to become a small static helper function.\n\nWith such a helper, it would become\n\n\t\tif (!j) {\n\t\t\tnext = strchrnul(line, '\\n');\n\t\t\treplace_cstring(&sigc->primary_key_fingerprint, line, next);\n\t\t} else {\n\t\t\treplace_cstring(&sigc->primary_key_fingerprint,\tNULL, NULL);\n\t\t}\n\nor if you do not like the overlong lines (I don't), perhaps\n\n\t\tfield = &sigc->primary_key_fingerprint;\n\t\tif (!j)\n\t\t\treplace_cstring(field, line, strchrnul(line, '\\n'));\n\t\telse\n\t\t\treplace_cstring(field, NULL, NULL);\n\nNote that sigc->key, sigc->signer, sigc->fingerprint fields all\nshare the same pattern and will benefit from the use of such a\nhelper function.\n\nThanks.\n\n[Reference]\n\n*1* Perhaps this one?  https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=dea9d426351e043f872007696e841afb22fe9689;hb=591523ec94b6279b8b39a01501d78cf980de8722#l480\n\n"},{"id":"386779","messageId":"87d0dkbyxb.hji@dyntopia.com","threadId":"52278","inReplyTo":"xmqqtv712145.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] gpg-interface: limit search for primary key fingerprint","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-21T23:19:12Z","receivedAt":"2019-11-21T23:20:08Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"On Mon, Nov 18 2019, Junio C Hamano wrote:\n> I wonder if it is simpler to define it next to 'next'.  Yes, this\n> new variable is used only within this block, but it also gets used\n> only in conjunction with that other variable.\n\nDone in v3 (sorry for the messed up versioning going from v0 to v2!).\n\n> A bigger question is, when this happens, what value do we want to\n> leave in sigc->primary_key_fingerprint?  As we can see from the\n> original code that makes sure the old value in the field will not\n> leak by first free()ing, it seems that it is possible in this code\n> that the field may not be NULL, but we just saw that on _our_\n> signature verification system, the primary key is not available.\n> Shouldn't we be nulling it out, after free()ing possibly leftover\n> value in the field?\n\nI investigated the code paths to `primary_key_fingerprint` and deduced\nthat it's only ever touched when GPG_STATUS_FINGERPRINT is encountered\nand a primary fingerprint is extracted.  However, v3 will NULL it even\nwhen no primary fingerprint is found.\n\n>> +\t\t\t\t\t\t\txmemdupz(line,\n>> +\t\t\t\t\t\t\t\t next - line);\n>> +\t\t\t\t\t}\n>\n> Avoid such an unnatural line-wrapping that makes the result harder\n> to read.\n\nSorry about that!  I figured that some projects prefer to always trust\nin the code formatter; so I just left it be.  Now I know that human\ndecisions are allowed :)\n\n> A short helper\n>\n> \tstatic void replace_cstring(const char **field,\n> \t\t\t\t    const char *line, const char *next)\n> \t{\n> \t\tfree(*field);\n> \t\tif (line && next)\n> \t\t\t*field = xmemdupz(line, next - line);\n> \t\telse\n> \t\t\t*field = NULL;\n> \t}\n>\n> may have quite a lot of uses in this function, not only for this\n> field.\n\nImplemented.  I wasn't sure whether to do it in a separate commit or\nnot, but #git-devel suggested that I do; so that's what I did.\n"},{"id":"386781","messageId":"20191121234336.26300-1-hji@dyntopia.com","threadId":"52278","inReplyTo":"20191116215850.3919-1-hji@dyntopia.com","subject":"[PATCH v3 0/2] gpg-interface: fix search for primary key fingerprint","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-21T23:43:34Z","receivedAt":"2019-11-21T23:43:44Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"As part of the process of implementing signature verification for git\nclone, I decided to refactor/unify the code for commit and merge\nverification to make it reusable during clones.\n\nThis lead me to discover that git requires merge signatures to be\ntrusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike\nthe behavior of verify-tag and verify-commit.\n\nSo, I figured that I'd make the minimum trust level configurable to make\nthe behavior of merge/commit/tag consistent.  And while doing so, I\nnoticed that parse_gpg_output() in gpg-interface.c assumes that the\nVALIDSIG status line has a field with a fingerprint for the primary key;\nbut that is only the case for OpenPGP signatures [1].\n\nThe consequence of that assumption is that the subsequent status line is\ninterpreted as the primary fingerprint for X509 signatures.  I'm not\nsure if the order is hardcoded in GnuPG, but in my testing the TRUST_\nstatus line always came after VALIDSIG -- and that breaks the config\noption to set a minimum trust level (not part of this patch):\n\n,----\n| $ git log -n1 --format=\"primary key: %GP\" signed-x509\n| gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65\n| gpgsm: Good signature from \"/CN=C O Mitter/O=Example/SN=C O/GN=Mitter\"\n| gpgsm:                 aka \"committer@example.com\"\n| primary key: TRUST_FULLY 0 shell\n`----\n\nAs per suggestion from Hamano, I also introduced a helper function,\nreplace_cstring().  I wasn't sure whether to add it in a separate commit\nor not, but the kind folks in #git-devel suggested I do.\n\n[1]: https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS\n\nHans Jerry Illikainen (2):\n  gpg-interface: refactor the free-and-xmemdupz pattern\n  gpg-interface: limit search for primary key fingerprint\n\n gpg-interface.c | 45 ++++++++++++++++++++++++++++++++-------------\n t/t4202-log.sh  | 20 ++++++++++++++++++++\n 2 files changed, 52 insertions(+), 13 deletions(-)\n\n--\n2.24.0.157.gba9f894af8\n"},{"id":"386782","messageId":"20191121234336.26300-2-hji@dyntopia.com","threadId":"52278","inReplyTo":"20191121234336.26300-1-hji@dyntopia.com","subject":"[PATCH v3 1/2] gpg-interface: refactor the free-and-xmemdupz pattern","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-21T23:43:35Z","receivedAt":"2019-11-21T23:43:50Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"This commit introduces a static replace_cstring() function.  This\nfunction simplifies the continuous pattern of free-and-xmemdupz() for\nGPG status line parsing.\n\nThe benefit of having it in a single helper function is that it helps\navoid the need for duplicate code that does the same thing.  It also\nhelps avoid potential memleaks if parsing of new status lines are\nintroduced in the future.\n\nSigned-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n---\n gpg-interface.c | 23 ++++++++++++++++-------\n 1 file changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex d60115ca40..b4c4443287 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -105,6 +105,17 @@ static struct {\n \t{ 0, \"VALIDSIG \", GPG_STATUS_FINGERPRINT },\n };\n \n+static void replace_cstring(const char **field, const char *line,\n+\t\t\t    const char *next)\n+{\n+\tfree(*field);\n+\n+\tif (line && next)\n+\t\t*field = xmemdupz(line, next - line);\n+\telse\n+\t\t*field = NULL;\n+}\n+\n static void parse_gpg_output(struct signature_check *sigc)\n {\n \tconst char *buf = sigc->gpg_status;\n@@ -136,21 +147,18 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\t\t/* Do we have key information? */\n \t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_KEYID) {\n \t\t\t\t\tnext = strchrnul(line, ' ');\n-\t\t\t\t\tfree(sigc->key);\n-\t\t\t\t\tsigc->key = xmemdupz(line, next - line);\n+\t\t\t\t\treplace_cstring(&sigc->key, line, next);\n \t\t\t\t\t/* Do we have signer information? */\n \t\t\t\t\tif (*next && (sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {\n \t\t\t\t\t\tline = next + 1;\n \t\t\t\t\t\tnext = strchrnul(line, '\\n');\n-\t\t\t\t\t\tfree(sigc->signer);\n-\t\t\t\t\t\tsigc->signer = xmemdupz(line, next - line);\n+\t\t\t\t\t\treplace_cstring(&sigc->signer, line, next);\n \t\t\t\t\t}\n \t\t\t\t}\n \t\t\t\t/* Do we have fingerprint? */\n \t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {\n \t\t\t\t\tnext = strchrnul(line, ' ');\n-\t\t\t\t\tfree(sigc->fingerprint);\n-\t\t\t\t\tsigc->fingerprint = xmemdupz(line, next - line);\n+\t\t\t\t\treplace_cstring(&sigc->fingerprint, line, next);\n \n \t\t\t\t\t/* Skip interim fields */\n \t\t\t\t\tfor (j = 9; j > 0; j--) {\n@@ -162,7 +170,8 @@ static void parse_gpg_output(struct signature_check *sigc)\n \n \t\t\t\t\tnext = strchrnul(line, '\\n');\n \t\t\t\t\tfree(sigc->primary_key_fingerprint);\n-\t\t\t\t\tsigc->primary_key_fingerprint = xmemdupz(line, next - line);\n+\t\t\t\t\treplace_cstring(&sigc->primary_key_fingerprint,\n+\t\t\t\t\t\t\tline, next);\n \t\t\t\t}\n \n \t\t\t\tbreak;\n-- \n2.24.0.157.gba9f894af8\n\n"},{"id":"386783","messageId":"20191121234336.26300-3-hji@dyntopia.com","threadId":"52278","inReplyTo":"20191121234336.26300-1-hji@dyntopia.com","subject":"[PATCH v3 2/2] gpg-interface: limit search for primary key fingerprint","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-21T23:43:36Z","receivedAt":"2019-11-21T23:43:51Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"The VALIDSIG status line from GnuPG with --status-fd is documented to\nhave 9 required and 1 optional fields [1].  The final, and optional,\nfield is used to specify the fingerprint of the primary key that made\nthe signature in case it was made by a subkey.  However, this field is\nonly available for OpenPGP signatures; not for CMS/X.509.\n\nThe current code assumes that the VALIDSIG status line always has 10\nfields.  Furthermore, the current code assumes that each field is\nseparated by a space (0x20) character.\n\nIf the VALIDSIG status line does not have the optional 10th field, the\ncurrent code will continue reading onto the next status line -- because\nonly 0x20 is considered a field separator, not 0xa.  And this is the\ncase for non-OpenPGP signatures [1].\n\nThe consequence is that a subsequent status line may be considered as\nthe \"primary key\" for signatures that does not have an actual primary\nkey.\n\nThe solution introduced by this commit is to add 0xa as a bound for the\nsearch for a primary key.  The search for the 10th VALIDSIG field is\naborted as soon as it sees a newline character.  This keeps the parser\nfrom interpreting subsequent lines as the primary key.\n\n[1] https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS#l483\n\nSigned-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n---\n gpg-interface.c | 24 +++++++++++++++++-------\n t/t4202-log.sh  | 20 ++++++++++++++++++++\n 2 files changed, 37 insertions(+), 7 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex b4c4443287..4269937b83 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -119,7 +119,8 @@ static void replace_cstring(const char **field, const char *line,\n static void parse_gpg_output(struct signature_check *sigc)\n {\n \tconst char *buf = sigc->gpg_status;\n-\tconst char *line, *next;\n+\tconst char *line, *next, *limit;\n+\tconst char **field;\n \tint i, j;\n \tint seen_exclusive_status = 0;\n \n@@ -160,18 +161,27 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\t\t\tnext = strchrnul(line, ' ');\n \t\t\t\t\treplace_cstring(&sigc->fingerprint, line, next);\n \n-\t\t\t\t\t/* Skip interim fields */\n+\t\t\t\t\t/*\n+\t\t\t\t\t * Skip interim fields.  The search is\n+\t\t\t\t\t * limited to the same line since only\n+\t\t\t\t\t * OpenPGP signatures has a field with\n+\t\t\t\t\t * the primary fingerprint.\n+\t\t\t\t\t */\n+\t\t\t\t\tlimit = strchrnul(line, '\\n');\n \t\t\t\t\tfor (j = 9; j > 0; j--) {\n-\t\t\t\t\t\tif (!*next)\n+\t\t\t\t\t\tif (!*next || limit <= next)\n \t\t\t\t\t\t\tbreak;\n \t\t\t\t\t\tline = next + 1;\n \t\t\t\t\t\tnext = strchrnul(line, ' ');\n \t\t\t\t\t}\n \n-\t\t\t\t\tnext = strchrnul(line, '\\n');\n-\t\t\t\t\tfree(sigc->primary_key_fingerprint);\n-\t\t\t\t\treplace_cstring(&sigc->primary_key_fingerprint,\n-\t\t\t\t\t\t\tline, next);\n+\t\t\t\t\tfield = &sigc->primary_key_fingerprint;\n+\t\t\t\t\tif (!j) {\n+\t\t\t\t\t\tnext = strchrnul(line, '\\n');\n+\t\t\t\t\t\treplace_cstring(field, line, next);\n+\t\t\t\t\t} else {\n+\t\t\t\t\t\treplace_cstring(field, NULL, NULL);\n+\t\t\t\t\t}\n \t\t\t\t}\n \n \t\t\t\tbreak;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex e803ba402e..17ec2401ec 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1570,6 +1570,14 @@ test_expect_success GPG 'setup signed branch' '\n \tgit commit -S -m signed_commit\n '\n \n+test_expect_success GPG 'setup signed branch with subkey' '\n+        test_when_finished \"git reset --hard && git checkout master\" &&\n+        git checkout -b signed-subkey master &&\n+        echo foo >foo &&\n+        git add foo &&\n+        git commit -SB7227189 -m signed_commit\n+'\n+\n test_expect_success GPGSM 'setup signed branch x509' '\n \ttest_when_finished \"git reset --hard && git checkout master\" &&\n \tgit checkout -b signed-x509 master &&\n@@ -1580,6 +1588,18 @@ test_expect_success GPGSM 'setup signed branch x509' '\n \tgit commit -S -m signed_commit\n '\n \n+test_expect_success GPGSM 'log x509 fingerprint' '\n+        echo \"F8BF62E0693D0694816377099909C779FA23FD65 | \" >expect &&\n+        git log -n1 --format=\"%GF | %GP\" signed-x509 >actual &&\n+        test_cmp expect actual\n+'\n+\n+test_expect_success GPGSM 'log OpenPGP fingerprint' '\n+        echo \"D4BE22311AD3131E5EDA29A461092E85B7227189\" > expect &&\n+        git log -n1 --format=\"%GP\" signed-subkey >actual &&\n+        test_cmp expect actual\n+'\n+\n test_expect_success GPG 'log --graph --show-signature' '\n \tgit log --graph --show-signature -n1 signed >actual &&\n \tgrep \"^| gpg: Signature made\" actual &&\n-- \n2.24.0.157.gba9f894af8\n\n"},{"id":"386790","messageId":"xmqqsgmg7hyf.fsf@gitster-ct.c.googlers.com","threadId":"52278","inReplyTo":"87d0dkbyxb.hji@dyntopia.com","subject":"Re: [PATCH 1/1] gpg-interface: limit search for primary key fingerprint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-22T02:39:20Z","receivedAt":"2019-11-22T02:39:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hans Jerry Illikainen <hji@dyntopia.com> writes:\n\n> On Mon, Nov 18 2019, Junio C Hamano wrote:\n> ...\n>> A short helper\n>>\n>> \tstatic void replace_cstring(const char **field,\n>> \t\t\t\t    const char *line, const char *next)\n>> \t{\n>> \t\tfree(*field);\n>> \t\tif (line && next)\n>> \t\t\t*field = xmemdupz(line, next - line);\n>> \t\telse\n>> \t\t\t*field = NULL;\n>> \t}\n>>\n>> may have quite a lot of uses in this function, not only for this\n>> field.\n>\n> Implemented.  I wasn't sure whether to do it in a separate commit or\n> not, but #git-devel suggested that I do; so that's what I did.\n\nIf such a refactoring helped the readability of _existing_ code that\ncan also use this helper, then I agree it is the right approach to\nmake that into a separate prelimimary commit.\n\nThanks for working on this.\n\n"},{"id":"386791","messageId":"xmqqo8x47hom.fsf@gitster-ct.c.googlers.com","threadId":"52278","inReplyTo":"20191121234336.26300-2-hji@dyntopia.com","subject":"Re: [PATCH v3 1/2] gpg-interface: refactor the free-and-xmemdupz pattern","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-22T02:45:13Z","receivedAt":"2019-11-22T02:45:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hans Jerry Illikainen <hji@dyntopia.com> writes:\n\n> This commit introduces a static replace_cstring() function.  This\n> function simplifies the continuous pattern of free-and-xmemdupz() for\n> GPG status line parsing.\n>\n> The benefit of having it in a single helper function is that it helps\n> avoid the need for duplicate code that does the same thing.  It also\n> helps avoid potential memleaks if parsing of new status lines are\n> introduced in the future.\n>\n> Signed-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n> ---\n>  gpg-interface.c | 23 ++++++++++++++++-------\n>  1 file changed, 16 insertions(+), 7 deletions(-)\n\nLooks quite cleanly done and clearly explained.  Thanks.\n\n> diff --git a/gpg-interface.c b/gpg-interface.c\n> index d60115ca40..b4c4443287 100644\n> --- a/gpg-interface.c\n> +++ b/gpg-interface.c\n> @@ -105,6 +105,17 @@ static struct {\n>  \t{ 0, \"VALIDSIG \", GPG_STATUS_FINGERPRINT },\n>  };\n>  \n> +static void replace_cstring(const char **field, const char *line,\n> +\t\t\t    const char *next)\n> +{\n> +\tfree(*field);\n> +\n> +\tif (line && next)\n> +\t\t*field = xmemdupz(line, next - line);\n> +\telse\n> +\t\t*field = NULL;\n> +}\n> +\n>  static void parse_gpg_output(struct signature_check *sigc)\n>  {\n>  \tconst char *buf = sigc->gpg_status;\n> @@ -136,21 +147,18 @@ static void parse_gpg_output(struct signature_check *sigc)\n>  \t\t\t\t/* Do we have key information? */\n>  \t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_KEYID) {\n>  \t\t\t\t\tnext = strchrnul(line, ' ');\n> -\t\t\t\t\tfree(sigc->key);\n> -\t\t\t\t\tsigc->key = xmemdupz(line, next - line);\n> +\t\t\t\t\treplace_cstring(&sigc->key, line, next);\n>  \t\t\t\t\t/* Do we have signer information? */\n>  \t\t\t\t\tif (*next && (sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {\n>  \t\t\t\t\t\tline = next + 1;\n>  \t\t\t\t\t\tnext = strchrnul(line, '\\n');\n> -\t\t\t\t\t\tfree(sigc->signer);\n> -\t\t\t\t\t\tsigc->signer = xmemdupz(line, next - line);\n> +\t\t\t\t\t\treplace_cstring(&sigc->signer, line, next);\n>  \t\t\t\t\t}\n>  \t\t\t\t}\n>  \t\t\t\t/* Do we have fingerprint? */\n>  \t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {\n>  \t\t\t\t\tnext = strchrnul(line, ' ');\n> -\t\t\t\t\tfree(sigc->fingerprint);\n> -\t\t\t\t\tsigc->fingerprint = xmemdupz(line, next - line);\n> +\t\t\t\t\treplace_cstring(&sigc->fingerprint, line, next);\n>  \n>  \t\t\t\t\t/* Skip interim fields */\n>  \t\t\t\t\tfor (j = 9; j > 0; j--) {\n> @@ -162,7 +170,8 @@ static void parse_gpg_output(struct signature_check *sigc)\n>  \n>  \t\t\t\t\tnext = strchrnul(line, '\\n');\n>  \t\t\t\t\tfree(sigc->primary_key_fingerprint);\n> -\t\t\t\t\tsigc->primary_key_fingerprint = xmemdupz(line, next - line);\n> +\t\t\t\t\treplace_cstring(&sigc->primary_key_fingerprint,\n> +\t\t\t\t\t\t\tline, next);\n>  \t\t\t\t}\n>  \n>  \t\t\t\tbreak;\n"},{"id":"386793","messageId":"xmqqimnc7ff2.fsf@gitster-ct.c.googlers.com","threadId":"52278","inReplyTo":"20191121234336.26300-3-hji@dyntopia.com","subject":"Re: [PATCH v3 2/2] gpg-interface: limit search for primary key fingerprint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-22T03:34:09Z","receivedAt":"2019-11-22T03:34:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hans Jerry Illikainen <hji@dyntopia.com> writes:\n\n> The VALIDSIG status line from GnuPG with --status-fd is documented to\n> have 9 required and 1 optional fields [1].  The final, and optional,\n> field is used to specify the fingerprint of the primary key that made\n> the signature in case it was made by a subkey.  However, this field is\n> only available for OpenPGP signatures; not for CMS/X.509.\n>\n> The current code assumes that the VALIDSIG status line always has 10\n> fields.  Furthermore, the current code assumes that each field is\n> separated by a space (0x20) character.\n\nThis make it sound as if the \"assumption\" of splitting at SP is a wrong\none, so I went back to the source.  The DETAILS document curiously\nforgets to say that these are \"usual space delimited arguments\" for the\nVALIDSIG, unlike it does for a few other entries in the \"General status\ncodes\" section.  But with the presense of \"usual space delimited\" in\nanother seciton with the lack of explicit delimiter definition in this\nsection, I would say it is safe to assume that the authors meant these\nfields to be separated by SPs. \n\nIn any case, I think this paragraph can safely disappear and would make\nthe overall clarity of the problem analysis even better.  We already\nsaid that assuming 10th field is wrong in the earlier paragraph, and we\nsay what happens when the is only 9 fields in the next paragraph.\n\n> If the VALIDSIG status line does not have the optional 10th field, the\n> current code will continue reading onto the next status line -- because\n> only 0x20 is considered a field separator, not 0xa.  And this is the\n> case for non-OpenPGP signatures [1].\n\nI actually think stresing on 0x20 vs 0x0a misleads readers in a wrong\ndirection.  If the code were written in such a way that both can be used\nas a field separator, we would still continue reading into the next\nline.  IOW, treating only SP as field delimiter is correct; what is\nwrong is we do not limit the search to the current/single line.\n\n\t... because the code does not pay attention to the end of the\n\tcurrent line when it looks for the end of the 10th (and\n\toptional) field that it incorrectly assumes to exist.\n\n> The consequence is that a subsequent status line may be considered as\n> the \"primary key\" for signatures that does not have an actual primary\n> key.\n\nOverall, all of the description in the above paragraphs are nicely\nanalysed and explained.  I wish all contributors' wrote their proposed\nlog messages clearly like you.\n\n> The solution introduced by this commit is to add 0xa as a bound for the\n> search for a primary key.  The search for the 10th VALIDSIG field is\n> aborted as soon as it sees a newline character.  This keeps the parser\n> from interpreting subsequent lines as the primary key.\n\nRather than \"... to add 0xa as ... primary key.\", I would probably write\nit as \"... to limit the search for the primary key on the single line.\"\nThe end of line being represented (internally as C string) with a byte\nwhose value is 0x0a or a newline character is an implementation detail.\n\nAlso we tend to describe the change we make as if the author is ordering\nthe codebase to \"become like so\" in imperative mood, so, perhaps\n\n\tLimit the search of these 9 or 10 fields to the single line\n\tto avoid this problem.  If the 10th field is missing, report\n\tthat there is no primary key fingerprint.\n\nwould be sufficient, as the last sentence, i.e. \"The consequence\nis...\", has already hinted what needs to be fixed clearly and quite\ndirectly.\n\n\n> [1] https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS#l483\n\nI actually wrote the URL with the exact revision in my earlier example\nbecause the URL written without is a moving target.  IOW, \"Find the tip\ncommit of whichever the default branch is, and then look at the path\ndoc/DETAILS in it and hope that line 483 will stay forever what we want\nto refer to\" was what I wanted to avoid.  Perhaps in addition to the\nabove URL (which can go stale), leave a textual reference so that\nreaders can notice that the link is stale and look for what you meant to\npoint them at?  Like so:\n\n\t[Reference]\n\n        *1* The description for VALIDSIG in the \"General status codes\"\n\tsection of GnuPG document that is at\n\n\thttps://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS#l483\n\n\tsays:\n\n        VALIDSIG <args>\n\n        The args are:\n\n        - <fingerprint_in_hex>\n        - <sig_creation_date>\n        - <sig-timestamp>\n        - <expire-timestamp>\n        - <sig-version>\n        - <reserved>\n        - <pubkey-algo>\n        - <hash-algo>\n        - <sig-class>\n        - [ <primary-key-fpr> ]\n\n        This status indicates that the signature is cryptographically\n        valid. ... PRIMARY-KEY-FPR is the fingerprint of the primary key\n        or identical to the first argument.  This is useful to get back\n        to the primary key without running gpg again for this purpose.\n\n        The primary-key-fpr parameter is used for OpenPGP and not\n        available for CMS signatures. ...\n\n> @@ -160,18 +161,27 @@ static void parse_gpg_output(struct signature_check *sigc)\n>  \t\t\t\t\tnext = strchrnul(line, ' ');\n>  \t\t\t\t\treplace_cstring(&sigc->fingerprint, line, next);\n>  \n> -\t\t\t\t\t/* Skip interim fields */\n> +\t\t\t\t\t/*\n> +\t\t\t\t\t * Skip interim fields.  The search is\n> +\t\t\t\t\t * limited to the same line since only\n> +\t\t\t\t\t * OpenPGP signatures has a field with\n> +\t\t\t\t\t * the primary fingerprint.\n> +\t\t\t\t\t */\n> +\t\t\t\t\tlimit = strchrnul(line, '\\n');\n>  \t\t\t\t\tfor (j = 9; j > 0; j--) {\n> -\t\t\t\t\t\tif (!*next)\n> +\t\t\t\t\t\tif (!*next || limit <= next)\n>  \t\t\t\t\t\t\tbreak;\n>  \t\t\t\t\t\tline = next + 1;\n>  \t\t\t\t\t\tnext = strchrnul(line, ' ');\n>  \t\t\t\t\t}\n\nNice.  We try not to go beyond the end of the current line, and\notherwise break out which leaves j non-zero.\n\n> -\t\t\t\t\tnext = strchrnul(line, '\\n');\n> -\t\t\t\t\tfree(sigc->primary_key_fingerprint);\n> -\t\t\t\t\treplace_cstring(&sigc->primary_key_fingerprint,\n> -\t\t\t\t\t\t\tline, next);\n\nAnd doing the above unconditionally was wrong, but ...\n\n> +\t\t\t\t\tfield = &sigc->primary_key_fingerprint;\n> +\t\t\t\t\tif (!j) {\n> +\t\t\t\t\t\tnext = strchrnul(line, '\\n');\n> +\t\t\t\t\t\treplace_cstring(field, line, next);\n> +\t\t\t\t\t} else {\n> +\t\t\t\t\t\treplace_cstring(field, NULL, NULL);\n> +\t\t\t\t\t}\n\n... we correct it by doing the replacing only when we did find the\n10th field.\n\nNicely done.\n\nBy the way, now you have another new variable \"field\" together with\n\"limit\" both of whose uses are limited to this narrower scope, I no\nlonger mind seeing declarations of them inside this scope, as opposed to\nmake them function-wide.  That's a fairly minor point.\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index e803ba402e..17ec2401ec 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -1570,6 +1570,14 @@ test_expect_success GPG 'setup signed branch' '\n>  \tgit commit -S -m signed_commit\n>  '\n>  \n> +test_expect_success GPG 'setup signed branch with subkey' '\n> +        test_when_finished \"git reset --hard && git checkout master\" &&\n> +        git checkout -b signed-subkey master &&\n> +        echo foo >foo &&\n> +        git add foo &&\n> +        git commit -SB7227189 -m signed_commit\n> +'\n> +\n\nThe new tests (not only this one) are indented with 8 SPs---will fix\nto use HT while queuing (no need to resend only to fix these).\n\n> +test_expect_success GPGSM 'log x509 fingerprint' '\n> +        echo \"F8BF62E0693D0694816377099909C779FA23FD65 | \" >expect &&\n> +        git log -n1 --format=\"%GF | %GP\" signed-x509 >actual &&\n> +        test_cmp expect actual\n> +'\n> +\n> +test_expect_success GPGSM 'log OpenPGP fingerprint' '\n> +        echo \"D4BE22311AD3131E5EDA29A461092E85B7227189\" > expect &&\n> +        git log -n1 --format=\"%GP\" signed-subkey >actual &&\n> +        test_cmp expect actual\n> +'\n\nThese two tests are really to the point.  Nice.\n\nThanks.\n"},{"id":"386794","messageId":"xmqqeey07exp.fsf@gitster-ct.c.googlers.com","threadId":"52278","inReplyTo":"xmqqsgmg7hyf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] gpg-interface: limit search for primary key fingerprint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-22T03:44:34Z","receivedAt":"2019-11-22T03:44:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Hans Jerry Illikainen <hji@dyntopia.com> writes:\n>\n>> On Mon, Nov 18 2019, Junio C Hamano wrote:\n>> ...\n>>> A short helper\n>>>\n>>> \tstatic void replace_cstring(const char **field,\n>>> \t\t\t\t    const char *line, const char *next)\n>>> \t{\n>>> \t\tfree(*field);\n>>> \t\tif (line && next)\n>>> \t\t\t*field = xmemdupz(line, next - line);\n>>> \t\telse\n>>> \t\t\t*field = NULL;\n>>> \t}\n>>>\n>>> may have quite a lot of uses in this function, not only for this\n>>> field.\n>>\n>> Implemented.  I wasn't sure whether to do it in a separate commit or\n>> not, but #git-devel suggested that I do; so that's what I did.\n>\n> If such a refactoring helped the readability of _existing_ code that\n> can also use this helper, then I agree it is the right approach to\n> make that into a separate prelimimary commit.\n\nI did not expect the \"how about doing it along this line...?\"\nsuggestion written in my MUA without even compilation testing would\nwork well, and acually I do not think the above would work without\nfurther tweaks on the types.  Wouldn't some of the fields this\nhelper works on be of type \"char *\"?\n\nPerhaps something like this squashed in...\n\n gpg-interface.c | 5 +++--\n 1 file changed, 3 insertions(+), 2 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 4269937b83..b481b0811b 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -105,7 +105,7 @@ static struct {\n \t{ 0, \"VALIDSIG \", GPG_STATUS_FINGERPRINT },\n };\n \n-static void replace_cstring(const char **field, const char *line,\n+static void replace_cstring(char **field, const char *line,\n \t\t\t    const char *next)\n {\n \tfree(*field);\n@@ -120,7 +120,6 @@ static void parse_gpg_output(struct signature_check *sigc)\n {\n \tconst char *buf = sigc->gpg_status;\n \tconst char *line, *next, *limit;\n-\tconst char **field;\n \tint i, j;\n \tint seen_exclusive_status = 0;\n \n@@ -158,6 +157,8 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\t\t}\n \t\t\t\t/* Do we have fingerprint? */\n \t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {\n+\t\t\t\t\tchar **field;\n+\n \t\t\t\t\tnext = strchrnul(line, ' ');\n \t\t\t\t\treplace_cstring(&sigc->fingerprint, line, next);\n \n"},{"id":"386865","messageId":"20191122202312.10120-1-hji@dyntopia.com","threadId":"52278","inReplyTo":"20191121234336.26300-1-hji@dyntopia.com","subject":"[PATCH v4 0/2] Limit search for primary fingerprint","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-22T20:23:10Z","receivedAt":"2019-11-22T20:23:20Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"As part of the process of implementing signature verification for git\nclone, I decided to refactor/unify the code for commit and merge\nverification to make it reusable during clones.\n\nThis lead me to discover that git requires merge signatures to be\ntrusted (as opposed to TRUST_UNKNOWN or TRUST_NEVER).  This is unlike\nthe behavior of verify-tag and verify-commit.\n\nSo, I figured that I'd make the minimum trust level configurable to make\nthe behavior of merge/commit/tag consistent.  And while doing so, I\nnoticed that parse_gpg_output() in gpg-interface.c assumes that the\nVALIDSIG status line has a field with a fingerprint for the primary key;\nbut that is only the case for OpenPGP signatures [1].\n\nThe consequence of that assumption is that the subsequent status line is\ninterpreted as the primary fingerprint for X509 signatures.  I'm not\nsure if the order is hardcoded in GnuPG, but in my testing the TRUST_\nstatus line always came after VALIDSIG -- and that breaks the config\noption to set a minimum trust level (not part of this patch):\n\n,----\n| $ git log -n1 --format=\"primary key: %GP\" signed-x509\n| gpgsm: Signature made 2019-11-16 14:13:09 using certificate ID 0xFA23FD65\n| gpgsm: Good signature from \"/CN=C O Mitter/O=Example/SN=C O/GN=Mitter\"\n| gpgsm:                 aka \"committer@example.com\"\n| primary key: TRUST_FULLY 0 shell\n`----\n\nAs per suggestion from Hamano, I also introduced a helper function,\nreplace_cstring().\n\nThis patch revision fixes the indentation in the test cases as well as\nan erroneous const qualification.\n\n[1]: https://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=6ce340e8c04794add995e84308bb3091450bd28f;hb=HEAD#l483\n\n\nHans Jerry Illikainen (2):\n  gpg-interface: refactor the free-and-xmemdupz pattern\n  gpg-interface: limit search for primary key fingerprint\n\n gpg-interface.c | 44 ++++++++++++++++++++++++++++++++------------\n t/t4202-log.sh  | 20 ++++++++++++++++++++\n 2 files changed, 52 insertions(+), 12 deletions(-)\n\n--\n2.24.GIT\n"},{"id":"386866","messageId":"20191122202312.10120-2-hji@dyntopia.com","threadId":"52278","inReplyTo":"20191122202312.10120-1-hji@dyntopia.com","subject":"[PATCH v4 1/2] gpg-interface: refactor the free-and-xmemdupz pattern","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-22T20:23:11Z","receivedAt":"2019-11-22T20:23:25Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"This commit introduces a static replace_cstring() function.  This\nfunction simplifies the continuous pattern of free-and-xmemdupz() for\nGPG status line parsing.\n\nThe benefit of having it in a single helper function is that it helps\navoid the need for duplicate code that does the same thing.  It also\nhelps avoid potential memleaks if parsing of new status lines are\nintroduced in the future.\n\nSigned-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n---\n gpg-interface.c | 22 +++++++++++++++-------\n 1 file changed, 15 insertions(+), 7 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex d60115ca40..37162c9a43 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -105,6 +105,16 @@ static struct {\n \t{ 0, \"VALIDSIG \", GPG_STATUS_FINGERPRINT },\n };\n \n+static void replace_cstring(char **field, const char *line, const char *next)\n+{\n+\tfree(*field);\n+\n+\tif (line && next)\n+\t\t*field = xmemdupz(line, next - line);\n+\telse\n+\t\t*field = NULL;\n+}\n+\n static void parse_gpg_output(struct signature_check *sigc)\n {\n \tconst char *buf = sigc->gpg_status;\n@@ -136,21 +146,18 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\t\t/* Do we have key information? */\n \t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_KEYID) {\n \t\t\t\t\tnext = strchrnul(line, ' ');\n-\t\t\t\t\tfree(sigc->key);\n-\t\t\t\t\tsigc->key = xmemdupz(line, next - line);\n+\t\t\t\t\treplace_cstring(&sigc->key, line, next);\n \t\t\t\t\t/* Do we have signer information? */\n \t\t\t\t\tif (*next && (sigcheck_gpg_status[i].flags & GPG_STATUS_UID)) {\n \t\t\t\t\t\tline = next + 1;\n \t\t\t\t\t\tnext = strchrnul(line, '\\n');\n-\t\t\t\t\t\tfree(sigc->signer);\n-\t\t\t\t\t\tsigc->signer = xmemdupz(line, next - line);\n+\t\t\t\t\t\treplace_cstring(&sigc->signer, line, next);\n \t\t\t\t\t}\n \t\t\t\t}\n \t\t\t\t/* Do we have fingerprint? */\n \t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {\n \t\t\t\t\tnext = strchrnul(line, ' ');\n-\t\t\t\t\tfree(sigc->fingerprint);\n-\t\t\t\t\tsigc->fingerprint = xmemdupz(line, next - line);\n+\t\t\t\t\treplace_cstring(&sigc->fingerprint, line, next);\n \n \t\t\t\t\t/* Skip interim fields */\n \t\t\t\t\tfor (j = 9; j > 0; j--) {\n@@ -162,7 +169,8 @@ static void parse_gpg_output(struct signature_check *sigc)\n \n \t\t\t\t\tnext = strchrnul(line, '\\n');\n \t\t\t\t\tfree(sigc->primary_key_fingerprint);\n-\t\t\t\t\tsigc->primary_key_fingerprint = xmemdupz(line, next - line);\n+\t\t\t\t\treplace_cstring(&sigc->primary_key_fingerprint,\n+\t\t\t\t\t\t\tline, next);\n \t\t\t\t}\n \n \t\t\t\tbreak;\n-- \n2.24.GIT\n\n"},{"id":"386867","messageId":"20191122202312.10120-3-hji@dyntopia.com","threadId":"52278","inReplyTo":"20191122202312.10120-1-hji@dyntopia.com","subject":"[PATCH v4 2/2] gpg-interface: limit search for primary key fingerprint","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-22T20:23:12Z","receivedAt":"2019-11-22T20:23:28Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"The VALIDSIG status line from GnuPG with --status-fd is documented to\nhave 9 required and 1 optional fields [1].  The final, and optional,\nfield is used to specify the fingerprint of the primary key that made\nthe signature in case it was made by a subkey.  However, this field is\nonly available for OpenPGP signatures; not for CMS/X.509.\n\nIf the VALIDSIG status line does not have the optional 10th field, the\ncurrent code will continue reading onto the next status line.  And this\nis the case for non-OpenPGP signatures [1].\n\nThe consequence is that a subsequent status line may be considered as\nthe \"primary key\" for signatures that does not have an actual primary\nkey.\n\nThe solution introduced by this commit is to limit the search for a\nprimary key to a single line.  The search for the 10th VALIDSIG field is\naborted as soon as it sees a newline character.  This keeps the parser\nfrom interpreting subsequent lines as the primary key.\n\n[Reference]\n[1] GnuPG Details, General status codes\nhttps://git.gnupg.org/cgi-bin/gitweb.cgi?p=gnupg.git;a=blob;f=doc/DETAILS;h=6ce340e8c04794add995e84308bb3091450bd28f;hb=HEAD#l483\n\nThe documentation say:\n\n    VALIDSIG <args>\n\n    The args are:\n\n    - <fingerprint_in_hex>\n    - <sig_creation_date>\n    - <sig-timestamp>\n    - <expire-timestamp>\n    - <sig-version>\n    - <reserved>\n    - <pubkey-algo>\n    - <hash-algo>\n    - <sig-class>\n    - [ <primary-key-fpr> ]\n\n    This status indicates that the signature is cryptographically\n    valid. [...] PRIMARY-KEY-FPR is the fingerprint of the primary key\n    or identical to the first argument.\n\n    The primary-key-fpr parameter is used for OpenPGP and not available\n    for CMS signatures.  [...]\n\nSigned-off-by: Hans Jerry Illikainen <hji@dyntopia.com>\n---\n gpg-interface.c | 24 ++++++++++++++++++------\n t/t4202-log.sh  | 20 ++++++++++++++++++++\n 2 files changed, 38 insertions(+), 6 deletions(-)\n\ndiff --git a/gpg-interface.c b/gpg-interface.c\nindex 37162c9a43..131e7d529e 100644\n--- a/gpg-interface.c\n+++ b/gpg-interface.c\n@@ -156,21 +156,33 @@ static void parse_gpg_output(struct signature_check *sigc)\n \t\t\t\t}\n \t\t\t\t/* Do we have fingerprint? */\n \t\t\t\tif (sigcheck_gpg_status[i].flags & GPG_STATUS_FINGERPRINT) {\n+\t\t\t\t\tconst char *limit;\n+\t\t\t\t\tchar **field;\n+\n \t\t\t\t\tnext = strchrnul(line, ' ');\n \t\t\t\t\treplace_cstring(&sigc->fingerprint, line, next);\n \n-\t\t\t\t\t/* Skip interim fields */\n+\t\t\t\t\t/*\n+\t\t\t\t\t * Skip interim fields.  The search is\n+\t\t\t\t\t * limited to the same line since only\n+\t\t\t\t\t * OpenPGP signatures has a field with\n+\t\t\t\t\t * the primary fingerprint.\n+\t\t\t\t\t */\n+\t\t\t\t\tlimit = strchrnul(line, '\\n');\n \t\t\t\t\tfor (j = 9; j > 0; j--) {\n-\t\t\t\t\t\tif (!*next)\n+\t\t\t\t\t\tif (!*next || limit <= next)\n \t\t\t\t\t\t\tbreak;\n \t\t\t\t\t\tline = next + 1;\n \t\t\t\t\t\tnext = strchrnul(line, ' ');\n \t\t\t\t\t}\n \n-\t\t\t\t\tnext = strchrnul(line, '\\n');\n-\t\t\t\t\tfree(sigc->primary_key_fingerprint);\n-\t\t\t\t\treplace_cstring(&sigc->primary_key_fingerprint,\n-\t\t\t\t\t\t\tline, next);\n+\t\t\t\t\tfield = &sigc->primary_key_fingerprint;\n+\t\t\t\t\tif (!j) {\n+\t\t\t\t\t\tnext = strchrnul(line, '\\n');\n+\t\t\t\t\t\treplace_cstring(field, line, next);\n+\t\t\t\t\t} else {\n+\t\t\t\t\t\treplace_cstring(field, NULL, NULL);\n+\t\t\t\t\t}\n \t\t\t\t}\n \n \t\t\t\tbreak;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex e803ba402e..d18613dad1 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1570,6 +1570,14 @@ test_expect_success GPG 'setup signed branch' '\n \tgit commit -S -m signed_commit\n '\n \n+test_expect_success GPG 'setup signed branch with subkey' '\n+\ttest_when_finished \"git reset --hard && git checkout master\" &&\n+\tgit checkout -b signed-subkey master &&\n+\techo foo >foo &&\n+\tgit add foo &&\n+\tgit commit -SB7227189 -m signed_commit\n+'\n+\n test_expect_success GPGSM 'setup signed branch x509' '\n \ttest_when_finished \"git reset --hard && git checkout master\" &&\n \tgit checkout -b signed-x509 master &&\n@@ -1580,6 +1588,18 @@ test_expect_success GPGSM 'setup signed branch x509' '\n \tgit commit -S -m signed_commit\n '\n \n+test_expect_success GPGSM 'log x509 fingerprint' '\n+\techo \"F8BF62E0693D0694816377099909C779FA23FD65 | \" >expect &&\n+\tgit log -n1 --format=\"%GF | %GP\" signed-x509 >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success GPGSM 'log OpenPGP fingerprint' '\n+\techo \"D4BE22311AD3131E5EDA29A461092E85B7227189\" > expect &&\n+\tgit log -n1 --format=\"%GP\" signed-subkey >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success GPG 'log --graph --show-signature' '\n \tgit log --graph --show-signature -n1 signed >actual &&\n \tgrep \"^| gpg: Signature made\" actual &&\n-- \n2.24.GIT\n\n"},{"id":"386868","messageId":"87v9rby82a.hji@dyntopia.com","threadId":"52278","inReplyTo":"xmqqeey07exp.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/1] gpg-interface: limit search for primary key fingerprint","fromName":"Hans Jerry Illikainen","fromEmail":"hji@dyntopia.com","sentAt":"2019-11-22T20:23:09Z","receivedAt":"2019-11-22T20:23:35Z","isPatch":true,"sender":{"key":"hji@dyntopia.com","avatar":"https://avatars.githubusercontent.com/u/8175517?v=4"},"body":"On Fri, Nov 22 2019, Junio C Hamano wrote:\n> Wouldn't some of the fields this helper works on be of type \"char *\"?\n\nWow, that's embarrassing.  I completely messed that one up after a\nlooong day.  Gah!  Fixed and re-built with DEVELOPER=1 and re-ran the\ntest suite for both commits in an attempt to avoid further fuckups.\n\nI also fixed the criticism on 2/2 (even though you mentioned that\nthere's no need for that) and sent it as v4 because I'm not sure what\nthe right approach is for changing only 1/2.\n\nFor future reference; how does the project prefer fixups for a single\ncommit on a multi-patch submission?\n\n--\nhji\n"},{"id":"386881","messageId":"xmqq1rtz4f9e.fsf@gitster-ct.c.googlers.com","threadId":"52278","inReplyTo":"87v9rby82a.hji@dyntopia.com","subject":"Re: [PATCH 1/1] gpg-interface: limit search for primary key fingerprint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-23T00:18:05Z","receivedAt":"2019-11-23T00:18:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hans Jerry Illikainen <hji@dyntopia.com> writes:\n\n> On Fri, Nov 22 2019, Junio C Hamano wrote:\n>> Wouldn't some of the fields this helper works on be of type \"char *\"?\n>\n> Wow, that's embarrassing.  I completely messed that one up after a\n> looong day.  Gah!  Fixed and re-built with DEVELOPER=1 and re-ran the\n> test suite for both commits in an attempt to avoid further fuckups.\n\nThe embarrassment is mine ;-)\n\n> I also fixed the criticism on 2/2 (even though you mentioned that\n> there's no need for that) and sent it as v4 because I'm not sure what\n> the right approach is for changing only 1/2.\n>\n> For future reference; how does the project prefer fixups for a single\n> commit on a multi-patch submission?\n\nUnless the series is insanely loooooooooooooooooooooooooooooooong,\nresending the whole thing, optionally with summary of the changes\nsince the previous iteration for each step after the three-dash\nlines (i.e. this allows readers to notice \"unchanged since v3\"\nand skip individual ones marked as such while reviewing v4), would\nbe a good way to help both reviewers who saw the previous round and\nthose who have skipped the previous round.\n\nThanks.\n"},{"id":"386882","messageId":"xmqqwobr30hx.fsf@gitster-ct.c.googlers.com","threadId":"52278","inReplyTo":"20191122202312.10120-1-hji@dyntopia.com","subject":"Re: [PATCH v4 0/2] Limit search for primary fingerprint","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-11-23T00:22:18Z","receivedAt":"2019-11-23T00:22:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Hans Jerry Illikainen <hji@dyntopia.com> writes:\n\n> ...\n> This patch revision fixes the indentation in the test cases as well as\n> an erroneous const qualification.\n\nI made minor tweaks to the log message, but otherwise I think this\nis ready for 'next'.\n\nThanks for working on this.\n"}]}