Re: [PATCH/RFC] builtin/tag.c: move PGP verification inside builtin.

[Santiago Torres <torresariass@gmail.com>]

> I know you are just copying this from the one in builtin/verify-tag.c,
> but I find the use of "size" and "len" for two different purposes
> confusing. Those words are synonyms, so how do the variables differ?
> 
> Perhaps "payload_size", or "signature_offset" would be a better term for
> "len".

I agree, I'll give this a go.
> 
> > +	if (size == len) {
> > +		write_in_full(1, buf, len);
> > +	}
> 
> If the two are the same, we have no signature. Should we be returning
> early, and skipping check_signature() in that case?

This makes sense, for both the builtin and the plumbing. Let me give
this a try.

 
> > @@ -104,13 +125,24 @@ static int delete_tag(const char *name, const char *ref,
> >  static int verify_tag(const char *name, const char *ref,
> >  				const unsigned char *sha1)
> >  {
> > -	const char *argv_verify_tag[] = {"verify-tag",
> > -					"-v", "SHA1_HEX", NULL};
> 
> So the original was passing "-v" to verify-tag. That should put
> GPG_VERIFY_VERBOSE into the flags field. But later:
> 
> > +	ret = run_gpg_verify(buf, size, 0);
> 
> We don't pass any flags. Shouldn't this unconditionally pass
> GPG_VERIFY_VERBOSE?
> 

Right, I missed this. Sorry about this.

> All of this seems like a repetition of verify_tag() in
> builtin/verify-tag.c (and ditto with run_gpg_verify()). Can we move
> those functions into tag.c and just call them from both places, or is
> there some difference that needs to be taken into account (and if the
> latter, can we refactor them to account for the differences?).
> 

Yep, this is what was troubling me (as I mentioned on the followup). I
didn't want to remove the "static" classifier for the function (as there
could be a major reason for this decision). 

If this last chage is ok with you I can send the fixed-up version right
away.

Thanks!
-Santiago.