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

From: Santiago Torres <santiago@nyu.edu>
Date: Apr 7, 2016, 03:38 UTC
Message-ID: <20160407033844.GB17848@LykOS>
In-Reply-To: <xmqqa8l6zoqc.fsf@gitster.mtv.corp.google.com>

Show 21 quoted lines

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

Yep, I agree. At least I believe that we can do better in terms of user feedback.

Show 9 quoted lines

> 
> 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));
>

Yep, I'll do some experimenting with this.

Show 5 quoted lines

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

This seems to be fundamental. As it was suggested before, making it clear that name is only for feedback purposes is really necessary.

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

I think we can do both. Let me try this out.

Would you suggest having these changes in a separate patch?

Thanks! -Santiago.