> > 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.
>
> 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.
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html