{"thread":{"id":"20445","subject":"surprising error message in parse_opt_with_commit","startedAt":"2009-08-06T19:27:25Z","lastAt":"2009-08-07T05:36:41Z","messageCount":5,"participants":["Tim Harper","Shawn O. Pearce","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"119819","messageId":"e1a5e9a00908061227n67a878f5tb2d5130582b4fd44@mail.gmail.com","threadId":"20445","inReplyTo":null,"subject":"surprising error message in parse_opt_with_commit","fromName":"Tim Harper","fromEmail":"timcharper@gmail.com","sentAt":"2009-08-06T19:27:25Z","receivedAt":"2009-08-06T19:27:25Z","isPatch":false,"sender":{"key":"timcharper@gmail.com","avatar":"https://gravatar.com/avatar/1a2e0c06c7862ff065ee6b1d53195333a5a0577c040ecb2856a150d8e0b00ecd?d=mp&s=160"},"body":"When I typed 'git branch --contains efabdfb' on a machine today, I was\nsurprised to receive this error message: \"error: malformed object name\nefabdfb\"\n\nI would have expected instead to receive the message: \"no such commit: efabdfb\".\n\nI went hunting through the source code and found the origination point\nof the error:\n\n/parse-options.c\n 610 int parse_opt_with_commit(const struct option *opt, const char\n*arg, int unset)\n 611 {\n 612 \tunsigned char sha1[20];\n 613 \tstruct commit *commit;\n 614\n 615 \tif (!arg)\n 616 \t\treturn -1;\n 617 \tif (get_sha1(arg, sha1))\n 618 \t\treturn error(\"malformed object name %s\", arg);\n 619 \tcommit = lookup_commit_reference(sha1);\n 620 \tif (!commit)\n 621 \t\treturn error(\"no such commit %s\", arg);\n 622 \tcommit_list_insert(commit, opt->value);\n 623 \treturn 0;\n 624 }\n\nIt appears the get_sha1 call is returning true, causing the 'malformed\nobject name' error to be returned.  However, it seems that ideally\nsince efabdfb is not malformed (it would be a valid ref if it\nexisted), the execution path should continue to line 619, receive no\ncommit, and fail on 621.\n\nAm I off base here?\n"},{"id":"119820","messageId":"20090806193413.GJ1033@spearce.org","threadId":"20445","inReplyTo":"e1a5e9a00908061227n67a878f5tb2d5130582b4fd44@mail.gmail.com","subject":"Re: surprising error message in parse_opt_with_commit","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-06T19:34:14Z","receivedAt":"2009-08-06T19:34:14Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Tim Harper <timcharper@gmail.com> wrote:\n>  610 int parse_opt_with_commit(const struct option *opt, const char\n> *arg, int unset)\n>  611 {\n>  612 \tunsigned char sha1[20];\n>  613 \tstruct commit *commit;\n>  614\n>  615 \tif (!arg)\n>  616 \t\treturn -1;\n>  617 \tif (get_sha1(arg, sha1))\n>  618 \t\treturn error(\"malformed object name %s\", arg);\n>  619 \tcommit = lookup_commit_reference(sha1);\n>  620 \tif (!commit)\n>  621 \t\treturn error(\"no such commit %s\", arg);\n>  622 \tcommit_list_insert(commit, opt->value);\n>  623 \treturn 0;\n>  624 }\n> \n> It appears the get_sha1 call is returning true, causing the 'malformed\n> object name' error to be returned.  However, it seems that ideally\n> since efabdfb is not malformed (it would be a valid ref if it\n> existed), the execution path should continue to line 619, receive no\n> commit, and fail on 621.\n\nget_sha1 is responsible for expanding an abbreviated ID to the\nfull ID.  If it can't do the expansion, it errors out.  The code\nis correct as-is, though the error message on 618 is a bit odd.\n\n-- \nShawn.\n"},{"id":"119824","messageId":"1249588435-23400-1-git-send-email-timcharper@gmail.com","threadId":"20445","inReplyTo":"20090806193413.GJ1033@spearce.org","subject":"[PATCH] clarify error message when an abbreviated non-existent commit was specified","fromName":"Tim Harper","fromEmail":"timcharper@gmail.com","sentAt":"2009-08-06T19:53:55Z","receivedAt":"2009-08-06T19:53:55Z","isPatch":true,"sender":{"key":"timcharper@gmail.com","avatar":"https://gravatar.com/avatar/1a2e0c06c7862ff065ee6b1d53195333a5a0577c040ecb2856a150d8e0b00ecd?d=mp&s=160"},"body":"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).\n\nThis commit changes the message to \"malformed object name or no such commit: efabdfb\"\n---\n parse-options.c |    2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 3b71fbb..95eb1c4 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -615,7 +615,7 @@ int parse_opt_with_commit(const struct option *opt, const char *arg, int unset)\n \tif (!arg)\n \t\treturn -1;\n \tif (get_sha1(arg, sha1))\n-\t\treturn error(\"malformed object name %s\", arg);\n+\t\treturn error(\"malformed object name or no such commit: %s\", arg);\n \tcommit = lookup_commit_reference(sha1);\n \tif (!commit)\n \t\treturn error(\"no such commit %s\", arg);\n-- \n1.6.4\n"},{"id":"119886","messageId":"e1a5e9a00908062217q4bd1ecafm5fd5e060aecfa467@mail.gmail.com","threadId":"20445","inReplyTo":"1249588435-23400-1-git-send-email-timcharper@gmail.com","subject":"Re: [PATCH] clarify error message when an abbreviated non-existent commit was specified","fromName":"Tim Harper","fromEmail":"timcharper@gmail.com","sentAt":"2009-08-07T05:17:48Z","receivedAt":"2009-08-07T05:17:48Z","isPatch":true,"sender":{"key":"timcharper@gmail.com","avatar":"https://gravatar.com/avatar/1a2e0c06c7862ff065ee6b1d53195333a5a0577c040ecb2856a150d8e0b00ecd?d=mp&s=160"},"body":"On Thu, Aug 6, 2009 at 1:53 PM, Tim Harper<timcharper@gmail.com> wrote:\n> 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).\n>\n> This commit changes the message to \"malformed object name or no such commit: efabdfb\"\n> ---\n>  parse-options.c |    2 +-\n>  1 files changed, 1 insertions(+), 1 deletions(-)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index 3b71fbb..95eb1c4 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -615,7 +615,7 @@ int parse_opt_with_commit(const struct option *opt, const char *arg, int unset)\n>        if (!arg)\n>                return -1;\n>        if (get_sha1(arg, sha1))\n> -               return error(\"malformed object name %s\", arg);\n> +               return error(\"malformed object name or no such commit: %s\", arg);\n>        commit = lookup_commit_reference(sha1);\n>        if (!commit)\n>                return error(\"no such commit %s\", arg);\n> --\n> 1.6.4\n>\n>\n\nDoes nobody think this is a good idea?\n"},{"id":"119890","messageId":"7vk51g5gnq.fsf@alter.siamese.dyndns.org","threadId":"20445","inReplyTo":"e1a5e9a00908062217q4bd1ecafm5fd5e060aecfa467@mail.gmail.com","subject":"Re: [PATCH] clarify error message when an abbreviated non-existent commit was specified","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-07T05:36:41Z","receivedAt":"2009-08-07T05:36:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Harper <timcharper@gmail.com> writes:\n\n>> diff --git a/parse-options.c b/parse-options.c\n>> index 3b71fbb..95eb1c4 100644\n>> --- a/parse-options.c\n>> +++ b/parse-options.c\n>> @@ -615,7 +615,7 @@ int parse_opt_with_commit(const struct option *opt, const char *arg, int unset)\n>>        if (!arg)\n>>                return -1;\n>>        if (get_sha1(arg, sha1))\n>> -               return error(\"malformed object name %s\", arg);\n>> +               return error(\"malformed object name or no such commit: %s\", arg);\n>>        commit = lookup_commit_reference(sha1);\n>>        if (!commit)\n>>                return error(\"no such commit %s\", arg);\n>> --\n>> 1.6.4\n>\n> Does nobody think this is a good idea?\n\nProbably people don't care enough.  I certainly didn't pay much attention\nto the discussion on a rather trivial patch that was not yet signed off.\n\nI'd probably write along this line instead, if I cared enough.  \n\n\tif (get_sha1(arg, sha1) ||\n            !(commit = lookup_commit_reference(sha1)))\n\t\treturn error(\"no such commit: %s\", arg);\n\nI think the important part of the message is that whatever the user gave\nus when we expected to see a string that names a commit was not a commit;\nit is immaterial if the failure was because an abbreviated hexadecimal\nform was mistyped (get_sha1() would fail in this case) or because a tag\nthat points at a non commit, e.g. \"v2.6.11-tree\", was given (l-c-r will\nfail in that case).\n\nGiving two different messages depending on the nature of an error will\nhelp debugging parse_opt_with_commit(), but that benefit is secondary.\n"}]}