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

From: Jeff King <peff@peff.net>
Date: Mar 24, 2016, 22:10 UTC
Message-ID: <20160324221020.GA17805@sigill.intra.peff.net>
In-Reply-To: <1458855560-28519-1-git-send-email-santiago@nyu.edu>

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

Show 9 quoted lines

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

Show 6 quoted lines

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

Show 6 quoted lines

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

Show 17 quoted lines

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