Re: [PATCH] tag.c: move PGP verification code from plumbing

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Mar 26, 2016, 06:33 UTC
Message-ID: <CAPig+cSQ2+yc6UCM08zMUDXKFgRBj5FUUEe+wFLxGkykE2yKxA@mail.gmail.com>
In-Reply-To: <20160325144509.GA20375@LykOS>

On Fri, Mar 25, 2016 at 10:45 AM, Santiago Torres <santiago@nyu.edu> wrote:

Show 21 quoted lines

>> >         while (i < argc)
>> > -               if (verify_tag(argv[i++], flags))
>> > +               name = argv[i++];
>> > +               if (get_sha1(name, sha1))
>> > +                       return error("tag '%s' not found.", name);
>> > +
>> > +               if (pgp_verify_tag(NULL, NULL, sha1, flags))
>> >                         had_error = 1;
>>
>> Meh, this isn't Python. Due to the missing braces, the only thing
>> inside the while() loop is the assignment to 'name'; all the other
>> indented code is outside the while().
>>
>> Did you run the test suite following this change? Did it all pass? If
>> so, perhaps an additional test or two to catch this sort of error
>> would be warranted.
>
> Wow, you're right! I just re-ran the tests again to make sure I didn't
> miss anything. All the tests pass for me, so I'll write an extra case to
> avoid this. Just to be sure, I should include it in t7030-verify-tag.sh
> right?

Generally speaking, it is desirable to have test coverage for all functionality you're refactoring to ensure that the refactoring doesn't break that functionality.

t7030-verify-tag.sh indeed seems like a good place to add a new test for catching this sort of regression.

t7004-tag.sh is also of interest since, in an earlier version of this patch, if I recall correctly, Peff caught a regression where "git tag -v" failed to pass the GPG_VERIFY_VERBOSE flag, and I don't think t7004-tag.sh would have caught that problem either, so a new test for that would be good, as well.

Speaking of GPG_VERIFY_VERBOSE, now that I'm examining the changes more closely, it appears that this version of the patch no longer respects GPG_VERIFY_VERBOSE at all but is instead hard-coded to be verbose. Is that desirable or intentional?