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

[Jeff King <peff@peff.net>]

On Thu, Mar 24, 2016 at 05:39:20PM -0400, santiago@nyu.edu wrote:

> +static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
> +{
> +	struct signature_check sigc;
> +	int len;
> +	int ret;
> +
> +	memset(&sigc, 0, sizeof(sigc));
> +
> +	len = parse_signature(buf, size);

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".

> +	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?

> +	ret = check_signature(buf, len, buf + len, size - len, &sigc);
> +	print_signature_buffer(&sigc, flags);
> +
> +	signature_check_clear(&sigc);
> +	return ret;
> +}

This part looks OK.

> @@ -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?

> +	enum object_type type;
> +	unsigned long size;
> +	const char* buf;
> +	int ret;
> +
> +	type = sha1_object_info(sha1, NULL);
> +	if (type != OBJ_TAG)
> +		return error("%s: cannot verify a non-tag object of type %s.",
> +				name, typename(type));
> +
> +	buf = read_sha1_file(sha1, &type, &size);
> +	if (!buf)
> +		return error("%s: unable to read file.", name);
> +
> +	ret = run_gpg_verify(buf, size, 0);
> +
> +	return ret;

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?).

-Peff