| Message ID | 20220302090250.590450-1-fs@gigacodes.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3,1/3] gpg-interface/gpgsm: fix for v2.3 | expand |
Fabian Stelzer <fs@gigacodes.de> writes: > Checking if signing was successful will now accept '[GNUPG]: > SIG_CREATED' on any beginning of a line. Not just explictly the second > one anymore. "the second or subsequent one", I would think, but the code change looks correct anyway. > Switch to gpg's `--with-colons` output format to make > parsing more robust. This avoids issues where the > human-readable output from gpg commands changes. Does this refer only to how parsing in tests is done? > Adjust error messages checking in tests for v2.3 specific output changes. Does this refer only to the change to 4202 where "failed to find the" and the colon after "certificate" are made optional, so that the regexp can read messages from both pre- and post-2.3 versions? > diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh > index 3e7ee1386a..6bc083ca77 100644 > --- a/t/lib-gpg.sh > +++ b/t/lib-gpg.sh > @@ -72,12 +72,10 @@ test_lazy_prereq GPGSM ' > --passphrase-fd 0 --pinentry-mode loopback \ > --import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 && > > - gpgsm --homedir "${GNUPGHOME}" -K | > - grep fingerprint: | > - cut -d" " -f4 | > - tr -d "\\n" >"${GNUPGHOME}/trustlist.txt" && > + gpgsm --homedir "${GNUPGHOME}" -K --with-colons | > + awk -F ":" "/^fpr:/ {printf \"%s S relax\\n\", \$10}" \ > + >"${GNUPGHOME}/trustlist.txt" && The old iteration had (fpr|fingerprint) which appeared as if it were catering to both pre- and post-2.3 versions, but "with colons", all versions we care about would say "fpr" and that is the reason why we no longer have such an alternative here? Just checking my understanding. > - echo " S relax" >>"${GNUPGHOME}/trustlist.txt" && This removal is because...? I do not recall seeing the explanation in the proposed log message. > echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \ > -u committer@example.com -o /dev/null --sign - > ' > diff --git a/t/t4202-log.sh b/t/t4202-log.sh > index 55fac64446..d599bf4b11 100755 > --- a/t/t4202-log.sh > +++ b/t/t4202-log.sh > @@ -2037,7 +2037,7 @@ test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 miss > git merge --no-ff -m msg signed_tag_x509_nokey && > GNUPGHOME=. git log --graph --show-signature -n1 plain-x509-nokey >actual && > grep "^|\\\ merged tag" actual && > - grep "^| | gpgsm: certificate not found" actual > + grep -Ei "^| | gpgsm:( failed to find the)? certificate:? not found" actual > ' OK. It might be easier to read if we give two expressions separately and say "we can take either of these", i.e. # the former is from pre-2.3, the latter is from 2.3 and later grep -e "^| | gpgsm: certificate not found" \ -e "^| | gpgsm: failed to find the certificate: not found" \ actual Thanks for working on this update.
On 02.03.2022 11:18, Junio C Hamano wrote: >Fabian Stelzer <fs@gigacodes.de> writes: > >> Checking if signing was successful will now accept '[GNUPG]: >> SIG_CREATED' on any beginning of a line. Not just explictly the second >> one anymore. > >"the second or subsequent one", I would think, but the code change >looks correct anyway. > >> Switch to gpg's `--with-colons` output format to make >> parsing more robust. This avoids issues where the >> human-readable output from gpg commands changes. > >Does this refer only to how parsing in tests is done? If only refers to the test prerequisite actually. I'll update the message. > >> Adjust error messages checking in tests for v2.3 specific output changes. > >Does this refer only to the change to 4202 where "failed to find >the" and the colon after "certificate" are made optional, so that >the regexp can read messages from both pre- and post-2.3 versions? > >> diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh >> index 3e7ee1386a..6bc083ca77 100644 >> --- a/t/lib-gpg.sh >> +++ b/t/lib-gpg.sh >> @@ -72,12 +72,10 @@ test_lazy_prereq GPGSM ' >> --passphrase-fd 0 --pinentry-mode loopback \ >> --import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 && >> >> - gpgsm --homedir "${GNUPGHOME}" -K | >> - grep fingerprint: | >> - cut -d" " -f4 | >> - tr -d "\\n" >"${GNUPGHOME}/trustlist.txt" && >> + gpgsm --homedir "${GNUPGHOME}" -K --with-colons | >> + awk -F ":" "/^fpr:/ {printf \"%s S relax\\n\", \$10}" \ >> + >"${GNUPGHOME}/trustlist.txt" && > >The old iteration had (fpr|fingerprint) which appeared as if it were >catering to both pre- and post-2.3 versions, but "with colons", all >versions we care about would say "fpr" and that is the reason why we >no longer have such an alternative here? Just checking my >understanding. Correct. The `with-colons` always uses fpr pre and post 2.3 > >> - echo " S relax" >>"${GNUPGHOME}/trustlist.txt" && > >This removal is because...? I do not recall seeing the explanation >in the proposed log message. Switching to awk allows us to integrate this trailing info into the awk expression itself making this extra echo unnecessary. > >> echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \ >> -u committer@example.com -o /dev/null --sign - >> ' >> diff --git a/t/t4202-log.sh b/t/t4202-log.sh >> index 55fac64446..d599bf4b11 100755 >> --- a/t/t4202-log.sh >> +++ b/t/t4202-log.sh >> @@ -2037,7 +2037,7 @@ test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 miss >> git merge --no-ff -m msg signed_tag_x509_nokey && >> GNUPGHOME=. git log --graph --show-signature -n1 plain-x509-nokey >actual && >> grep "^|\\\ merged tag" actual && >> - grep "^| | gpgsm: certificate not found" actual >> + grep -Ei "^| | gpgsm:( failed to find the)? certificate:? not found" actual >> ' > >OK. It might be easier to read if we give two expressions >separately and say "we can take either of these", i.e. > > # the former is from pre-2.3, the latter is from 2.3 and later > grep -e "^| | gpgsm: certificate not found" \ > -e "^| | gpgsm: failed to find the certificate: not found" \ > actual > >Thanks for working on this update. Easy enough. Initially I used a subshell and 2 grep calls but this is obviously easier. I prefer the static strings over the regex as well. I'll send a new patch probably tomorrow and try to improve the commit message. Thanks
diff --git a/gpg-interface.c b/gpg-interface.c index aa50224e67..280f1fa1a5 100644 --- a/gpg-interface.c +++ b/gpg-interface.c @@ -934,6 +934,7 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature, struct child_process gpg = CHILD_PROCESS_INIT; int ret; size_t bottom; + const char *cp; struct strbuf gpg_status = STRBUF_INIT; strvec_pushl(&gpg.args, @@ -953,7 +954,13 @@ static int sign_buffer_gpg(struct strbuf *buffer, struct strbuf *signature, signature, 1024, &gpg_status, 0); sigchain_pop(SIGPIPE); - ret |= !strstr(gpg_status.buf, "\n[GNUPG:] SIG_CREATED "); + for (cp = gpg_status.buf; + cp && (cp = strstr(cp, "[GNUPG:] SIG_CREATED ")); + cp++) { + if (cp == gpg_status.buf || cp[-1] == '\n') + break; /* found */ + } + ret |= !cp; strbuf_release(&gpg_status); if (ret) return error(_("gpg failed to sign the data")); diff --git a/t/lib-gpg.sh b/t/lib-gpg.sh index 3e7ee1386a..6bc083ca77 100644 --- a/t/lib-gpg.sh +++ b/t/lib-gpg.sh @@ -72,12 +72,10 @@ test_lazy_prereq GPGSM ' --passphrase-fd 0 --pinentry-mode loopback \ --import "$TEST_DIRECTORY"/lib-gpg/gpgsm_cert.p12 && - gpgsm --homedir "${GNUPGHOME}" -K | - grep fingerprint: | - cut -d" " -f4 | - tr -d "\\n" >"${GNUPGHOME}/trustlist.txt" && + gpgsm --homedir "${GNUPGHOME}" -K --with-colons | + awk -F ":" "/^fpr:/ {printf \"%s S relax\\n\", \$10}" \ + >"${GNUPGHOME}/trustlist.txt" && - echo " S relax" >>"${GNUPGHOME}/trustlist.txt" && echo hello | gpgsm --homedir "${GNUPGHOME}" >/dev/null \ -u committer@example.com -o /dev/null --sign - ' diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 55fac64446..d599bf4b11 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -2037,7 +2037,7 @@ test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 miss git merge --no-ff -m msg signed_tag_x509_nokey && GNUPGHOME=. git log --graph --show-signature -n1 plain-x509-nokey >actual && grep "^|\\\ merged tag" actual && - grep "^| | gpgsm: certificate not found" actual + grep -Ei "^| | gpgsm:( failed to find the)? certificate:? not found" actual ' test_expect_success GPGSM 'log --graph --show-signature for merged tag x509 bad signature' '