[PATCH 0/3] Iron 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(-)

-- 
1.8.3.2.737.gcbc076a.dirty

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 <redacted>
---
 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(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index 6b0f1ba..7cde4b3 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt

@@ -37,9 +37,9 @@ OPTIONS
 
 --name-only::
 	Instead of printing both the SHA-1 and the name, print only
-	the name.  If given with --tags the usual tag prefix of
-	"tags/" is also omitted from the name, matching the output
-	of `git-describe` more closely.
+	the name.  The usual tag prefix of "tags/" is also omitted
+	from the name, matching the output of `git-describe` more
+	closely.
 
 --no-undefined::
 	Die with error code != 0 when a reference is undefined,

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 87d4854..37207a9 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c

@@ -138,6 +138,9 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
 			path = shorten_unambiguous_ref(path, 0);
 		else if (!prefixcmp(path, "refs/heads/"))
 			path = path + 11;
+		else if (data->name_only
+		    && !prefixcmp(path, "refs/tags/"))
+			path = path + 10;
 		else if (!prefixcmp(path, "refs/"))
 			path = path + 5;
 

diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index cb03d28..9bec360 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh

@@ -302,7 +302,7 @@ cat > expect <<\EOF
 | |
 | |     side-2
 | |
-| * commit tags/side-1
+| * commit side-1
 | | Author: A U Thor <author@example.com>
 | |
 | |     side-1

@@ -327,17 +327,17 @@ cat > expect <<\EOF
 |
 |       fourth
 |
-* commit tags/side-1~1
+* commit side-1~1
 | Author: A U Thor <author@example.com>
 |
 |     third
 |
-* commit tags/side-1~2
+* commit side-1~2
 | Author: A U Thor <author@example.com>
 |
 |     second
 |
-* commit tags/side-1~3
+* commit side-1~3
   Author: A U Thor <author@example.com>
 
       initial

diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh
index 28d4f6b..5a8175e 100755
--- a/t/t6007-rev-list-cherry-pick-file.sh
+++ b/t/t6007-rev-list-cherry-pick-file.sh

@@ -49,8 +49,8 @@ test_expect_success setup '
 '
 
 cat >expect <<EOF
-<tags/B
->tags/C
+<B
+>C
 EOF
 
 test_expect_success '--left-right' '

@@ -70,7 +70,7 @@ test_expect_success '--cherry-pick foo comes up empty' '
 '
 
 cat >expect <<EOF
->tags/C
+>C
 EOF
 
 test_expect_success '--cherry-pick bar does not come up empty' '

@@ -88,8 +88,8 @@ test_expect_success 'bar does not come up empty' '
 '
 
 cat >expect <<EOF
-<tags/F
->tags/E
+<F
+>E
 EOF
 
 test_expect_success '--cherry-pick bar does not come up empty (II)' '

@@ -100,10 +100,10 @@ test_expect_success '--cherry-pick bar does not come up empty (II)' '
 '
 
 cat >expect <<EOF
-+tags/F
-=tags/D
-+tags/E
-=tags/C
++F
+=D
++E
+=C
 EOF
 
 test_expect_success '--cherry-mark' '

@@ -114,10 +114,10 @@ test_expect_success '--cherry-mark' '
 '
 
 cat >expect <<EOF
-<tags/F
-=tags/D
->tags/E
-=tags/C
+<F
+=D
+>E
+=C
 EOF
 
 test_expect_success '--cherry-mark --left-right' '

@@ -128,7 +128,7 @@ test_expect_success '--cherry-mark --left-right' '
 '
 
 cat >expect <<EOF
-tags/E
+E
 EOF
 
 test_expect_success '--cherry-pick --right-only' '

@@ -146,8 +146,8 @@ test_expect_success '--cherry-pick --left-only' '
 '
 
 cat >expect <<EOF
-+tags/E
-=tags/C
++E
+=C
 EOF
 
 test_expect_success '--cherry' '

-- 
1.8.3.2.737.gcbc076a.dirty

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

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index 7cde4b3..94bded8 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt

@@ -32,8 +32,10 @@ OPTIONS
 	List all commits reachable from all refs
 
 --stdin::
-	Read from stdin, append "(<rev_name>)" to all sha1's of nameable
-	commits, and pass to stdout
+	Transform stdin by substituting all the 40-character SHA-1
+	hexes (say $hex) with "$hex ($rev_name)".  When used with
+	--name-only, substitute with "$rev_name", omitting $hex
+	altogether.  Intended for the scripter's use.
 
 --name-only::
 	Instead of printing both the SHA-1 and the name, print only

-- 
1.8.3.2.737.gcbc076a.dirty

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 <redacted>
---
 builtin/name-rev.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 37207a9..8ba5d72 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c

@@ -186,7 +186,14 @@ static void show_name(const struct object *obj,
 	if (!name_only)
 		printf("%s ", caller_name ? caller_name : sha1_to_hex(sha1));
 	name = get_rev_name(obj);
-	if (name)
+
+	if (name && name_only) {
+		/* strip possible trailing ^0 from name */
+		int len = strlen(name);
+		if (len > 2 && !strcmp(name + len - 2, "^0"))
+			len -= 2;
+		printf("%.*s\n", len, name);
+	} else if (name)
 		printf("%s\n", name);
 	else if (allow_undefined)
 		printf("undefined\n");

-- 
1.8.3.2.737.gcbc076a.dirty

Ramkumar Ramachandra [off-list ref] writes:

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

...  As a result, while

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

The above two look consistent, yes.

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/".

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:

quoted

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/".

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.

Yeah, you asked for it using --all.

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?

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