{"thread":{"id":"33887","subject":"[PATCH 1/2] sha1_name: fix error message for @{u}","startedAt":"2013-05-21T10:41:53Z","lastAt":"2013-05-21T20:33:31Z","messageCount":19,"participants":["Ramkumar Ramachandra","Junio C Hamano","Kevin Bracey"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"218043","messageId":"1369132915-25657-1-git-send-email-artagnon@gmail.com","threadId":"33887","inReplyTo":null,"subject":"[PATCH 0/2] Fix invalid revision error messages for 1.8.3","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-21T10:41:53Z","receivedAt":"2013-05-21T10:41:53Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi,\n\nSeeing other patches on the list, I decided that I should do something\nfor 1.8.3 as well (as opposed to constantly writing new features).  So\nhere's my contribution.\n\nThe first error message has annoyed me endlessly, and I took this\nopportunity to fix it.  Interested people can sprinkle in some advice\nlater.  The second one is a low-hanging \"while we're there\".\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                   | 17 +++++++++++------\n t/t1507-rev-parse-upstream.sh | 15 +++++----------\n 2 files changed, 16 insertions(+), 16 deletions(-)\n\n-- \n1.8.3.rc3.6.ga9126d5.dirty\n"},{"id":"218042","messageId":"1369132915-25657-2-git-send-email-artagnon@gmail.com","threadId":"33887","inReplyTo":"1369132915-25657-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-21T10:41:54Z","receivedAt":"2013-05-21T10:41: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, we 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\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.6.ga9126d5.dirty\n"},{"id":"218044","messageId":"1369132915-25657-3-git-send-email-artagnon@gmail.com","threadId":"33887","inReplyTo":"1369132915-25657-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-21T10:41:55Z","receivedAt":"2013-05-21T10:41: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\nfor the current branch doesn't go back far enough, we get errors like:\n\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\nThe empty string '' looks ugly and inconsistent with the output of\n<branch>@{<N>}.  Replace it with the string 'current branch'.\n\nSigned-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n---\n sha1_name.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 416a673..683b4bd 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -517,6 +517,10 @@ 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\tstr = \"current branch\";\n+\t\t\t\tlen = strlen(\"current branch\");\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.6.ga9126d5.dirty\n"},{"id":"218057","messageId":"7vy5b8p9wm.fsf@alter.siamese.dyndns.org","threadId":"33887","inReplyTo":"1369132915-25657-1-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 0/2] Fix invalid revision error messages for 1.8.3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-21T16:36:41Z","receivedAt":"2013-05-21T16:36:41Z","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> Seeing other patches on the list, I decided that I should do something\n> for 1.8.3 as well\n\nFixes to something that are broken the same way between 'master' and\nolder release versions are the same as enhancements (which you can\nview as \"fix to lack of feature\").  They are not regression fixes\nand not for 1.8.3 at this point in the cycle, deep into -rc.\n"},{"id":"218058","messageId":"7vtxlwp9mf.fsf@alter.siamese.dyndns.org","threadId":"33887","inReplyTo":"1369132915-25657-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-21T16:42:48Z","receivedAt":"2013-05-21T16:42:48Z","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> Currently, when no (valid) upstream is configured for a branch, we get\n> an 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>\n> The \"error: \" line actually appears twice, and the rest of the error\n> message is useless.  In sha1_name.c:interpret_branch_name(), there is\n> really no point in processing further if @{u} couldn't be resolved, and\n> we might as well die() instead of returning an error().  After making\n> this change, you get:\n>\n>   $ git show @{u}\n>   fatal: No upstream configured for branch 'upstream-error'\n>\n> Also tweak a few tests in t1507 to expect this output.\n\nDoes a failure in interpret-branch-name that issue these error\nmessages always followed by die() in the caller?  I know you looked\nat the cases you noticed as an end-user (like the above \"git show @{u}\"\nexample), but if some codepaths did this:\n\n\tif (interpret-branch-name()) {\n        \tyou do not seem to have upstream defined,\n\t        so I will helpfully do something else that\n                you probably have meant.\n\t}\n\nthis patch will break that codepath you did not look.\n\nI do not offhand know if there is such a codepath, so if you did a\ncode audit and know this patch is regression-free, please say that\nin the log message.  \"I ran all the tests and they passed\" is not\ngood enough.\n\nOther than that, the idea sounds OK.\n\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>  \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}\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>  \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"},{"id":"218059","messageId":"7vppwkp961.fsf@alter.siamese.dyndns.org","threadId":"33887","inReplyTo":"1369132915-25657-3-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 2/2] sha1_name: fix error message for @{<N>}, @{<date>}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-21T16:52:38Z","receivedAt":"2013-05-21T16:52:38Z","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> Currently, when we try to resolve @{<N>} or @{<date>} when the reflog\n> for the current branch doesn't go back far enough, we get errors like:\n>\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> The empty string '' looks ugly and inconsistent with the output of\n> <branch>@{<N>}.  Replace it with the string 'current branch'.\n\nWouldn't that be '*the* current branch'?\n\nMore importantly, doesn't \"real_ref\" have the name of the branch?\n\nSuppose the user said \"git show @{10000}\" instead of \"git show\nmaster@{10000}\" while on 'master'.\n\nIt could be argued that it may look nicer to say \"your current\nbranch does not have enough update history\" instead of saying\n\"master does not...\" (i.e. different input to ask for the same\nthing, different output depending on the way the user asked).  It\nalso could be argued that they should produce the same diagnosis\nthat is more informative.\n\nI am slightly leaning toward the latter.\n\n> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>\n> ---\n>  sha1_name.c | 4 ++++\n>  1 file changed, 4 insertions(+)\n>\n> diff --git a/sha1_name.c b/sha1_name.c\n> index 416a673..683b4bd 100644\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -517,6 +517,10 @@ 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\tstr = \"current branch\";\n> +\t\t\t\tlen = strlen(\"current branch\");\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"},{"id":"218069","messageId":"519BB104.9060802@bracey.fi","threadId":"33887","inReplyTo":"7vppwkp961.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] sha1_name: fix error message for @{<N>}, @{<date>}","fromName":"Kevin Bracey","fromEmail":"kevin@bracey.fi","sentAt":"2013-05-21T17:38:12Z","receivedAt":"2013-05-21T17:38:12Z","isPatch":true,"sender":{"key":"kevin@bracey.fi","avatar":"https://avatars.githubusercontent.com/u/96079793?v=4"},"body":"On 21/05/2013 19:52, Junio C Hamano wrote:\n> Ramkumar Ramachandra <artagnon@gmail.com> writes:\n>\n>> The empty string '' looks ugly and inconsistent with the output of\n>> <branch>@{<N>}.  Replace it with the string 'current branch'.\n> Wouldn't that be '*the* current branch'?\n>\n> More importantly, doesn't \"real_ref\" have the name of the branch?\n>\n> Suppose the user said \"git show @{10000}\" instead of \"git show\n> master@{10000}\" while on 'master'.\n>\n> It could be argued that it may look nicer to say \"your current\n> branch does not have enough update history\" instead of saying\n> \"master does not...\" (i.e. different input to ask for the same\n> thing, different output depending on the way the user asked).  It\n> also could be argued that they should produce the same diagnosis\n> that is more informative.\n>\n> I am slightly leaning toward the latter.\nThat would also avoid the complaint I was about to make that putting \n'current branch' in scare quotes would be annoying.\n\nKevin\n"},{"id":"218068","messageId":"CALkWK0nXbncV4bjHLSQCu21w36vQP5E9irNhBbyXoEZ4-oqfcQ@mail.gmail.com","threadId":"33887","inReplyTo":"7vy5b8p9wm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/2] Fix invalid revision error messages for 1.8.3","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-21T17:50:48Z","receivedAt":"2013-05-21T17:50:48Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Fixes to something that are broken the same way between 'master' and\n> older release versions are the same as enhancements (which you can\n> view as \"fix to lack of feature\").  They are not regression fixes\n> and not for 1.8.3 at this point in the cycle, deep into -rc.\n\nIf we view them as enhancements, well and good.  Let's polish them\nuntil we're really happy with them: they're written with the \"minimal,\nbut correct\" philosophy, because the -rc3 window is too small for a\nreview.\n\nJust to share opinion, they looked like \"bugs\" to me, because it's not\nabout \"improving\" the error messages; it's about correcting a defect.\nThe author could not have possibly intended two \"error: \" lines in the\nfirst one, or an empty string in the second one.  At some point in the\npast, the behavior must have been different (a \"feature\" must have\nintroduced these problems: like implicit HEAD for @{<N>}): the\n\"regression\" was introduced in the version after that.  So, is it\nbecause that version was too long ago that we don't consider it a\nregression (do we backport fixes)?\n"},{"id":"218070","messageId":"CALkWK0mTWtJ_U1O7ZkNU3aNFwGH456xtmDJhhmS3z1tfwFPNgA@mail.gmail.com","threadId":"33887","inReplyTo":"7vtxlwp9mf.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-21T17:56:50Z","receivedAt":"2013-05-21T17:56:50Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> Does a failure in interpret-branch-name that issue these error\n> messages always followed by die() in the caller?  I know you looked\n> at the cases you noticed as an end-user (like the above \"git show @{u}\"\n> example), but if some codepaths did this:\n>\n>         if (interpret-branch-name()) {\n>                 you do not seem to have upstream defined,\n>                 so I will helpfully do something else that\n>                 you probably have meant.\n>         }\n>\n> this patch will break that codepath you did not look.\n\nHow can that ever happen in a non end-user case?  That failure\nrequires a string containing \"@{u}\" to be constructed and passed as an\nargument.  Why would we ever programmatically construct \"@{u}\" to find\nthe upstream?\n\nTo put it another way: unless an end-user facing application finds an\n\"@{u}\" while parsing argv and passes it on to interpret-branch-name,\nisn't it impossible for an \"@{u}\" to end up in the argument?\n"},{"id":"218071","messageId":"7vk3msnrlf.fsf@alter.siamese.dyndns.org","threadId":"33887","inReplyTo":"CALkWK0nXbncV4bjHLSQCu21w36vQP5E9irNhBbyXoEZ4-oqfcQ@mail.gmail.com","subject":"Re: [PATCH 0/2] Fix invalid revision error messages for 1.8.3","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-21T17:57:32Z","receivedAt":"2013-05-21T17:57:32Z","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> So, is it\n> because that version was too long ago that we don't consider it a\n> regression (do we backport fixes)?\n\nThe \"regression fixes\" pre-release -rc period is for is to make sure\nto avoid unwanted/unintended behaviour changes between releases.\n\nPeople have _already_ seen and lived with these issues in released\nversions.  Changing it may or may not be getting it back to the\nstate to that of an even older release, but at that point the\ndifferences do not matter.  It is a \"fix\", too late for the kind of\nregression fixes we focus during _this_ -rc period, which is about\nregressions between v1.8.2 and 'master'.\n"},{"id":"218072","messageId":"7vfvxgnrdo.fsf@alter.siamese.dyndns.org","threadId":"33887","inReplyTo":"CALkWK0mTWtJ_U1O7ZkNU3aNFwGH456xtmDJhhmS3z1tfwFPNgA@mail.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-21T18:02:11Z","receivedAt":"2013-05-21T18:02:11Z","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> Junio C Hamano wrote:\n>> Does a failure in interpret-branch-name that issue these error\n>> messages always followed by die() in the caller?  I know you looked\n>> at the cases you noticed as an end-user (like the above \"git show @{u}\"\n>> example), but if some codepaths did this:\n>>\n>>         if (interpret-branch-name()) {\n>>                 you do not seem to have upstream defined,\n>>                 so I will helpfully do something else that\n>>                 you probably have meant.\n>>         }\n>>\n>> this patch will break that codepath you did not look.\n>\n> How can that ever happen in a non end-user case?  That failure\n> requires a string containing \"@{u}\" to be constructed and passed as an\n> argument.  Why would we ever programmatically construct \"@{u}\" to find\n> the upstream?\n>\n> To put it another way: unless an end-user facing application finds an\n> \"@{u}\" while parsing argv and passes it on to interpret-branch-name,\n> isn't it impossible for an \"@{u}\" to end up in the argument?\n\nSo did you or did you not audit the codepath?\n"},{"id":"218073","messageId":"CALkWK0nEXKXxercc1mNjyK-QX0pOBeKWAxPZtSPvN_h1eniO5g@mail.gmail.com","threadId":"33887","inReplyTo":"7vfvxgnrdo.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-21T18:04:53Z","receivedAt":"2013-05-21T18:04:53Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> So did you or did you not audit the codepath?\n\nNo; I was explaining why I didn't in the first place.  Going through it now.\n"},{"id":"218074","messageId":"7vbo84nr1o.fsf@alter.siamese.dyndns.org","threadId":"33887","inReplyTo":"CALkWK0nEXKXxercc1mNjyK-QX0pOBeKWAxPZtSPvN_h1eniO5g@mail.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-21T18:09:23Z","receivedAt":"2013-05-21T18:09:23Z","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> Junio C Hamano wrote:\n>> So did you or did you not audit the codepath?\n>\n> No; I was explaining why I didn't in the first place.  Going through it now.\n\nI did not mean \"You must do so or we should discard the patch\".  I\njust wanted to make sure the log messages say how firmly the change\nis backed.\n"},{"id":"218075","messageId":"CALkWK0=kSxRC_d9feL-_foM3uU11E_Hx_xUL++V6q9k-7kjzbQ@mail.gmail.com","threadId":"33887","inReplyTo":"7vppwkp961.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/2] sha1_name: fix error message for @{<N>}, @{<date>}","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-21T18:09:54Z","receivedAt":"2013-05-21T18:09:54Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> More importantly, doesn't \"real_ref\" have the name of the branch?\n>\n> Suppose the user said \"git show @{10000}\" instead of \"git show\n> master@{10000}\" while on 'master'.\n\nMy stupidity, sorry.\n\n> It could be argued that it may look nicer to say \"your current\n> branch does not have enough update history\" instead of saying\n> \"master does not...\" (i.e. different input to ask for the same\n> thing, different output depending on the way the user asked).  It\n> also could be argued that they should produce the same diagnosis\n> that is more informative.\n\nYeah, I wanted to discuss this: the problem is that even something as\nlow-level as rev-list will print this \"pretty\" error.  It's certainly\nuseful for porcelain.  How do we achieve this?  An extra\n\"is-porcelain\" argument?\n"},{"id":"218076","messageId":"CALkWK0mJMUYe1GpGXQ+mw-ZomODuiO5KqbrwgskTMek1O3WexQ@mail.gmail.com","threadId":"33887","inReplyTo":"7vk3msnrlf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 0/2] Fix invalid revision error messages for 1.8.3","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-21T18:16:24Z","receivedAt":"2013-05-21T18:16:24Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n> People have _already_ seen and lived with these issues in released\n> versions.  Changing it may or may not be getting it back to the\n> state to that of an even older release, but at that point the\n> differences do not matter.  It is a \"fix\", too late for the kind of\n> regression fixes we focus during _this_ -rc period, which is about\n> regressions between v1.8.2 and 'master'.\n\nMakes sense.\n\nOn a related note, I really wonder why people run anything < master\ngit; it's so easy to compile and use from ~.  The idea isn't insane at\nall: most people run a -p ruby from ~ using things like rbenv (yes, it\ncompiles from source).\n\n(ofcourse servers have to run a release)\n"},{"id":"218084","messageId":"CALkWK0m7VBz3wDGUACJAfp33M1GYqKCeMCkQwrgA7kqRMp_rtQ@mail.gmail.com","threadId":"33887","inReplyTo":"CALkWK0nEXKXxercc1mNjyK-QX0pOBeKWAxPZtSPvN_h1eniO5g@mail.gmail.com","subject":"Re: [PATCH 1/2] sha1_name: fix error message for @{u}","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2013-05-21T19:19:47Z","receivedAt":"2013-05-21T19:19:47Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Junio C Hamano wrote:\n>> So did you or did you not audit the codepath?\n>\n> No; I was explaining why I didn't in the first place.  Going through it now.\n\nSo, this is what I have:\n\ninterpret_branch_name -> interpret_branch_name (recursion)\n                      -> get_sha1_basic -> get_sha1 [context] (end-user data)\n                      -> substitute_branch_name -> dwim (end-user data)\n\t\t      -> strbuf_branchname (callers pass a branch name; no @{u})\n\t\t      -> revision.c:add_pending_object [with_mode] (end-user data)\n\n[die_]verify_filename -> builtin/rev-parse.c (end-user)\n\t\t      -> builtin/reset.c (end-user)\n\t\t      -> builtin/grep.c:cmd_grep (end-user)\n\t\t      -> revision.c:setup_revisions (end-user data)\n\nWe used to die in die_verify_filename() earlier, but we die in\ninterpret_branch_name() after the patch.  Do we have to dig deeper?\n"},{"id":"218088","messageId":"7vtxlwm6z4.fsf@alter.siamese.dyndns.org","threadId":"33887","inReplyTo":"CALkWK0m7VBz3wDGUACJAfp33M1GYqKCeMCkQwrgA7kqRMp_rtQ@mail.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-21T20:08:15Z","receivedAt":"2013-05-21T20:08:15Z","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> Ramkumar Ramachandra wrote:\n>> Junio C Hamano wrote:\n>>> So did you or did you not audit the codepath?\n>>\n>> No; I was explaining why I didn't in the first place.  Going through it now.\n>\n> So, this is what I have:\n>\n> interpret_branch_name -> interpret_branch_name (recursion)\n>                       -> get_sha1_basic -> get_sha1 [context] (end-user data)\n>                       -> substitute_branch_name -> dwim (end-user data)\n> \t\t      -> strbuf_branchname (callers pass a branch name; no @{u})\n> \t\t      -> revision.c:add_pending_object [with_mode] (end-user data)\n>\n> [die_]verify_filename -> builtin/rev-parse.c (end-user)\n> \t\t      -> builtin/reset.c (end-user)\n> \t\t      -> builtin/grep.c:cmd_grep (end-user)\n> \t\t      -> revision.c:setup_revisions (end-user data)\n\nIt seems that you are digging in the wrong direction?  I was worried\nabout the callers of interpret_branch_name().\n\nBut whatever.\n\nI looked at the callers myself while waiting for the test suite to\npass for five integration branches and I think the patch is safe.\nThere were some silent error returns from the function but your\npatch did not touch them (which is good).\n\n> We used to die in die_verify_filename() earlier, but we die in\n> interpret_branch_name() after the patch.\n\nI think that is a desired outcome.  Thanks.\n"},{"id":"218089","messageId":"CALkWK0kVAG4Gg3mgZv+0pXJocwDzZYS3gwVjxPy9cmBEkB2sFg@mail.gmail.com","threadId":"33887","inReplyTo":"7vtxlwm6z4.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-21T20:14:42Z","receivedAt":"2013-05-21T20:14:42Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Junio C Hamano wrote:\n>> interpret_branch_name -> interpret_branch_name (recursion)\n>>                       -> get_sha1_basic -> get_sha1 [context] (end-user data)\n>>                       -> substitute_branch_name -> dwim (end-user data)\n>>                     -> strbuf_branchname (callers pass a branch name; no @{u})\n>>                     -> revision.c:add_pending_object [with_mode] (end-user data)\n>>\n>> [die_]verify_filename -> builtin/rev-parse.c (end-user)\n>>                     -> builtin/reset.c (end-user)\n>>                     -> builtin/grep.c:cmd_grep (end-user)\n>>                     -> revision.c:setup_revisions (end-user data)\n>\n> It seems that you are digging in the wrong direction?  I was worried\n> about the callers of interpret_branch_name().\n\nUm, aren't interpret_branch_name, get_sha1_basic,\nsubstitute_branch_name, strbuf_branchname, and add_pending_object the\nfive callers of interpret_branch_name?  I've tried to show how they\nare called with either end-user data or programmatic data without a\n\"@{u}\".  What am I missing?\n"},{"id":"218091","messageId":"7vli78m5t0.fsf@alter.siamese.dyndns.org","threadId":"33887","inReplyTo":"CALkWK0kVAG4Gg3mgZv+0pXJocwDzZYS3gwVjxPy9cmBEkB2sFg@mail.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-21T20:33:31Z","receivedAt":"2013-05-21T20:33:31Z","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> \"@{u}\".  What am I missing?\n\nYou draw the arrow the other way around, that is what made the text\nconfusing.\n"}]}