Re: [PATCH v3 4/4] tag: use pgp_verify_function in tag -v call
- From
Santiago Torres <santiago@nyu.edu>
- Date
- Apr 4, 2016, 18:24 UTC
- Message-ID
- <20160404182447.GA6773@LykOS>
- In-Reply-To
- <20160404133853.GB25404@sigill.intra.peff.net>
On Mon, Apr 04, 2016 at 09:38:54AM -0400, Jeff King wrote:
Show 14 quoted lines
> On Mon, Apr 04, 2016 at 12:12:04AM -0400, Santiago Torres wrote: > > > > As a side note, it might actually be an improvement for pgp_verify_tag > > > to take a sha1 (so that git-tag is sure that it is verifying the same > > > object that it is printing), but that refactoring should probably come > > > separately, I think. > > > > Just to be sure, this refactoring is something we should still include > > in this set of patches, right? I think that otherwise we'd lose the > > desambigutaion that git tag -v does in this patch. > > I think it can be part of this series, but doesn't have to be. As I > understand it, the current code is just handing the name to the `git > verify-tag` process, so if we continue to do so, that would be OK.
IIRC, the current code for git tag -v hands the hex-representation[1] of the sha1 to git verify-tag --- I believe that's related to the desamgibuation issue I've seen people discuss. I think this behavior is lost unless we add this on top of the patch.
Show 11 quoted lines
> > > I also think that most of the rippling is gone if we use and adaptor as > > you suggested. Should I add a patch on top of this to support a sha1 as > > part for gpg_verify_tag()? > > Yes, though I'd generally advise against a function taking either a name or > a sha1, and ignoring the other option. That often leads to confusing > interfaces for the callers. Instead, perhaps just take the sha1, and let > the caller do the get_sha1() themselves. Or possibly provide two > functions, one of which is a convenience to translate the name to sha1 > and then call the other.
I think the former sounds easier. I can replace the name argument and move the sha1-resolution code to in verify-tag. git tag -v already resolves the tagname to a sha1, so it is easier there.
Does this sound reasonable?
Thanks! -Santiago
[1] https://git.kernel.org/cgit/git/git.git/tree/builtin/tag.c#n109