{"thread":{"id":"41250","subject":"Regression? Ambiguous tags listed as \"tags/\"","startedAt":"2016-01-23T23:56:12Z","lastAt":"2016-01-26T04:25:08Z","messageCount":19,"participants":["Pete Harlan","Jeff King","Junio C Hamano","Eric Sunshine","Karthik Nayak"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"276614","messageId":"CAAB=nN=RSQuDPdg6Y2heeBXpQVdWSwFAcDDTfxE=XRSOu5gGEg@mail.gmail.com","threadId":"41250","inReplyTo":null,"subject":"Regression? Ambiguous tags listed as \"tags/\"","fromName":"Pete Harlan","fromEmail":"pgit@tento.net","sentAt":"2016-01-23T23:56:12Z","receivedAt":"2016-01-23T23:56:12Z","isPatch":false,"sender":{"key":"pgit@tento.net","avatar":null},"body":"There was a behavior change in \"git tag\" in b7cc53e9 (tag.c: use\n'ref-filter' APIs, 2015-07-11, v2.5.0-276-gb7cc53e), such that \"git\ntag\" now writes \"foo\" as \"tags/foo\" if there is also a branch named\n\"foo\":\n\n % git tag\n tags/branchortag\n tagonly\n\nPrevious behavior:\n\n % git tag\n branchortag\n tagonly\n\nI prefer the previous behavior. Perhaps the change was intentional,\nbut I didn't see it documented.\n\nThanks,\n\nPete Harlan\npgit@tento.net\n"},{"id":"276615","messageId":"20160124071234.GA24084@sigill.intra.peff.net","threadId":"41250","inReplyTo":"CAAB=nN=RSQuDPdg6Y2heeBXpQVdWSwFAcDDTfxE=XRSOu5gGEg@mail.gmail.com","subject":"[PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-24T07:12:35Z","receivedAt":"2016-01-24T07:12:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Jan 23, 2016 at 03:56:12PM -0800, Pete Harlan wrote:\n\n> There was a behavior change in \"git tag\" in b7cc53e9 (tag.c: use\n> 'ref-filter' APIs, 2015-07-11, v2.5.0-276-gb7cc53e), such that \"git\n> tag\" now writes \"foo\" as \"tags/foo\" if there is also a branch named\n> \"foo\":\n> \n> % git tag\n> tags/branchortag\n> tagonly\n> \n> Previous behavior:\n> \n> % git tag\n> branchortag\n> tagonly\n> \n> I prefer the previous behavior. Perhaps the change was intentional,\n> but I didn't see it documented.\n\nI don't think it was intentional, and I think the new output is actively\nwrong. My reasoning (and a fix) are below.\n\n-- >8 --\nSubject: tag: do not show ambiguous tag names as \"tags/foo\"\n\nSince b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),\ngit-tag has started showing tags with ambiguous names (i.e.,\nwhen both \"heads/foo\" and \"tags/foo\" exists) as \"tags/foo\"\ninstead of just \"foo\". This is both:\n\n - pointless; the output of \"git tag\" includes only\n refs/tags, so we know that \"foo\" means the one in\n \"refs/tags\".\n\nand\n\n - ambiguous; in the original output, we know that the line\n \"foo\" means that \"refs/tags/foo\" exists. In the new\n output, it is unclear whether we mean \"refs/tags/foo\" or\n \"refs/tags/tags/foo\".\n\nThe reason this happens is that commit b7cc53e9 switched\ngit-tag to use ref-filter's \"%(refname:short)\" output\nformatting, which was adapted from for-each-ref. This more\ngeneral code does not know that we care only about tags, and\nuses shorten_unambiguous_ref to get the short-name. We need\nto tell it that we care only about \"refs/tags/\", and it\nshould shorten with respect to that value.\n\nIn theory, the ref-filter code could figure this out by us\npassing FILTER_REFS_TAGS. But there are two complications\nthere:\n\n 1. The handling of refname:short is deep in formatting\n code that does not even have our ref_filter struct, let\n alone the arguments to the filter_ref struct.\n\n 2. In git v2.7.0, we expose the formatting language to the\n user. If we follow this path, it will mean that\n \"%(refname:short)\" behaves differently for \"tag\" versus\n \"for-each-ref\" (including \"for-each-ref refs/tags/\"),\n which can lead to confusion.\n\nInstead, let's extend the \"short\" modifier in the formatting\nlanguage to handle a specific prefix. This fixes \"git tag\",\nand lets users invoke the same behavior from their own\ncustom formats (for \"tag\" or \"for-each-ref\") while leaving\n\":short\" with its same consistent meaning in all places.\n\nWe introduce a test in t7004 for \"git tag\", which fails\nwithout this patch. We also add a similar test in t3203 for\n\"git branch\", which does not actually fail. But since it is\nlikely that \"branch\" will eventually use the same formatting\ncode, the test helps defend against future regressions.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-for-each-ref.txt | 4 +++-\n Documentation/git-tag.txt | 2 +-\n builtin/tag.c | 4 ++--\n ref-filter.c | 3 +++\n t/t3203-branch-output.sh | 8 ++++++++\n t/t7004-tag.sh | 8 ++++++++\n 6 files changed, 25 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 06208c4..faf10c5 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -92,7 +92,9 @@ refname::\n \tThe name of the ref (the part after $GIT_DIR/).\n \tFor a non-ambiguous short name of the ref append `:short`.\n \tThe option core.warnAmbiguousRefs is used to select the strict\n-\tabbreviation mode.\n+\tabbreviation mode. For shortening in a specific hierarchy, use\n+\t`:short=`, which will remove `` (if present)\n+\tfrom the front of the ref. E.g., `%(refname:short=refs/tags/)`.\n \n objecttype::\n \tThe type of the object (`blob`, `tree`, `commit`, `tag`).\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 7220e5e..62ff509 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -163,7 +163,7 @@ This option is only applicable when listing tags without annotation lines.\n \tA string that interpolates `%(fieldname)` from the object\n \tpointed at by a ref being shown. The format is the same as\n \tthat of linkgit:git-for-each-ref[1]. When unspecified,\n-\tdefaults to `%(refname:short)`.\n+\tdefaults to `%(refname:short=refs/tags/)`.\n \n --[no-]merged []::\n \tOnly list tags whose tips are reachable, or not reachable\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 8db8c87..f60feb1 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -44,11 +44,11 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\n \tif (!format) {\n \t\tif (filter->lines) {\n \t\t\tto_free = xstrfmt(\"%s %%(contents:lines=%d)\",\n-\t\t\t\t\t \"%(align:15)%(refname:short)%(end)\",\n+\t\t\t\t\t \"%(align:15)%(refname:short=refs/tags/)%(end)\",\n \t\t\t\t\t filter->lines);\n \t\t\tformat = to_free;\n \t\t} else\n-\t\t\tformat = \"%(refname:short)\";\n+\t\t\tformat = \"%(refname:short=refs/tags/)\";\n \t}\n \n \tverify_ref_format(format);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 7bef7f8..b878b77 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -909,11 +909,14 @@ static void populate_value(struct ref_array_item *ref)\n \t\tformatp = strchr(name, ':');\n \t\tif (formatp) {\n \t\t\tint num_ours, num_theirs;\n+\t\t\tconst char *prefix;\n \n \t\t\tformatp++;\n \t\t\tif (!strcmp(formatp, \"short\"))\n \t\t\t\trefname = shorten_unambiguous_ref(refname,\n \t\t\t\t\t\t warn_ambiguous_refs);\n+\t\t\telse if (skip_prefix(formatp, \"short=\", &prefix))\n+\t\t\t\tskip_prefix(refname, prefix, &refname);\n \t\t\telse if (!strcmp(formatp, \"track\") &&\n \t\t\t\t (starts_with(name, \"upstream\") ||\n \t\t\t\t starts_with(name, \"push\"))) {\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex d3913f9..4261403 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -176,4 +176,12 @@ test_expect_success 'git branch --points-at option' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ambiguous branch/tag not marked' '\n+\tgit tag ambiguous &&\n+\tgit branch ambiguous &&\n+\techo \" ambiguous\" >expect &&\n+\tgit branch --list ambiguous >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 2797f22..cf3469b 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1558,4 +1558,12 @@ test_expect_success '--no-merged show unmerged tags' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ambiguous branch/tags not marked' '\n+\tgit tag ambiguous &&\n+\tgit branch ambiguous &&\n+\techo ambiguous >expect &&\n+\tgit tag -l ambiguous >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.7.0.425.g047222e\n"},{"id":"276616","messageId":"20160124071815.GB24084@sigill.intra.peff.net","threadId":"41250","inReplyTo":"20160124071234.GA24084@sigill.intra.peff.net","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-24T07:18:15Z","receivedAt":"2016-01-24T07:18:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 24, 2016 at 02:12:35AM -0500, Jeff King wrote:\n\n> In theory, the ref-filter code could figure this out by us\n> passing FILTER_REFS_TAGS. But there are two complications\n> there:\n> \n> 1. The handling of refname:short is deep in formatting\n> code that does not even have our ref_filter struct, let\n> alone the arguments to the filter_ref struct.\n> \n> 2. In git v2.7.0, we expose the formatting language to the\n> user. If we follow this path, it will mean that\n> \"%(refname:short)\" behaves differently for \"tag\" versus\n> \"for-each-ref\" (including \"for-each-ref refs/tags/\"),\n> which can lead to confusion.\n> \n> Instead, let's extend the \"short\" modifier in the formatting\n> language to handle a specific prefix. This fixes \"git tag\",\n> and lets users invoke the same behavior from their own\n> custom formats (for \"tag\" or \"for-each-ref\") while leaving\n> \":short\" with its same consistent meaning in all places.\n\nI think the patch I posted is a reasonable way to go. But I also don't\nthink that having \"%(refname:short)\" behave specially for \"git-tag\" is\nall that unreasonable, either. But I'm open to argument.\n\nHere are a few more considerations I had.\n\n - I'm not sure if the \"special\" behavior works as well for git-branch,\n which may want to shorten both \"refs/heads/\" and \"refs/remotes/\",\n depending on the type of ref.\n\n My solution may not extend there naturally, either, depending on how\n it is implemented.\n\n - To let users get the same behavior out of for-each-ref, we could\n perhaps auto-infer that looking at \"refs/tags/\" means shortening and\n disambiguation should only happen with respect to the \"refs/tags/\"\n hierarchy.\n\n But I'm uncomfortable changing the meaning of \":short\" without at\n least a new option. And what would it mean for \"git for-each-ref\n refs/heads/foo/\"? Would it shorten \"refs/heads/foo/bar\" to just\n \"bar\", or would it still be \"foo/bar\"?\n\n-Peff\n"},{"id":"276679","messageId":"xmqqvb6iboxj.fsf@gitster.mtv.corp.google.com","threadId":"41250","inReplyTo":"20160124071815.GB24084@sigill.intra.peff.net","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-24T22:19:52Z","receivedAt":"2016-01-24T22:19:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sun, Jan 24, 2016 at 02:12:35AM -0500, Jeff King wrote:\n>\n>> In theory, the ref-filter code could figure this out by us\n>> passing FILTER_REFS_TAGS. But there are two complications\n>> there:\n>> \n>> 1. The handling of refname:short is deep in formatting\n>> code that does not even have our ref_filter struct, let\n>> alone the arguments to the filter_ref struct.\n>> \n>> 2. In git v2.7.0, we expose the formatting language to the\n>> user. If we follow this path, it will mean that\n>> \"%(refname:short)\" behaves differently for \"tag\" versus\n>> \"for-each-ref\" (including \"for-each-ref refs/tags/\"),\n>> which can lead to confusion.\n>> \n>> Instead, let's extend the \"short\" modifier in the formatting\n>> language to handle a specific prefix. This fixes \"git tag\",\n>> and lets users invoke the same behavior from their own\n>> custom formats (for \"tag\" or \"for-each-ref\") while leaving\n>> \":short\" with its same consistent meaning in all places.\n\nYeah, I do agree with the analysis.\n\nI however wonder if short=$prefix is a good end-user interface,\nthough, as strip=$prefix may be more intuitive to them, I suspect.\n\n> I think the patch I posted is a reasonable way to go. But I also don't\n> think that having \"%(refname:short)\" behave specially for \"git-tag\" is\n> all that unreasonable, either. But I'm open to argument.\n>\n> Here are a few more considerations I had.\n>\n> - I'm not sure if the \"special\" behavior works as well for git-branch,\n> which may want to shorten both \"refs/heads/\" and \"refs/remotes/\",\n> depending on the type of ref.\n>\n> My solution may not extend there naturally, either, depending on how\n> it is implemented.\n>\n> - To let users get the same behavior out of for-each-ref, we could\n> perhaps auto-infer that looking at \"refs/tags/\" means shortening and\n> disambiguation should only happen with respect to the \"refs/tags/\"\n> hierarchy.\n>\n> But I'm uncomfortable changing the meaning of \":short\" without at\n> least a new option. And what would it mean for \"git for-each-ref\n> refs/heads/foo/\"? Would it shorten \"refs/heads/foo/bar\" to just\n> \"bar\", or would it still be \"foo/bar\"?\n\nAlso there is \"what happens if the expected prefix is not there?\"\nquestion. Perhaps strip=2 can be defined to \"strip 2 levels of\nhierarchy prefix no matter what that is\", and strip refs/tags/foo,\nrefs/heads/foo and refs/remotes/origin/foo to foo, foo, origin/foo,\nrespectively? The filtering natively done by the listing mode of\n\"branch\" and \"tags\" would ensure the prefixes are always what we\nimplicitly expect, so the case we need to worry about how we should\nsignal errors becomes \"what if there are not enough levels\". That\nmay be simpler to handle.\n"},{"id":"276680","messageId":"20160124222736.GA29115@sigill.intra.peff.net","threadId":"41250","inReplyTo":"xmqqvb6iboxj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-24T22:27:36Z","receivedAt":"2016-01-24T22:27:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 24, 2016 at 02:19:52PM -0800, Junio C Hamano wrote:\n\n> >> Instead, let's extend the \"short\" modifier in the formatting\n> >> language to handle a specific prefix. This fixes \"git tag\",\n> >> and lets users invoke the same behavior from their own\n> >> custom formats (for \"tag\" or \"for-each-ref\") while leaving\n> >> \":short\" with its same consistent meaning in all places.\n> \n> Yeah, I do agree with the analysis.\n> \n> I however wonder if short=$prefix is a good end-user interface,\n> though, as strip=$prefix may be more intuitive to them, I suspect.\n\nYeah, I picked \"short\" just because of the existing feature. But we are\nnot bound to use the same name at all, and \"strip\" is probably more\ndescriptive (I thought \"prefix\" might also be, but that only\ncommunicates what it _is_, not that we are removing it).\n\n> Also there is \"what happens if the expected prefix is not there?\"\n> question.\n\nI think \"do not strip anything\" (as I have here) is an OK behavior. It\nwould not come up for sane requests (i.e., you would generally be\nfiltering to match your prefix anyway).\n\nBut...\n\n> Perhaps strip=2 can be defined to \"strip 2 levels of\n> hierarchy prefix no matter what that is\", and strip refs/tags/foo,\n> refs/heads/foo and refs/remotes/origin/foo to foo, foo, origin/foo,\n> respectively? The filtering natively done by the listing mode of\n> \"branch\" and \"tags\" would ensure the prefixes are always what we\n> implicitly expect, so the case we need to worry about how we should\n> signal errors becomes \"what if there are not enough levels\". That\n> may be simpler to handle.\n\nYeah, \"strip=2\" would also get the job done, and extends more naturally\nto the branch case.\n\nTo be honest, I cannot imagine anybody using anything _but_ strip=2, but\nmaybe there are special cases, like:\n\n git for-each-ref --format='%(refname:strip=3)' refs/heads/jk/\n\nto get my list of topics, sans initials.\n\nI had originally hoped to avoid exposing any of this to the user, and\njust make things internal, so that we would not be locked into a\nparticular formatting behavior. But since we now have \"tag --format\" and\nadvertise \"%(refname:short)\" as its default, I think it has become\nuser-visible.\n\nLet me see what the \"strip=X\" patch looks like.\n\n-Peff\n"},{"id":"276681","messageId":"20160124230531.GB29115@sigill.intra.peff.net","threadId":"41250","inReplyTo":"xmqqvb6iboxj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-24T23:05:31Z","receivedAt":"2016-01-24T23:05:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 24, 2016 at 02:19:52PM -0800, Junio C Hamano wrote:\n\n> Also there is \"what happens if the expected prefix is not there?\"\n> question. Perhaps strip=2 can be defined to \"strip 2 levels of\n> hierarchy prefix no matter what that is\", and strip refs/tags/foo,\n> refs/heads/foo and refs/remotes/origin/foo to foo, foo, origin/foo,\n> respectively? The filtering natively done by the listing mode of\n> \"branch\" and \"tags\" would ensure the prefixes are always what we\n> implicitly expect, so the case we need to worry about how we should\n> signal errors becomes \"what if there are not enough levels\". That\n> may be simpler to handle.\n\nThe \"not enough levels\" question is interesting. Here are the options I\ncan think of:\n\n 1. Signal an error and die. Safest, but potentially annoying.\n\n 2. Strip everything and print a blank. Logically consistent, but the\n output is not particularly useful, and may introduce parsing\n confusion.\n\n 3. Strip nothing (i.e., \"%(refname:strip=4)\" on \"refs/heads/master\"\n remains \"refs/heads/master\"). Easy to explain, and provides\n useful-ish output. The output is technically ambiguous, though (was\n it \"refs/heads/foo/refs/heads/master\", or just\n \"refs/heads/master\"?).\n\n 4. Strip all but the final entry. Kind-of also useful, but like (3),\n ambiguous.\n\nI implemented (3) semi-arbitrarily (mostly because it was only slightly\nless easy than (2), and (2) seems kind of crazy).\n\nThere is also a question of what \"strip=-1\" or \"strip=foobar\" should do.\nThey are both equivalent to strip=0 in my implementation, but we could\nalso report an error.\n\nI ended up doing a preparatory patch for t6300; I wanted to add new\ntests there, and the existing content was so messy I couldn't bear it.\n\n [1/2]: t6300: use test_atom for some un-modern tests\n [2/2]: tag: do not show ambiguous tag names as \"tags/foo\"\n\n-Peff\n"},{"id":"276682","messageId":"20160124230818.GA16455@sigill.intra.peff.net","threadId":"41250","inReplyTo":"20160124230531.GB29115@sigill.intra.peff.net","subject":"[PATCH 1/2] t6300: use test_atom for some un-modern tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-24T23:08:18Z","receivedAt":"2016-01-24T23:08:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Because this script has to test so many formatters, we have\nthe nice \"test_atom\" helper, but we don't use it\nconsistently. Let's do so. This is shorter, gets rid of some\ntests that have their \"expected\" setup outside of a\ntest_expect_success block, and lets us organize the changes\nbetter (e.g., putting \"refname:short\" near \"refname\").\n\nWe also expand the \"%(push)\" tests a little to match the\n\"%(upstream)\" ones.\n\nSigned-off-by: Jeff King \n---\nI suspect one could do even more cleanup in this script, but this looked\nlike all of the low-hanging fruit (some of the others are\norder-dependent, for example).\n\nOne of the blocks that goes away must be setting a new\nrecord for \"pointless use of cat\". See if you can spot it. :)\n\n t/t6300-for-each-ref.sh | 62 ++++++++-----------------------------------------\n 1 file changed, 10 insertions(+), 52 deletions(-)\n\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 03873b0..859b237 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -49,11 +49,15 @@ test_atom() {\n }\n \n test_atom head refname refs/heads/master\n+test_atom head refname:short master\n test_atom head upstream refs/remotes/origin/master\n+test_atom head upstream:short origin/master\n test_atom head push refs/remotes/myfork/master\n+test_atom head push:short myfork/master\n test_atom head objecttype commit\n test_atom head objectsize 171\n test_atom head objectname $(git rev-parse refs/heads/master)\n+test_atom head objectname:short $(git rev-parse --short refs/heads/master)\n test_atom head tree $(git rev-parse refs/heads/master^{tree})\n test_atom head parent ''\n test_atom head numparent 0\n@@ -86,11 +90,13 @@ test_atom head contents 'Initial\n test_atom head HEAD '*'\n \n test_atom tag refname refs/tags/testtag\n+test_atom tag refname:short testtag\n test_atom tag upstream ''\n test_atom tag push ''\n test_atom tag objecttype tag\n test_atom tag objectsize 154\n test_atom tag objectname $(git rev-parse refs/tags/testtag)\n+test_atom tag objectname:short $(git rev-parse --short refs/tags/testtag)\n test_atom tag tree ''\n test_atom tag parent ''\n test_atom tag numparent ''\n@@ -338,47 +344,14 @@ for i in \"--perl --shell\" \"-s --python\" \"--python --tcl\" \"--tcl --perl\"; do\n \t\"\n done\n \n-cat >expected <<\\EOF\n-master\n-testtag\n-EOF\n-\n-test_expect_success 'Check short refname format' '\n-\t(git for-each-ref --format=\"%(refname:short)\" refs/heads &&\n-\tgit for-each-ref --format=\"%(refname:short)\" refs/tags) >actual &&\n-\ttest_cmp expected actual\n-'\n-\n-cat >expected <actual &&\n-\ttest_cmp expected actual\n-'\n-\n test_expect_success 'setup for upstream:track[short]' '\n \ttest_commit two\n '\n \n-cat >expected <actual &&\n-\ttest_cmp expected actual\n-'\n-\n-cat >expected <\n-EOF\n-\n-test_expect_success 'Check upstream:trackshort format' '\n-\tgit for-each-ref --format=\"%(upstream:trackshort)\" refs/heads >actual &&\n-\ttest_cmp expected actual\n-'\n+test_atom head upstream:track '[ahead 1]'\n+test_atom head upstream:trackshort '>'\n+test_atom head push:track '[ahead 1]'\n+test_atom head push:trackshort '>'\n \n test_expect_success 'Check that :track[short] cannot be used with other atoms' '\n \ttest_must_fail git for-each-ref --format=\"%(refname:track)\" 2>/dev/null &&\n@@ -398,21 +371,6 @@ test_expect_success 'Check that :track[short] works when upstream is invalid' '\n \ttest_cmp expected actual\n '\n \n-test_expect_success '%(push) supports tracking specifiers, too' '\n-\techo \"[ahead 1]\" >expected &&\n-\tgit for-each-ref --format=\"%(push:track)\" refs/heads >actual &&\n-\ttest_cmp expected actual\n-'\n-\n-cat >expected <actual &&\n-\ttest_cmp expected actual\n-'\n-\n test_expect_success 'Check for invalid refname format' '\n \ttest_must_fail git for-each-ref --format=\"%(refname:INVALID)\"\n '\n-- \n2.7.0.427.g4c6e021\n"},{"id":"276683","messageId":"20160124230840.GB16455@sigill.intra.peff.net","threadId":"41250","inReplyTo":"20160124230531.GB29115@sigill.intra.peff.net","subject":"[PATCH 2/2] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-24T23:08:40Z","receivedAt":"2016-01-24T23:08:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),\ngit-tag has started showing tags with ambiguous names (i.e.,\nwhen both \"heads/foo\" and \"tags/foo\" exists) as \"tags/foo\"\ninstead of just \"foo\". This is both:\n\n - pointless; the output of \"git tag\" includes only\n refs/tags, so we know that \"foo\" means the one in\n \"refs/tags\".\n\nand\n\n - ambiguous; in the original output, we know that the line\n \"foo\" means that \"refs/tags/foo\" exists. In the new\n output, it is unclear whether we mean \"refs/tags/foo\" or\n \"refs/tags/tags/foo\".\n\nThe reason this happens is that commit b7cc53e9 switched\ngit-tag to use ref-filter's \"%(refname:short)\" output\nformatting, which was adapted from for-each-ref. This more\ngeneral code does not know that we care only about tags, and\nuses shorten_unambiguous_ref to get the short-name. We need\nto tell it that we care only about \"refs/tags/\", and it\nshould shorten with respect to that value.\n\nIn theory, the ref-filter code could figure this out by us\npassing FILTER_REFS_TAGS. But there are two complications\nthere:\n\n 1. The handling of refname:short is deep in formatting\n code that does not even have our ref_filter struct, let\n alone the arguments to the filter_ref struct.\n\n 2. In git v2.7.0, we expose the formatting language to the\n user. If we follow this path, it will mean that\n \"%(refname:short)\" behaves differently for \"tag\" versus\n \"for-each-ref\" (including \"for-each-ref refs/tags/\"),\n which can lead to confusion.\n\nInstead, let's add a new modifier to the formatting\nlanguage, \"strip\", to remove a specific set of prefix\ncomponents. This fixes \"git tag\", and lets users invoke the\nsame behavior from their own custom formats (for \"tag\" or\n\"for-each-ref\") while leaving \":short\" with its same\nconsistent meaning in all places.\n\nWe introduce a test in t7004 for \"git tag\", which fails\nwithout this patch. We also add a similar test in t3203 for\n\"git branch\", which does not actually fail. But since it is\nlikely that \"branch\" will eventually use the same formatting\ncode, the test helps defend against future regressions.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-for-each-ref.txt | 6 +++++-\n Documentation/git-tag.txt | 2 +-\n builtin/tag.c | 4 ++--\n ref-filter.c | 13 ++++++++++++-\n t/t3203-branch-output.sh | 8 ++++++++\n t/t6300-for-each-ref.sh | 4 ++++\n t/t7004-tag.sh | 8 ++++++++\n 7 files changed, 40 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 06208c4..f15c817 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -92,7 +92,11 @@ refname::\n \tThe name of the ref (the part after $GIT_DIR/).\n \tFor a non-ambiguous short name of the ref append `:short`.\n \tThe option core.warnAmbiguousRefs is used to select the strict\n-\tabbreviation mode.\n+\tabbreviation mode. If `strip=` is appended, strips ``\n+\tslash-separated path components from the front of the refname\n+\t(e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.\n+\tIf the ref has fewer components than ``, the whole,\n+\tunstripped `%(refname)` is printed.\n \n objecttype::\n \tThe type of the object (`blob`, `tree`, `commit`, `tag`).\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 7220e5e..abab481 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -163,7 +163,7 @@ This option is only applicable when listing tags without annotation lines.\n \tA string that interpolates `%(fieldname)` from the object\n \tpointed at by a ref being shown. The format is the same as\n \tthat of linkgit:git-for-each-ref[1]. When unspecified,\n-\tdefaults to `%(refname:short)`.\n+\tdefaults to `%(refname:strip=2)`.\n \n --[no-]merged []::\n \tOnly list tags whose tips are reachable, or not reachable\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 8db8c87..1705c94 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -44,11 +44,11 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\n \tif (!format) {\n \t\tif (filter->lines) {\n \t\t\tto_free = xstrfmt(\"%s %%(contents:lines=%d)\",\n-\t\t\t\t\t \"%(align:15)%(refname:short)%(end)\",\n+\t\t\t\t\t \"%(align:15)%(refname:strip=2)%(end)\",\n \t\t\t\t\t filter->lines);\n \t\t\tformat = to_free;\n \t\t} else\n-\t\t\tformat = \"%(refname:short)\";\n+\t\t\tformat = \"%(refname:strip=2)\";\n \t}\n \n \tverify_ref_format(format);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 7bef7f8..9f54adc 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -909,12 +909,23 @@ static void populate_value(struct ref_array_item *ref)\n \t\tformatp = strchr(name, ':');\n \t\tif (formatp) {\n \t\t\tint num_ours, num_theirs;\n+\t\t\tconst char *arg;\n \n \t\t\tformatp++;\n \t\t\tif (!strcmp(formatp, \"short\"))\n \t\t\t\trefname = shorten_unambiguous_ref(refname,\n \t\t\t\t\t\t warn_ambiguous_refs);\n-\t\t\telse if (!strcmp(formatp, \"track\") &&\n+\t\t\telse if (skip_prefix(formatp, \"strip=\", &arg)) {\n+\t\t\t\tint strip = atoi(arg);\n+\t\t\t\tconst char *start = refname;\n+\t\t\t\twhile (strip && *start) {\n+\t\t\t\t\tif (*start == '/')\n+\t\t\t\t\t\tstrip--;\n+\t\t\t\t\tstart++;\n+\t\t\t\t}\n+\t\t\t\tif (!strip)\n+\t\t\t\t\trefname = start;\n+\t\t\t} else if (!strcmp(formatp, \"track\") &&\n \t\t\t\t (starts_with(name, \"upstream\") ||\n \t\t\t\t starts_with(name, \"push\"))) {\n \ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex d3913f9..4261403 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -176,4 +176,12 @@ test_expect_success 'git branch --points-at option' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ambiguous branch/tag not marked' '\n+\tgit tag ambiguous &&\n+\tgit branch ambiguous &&\n+\techo \" ambiguous\" >expect &&\n+\tgit branch --list ambiguous >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 859b237..1f3abeb 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -50,6 +50,10 @@ test_atom() {\n \n test_atom head refname refs/heads/master\n test_atom head refname:short master\n+test_atom head refname:strip=0 refs/heads/master\n+test_atom head refname:strip=1 heads/master\n+test_atom head refname:strip=2 master\n+test_atom head refname:strip=3 refs/heads/master\n test_atom head upstream refs/remotes/origin/master\n test_atom head upstream:short origin/master\n test_atom head push refs/remotes/myfork/master\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 2797f22..cf3469b 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1558,4 +1558,12 @@ test_expect_success '--no-merged show unmerged tags' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ambiguous branch/tags not marked' '\n+\tgit tag ambiguous &&\n+\tgit branch ambiguous &&\n+\techo ambiguous >expect &&\n+\tgit tag -l ambiguous >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.7.0.427.g4c6e021\n"},{"id":"276684","messageId":"CAPig+cTq0j0ss=qw9Dx8-PqFA5WJwP0mpvoO+5=NXtOt2EUNww@mail.gmail.com","threadId":"41250","inReplyTo":"20160124222736.GA29115@sigill.intra.peff.net","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2016-01-24T23:39:05Z","receivedAt":"2016-01-24T23:39:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Jan 24, 2016 at 5:27 PM, Jeff King wrote:\n> On Sun, Jan 24, 2016 at 02:19:52PM -0800, Junio C Hamano wrote:\n>> Perhaps strip=2 can be defined to \"strip 2 levels of\n>> hierarchy prefix no matter what that is\", and strip refs/tags/foo,\n>> refs/heads/foo and refs/remotes/origin/foo to foo, foo, origin/foo,\n>> respectively? The filtering natively done by the listing mode of\n>> \"branch\" and \"tags\" would ensure the prefixes are always what we\n>> implicitly expect, so the case we need to worry about how we should\n>> signal errors becomes \"what if there are not enough levels\". That\n>> may be simpler to handle.\n>\n> Yeah, \"strip=2\" would also get the job done, and extends more naturally\n> to the branch case.\n>\n> To be honest, I cannot imagine anybody using anything _but_ strip=2, but\n> maybe there are special cases, like:\n>\n> git for-each-ref --format='%(refname:strip=3)' refs/heads/jk/\n>\n> to get my list of topics, sans initials.\n\nWhat if the option was named \":stripprefix=\" in its most general form:\n\n %(refname:stripprefix=refs/tags/)\n\nwith plain:\n\n %(refname:stripprefix)\n\nshorthand for \":stripprefix=refs/*/\" or something?\n"},{"id":"276694","messageId":"xmqqsi1m9yxh.fsf@gitster.mtv.corp.google.com","threadId":"41250","inReplyTo":"20160124222736.GA29115@sigill.intra.peff.net","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-25T02:26:50Z","receivedAt":"2016-01-25T02:26:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, \"strip=2\" would also get the job done, and extends more naturally\n> to the branch case.\n>\n> To be honest, I cannot imagine anybody using anything _but_ strip=2...\n\nI 100% agree, and I do consider this to be internal implementation\ndetail for the listing modes of \"tag\" (and \"branch\"), which may be\nexposed to the user (by documenting that %(refname:X) is used by\ndefault), so perhaps even the flexibility of strip=2 is unwanted.\n\nI know what \"remove-standard-prefix\" is way too long for the value\nof X above, but then we can say \"the command will error out if you\nallow your for-each-ref invocation to step outside of the area that\nhas standard prefix to be removed.\" without having to worry about\n\"what is the sensible thing to do when the prefixes are not what we\nexpect (too short for strip=2 or no match for short=refs/tags/)\".\n"},{"id":"276709","messageId":"20160125095629.GB24452@sigill.intra.peff.net","threadId":"41250","inReplyTo":"CAPig+cTq0j0ss=qw9Dx8-PqFA5WJwP0mpvoO+5=NXtOt2EUNww@mail.gmail.com","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-25T09:56:29Z","receivedAt":"2016-01-25T09:56:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 24, 2016 at 06:39:05PM -0500, Eric Sunshine wrote:\n\n> > Yeah, \"strip=2\" would also get the job done, and extends more naturally\n> > to the branch case.\n> >\n> > To be honest, I cannot imagine anybody using anything _but_ strip=2, but\n> > maybe there are special cases, like:\n> >\n> > git for-each-ref --format='%(refname:strip=3)' refs/heads/jk/\n> >\n> > to get my list of topics, sans initials.\n> \n> What if the option was named \":stripprefix=\" in its most general form:\n> \n> %(refname:stripprefix=refs/tags/)\n> \n> with plain:\n> \n> %(refname:stripprefix)\n> \n> shorthand for \":stripprefix=refs/*/\" or something?\n\nThat would work, though I was really hoping not to get into something as\ncomplicated as globbing. I'm not sure it really buys us that much here.\n\n-Peff\n"},{"id":"276711","messageId":"20160125100141.GC24452@sigill.intra.peff.net","threadId":"41250","inReplyTo":"xmqqsi1m9yxh.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-25T10:01:41Z","receivedAt":"2016-01-25T10:01:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Jan 24, 2016 at 06:26:50PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Yeah, \"strip=2\" would also get the job done, and extends more naturally\n> > to the branch case.\n> >\n> > To be honest, I cannot imagine anybody using anything _but_ strip=2...\n> \n> I 100% agree, and I do consider this to be internal implementation\n> detail for the listing modes of \"tag\" (and \"branch\"), which may be\n> exposed to the user (by documenting that %(refname:X) is used by\n> default), so perhaps even the flexibility of strip=2 is unwanted.\n> \n> I know what \"remove-standard-prefix\" is way too long for the value\n> of X above, but then we can say \"the command will error out if you\n> allow your for-each-ref invocation to step outside of the area that\n> has standard prefix to be removed.\" without having to worry about\n> \"what is the sensible thing to do when the prefixes are not what we\n> expect (too short for strip=2 or no match for short=refs/tags/)\".\n\nI'm not sure \"remove-standard-prefix\" doesn't open its own questions.\nLike \"what are the standard prefixes?\".\n\nIf we are going to go with \"remove a prefix\", I really don't think\n\"remove if present\" is too complicated a set of semantics (as opposed to\n\"error out\" you mentioned above).\n\nI do like \"strip=\" for its simplicity (it's easy to explain), and the\nfact that it will probably handle the git-branch case for us. The only\nopen question is what to do if there are fewer components, but I really\nthink any of the 4 behaviors I gave earlier would be fine.\n\nEric' globbing suggestion is simpler for the error case (as a prefix, it\ncan be \"remove if present\"), but I think introducing globbing in general\nopens up too many corner cases (e.g., does \"*\" match \"/\", is \"**\"\nsupported, etc).\n\n-Peff\n"},{"id":"276729","messageId":"CAOLa=ZQmX6QMTz+dki+q07F+pHjUETLRdjJR5=nw8meYfoyEeQ@mail.gmail.com","threadId":"41250","inReplyTo":"20160125100141.GC24452@sigill.intra.peff.net","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-25T19:29:50Z","receivedAt":"2016-01-25T19:29:50Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Mon, Jan 25, 2016 at 3:31 PM, Jeff King wrote:\n> On Sun, Jan 24, 2016 at 06:26:50PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>>\n>> > Yeah, \"strip=2\" would also get the job done, and extends more naturally\n>> > to the branch case.\n>> >\n>> > To be honest, I cannot imagine anybody using anything _but_ strip=2...\n>>\n>> I 100% agree, and I do consider this to be internal implementation\n>> detail for the listing modes of \"tag\" (and \"branch\"), which may be\n>> exposed to the user (by documenting that %(refname:X) is used by\n>> default), so perhaps even the flexibility of strip=2 is unwanted.\n>>\n>> I know what \"remove-standard-prefix\" is way too long for the value\n>> of X above, but then we can say \"the command will error out if you\n>> allow your for-each-ref invocation to step outside of the area that\n>> has standard prefix to be removed.\" without having to worry about\n>> \"what is the sensible thing to do when the prefixes are not what we\n>> expect (too short for strip=2 or no match for short=refs/tags/)\".\n>\n> I'm not sure \"remove-standard-prefix\" doesn't open its own questions.\n> Like \"what are the standard prefixes?\".\n>\n> If we are going to go with \"remove a prefix\", I really don't think\n> \"remove if present\" is too complicated a set of semantics (as opposed to\n> \"error out\" you mentioned above).\n>\n> I do like \"strip=\" for its simplicity (it's easy to explain), and the\n> fact that it will probably handle the git-branch case for us. The only\n> open question is what to do if there are fewer components, but I really\n> think any of the 4 behaviors I gave earlier would be fine.\n\nThis seems ideal, it's also quite useful like your mentioned example.\nAnd would provide common ground for the upcoming git-branch patches\nas you said.\n\nWhat about a scenario wherein we have\nrefs/branches/abc/foo\nrefs/branches/xyz\n\nshould --format=\"%(refname:strip=3)\" give us\n foo\n xyz (here stripping till 2 '/', which is the maximum)\n\nor\n foo\n refs/branches/xyz (no stripping done at all)\n\nI prefer the former, cause that would allow us to give a maximum value for\nstripping and have everything below that maximum value stripped as well.\n\n-- \nRegards,\nKarthik Nayak\n"},{"id":"276734","messageId":"xmqqmvrtfm0u.fsf@gitster.mtv.corp.google.com","threadId":"41250","inReplyTo":"20160125100141.GC24452@sigill.intra.peff.net","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-25T20:21:21Z","receivedAt":"2016-01-25T20:21:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'm not sure \"remove-standard-prefix\" doesn't open its own questions.\n> Like \"what are the standard prefixes?\".\n\nIt is easy to define, no? This is invented for the internal use of\nthe listing modes of \"tag\" and \"branch\", so the users are welcome to\nuse it if they see fit, but how the prefixes are stripped is defined\nby the convenience of these commands--the behaviour might even change\nwhen these commands are enhanced.\n\n> If we are going to go with \"remove a prefix\", I really don't think\n> \"remove if present\" is too complicated a set of semantics (as opposed to\n> \"error out\" you mentioned above).\n>\n> I do like \"strip=\" for its simplicity (it's easy to explain), and the\n> fact that it will probably handle the git-branch case for us. The only\n> open question is what to do if there are fewer components, but I really\n> think any of the 4 behaviors I gave earlier would be fine.\n>\n> Eric' globbing suggestion is simpler for the error case (as a prefix, it\n> can be \"remove if present\"), but I think introducing globbing in general\n> opens up too many corner cases (e.g., does \"*\" match \"/\", is \"**\"\n> supported, etc).\n\nYeah, I really do not like any of the \"do not error out but assume\nthat the user would not care about the ambiguities\" solutions for\nsomething we primarily intend to use for internal purposes.\n\nI agree that \"strip=\", \"remove-prefix=\", and the friends\nare good for end-user scripting, but they can come later, outside of\nthe scope of this regression fix, and that is the proper time to\ndebate and decide if it is really ok to assume that the user does\nnot care about ambiguity, or it is prudent to error out. A separate\n\"remove-standard-prefix\" that is meant for internal use would allow\nus to push the fix out without having to decide now.\n"},{"id":"276749","messageId":"xmqq60yhdx4v.fsf@gitster.mtv.corp.google.com","threadId":"41250","inReplyTo":"20160124230840.GB16455@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-26T00:04:16Z","receivedAt":"2016-01-26T00:04:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Since b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),\n> git-tag has started showing tags with ambiguous names (i.e.,\n> when both \"heads/foo\" and \"tags/foo\" exists) as \"tags/foo\"\n> instead of just \"foo\". This is both:\n>\n> - pointless; the output of \"git tag\" includes only\n> refs/tags, so we know that \"foo\" means the one in\n> \"refs/tags\".\n>\n> and\n>\n> - ambiguous; in the original output, we know that the line\n> \"foo\" means that \"refs/tags/foo\" exists. In the new\n> output, it is unclear whether we mean \"refs/tags/foo\" or\n> \"refs/tags/tags/foo\".\n>\n> The reason this happens is that commit b7cc53e9 switched\n> git-tag to use ref-filter's \"%(refname:short)\" output\n> formatting, which was adapted from for-each-ref.\n> ...\n\nKarthik, getting a fix in for \"git tag\" regression is more important\nthan the topics parked in 'pu', so I'll queue this patch in the\nearly part of 'pu'.\n\nI personally feel that \"refname:strip=\" would be a good mechanism\nfor end users to specify a custom format, and it is unclear to me\nwhat should happen when there are not enough elements to be\nstripped, so I do not think we want to cast the \"we will show the\nwhole thing\" decision in stone prematurely only because we want to\npush out the regression fix soon. So I may ask Jeff to rework this\none (or I may end up trying to do so myself) not to squat on the\nnice strip= notation. refname:strip-standard-prefix that removes\nthe known prefix (\"refs/heads\", \"refs/remotes\" and \"refs/tags\") if\npresent and does not touch the refname otherwise would leave us more\ntime to decide what strip= should do in the error case.\n\nUnfortunately, this means kn/ref-filter-atom-parsing topic from you\nthat were parked on 'pu' must be ejected for now, as any change in\nthis area overlaps with it, and the atom parsing code would need to\nbe updated to learn about the new attribute of the 'refname' atom\n(be it 'remove-prefix=', 'strip=', or something else) that\nwe would decide to use for the regression fix anyway.\n\nThanks.\n\n> Documentation/git-for-each-ref.txt | 6 +++++-\n> Documentation/git-tag.txt | 2 +-\n> builtin/tag.c | 4 ++--\n> ref-filter.c | 13 ++++++++++++-\n> t/t3203-branch-output.sh | 8 ++++++++\n> t/t6300-for-each-ref.sh | 4 ++++\n> t/t7004-tag.sh | 8 ++++++++\n> 7 files changed, 40 insertions(+), 5 deletions(-)\n>\n> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\n> index 06208c4..f15c817 100644\n> --- a/Documentation/git-for-each-ref.txt\n> +++ b/Documentation/git-for-each-ref.txt\n> @@ -92,7 +92,11 @@ refname::\n> \tThe name of the ref (the part after $GIT_DIR/).\n> \tFor a non-ambiguous short name of the ref append `:short`.\n> \tThe option core.warnAmbiguousRefs is used to select the strict\n> -\tabbreviation mode.\n> +\tabbreviation mode. If `strip=` is appended, strips ``\n> +\tslash-separated path components from the front of the refname\n> +\t(e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.\n> +\tIf the ref has fewer components than ``, the whole,\n> +\tunstripped `%(refname)` is printed.\n> \n> objecttype::\n> \tThe type of the object (`blob`, `tree`, `commit`, `tag`).\n> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\n> index 7220e5e..abab481 100644\n> --- a/Documentation/git-tag.txt\n> +++ b/Documentation/git-tag.txt\n> @@ -163,7 +163,7 @@ This option is only applicable when listing tags without annotation lines.\n> \tA string that interpolates `%(fieldname)` from the object\n> \tpointed at by a ref being shown. The format is the same as\n> \tthat of linkgit:git-for-each-ref[1]. When unspecified,\n> -\tdefaults to `%(refname:short)`.\n> +\tdefaults to `%(refname:strip=2)`.\n> \n> --[no-]merged []::\n> \tOnly list tags whose tips are reachable, or not reachable\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 8db8c87..1705c94 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -44,11 +44,11 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\n> \tif (!format) {\n> \t\tif (filter->lines) {\n> \t\t\tto_free = xstrfmt(\"%s %%(contents:lines=%d)\",\n> -\t\t\t\t\t \"%(align:15)%(refname:short)%(end)\",\n> +\t\t\t\t\t \"%(align:15)%(refname:strip=2)%(end)\",\n> \t\t\t\t\t filter->lines);\n> \t\t\tformat = to_free;\n> \t\t} else\n> -\t\t\tformat = \"%(refname:short)\";\n> +\t\t\tformat = \"%(refname:strip=2)\";\n> \t}\n> \n> \tverify_ref_format(format);\n> diff --git a/ref-filter.c b/ref-filter.c\n> index 7bef7f8..9f54adc 100644\n> --- a/ref-filter.c\n> +++ b/ref-filter.c\n> @@ -909,12 +909,23 @@ static void populate_value(struct ref_array_item *ref)\n> \t\tformatp = strchr(name, ':');\n> \t\tif (formatp) {\n> \t\t\tint num_ours, num_theirs;\n> +\t\t\tconst char *arg;\n> \n> \t\t\tformatp++;\n> \t\t\tif (!strcmp(formatp, \"short\"))\n> \t\t\t\trefname = shorten_unambiguous_ref(refname,\n> \t\t\t\t\t\t warn_ambiguous_refs);\n> -\t\t\telse if (!strcmp(formatp, \"track\") &&\n> +\t\t\telse if (skip_prefix(formatp, \"strip=\", &arg)) {\n> +\t\t\t\tint strip = atoi(arg);\n> +\t\t\t\tconst char *start = refname;\n> +\t\t\t\twhile (strip && *start) {\n> +\t\t\t\t\tif (*start == '/')\n> +\t\t\t\t\t\tstrip--;\n> +\t\t\t\t\tstart++;\n> +\t\t\t\t}\n> +\t\t\t\tif (!strip)\n> +\t\t\t\t\trefname = start;\n> +\t\t\t} else if (!strcmp(formatp, \"track\") &&\n> \t\t\t\t (starts_with(name, \"upstream\") ||\n> \t\t\t\t starts_with(name, \"push\"))) {\n> \n> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\n> index d3913f9..4261403 100755\n> --- a/t/t3203-branch-output.sh\n> +++ b/t/t3203-branch-output.sh\n> @@ -176,4 +176,12 @@ test_expect_success 'git branch --points-at option' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'ambiguous branch/tag not marked' '\n> +\tgit tag ambiguous &&\n> +\tgit branch ambiguous &&\n> +\techo \" ambiguous\" >expect &&\n> +\tgit branch --list ambiguous >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\n> index 859b237..1f3abeb 100755\n> --- a/t/t6300-for-each-ref.sh\n> +++ b/t/t6300-for-each-ref.sh\n> @@ -50,6 +50,10 @@ test_atom() {\n> \n> test_atom head refname refs/heads/master\n> test_atom head refname:short master\n> +test_atom head refname:strip=0 refs/heads/master\n> +test_atom head refname:strip=1 heads/master\n> +test_atom head refname:strip=2 master\n> +test_atom head refname:strip=3 refs/heads/master\n> test_atom head upstream refs/remotes/origin/master\n> test_atom head upstream:short origin/master\n> test_atom head push refs/remotes/myfork/master\n> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\n> index 2797f22..cf3469b 100755\n> --- a/t/t7004-tag.sh\n> +++ b/t/t7004-tag.sh\n> @@ -1558,4 +1558,12 @@ test_expect_success '--no-merged show unmerged tags' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'ambiguous branch/tags not marked' '\n> +\tgit tag ambiguous &&\n> +\tgit branch ambiguous &&\n> +\techo ambiguous >expect &&\n> +\tgit tag -l ambiguous >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_done\n"},{"id":"276752","messageId":"20160126023753.GA4012@sigill.intra.peff.net","threadId":"41250","inReplyTo":"xmqqmvrtfm0u.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-26T02:37:53Z","receivedAt":"2016-01-26T02:37:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 25, 2016 at 12:21:21PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I'm not sure \"remove-standard-prefix\" doesn't open its own questions.\n> > Like \"what are the standard prefixes?\".\n> \n> It is easy to define, no? This is invented for the internal use of\n> the listing modes of \"tag\" and \"branch\", so the users are welcome to\n> use it if they see fit, but how the prefixes are stripped is defined\n> by the convenience of these commands--the behaviour might even change\n> when these commands are enhanced.\n\nWhat does this do:\n\n git for-each-ref --format='%(refname:remove-standard-prefix)'\n\n?\n\nIs there no standard prefix, because we are not in branch/tag? Does it\nremove well-known prefixes like \"refs/heads/\", \"refs/tags/\", etc? Does\nit remove the first two components (e.g., what happens to\n\"refs/foo/bar\")? If so, what happens to \"refs/stash\"?\n\nWe can say \"it is all internally defined and subject to change\". But\nthat is not very helpful to a user, and this \"it's magic, don't rely on\nit\" wart will be part of the user-facing interface. We have to write\n_something_ in the \"default format\" documentation for git-tag.\n\n> Yeah, I really do not like any of the \"do not error out but assume\n> that the user would not care about the ambiguities\" solutions for\n> something we primarily intend to use for internal purposes.\n> \n> I agree that \"strip=\", \"remove-prefix=\", and the friends\n> are good for end-user scripting, but they can come later, outside of\n> the scope of this regression fix, and that is the proper time to\n> debate and decide if it is really ok to assume that the user does\n> not care about ambiguity, or it is prudent to error out. A separate\n> \"remove-standard-prefix\" that is meant for internal use would allow\n> us to push the fix out without having to decide now.\n\nYeah, I am definitely on board with trying to do the most minimal thing\nfor the regression fix and worry about more advanced concerns later (if\nat all). It seems to me that \"error out\" is a pretty minimal behavior,\nthough, and one that doesn't lock us into any behaviors (i.e., it is\ngenerally OK to take something that did not work at all before, and give\nit new useful behavior; it is not OK to change something that did\nsomething useful before).\n\nSo what about this:\n\n 1. Implement \":strip=\" and use it from git-tag.\n\n 2. Have it error out on a ref with fewer than components. This\n should be impossible to trigger via \"git-tag\" with the default\n format.\n\n 3. Explicitly document that the behavior for values of that are\n not positive integers is undefined and subject to change (or\n alternatively, we can simply error out).\n\nThat _is_ user-visible, but it seems like \"strip\" is a fairly flexible\nmechanism by itself (enough that I do not mind living with it forever),\nand we haven't made any promises about the ambiguous parts.\n\n\nIf we are going to do something _really_ internal and undocumented to\nfix the regression, I think I would rather not introduce a new formatter\nat all, but simply teach \"%(refname:short)\" to do the magic internal\nshortening in the context of git-tag. That does not let people emulate\n\"tag\" with \"for-each-ref\", but that is not part of the regression or its\nfix.\n\n-Peff\n"},{"id":"276753","messageId":"20160126030005.GA27992@sigill.intra.peff.net","threadId":"41250","inReplyTo":"20160126023753.GA4012@sigill.intra.peff.net","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-26T03:00:05Z","receivedAt":"2016-01-26T03:00:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 25, 2016 at 09:37:53PM -0500, Jeff King wrote:\n\n> So what about this:\n> \n> 1. Implement \":strip=\" and use it from git-tag.\n> \n> 2. Have it error out on a ref with fewer than components. This\n> should be impossible to trigger via \"git-tag\" with the default\n> format.\n> \n> 3. Explicitly document that the behavior for values of that are\n> not positive integers is undefined and subject to change (or\n> alternatively, we can simply error out).\n> \n> That _is_ user-visible, but it seems like \"strip\" is a fairly flexible\n> mechanism by itself (enough that I do not mind living with it forever),\n> and we haven't made any promises about the ambiguous parts.\n\nHere's the matching patch. It's \"v3 2/2\", where \"v3 1/2\" is the same\ntest-cleanup patch I posted earlier.\n\n-- >8 --\nSubject: [PATCH v2 2/2] tag: do not show ambiguous tag names as \"tags/foo\"\n\nSince b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),\ngit-tag has started showing tags with ambiguous names (i.e.,\nwhen both \"heads/foo\" and \"tags/foo\" exists) as \"tags/foo\"\ninstead of just \"foo\". This is both:\n\n - pointless; the output of \"git tag\" includes only\n refs/tags, so we know that \"foo\" means the one in\n \"refs/tags\".\n\nand\n\n - ambiguous; in the original output, we know that the line\n \"foo\" means that \"refs/tags/foo\" exists. In the new\n output, it is unclear whether we mean \"refs/tags/foo\" or\n \"refs/tags/tags/foo\".\n\nThe reason this happens is that commit b7cc53e9 switched\ngit-tag to use ref-filter's \"%(refname:short)\" output\nformatting, which was adapted from for-each-ref. This more\ngeneral code does not know that we care only about tags, and\nuses shorten_unambiguous_ref to get the short-name. We need\nto tell it that we care only about \"refs/tags/\", and it\nshould shorten with respect to that value.\n\nIn theory, the ref-filter code could figure this out by us\npassing FILTER_REFS_TAGS. But there are two complications\nthere:\n\n 1. The handling of refname:short is deep in formatting\n code that does not even have our ref_filter struct, let\n alone the arguments to the filter_ref struct.\n\n 2. In git v2.7.0, we expose the formatting language to the\n user. If we follow this path, it will mean that\n \"%(refname:short)\" behaves differently for \"tag\" versus\n \"for-each-ref\" (including \"for-each-ref refs/tags/\"),\n which can lead to confusion.\n\nInstead, let's add a new modifier to the formatting\nlanguage, \"strip\", to remove a specific set of prefix\ncomponents. This fixes \"git tag\", and lets users invoke the\nsame behavior from their own custom formats (for \"tag\" or\n\"for-each-ref\") while leaving \":short\" with its same\nconsistent meaning in all places.\n\nWe introduce a test in t7004 for \"git tag\", which fails\nwithout this patch. We also add a similar test in t3203 for\n\"git branch\", which does not actually fail. But since it is\nlikely that \"branch\" will eventually use the same formatting\ncode, the test helps defend against future regressions.\n\nSigned-off-by: Jeff King \n---\n Documentation/git-for-each-ref.txt | 6 +++++-\n Documentation/git-tag.txt | 2 +-\n builtin/tag.c | 4 ++--\n ref-filter.c | 26 ++++++++++++++++++++++++++\n t/t3203-branch-output.sh | 8 ++++++++\n t/t6300-for-each-ref.sh | 12 ++++++++++++\n t/t7004-tag.sh | 8 ++++++++\n 7 files changed, 62 insertions(+), 4 deletions(-)\n\ndiff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt\nindex 06208c4..2e3e96f 100644\n--- a/Documentation/git-for-each-ref.txt\n+++ b/Documentation/git-for-each-ref.txt\n@@ -92,7 +92,11 @@ refname::\n \tThe name of the ref (the part after $GIT_DIR/).\n \tFor a non-ambiguous short name of the ref append `:short`.\n \tThe option core.warnAmbiguousRefs is used to select the strict\n-\tabbreviation mode.\n+\tabbreviation mode. If `strip=` is appended, strips ``\n+\tslash-separated path components from the front of the refname\n+\t(e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.\n+\t`` must be a positive integer. If a displayed ref has fewer\n+\tcomponents than ``, the command aborts with an error.\n \n objecttype::\n \tThe type of the object (`blob`, `tree`, `commit`, `tag`).\ndiff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt\nindex 7220e5e..abab481 100644\n--- a/Documentation/git-tag.txt\n+++ b/Documentation/git-tag.txt\n@@ -163,7 +163,7 @@ This option is only applicable when listing tags without annotation lines.\n \tA string that interpolates `%(fieldname)` from the object\n \tpointed at by a ref being shown. The format is the same as\n \tthat of linkgit:git-for-each-ref[1]. When unspecified,\n-\tdefaults to `%(refname:short)`.\n+\tdefaults to `%(refname:strip=2)`.\n \n --[no-]merged []::\n \tOnly list tags whose tips are reachable, or not reachable\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 8db8c87..1705c94 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -44,11 +44,11 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con\n \tif (!format) {\n \t\tif (filter->lines) {\n \t\t\tto_free = xstrfmt(\"%s %%(contents:lines=%d)\",\n-\t\t\t\t\t \"%(align:15)%(refname:short)%(end)\",\n+\t\t\t\t\t \"%(align:15)%(refname:strip=2)%(end)\",\n \t\t\t\t\t filter->lines);\n \t\t\tformat = to_free;\n \t\t} else\n-\t\t\tformat = \"%(refname:short)\";\n+\t\t\tformat = \"%(refname:strip=2)\";\n \t}\n \n \tverify_ref_format(format);\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 7bef7f8..f097176 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -763,6 +763,29 @@ static inline char *copy_advance(char *dst, const char *src)\n \treturn dst;\n }\n \n+static const char *strip_ref_components(const char *refname, const char *nr_arg)\n+{\n+\tchar *end;\n+\tlong nr = strtol(nr_arg, &end, 10);\n+\tlong remaining = nr;\n+\tconst char *start = refname;\n+\n+\tif (nr < 1 || *end != '\\0')\n+\t\tdie(\":strip= requires a positive integer argument\");\n+\n+\twhile (remaining) {\n+\t\tswitch (*start++) {\n+\t\tcase '\\0':\n+\t\t\tdie(\"ref '%s' does not have %ld components to :strip\",\n+\t\t\t refname, nr);\n+\t\tcase '/':\n+\t\t\tremaining--;\n+\t\t\tbreak;\n+\t\t}\n+\t}\n+\treturn start;\n+}\n+\n /*\n * Parse the object referred by ref, and grab needed value.\n */\n@@ -909,11 +932,14 @@ static void populate_value(struct ref_array_item *ref)\n \t\tformatp = strchr(name, ':');\n \t\tif (formatp) {\n \t\t\tint num_ours, num_theirs;\n+\t\t\tconst char *arg;\n \n \t\t\tformatp++;\n \t\t\tif (!strcmp(formatp, \"short\"))\n \t\t\t\trefname = shorten_unambiguous_ref(refname,\n \t\t\t\t\t\t warn_ambiguous_refs);\n+\t\t\telse if (skip_prefix(formatp, \"strip=\", &arg))\n+\t\t\t\trefname = strip_ref_components(refname, arg);\n \t\t\telse if (!strcmp(formatp, \"track\") &&\n \t\t\t\t (starts_with(name, \"upstream\") ||\n \t\t\t\t starts_with(name, \"push\"))) {\ndiff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh\nindex d3913f9..4261403 100755\n--- a/t/t3203-branch-output.sh\n+++ b/t/t3203-branch-output.sh\n@@ -176,4 +176,12 @@ test_expect_success 'git branch --points-at option' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ambiguous branch/tag not marked' '\n+\tgit tag ambiguous &&\n+\tgit branch ambiguous &&\n+\techo \" ambiguous\" >expect &&\n+\tgit branch --list ambiguous >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\ndiff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh\nindex 859b237..19a2823 100755\n--- a/t/t6300-for-each-ref.sh\n+++ b/t/t6300-for-each-ref.sh\n@@ -50,6 +50,8 @@ test_atom() {\n \n test_atom head refname refs/heads/master\n test_atom head refname:short master\n+test_atom head refname:strip=1 heads/master\n+test_atom head refname:strip=2 master\n test_atom head upstream refs/remotes/origin/master\n test_atom head upstream:short origin/master\n test_atom head push refs/remotes/myfork/master\n@@ -132,6 +134,16 @@ test_expect_success 'Check invalid atoms names are errors' '\n \ttest_must_fail git for-each-ref --format=\"%(INVALID)\" refs/heads\n '\n \n+test_expect_success 'arguments to :strip must be positive integers' '\n+\ttest_must_fail git for-each-ref --format=\"%(refname:strip=0)\" &&\n+\ttest_must_fail git for-each-ref --format=\"%(refname:strip=-1)\" &&\n+\ttest_must_fail git for-each-ref --format=\"%(refname:strip=foo)\"\n+'\n+\n+test_expect_success 'stripping refnames too far gives an error' '\n+\ttest_must_fail git for-each-ref --format=\"%(refname:strip=3)\"\n+'\n+\n test_expect_success 'Check format specifiers are ignored in naming date atoms' '\n \tgit for-each-ref --format=\"%(authordate)\" refs/heads &&\n \tgit for-each-ref --format=\"%(authordate:default) %(authordate)\" refs/heads &&\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 2797f22..cf3469b 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -1558,4 +1558,12 @@ test_expect_success '--no-merged show unmerged tags' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'ambiguous branch/tags not marked' '\n+\tgit tag ambiguous &&\n+\tgit branch ambiguous &&\n+\techo ambiguous >expect &&\n+\tgit tag -l ambiguous >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.7.0.427.g4c6e021\n"},{"id":"276754","messageId":"xmqq1t95dnt5.fsf@gitster.mtv.corp.google.com","threadId":"41250","inReplyTo":"20160126023753.GA4012@sigill.intra.peff.net","subject":"Re: [PATCH] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-26T03:25:42Z","receivedAt":"2016-01-26T03:25:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> What does this do:\n>\n> git for-each-ref --format='%(refname:remove-standard-prefix)'\n>\n> ?\n>\n> Is there no standard prefix, because we are not in branch/tag? Does it\n> remove well-known prefixes like \"refs/heads/\", \"refs/tags/\", etc? Does\n> it remove the first two components (e.g., what happens to\n> \"refs/foo/bar\")? If so, what happens to \"refs/stash\"?\n\nI think our mails crossed. I envisioned that the documentation\nwould go something like this:\n\nremove-standard-prefix::\n\tWhen the refname begins with one of the well-known prefixes,\n\tthe prefix is stripped from it (and when it does not start\n\twith any of the well-known prefixes, the refname is left\n\tas-is). \"refs/heads/\", \"refs/remotes/\" and \"refs/tags/\" are\n\tthe well-known prefixes that are removed (as this modifier\n\tis primarily meant to allow users emulate the listing modes\n\tof \"git tag\" and \"git branch\") but this set may change over\n\ttime (we may teach it to remove \"refs/changes/\" in later\n\tversions of Git, for example).\n\n> Yeah, I am definitely on board with trying to do the most minimal thing\n> for the regression fix and worry about more advanced concerns later (if\n> at all). It seems to me that \"error out\" is a pretty minimal behavior,\n> though, and one that doesn't lock us into any behaviors (i.e., it is\n> generally OK to take something that did not work at all before, and give\n> it new useful behavior; it is not OK to change something that did\n> something useful before).\n\nThe thing that worried me the most is that \"strip=\" is a very\nintuitive and nice notation that end users would want to use it\nbeyond emulating \"git tag\" literally, and one behaviour we happen to\npick, be it \"error out\" or \"leave it intact\", would have a high\nchance of being not the most useful one. If we define it to error\nout, somebody somewhere will abuse it to \"ensure that all branch\nnames are at least two levels deep\" or something we do not\nanticipate now and start relying on the \"erroring out\" behaviour,\nand then complain when we later \"allow it to do something more\nuseful\" that it no longer errors out.\n\nHaving said all that, I think I can live with \"strip=\" that\nhappens to pick a single behaviour that we pick with the best\nknowledge we have right now. If we fear the compatiblity wart so\nseriously, we can always add a separate \"strip-nofail=\" variant\nthat gives a different behaviour from \"strip=\" that errors out.\n\nAnother useful variant might be \"strip levels if we can,\notherwise pretend that the ref did not even pass the filtering\ncriteria and do not show anything about the ref on the output\", by\nthe way.\n\n> So what about this:\n>\n> 1. Implement \":strip=\" and use it from git-tag.\n>\n> 2. Have it error out on a ref with fewer than components. This\n> should be impossible to trigger via \"git-tag\" with the default\n> format.\n>\n> 3. Explicitly document that the behavior for values of that are\n> not positive integers is undefined and subject to change (or\n> alternatively, we can simply error out).\n>\n>\n> That _is_ user-visible, but it seems like \"strip\" is a fairly flexible\n> mechanism by itself (enough that I do not mind living with it forever),\n> and we haven't made any promises about the ambiguous parts.\n\nOK.\n"},{"id":"276755","messageId":"CAOLa=ZRWGHf=Q3L6x5wf1Aiy2Q=T2117+YGPxMxM-weYkC+CoQ@mail.gmail.com","threadId":"41250","inReplyTo":"xmqq60yhdx4v.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] tag: do not show ambiguous tag names as \"tags/foo\"","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2016-01-26T04:25:08Z","receivedAt":"2016-01-26T04:25:08Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"On Tue, Jan 26, 2016 at 5:34 AM, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> Since b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),\n>> git-tag has started showing tags with ambiguous names (i.e.,\n>> when both \"heads/foo\" and \"tags/foo\" exists) as \"tags/foo\"\n>> instead of just \"foo\". This is both:\n>>\n>> - pointless; the output of \"git tag\" includes only\n>> refs/tags, so we know that \"foo\" means the one in\n>> \"refs/tags\".\n>>\n>> and\n>>\n>> - ambiguous; in the original output, we know that the line\n>> \"foo\" means that \"refs/tags/foo\" exists. In the new\n>> output, it is unclear whether we mean \"refs/tags/foo\" or\n>> \"refs/tags/tags/foo\".\n>>\n>> The reason this happens is that commit b7cc53e9 switched\n>> git-tag to use ref-filter's \"%(refname:short)\" output\n>> formatting, which was adapted from for-each-ref.\n>> ...\n>\n> Karthik, getting a fix in for \"git tag\" regression is more important\n> than the topics parked in 'pu', so I'll queue this patch in the\n> early part of 'pu'.\n>\n> I personally feel that \"refname:strip=\" would be a good mechanism\n> for end users to specify a custom format, and it is unclear to me\n> what should happen when there are not enough elements to be\n> stripped, so I do not think we want to cast the \"we will show the\n> whole thing\" decision in stone prematurely only because we want to\n> push out the regression fix soon. So I may ask Jeff to rework this\n> one (or I may end up trying to do so myself) not to squat on the\n> nice strip= notation. refname:strip-standard-prefix that removes\n> the known prefix (\"refs/heads\", \"refs/remotes\" and \"refs/tags\") if\n> present and does not touch the refname otherwise would leave us more\n> time to decide what strip= should do in the error case.\n>\n> Unfortunately, this means kn/ref-filter-atom-parsing topic from you\n> that were parked on 'pu' must be ejected for now, as any change in\n> this area overlaps with it, and the atom parsing code would need to\n> be updated to learn about the new attribute of the 'refname' atom\n> (be it 'remove-prefix=', 'strip=', or something else) that\n> we would decide to use for the regression fix anyway.\n\nThat should be fine, there are still changes to be done there so I'll rebase\non this and send that series.\n\n-- \nRegards,\nKarthik Nayak\n"}]}