On Mon, Apr 4, 2016 at 10:10 PM, Santiago Torres <santiago@xxxxxxx> wrote: > On Mon, Apr 04, 2016 at 10:00:17PM -0400, Eric Sunshine wrote: >> On Mon, Apr 4, 2016 at 6:22 PM, <santiago@xxxxxxx> 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. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html