Regression? Ambiguous tags listed as "tags/<foo>"

Regression? Ambiguous tags listed as "tags/<foo>"

[Pete Harlan]

There was a behavior change in "git tag" in b7cc53e9 (tag.c: use
'ref-filter' APIs, 2015-07-11, v2.5.0-276-gb7cc53e), such that "git
tag" now writes "foo" as "tags/foo" if there is also a branch named
"foo":

    % git tag
    tags/branchortag
    tagonly

Previous behavior:

    % git tag
    branchortag
    tagonly

I prefer the previous behavior.  Perhaps the change was intentional,
but I didn't see it documented.

Thanks,

Pete Harlan
pgit@tento.net

[PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Sat, Jan 23, 2016 at 03:56:12PM -0800, Pete Harlan wrote:

> There was a behavior change in "git tag" in b7cc53e9 (tag.c: use
> 'ref-filter' APIs, 2015-07-11, v2.5.0-276-gb7cc53e), such that "git
> tag" now writes "foo" as "tags/foo" if there is also a branch named
> "foo":
> 
>     % git tag
>     tags/branchortag
>     tagonly
> 
> Previous behavior:
> 
>     % git tag
>     branchortag
>     tagonly
> 
> I prefer the previous behavior.  Perhaps the change was intentional,
> but I didn't see it documented.

I don't think it was intentional, and I think the new output is actively
wrong. My reasoning (and a fix) are below.

-- >8 --
Subject: tag: do not show ambiguous tag names as "tags/foo"

Since b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),
git-tag has started showing tags with ambiguous names (i.e.,
when both "heads/foo" and "tags/foo" exists) as "tags/foo"
instead of just "foo". This is both:

  - pointless; the output of "git tag" includes only
    refs/tags, so we know that "foo" means the one in
    "refs/tags".

and

  - ambiguous; in the original output, we know that the line
    "foo" means that "refs/tags/foo" exists. In the new
    output, it is unclear whether we mean "refs/tags/foo" or
    "refs/tags/tags/foo".

The reason this happens is that commit b7cc53e9 switched
git-tag to use ref-filter's "%(refname:short)" output
formatting, which was adapted from for-each-ref. This more
general code does not know that we care only about tags, and
uses shorten_unambiguous_ref to get the short-name. We need
to tell it that we care only about "refs/tags/", and it
should shorten with respect to that value.

In theory, the ref-filter code could figure this out by us
passing FILTER_REFS_TAGS. But there are two complications
there:

  1. The handling of refname:short is deep in formatting
     code that does not even have our ref_filter struct, let
     alone the arguments to the filter_ref struct.

  2. In git v2.7.0, we expose the formatting language to the
     user. If we follow this path, it will mean that
     "%(refname:short)" behaves differently for "tag" versus
     "for-each-ref" (including "for-each-ref refs/tags/"),
     which can lead to confusion.

Instead, let's extend the "short" modifier in the formatting
language to handle a specific prefix. This fixes "git tag",
and lets users invoke the same behavior from their own
custom formats (for "tag" or "for-each-ref") while leaving
":short" with its same consistent meaning in all places.

We introduce a test in t7004 for "git tag", which fails
without this patch. We also add a similar test in t3203 for
"git branch", which does not actually fail. But since it is
likely that "branch" will eventually use the same formatting
code, the test helps defend against future regressions.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-for-each-ref.txt | 4 +++-
 Documentation/git-tag.txt          | 2 +-
 builtin/tag.c                      | 4 ++--
 ref-filter.c                       | 3 +++
 t/t3203-branch-output.sh           | 8 ++++++++
 t/t7004-tag.sh                     | 8 ++++++++
 6 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 06208c4..faf10c5 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -92,7 +92,9 @@ refname::
 	The name of the ref (the part after $GIT_DIR/).
 	For a non-ambiguous short name of the ref append `:short`.
 	The option core.warnAmbiguousRefs is used to select the strict
-	abbreviation mode.
+	abbreviation mode. For shortening in a specific hierarchy, use
+	`:short=<prefix>`, which will remove `<prefix>` (if present)
+	from the front of the ref. E.g., `%(refname:short=refs/tags/)`.
 
 objecttype::
 	The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 7220e5e..62ff509 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -163,7 +163,7 @@ This option is only applicable when listing tags without annotation lines.
 	A string that interpolates `%(fieldname)` from the object
 	pointed at by a ref being shown.  The format is the same as
 	that of linkgit:git-for-each-ref[1].  When unspecified,
-	defaults to `%(refname:short)`.
+	defaults to `%(refname:short=refs/tags/)`.
 
 --[no-]merged [<commit>]::
 	Only list tags whose tips are reachable, or not reachable
diff --git a/builtin/tag.c b/builtin/tag.c
index 8db8c87..f60feb1 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -44,11 +44,11 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
 	if (!format) {
 		if (filter->lines) {
 			to_free = xstrfmt("%s %%(contents:lines=%d)",
-					  "%(align:15)%(refname:short)%(end)",
+					  "%(align:15)%(refname:short=refs/tags/)%(end)",
 					  filter->lines);
 			format = to_free;
 		} else
-			format = "%(refname:short)";
+			format = "%(refname:short=refs/tags/)";
 	}
 
 	verify_ref_format(format);
diff --git a/ref-filter.c b/ref-filter.c
index 7bef7f8..b878b77 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -909,11 +909,14 @@ static void populate_value(struct ref_array_item *ref)
 		formatp = strchr(name, ':');
 		if (formatp) {
 			int num_ours, num_theirs;
+			const char *prefix;
 
 			formatp++;
 			if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
+			else if (skip_prefix(formatp, "short=", &prefix))
+				skip_prefix(refname, prefix, &refname);
 			else if (!strcmp(formatp, "track") &&
 				 (starts_with(name, "upstream") ||
 				  starts_with(name, "push"))) {
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index d3913f9..4261403 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -176,4 +176,12 @@ test_expect_success 'git branch --points-at option' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ambiguous branch/tag not marked' '
+	git tag ambiguous &&
+	git branch ambiguous &&
+	echo "  ambiguous" >expect &&
+	git branch --list ambiguous >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 2797f22..cf3469b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1558,4 +1558,12 @@ test_expect_success '--no-merged show unmerged tags' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ambiguous branch/tags not marked' '
+	git tag ambiguous &&
+	git branch ambiguous &&
+	echo ambiguous >expect &&
+	git tag -l ambiguous >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.7.0.425.g047222e

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Sun, Jan 24, 2016 at 02:12:35AM -0500, Jeff King wrote:

> In theory, the ref-filter code could figure this out by us
> passing FILTER_REFS_TAGS. But there are two complications
> there:
> 
>   1. The handling of refname:short is deep in formatting
>      code that does not even have our ref_filter struct, let
>      alone the arguments to the filter_ref struct.
> 
>   2. In git v2.7.0, we expose the formatting language to the
>      user. If we follow this path, it will mean that
>      "%(refname:short)" behaves differently for "tag" versus
>      "for-each-ref" (including "for-each-ref refs/tags/"),
>      which can lead to confusion.
> 
> Instead, let's extend the "short" modifier in the formatting
> language to handle a specific prefix. This fixes "git tag",
> and lets users invoke the same behavior from their own
> custom formats (for "tag" or "for-each-ref") while leaving
> ":short" with its same consistent meaning in all places.

I think the patch I posted is a reasonable way to go. But I also don't
think that having "%(refname:short)" behave specially for "git-tag" is
all that unreasonable, either. But I'm open to argument.

Here are a few more considerations I had.

 - I'm not sure if the "special" behavior works as well for git-branch,
   which may want to shorten both "refs/heads/" and "refs/remotes/",
   depending on the type of ref.

   My solution may not extend there naturally, either, depending on how
   it is implemented.

 - To let users get the same behavior out of for-each-ref, we could
   perhaps auto-infer that looking at "refs/tags/" means shortening and
   disambiguation should only happen with respect to the "refs/tags/"
   hierarchy.

   But I'm uncomfortable changing the meaning of ":short" without at
   least a new option. And what would it mean for "git for-each-ref
   refs/heads/foo/"? Would it shorten "refs/heads/foo/bar" to just
   "bar", or would it still be "foo/bar"?

-Peff

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Sun, Jan 24, 2016 at 02:12:35AM -0500, Jeff King wrote:
>
>> In theory, the ref-filter code could figure this out by us
>> passing FILTER_REFS_TAGS. But there are two complications
>> there:
>> 
>>   1. The handling of refname:short is deep in formatting
>>      code that does not even have our ref_filter struct, let
>>      alone the arguments to the filter_ref struct.
>> 
>>   2. In git v2.7.0, we expose the formatting language to the
>>      user. If we follow this path, it will mean that
>>      "%(refname:short)" behaves differently for "tag" versus
>>      "for-each-ref" (including "for-each-ref refs/tags/"),
>>      which can lead to confusion.
>> 
>> Instead, let's extend the "short" modifier in the formatting
>> language to handle a specific prefix. This fixes "git tag",
>> and lets users invoke the same behavior from their own
>> custom formats (for "tag" or "for-each-ref") while leaving
>> ":short" with its same consistent meaning in all places.

Yeah, I do agree with the analysis.

I however wonder if short=$prefix is a good end-user interface,
though, as strip=$prefix may be more intuitive to them, I suspect.

> I think the patch I posted is a reasonable way to go. But I also don't
> think that having "%(refname:short)" behave specially for "git-tag" is
> all that unreasonable, either. But I'm open to argument.
>
> Here are a few more considerations I had.
>
>  - I'm not sure if the "special" behavior works as well for git-branch,
>    which may want to shorten both "refs/heads/" and "refs/remotes/",
>    depending on the type of ref.
>
>    My solution may not extend there naturally, either, depending on how
>    it is implemented.
>
>  - To let users get the same behavior out of for-each-ref, we could
>    perhaps auto-infer that looking at "refs/tags/" means shortening and
>    disambiguation should only happen with respect to the "refs/tags/"
>    hierarchy.
>
>    But I'm uncomfortable changing the meaning of ":short" without at
>    least a new option. And what would it mean for "git for-each-ref
>    refs/heads/foo/"? Would it shorten "refs/heads/foo/bar" to just
>    "bar", or would it still be "foo/bar"?

Also there is "what happens if the expected prefix is not there?"
question.  Perhaps strip=2 can be defined to "strip 2 levels of
hierarchy prefix no matter what that is", and strip refs/tags/foo,
refs/heads/foo and refs/remotes/origin/foo to foo, foo, origin/foo,
respectively?  The filtering natively done by the listing mode of
"branch" and "tags" would ensure the prefixes are always what we
implicitly expect, so the case we need to worry about how we should
signal errors becomes "what if there are not enough levels".  That
may be simpler to handle.

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Sun, Jan 24, 2016 at 02:19:52PM -0800, Junio C Hamano wrote:

> >> Instead, let's extend the "short" modifier in the formatting
> >> language to handle a specific prefix. This fixes "git tag",
> >> and lets users invoke the same behavior from their own
> >> custom formats (for "tag" or "for-each-ref") while leaving
> >> ":short" with its same consistent meaning in all places.
> 
> Yeah, I do agree with the analysis.
> 
> I however wonder if short=$prefix is a good end-user interface,
> though, as strip=$prefix may be more intuitive to them, I suspect.

Yeah, I picked "short" just because of the existing feature. But we are
not bound to use the same name at all, and "strip" is probably more
descriptive (I thought "prefix" might also be, but that only
communicates what it _is_, not that we are removing it).

> Also there is "what happens if the expected prefix is not there?"
> question.

I think "do not strip anything" (as I have here) is an OK behavior. It
would not come up for sane requests (i.e., you would generally be
filtering to match your prefix anyway).

But...

> Perhaps strip=2 can be defined to "strip 2 levels of
> hierarchy prefix no matter what that is", and strip refs/tags/foo,
> refs/heads/foo and refs/remotes/origin/foo to foo, foo, origin/foo,
> respectively?  The filtering natively done by the listing mode of
> "branch" and "tags" would ensure the prefixes are always what we
> implicitly expect, so the case we need to worry about how we should
> signal errors becomes "what if there are not enough levels".  That
> may be simpler to handle.

Yeah, "strip=2" would also get the job done, and extends more naturally
to the branch case.

To be honest, I cannot imagine anybody using anything _but_ strip=2, but
maybe there are special cases, like:

  git for-each-ref --format='%(refname:strip=3)' refs/heads/jk/

to get my list of topics, sans initials.

I had originally hoped to avoid exposing any of this to the user, and
just make things internal, so that we would not be locked into a
particular formatting behavior. But since we now have "tag --format" and
advertise "%(refname:short)" as its default, I think it has become
user-visible.

Let me see what the "strip=X" patch looks like.

-Peff

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Eric Sunshine]

On Sun, Jan 24, 2016 at 5:27 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Jan 24, 2016 at 02:19:52PM -0800, Junio C Hamano wrote:
>> Perhaps strip=2 can be defined to "strip 2 levels of
>> hierarchy prefix no matter what that is", and strip refs/tags/foo,
>> refs/heads/foo and refs/remotes/origin/foo to foo, foo, origin/foo,
>> respectively?  The filtering natively done by the listing mode of
>> "branch" and "tags" would ensure the prefixes are always what we
>> implicitly expect, so the case we need to worry about how we should
>> signal errors becomes "what if there are not enough levels".  That
>> may be simpler to handle.
>
> Yeah, "strip=2" would also get the job done, and extends more naturally
> to the branch case.
>
> To be honest, I cannot imagine anybody using anything _but_ strip=2, but
> maybe there are special cases, like:
>
>   git for-each-ref --format='%(refname:strip=3)' refs/heads/jk/
>
> to get my list of topics, sans initials.

What if the option was named ":stripprefix=" in its most general form:

    %(refname:stripprefix=refs/tags/)

with plain:

    %(refname:stripprefix)

shorthand for ":stripprefix=refs/*/" or something?

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Sun, Jan 24, 2016 at 06:39:05PM -0500, Eric Sunshine wrote:

> > Yeah, "strip=2" would also get the job done, and extends more naturally
> > to the branch case.
> >
> > To be honest, I cannot imagine anybody using anything _but_ strip=2, but
> > maybe there are special cases, like:
> >
> >   git for-each-ref --format='%(refname:strip=3)' refs/heads/jk/
> >
> > to get my list of topics, sans initials.
> 
> What if the option was named ":stripprefix=" in its most general form:
> 
>     %(refname:stripprefix=refs/tags/)
> 
> with plain:
> 
>     %(refname:stripprefix)
> 
> shorthand for ":stripprefix=refs/*/" or something?

That would work, though I was really hoping not to get into something as
complicated as globbing. I'm not sure it really buys us that much here.

-Peff

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, "strip=2" would also get the job done, and extends more naturally
> to the branch case.
>
> To be honest, I cannot imagine anybody using anything _but_ strip=2...

I 100% agree, and I do consider this to be internal implementation
detail for the listing modes of "tag" (and "branch"), which may be
exposed to the user (by documenting that %(refname:X) is used by
default), so perhaps even the flexibility of strip=2 is unwanted.

I know what "remove-standard-prefix" is way too long for the value
of X above, but then we can say "the command will error out if you
allow your for-each-ref invocation to step outside of the area that
has standard prefix to be removed." without having to worry about
"what is the sensible thing to do when the prefixes are not what we
expect (too short for strip=2 or no match for short=refs/tags/)".

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Sun, Jan 24, 2016 at 06:26:50PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Yeah, "strip=2" would also get the job done, and extends more naturally
> > to the branch case.
> >
> > To be honest, I cannot imagine anybody using anything _but_ strip=2...
> 
> I 100% agree, and I do consider this to be internal implementation
> detail for the listing modes of "tag" (and "branch"), which may be
> exposed to the user (by documenting that %(refname:X) is used by
> default), so perhaps even the flexibility of strip=2 is unwanted.
> 
> I know what "remove-standard-prefix" is way too long for the value
> of X above, but then we can say "the command will error out if you
> allow your for-each-ref invocation to step outside of the area that
> has standard prefix to be removed." without having to worry about
> "what is the sensible thing to do when the prefixes are not what we
> expect (too short for strip=2 or no match for short=refs/tags/)".

I'm not sure "remove-standard-prefix" doesn't open its own questions.
Like "what are the standard prefixes?".

If we are going to go with "remove a prefix", I really don't think
"remove if present" is too complicated a set of semantics (as opposed to
"error out" you mentioned above).

I do like "strip=<n>" for its simplicity (it's easy to explain), and the
fact that it will probably handle the git-branch case for us. The only
open question is what to do if there are fewer components, but I really
think any of the 4 behaviors I gave earlier would be fine.

Eric' globbing suggestion is simpler for the error case (as a prefix, it
can be "remove if present"), but I think introducing globbing in general
opens up too many corner cases (e.g., does "*" match "/", is "**"
supported, etc).

-Peff

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Karthik Nayak]

On Mon, Jan 25, 2016 at 3:31 PM, Jeff King <peff@peff.net> wrote:
> On Sun, Jan 24, 2016 at 06:26:50PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > Yeah, "strip=2" would also get the job done, and extends more naturally
>> > to the branch case.
>> >
>> > To be honest, I cannot imagine anybody using anything _but_ strip=2...
>>
>> I 100% agree, and I do consider this to be internal implementation
>> detail for the listing modes of "tag" (and "branch"), which may be
>> exposed to the user (by documenting that %(refname:X) is used by
>> default), so perhaps even the flexibility of strip=2 is unwanted.
>>
>> I know what "remove-standard-prefix" is way too long for the value
>> of X above, but then we can say "the command will error out if you
>> allow your for-each-ref invocation to step outside of the area that
>> has standard prefix to be removed." without having to worry about
>> "what is the sensible thing to do when the prefixes are not what we
>> expect (too short for strip=2 or no match for short=refs/tags/)".
>
> I'm not sure "remove-standard-prefix" doesn't open its own questions.
> Like "what are the standard prefixes?".
>
> If we are going to go with "remove a prefix", I really don't think
> "remove if present" is too complicated a set of semantics (as opposed to
> "error out" you mentioned above).
>
> I do like "strip=<n>" for its simplicity (it's easy to explain), and the
> fact that it will probably handle the git-branch case for us. The only
> open question is what to do if there are fewer components, but I really
> think any of the 4 behaviors I gave earlier would be fine.

This seems ideal, it's also quite useful like your mentioned example.
And would provide common ground for the upcoming git-branch patches
as you said.

What about a scenario wherein we have
refs/branches/abc/foo
refs/branches/xyz

should --format="%(refname:strip=3)" give us
    foo
    xyz (here stripping till 2 '/', which is the maximum)

or
    foo
    refs/branches/xyz (no stripping done at all)

I prefer the former, cause that would allow us to give a maximum value for
stripping and have everything below that maximum value stripped as well.

-- 
Regards,
Karthik Nayak

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm not sure "remove-standard-prefix" doesn't open its own questions.
> Like "what are the standard prefixes?".

It is easy to define, no?  This is invented for the internal use of
the listing modes of "tag" and "branch", so the users are welcome to
use it if they see fit, but how the prefixes are stripped is defined
by the convenience of these commands--the behaviour might even change
when these commands are enhanced.

> If we are going to go with "remove a prefix", I really don't think
> "remove if present" is too complicated a set of semantics (as opposed to
> "error out" you mentioned above).
>
> I do like "strip=<n>" for its simplicity (it's easy to explain), and the
> fact that it will probably handle the git-branch case for us. The only
> open question is what to do if there are fewer components, but I really
> think any of the 4 behaviors I gave earlier would be fine.
>
> Eric' globbing suggestion is simpler for the error case (as a prefix, it
> can be "remove if present"), but I think introducing globbing in general
> opens up too many corner cases (e.g., does "*" match "/", is "**"
> supported, etc).

Yeah, I really do not like any of the "do not error out but assume
that the user would not care about the ambiguities" solutions for
something we primarily intend to use for internal purposes.

I agree that "strip=<n>", "remove-prefix=<glob>", and the friends
are good for end-user scripting, but they can come later, outside of
the scope of this regression fix, and that is the proper time to
debate and decide if it is really ok to assume that the user does
not care about ambiguity, or it is prudent to error out.  A separate
"remove-standard-prefix" that is meant for internal use would allow
us to push the fix out without having to decide now.

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Mon, Jan 25, 2016 at 12:21:21PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I'm not sure "remove-standard-prefix" doesn't open its own questions.
> > Like "what are the standard prefixes?".
> 
> It is easy to define, no?  This is invented for the internal use of
> the listing modes of "tag" and "branch", so the users are welcome to
> use it if they see fit, but how the prefixes are stripped is defined
> by the convenience of these commands--the behaviour might even change
> when these commands are enhanced.

What does this do:

  git for-each-ref --format='%(refname:remove-standard-prefix)'

?

Is there no standard prefix, because we are not in branch/tag? Does it
remove well-known prefixes like "refs/heads/", "refs/tags/", etc? Does
it remove the first two components (e.g., what happens to
"refs/foo/bar")? If so, what happens to "refs/stash"?

We can say "it is all internally defined and subject to change". But
that is not very helpful to a user, and this "it's magic, don't rely on
it" wart will be part of the user-facing interface. We have to write
_something_ in the "default format" documentation for git-tag.

> Yeah, I really do not like any of the "do not error out but assume
> that the user would not care about the ambiguities" solutions for
> something we primarily intend to use for internal purposes.
> 
> I agree that "strip=<n>", "remove-prefix=<glob>", and the friends
> are good for end-user scripting, but they can come later, outside of
> the scope of this regression fix, and that is the proper time to
> debate and decide if it is really ok to assume that the user does
> not care about ambiguity, or it is prudent to error out.  A separate
> "remove-standard-prefix" that is meant for internal use would allow
> us to push the fix out without having to decide now.

Yeah, I am definitely on board with trying to do the most minimal thing
for the regression fix and worry about more advanced concerns later (if
at all). It seems to me that "error out" is a pretty minimal behavior,
though, and one that doesn't lock us into any behaviors (i.e., it is
generally OK to take something that did not work at all before, and give
it new useful behavior; it is not OK to change something that did
something useful before).

So what about this:

  1. Implement ":strip=<n>" and use it from git-tag.

  2. Have it error out on a ref with fewer than <n> components. This
     should be impossible to trigger via "git-tag" with the default
     format.

  3. Explicitly document that the behavior for values of <n> that are
     not positive integers is undefined and subject to change (or
     alternatively, we can simply error out).

That _is_ user-visible, but it seems like "strip" is a fairly flexible
mechanism by itself (enough that I do not mind living with it forever),
and we haven't made any promises about the ambiguous parts.


If we are going to do something _really_ internal and undocumented to
fix the regression, I think I would rather not introduce a new formatter
at all, but simply teach "%(refname:short)" to do the magic internal
shortening in the context of git-tag. That does not let people emulate
"tag" with "for-each-ref", but that is not part of the regression or its
fix.

-Peff

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Mon, Jan 25, 2016 at 09:37:53PM -0500, Jeff King wrote:

> So what about this:
> 
>   1. Implement ":strip=<n>" and use it from git-tag.
> 
>   2. Have it error out on a ref with fewer than <n> components. This
>      should be impossible to trigger via "git-tag" with the default
>      format.
> 
>   3. Explicitly document that the behavior for values of <n> that are
>      not positive integers is undefined and subject to change (or
>      alternatively, we can simply error out).
> 
> That _is_ user-visible, but it seems like "strip" is a fairly flexible
> mechanism by itself (enough that I do not mind living with it forever),
> and we haven't made any promises about the ambiguous parts.

Here's the matching patch. It's "v3 2/2", where "v3 1/2" is the same
test-cleanup patch I posted earlier.

-- >8 --
Subject: [PATCH v2 2/2] tag: do not show ambiguous tag names as "tags/foo"

Since b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),
git-tag has started showing tags with ambiguous names (i.e.,
when both "heads/foo" and "tags/foo" exists) as "tags/foo"
instead of just "foo". This is both:

  - pointless; the output of "git tag" includes only
    refs/tags, so we know that "foo" means the one in
    "refs/tags".

and

  - ambiguous; in the original output, we know that the line
    "foo" means that "refs/tags/foo" exists. In the new
    output, it is unclear whether we mean "refs/tags/foo" or
    "refs/tags/tags/foo".

The reason this happens is that commit b7cc53e9 switched
git-tag to use ref-filter's "%(refname:short)" output
formatting, which was adapted from for-each-ref. This more
general code does not know that we care only about tags, and
uses shorten_unambiguous_ref to get the short-name. We need
to tell it that we care only about "refs/tags/", and it
should shorten with respect to that value.

In theory, the ref-filter code could figure this out by us
passing FILTER_REFS_TAGS. But there are two complications
there:

  1. The handling of refname:short is deep in formatting
     code that does not even have our ref_filter struct, let
     alone the arguments to the filter_ref struct.

  2. In git v2.7.0, we expose the formatting language to the
     user. If we follow this path, it will mean that
     "%(refname:short)" behaves differently for "tag" versus
     "for-each-ref" (including "for-each-ref refs/tags/"),
     which can lead to confusion.

Instead, let's add a new modifier to the formatting
language, "strip", to remove a specific set of prefix
components. This fixes "git tag", and lets users invoke the
same behavior from their own custom formats (for "tag" or
"for-each-ref") while leaving ":short" with its same
consistent meaning in all places.

We introduce a test in t7004 for "git tag", which fails
without this patch. We also add a similar test in t3203 for
"git branch", which does not actually fail. But since it is
likely that "branch" will eventually use the same formatting
code, the test helps defend against future regressions.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-for-each-ref.txt |  6 +++++-
 Documentation/git-tag.txt          |  2 +-
 builtin/tag.c                      |  4 ++--
 ref-filter.c                       | 26 ++++++++++++++++++++++++++
 t/t3203-branch-output.sh           |  8 ++++++++
 t/t6300-for-each-ref.sh            | 12 ++++++++++++
 t/t7004-tag.sh                     |  8 ++++++++
 7 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 06208c4..2e3e96f 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -92,7 +92,11 @@ refname::
 	The name of the ref (the part after $GIT_DIR/).
 	For a non-ambiguous short name of the ref append `:short`.
 	The option core.warnAmbiguousRefs is used to select the strict
-	abbreviation mode.
+	abbreviation mode. If `strip=<N>` is appended, strips `<N>`
+	slash-separated path components from the front of the refname
+	(e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
+	`<N>` must be a positive integer.  If a displayed ref has fewer
+	components than `<N>`, the command aborts with an error.
 
 objecttype::
 	The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 7220e5e..abab481 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -163,7 +163,7 @@ This option is only applicable when listing tags without annotation lines.
 	A string that interpolates `%(fieldname)` from the object
 	pointed at by a ref being shown.  The format is the same as
 	that of linkgit:git-for-each-ref[1].  When unspecified,
-	defaults to `%(refname:short)`.
+	defaults to `%(refname:strip=2)`.
 
 --[no-]merged [<commit>]::
 	Only list tags whose tips are reachable, or not reachable
diff --git a/builtin/tag.c b/builtin/tag.c
index 8db8c87..1705c94 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -44,11 +44,11 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
 	if (!format) {
 		if (filter->lines) {
 			to_free = xstrfmt("%s %%(contents:lines=%d)",
-					  "%(align:15)%(refname:short)%(end)",
+					  "%(align:15)%(refname:strip=2)%(end)",
 					  filter->lines);
 			format = to_free;
 		} else
-			format = "%(refname:short)";
+			format = "%(refname:strip=2)";
 	}
 
 	verify_ref_format(format);
diff --git a/ref-filter.c b/ref-filter.c
index 7bef7f8..f097176 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -763,6 +763,29 @@ static inline char *copy_advance(char *dst, const char *src)
 	return dst;
 }
 
+static const char *strip_ref_components(const char *refname, const char *nr_arg)
+{
+	char *end;
+	long nr = strtol(nr_arg, &end, 10);
+	long remaining = nr;
+	const char *start = refname;
+
+	if (nr < 1 || *end != '\0')
+		die(":strip= requires a positive integer argument");
+
+	while (remaining) {
+		switch (*start++) {
+		case '\0':
+			die("ref '%s' does not have %ld components to :strip",
+			    refname, nr);
+		case '/':
+			remaining--;
+			break;
+		}
+	}
+	return start;
+}
+
 /*
  * Parse the object referred by ref, and grab needed value.
  */
@@ -909,11 +932,14 @@ static void populate_value(struct ref_array_item *ref)
 		formatp = strchr(name, ':');
 		if (formatp) {
 			int num_ours, num_theirs;
+			const char *arg;
 
 			formatp++;
 			if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
+			else if (skip_prefix(formatp, "strip=", &arg))
+				refname = strip_ref_components(refname, arg);
 			else if (!strcmp(formatp, "track") &&
 				 (starts_with(name, "upstream") ||
 				  starts_with(name, "push"))) {
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index d3913f9..4261403 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -176,4 +176,12 @@ test_expect_success 'git branch --points-at option' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ambiguous branch/tag not marked' '
+	git tag ambiguous &&
+	git branch ambiguous &&
+	echo "  ambiguous" >expect &&
+	git branch --list ambiguous >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 859b237..19a2823 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -50,6 +50,8 @@ test_atom() {
 
 test_atom head refname refs/heads/master
 test_atom head refname:short master
+test_atom head refname:strip=1 heads/master
+test_atom head refname:strip=2 master
 test_atom head upstream refs/remotes/origin/master
 test_atom head upstream:short origin/master
 test_atom head push refs/remotes/myfork/master
@@ -132,6 +134,16 @@ test_expect_success 'Check invalid atoms names are errors' '
 	test_must_fail git for-each-ref --format="%(INVALID)" refs/heads
 '
 
+test_expect_success 'arguments to :strip must be positive integers' '
+	test_must_fail git for-each-ref --format="%(refname:strip=0)" &&
+	test_must_fail git for-each-ref --format="%(refname:strip=-1)" &&
+	test_must_fail git for-each-ref --format="%(refname:strip=foo)"
+'
+
+test_expect_success 'stripping refnames too far gives an error' '
+	test_must_fail git for-each-ref --format="%(refname:strip=3)"
+'
+
 test_expect_success 'Check format specifiers are ignored in naming date atoms' '
 	git for-each-ref --format="%(authordate)" refs/heads &&
 	git for-each-ref --format="%(authordate:default) %(authordate)" refs/heads &&
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 2797f22..cf3469b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1558,4 +1558,12 @@ test_expect_success '--no-merged show unmerged tags' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ambiguous branch/tags not marked' '
+	git tag ambiguous &&
+	git branch ambiguous &&
+	echo ambiguous >expect &&
+	git tag -l ambiguous >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.7.0.427.g4c6e021

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> What does this do:
>
>   git for-each-ref --format='%(refname:remove-standard-prefix)'
>
> ?
>
> Is there no standard prefix, because we are not in branch/tag? Does it
> remove well-known prefixes like "refs/heads/", "refs/tags/", etc? Does
> it remove the first two components (e.g., what happens to
> "refs/foo/bar")? If so, what happens to "refs/stash"?

I think our mails crossed.  I envisioned that the documentation
would go something like this:

remove-standard-prefix::
	When the refname begins with one of the well-known prefixes,
	the prefix is stripped from it (and when it does not start
	with any of the well-known prefixes, the refname is left
	as-is).  "refs/heads/", "refs/remotes/" and "refs/tags/" are
	the well-known prefixes that are removed (as this modifier
	is primarily meant to allow users emulate the listing modes
	of "git tag" and "git branch") but this set may change over
	time (we may teach it to remove "refs/changes/" in later
	versions of Git, for example).

> Yeah, I am definitely on board with trying to do the most minimal thing
> for the regression fix and worry about more advanced concerns later (if
> at all). It seems to me that "error out" is a pretty minimal behavior,
> though, and one that doesn't lock us into any behaviors (i.e., it is
> generally OK to take something that did not work at all before, and give
> it new useful behavior; it is not OK to change something that did
> something useful before).

The thing that worried me the most is that "strip=<n>" is a very
intuitive and nice notation that end users would want to use it
beyond emulating "git tag" literally, and one behaviour we happen to
pick, be it "error out" or "leave it intact", would have a high
chance of being not the most useful one.  If we define it to error
out, somebody somewhere will abuse it to "ensure that all branch
names are at least two levels deep" or something we do not
anticipate now and start relying on the "erroring out" behaviour,
and then complain when we later "allow it to do something more
useful" that it no longer errors out.

Having said all that, I think I can live with "strip=<n>" that
happens to pick a single behaviour that we pick with the best
knowledge we have right now.  If we fear the compatiblity wart so
seriously, we can always add a separate "strip-nofail=<n>" variant
that gives a different behaviour from "strip=<n>" that errors out.

Another useful variant might be "strip <n> levels if we can,
otherwise pretend that the ref did not even pass the filtering
criteria and do not show anything about the ref on the output", by
the way.

> So what about this:
>
>   1. Implement ":strip=<n>" and use it from git-tag.
>
>   2. Have it error out on a ref with fewer than <n> components. This
>      should be impossible to trigger via "git-tag" with the default
>      format.
>
>   3. Explicitly document that the behavior for values of <n> that are
>      not positive integers is undefined and subject to change (or
>      alternatively, we can simply error out).
>
>
> That _is_ user-visible, but it seems like "strip" is a fairly flexible
> mechanism by itself (enough that I do not mind living with it forever),
> and we haven't made any promises about the ambiguous parts.

OK.

Re: [PATCH] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

On Sun, Jan 24, 2016 at 02:19:52PM -0800, Junio C Hamano wrote:

> Also there is "what happens if the expected prefix is not there?"
> question.  Perhaps strip=2 can be defined to "strip 2 levels of
> hierarchy prefix no matter what that is", and strip refs/tags/foo,
> refs/heads/foo and refs/remotes/origin/foo to foo, foo, origin/foo,
> respectively?  The filtering natively done by the listing mode of
> "branch" and "tags" would ensure the prefixes are always what we
> implicitly expect, so the case we need to worry about how we should
> signal errors becomes "what if there are not enough levels".  That
> may be simpler to handle.

The "not enough levels" question is interesting. Here are the options I
can think of:

  1. Signal an error and die. Safest, but potentially annoying.

  2. Strip everything and print a blank. Logically consistent, but the
     output is not particularly useful, and may introduce parsing
     confusion.

  3. Strip nothing (i.e., "%(refname:strip=4)" on "refs/heads/master"
     remains "refs/heads/master"). Easy to explain, and provides
     useful-ish output. The output is technically ambiguous, though (was
     it "refs/heads/foo/refs/heads/master", or just
     "refs/heads/master"?).

  4. Strip all but the final entry. Kind-of also useful, but like (3),
     ambiguous.

I implemented (3) semi-arbitrarily (mostly because it was only slightly
less easy than (2), and (2) seems kind of crazy).

There is also a question of what "strip=-1" or "strip=foobar" should do.
They are both equivalent to strip=0 in my implementation, but we could
also report an error.

I ended up doing a preparatory patch for t6300; I wanted to add new
tests there, and the existing content was so messy I couldn't bear it.

  [1/2]: t6300: use test_atom for some un-modern tests
  [2/2]: tag: do not show ambiguous tag names as "tags/foo"

-Peff

[PATCH 1/2] t6300: use test_atom for some un-modern tests

[Jeff King]

Because this script has to test so many formatters, we have
the nice "test_atom" helper, but we don't use it
consistently. Let's do so. This is shorter, gets rid of some
tests that have their "expected" setup outside of a
test_expect_success block, and lets us organize the changes
better (e.g., putting "refname:short" near "refname").

We also expand the "%(push)" tests a little to match the
"%(upstream)" ones.

Signed-off-by: Jeff King <peff@peff.net>
---
I suspect one could do even more cleanup in this script, but this looked
like all of the low-hanging fruit (some of the others are
order-dependent, for example).

One of the blocks that goes away must be setting a new
record for "pointless use of cat". See if you can spot it. :)

 t/t6300-for-each-ref.sh | 62 ++++++++-----------------------------------------
 1 file changed, 10 insertions(+), 52 deletions(-)

diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 03873b0..859b237 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -49,11 +49,15 @@ test_atom() {
 }
 
 test_atom head refname refs/heads/master
+test_atom head refname:short master
 test_atom head upstream refs/remotes/origin/master
+test_atom head upstream:short origin/master
 test_atom head push refs/remotes/myfork/master
+test_atom head push:short myfork/master
 test_atom head objecttype commit
 test_atom head objectsize 171
 test_atom head objectname $(git rev-parse refs/heads/master)
+test_atom head objectname:short $(git rev-parse --short refs/heads/master)
 test_atom head tree $(git rev-parse refs/heads/master^{tree})
 test_atom head parent ''
 test_atom head numparent 0
@@ -86,11 +90,13 @@ test_atom head contents 'Initial
 test_atom head HEAD '*'
 
 test_atom tag refname refs/tags/testtag
+test_atom tag refname:short testtag
 test_atom tag upstream ''
 test_atom tag push ''
 test_atom tag objecttype tag
 test_atom tag objectsize 154
 test_atom tag objectname $(git rev-parse refs/tags/testtag)
+test_atom tag objectname:short $(git rev-parse --short refs/tags/testtag)
 test_atom tag tree ''
 test_atom tag parent ''
 test_atom tag numparent ''
@@ -338,47 +344,14 @@ for i in "--perl --shell" "-s --python" "--python --tcl" "--tcl --perl"; do
 	"
 done
 
-cat >expected <<\EOF
-master
-testtag
-EOF
-
-test_expect_success 'Check short refname format' '
-	(git for-each-ref --format="%(refname:short)" refs/heads &&
-	git for-each-ref --format="%(refname:short)" refs/tags) >actual &&
-	test_cmp expected actual
-'
-
-cat >expected <<EOF
-origin/master
-EOF
-
-test_expect_success 'Check short upstream format' '
-	git for-each-ref --format="%(upstream:short)" refs/heads >actual &&
-	test_cmp expected actual
-'
-
 test_expect_success 'setup for upstream:track[short]' '
 	test_commit two
 '
 
-cat >expected <<EOF
-[ahead 1]
-EOF
-
-test_expect_success 'Check upstream:track format' '
-	git for-each-ref --format="%(upstream:track)" refs/heads >actual &&
-	test_cmp expected actual
-'
-
-cat >expected <<EOF
->
-EOF
-
-test_expect_success 'Check upstream:trackshort format' '
-	git for-each-ref --format="%(upstream:trackshort)" refs/heads >actual &&
-	test_cmp expected actual
-'
+test_atom head upstream:track '[ahead 1]'
+test_atom head upstream:trackshort '>'
+test_atom head push:track '[ahead 1]'
+test_atom head push:trackshort '>'
 
 test_expect_success 'Check that :track[short] cannot be used with other atoms' '
 	test_must_fail git for-each-ref --format="%(refname:track)" 2>/dev/null &&
@@ -398,21 +371,6 @@ test_expect_success 'Check that :track[short] works when upstream is invalid' '
 	test_cmp expected actual
 '
 
-test_expect_success '%(push) supports tracking specifiers, too' '
-	echo "[ahead 1]" >expected &&
-	git for-each-ref --format="%(push:track)" refs/heads >actual &&
-	test_cmp expected actual
-'
-
-cat >expected <<EOF
-$(git rev-parse --short HEAD)
-EOF
-
-test_expect_success 'Check short objectname format' '
-	git for-each-ref --format="%(objectname:short)" refs/heads >actual &&
-	test_cmp expected actual
-'
-
 test_expect_success 'Check for invalid refname format' '
 	test_must_fail git for-each-ref --format="%(refname:INVALID)"
 '
-- 
2.7.0.427.g4c6e021

[PATCH 2/2] tag: do not show ambiguous tag names as "tags/foo"

[Jeff King]

Since b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),
git-tag has started showing tags with ambiguous names (i.e.,
when both "heads/foo" and "tags/foo" exists) as "tags/foo"
instead of just "foo". This is both:

  - pointless; the output of "git tag" includes only
    refs/tags, so we know that "foo" means the one in
    "refs/tags".

and

  - ambiguous; in the original output, we know that the line
    "foo" means that "refs/tags/foo" exists. In the new
    output, it is unclear whether we mean "refs/tags/foo" or
    "refs/tags/tags/foo".

The reason this happens is that commit b7cc53e9 switched
git-tag to use ref-filter's "%(refname:short)" output
formatting, which was adapted from for-each-ref. This more
general code does not know that we care only about tags, and
uses shorten_unambiguous_ref to get the short-name. We need
to tell it that we care only about "refs/tags/", and it
should shorten with respect to that value.

In theory, the ref-filter code could figure this out by us
passing FILTER_REFS_TAGS. But there are two complications
there:

  1. The handling of refname:short is deep in formatting
     code that does not even have our ref_filter struct, let
     alone the arguments to the filter_ref struct.

  2. In git v2.7.0, we expose the formatting language to the
     user. If we follow this path, it will mean that
     "%(refname:short)" behaves differently for "tag" versus
     "for-each-ref" (including "for-each-ref refs/tags/"),
     which can lead to confusion.

Instead, let's add a new modifier to the formatting
language, "strip", to remove a specific set of prefix
components. This fixes "git tag", and lets users invoke the
same behavior from their own custom formats (for "tag" or
"for-each-ref") while leaving ":short" with its same
consistent meaning in all places.

We introduce a test in t7004 for "git tag", which fails
without this patch. We also add a similar test in t3203 for
"git branch", which does not actually fail. But since it is
likely that "branch" will eventually use the same formatting
code, the test helps defend against future regressions.

Signed-off-by: Jeff King <peff@peff.net>
---
 Documentation/git-for-each-ref.txt |  6 +++++-
 Documentation/git-tag.txt          |  2 +-
 builtin/tag.c                      |  4 ++--
 ref-filter.c                       | 13 ++++++++++++-
 t/t3203-branch-output.sh           |  8 ++++++++
 t/t6300-for-each-ref.sh            |  4 ++++
 t/t7004-tag.sh                     |  8 ++++++++
 7 files changed, 40 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
index 06208c4..f15c817 100644
--- a/Documentation/git-for-each-ref.txt
+++ b/Documentation/git-for-each-ref.txt
@@ -92,7 +92,11 @@ refname::
 	The name of the ref (the part after $GIT_DIR/).
 	For a non-ambiguous short name of the ref append `:short`.
 	The option core.warnAmbiguousRefs is used to select the strict
-	abbreviation mode.
+	abbreviation mode. If `strip=<N>` is appended, strips `<N>`
+	slash-separated path components from the front of the refname
+	(e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
+	If the ref has fewer components than `<N>`, the whole,
+	unstripped `%(refname)` is printed.
 
 objecttype::
 	The type of the object (`blob`, `tree`, `commit`, `tag`).
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 7220e5e..abab481 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -163,7 +163,7 @@ This option is only applicable when listing tags without annotation lines.
 	A string that interpolates `%(fieldname)` from the object
 	pointed at by a ref being shown.  The format is the same as
 	that of linkgit:git-for-each-ref[1].  When unspecified,
-	defaults to `%(refname:short)`.
+	defaults to `%(refname:strip=2)`.
 
 --[no-]merged [<commit>]::
 	Only list tags whose tips are reachable, or not reachable
diff --git a/builtin/tag.c b/builtin/tag.c
index 8db8c87..1705c94 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -44,11 +44,11 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
 	if (!format) {
 		if (filter->lines) {
 			to_free = xstrfmt("%s %%(contents:lines=%d)",
-					  "%(align:15)%(refname:short)%(end)",
+					  "%(align:15)%(refname:strip=2)%(end)",
 					  filter->lines);
 			format = to_free;
 		} else
-			format = "%(refname:short)";
+			format = "%(refname:strip=2)";
 	}
 
 	verify_ref_format(format);
diff --git a/ref-filter.c b/ref-filter.c
index 7bef7f8..9f54adc 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -909,12 +909,23 @@ static void populate_value(struct ref_array_item *ref)
 		formatp = strchr(name, ':');
 		if (formatp) {
 			int num_ours, num_theirs;
+			const char *arg;
 
 			formatp++;
 			if (!strcmp(formatp, "short"))
 				refname = shorten_unambiguous_ref(refname,
 						      warn_ambiguous_refs);
-			else if (!strcmp(formatp, "track") &&
+			else if (skip_prefix(formatp, "strip=", &arg)) {
+				int strip = atoi(arg);
+				const char *start = refname;
+				while (strip && *start) {
+					if (*start == '/')
+						strip--;
+					start++;
+				}
+				if (!strip)
+					refname = start;
+			} else if (!strcmp(formatp, "track") &&
 				 (starts_with(name, "upstream") ||
 				  starts_with(name, "push"))) {
 
diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index d3913f9..4261403 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -176,4 +176,12 @@ test_expect_success 'git branch --points-at option' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ambiguous branch/tag not marked' '
+	git tag ambiguous &&
+	git branch ambiguous &&
+	echo "  ambiguous" >expect &&
+	git branch --list ambiguous >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 859b237..1f3abeb 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -50,6 +50,10 @@ test_atom() {
 
 test_atom head refname refs/heads/master
 test_atom head refname:short master
+test_atom head refname:strip=0 refs/heads/master
+test_atom head refname:strip=1 heads/master
+test_atom head refname:strip=2 master
+test_atom head refname:strip=3 refs/heads/master
 test_atom head upstream refs/remotes/origin/master
 test_atom head upstream:short origin/master
 test_atom head push refs/remotes/myfork/master
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 2797f22..cf3469b 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -1558,4 +1558,12 @@ test_expect_success '--no-merged show unmerged tags' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ambiguous branch/tags not marked' '
+	git tag ambiguous &&
+	git branch ambiguous &&
+	echo ambiguous >expect &&
+	git tag -l ambiguous >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.7.0.427.g4c6e021

Re: [PATCH 2/2] tag: do not show ambiguous tag names as "tags/foo"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Since b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),
> git-tag has started showing tags with ambiguous names (i.e.,
> when both "heads/foo" and "tags/foo" exists) as "tags/foo"
> instead of just "foo". This is both:
>
>   - pointless; the output of "git tag" includes only
>     refs/tags, so we know that "foo" means the one in
>     "refs/tags".
>
> and
>
>   - ambiguous; in the original output, we know that the line
>     "foo" means that "refs/tags/foo" exists. In the new
>     output, it is unclear whether we mean "refs/tags/foo" or
>     "refs/tags/tags/foo".
>
> The reason this happens is that commit b7cc53e9 switched
> git-tag to use ref-filter's "%(refname:short)" output
> formatting, which was adapted from for-each-ref.
> ...

Karthik, getting a fix in for "git tag" regression is more important
than the topics parked in 'pu', so I'll queue this patch in the
early part of 'pu'.

I personally feel that "refname:strip=<n>" would be a good mechanism
for end users to specify a custom format, and it is unclear to me
what should happen when there are not enough elements to be
stripped, so I do not think we want to cast the "we will show the
whole thing" decision in stone prematurely only because we want to
push out the regression fix soon.  So I may ask Jeff to rework this
one (or I may end up trying to do so myself) not to squat on the
nice strip=<n> notation.  refname:strip-standard-prefix that removes
the known prefix ("refs/heads", "refs/remotes" and "refs/tags") if
present and does not touch the refname otherwise would leave us more
time to decide what strip=<n> should do in the error case.

Unfortunately, this means kn/ref-filter-atom-parsing topic from you
that were parked on 'pu' must be ejected for now, as any change in
this area overlaps with it, and the atom parsing code would need to
be updated to learn about the new attribute of the 'refname' atom
(be it 'remove-prefix=<glob>', 'strip=<n>', or something else) that
we would decide to use for the regression fix anyway.

Thanks.

>  Documentation/git-for-each-ref.txt |  6 +++++-
>  Documentation/git-tag.txt          |  2 +-
>  builtin/tag.c                      |  4 ++--
>  ref-filter.c                       | 13 ++++++++++++-
>  t/t3203-branch-output.sh           |  8 ++++++++
>  t/t6300-for-each-ref.sh            |  4 ++++
>  t/t7004-tag.sh                     |  8 ++++++++
>  7 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/Documentation/git-for-each-ref.txt b/Documentation/git-for-each-ref.txt
> index 06208c4..f15c817 100644
> --- a/Documentation/git-for-each-ref.txt
> +++ b/Documentation/git-for-each-ref.txt
> @@ -92,7 +92,11 @@ refname::
>  	The name of the ref (the part after $GIT_DIR/).
>  	For a non-ambiguous short name of the ref append `:short`.
>  	The option core.warnAmbiguousRefs is used to select the strict
> -	abbreviation mode.
> +	abbreviation mode. If `strip=<N>` is appended, strips `<N>`
> +	slash-separated path components from the front of the refname
> +	(e.g., `%(refname:strip=2)` turns `refs/tags/foo` into `foo`.
> +	If the ref has fewer components than `<N>`, the whole,
> +	unstripped `%(refname)` is printed.
>  
>  objecttype::
>  	The type of the object (`blob`, `tree`, `commit`, `tag`).
> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 7220e5e..abab481 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -163,7 +163,7 @@ This option is only applicable when listing tags without annotation lines.
>  	A string that interpolates `%(fieldname)` from the object
>  	pointed at by a ref being shown.  The format is the same as
>  	that of linkgit:git-for-each-ref[1].  When unspecified,
> -	defaults to `%(refname:short)`.
> +	defaults to `%(refname:strip=2)`.
>  
>  --[no-]merged [<commit>]::
>  	Only list tags whose tips are reachable, or not reachable
> diff --git a/builtin/tag.c b/builtin/tag.c
> index 8db8c87..1705c94 100644
> --- a/builtin/tag.c
> +++ b/builtin/tag.c
> @@ -44,11 +44,11 @@ static int list_tags(struct ref_filter *filter, struct ref_sorting *sorting, con
>  	if (!format) {
>  		if (filter->lines) {
>  			to_free = xstrfmt("%s %%(contents:lines=%d)",
> -					  "%(align:15)%(refname:short)%(end)",
> +					  "%(align:15)%(refname:strip=2)%(end)",
>  					  filter->lines);
>  			format = to_free;
>  		} else
> -			format = "%(refname:short)";
> +			format = "%(refname:strip=2)";
>  	}
>  
>  	verify_ref_format(format);
> diff --git a/ref-filter.c b/ref-filter.c
> index 7bef7f8..9f54adc 100644
> --- a/ref-filter.c
> +++ b/ref-filter.c
> @@ -909,12 +909,23 @@ static void populate_value(struct ref_array_item *ref)
>  		formatp = strchr(name, ':');
>  		if (formatp) {
>  			int num_ours, num_theirs;
> +			const char *arg;
>  
>  			formatp++;
>  			if (!strcmp(formatp, "short"))
>  				refname = shorten_unambiguous_ref(refname,
>  						      warn_ambiguous_refs);
> -			else if (!strcmp(formatp, "track") &&
> +			else if (skip_prefix(formatp, "strip=", &arg)) {
> +				int strip = atoi(arg);
> +				const char *start = refname;
> +				while (strip && *start) {
> +					if (*start == '/')
> +						strip--;
> +					start++;
> +				}
> +				if (!strip)
> +					refname = start;
> +			} else if (!strcmp(formatp, "track") &&
>  				 (starts_with(name, "upstream") ||
>  				  starts_with(name, "push"))) {
>  
> diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
> index d3913f9..4261403 100755
> --- a/t/t3203-branch-output.sh
> +++ b/t/t3203-branch-output.sh
> @@ -176,4 +176,12 @@ test_expect_success 'git branch --points-at option' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'ambiguous branch/tag not marked' '
> +	git tag ambiguous &&
> +	git branch ambiguous &&
> +	echo "  ambiguous" >expect &&
> +	git branch --list ambiguous >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done
> diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
> index 859b237..1f3abeb 100755
> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -50,6 +50,10 @@ test_atom() {
>  
>  test_atom head refname refs/heads/master
>  test_atom head refname:short master
> +test_atom head refname:strip=0 refs/heads/master
> +test_atom head refname:strip=1 heads/master
> +test_atom head refname:strip=2 master
> +test_atom head refname:strip=3 refs/heads/master
>  test_atom head upstream refs/remotes/origin/master
>  test_atom head upstream:short origin/master
>  test_atom head push refs/remotes/myfork/master
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index 2797f22..cf3469b 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1558,4 +1558,12 @@ test_expect_success '--no-merged show unmerged tags' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'ambiguous branch/tags not marked' '
> +	git tag ambiguous &&
> +	git branch ambiguous &&
> +	echo ambiguous >expect &&
> +	git tag -l ambiguous >actual &&
> +	test_cmp expect actual
> +'
> +
>  test_done

Re: [PATCH 2/2] tag: do not show ambiguous tag names as "tags/foo"

[Karthik Nayak]

On Tue, Jan 26, 2016 at 5:34 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
>
>> Since b7cc53e9 (tag.c: use 'ref-filter' APIs, 2015-07-11),
>> git-tag has started showing tags with ambiguous names (i.e.,
>> when both "heads/foo" and "tags/foo" exists) as "tags/foo"
>> instead of just "foo". This is both:
>>
>>   - pointless; the output of "git tag" includes only
>>     refs/tags, so we know that "foo" means the one in
>>     "refs/tags".
>>
>> and
>>
>>   - ambiguous; in the original output, we know that the line
>>     "foo" means that "refs/tags/foo" exists. In the new
>>     output, it is unclear whether we mean "refs/tags/foo" or
>>     "refs/tags/tags/foo".
>>
>> The reason this happens is that commit b7cc53e9 switched
>> git-tag to use ref-filter's "%(refname:short)" output
>> formatting, which was adapted from for-each-ref.
>> ...
>
> Karthik, getting a fix in for "git tag" regression is more important
> than the topics parked in 'pu', so I'll queue this patch in the
> early part of 'pu'.
>
> I personally feel that "refname:strip=<n>" would be a good mechanism
> for end users to specify a custom format, and it is unclear to me
> what should happen when there are not enough elements to be
> stripped, so I do not think we want to cast the "we will show the
> whole thing" decision in stone prematurely only because we want to
> push out the regression fix soon.  So I may ask Jeff to rework this
> one (or I may end up trying to do so myself) not to squat on the
> nice strip=<n> notation.  refname:strip-standard-prefix that removes
> the known prefix ("refs/heads", "refs/remotes" and "refs/tags") if
> present and does not touch the refname otherwise would leave us more
> time to decide what strip=<n> should do in the error case.
>
> Unfortunately, this means kn/ref-filter-atom-parsing topic from you
> that were parked on 'pu' must be ejected for now, as any change in
> this area overlaps with it, and the atom parsing code would need to
> be updated to learn about the new attribute of the 'refname' atom
> (be it 'remove-prefix=<glob>', 'strip=<n>', or something else) that
> we would decide to use for the regression fix anyway.

That should be fine, there are still changes to be done there so I'll rebase
on this and send that series.

-- 
Regards,
Karthik Nayak