Re: [PATCH] clarify error message when an abbreviated non-existent commit was specified

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 7, 2009, 05:36 UTC
Message-ID: <7vk51g5gnq.fsf@alter.siamese.dyndns.org>
In-Reply-To: <e1a5e9a00908062217q4bd1ecafm5fd5e060aecfa467@mail.gmail.com>

Tim Harper <timcharper@gmail.com> writes:

Show 17 quoted lines

>> diff --git a/parse-options.c b/parse-options.c
>> index 3b71fbb..95eb1c4 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -615,7 +615,7 @@ int parse_opt_with_commit(const struct option *opt, const char *arg, int unset)
>>        if (!arg)
>>                return -1;
>>        if (get_sha1(arg, sha1))
>> -               return error("malformed object name %s", arg);
>> +               return error("malformed object name or no such commit: %s", arg);
>>        commit = lookup_commit_reference(sha1);
>>        if (!commit)
>>                return error("no such commit %s", arg);
>> --
>> 1.6.4
>
> Does nobody think this is a good idea?

Probably people don't care enough. I certainly didn't pay much attention to the discussion on a rather trivial patch that was not yet signed off.

I'd probably write along this line instead, if I cared enough.

	if (get_sha1(arg, sha1) ||
            !(commit = lookup_commit_reference(sha1)))
		return error("no such commit: %s", arg);

I think the important part of the message is that whatever the user gave us when we expected to see a string that names a commit was not a commit; it is immaterial if the failure was because an abbreviated hexadecimal form was mistyped (get_sha1() would fail in this case) or because a tag that points at a non commit, e.g. "v2.6.11-tree", was given (l-c-r will fail in that case).

Giving two different messages depending on the nature of an error will help debugging parse_opt_with_commit(), but that benefit is secondary.