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

From: Jeff King <peff@peff.net>
Date: Jan 24, 2016, 23:05 UTC
Message-ID: <20160124230531.GB29115@sigill.intra.peff.net>
In-Reply-To: <xmqqvb6iboxj.fsf@gitster.mtv.corp.google.com>

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

Show 9 quoted lines

> 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