Re: [PATCH/RFC] builtin/tag: Changes argument format for verify

From: Santiago Torres <santiago@nyu.edu>
Date: Mar 3, 2016, 22:05 UTC
Message-ID: <20160303220502.GA2234@LykOS>
In-Reply-To: <20160227183133.GB12822@sigill.intra.peff.net>

Hi Peff.

I've been trying to shape these changes into sensible patch, but it is not as trivial as I originally thought. I think the issue lies in the tag desambiguation aspect of the git-tag command.

It seems that verify-tag can take either the refname or the hash of the object. However, git tag --verify takes only the refname, so it doesn't resolve the tag-sha1 if that's specified as an argument.

I'm wondering if this is what we would want in the updated patch (accept sha1's also). If so, does the following make sense?

1) if arg not in .git/tag/refs
2) then try to resolve using get_sha1(name, sha1) take it from there.

Also, would it make sense to remove the verify-tag command altogether? On the same line, it seems that there used to be a --raw flag on the verify-tag command, should I propagate this to git tag --verify?

Thanks! -Santiago.

On Sat, Feb 27, 2016 at 01:31:33PM -0500, Jeff King wrote:

Show 26 quoted lines

> On Sat, Feb 27, 2016 at 12:45:24PM -0500, Santiago Torres wrote:
> 
> > > A much more interesting change in this area, I think, would be to skip
> > > verify-tag entirely. Once upon a time it had a lot of logic itself, but
> > > these days it is a thin wrapper over run_gpg_verify(), and we could
> > > improve the efficiency quite a bit by eliminates the sub-process
> > > entirely.
> > 
> > I agree here too. while going through gdb to follow the logic on this I saw that
> > this code forks three times (git, tag-verify and gpg). I'm sure that
> > removing one layer should be good efficiencly-wise.
> > 
> > Is it ok if I give this a shot?
> 
> Sure.
> 
> I suspect the extra process is there for historical reasons; git-tag was
> originally a shell script that called out to git-verify-tag, and the
> conversion to C retained the separate call.
> 
> I cannot think of a reason that it would be a bad thing to do it all in
> a single process. Do note the trickery with SIGPIPE in verify-tag,
> though. We probably need to do the same here (in fact, I wonder if that
> should be pushed down into the code that calls gpg).
> 
> -Peff