# surprising error message in parse_opt_with_commit 5 messages from 2009-08-06 to 2009-08-07. Participants: Tim Harper, Shawn O. Pearce, Junio C Hamano. Thread: https://gitlist.dev/t/20445 ## Tim Harper, 2009-08-06 19:27 Subject: surprising error message in parse_opt_with_commit Message-ID: URL: https://gitlist.dev/e/e1a5e9a00908061227n67a878f5tb2d5130582b4fd44%40mail.gmail.com ``` 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? ``` ## Shawn O. Pearce, 2009-08-06 19:34 Subject: Re: surprising error message in parse_opt_with_commit Message-ID: <20090806193413.GJ1033@spearce.org> URL: https://gitlist.dev/e/20090806193413.GJ1033%40spearce.org In-Reply-To: ``` Tim Harper wrote: > 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. 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. -- Shawn. ``` ## Tim Harper, 2009-08-06 19:53 Subject: [PATCH] clarify error message when an abbreviated non-existent commit was specified Message-ID: <1249588435-23400-1-git-send-email-timcharper@gmail.com> URL: https://gitlist.dev/e/1249588435-23400-1-git-send-email-timcharper%40gmail.com In-Reply-To: <20090806193413.GJ1033@spearce.org> ``` 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(-) 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 ``` ## Tim Harper, 2009-08-07 05:17 Subject: Re: [PATCH] clarify error message when an abbreviated non-existent commit was specified Message-ID: URL: https://gitlist.dev/e/e1a5e9a00908062217q4bd1ecafm5fd5e060aecfa467%40mail.gmail.com In-Reply-To: <1249588435-23400-1-git-send-email-timcharper@gmail.com> ``` On Thu, Aug 6, 2009 at 1:53 PM, Tim Harper wrote: > 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(-) > > 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? ``` ## Junio C Hamano, 2009-08-07 05:36 Subject: Re: [PATCH] clarify error message when an abbreviated non-existent commit was specified Message-ID: <7vk51g5gnq.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vk51g5gnq.fsf%40alter.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. ```