From: Junio C Hamano Date: Fri, 07 Aug 2009 05:36:41 GMT Subject: Re: [PATCH] clarify error message when an abbreviated non-existent commit was specified Message-ID: <7vk51g5gnq.fsf@alter.siamese.dyndns.org> In-Reply-To: Tim Harper writes: >> 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.