[PATCH 0/5] Fix describe --contains --all

[PATCH 0/5] Fix describe --contains --all

[Ramkumar Ramachandra]

Hi,

I discovered --contains --all in describe, and found out that it
doesn't work as advertised for a deep historical reason.  [3/5] is
super-important and addresses this issue; everything else is
decoration.

Thanks.

Ramkumar Ramachandra (5):
  prompt: clean up describe logic
  prompt: do not double-discriminate detached HEAD
  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                   | 13 ++++++++++---
 contrib/completion/git-prompt.sh     | 20 +++++++++++---------
 t/t4202-log.sh                       |  8 ++++----
 t/t6007-rev-list-cherry-pick-file.sh | 32 ++++++++++++++++----------------
 t/t9903-bash-prompt.sh               |  2 +-
 6 files changed, 49 insertions(+), 38 deletions(-)

-- 
1.8.3.1.456.gb7f4cb6.dirty

[PATCH 1/5] prompt: clean up describe logic

[Ramkumar Ramachandra]

The describe logic is convoluted and unclean:

1. Reading .git/HEAD by hand and using the first 7 characters, of the
   SHA-1 does not guarantee an unambiguous output; we can use rev-parse
   --short in its place to get a unique SHA-1.

2. Use the --always option of describe to automatically output the short
   SHA-1 when everything else fails.

The patch introduces one small change: since we are not checking the
return value of describe (with --always, it always returns something
valid), we cannot discriminate a raw SHA-1 from everything else and
suffix it with a "...".

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-prompt.sh | 16 +++++++---------
 t/t9903-bash-prompt.sh           |  2 +-
 2 files changed, 8 insertions(+), 10 deletions(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 86a4f3f..9ed6ff1 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -380,20 +380,18 @@ __git_ps1 ()
 			test -n "$b" ||
 			b="$(git symbolic-ref HEAD 2>/dev/null)" || {
 				detached=yes
-				b="$(
 				case "${GIT_PS1_DESCRIBE_STYLE-}" in
 				(contains)
-					git describe --contains HEAD ;;
+					b=$(git describe --always --contains HEAD) ;;
 				(branch)
-					git describe --contains --all HEAD ;;
+					b=$(git describe --always --contains --all HEAD) ;;
 				(describe)
-					git describe HEAD ;;
+					b=$(git describe --always HEAD) ;;
 				(* | default)
-					git describe --tags --exact-match HEAD ;;
-				esac 2>/dev/null)" ||
-
-				b="$(cut -c1-7 "$g/HEAD" 2>/dev/null)..." ||
-				b="unknown"
+					b=$(git describe --tags --exact-match HEAD 2>/dev/null)
+					test -z $b && b="$(git rev-parse --short HEAD)"
+					;;
+				esac
 				b="($b)"
 			}
 		fi
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 15521cc..b0ad477 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -169,7 +169,7 @@ test_expect_success 'prompt - branch name' '
 '
 
 test_expect_success 'prompt - detached head' '
-	printf " ((%s...))" $(git log -1 --format="%h" b1^) > expected &&
+	printf " ((%s))" $(git log -1 --format="%h" b1^) > expected &&
 	git checkout b1^ &&
 	test_when_finished "git checkout master" &&
 	__git_ps1 > "$actual" &&
-- 
1.8.3.1.456.gb7f4cb6.dirty

[PATCH 2/5] prompt: do not double-discriminate detached HEAD

[Ramkumar Ramachandra]

When GIT_PS1_SHOWCOLORHINTS is turned on, there is no need to put a
detached HEAD within parenthesis: the color can be used to discriminate
the detached HEAD.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 contrib/completion/git-prompt.sh | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9ed6ff1..a127d3b 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -392,7 +392,11 @@ __git_ps1 ()
 					test -z $b && b="$(git rev-parse --short HEAD)"
 					;;
 				esac
-				b="($b)"
+
+				# if there is no color, use
+				# parenthesis to indicate that the
+				# HEAD is detached
+				test -n "${GIT_PS1_SHOWCOLORHINTS-}" || b="($b)"
 			}
 		fi
 
-- 
1.8.3.1.456.gb7f4cb6.dirty

[PATCH 3/5] name-rev: fix assumption about --name-only usage

[Ramkumar Ramachandra]

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, 24 insertions(+), 25 deletions(-)

diff --git a/Documentation/git-name-rev.txt b/Documentation/git-name-rev.txt
index ad1d146..5cd0d0d 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -36,9 +36,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 6238247..524d790 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -112,8 +112,7 @@ static int name_ref(const char *path, const unsigned char *sha1, int flags, void
 
 		if (!prefixcmp(path, "refs/heads/"))
 			path = path + 11;
-		else if (data->tags_only
-		    && data->name_only
+		else if (data->name_only
 		    && !prefixcmp(path, "refs/tags/"))
 			path = path + 10;
 		else if (!prefixcmp(path, "refs/"))
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.1.456.gb7f4cb6.dirty

[PATCH 4/5] name-rev: strip trailing ^0 in when --name-only

[Ramkumar Ramachandra]

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 | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/builtin/name-rev.c b/builtin/name-rev.c
index 524d790..4b17209 100644
--- a/builtin/name-rev.c
+++ b/builtin/name-rev.c
@@ -160,7 +160,15 @@ 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.1.456.gb7f4cb6.dirty

[PATCH 5/5] name-rev doc: rewrite --stdin paragraph

[Ramkumar Ramachandra]

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 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 5cd0d0d..67f0487 100644
--- a/Documentation/git-name-rev.txt
+++ b/Documentation/git-name-rev.txt
@@ -31,8 +31,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.1.456.gb7f4cb6.dirty