Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags
On Tue, Apr 5, 2016 at 12:07 PM, wrote: > t7030-verify-tag: Adds validation for multiple tags s/t7030-verify-tag/t7030/ s/Adds/add > The verify-tag command supports multiple tag names as an argument. > However, existing tests only test for invocation with a single tag, so > we add a test invoking with multiple tags. > > Helped-by: Jeff King > Signed-off-by: Santiago Torres > --- > diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh > @@ -112,4 +112,16 @@ test_expect_success GPG 'verify signatures with --raw' ' > +test_expect_success GPG 'verify multiple tags' ' > + tags="fourth-signed sixth-signed seventh-signed" && > + for i in $tags; do Style: 'do' on its own line; drop semicolon for i in ... do ... done > + git verify-tag -v --raw $i || return 1 > + done >expect.stdout 2>expect.stderr.1 && > + grep "^.GNUPG" expect.stderr && Hmm, is there a reason you didn't stick with the more strict regex Peff suggested? ^.GNUPG:. (Genuine question: I'm not saying your choice is wrong, I'm just interested in the reasoning behind the decision.) > + git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 && > + grep "^.GNUPG" actual.stderr && > + test_cmp expect.stdout actual.stdout && > + test_cmp expect.stderr actual.stderr > +' > + > test_done -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags
santi...@nyu.edu writes: > From: Santiago Torres > > The verify-tag command supports multiple tag names as an argument. That makes it sound as if this is a valid input $ git verify-tag "one two three four" but that is not what you meant. > However, existing tests only test for invocation with a single tag, so > we add a test invoking with multiple tags. I'd rephrase the above like so: t7030: test verifying multiple tags The verify-tag command accepts multiple tag names to verify, but existing tests only test for invocation with a single tag. Add a test invoking it with multiple tags. I had the same (minor) problem with Eric on the patch text itself. > > Helped-by: Jeff King > Signed-off-by: Santiago Torres > --- > t/t7030-verify-tag.sh | 12 > 1 file changed, 12 insertions(+) > > diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh > index 4608e71..c01621a 100755 > --- a/t/t7030-verify-tag.sh > +++ b/t/t7030-verify-tag.sh > @@ -112,4 +112,16 @@ test_expect_success GPG 'verify signatures with --raw' ' > ) > ' > > +test_expect_success GPG 'verify multiple tags' ' > + tags="fourth-signed sixth-signed seventh-signed" && > + for i in $tags; do > + git verify-tag -v --raw $i || return 1 > + done >expect.stdout 2>expect.stderr.1 && > + grep "^.GNUPG" expect.stderr && > + git verify-tag -v --raw $tags >actual.stdout 2>actual.stderr.1 && > + grep "^.GNUPG" actual.stderr && > + test_cmp expect.stdout actual.stdout && > + test_cmp expect.stderr actual.stderr > +' > + > test_done -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags
Sorry for the delay! I just realized I had missed the second comment. > + grep "^.GNUPG" expect.stderr && > > Hmm, is there a reason you didn't stick with the more strict regex > Peff suggested? > > ^.GNUPG:. > > (Genuine question: I'm not saying your choice is wrong, I'm just > interested in the reasoning behind the decision.) > I actually had missed the ":". I read the email and tried to internalize what the new test was actually doing, then I rewrote the test. I think I could add it for completeness though. Thanks again for the reviews! -Santiago. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags
On Sun, Apr 17, 2016 at 1:31 PM, Santiago Torres wrote: >> + grep "^.GNUPG" expect.stderr && >> >> Hmm, is there a reason you didn't stick with the more strict regex >> Peff suggested? >> >> ^.GNUPG:. >> >> (Genuine question: I'm not saying your choice is wrong, I'm just >> interested in the reasoning behind the decision.) > > I actually had missed the ":". I read the email and tried to internalize > what the new test was actually doing, then I rewrote the test. > > I think I could add it for completeness though. Junio already made this correction and others in the three patches he queued on his 'pu' branch. It's possible that he also made other tweaks not mentioned in the reviews, so it's a good idea to compare what he queued against what you plan to send for the re-roll to ensure that nothing is missed. Thanks. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags
On Sun, Apr 17, 2016 at 02:19:00PM -0400, Eric Sunshine wrote: > On Sun, Apr 17, 2016 at 1:31 PM, Santiago Torres wrote: > >> + grep "^.GNUPG" expect.stderr && > >> > >> Hmm, is there a reason you didn't stick with the more strict regex > >> Peff suggested? > >> > >> ^.GNUPG:. > >> > >> (Genuine question: I'm not saying your choice is wrong, I'm just > >> interested in the reasoning behind the decision.) > > > > I actually had missed the ":". I read the email and tried to internalize > > what the new test was actually doing, then I rewrote the test. > > > > I think I could add it for completeness though. > > Junio already made this correction and others in the three patches he > queued on his 'pu' branch. It's possible that he also made other > tweaks not mentioned in the reviews, so it's a good idea to compare > what he queued against what you plan to send for the re-roll to ensure > that nothing is missed. Thanks. Oh, I'm looking at the patches in pu, I didn't know they were there yet. Thanks for the heads up. Also, would it make sense to copy the commit messages as they are on the pu branch? for consistency? Or should ommit those three patches and work on 4+ for the re-roll instead? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 2/6] t7030-verify-tag: Adds validation for multiple tags
On Sun, Apr 17, 2016 at 2:38 PM, Santiago Torres wrote: > On Sun, Apr 17, 2016 at 02:19:00PM -0400, Eric Sunshine wrote: >> Junio already made this correction and others in the three patches he >> queued on his 'pu' branch. It's possible that he also made other >> tweaks not mentioned in the reviews, so it's a good idea to compare >> what he queued against what you plan to send for the re-roll to ensure >> that nothing is missed. Thanks. > > Oh, I'm looking at the patches in pu, I didn't know they were there yet. > Thanks for the heads up. > > Also, would it make sense to copy the commit messages as they are on the pu > branch? for consistency? Or should ommit those three patches and work > on 4+ for the re-roll instead? I just re-read the commit messages as Junio queued them on 'pu', and they are all good, so yes it would be plenty sensible to copy the commit messages from the three queued patches (along with whatever other tweaks he made). Since the patches are only in 'pu' (but not in 'next'), when you re-roll, resubmit the entire series, not just the patches he didn't queue. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
