{"thread":{"id":"41909","subject":"Since 2.7.0 git branch displays symbolic references as ref -> ref instead of ref -> branch","startedAt":"2016-04-03T02:54:22Z","lastAt":"2016-04-05T06:21:14Z","messageCount":3,"participants":["Phil Sainty","Jeff King","Karthik Nayak"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"282545","messageId":"570085DE.9000002@orcon.net.nz","threadId":"41909","inReplyTo":null,"subject":"Since 2.7.0 git branch displays symbolic references as ref -> ref instead of ref -> branch","fromName":"Phil Sainty","fromEmail":"psainty@orcon.net.nz","sentAt":"2016-04-03T02:54:22Z","receivedAt":"2016-04-03T02:54:22Z","isPatch":false,"sender":{"key":"psainty@orcon.net.nz","avatar":null},"body":"Given the following symbolic reference:\n\n$ git symbolic-ref refs/heads/m refs/heads/master\n\n\nCorrect in 2.6.6:\n\n$ PATH=~/git/git-2.6.6:$PATH git branch\n   m -> master\n* master\n\n\nWrong in 2.7.0:\n\n$ PATH=~/git/git-2.7.0:$PATH git branch\n   m -> m\n* master\n\n\nStill wrong in current version 2.8.0:\n\n$ PATH=~/git/git-2.8.0:$PATH git branch\n   m -> m\n* master\n\n\n\nAs the new output isn't very useful, I assume this is an\nunintentional bug/regression.\n\nThe 2.6.6 output has been used since at least version 1.7.12.4\n(when I first started making use of this ability).\n\n\n-Phil\n"},{"id":"282547","messageId":"20160403041426.GA22211@sigill.intra.peff.net","threadId":"41909","inReplyTo":"570085DE.9000002@orcon.net.nz","subject":"[PATCH] branch: fix shortening of non-remote symrefs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-03T04:14:26Z","receivedAt":"2016-04-03T04:14:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 03, 2016 at 02:54:22PM +1200, Phil Sainty wrote:\n\n> Given the following symbolic reference:\n> \n> $ git symbolic-ref refs/heads/m refs/heads/master\n> \n> \n> Correct in 2.6.6:\n> \n> $ PATH=~/git/git-2.6.6:$PATH git branch\n>   m -> master\n> * master\n> \n> \n> Wrong in 2.7.0:\n> \n> $ PATH=~/git/git-2.7.0:$PATH git branch\n>   m -> m\n> * master\n\nThanks for an easy test case. Though we don't officially support\narbitrary symrefs in the ref namespace, they do mostly work. And\ncertainly the current output is nonsense, and it worked before. This\nbisects to aedcb7d (branch.c: use 'ref-filter' APIs, 2015-09-23).\n\nThe fix is below. Karthik, I didn't look at all how this interacts with\nyour work to convert branch to ref-filter for printing. I imagine it\ndrops this code completely, but we should make sure that ref-filter gets\nthis case right. I almost didn't prepare this patch at all, but I\nsuspect we may want it for \"maint\", while the full conversion would wait\nfor \"master\".\n\n-- >8 --\nSubject: branch: fix shortening of non-remote symrefs\n\nCommit aedcb7d (branch.c: use 'ref-filter' APIs, 2015-09-23)\nadjusted the symref-printing code to look like this:\n\n    if (item->symref) {\n\t    skip_prefix(item->symref, \"refs/remotes/\", &desc);\n\t    strbuf_addf(&out, \" -> %s\", desc);\n    }\n\nThis has three bugs in it:\n\n  1. It always skips past \"refs/remotes/\", instead of\n     skipping past the prefix associated with the branch we\n     are showing (so commonly we see \"refs/remotes/\" for the\n     refs/remotes/origin/HEAD symref, but the previous code\n     would skip \"refs/heads/\" when showing a symref it found\n     in refs/heads/.\n\n  2. If skip_prefix() does not match, it leaves \"desc\"\n     untouched, and we show whatever happened to be in it\n     (which is the refname from a call to skip_prefix()\n     earlier in the function).\n\n  3. If we do match with skip_prefix(), we stomp on the\n     \"desc\" variable, which is later passed to\n     add_verbose_info(). We probably want to retain the\n     original refname there (though it likely doesn't matter\n     in practice, since after all, one points to the other).\n\nThe fix to match the original code is fairly easy: record\nthe prefix to strip based on item->kind, and use it here.\nHowever, since we already have a local variable named \"prefix\",\nlet's give the two prefixes verbose names so we don't\nconfuse them.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nThe test makes sure we restored the v2.6.x behavior, namely that\ncross-prefix symrefs will not be shortened at all. It might be nice to\nshow:\n\n  ref-to-remote -> remotes/origin/branch-one\n\nor something, but that should be separate from the fix (and I don't\noverly care either way, so I probably won't work on it).\n\n builtin/branch.c         | 19 ++++++++++++-------\n t/t3203-branch-output.sh | 12 ++++++++++++\n 2 files changed, 24 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 7b45b6b..f6c23bf 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -393,22 +393,25 @@ static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,\n \tint current = 0;\n \tint color;\n \tstruct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n-\tconst char *prefix = \"\";\n+\tconst char *prefix_to_show = \"\";\n+\tconst char *prefix_to_skip = NULL;\n \tconst char *desc = item->refname;\n \tchar *to_free = NULL;\n \n \tswitch (item->kind) {\n \tcase FILTER_REFS_BRANCHES:\n-\t\tskip_prefix(desc, \"refs/heads/\", &desc);\n+\t\tprefix_to_skip = \"refs/heads/\";\n+\t\tskip_prefix(desc, prefix_to_skip, &desc);\n \t\tif (!filter->detached && !strcmp(desc, head))\n \t\t\tcurrent = 1;\n \t\telse\n \t\t\tcolor = BRANCH_COLOR_LOCAL;\n \t\tbreak;\n \tcase FILTER_REFS_REMOTES:\n-\t\tskip_prefix(desc, \"refs/remotes/\", &desc);\n+\t\tprefix_to_skip = \"refs/remotes/\";\n+\t\tskip_prefix(desc, prefix_to_skip, &desc);\n \t\tcolor = BRANCH_COLOR_REMOTE;\n-\t\tprefix = remote_prefix;\n+\t\tprefix_to_show = remote_prefix;\n \t\tbreak;\n \tcase FILTER_REFS_DETACHED_HEAD:\n \t\tdesc = to_free = get_head_description();\n@@ -425,7 +428,7 @@ static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,\n \t\tcolor = BRANCH_COLOR_CURRENT;\n \t}\n \n-\tstrbuf_addf(&name, \"%s%s\", prefix, desc);\n+\tstrbuf_addf(&name, \"%s%s\", prefix_to_show, desc);\n \tif (filter->verbose) {\n \t\tint utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);\n \t\tstrbuf_addf(&out, \"%c %s%-*s%s\", c, branch_get_color(color),\n@@ -436,8 +439,10 @@ static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,\n \t\t\t    name.buf, branch_get_color(BRANCH_COLOR_RESET));\n \n \tif (item->symref) {\n-\t\tskip_prefix(item->symref, \"refs/remotes/\", &desc);\n-\t\tstrbuf_addf(&out, \" -> %s\", desc);\n+\t\tconst char *symref = item->symref;\n+\t\tif (prefix_to_skip)\n+\t\t\tskip_prefix(symref, prefix_to_skip, &symref);\n+\t\tstrbuf_addf(&out, \" -> %s\", symref);\n \t}\n \telse if (filter->verbose)\n \t\t/* \" f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h\" */\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex 4261403..c6a3ccb 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -184,4 +184,16 @@ test_expect_success 'ambiguous branch/tag not marked' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'local-branch symrefs shortened properly' '\n+\tgit symbolic-ref refs/heads/ref-to-branch refs/heads/branch-one &&\n+\tgit symbolic-ref refs/heads/ref-to-remote refs/remotes/origin/branch-one &&\n+\tcat >expect <<-\\EOF &&\n+\t  ref-to-branch -> branch-one\n+\t  ref-to-remote -> refs/remotes/origin/branch-one\n+\tEOF\n+\tgit branch >actual.raw &&\n+\tgrep ref-to <actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.8.0.429.gaaf8de0\n"},{"id":"282684","messageId":"CAOLa=ZTYc1+1ekC0q+c2sZ9==Wzw_=84V3bodhh0+pBnFAfjWg@mail.gmail.com","threadId":"41909","inReplyTo":"20160403041426.GA22211@sigill.intra.peff.net","subject":"Re: [PATCH] branch: fix shortening of non-remote symrefs","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-04-05T06:21:14Z","receivedAt":"2016-04-05T06:21:14Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Hello,\n\nOn Sun, Apr 3, 2016 at 9:44 AM, Jeff King <peff@peff.net> wrote:\n> On Sun, Apr 03, 2016 at 02:54:22PM +1200, Phil Sainty wrote:\n>\n>> Given the following symbolic reference:\n>>\n>> $ git symbolic-ref refs/heads/m refs/heads/master\n>>\n>>\n>> Correct in 2.6.6:\n>>\n>> $ PATH=~/git/git-2.6.6:$PATH git branch\n>>   m -> master\n>> * master\n>>\n>>\n>> Wrong in 2.7.0:\n>>\n>> $ PATH=~/git/git-2.7.0:$PATH git branch\n>>   m -> m\n>> * master\n\nThanks for reporting this.\n\n>\n> Thanks for an easy test case. Though we don't officially support\n> arbitrary symrefs in the ref namespace, they do mostly work. And\n> certainly the current output is nonsense, and it worked before. This\n> bisects to aedcb7d (branch.c: use 'ref-filter' APIs, 2015-09-23).\n>\n> The fix is below. Karthik, I didn't look at all how this interacts with\n> your work to convert branch to ref-filter for printing. I imagine it\n> drops this code completely, but we should make sure that ref-filter gets\n> this case right. I almost didn't prepare this patch at all, but I\n> suspect we may want it for \"maint\", while the full conversion would wait\n> for \"master\".\n>\n\nIt's dropped in my latest series. I should be able to replicate what you've done\nhere onto ref-filter.c. Since I'm re-rolling my patches, I'll add this\none along too.\n\n> -- >8 --\n> Subject: branch: fix shortening of non-remote symrefs\n>\n> Commit aedcb7d (branch.c: use 'ref-filter' APIs, 2015-09-23)\n> adjusted the symref-printing code to look like this:\n>\n>     if (item->symref) {\n>             skip_prefix(item->symref, \"refs/remotes/\", &desc);\n>             strbuf_addf(&out, \" -> %s\", desc);\n>     }\n>\n> This has three bugs in it:\n>\n>   1. It always skips past \"refs/remotes/\", instead of\n>      skipping past the prefix associated with the branch we\n>      are showing (so commonly we see \"refs/remotes/\" for the\n>      refs/remotes/origin/HEAD symref, but the previous code\n>      would skip \"refs/heads/\" when showing a symref it found\n>      in refs/heads/.\n>\n>   2. If skip_prefix() does not match, it leaves \"desc\"\n>      untouched, and we show whatever happened to be in it\n>      (which is the refname from a call to skip_prefix()\n>      earlier in the function).\n>\n>   3. If we do match with skip_prefix(), we stomp on the\n>      \"desc\" variable, which is later passed to\n>      add_verbose_info(). We probably want to retain the\n>      original refname there (though it likely doesn't matter\n>      in practice, since after all, one points to the other).\n>\n> The fix to match the original code is fairly easy: record\n> the prefix to strip based on item->kind, and use it here.\n> However, since we already have a local variable named \"prefix\",\n> let's give the two prefixes verbose names so we don't\n> confuse them.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> The test makes sure we restored the v2.6.x behavior, namely that\n> cross-prefix symrefs will not be shortened at all. It might be nice to\n> show:\n>\n>   ref-to-remote -> remotes/origin/branch-one\n>\n> or something, but that should be separate from the fix (and I don't\n> overly care either way, so I probably won't work on it).\n>\n>  builtin/branch.c         | 19 ++++++++++++-------\n>  t/t3203-branch-output.sh | 12 ++++++++++++\n>  2 files changed, 24 insertions(+), 7 deletions(-)\n>\n> diff --git a/builtin/branch.c b/builtin/branch.c\n> index 7b45b6b..f6c23bf 100644\n> --- a/builtin/branch.c\n> +++ b/builtin/branch.c\n> @@ -393,22 +393,25 @@ static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,\n>         int current = 0;\n>         int color;\n>         struct strbuf out = STRBUF_INIT, name = STRBUF_INIT;\n> -       const char *prefix = \"\";\n> +       const char *prefix_to_show = \"\";\n> +       const char *prefix_to_skip = NULL;\n>         const char *desc = item->refname;\n>         char *to_free = NULL;\n>\n>         switch (item->kind) {\n>         case FILTER_REFS_BRANCHES:\n> -               skip_prefix(desc, \"refs/heads/\", &desc);\n> +               prefix_to_skip = \"refs/heads/\";\n> +               skip_prefix(desc, prefix_to_skip, &desc);\n>                 if (!filter->detached && !strcmp(desc, head))\n>                         current = 1;\n>                 else\n>                         color = BRANCH_COLOR_LOCAL;\n>                 break;\n>         case FILTER_REFS_REMOTES:\n> -               skip_prefix(desc, \"refs/remotes/\", &desc);\n> +               prefix_to_skip = \"refs/remotes/\";\n> +               skip_prefix(desc, prefix_to_skip, &desc);\n>                 color = BRANCH_COLOR_REMOTE;\n> -               prefix = remote_prefix;\n> +               prefix_to_show = remote_prefix;\n>                 break;\n>         case FILTER_REFS_DETACHED_HEAD:\n>                 desc = to_free = get_head_description();\n> @@ -425,7 +428,7 @@ static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,\n>                 color = BRANCH_COLOR_CURRENT;\n>         }\n>\n> -       strbuf_addf(&name, \"%s%s\", prefix, desc);\n> +       strbuf_addf(&name, \"%s%s\", prefix_to_show, desc);\n>         if (filter->verbose) {\n>                 int utf8_compensation = strlen(name.buf) - utf8_strwidth(name.buf);\n>                 strbuf_addf(&out, \"%c %s%-*s%s\", c, branch_get_color(color),\n> @@ -436,8 +439,10 @@ static void format_and_print_ref_item(struct ref_array_item *item, int maxwidth,\n>                             name.buf, branch_get_color(BRANCH_COLOR_RESET));\n>\n>         if (item->symref) {\n> -               skip_prefix(item->symref, \"refs/remotes/\", &desc);\n> -               strbuf_addf(&out, \" -> %s\", desc);\n> +               const char *symref = item->symref;\n> +               if (prefix_to_skip)\n> +                       skip_prefix(symref, prefix_to_skip, &symref);\n> +               strbuf_addf(&out, \" -> %s\", symref);\n>         }\n>         else if (filter->verbose)\n>                 /* \" f7c0c00 [ahead 58, behind 197] vcs-svn: drop obj_pool.h\" */\n> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\n> index 4261403..c6a3ccb 100755\n> --- a/t/t3203-branch-output.sh\n> +++ b/t/t3203-branch-output.sh\n> @@ -184,4 +184,16 @@ test_expect_success 'ambiguous branch/tag not marked' '\n>         test_cmp expect actual\n>  '\n>\n> +test_expect_success 'local-branch symrefs shortened properly' '\n> +       git symbolic-ref refs/heads/ref-to-branch refs/heads/branch-one &&\n> +       git symbolic-ref refs/heads/ref-to-remote refs/remotes/origin/branch-one &&\n> +       cat >expect <<-\\EOF &&\n> +         ref-to-branch -> branch-one\n> +         ref-to-remote -> refs/remotes/origin/branch-one\n> +       EOF\n> +       git branch >actual.raw &&\n> +       grep ref-to <actual.raw >actual &&\n> +       test_cmp expect actual\n> +'\n> +\n>  test_done\n> --\n> 2.8.0.429.gaaf8de0\n>\n\nThis seems to fix the bug very easily and effectively. Thanks for the patch :)\n\n-- \nRegards,\nKarthik Nayak\n"}]}