Re: [PATCH v5 4/6] builtin/verify-tag: replace name argument with sha1

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 6, 2016, 16:27 UTC
Message-ID: <xmqqa8l6zoqc.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1459872449-7537-5-git-send-email-santiago@nyu.edu>

santiago@nyu.edu writes:

Show 40 quoted lines

> From: Santiago Torres <santiago@nyu.edu>
>
> This change is meant to prepare verify_tag for libification. Many
> existing modules/commands already do the refname to sha1 resolution, so
> should avoid resolving the refname twice. To avoid breaking
> builtin/verify-tag, we move the refname resolution outside of the
> verify_tag() call.
>
> Signed-off-by: Santiago Torres <santiago@nyu.edu>
> ---
>  builtin/verify-tag.c | 25 ++++++++++++++++---------
>  1 file changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index 1ca9a05..7a7c376 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -42,25 +42,23 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
>  	return ret;
>  }
>  
> -static int verify_tag(const char *name, unsigned flags)
> +static int verify_tag(const unsigned char *sha1, unsigned flags)
>  {
>  	enum object_type type;
> -	unsigned char sha1[20];
>  	char *buf;
> +	char *hex_sha1;
>  	unsigned long size;
>  	int ret;
>  
> -	if (get_sha1(name, sha1))
> -		return error("tag '%s' not found.", name);
> -
> +	hex_sha1 = sha1_to_hex(sha1);
>  	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));
> +				hex_sha1, typename(type));

So, if I said

    $ git verify-tag master

the code used to take "master" from argv[], fed it to verify_tag() as parameter 'name', turned it to the object name of the commit, noticed that it is not a tag, and complained that "master: cannot verify".

With this rewrite, the same invocation would cause "master" to be turned into the object name, which is passed to verify_tag() and the complaint is an overlong

    76bece327f490cb344b95ae8f869cbeb89a4d20b: cannot verify a non-tag object of type commit

That does not sound like a good change at all.

If you want to support a future caller of a libified version of verify_tag() that has a raw object name but not the original name, I'd suggest to make this function keep parameter 'name' while adding the new parameter 'sha1'. Then, the error reporting may become:

	return error("%s: cannot verify a non-tag object of type '%s'",
		     name ? name : sha1_to_hex(sha1), typename(type));

and the output would still be useful.  Further improvements may be

 - rename 'name' to 'report_name' to clarify that the parameter is
   only used for reporting, and that the tag object to verify is
   always identified by the new 'sha1' parameter.

 - use find_unique_abbrev() to shorten the fallback name displayed
   in the error message.