patch, 3 partsIron output of describe --contains --all

Hi,

I actually sent these patches to the list last month, but nobody seemed to be interested. This is an unedited resend.

I looked into adding tests, but decided that it was a lost cause: the output is too loosely defined for any scripts to rely on it strongly. [1/3] already shows a race between branches and tags. For another example, consider two tags are pointing to same commit (D and R in t6120-describe.sh). Run the following on the two tags and see what happens for yourself:

  $ git describe --contains
  $ git describe --all
  $ git describe --tags

Now think about various combinations of these options. I'm not saying that it's a Bad Thing (TM), but that nobody has bothered tightening the output.

My main motivation for doing this series is my prompt: I don't want to see

  artagnon|(tags/v1.8.3^0)~/src/git$

when

  artagnon|(v1.8.3)~/src/git$

is so much more pleasant and consistent. Obviously, hacking around this problem in the prompt script is the Wrong thing to do.

Thanks.

Ramkumar Ramachandra (3):
  name-rev: fix assumption about --name-only usage
  name-rev: strip trailing ^0 in when --name-only
  name-rev doc: rewrite --stdin paragraph

 Documentation/git-name-rev.txt       | 12 +++++++-----
 builtin/name-rev.c                   | 12 +++++++++++-
 t/t4202-log.sh                       |  8 ++++----
 t/t6007-rev-list-cherry-pick-file.sh | 32 ++++++++++++++++----------------
 4 files changed, 38 insertions(+), 26 deletions(-)

236157 (Teach git-describe how to run name-rev, 2007-05-21) introduced `git name-rev --name-only`, with the intent of using it to implement `git describe --contains`. According to the message, users wanted to use describe to figure out which tags contains a specific commit. name-rev already did this, but didn't print out in the same format as describe:

  $ git describe v1.8.3~1
  v1.8.3-rc3-8-g5e49f30

  $ git name-rev v1.8.3~1
  v1.8.3~1 tags/v1.8.3~1

There are two problems with using the output of name-rev in describe: first, it prints out the given argument before describing it. Second, it prefixes "tags/" to the tag description. To eliminate these two problems, 236157 proposed that the --name-rev option would strip these things when used with --tags, to match the describe output more closely:

  $ git name-rev --name-only --tags v1.8.3~1
  v1.8.3~1

236157 did not anticipate a problem with always combining --name-rev with --tags, because it was primarily intended to be used from describe, where it hard-coded these two arguments in the execv() of name-rev.

Later, 3f7701 (make 'git describe --all --contains' work, 2007-12-19) noticed that describe didn't work with --contains and --all. This is because --contains implied a call to --name-rev (in with --tags was hard-coded), but --all implied that any ref should be used to describe the given argument (not just tags). 3f7701 took the band-aid approach, and made --all disable --tags when calling name-rev. As a result, while

  $ git describe --contains v1.8.3~1
  v1.8.3~1

would get name-rev to print output in the same format as describe,

  $ git describe --contains --all v1.8.3~1
  tags/v1.8.3~1

would not strip the leading "tags/".

The bug exists in git to this day. Fix it by removing the assumption that name-rev --name-only is only intended to be used with --tags. Also update some tests.

Users and scripts have learnt to live with 3f7701, and it will continue to be a small quirk. Even after this patch, notice

  $ git checkout -b foom v1.8.3
  $ git describe --contains @~1
  v1.8.3~1
  $ git describe --contains --all @~1
  foom~1

In other words, --contains implies --tags in name-rev, which gives precedence to tags; --all cancels that effect thereby giving precedence to branches in the case of a tie.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-name-rev.txt       |  6 +++---
 builtin/name-rev.c                   |  3 +++
 t/t4202-log.sh                       |  8 ++++----
 t/t6007-rev-list-cherry-pick-file.sh | 32 ++++++++++++++++----------------
 4 files changed, 26 insertions(+), 23 deletions(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

The above two look consistent, yes.

If you _know_ v1.8.3 does not appear outside "tags/", this does look inconsistent, but I do not think the code checks it. Ahd if the code does not, I am not sure not stripping "tags/" is necessarily a bad thing, because "--all" allows names to come outside "tags/" hierarchy.

Also how should this interact with v1.8.3-1-g98c5c4a that changed the rule somewhat so that the common prefix is stripped when we know the result is not ambiguous?

Junio C Hamano wrote:

Yeah, you asked for it using --all.

Completely independent of everything else. The condition is "if name-only && prefix == refs/tags", strip that prefix.

236157 (Teach git-describe how to run name-rev, 2007-05-21) introduced `git name-rev --name-only`, with the intent of using it to implement `git describe --contains`. According to the message, one of the primary objectives of --name-only was to make the output of name-rev match that of describe.

  $ git describe --contains --all master
  master

  $ git describe --contains --all master~1
  master~1

  $ git describe --contains --all v1.8.3~1
  v1.8.3~1

  $ git describe --contains --all v1.8.3
  v1.8.3^0

The last invocation unnecessarily prints a trailing "^0" (--stdin does not suffer from this defect). Fix this.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 builtin/name-rev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

WRT "describe --contains", I do agree that both of these

    $ git describe $(git rev-parse v1.8.3^0)
    $ git describe --contains $(git rev-parse v1.8.3^0)
        
should just say "v1.8.3" without ~0/^0/~0~0~0 etc. and the last
example you showed will be improved by dropping ^0 at the end.

However.

I was a bit bothered by the description talking _only_ about describe, but the actual change is to modify what name-rev gives its direct users as well. And that made me realize that the patch itself has an undesirable side effect.

"describe" is _only_ about commit history graph, so in its context v1.8.3 means the same thing as v1.8.3^0 (we never want to get a tag; we always want a commit). But I do not think "name-rev" is limited to commits, in the sense that you would see this:

    $ git rev-parse v1.8.3 v1.8.3^0 | git name-rev --stdin
    8af06057d0c31a24e8737ae846ac2e116e8bafb9
    edca4152560522a431a51fc0a06147fc680b5b18 (tags/v1.8.3^0)

The second object is _not_ v1.8.3 but is v1.8.3^0 in the context of name-rev, whose purpose is to give you a string you can feed "rev-parse" and get the object name back. "rev-parse v1.8.3" will not give you the commit object name, so you need to keep "^0".

Junio C Hamano <gitster@pobox.com> writes:

Well, the code in "name-rev" other than --stdin mode is already broken (and the documentation half-describes this breakage) in that it describes the peeled commit and rejects anything other than commit objects. The reason I say "half-describes" is that it only says that the command takes "commit-ish" and leaves it unclear if it comes up with a name for the tag itself that happens to be commit-ish, or it does so for the commit that is referred by the tag.

I'll send out a WIP to fix that, and also help the topic to strip unnecessary "^0" suffix when name-rev is run as an implementation detail of "describe" shortly.

Junio C Hamano wrote:

Quite frankly, I thought the unstripped ^0 in one codepath was an unintended quirk. What exactly do you want name-rev to give you?

  $ git tag foo @^
  $ git name-rev foo
  foo tags/foo

So you can distinguish between annotated tags, unannotated tags, and head-refs. Can you get it to tell you anything reliably though?

  $ git tag bar @
  $ git tag -a baz @
  $ git name-rev @
  $ git name-rev bar
  $ git name-rev baz

ref, annotated, or unannotated tag? I do not think name-rev is fundamentally different from describe: it is also only dependent on the commit history graph. Whether I specify a revision using @, HEAD, baz, or bar, I should get the same answer (it's just a recursive peeler). I'm not sure what you gain by knowing the object type of the output. If you wanted to feed something into rev-parse and get out a commit, you'd send in $REV^0 without bothering about what it is, no?

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Documentation/git-name-rev.txt | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Ramkumar Ramachandra <artagnon@gmail.com> writes:

Looks obviously correct and more helpful description.

Thanks.