{"thread":{"id":"33897","subject":"[PATCH v2 0/2] Fix invalid revision error messages","startedAt":"2013-05-22T10:39:53Z","lastAt":"2013-05-23T11:03:21Z","messageCount":6,"participants":["Ramkumar Ramachandra","Junio C Hamano","Eric Sunshine"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"218159","messageId":"1369219195-20096-1-git-send-email-artagnon@gmail.com","threadId":"33897","inReplyTo":null,"subject":"[PATCH v2 0/2] Fix invalid revision error messages","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-22T10:39:53Z","receivedAt":"2013-05-22T10:39:53Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"As Junio pointed out in [0/2], this is not for 1.8.3; it's just a\nregular \"enhacement\".\n\nIn [1/2], I've extended the commit message with the justification I\nwrote out for Junio.\n\nIn [2/2], I've made sure to print the \"correct\" error message\neverytime: I missed the detached HEAD case last time.  I'm not in\nfavor of anything \"prettier\", as I already explained in my email.\n\nThanks.\n\nRamkumar Ramachandra (2):\n  sha1_name: fix error message for @{u}\n  sha1_name: fix error message for @{<N>}, @{<date>}\n\n sha1_name.c                   | 23 +++++++++++++++++------\n t/t1507-rev-parse-upstream.sh | 15 +++++----------\n 2 files changed, 22 insertions(+), 16 deletions(-)\n\n-- \n1.8.3.rc3.10.g6f8d616\n"},{"id":"218161","messageId":"1369219195-20096-2-git-send-email-artagnon@gmail.com","threadId":"33897","inReplyTo":"1369219195-20096-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/2] sha1_name: fix error message for @{u}","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-22T10:39:54Z","receivedAt":"2013-05-22T10:39:54Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Currently, when no (valid) upstream is configured for a branch, you get\nan error like:\n\n  $ git show @{u}\n  error: No upstream configured for branch 'upstream-error'\n  error: No upstream configured for branch 'upstream-error'\n  fatal: ambiguous argument '@{u}': unknown revision or path not in the working tree.\n  Use '--' to separate paths from revisions, like this:\n  'git <command> [<revision>...] -- [<file>...]'\n\nThe \"error: \" line actually appears twice, and the rest of the error\nmessage is useless.  In sha1_name.c:interpret_branch_name(), there is\nreally no point in processing further if @{u} couldn't be resolved, and\nwe might as well die() instead of returning an error().  After making\nthis change, you get:\n\n  $ git show @{u}\n  fatal: No upstream configured for branch 'upstream-error'\n\nAlso tweak a few tests in t1507 to expect this output.\n\nTo justify that this change is safe, consider that all callers of\ninterpret_branch_name() have to fall in two categories:\n\n1. Direct end-user facing applications like [rev-parse, show] calling in\n   with end-user data (in which case the data can contain \"@{u}\").\n   Failing immediately is the right thing to do: the only difference is\n   that the die() happens in interpret_branch_name() instead of\n   die_verify_filename(), and this is desirable.\n\n2. Callers calling in with programmatic data, and expecting the function\n   to return and not die().  In this case, why would anyone ever\n   construct a string containing \"@{u}\" programmatically in the first\n   place?  So, these callers can never hit the codepath touched by this\n   patch, and the change does not affect them.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n sha1_name.c                   | 13 +++++++------\n t/t1507-rev-parse-upstream.sh | 15 +++++----------\n 2 files changed, 12 insertions(+), 16 deletions(-)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 3820f28..416a673 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1033,14 +1033,15 @@ int interpret_branch_name(const char *name, struct strbuf *buf)\n \t * points to something different than a branch.\n \t */\n \tif (!upstream)\n-\t\treturn error(_(\"HEAD does not point to a branch\"));\n+\t\tdie(_(\"HEAD does not point to a branch\"));\n \tif (!upstream->merge || !upstream->merge[0]->dst) {\n \t\tif (!ref_exists(upstream->refname))\n-\t\t\treturn error(_(\"No such branch: '%s'\"), cp);\n-\t\tif (!upstream->merge)\n-\t\t\treturn error(_(\"No upstream configured for branch '%s'\"),\n-\t\t\t\t     upstream->name);\n-\t\treturn error(\n+\t\t\tdie(_(\"No such branch: '%s'\"), cp);\n+\t\tif (!upstream->merge) {\n+\t\t\tdie(_(\"No upstream configured for branch '%s'\"),\n+\t\t\t\tupstream->name);\n+\t\t}\n+\t\tdie(\n \t\t\t_(\"Upstream branch '%s' not stored as a remote-tracking branch\"),\n \t\t\tupstream->merge[0]->src);\n \t}\ndiff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\nindex b27a720..2a19e79 100755\n--- a/t/t1507-rev-parse-upstream.sh\n+++ b/t/t1507-rev-parse-upstream.sh\n@@ -129,8 +129,7 @@ test_expect_success 'branch@{u} works when tracking a local branch' '\n \n test_expect_success 'branch@{u} error message when no upstream' '\n \tcat >expect <<-EOF &&\n-\terror: No upstream configured for branch ${sq}non-tracking${sq}\n-\tfatal: Needed a single revision\n+\tfatal: No upstream configured for branch ${sq}non-tracking${sq}\n \tEOF\n \terror_message non-tracking@{u} 2>actual &&\n \ttest_i18ncmp expect actual\n@@ -138,8 +137,7 @@ test_expect_success 'branch@{u} error message when no upstream' '\n \n test_expect_success '@{u} error message when no upstream' '\n \tcat >expect <<-EOF &&\n-\terror: No upstream configured for branch ${sq}master${sq}\n-\tfatal: Needed a single revision\n+\tfatal: No upstream configured for branch ${sq}master${sq}\n \tEOF\n \ttest_must_fail git rev-parse --verify @{u} 2>actual &&\n \ttest_i18ncmp expect actual\n@@ -147,8 +145,7 @@ test_expect_success '@{u} error message when no upstream' '\n \n test_expect_success 'branch@{u} error message with misspelt branch' '\n \tcat >expect <<-EOF &&\n-\terror: No such branch: ${sq}no-such-branch${sq}\n-\tfatal: Needed a single revision\n+\tfatal: No such branch: ${sq}no-such-branch${sq}\n \tEOF\n \terror_message no-such-branch@{u} 2>actual &&\n \ttest_i18ncmp expect actual\n@@ -156,8 +153,7 @@ test_expect_success 'branch@{u} error message with misspelt branch' '\n \n test_expect_success '@{u} error message when not on a branch' '\n \tcat >expect <<-EOF &&\n-\terror: HEAD does not point to a branch\n-\tfatal: Needed a single revision\n+\tfatal: HEAD does not point to a branch\n \tEOF\n \tgit checkout HEAD^0 &&\n \ttest_must_fail git rev-parse --verify @{u} 2>actual &&\n@@ -166,8 +162,7 @@ test_expect_success '@{u} error message when not on a branch' '\n \n test_expect_success 'branch@{u} error message if upstream branch not fetched' '\n \tcat >expect <<-EOF &&\n-\terror: Upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch\n-\tfatal: Needed a single revision\n+\tfatal: Upstream branch ${sq}refs/heads/side${sq} not stored as a remote-tracking branch\n \tEOF\n \terror_message bad-upstream@{u} 2>actual &&\n \ttest_i18ncmp expect actual\n-- \n1.8.3.rc3.10.g6f8d616\n"},{"id":"218160","messageId":"1369219195-20096-3-git-send-email-artagnon@gmail.com","threadId":"33897","inReplyTo":"1369219195-20096-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/2] sha1_name: fix error message for @{<N>}, @{<date>}","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-22T10:39:55Z","receivedAt":"2013-05-22T10:39:55Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Currently, when we try to resolve @{<N>} or @{<date>} when the reflog\ndoesn't go back far enough, we get errors like:\n\n  # on branch master\n  $ git show @{10000}\n  fatal: Log for '' only has 7 entries.\n\n  $ git show @{10000.days.ago}\n  warning: Log for '' only goes back to Tue, 21 May 2013 14:14:45 +0530.\n  ...\n\n  # detached HEAD case\n  $ git show @{10000}\n  fatal: Log for '' only has 2005 entries.\n\n  $ git show master@{10000}\n  fatal: Log for 'master' only has 7 entries.\n\nThe empty string '' is ugly, inconsistent, and failing to convey\ninformation about whose logs we are inspecting.  Change this so that we\nget:\n\n  # on branch master\n  $ git show @{10000}\n  fatal: Log for 'master' only has 7 entries.\n\n  $ git show @{10000.days.ago}\n  warning: Log for 'master' only goes back to Tue, 21 May 2013 14:14:45 +0530.\n  ...\n\n  # detached HEAD case\n  $ git show @{10000}\n  fatal: Log for 'HEAD' only has 2005 entries.\n\n  $ git show master@{10000}\n  fatal: Log for 'master' only has 7 entries.\n\nSimple, consistent, and informative; suitable for output even from\nplumbing commands like rev-parse.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n sha1_name.c | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 416a673..b7e008a 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -517,6 +517,16 @@ static int get_sha1_basic(const char *str, int len, unsigned char *sha1)\n \t\t}\n \t\tif (read_ref_at(real_ref, at_time, nth, sha1, NULL,\n \t\t\t\t&co_time, &co_tz, &co_cnt)) {\n+\t\t\tif (!len) {\n+\t\t\t\tif (!prefixcmp(real_ref, \"refs/heads/\")) {\n+\t\t\t\t\tstr = real_ref + 11;\n+\t\t\t\t\tlen = strlen(real_ref + 11);\n+\t\t\t\t} else {\n+\t\t\t\t\t/* detached HEAD */\n+\t\t\t\t\tstr = \"HEAD\";\n+\t\t\t\t\tlen = 4;\n+\t\t\t\t}\n+\t\t\t}\n \t\t\tif (at_time)\n \t\t\t\twarning(\"Log for '%.*s' only goes \"\n \t\t\t\t\t\"back to %s.\", len, str,\n-- \n1.8.3.rc3.10.g6f8d616\n"},{"id":"218195","messageId":"7vhahukjeg.fsf@alter.siamese.dyndns.org","threadId":"33897","inReplyTo":"1369219195-20096-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 1/2] sha1_name: fix error message for @{u}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-22T17:35:03Z","receivedAt":"2013-05-22T17:35:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra <artagnon@gmail.com> writes:\n\n> 2. Callers calling in with programmatic data, and expecting the function\n>    to return and not die().  In this case, why would anyone ever\n>    construct a string containing \"@{u}\" programmatically in the first\n>    place?\n\nIf you have to ask why, and cannot answer the question yourself,\nthen you would not know if such a caller exists.  After a code\naudit, I know there is no such caller that appends @{u} but if you\nwere writing a quick-and-dirty caller, I would not be surprised if\nyou find it more convenient to form a textual extended SHA-1\nexpression and have get_sha1() do its work, instead of asking the\nsame question programmatically.\n\nIn this case, I think you already checked there is no such problem,\nand it is a more straight-forward justification to say that you did\na code-audit and made sure that all the callers that used to hit one\nof these errors() want to die().\n\nAlso such a caller, if existed, would either\n\n    (1) want to die itself, in which case these error() messages are\n        superfluous; or\n\n    (2) want to continue (possibly dying with its own message), in\n        which case these error() messages are unwanted.\n\nBecause you are changing only the existing call sites of error()\ninto die(), and not changing silent -1 returns to die(), this change\nis safe for both kinds of such callers, I think.\n\n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n> ---\n>  sha1_name.c                   | 13 +++++++------\n>  t/t1507-rev-parse-upstream.sh | 15 +++++----------\n>  2 files changed, 12 insertions(+), 16 deletions(-)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 3820f28..416a673 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -1033,14 +1033,15 @@ int interpret_branch_name(const char *name, struct strbuf *buf)\n>  \t * points to something different than a branch.\n>  \t */\n>  \tif (!upstream)\n> -\t\treturn error(_(\"HEAD does not point to a branch\"));\n> +\t\tdie(_(\"HEAD does not point to a branch\"));\n\nOK.\n\n>  \tif (!upstream->merge || !upstream->merge[0]->dst) {\n>  \t\tif (!ref_exists(upstream->refname))\n> +\t\t\tdie(_(\"No such branch: '%s'\"), cp);\n\nOK.\n\n> +\t\tif (!upstream->merge) {\n> +\t\t\tdie(_(\"No upstream configured for branch '%s'\"),\n> +\t\t\t\tupstream->name);\n> +\t\t}\n\nOK, but I would not add extra {} if I were doing this change.\n\n> +\t\tdie(\n>  \t\t\t_(\"Upstream branch '%s' not stored as a remote-tracking branch\"),\n>  \t\t\tupstream->merge[0]->src);\n\nOK, but I would fix the indentation while at it if I were doing this change.\n\n> diff --git a/t/t1507-rev-parse-upstream.sh b/t/t1507-rev-parse-upstream.sh\n> index b27a720..2a19e79 100755\n> --- a/t/t1507-rev-parse-upstream.sh\n> +++ b/t/t1507-rev-parse-upstream.sh\n> @@ -129,8 +129,7 @@ test_expect_success 'branch@{u} works when tracking a local branch' '\n>  \n>  test_expect_success 'branch@{u} error message when no upstream' '\n>  \tcat >expect <<-EOF &&\n> -\terror: No upstream configured for branch ${sq}non-tracking${sq}\n> -\tfatal: Needed a single revision\n> +\tfatal: No upstream configured for branch ${sq}non-tracking${sq}\n\nMuch nicer.\n"},{"id":"218208","messageId":"CAPig+cQwcNxvvXZ_PFtUZ3HnH5tdpsL8RLQ02dTDz37y0XpJhg@mail.gmail.com","threadId":"33897","inReplyTo":"1369219195-20096-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/2] sha1_name: fix error message for @{<N>}, @{<date>}","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-22T20:39:54Z","receivedAt":"2013-05-22T20:39:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, May 22, 2013 at 6:39 AM, Ramkumar Ramachandra\n<artagnon@gmail.com> wrote:\n> Currently, when we try to resolve @{<N>} or @{<date>} when the reflog\n> doesn't go back far enough, we get errors like:\n>\n>   # on branch master\n>   $ git show @{10000}\n>   fatal: Log for '' only has 7 entries.\n>\n>   $ git show @{10000.days.ago}\n>   warning: Log for '' only goes back to Tue, 21 May 2013 14:14:45 +0530.\n>   ...\n>\n>   # detached HEAD case\n>   $ git show @{10000}\n>   fatal: Log for '' only has 2005 entries.\n>\n>   $ git show master@{10000}\n>   fatal: Log for 'master' only has 7 entries.\n>\n> The empty string '' is ugly, inconsistent, and failing to convey\n\ns/failing/fails/\n\n> information about whose logs we are inspecting.  Change this so that we\n> get:\n>\n>   # on branch master\n>   $ git show @{10000}\n>   fatal: Log for 'master' only has 7 entries.\n>\n>   $ git show @{10000.days.ago}\n>   warning: Log for 'master' only goes back to Tue, 21 May 2013 14:14:45 +0530.\n>   ...\n>\n>   # detached HEAD case\n>   $ git show @{10000}\n>   fatal: Log for 'HEAD' only has 2005 entries.\n>\n>   $ git show master@{10000}\n>   fatal: Log for 'master' only has 7 entries.\n>\n> Simple, consistent, and informative; suitable for output even from\n> plumbing commands like rev-parse.\n>\n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n"},{"id":"218255","messageId":"CALkWK0kBzsti=gqu6G1mGLA3cc5kv7+hc0=vgA7UGw2REXRi1w@mail.gmail.com","threadId":"33897","inReplyTo":"7vhahukjeg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/2] sha1_name: fix error message for @{u}","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-23T11:03:21Z","receivedAt":"2013-05-23T11:03:21Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> If you have to ask why, and cannot answer the question yourself,\n> then you would not know if such a caller exists.  After a code\n> audit, I know there is no such caller that appends @{u} but if you\n> were writing a quick-and-dirty caller, I would not be surprised if\n> you find it more convenient to form a textual extended SHA-1\n> expression and have get_sha1() do its work, instead of asking the\n> same question programmatically.\n\nI'll mention that a simple grep for \"@{u}\" and \"@{upstream}\" found nothing.\n\n> In this case, I think you already checked there is no such problem,\n> and it is a more straight-forward justification to say that you did\n> a code-audit and made sure that all the callers that used to hit one\n> of these errors() want to die().\n\nIt's not about callers eventually wanting to die(); it's about whether\ncallers want to die() without doing anything useful after getting this\n-1.  And that is impossible for me to say with confidence, unless I do\na _very_ extensive code review (which I didn't do).\n\n> Also such a caller, if existed, would either\n>\n>     (1) want to die itself, in which case these error() messages are\n>         superfluous; or\n>\n>     (2) want to continue (possibly dying with its own message), in\n>         which case these error() messages are unwanted.\n>\n> Because you are changing only the existing call sites of error()\n> into die(), and not changing silent -1 returns to die(), this change\n> is safe for both kinds of such callers, I think.\n\nTake the example of git branch (-vv) output: let's imagine a universe\nin which it determines upstream by calling in with a hard-coded \"@{u}\"\nstring.  Should the entire program die() and stop printing the rest of\nthe branches?  Ofcourse not.  Is your argument that no caller should\ndo this in the first place, because of spurious error() messages\npolluting the output (of git branch)?  How is this argument stronger\nthan my grep for \"@{u}\"?\n\n>> +             die(\n>>                       _(\"Upstream branch '%s' not stored as a remote-tracking branch\"),\n>>                       upstream->merge[0]->src);\n>\n> OK, but I would fix the indentation while at it if I were doing this change.\n\nBut my Emacs reports that the indentation is correct.  Did you mean:\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex b7e008a..b00ea0f 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1051,8 +1051,7 @@ int interpret_branch_name(const char *name,\nstruct strbuf *buf)\n \t\t\tdie(_(\"No upstream configured for branch '%s'\"),\n \t\t\t\tupstream->name);\n \t\t}\n-\t\tdie(\n-\t\t\t_(\"Upstream branch '%s' not stored as a remote-tracking branch\"),\n+\t\tdie(_(\"Upstream branch '%s' not stored as a remote-tracking branch\"),\n \t\t\tupstream->merge[0]->src);\n \t}\n \tfree(cp);\n\n\nYeah, I'll do this.\n"}]}