surprising error message in parse_opt_with_commit

surprising error message in parse_opt_with_commit

[Tim Harper]

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?

Re: surprising error message in parse_opt_with_commit

[Shawn O. Pearce]

Tim Harper <timcharper@gmail.com> 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.

Re: surprising error message in parse_opt_with_commit

[Tim Harper]

On Thu, Aug 6, 2009 at 1:34 PM, Shawn O. Pearce<spearce@spearce.org> wrote:
> Tim Harper <timcharper@gmail.com> 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.
>

ok, that makes more sense.  I notice if I pass a full, mutated
40-character sha1 commit, I get the error message I expect.

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

[Tim Harper]

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

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

[Tim Harper]

On Thu, Aug 6, 2009 at 1:53 PM, Tim Harper<timcharper@gmail.com> 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?

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

[Junio C Hamano]

Tim Harper <timcharper@gmail.com> 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.