surprising error message in parse_opt_with_commit

When I typed 'git branch --contains efabdfb' on a machine today, I was surprised to receive this error message: "error: malformed object name efabdfb"

I would have expected instead to receive the message: "no such commit: efabdfb".

I went hunting through the source code and found the origination point of the error:

/parse-options.c
 610 int parse_opt_with_commit(const struct option *opt, const char
*arg, int unset)
 611 {
 612 	unsigned char sha1[20];
 613 	struct commit *commit;
 614
 615 	if (!arg)
 616 		return -1;
 617 	if (get_sha1(arg, sha1))
 618 		return error("malformed object name %s", arg);
 619 	commit = lookup_commit_reference(sha1);
 620 	if (!commit)
 621 		return error("no such commit %s", arg);
 622 	commit_list_insert(commit, opt->value);
 623 	return 0;
 624 }

It appears the get_sha1 call is returning true, causing the 'malformed object name' error to be returned. However, it seems that ideally since efabdfb is not malformed (it would be a valid ref if it existed), the execution path should continue to line 619, receive no commit, and fail on 621.

Am I off base here?

Tim Harper <timcharper@gmail.com> wrote:

get_sha1 is responsible for expanding an abbreviated ID to the full ID. If it can't do the expansion, it errors out. The code is correct as-is, though the error message on 618 is a bit odd.

When running the command 'git branch --contains efabdfb' on a repository that doesn't yet have efabdfb, git reports: "malformed object name efabdfb". To the uninitiated, this makes little sense (as far as they are concerned, efabdfb is perfectly formed).

This commit changes the message to "malformed object name or no such commit: efabdfb"
---
 parse-options.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

On Thu, Aug 6, 2009 at 1:53 PM, Tim Harper<timcharper@gmail.com> wrote:

Does nobody think this is a good idea?

Tim Harper <timcharper@gmail.com> writes:

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.