Re: [PATCH v4 6/6] tag.c: Change gpg_verify_tag argument to sha1

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 5, 2016, 03:46 UTC
Message-ID: <CAPig+cSkVtXezozamFohzRyvH46oO3ZFDBXzpV4fHtUUm3epVw@mail.gmail.com>
In-Reply-To: <20160405021028.GE12006@LykOS>

On Mon, Apr 4, 2016 at 10:10 PM, Santiago Torres <santiago@nyu.edu> wrote:

Show 20 quoted lines

> On Mon, Apr 04, 2016 at 10:00:17PM -0400, Eric Sunshine wrote:
>> On Mon, Apr 4, 2016 at 6:22 PM,  <santiago@nyu.edu> wrote:
>> > -       return gpg_verify_tag(name, GPG_VERIFY_VERBOSE);
>> > +       return gpg_verify_tag(sha1, GPG_VERIFY_VERBOSE);
>>
>> So, by this point, 'name' has already been resolved to 'sha1', thus
>> this change avoids a second resolution of 'name' inside
>> gpg_verify_tag(). Therefore, this is really an optimization, right?
>> Perhaps the intent of the patch would be clearer if the commit message
>> sold it as such. For instance, the commit message might start off:
>>
>>     tag: avoid resolving tag name twice
>>
>> and then go on to say that by hefting tag name resolution out of
>> gpg_verify_tag(), the extra resolution can be avoided.
>
> Yep, this is actually true, but something I didn't consider. I think
> that, from what I could draw on [1] and [2], git tag -v is reserved to
> tags only (refs/tags iirc). This patch makes it so that this behavior is
> not lost. I'm not sure if it should be separate from 5/6 though.

Okay, so this is a fix for a regression introduced by the previous patch. I agree that this is suboptimal. You can avoid this regression issue altogether by merely re-ordering the patch series. For instance, the series could be ordered like this:

1. SIGPIPE dance
2. add new t7030 test
3. improve variable name in verify_tag()
4. heft get_sha1() lookup out of verify_tag()
5. move code and publish gpg_verify_tag()
6. re-implement "git-tag -v" in terms of gpg_verify_tag()

Sell patch #4 as a libification preparation step, explaining as justification that some future clients may already have the sha1 in hand and would want to avoid duplicate resolution of the tag name.