Re: [PATCH 6/8] gpg-interface: find the last gpg signature line
On Tue, Apr 10, 2018 at 4:17 PM, Junio C Hamano wrote: > Junio C Hamano writes: > >>> That test was fixed a week ago: >>> https://github.com/git/git/commit/a99d903f21d102a5768f19157085a9733aeb68dd >> >> Well, you cannot expect any reviewer to know about a change that has >> never been sent to the list and has never been part of even 'pu' >> branch, no matter how old such a private "fix" is. >> >> What other unpublished things that are not even on 'pu' do these >> patches depend on? > > Answering my own question after digging a bit more, now I know that > a99d903 comes from the 'private' branch in peff/git/ repository > hosted at GitHub [*1*]. The branch builds on 'next' by merging many > private branches, and 'jk/non-pgp-signatures' branch has that commit. > > peff.git/private$ git log --oneline --boundary 0c8726318..7a526834e^2 > c9ce7c5b7 gpg-interface: handle multiple signing tools > 914951682 gpg-interface: handle bool user.signingkey > 1f2ea84b3 gpg-interface: return signature type from parse_signature() > 6d2ce6547 gpg-interface: prepare for parsing arbitrary PEM blocks > fb1d173db gpg-interface: find the last gpg signature line > 6bc4e7e17 gpg-interface: extract gpg line matching helper > 4f883ac49 gpg-interface: fix const-correctness of "eol" pointer > ae6529fdb gpg-interface: use size_t for signature buffer size > 1bca4296b gpg-interface: modernize function declarations > a99d903f2 t7004: fix mistaken tag name > - 468165c1d Git 2.17 > > It seems to me that Peff did the t7004 change as the earliest > preliminary step of the series, but it caused confusion when it was > not sent as part of the series by mistake. Judging from the shape > of the topic, I do not think this topic has any other hidden > dependencies as it builds directly on top of v2.17.0. > > For those who are reading and reviewing from the sideline, I've > attached that missing 0.9/8 patch at the end of this message. Sorry for the confusion Junio. I'm not used to the mailing list workflow and it seems that I missed a patch. I'll make sure to include that patch in v2. > > [Footnote] > > *1* I do not know if it deserves to be called a bug, but it > certainly is an anomaly GitHub exhibits that a99d903f can be > viewed at https://github.com/git/git/commit/a99d903f..., as if > it is part of the official git/git history, when it only exists > in a fork of that repository. I can understand why it happens > but it certainly is unexpected. > > -- >8 -- > From: Jeff King > Date: Tue, 3 Apr 2018 17:10:30 -0400 > Subject: [PATCH 0.9/8] t7004: fix mistaken tag name > > We have a series of tests which create signed tags with > various properties, but one test accidentally verifies a tag > from much earlier in the series. > --- > t/t7004-tag.sh | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > index 2aac77af7..ee093b393 100755 > --- a/t/t7004-tag.sh > +++ b/t/t7004-tag.sh > @@ -1056,7 +1056,7 @@ test_expect_success GPG \ > git tag -s -F sigblanknonlfile blanknonlfile-signed-tag && > get_tag_msg blanknonlfile-signed-tag >actual && > test_cmp expect actual && > - git tag -v signed-tag > + git tag -v blanknonlfile-signed-tag > ' > > # messages with commented lines for signed tags: > -- > 2.17.0-140-g0b0cc9f867 > -- -Ben Toews
Re: [PATCH 6/8] gpg-interface: find the last gpg signature line
Junio C Hamano writes: >> That test was fixed a week ago: >> https://github.com/git/git/commit/a99d903f21d102a5768f19157085a9733aeb68dd > > Well, you cannot expect any reviewer to know about a change that has > never been sent to the list and has never been part of even 'pu' > branch, no matter how old such a private "fix" is. > > What other unpublished things that are not even on 'pu' do these > patches depend on? Answering my own question after digging a bit more, now I know that a99d903 comes from the 'private' branch in peff/git/ repository hosted at GitHub [*1*]. The branch builds on 'next' by merging many private branches, and 'jk/non-pgp-signatures' branch has that commit. peff.git/private$ git log --oneline --boundary 0c8726318..7a526834e^2 c9ce7c5b7 gpg-interface: handle multiple signing tools 914951682 gpg-interface: handle bool user.signingkey 1f2ea84b3 gpg-interface: return signature type from parse_signature() 6d2ce6547 gpg-interface: prepare for parsing arbitrary PEM blocks fb1d173db gpg-interface: find the last gpg signature line 6bc4e7e17 gpg-interface: extract gpg line matching helper 4f883ac49 gpg-interface: fix const-correctness of "eol" pointer ae6529fdb gpg-interface: use size_t for signature buffer size 1bca4296b gpg-interface: modernize function declarations a99d903f2 t7004: fix mistaken tag name - 468165c1d Git 2.17 It seems to me that Peff did the t7004 change as the earliest preliminary step of the series, but it caused confusion when it was not sent as part of the series by mistake. Judging from the shape of the topic, I do not think this topic has any other hidden dependencies as it builds directly on top of v2.17.0. For those who are reading and reviewing from the sideline, I've attached that missing 0.9/8 patch at the end of this message. [Footnote] *1* I do not know if it deserves to be called a bug, but it certainly is an anomaly GitHub exhibits that a99d903f can be viewed at https://github.com/git/git/commit/a99d903f..., as if it is part of the official git/git history, when it only exists in a fork of that repository. I can understand why it happens but it certainly is unexpected. -- >8 -- From: Jeff King Date: Tue, 3 Apr 2018 17:10:30 -0400 Subject: [PATCH 0.9/8] t7004: fix mistaken tag name We have a series of tests which create signed tags with various properties, but one test accidentally verifies a tag from much earlier in the series. --- t/t7004-tag.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh index 2aac77af7..ee093b393 100755 --- a/t/t7004-tag.sh +++ b/t/t7004-tag.sh @@ -1056,7 +1056,7 @@ test_expect_success GPG \ git tag -s -F sigblanknonlfile blanknonlfile-signed-tag && get_tag_msg blanknonlfile-signed-tag >actual && test_cmp expect actual && - git tag -v signed-tag + git tag -v blanknonlfile-signed-tag ' # messages with commented lines for signed tags: -- 2.17.0-140-g0b0cc9f867
Re: [PATCH 6/8] gpg-interface: find the last gpg signature line
Ben Toews writes: >> H, what vintage of our codebase is this patch based on? Did I >> miss a patch that removes these lines >> >> >> printf ' ' >sigblanknonlfile >> get_tag_header blanknonlfile-signed-tag $commit commit $time >expect >> echo '-BEGIN PGP SIGNATURE-' >>expect >> test_expect_success GPG \ >> 'creating a signed tag with spaces and no newline should >> succeed' ' >> git tag -s -F sigblanknonlfile blanknonlfile-signed-tag && >> get_tag_msg blanknonlfile-signed-tag >actual && >> test_cmp expect actual && >> git tag -v signed-tag >> ' >> >> which appear between the pre- and post- context of the lines you are >> inserting? They date back to 2007-2009. >> > > That test was fixed a week ago: > https://github.com/git/git/commit/a99d903f21d102a5768f19157085a9733aeb68dd Well, you cannot expect any reviewer to know about a change that has never been sent to the list and has never been part of even 'pu' branch, no matter how old such a private "fix" is. What other unpublished things that are not even on 'pu' do these patches depend on?
Re: [PATCH 6/8] gpg-interface: find the last gpg signature line
On Tue, Apr 10, 2018 at 3:44 AM, Junio C Hamano wrote: > Ben Toews writes: > >> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh >> index ee093b393d..e3f1e014aa 100755 >> --- a/t/t7004-tag.sh >> +++ b/t/t7004-tag.sh >> @@ -1059,6 +1059,17 @@ test_expect_success GPG \ >> git tag -v blanknonlfile-signed-tag >> ' >> >> +test_expect_success GPG 'signed tag with embedded PGP message' ' >> + cat >msg <<-\EOF && >> + -BEGIN PGP MESSAGE- >> + >> + this is not a real PGP message >> + -END PGP MESSAGE- >> + EOF >> + git tag -s -F msg confusing-pgp-message && >> + git tag -v confusing-pgp-message >> +' >> + >> # messages with commented lines for signed tags: >> >> cat >sigcommentsfile < > H, what vintage of our codebase is this patch based on? Did I > miss a patch that removes these lines > > > printf ' ' >sigblanknonlfile > get_tag_header blanknonlfile-signed-tag $commit commit $time >expect > echo '-BEGIN PGP SIGNATURE-' >>expect > test_expect_success GPG \ > 'creating a signed tag with spaces and no newline should succeed' > ' > git tag -s -F sigblanknonlfile blanknonlfile-signed-tag && > get_tag_msg blanknonlfile-signed-tag >actual && > test_cmp expect actual && > git tag -v signed-tag > ' > > which appear between the pre- and post- context of the lines you are > inserting? They date back to 2007-2009. > That test was fixed a week ago: https://github.com/git/git/commit/a99d903f21d102a5768f19157085a9733aeb68dd -- -Ben Toews
Re: [PATCH 6/8] gpg-interface: find the last gpg signature line
Ben Toews writes: > diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh > index ee093b393d..e3f1e014aa 100755 > --- a/t/t7004-tag.sh > +++ b/t/t7004-tag.sh > @@ -1059,6 +1059,17 @@ test_expect_success GPG \ > git tag -v blanknonlfile-signed-tag > ' > > +test_expect_success GPG 'signed tag with embedded PGP message' ' > + cat >msg <<-\EOF && > + -BEGIN PGP MESSAGE- > + > + this is not a real PGP message > + -END PGP MESSAGE- > + EOF > + git tag -s -F msg confusing-pgp-message && > + git tag -v confusing-pgp-message > +' > + > # messages with commented lines for signed tags: > > cat >sigcommentsfileexpect echo '-BEGIN PGP SIGNATURE-' >>expect test_expect_success GPG \ 'creating a signed tag with spaces and no newline should succeed' ' git tag -s -F sigblanknonlfile blanknonlfile-signed-tag && get_tag_msg blanknonlfile-signed-tag >actual && test_cmp expect actual && git tag -v signed-tag ' which appear between the pre- and post- context of the lines you are inserting? They date back to 2007-2009.
Re: [PATCH 6/8] gpg-interface: find the last gpg signature line
On Mon, Apr 9, 2018 at 4:41 PM, Ben Toews wrote:
> From: Jeff King
>
> A signed tag has a detached signature like this:
>
> object ...
> [...more header...]
>
> This is the tag body.
>
> -BEGIN PGP SIGNATURE-
> [opaque gpg data]
> -END PGP SIGNATURE-
>
> Our parser finds the _first_ line that appears to start a
> PGP signature block, meaning we may be confused by a
> signature (or a signature-like line) in the actual body.
> Let's keep parsing and always find the final block, which
> should be the detached signature over all of the preceding
> content.
> ---
> diff --git a/gpg-interface.c b/gpg-interface.c
> @@ -110,11 +110,17 @@ static int is_gpg_start(const char *line)
> size_t parse_signature(const char *buf, size_t size)
> {
> size_t len = 0;
> - while (len < size && !is_gpg_start(buf + len)) {
> - const char *eol = memchr(buf + len, '\n', size - len);
> + size_t match = size;
If no GPG-start line is found then 'size' will be returned, which
matches the logic before this change. Okay.
> + while (len < size) {
> + const char *eol;
> +
> + if (is_gpg_start(buf + len))
> + match = len;
Otherwise, the position of the final GPG-start line will be remembered
and returned. Makes sense.
> + eol = memchr(buf + len, '\n', size - len);
> len += eol ? eol - (buf + len) + 1 : size - len;
> }
> - return len;
> + return match;
> }
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> @@ -1059,6 +1059,17 @@ test_expect_success GPG \
> +test_expect_success GPG 'signed tag with embedded PGP message' '
> + cat >msg <<-\EOF &&
> + -BEGIN PGP MESSAGE-
> +
> + this is not a real PGP message
> + -END PGP MESSAGE-
> + EOF
This bogus PGP message is just in the body...
> + git tag -s -F msg confusing-pgp-message &&
and "git tag -s" adds the real PGP message at the end...
> + git tag -v confusing-pgp-message
and the new logic finds the real PGP message rather than the bogus
one, so "git tag -v" exits successfully. Looks good.
> +'
