[PATCH] describe: dont abort too early when searching tags

[PATCH] describe: dont abort too early when searching tags

[Benno Evers]

When searching the commit graph for tag candidates, `git-describe`
will stop as soon as there is only one active branch left and
it already found an annotated tag as a candidate.

This works well as long as all branches eventually connect back
to a common root, but if the tags are found across branches
with no common ancestor

                  B
                  o----.
                        \
          o-----o---o----x
          A

it can happen that the search on one branch terminates prematurely
because a tag was found on another, independent branch. This scenario
isn't quite as obscure as it sounds, since cloning with a limited
depth often introduces many independent "dead ends" into the commit
graph.

The help text of `git-describe` states pretty clearly that when
describing a commit D, the number appended to the emitted tag X should
correspond to the number of commits found by `git log X..D`.

Thus, this commit modifies the stopping condition to only abort
the search when only one branch is left to search *and* all current
best candidates are descendants from that branch.

Signed-off-by: Benno Evers <benno@bmevers.de>
---
We originally found this issue in one of our internal CI jobs,
which relied on `git-describe` to assign filenames to the generated
artifacts.
---
 builtin/describe.c  | 22 +++++++++++++++----
 t/t6120-describe.sh | 53 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/builtin/describe.c b/builtin/describe.c
index b6df81d8d0..420f4c6401 100644
--- a/builtin/describe.c
+++ b/builtin/describe.c
@@ -376,11 +376,25 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)
 			if (!(c->object.flags & t->flag_within))
 				t->depth++;
 		}
+		/* Stop if last remaining path already covered by best candidate(s) */
 		if (annotated_cnt && !list) {
-			if (debug)
-				fprintf(stderr, _("finished search at %s\n"),
-					oid_to_hex(&c->object.oid));
-			break;
+			int best_depth = INT_MAX;
+			unsigned best_within = 0;
+			for (cur_match = 0; cur_match < match_cnt; cur_match++) {
+				struct possible_tag *t = &all_matches[cur_match];
+				if (t->depth < best_depth) {
+					best_depth = t->depth;
+					best_within = t->flag_within;
+				} else if (t->depth == best_depth) {
+					best_within |= t->flag_within;
+				}
+			}
+			if ((c->object.flags & best_within) == best_within) {
+				if (debug)
+					fprintf(stderr, _("finished search at %s\n"),
+						oid_to_hex(&c->object.oid));
+				break;
+			}
 		}
 		while (parents) {
 			struct commit *p = parents->item;
diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 09c50f3f04..d8cc08258e 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -479,4 +479,55 @@ test_expect_success 'name-rev covers all conditions while looking at parents' '
 	)
 '
 
-test_done
+#               B
+#               o
+#                \
+#  o-----o---o----x
+#        A
+#
+test_expect_success 'describe commits with disjoint bases' '
+	git init disjoint1 &&
+	(
+		cd disjoint1 &&
+
+		echo o >> file && git add file && git commit -m o &&
+		echo A >> file && git add file && git commit -m A &&
+		git tag A -a -m A &&
+		echo o >> file && git add file && git commit -m o &&
+
+		git checkout --orphan branch && rm file &&
+		echo B > file2 && git add file2 && git commit -m B &&
+		git tag B -a -m B &&
+		git merge --no-ff --allow-unrelated-histories master -m x &&
+
+		check_describe "A-3-*" HEAD
+	)
+'
+
+#           B
+#   o---o---o------------.
+#                         \
+#                  o---o---x
+#                  A
+#
+test_expect_success 'describe commits with disjoint bases 2' '
+	git init disjoint2 &&
+	(
+		cd disjoint2 &&
+
+		echo A >> file && git add file && GIT_COMMITTER_DATE="2020-01-01 18:00" git commit -m A &&
+		git tag A -a -m A &&
+		echo o >> file && git add file && GIT_COMMITTER_DATE="2020-01-01 18:01" git commit -m o &&
+
+		git checkout --orphan branch &&
+		echo o >> file2 && git add file2 && GIT_COMMITTER_DATE="2020-01-01 15:00" git commit -m o &&
+		echo o >> file2 && git add file2 && GIT_COMMITTER_DATE="2020-01-01 15:01" git commit -m o &&
+		echo B >> file2 && git add file2 && GIT_COMMITTER_DATE="2020-01-01 15:02" git commit -m B &&
+		git tag B -a -m B &&
+		git merge --no-ff --allow-unrelated-histories master -m x &&
+
+		check_describe "B-3-*" HEAD
+	)
+'
+
+test_done
\ No newline at end of file
-- 
2.20.1

Re: [PATCH] describe: dont abort too early when searching tags

[Junio C Hamano]

Benno Evers <benno@bmevers.de> writes:

> When searching the commit graph for tag candidates, `git-describe`
> will stop as soon as there is only one active branch left and
> it already found an annotated tag as a candidate.
>
> This works well as long as all branches eventually connect back
> to a common root, but if the tags are found across branches
> with no common ancestor
>
>                   B
>                   o----.
>                         \
>           o-----o---o----x
>           A
>
> it can happen that the search on one branch terminates prematurely
> because a tag was found on another, independent branch. This scenario
> isn't quite as obscure as it sounds, since cloning with a limited
> depth often introduces many independent "dead ends" into the commit
> graph.
>
> The help text of `git-describe` states pretty clearly that when
> describing a commit D, the number appended to the emitted tag X should
> correspond to the number of commits found by `git log X..D`.

To be fair, that description was written for the case in a normal
history with only a single root.

How much, if any, does this change hurt the performance by forcing
the code to dig further if there aren't multiple roots?  If there is
such an unnecessary overhead that degrades performance for the more
common case, can we improve it further to avoid it?

Thanks.

Re: [PATCH] describe: dont abort too early when searching tags

[benno]

From: Benno Evers <benno@bmevers.de>

> How much, if any, does this change hurt the performance by forcing
> the code to dig further if there aren't multiple roots?  If there is
> such an unnecessary overhead that degrades performance for the more
> common case, can we improve it further to avoid it?

If there aren't multiple roots, then this should be visiting exactly
the same number of commits as before. This is because in this case, if
we're down to a single branch, the current commit must be an ancestor
of *all* tag candidates so the stopping condition is always true.

It's actually a bit challenging to find good test repositories for
this in the wild, as the big ones that use a merging workflow (like git
itself, the kernel, etc.) usually have so many branches active at any
point in time that the search will only stop when it hits the max number
of candidates. So I might be missing some edge cases. However, for the
"normal" repositories that I tested the number of traversed commits was
always the same before and after the change.
 

Re: [PATCH] describe: dont abort too early when searching tags

[Junio C Hamano]

benno@bmevers.de writes:

> From: Benno Evers <benno@bmevers.de>
>
>> How much, if any, does this change hurt the performance by forcing
>> the code to dig further if there aren't multiple roots?  If there is
>> such an unnecessary overhead that degrades performance for the more
>> common case, can we improve it further to avoid it?
>
> If there aren't multiple roots, then this should be visiting exactly
> the same number of commits as before. This is because in this case, if
> we're down to a single branch, the current commit must be an ancestor
> of *all* tag candidates so the stopping condition is always true.

Sounds good.  Can some of that analysis go in the proposed log
message text, so that other people do not have to ask the same
question later?

Thanks.

[PATCH 00/10] describe tests: refactor & fix recent broken tests

[Ævar Arnfjörð Bjarmason]

While looking at the "git describe" tests due to the on-list
%(describe) discussion I discovered that the feature added in
30b1c7ad9d (describe: don't abort too early when searching tags,
2020-02-26) has never been tested for.

This is because it defined a custom test function that called
test_expect_success, which then got called inside another top-level
test_expect_success. Thus even if it failed we'd pass the test.

This series fixes that issue, and makes some general improvements to
the "describe" tests. I then make test_expect_{success,failure} return
1 to catch these sorts of issues in the future, which required fixes
to a couple of svn tests that ran with "set -e".

I was on the fence about whether to send this after the recent rc0,
but figured that since it's test-only Junio might want to pick it up,
and possibly for the next rc in case we'd like to do some pre-release
testing for this never-before-tested feature added in 2.26.0 (although
the actual implementation looks fine to me).

Ævar Arnfjörð Bjarmason (10):
  describe tests: improve test for --work-tree & --dirty
  describe tests: refactor away from glob matching
  describe tests: always assert empty stderr from "describe"
  test-lib functions: add an --annotated-tag option to "test_commit"
  describe tests: convert setup to use test_commit
  describe tests: fix nested "test_expect_success" call
  describe tests: support -C in "check_describe"
  svn tests: remove legacy re-setup from init-clone test
  svn tests: refactor away a "set -e" in test body
  test-lib: return 1 from test_expect_{success,failure}

 t/t1403-show-ref.sh           |   6 +-
 t/t6120-describe.sh           | 193 +++++++++++++++-------------------
 t/t9117-git-svn-init-clone.sh |   6 --
 t/t9148-git-svn-propset.sh    |  27 ++---
 t/test-lib-functions.sh       |  20 +++-
 5 files changed, 122 insertions(+), 130 deletions(-)

-- 
2.31.0.rc0.116.g45ec00aa00

[PATCH 01/10] describe tests: improve test for --work-tree & --dirty

[Ævar Arnfjörð Bjarmason]

Improve tests added in 9f67d2e8279 (Teach "git describe" --dirty
option, 2009-10-21) and 2ed5c8e174d (describe: setup working tree for
--dirty, 2019-02-03) so that they make sense in combination with each
other.

The "check_describe" being removed here was the earlier test, we then
later added these --work-tree tests which really just wanted to check
if we got the exact same output from "describe", but the test wasn't
structured to test for that.

Let's change it to do that, which both improves test coverage and
makes it more obvious what's going on here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index e89b6747be..7bc2aaa46e 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -183,24 +183,24 @@ test_expect_success 'set-up dirty work tree' '
 	echo >>file
 '
 
-check_describe "A-*[0-9a-f]-dirty" --dirty
-
 test_expect_success 'describe --dirty with --work-tree (dirty)' '
+	git describe --dirty >expected &&
 	(
 		cd "$TEST_DIRECTORY" &&
 		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
 	) &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+-dirty$" out
+	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+-dirty$" out &&
+	test_cmp expected out
 '
 
-check_describe "A-*[0-9a-f].mod" --dirty=.mod
-
 test_expect_success 'describe --dirty=.mod with --work-tree (dirty)' '
+	git describe --dirty=.mod >expected &&
 	(
 		cd "$TEST_DIRECTORY" &&
 		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty=.mod >"$TRASH_DIRECTORY/out"
 	) &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+.mod$" out
+	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+.mod$" out &&
+	test_cmp expected out
 '
 
 test_expect_success 'describe --dirty HEAD' '
-- 
2.31.0.rc0.116.g45ec00aa00

[PATCH 02/10] describe tests: refactor away from glob matching

[Ævar Arnfjörð Bjarmason]

Change the glob matching via a "case" statement to a "test_cmp" after
we've stripped out the hash-specific g<hash-abbrev>
suffix. 5312ab11fbf (Add describe test., 2007-01-13).

This means that we can use test_cmp to compare the output. I could
omit the "-8" change of e.g. "A-*" to "A-8-gHASH", but I think it
makes sense to test that here explicitly. It means you need to add new
tests to the bottom of the file, but that's not a burden in this case.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 78 ++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 40 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 7bc2aaa46e..e4fd5d567f 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -21,12 +21,10 @@ check_describe () {
 	shift
 	describe_opts="$@"
 	test_expect_success "describe $describe_opts" '
-	R=$(git describe $describe_opts 2>err.actual) &&
-	case "$R" in
-	$expect)	echo happy ;;
-	*)	echo "Oops - $R is not $expect" &&
-		false ;;
-	esac
+		git describe $describe_opts 2>err.actual >raw &&
+		sed -e "s/-g[0-9a-f]*\$/-gHASH/" <raw >actual &&
+		echo $expect >expect &&
+		test_cmp expect actual
 	'
 }
 
@@ -91,29 +89,29 @@ test_expect_success setup '
 
 '
 
-check_describe A-* HEAD
-check_describe A-* HEAD^
-check_describe R-* HEAD^^
-check_describe A-* HEAD^^2
+check_describe A-8-gHASH HEAD
+check_describe A-7-gHASH HEAD^
+check_describe R-2-gHASH HEAD^^
+check_describe A-3-gHASH HEAD^^2
 check_describe B HEAD^^2^
-check_describe R-* HEAD^^^
+check_describe R-1-gHASH HEAD^^^
 
-check_describe c-* --tags HEAD
-check_describe c-* --tags HEAD^
-check_describe e-* --tags HEAD^^
-check_describe c-* --tags HEAD^^2
+check_describe c-7-gHASH --tags HEAD
+check_describe c-6-gHASH --tags HEAD^
+check_describe e-1-gHASH --tags HEAD^^
+check_describe c-2-gHASH --tags HEAD^^2
 check_describe B --tags HEAD^^2^
 check_describe e --tags HEAD^^^
 
 check_describe heads/main --all HEAD
-check_describe tags/c-* --all HEAD^
+check_describe tags/c-6-gHASH --all HEAD^
 check_describe tags/e --all HEAD^^^
 
-check_describe B-0-* --long HEAD^^2^
-check_describe A-3-* --long HEAD^^2
+check_describe B-0-gHASH --long HEAD^^2^
+check_describe A-3-gHASH --long HEAD^^2
 
-check_describe c-7-* --tags
-check_describe e-3-* --first-parent --tags
+check_describe c-7-gHASH --tags
+check_describe e-3-gHASH --first-parent --tags
 
 test_expect_success 'describe --contains defaults to HEAD without commit-ish' '
 	echo "A^0" >expect &&
@@ -134,7 +132,7 @@ test_expect_success 'rename tag A to Q locally' '
 cat - >err.expect <<EOF
 warning: tag 'Q' is externally known as 'A'
 EOF
-check_describe A-* HEAD
+check_describe A-8-gHASH HEAD
 test_expect_success 'warning was displayed for Q' '
 	test_cmp err.expect err.actual
 '
@@ -161,22 +159,22 @@ test_expect_success 'rename tag Q back to A' '
 '
 
 test_expect_success 'pack tag refs' 'git pack-refs'
-check_describe A-* HEAD
+check_describe A-8-gHASH HEAD
 
 test_expect_success 'describe works from outside repo using --git-dir' '
 	git clone --bare "$TRASH_DIRECTORY" "$TRASH_DIRECTORY/bare" &&
 	git --git-dir "$TRASH_DIRECTORY/bare" describe >out &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+$" out
+	grep -E "^A-8-g[0-9a-f]+$" out
 '
 
-check_describe "A-*[0-9a-f]" --dirty
+check_describe "A-8-gHASH" --dirty
 
 test_expect_success 'describe --dirty with --work-tree' '
 	(
 		cd "$TEST_DIRECTORY" &&
 		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
 	) &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+$" out
+	grep -E "^A-8-g[0-9a-f]+$" out
 '
 
 test_expect_success 'set-up dirty work tree' '
@@ -189,7 +187,7 @@ test_expect_success 'describe --dirty with --work-tree (dirty)' '
 		cd "$TEST_DIRECTORY" &&
 		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
 	) &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+-dirty$" out &&
+	grep -E "^A-8-g[0-9a-f]+-dirty$" out &&
 	test_cmp expected out
 '
 
@@ -199,7 +197,7 @@ test_expect_success 'describe --dirty=.mod with --work-tree (dirty)' '
 		cd "$TEST_DIRECTORY" &&
 		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty=.mod >"$TRASH_DIRECTORY/out"
 	) &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+.mod$" out &&
+	grep -E "^A-8-g[0-9a-f]+.mod$" out &&
 	test_cmp expected out
 '
 
@@ -223,21 +221,21 @@ test_expect_success 'set-up matching pattern tests' '
 
 '
 
-check_describe "test-annotated-*" --match="test-*"
+check_describe "test-annotated-3-gHASH" --match="test-*"
 
-check_describe "test1-lightweight-*" --tags --match="test1-*"
+check_describe "test1-lightweight-2-gHASH" --tags --match="test1-*"
 
-check_describe "test2-lightweight-*" --tags --match="test2-*"
+check_describe "test2-lightweight-1-gHASH" --tags --match="test2-*"
 
-check_describe "test2-lightweight-*" --long --tags --match="test2-*" HEAD^
+check_describe "test2-lightweight-0-gHASH" --long --tags --match="test2-*" HEAD^
 
-check_describe "test2-lightweight-*" --long --tags --match="test1-*" --match="test2-*" HEAD^
+check_describe "test2-lightweight-0-gHASH" --long --tags --match="test1-*" --match="test2-*" HEAD^
 
-check_describe "test2-lightweight-*" --long --tags --match="test1-*" --no-match --match="test2-*" HEAD^
+check_describe "test2-lightweight-0-gHASH" --long --tags --match="test1-*" --no-match --match="test2-*" HEAD^
 
-check_describe "test1-lightweight-*" --long --tags --match="test1-*" --match="test3-*" HEAD
+check_describe "test1-lightweight-2-gHASH" --long --tags --match="test1-*" --match="test3-*" HEAD
 
-check_describe "test1-lightweight-*" --long --tags --match="test3-*" --match="test1-*" HEAD
+check_describe "test1-lightweight-2-gHASH" --long --tags --match="test3-*" --match="test1-*" HEAD
 
 test_expect_success 'set-up branches' '
 	git branch branch_A A &&
@@ -247,11 +245,11 @@ test_expect_success 'set-up branches' '
 	git update-ref refs/original/original_branch_A test-annotated~2
 '
 
-check_describe "heads/branch_A*" --all --match="branch_*" --exclude="branch_C" HEAD
+check_describe "heads/branch_A-11-gHASH" --all --match="branch_*" --exclude="branch_C" HEAD
 
-check_describe "remotes/origin/remote_branch_A*" --all --match="origin/remote_branch_*" --exclude="origin/remote_branch_C" HEAD
+check_describe "remotes/origin/remote_branch_A-11-gHASH" --all --match="origin/remote_branch_*" --exclude="origin/remote_branch_C" HEAD
 
-check_describe "original/original_branch_A*" --all test-annotated~1
+check_describe "original/original_branch_A-6-gHASH" --all test-annotated~1
 
 test_expect_success '--match does not work for other types' '
 	test_must_fail git describe --all --match="*original_branch_*" test-annotated~1
@@ -521,7 +519,7 @@ test_expect_success 'describe commits with disjoint bases' '
 		git tag B -a -m B &&
 		git merge --no-ff --allow-unrelated-histories main -m x &&
 
-		check_describe "A-3-*" HEAD
+		check_describe "A-3-gHASH" HEAD
 	)
 '
 
@@ -547,7 +545,7 @@ test_expect_success 'describe commits with disjoint bases 2' '
 		git tag B -a -m B &&
 		git merge --no-ff --allow-unrelated-histories main -m x &&
 
-		check_describe "B-3-*" HEAD
+		check_describe "B-3-gHASH" HEAD
 	)
 '
 
-- 
2.31.0.rc0.116.g45ec00aa00

[PATCH 03/10] describe tests: always assert empty stderr from "describe"

[Ævar Arnfjörð Bjarmason]

Invert a test added in 3291fe4072e (Add git-describe test for "verify
annotated tag names on output", 2008-03-03) to make checking that we
don't have warnings the rule rather than the exception.

There was only one case where we expected and got a warning. Let's
test for that case explicitly, and assert no warnings or other stderr
output for all the rest.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 26 ++++++++++++--------------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index e4fd5d567f..ef70c695be 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -21,7 +21,8 @@ check_describe () {
 	shift
 	describe_opts="$@"
 	test_expect_success "describe $describe_opts" '
-		git describe $describe_opts 2>err.actual >raw &&
+		git describe $describe_opts 2>err >raw &&
+		test_must_be_empty err &&
 		sed -e "s/-g[0-9a-f]*\$/-gHASH/" <raw >actual &&
 		echo $expect >expect &&
 		test_cmp expect actual
@@ -122,20 +123,17 @@ test_expect_success 'describe --contains defaults to HEAD without commit-ish' '
 '
 
 check_describe tags/A --all A^0
-test_expect_success 'no warning was displayed for A' '
-	test_must_be_empty err.actual
-'
 
-test_expect_success 'rename tag A to Q locally' '
-	mv .git/refs/tags/A .git/refs/tags/Q
-'
-cat - >err.expect <<EOF
-warning: tag 'Q' is externally known as 'A'
-EOF
-check_describe A-8-gHASH HEAD
-test_expect_success 'warning was displayed for Q' '
-	test_cmp err.expect err.actual
-'
+test_expect_success 'renaming tag A to Q locally produces a warning' "
+	mv .git/refs/tags/A .git/refs/tags/Q &&
+	git describe HEAD 2>actual >out &&
+	cat >expected <<-\EOF &&
+	warning: tag 'Q' is externally known as 'A'
+	EOF
+	test_cmp expected actual &&
+	grep -E '^A-8-g[0-9a-f]+$' out
+"
+
 test_expect_success 'misnamed annotated tag forces long output' '
 	description=$(git describe --no-long Q^0) &&
 	expr "$description" : "A-0-g[0-9a-f]*$" &&
-- 
2.31.0.rc0.116.g45ec00aa00

[PATCH 04/10] test-lib functions: add an --annotated-tag option to "test_commit"

[Ævar Arnfjörð Bjarmason]

Add an --annotated-tag option to test_commit. The tag will share the
same message as the commit, and we'll call test_tick before creating
it (unless --notick) is provided.

There's quite a few tests that could be simplified with this
construct. I've picked one to convert in this change as a
demonstration.

The placement of --annotated-tag after "notick" in the case of the
documentation, and then after "no_tag" in the case of the code is
slightly inconsistent. It's to evade a merge conflict with two other
commits adding a --printf option, and another one adding documentation
for --no-tag.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t1403-show-ref.sh     |  6 ++----
 t/test-lib-functions.sh | 18 +++++++++++++++++-
 2 files changed, 19 insertions(+), 5 deletions(-)

diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 6ce62f878c..7c873033e9 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -7,11 +7,9 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 
 test_expect_success setup '
-	test_commit A &&
-	git tag -f -a -m "annotated A" A &&
+	test_commit --annotated-tag A &&
 	git checkout -b side &&
-	test_commit B &&
-	git tag -f -a -m "annotated B" B &&
+	test_commit --annotated-tag B &&
 	git checkout main &&
 	test_commit C &&
 	git branch B A^0
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 6348e8d733..c6cdabf53e 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -171,6 +171,10 @@ debug () {
 #	Run all git commands in directory <dir>
 #   --notick
 #	Do not call test_tick before making a commit
+#   --annotated-tag
+#	Create an annotated tag with "-a -m <message>". Calls
+#	test_tick between making the commit and tag unless --notick is
+#	given.
 #   --append
 #	Use "echo >>" instead of "echo >" when writing "<contents>" to
 #	"<file>"
@@ -191,6 +195,7 @@ test_commit () {
 	signoff= &&
 	indir= &&
 	no_tag= &&
+	annotated_tag= &&
 	while test $# != 0
 	do
 		case "$1" in
@@ -220,6 +225,9 @@ test_commit () {
 		--no-tag)
 			no_tag=yes
 			;;
+		--annotated-tag)
+			annotated_tag=yes
+			;;
 		*)
 			break
 			;;
@@ -244,7 +252,15 @@ test_commit () {
 	    $signoff -m "$1" &&
 	if test -z "$no_tag"
 	then
-		git ${indir:+ -C "$indir"} tag "${4:-$1}"
+		if test -n "$annotated_tag"
+		then
+			if test -z "$notick"
+			then
+				test_tick
+			fi &&
+			test_tick
+		fi &&
+		git ${indir:+ -C "$indir"} tag ${annotated_tag:+ -a -m "$1"} "${4:-$1}"
 	fi
 }
 
-- 
2.31.0.rc0.116.g45ec00aa00

[PATCH 05/10] describe tests: convert setup to use test_commit

[Ævar Arnfjörð Bjarmason]

Convert the setup of the describe tests to use test_commit when
possible. This makes use of the new --annotated-tag option to
test_commit.

Some of the setup here could simply be removed since the data being
created wasn't important to any of the subsequent tests, so I've done
so. E.g. assigning to the "one" variable was always useless, and just
checking that we can describe HEAD after the first commit wasn't
useful.

In the case of the "two" variable we could instead use the tag we just
created. See 5312ab11fbf (Add describe test., 2007-01-13) for the
initial version of this code. There's other cases here like redundant
"test_tick" invocations, or the simplification of not echoing "X" to a
file we're about to tag as "x", now we just use "x" in both cases.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 58 ++++++++++-----------------------------------
 1 file changed, 13 insertions(+), 45 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index ef70c695be..8dc76f8e9e 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -30,64 +30,32 @@ check_describe () {
 }
 
 test_expect_success setup '
+	test_commit initial file one &&
+	test_commit second file two &&
+	test_commit third file three &&
+	test_commit --annotated-tag A file A &&
+	test_commit c file c &&
 
-	test_tick &&
-	echo one >file && git add file && git commit -m initial &&
-	one=$(git rev-parse HEAD) &&
-
-	git describe --always HEAD &&
-
-	test_tick &&
-	echo two >file && git add file && git commit -m second &&
-	two=$(git rev-parse HEAD) &&
-
-	test_tick &&
-	echo three >file && git add file && git commit -m third &&
-
-	test_tick &&
-	echo A >file && git add file && git commit -m A &&
-	test_tick &&
-	git tag -a -m A A &&
-
-	test_tick &&
-	echo c >file && git add file && git commit -m c &&
-	test_tick &&
-	git tag c &&
-
-	git reset --hard $two &&
-	test_tick &&
-	echo B >side && git add side && git commit -m B &&
-	test_tick &&
-	git tag -a -m B B &&
+	git reset --hard second &&
+	test_commit --annotated-tag B side B &&
 
 	test_tick &&
 	git merge -m Merged c &&
 	merged=$(git rev-parse HEAD) &&
 
-	git reset --hard $two &&
-	test_tick &&
-	echo D >another && git add another && git commit -m D &&
-	test_tick &&
-	git tag -a -m D D &&
-	test_tick &&
-	git tag -a -m R R &&
-
-	test_tick &&
-	echo DD >another && git commit -a -m another &&
+	git reset --hard second &&
+	test_commit --no-tag D another D &&
 
 	test_tick &&
-	git tag e &&
+	git tag -a -m R R &&
 
-	test_tick &&
-	echo DDD >another && git commit -a -m "yet another" &&
+	test_commit e another DD &&
+	test_commit --no-tag "yet another" another DDD &&
 
 	test_tick &&
 	git merge -m Merged $merged &&
 
-	test_tick &&
-	echo X >file && echo X >side && git add file side &&
-	git commit -m x
-
+	test_commit --no-tag x file
 '
 
 check_describe A-8-gHASH HEAD
-- 
2.31.0.rc0.116.g45ec00aa00

[PATCH 06/10] describe tests: fix nested "test_expect_success" call

[Ævar Arnfjörð Bjarmason]

Fix a nested invocation of "test_expect_success", the
"check_describe()" function is a wrapper for calling
test_expect_success, and therefore needs to be called outside the body
of another "test_expect_success".

The two tests added in 30b1c7ad9d6 (describe: don't abort too early
when searching tags, 2020-02-26) were not testing for anything due to
this logic error. Without this fix reverting the C code changes in
that commit still has all tests passing, with this fix we're actually
testing the "describe" output. This is because "test_expect_success"
calls "test_finish_", whose last statement happens to be true.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 8dc76f8e9e..ae801c740b 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -470,7 +470,7 @@ test_expect_success 'name-rev covers all conditions while looking at parents' '
 #  o-----o---o----x
 #        A
 #
-test_expect_success 'describe commits with disjoint bases' '
+test_expect_success 'setup: describe commits with disjoint bases' '
 	git init disjoint1 &&
 	(
 		cd disjoint1 &&
@@ -483,19 +483,22 @@ test_expect_success 'describe commits with disjoint bases' '
 		git checkout --orphan branch && rm file &&
 		echo B > file2 && git add file2 && git commit -m B &&
 		git tag B -a -m B &&
-		git merge --no-ff --allow-unrelated-histories main -m x &&
-
-		check_describe "A-3-gHASH" HEAD
+		git merge --no-ff --allow-unrelated-histories main -m x
 	)
 '
 
+(
+	cd disjoint1 &&
+	check_describe "A-3-gHASH" HEAD
+)
+
 #           B
 #   o---o---o------------.
 #                         \
 #                  o---o---x
 #                  A
 #
-test_expect_success 'describe commits with disjoint bases 2' '
+test_expect_success 'setup: describe commits with disjoint bases 2' '
 	git init disjoint2 &&
 	(
 		cd disjoint2 &&
@@ -509,10 +512,13 @@ test_expect_success 'describe commits with disjoint bases 2' '
 		echo o >> file2 && git add file2 && GIT_COMMITTER_DATE="2020-01-01 15:01" git commit -m o &&
 		echo B >> file2 && git add file2 && GIT_COMMITTER_DATE="2020-01-01 15:02" git commit -m B &&
 		git tag B -a -m B &&
-		git merge --no-ff --allow-unrelated-histories main -m x &&
-
-		check_describe "B-3-gHASH" HEAD
+		git merge --no-ff --allow-unrelated-histories main -m x
 	)
 '
 
+(
+	cd disjoint2 &&
+	check_describe "B-3-gHASH" HEAD
+)
+
 test_done
-- 
2.31.0.rc0.116.g45ec00aa00

[PATCH 07/10] describe tests: support -C in "check_describe"

[Ævar Arnfjörð Bjarmason]

Change a subshell added in a preceding commit to instead use a new
"-C" option to "check_describe". The idiom for this is copied as-is
from the "test_commit" function in test-lib-functions.sh

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index ae801c740b..e48c4f604b 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -17,11 +17,26 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 
 check_describe () {
+	indir= &&
+	while test $# != 0
+	do
+		case "$1" in
+		-C)
+			indir="$2"
+			shift
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done &&
+	indir=${indir:+"$indir"/} &&
 	expect="$1"
 	shift
 	describe_opts="$@"
 	test_expect_success "describe $describe_opts" '
-		git describe $describe_opts 2>err >raw &&
+		git ${indir:+ -C "$indir"} describe $describe_opts 2>err >raw &&
 		test_must_be_empty err &&
 		sed -e "s/-g[0-9a-f]*\$/-gHASH/" <raw >actual &&
 		echo $expect >expect &&
@@ -487,10 +502,7 @@ test_expect_success 'setup: describe commits with disjoint bases' '
 	)
 '
 
-(
-	cd disjoint1 &&
-	check_describe "A-3-gHASH" HEAD
-)
+check_describe -C disjoint1 "A-3-gHASH" HEAD
 
 #           B
 #   o---o---o------------.
@@ -516,9 +528,6 @@ test_expect_success 'setup: describe commits with disjoint bases 2' '
 	)
 '
 
-(
-	cd disjoint2 &&
-	check_describe "B-3-gHASH" HEAD
-)
+check_describe -C disjoint2 "B-3-gHASH" HEAD
 
 test_done
-- 
2.31.0.rc0.116.g45ec00aa00

[PATCH 08/10] svn tests: remove legacy re-setup from init-clone test

[Ævar Arnfjörð Bjarmason]

Remove the immediate "rm -rf .git" from the start of this test. This
was added back in 41337e22f0 (git-svn: add tests for command-line
usage of init and clone commands, 2007-11-17) when there was a "trash"
directory shared by all the tests, but ever since abc5d372ec (Enable
parallel tests, 2008-08-08) we've had per-test trash directories.

So this setup can simply be removed. We could use
TEST_NO_CREATE_REPO=true, but I don't think it's worth the effort to
go out of our way to be different. It doesn't matter that we now have
a redundant .git at the top-level.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t9117-git-svn-init-clone.sh | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 044f65e916..62de819a44 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -7,12 +7,6 @@ test_description='git svn init/clone tests'
 
 . ./lib-git-svn.sh
 
-# setup, run inside tmp so we don't have any conflicts with $svnrepo
-set -e
-rm -r .git
-mkdir tmp
-cd tmp
-
 test_expect_success 'setup svnrepo' '
 	mkdir project project/trunk project/branches project/tags &&
 	echo foo > project/trunk/foo &&
-- 
2.31.0.rc0.116.g45ec00aa00

[PATCH 09/10] svn tests: refactor away a "set -e" in test body

[Ævar Arnfjörð Bjarmason]

Refactor a test added in 83c9433e67 (git-svn: support for git-svn
propset, 2014-12-07) to avoid using "set -e" in the test body. This
would have broken in combination with a subsequent change to make
"test_expect_success" return 1 to catch such cases.

While I'm at it rewrite the test to conform to a modern style in our
tests, using the "test_when_finished" function for the "rm -rf", and
avoiding repeated "mkdir" in favor of "mkdir -p".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t9148-git-svn-propset.sh | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/t/t9148-git-svn-propset.sh b/t/t9148-git-svn-propset.sh
index 102639090c..2b55e76be6 100755
--- a/t/t9148-git-svn-propset.sh
+++ b/t/t9148-git-svn-propset.sh
@@ -7,19 +7,22 @@ test_description='git svn propset tests'
 
 . ./lib-git-svn.sh
 
-foo_subdir2="subdir/subdir2/foo_subdir2"
+test_expect_success 'setup propset via import' '
+	test_when_finished "rm -rf import" &&
 
-set -e
-mkdir import &&
-(set -e ; cd import
-	mkdir subdir
-	mkdir subdir/subdir2
-	touch foo 		# for 'add props top level'
-	touch subdir/foo_subdir # for 'add props relative'
-	touch "$foo_subdir2"	# for 'add props subdir'
-	svn_cmd import -m 'import for git svn' . "$svnrepo" >/dev/null
-)
-rm -rf import
+	foo_subdir2="subdir/subdir2/foo_subdir2" &&
+	mkdir -p import/subdir/subdir2 &&
+	(
+		cd import &&
+		# for "add props top level"
+		touch foo &&
+		# for "add props relative"
+		touch subdir/foo_subdir &&
+		# for "add props subdir"
+		touch "$foo_subdir2" &&
+		svn_cmd import -m "import for git svn" . "$svnrepo"
+	)
+'
 
 test_expect_success 'initialize git svn' '
 	git svn init "$svnrepo"
-- 
2.31.0.rc0.116.g45ec00aa00

[PATCH 10/10] test-lib: return 1 from test_expect_{success,failure}

[Ævar Arnfjörð Bjarmason]

An earlier commit fixed an issue in "check_describe" with
"test_expect_success" being called within another
"test_expect_success", causing the test to succeed even if it should
fail.

Let's try to guard against this in the test library by returning 1
from these two functions. This change would have caught the issue I've
now fixed in the "check_describe" function.

I could equivalently add this "return 1" to the "test_finish_"
function itself, but I think doing it here is more readable.

Because of this change any tests which ran under "set -e" needed to be
refactored not to use "set -e". Luckily there were only two such
tests, earlier commits did that refactoring.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/test-lib-functions.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index c6cdabf53e..3dd68091bb 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -636,6 +636,7 @@ test_expect_failure () {
 		fi
 	fi
 	test_finish_
+	return 1
 }
 
 test_expect_success () {
@@ -656,6 +657,7 @@ test_expect_success () {
 		fi
 	fi
 	test_finish_
+	return 1
 }
 
 # test_external runs external test scripts that provide continuous
-- 
2.31.0.rc0.116.g45ec00aa00

Re: [PATCH 09/10] svn tests: refactor away a "set -e" in test body

[Eric Wong]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> Refactor a test added in 83c9433e67 (git-svn: support for git-svn
> propset, 2014-12-07) to avoid using "set -e" in the test body. This
> would have broken in combination with a subsequent change to make
> "test_expect_success" return 1 to catch such cases.
> 
> While I'm at it rewrite the test to conform to a modern style in our
> tests, using the "test_when_finished" function for the "rm -rf", and
> avoiding repeated "mkdir" in favor of "mkdir -p".

Thank you for working on these (and sorry for making a mess all
those years ago!).

I had it in my mind that "mkdir -p" wasn't portable, but systems
without it probably haven't been relevant since pre-git times.

> --- a/t/t9148-git-svn-propset.sh
> +++ b/t/t9148-git-svn-propset.sh
> @@ -7,19 +7,22 @@ test_description='git svn propset tests'
>  
>  . ./lib-git-svn.sh
>  
> -foo_subdir2="subdir/subdir2/foo_subdir2"
> +test_expect_success 'setup propset via import' '
> +	test_when_finished "rm -rf import" &&
>  
> -set -e
> -mkdir import &&
> -(set -e ; cd import
> -	mkdir subdir
> -	mkdir subdir/subdir2
> -	touch foo 		# for 'add props top level'
> -	touch subdir/foo_subdir # for 'add props relative'
> -	touch "$foo_subdir2"	# for 'add props subdir'
> -	svn_cmd import -m 'import for git svn' . "$svnrepo" >/dev/null
> -)
> -rm -rf import
> +	foo_subdir2="subdir/subdir2/foo_subdir2" &&
> +	mkdir -p import/subdir/subdir2 &&
> +	(
> +		cd import &&
> +		# for "add props top level"
> +		touch foo &&
> +		# for "add props relative"
> +		touch subdir/foo_subdir &&
> +		# for "add props subdir"
> +		touch "$foo_subdir2" &&
> +		svn_cmd import -m "import for git svn" . "$svnrepo"

I've noticed '>' could be used instead of touch, but that's
probably better as another patch :>

Re: [PATCH 02/10] describe tests: refactor away from glob matching

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the glob matching via a "case" statement to a "test_cmp" after
> we've stripped out the hash-specific g<hash-abbrev>
> suffix. 5312ab11fbf (Add describe test., 2007-01-13).
>
> This means that we can use test_cmp to compare the output. I could
> omit the "-8" change of e.g. "A-*" to "A-8-gHASH", but I think it
> makes sense to test that here explicitly. It means you need to add new
> tests to the bottom of the file, but that's not a burden in this case.

I think the point in these tests are that they consider "which tip
the tested commit is the closest" is the only piece of information
that matter, and allows the numbers ("number of commits on top of")
to be redefined in the future without having to change the tests,
but I tend to agree that such a change should be accompanied by and
documented with changes to these tests. 

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t6120-describe.sh | 78 ++++++++++++++++++++++-----------------------
>  1 file changed, 38 insertions(+), 40 deletions(-)
>
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index 7bc2aaa46e..e4fd5d567f 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -21,12 +21,10 @@ check_describe () {
>  	shift
>  	describe_opts="$@"

Just a style thing, when we are not invoking the "each positional
arg is double-quoted individually against being split at $IFS" magic,
we prefer to spell this as "$*".

>  	test_expect_success "describe $describe_opts" '
> +		git describe $describe_opts 2>err.actual >raw &&
> +		sed -e "s/-g[0-9a-f]*\$/-gHASH/" <raw >actual &&

The exact ones would be passed as-is (i.e. "test_cmp raw actual"
could pass), which is what we want anyway.

If we are planning to further extend this helper to make it more
capable, we might want to consider quoting $describe to be evaled
properly so that we can do an equivalent of

	check_describe A-8-gHASH --dirty='.d i r t y' HEAD

when we gain new option that is more intresting than --dirty=<mark>
that legitimately would benefit from being able to pass arguments
with $IFS whitespace in them.

But that is outside the scope of this step.  I haven't seen the
overall topic yet, so it may or may not be within the scope of this
series.  We'll see.

> +		echo $expect >expect &&

Let's double-quote to relieve readers from having to wonder if you
are expecting the callers to feed crazy things like "a  b" and this
echo to normalize it to "a b".

> +		test_cmp expect actual
>  	'
>  }

Re: [PATCH 03/10] describe tests: always assert empty stderr from "describe"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Invert a test added in 3291fe4072e (Add git-describe test for "verify
> annotated tag names on output", 2008-03-03) to make checking that we
> don't have warnings the rule rather than the exception.
>
> There was only one case where we expected and got a warning. Let's
> test for that case explicitly, and assert no warnings or other stderr
> output for all the rest.

When we are expecting an error from "describe", we would want to
make sure that we will see an expected explanation of the failure
(e.g. "you passed a non-commit") in the standard error stream, but I
am somewhat skeptical about the value of a change like this that
insists that there is nothing on the standard error stream when the
command succeeds.

It is unlikely to trigger auto GC during the operation of "git
describe" and seeing some output in the standard error stream, but
it is easy to imagine that we may add an "automatically cache
precomputed topology information" feature and trigger during a
history traversal operation like this one, with some note to the
standard error output.

> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  t/t6120-describe.sh | 26 ++++++++++++--------------
>  1 file changed, 12 insertions(+), 14 deletions(-)
>
> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
> index e4fd5d567f..ef70c695be 100755
> --- a/t/t6120-describe.sh
> +++ b/t/t6120-describe.sh
> @@ -21,7 +21,8 @@ check_describe () {
>  	shift
>  	describe_opts="$@"
>  	test_expect_success "describe $describe_opts" '
> -		git describe $describe_opts 2>err.actual >raw &&
> +		git describe $describe_opts 2>err >raw &&
> +		test_must_be_empty err &&
>  		sed -e "s/-g[0-9a-f]*\$/-gHASH/" <raw >actual &&
>  		echo $expect >expect &&
>  		test_cmp expect actual
> @@ -122,20 +123,17 @@ test_expect_success 'describe --contains defaults to HEAD without commit-ish' '
>  '
>  
>  check_describe tags/A --all A^0
> -test_expect_success 'no warning was displayed for A' '
> -	test_must_be_empty err.actual
> -'
>  
> -test_expect_success 'rename tag A to Q locally' '
> -	mv .git/refs/tags/A .git/refs/tags/Q
> -'
> -cat - >err.expect <<EOF
> -warning: tag 'Q' is externally known as 'A'
> -EOF
> -check_describe A-8-gHASH HEAD
> -test_expect_success 'warning was displayed for Q' '
> -	test_cmp err.expect err.actual
> -'
> +test_expect_success 'renaming tag A to Q locally produces a warning' "
> +	mv .git/refs/tags/A .git/refs/tags/Q &&
> +	git describe HEAD 2>actual >out &&
> +	cat >expected <<-\EOF &&
> +	warning: tag 'Q' is externally known as 'A'
> +	EOF
> +	test_cmp expected actual &&
> +	grep -E '^A-8-g[0-9a-f]+$' out
> +"
> +
>  test_expect_success 'misnamed annotated tag forces long output' '
>  	description=$(git describe --no-long Q^0) &&
>  	expr "$description" : "A-0-g[0-9a-f]*$" &&

Re: [PATCH 04/10] test-lib functions: add an --annotated-tag option to "test_commit"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> @@ -220,6 +225,9 @@ test_commit () {
>  		--no-tag)
>  			no_tag=yes
>  			;;
> +		--annotated-tag)
> +			annotated_tag=yes
> +			;;

A new option is welcome, but shouldn't we be straightening out the
variables that keep track of the options around tagging?  It's not
like it is possible to have 4 (two variables that can independently
set to 'yes') possibilities, so something along the lines of ...

         test_commit () {
        +	tag=light &&
                notick= &&
         ...
		while test $# != 0
		do
			case "$1" in
			...
			--no-tag)
	-			no_tag=yes
	+			tag=none
				;;
	+		--annotated)
	+			tag=annotated
	+			;;
		...
	-	if test -z "$no_tag"
	-	then
	+	case "$tag" in
	+	none)
	+		;;
	+	light)
			git ${indir:+ -C "$indir") tag "${4:-$1}"
	+		;;
	+	annotated)
	+		git ${indir:+ -C "$indir") tag -m "$1" "${4:-$1}"
	+		;;
	+	esac
		...

after this step (meaning, we may want to start from fixing the no_tag=yes
to tag=none before introducing this new feature).

Thanks.

> @@ -244,7 +252,15 @@ test_commit () {
>  	    $signoff -m "$1" &&
>  	if test -z "$no_tag"
>  	then
> -		git ${indir:+ -C "$indir"} tag "${4:-$1}"
> +		if test -n "$annotated_tag"
> +		then
> +			if test -z "$notick"
> +			then
> +				test_tick
> +			fi &&
> +			test_tick
> +		fi &&
> +		git ${indir:+ -C "$indir"} tag ${annotated_tag:+ -a -m "$1"} "${4:-$1}"
>  	fi
>  }

Re: [PATCH 05/10] describe tests: convert setup to use test_commit

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Convert the setup of the describe tests to use test_commit when
> possible. This makes use of the new --annotated-tag option to
> test_commit.

Much nicer ;-)

>  t/t6120-describe.sh | 58 ++++++++++-----------------------------------
>  1 file changed, 13 insertions(+), 45 deletions(-)

Re: [PATCH 10/10] test-lib: return 1 from test_expect_{success,failure}

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index c6cdabf53e..3dd68091bb 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -636,6 +636,7 @@ test_expect_failure () {
>  		fi
>  	fi
>  	test_finish_
> +	return 1
>  }
>  
>  test_expect_success () {
> @@ -656,6 +657,7 @@ test_expect_success () {
>  		fi
>  	fi
>  	test_finish_
> +	return 1
>  }
>  
>  # test_external runs external test scripts that provide continuous

Hmph.

This does not catch if the outer expect_success does not catch a
failure in the inner expect_success and signal a failure.

When I asked if this kind of breakage is an easy mistake to catch by
the test lint, I had something along this in mind:

	test_expect_success () {
		if test -n "$GIT_IN_TEST_EXPECT"
		then
			BUG caling "$GIT_IN_TEST_EXPECT" inside test_expect_success
		fi
		GIT_IN_TEST_EXPECT=test_expect_success

		... do the 'eval the given test body' thing ..

		GIT_IN_TEST_EXPECT=
	}

After all, the error is in the outer expect_success in that it
called another one, so it feels more natural that the called inner
expect_success to notice the situation and barf.

Thanks.

Re: [PATCH 04/10] test-lib functions: add an --annotated-tag option to "test_commit"

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 01 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> @@ -220,6 +225,9 @@ test_commit () {
>>  		--no-tag)
>>  			no_tag=yes
>>  			;;
>> +		--annotated-tag)
>> +			annotated_tag=yes
>> +			;;
>
> A new option is welcome, but shouldn't we be straightening out the
> variables that keep track of the options around tagging?  It's not
> like it is possible to have 4 (two variables that can independently
> set to 'yes') possibilities, so something along the lines of ...
>
>          test_commit () {
>         +	tag=light &&
>                 notick= &&
>          ...
> 		while test $# != 0
> 		do
> 			case "$1" in
> 			...
> 			--no-tag)
> 	-			no_tag=yes
> 	+			tag=none
> 				;;
> 	+		--annotated)
> 	+			tag=annotated
> 	+			;;
> 		...
> 	-	if test -z "$no_tag"
> 	-	then
> 	+	case "$tag" in
> 	+	none)
> 	+		;;
> 	+	light)
> 			git ${indir:+ -C "$indir") tag "${4:-$1}"
> 	+		;;
> 	+	annotated)
> 	+		git ${indir:+ -C "$indir") tag -m "$1" "${4:-$1}"
> 	+		;;
> 	+	esac
> 		...
>
> after this step (meaning, we may want to start from fixing the no_tag=yes
> to tag=none before introducing this new feature).

Yeah, as noted in the last paragraph of the commit message:
    
    The placement of --annotated-tag after "notick" in the case of the
    documentation, and then after "no_tag" in the case of the code is
    slightly inconsistent. It's to evade a merge conflict with two other
    commits adding a --printf option, and another one adding documentation
    for --no-tag.

So the existing patch + not doing a bigger refactoring is because I
didn't want to cause conflicts for you to solve with other in-flight
topics. It would be easier with ab/pickaxe-pcre2 merged down :)

I'd prefer to keep this as-is for now, and just refactor this function a
bit after the current topics land.

In particular I'd like to make the the "message file contents tags"
arguments optionally have --long-option versions, so e.g. tests that
need a separate --commit-message and --tag-message can use the helper,
there's also quite a few that could use a --file=<existing file>
v.s. echo-ing a "message" to a "file".


>> @@ -244,7 +252,15 @@ test_commit () {
>>  	    $signoff -m "$1" &&
>>  	if test -z "$no_tag"
>>  	then
>> -		git ${indir:+ -C "$indir"} tag "${4:-$1}"
>> +		if test -n "$annotated_tag"
>> +		then
>> +			if test -z "$notick"
>> +			then
>> +				test_tick
>> +			fi &&
>> +			test_tick
>> +		fi &&
>> +		git ${indir:+ -C "$indir"} tag ${annotated_tag:+ -a -m "$1"} "${4:-$1}"
>>  	fi
>>  }

Re: [PATCH 04/10] test-lib functions: add an --annotated-tag option to "test_commit"

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> Yeah, as noted in the last paragraph of the commit message:
>     
>     The placement of --annotated-tag after "notick" in the case of the
>     documentation, and then after "no_tag" in the case of the code is
>     slightly inconsistent. It's to evade a merge conflict with two other
>     commits adding a --printf option, and another one adding documentation
>     for --no-tag.
>
> So the existing patch + not doing a bigger refactoring is because I
> didn't want to cause conflicts for you to solve with other in-flight
> topics. It would be easier with ab/pickaxe-pcre2 merged down :)

As things that are not yet in 'master' will not be merged down for a
few weeks anyway, the conflicts don't matter that much---it is a
valid choice not to accept new topics that conflicts with topics
cooking in 'next' after all.  That way, the number of topics that
folks have to look at will not increase, which gives more opportunity
for topics that are already in 'seen' to get reviewed ;-)

Re: [PATCH 00/10] describe tests: refactor & fix recent broken tests

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> While looking at the "git describe" tests due to the on-list
> %(describe) discussion I discovered that the feature added in
> 30b1c7ad9d (describe: don't abort too early when searching tags,
> 2020-02-26) has never been tested for.
>
> This is because it defined a custom test function that called
> test_expect_success, which then got called inside another top-level
> test_expect_success. Thus even if it failed we'd pass the test.

Thanks.  I think I've seen something similar recently.  I wonder if
it is an easy mistake our test linter can catch.

> This series fixes that issue, and makes some general improvements to
> the "describe" tests. I then make test_expect_{success,failure} return
> 1 to catch these sorts of issues in the future, which required fixes
> to a couple of svn tests that ran with "set -e".

OK.

> I was on the fence about whether to send this after the recent rc0,
> but figured that since it's test-only Junio might want to pick it up,

Even if it is too late, giving earlier exposure would by itself be a
good thing (just don't forget to resend after the dust settles, so
that it would land eventually).

> and possibly for the next rc in case we'd like to do some pre-release
> testing for this never-before-tested feature added in 2.26.0 (although
> the actual implementation looks fine to me).

Thanks; it obviously is a lower priority as we have survived with
the broken test for a long time since 2.26 ;-)

> Ævar Arnfjörð Bjarmason (10):
>   describe tests: improve test for --work-tree & --dirty
>   describe tests: refactor away from glob matching
>   describe tests: always assert empty stderr from "describe"
>   test-lib functions: add an --annotated-tag option to "test_commit"
>   describe tests: convert setup to use test_commit
>   describe tests: fix nested "test_expect_success" call
>   describe tests: support -C in "check_describe"
>   svn tests: remove legacy re-setup from init-clone test
>   svn tests: refactor away a "set -e" in test body
>   test-lib: return 1 from test_expect_{success,failure}
>
>  t/t1403-show-ref.sh           |   6 +-
>  t/t6120-describe.sh           | 193 +++++++++++++++-------------------
>  t/t9117-git-svn-init-clone.sh |   6 --
>  t/t9148-git-svn-propset.sh    |  27 ++---
>  t/test-lib-functions.sh       |  20 +++-
>  5 files changed, 122 insertions(+), 130 deletions(-)

Re: [PATCH 00/10] describe tests: refactor & fix recent broken tests

[Junio C Hamano]

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> While looking at the "git describe" tests due to the on-list
> %(describe) discussion I discovered that the feature added in
> 30b1c7ad9d (describe: don't abort too early when searching tags,
> 2020-02-26) has never been tested for.

This came during -rc freeze and got reviewed, so I'd expect a reroll
sometime but does not have to be done in a hurry before the final
which is expected to happen on the 15th.

Thanks.

[PATCH v2 0/5] describe test fixes

[Ævar Arnfjörð Bjarmason]

As noted in v1 we have a test for "git describe" test that tests for
nothing at all, due to being a nested test_expect_success.

This series is based on my "test-lib.sh fixes" series[2] and fixes
that bug, and should address all feedback on v1. In particular:

 * Shell quoting fixes
 * The "nothing should have stderr" general testing is gone per
   Junio's request.
 * Required test-lib.sh fixes moved to [1]
 * We no longer change the return value of
   test_expect_{success,failure} to narrowly catch the bug being fixed
   here.
 * Ejected SVN test fixup patches, needed for the now-ejected
   test_expect_{success,failure} change. Those fixes still make sense,
   but I'll submit them separately (they don't depend on anything
   else).

I think the "catch the bug" is probably a good idea, but Junio's
suggestion of tracking this via some env variable "stack depth" is
something that would probably collide with t0000*.sh changes I have
unsubmitted/outstanding, and I don't have time to pursue it now. So
I've left that out.

1. https://lore.kernel.org/git/20210228195414.21372-1-avarab@gmail.com/#t
2. https://lore.kernel.org/git/cover-00.16-00000000000-20210412T110456Z-avarab@gmail.com/

Ævar Arnfjörð Bjarmason (5):
  describe tests: improve test for --work-tree & --dirty
  describe tests: refactor away from glob matching
  describe tests: don't rely on err.actual from "check_describe"
  describe tests: fix nested "test_expect_success" call
  describe tests: support -C in "check_describe"

 t/t6120-describe.sh | 134 ++++++++++++++++++++++++--------------------
 1 file changed, 72 insertions(+), 62 deletions(-)

Range-diff against v1:
 1:  d76214a9171 =  1:  c41a777462e describe tests: improve test for --work-tree & --dirty
 2:  c1b8625de15 !  2:  b428f468d68 describe tests: refactor away from glob matching
    @@ t/t6120-describe.sh: check_describe () {
     -	esac
     +		git describe $describe_opts 2>err.actual >raw &&
     +		sed -e "s/-g[0-9a-f]*\$/-gHASH/" <raw >actual &&
    -+		echo $expect >expect &&
    ++		echo "$expect" >expect &&
     +		test_cmp expect actual
      	'
      }
      
     @@ t/t6120-describe.sh: test_expect_success setup '
    - 
    + 	test_commit --no-tag x file
      '
      
     -check_describe A-* HEAD
 3:  ac1a658d07f !  3:  50b5a41f88d describe tests: always assert empty stderr from "describe"
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    describe tests: always assert empty stderr from "describe"
    +    describe tests: don't rely on err.actual from "check_describe"
     
    -    Invert a test added in 3291fe4072e (Add git-describe test for "verify
    -    annotated tag names on output", 2008-03-03) to make checking that we
    -    don't have warnings the rule rather than the exception.
    +    Convert the one test that relied on the "err.actual" file produced by
    +    check_describe() to instead do its own check of "git describe"
    +    output.
     
    -    There was only one case where we expected and got a warning. Let's
    -    test for that case explicitly, and assert no warnings or other stderr
    -    output for all the rest.
    +    This means that the two tests won't have an inter-dependency (e.g. if
    +    the earlier test is skipped).
    +
    +    An earlier version of this patch instead asserted that no other test
    +    had any output on stderr. We're not doing that here out of fear that
    +    "gc --auto" or another future change to "git describe" will cause it
    +    to legitimately emit output on stderr unexpectedly[1].
    +
    +    I'd think that inverting the test added in 3291fe4072e (Add
    +    git-describe test for "verify annotated tag names on output",
    +    2008-03-03) to make checking that we don't have warnings the rule
    +    rather than the exception would be the sort of thing the describe
    +    tests should be catching, but for now let's leave it as it is.
    +
    +    1. http://lore.kernel.org/git/xmqqwnuqo8ze.fsf@gitster.c.googlers.com
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ t/t6120-describe.sh: check_describe () {
      	describe_opts="$@"
      	test_expect_success "describe $describe_opts" '
     -		git describe $describe_opts 2>err.actual >raw &&
    -+		git describe $describe_opts 2>err >raw &&
    -+		test_must_be_empty err &&
    ++		git describe $describe_opts >raw &&
      		sed -e "s/-g[0-9a-f]*\$/-gHASH/" <raw >actual &&
    - 		echo $expect >expect &&
    + 		echo "$expect" >expect &&
      		test_cmp expect actual
     @@ t/t6120-describe.sh: test_expect_success 'describe --contains defaults to HEAD without commit-ish' '
      '
    @@ t/t6120-describe.sh: test_expect_success 'describe --contains defaults to HEAD w
     -'
     +test_expect_success 'renaming tag A to Q locally produces a warning' "
     +	mv .git/refs/tags/A .git/refs/tags/Q &&
    -+	git describe HEAD 2>actual >out &&
    ++	git describe HEAD 2>err >out &&
     +	cat >expected <<-\EOF &&
     +	warning: tag 'Q' is externally known as 'A'
     +	EOF
    -+	test_cmp expected actual &&
    ++	test_cmp expected err &&
     +	grep -E '^A-8-g[0-9a-f]+$' out
     +"
     +
 4:  15efc2c6242 <  -:  ----------- test-lib functions: add an --annotated-tag option to "test_commit"
 5:  06a8904d693 <  -:  ----------- describe tests: convert setup to use test_commit
 6:  91424c8392b =  4:  5c81358d6bb describe tests: fix nested "test_expect_success" call
 7:  ecb8f6fb02f !  5:  798f6cd63c8 describe tests: support -C in "check_describe"
    @@ t/t6120-describe.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
      	shift
      	describe_opts="$@"
      	test_expect_success "describe $describe_opts" '
    --		git describe $describe_opts 2>err >raw &&
    -+		git ${indir:+ -C "$indir"} describe $describe_opts 2>err >raw &&
    - 		test_must_be_empty err &&
    +-		git describe $describe_opts >raw &&
    ++		git ${indir:+ -C "$indir"} describe $describe_opts >raw &&
      		sed -e "s/-g[0-9a-f]*\$/-gHASH/" <raw >actual &&
    - 		echo $expect >expect &&
    + 		echo "$expect" >expect &&
    + 		test_cmp expect actual
     @@ t/t6120-describe.sh: test_expect_success 'setup: describe commits with disjoint bases' '
      	)
      '
 8:  be5ed59dc61 <  -:  ----------- svn tests: remove legacy re-setup from init-clone test
 9:  0b4238d012a <  -:  ----------- svn tests: refactor away a "set -e" in test body
10:  4f2c4f1fdd5 <  -:  ----------- test-lib: return 1 from test_expect_{success,failure}
-- 
2.31.1.634.gb41287a30b0

[PATCH v2 1/5] describe tests: improve test for --work-tree & --dirty

[Ævar Arnfjörð Bjarmason]

Improve tests added in 9f67d2e8279 (Teach "git describe" --dirty
option, 2009-10-21) and 2ed5c8e174d (describe: setup working tree for
--dirty, 2019-02-03) so that they make sense in combination with each
other.

The "check_describe" being removed here was the earlier test, we then
later added these --work-tree tests which really just wanted to check
if we got the exact same output from "describe", but the test wasn't
structured to test for that.

Let's change it to do that, which both improves test coverage and
makes it more obvious what's going on here.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 88fddc91424..a83ea15faaf 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -151,24 +151,24 @@ test_expect_success 'set-up dirty work tree' '
 	echo >>file
 '
 
-check_describe "A-*[0-9a-f]-dirty" --dirty
-
 test_expect_success 'describe --dirty with --work-tree (dirty)' '
+	git describe --dirty >expected &&
 	(
 		cd "$TEST_DIRECTORY" &&
 		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
 	) &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+-dirty$" out
+	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+-dirty$" out &&
+	test_cmp expected out
 '
 
-check_describe "A-*[0-9a-f].mod" --dirty=.mod
-
 test_expect_success 'describe --dirty=.mod with --work-tree (dirty)' '
+	git describe --dirty=.mod >expected &&
 	(
 		cd "$TEST_DIRECTORY" &&
 		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty=.mod >"$TRASH_DIRECTORY/out"
 	) &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+.mod$" out
+	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+.mod$" out &&
+	test_cmp expected out
 '
 
 test_expect_success 'describe --dirty HEAD' '
-- 
2.31.1.634.gb41287a30b0

[PATCH v2 2/5] describe tests: refactor away from glob matching

[Ævar Arnfjörð Bjarmason]

Change the glob matching via a "case" statement to a "test_cmp" after
we've stripped out the hash-specific g<hash-abbrev>
suffix. 5312ab11fbf (Add describe test., 2007-01-13).

This means that we can use test_cmp to compare the output. I could
omit the "-8" change of e.g. "A-*" to "A-8-gHASH", but I think it
makes sense to test that here explicitly. It means you need to add new
tests to the bottom of the file, but that's not a burden in this case.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 78 ++++++++++++++++++++++-----------------------
 1 file changed, 38 insertions(+), 40 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index a83ea15faaf..13117bbcfb7 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -21,12 +21,10 @@ check_describe () {
 	shift
 	describe_opts="$@"
 	test_expect_success "describe $describe_opts" '
-	R=$(git describe $describe_opts 2>err.actual) &&
-	case "$R" in
-	$expect)	echo happy ;;
-	*)	echo "Oops - $R is not $expect" &&
-		false ;;
-	esac
+		git describe $describe_opts 2>err.actual >raw &&
+		sed -e "s/-g[0-9a-f]*\$/-gHASH/" <raw >actual &&
+		echo "$expect" >expect &&
+		test_cmp expect actual
 	'
 }
 
@@ -59,29 +57,29 @@ test_expect_success setup '
 	test_commit --no-tag x file
 '
 
-check_describe A-* HEAD
-check_describe A-* HEAD^
-check_describe R-* HEAD^^
-check_describe A-* HEAD^^2
+check_describe A-8-gHASH HEAD
+check_describe A-7-gHASH HEAD^
+check_describe R-2-gHASH HEAD^^
+check_describe A-3-gHASH HEAD^^2
 check_describe B HEAD^^2^
-check_describe R-* HEAD^^^
+check_describe R-1-gHASH HEAD^^^
 
-check_describe c-* --tags HEAD
-check_describe c-* --tags HEAD^
-check_describe e-* --tags HEAD^^
-check_describe c-* --tags HEAD^^2
+check_describe c-7-gHASH --tags HEAD
+check_describe c-6-gHASH --tags HEAD^
+check_describe e-1-gHASH --tags HEAD^^
+check_describe c-2-gHASH --tags HEAD^^2
 check_describe B --tags HEAD^^2^
 check_describe e --tags HEAD^^^
 
 check_describe heads/main --all HEAD
-check_describe tags/c-* --all HEAD^
+check_describe tags/c-6-gHASH --all HEAD^
 check_describe tags/e --all HEAD^^^
 
-check_describe B-0-* --long HEAD^^2^
-check_describe A-3-* --long HEAD^^2
+check_describe B-0-gHASH --long HEAD^^2^
+check_describe A-3-gHASH --long HEAD^^2
 
-check_describe c-7-* --tags
-check_describe e-3-* --first-parent --tags
+check_describe c-7-gHASH --tags
+check_describe e-3-gHASH --first-parent --tags
 
 test_expect_success 'describe --contains defaults to HEAD without commit-ish' '
 	echo "A^0" >expect &&
@@ -102,7 +100,7 @@ test_expect_success 'rename tag A to Q locally' '
 cat - >err.expect <<EOF
 warning: tag 'Q' is externally known as 'A'
 EOF
-check_describe A-* HEAD
+check_describe A-8-gHASH HEAD
 test_expect_success 'warning was displayed for Q' '
 	test_cmp err.expect err.actual
 '
@@ -129,22 +127,22 @@ test_expect_success 'rename tag Q back to A' '
 '
 
 test_expect_success 'pack tag refs' 'git pack-refs'
-check_describe A-* HEAD
+check_describe A-8-gHASH HEAD
 
 test_expect_success 'describe works from outside repo using --git-dir' '
 	git clone --bare "$TRASH_DIRECTORY" "$TRASH_DIRECTORY/bare" &&
 	git --git-dir "$TRASH_DIRECTORY/bare" describe >out &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+$" out
+	grep -E "^A-8-g[0-9a-f]+$" out
 '
 
-check_describe "A-*[0-9a-f]" --dirty
+check_describe "A-8-gHASH" --dirty
 
 test_expect_success 'describe --dirty with --work-tree' '
 	(
 		cd "$TEST_DIRECTORY" &&
 		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
 	) &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+$" out
+	grep -E "^A-8-g[0-9a-f]+$" out
 '
 
 test_expect_success 'set-up dirty work tree' '
@@ -157,7 +155,7 @@ test_expect_success 'describe --dirty with --work-tree (dirty)' '
 		cd "$TEST_DIRECTORY" &&
 		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty >"$TRASH_DIRECTORY/out"
 	) &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+-dirty$" out &&
+	grep -E "^A-8-g[0-9a-f]+-dirty$" out &&
 	test_cmp expected out
 '
 
@@ -167,7 +165,7 @@ test_expect_success 'describe --dirty=.mod with --work-tree (dirty)' '
 		cd "$TEST_DIRECTORY" &&
 		git --git-dir "$TRASH_DIRECTORY/.git" --work-tree "$TRASH_DIRECTORY" describe --dirty=.mod >"$TRASH_DIRECTORY/out"
 	) &&
-	grep -E "^A-[1-9][0-9]?-g[0-9a-f]+.mod$" out &&
+	grep -E "^A-8-g[0-9a-f]+.mod$" out &&
 	test_cmp expected out
 '
 
@@ -191,21 +189,21 @@ test_expect_success 'set-up matching pattern tests' '
 
 '
 
-check_describe "test-annotated-*" --match="test-*"
+check_describe "test-annotated-3-gHASH" --match="test-*"
 
-check_describe "test1-lightweight-*" --tags --match="test1-*"
+check_describe "test1-lightweight-2-gHASH" --tags --match="test1-*"
 
-check_describe "test2-lightweight-*" --tags --match="test2-*"
+check_describe "test2-lightweight-1-gHASH" --tags --match="test2-*"
 
-check_describe "test2-lightweight-*" --long --tags --match="test2-*" HEAD^
+check_describe "test2-lightweight-0-gHASH" --long --tags --match="test2-*" HEAD^
 
-check_describe "test2-lightweight-*" --long --tags --match="test1-*" --match="test2-*" HEAD^
+check_describe "test2-lightweight-0-gHASH" --long --tags --match="test1-*" --match="test2-*" HEAD^
 
-check_describe "test2-lightweight-*" --long --tags --match="test1-*" --no-match --match="test2-*" HEAD^
+check_describe "test2-lightweight-0-gHASH" --long --tags --match="test1-*" --no-match --match="test2-*" HEAD^
 
-check_describe "test1-lightweight-*" --long --tags --match="test1-*" --match="test3-*" HEAD
+check_describe "test1-lightweight-2-gHASH" --long --tags --match="test1-*" --match="test3-*" HEAD
 
-check_describe "test1-lightweight-*" --long --tags --match="test3-*" --match="test1-*" HEAD
+check_describe "test1-lightweight-2-gHASH" --long --tags --match="test3-*" --match="test1-*" HEAD
 
 test_expect_success 'set-up branches' '
 	git branch branch_A A &&
@@ -215,11 +213,11 @@ test_expect_success 'set-up branches' '
 	git update-ref refs/original/original_branch_A test-annotated~2
 '
 
-check_describe "heads/branch_A*" --all --match="branch_*" --exclude="branch_C" HEAD
+check_describe "heads/branch_A-11-gHASH" --all --match="branch_*" --exclude="branch_C" HEAD
 
-check_describe "remotes/origin/remote_branch_A*" --all --match="origin/remote_branch_*" --exclude="origin/remote_branch_C" HEAD
+check_describe "remotes/origin/remote_branch_A-11-gHASH" --all --match="origin/remote_branch_*" --exclude="origin/remote_branch_C" HEAD
 
-check_describe "original/original_branch_A*" --all test-annotated~1
+check_describe "original/original_branch_A-6-gHASH" --all test-annotated~1
 
 test_expect_success '--match does not work for other types' '
 	test_must_fail git describe --all --match="*original_branch_*" test-annotated~1
@@ -489,7 +487,7 @@ test_expect_success 'describe commits with disjoint bases' '
 		git tag B -a -m B &&
 		git merge --no-ff --allow-unrelated-histories main -m x &&
 
-		check_describe "A-3-*" HEAD
+		check_describe "A-3-gHASH" HEAD
 	)
 '
 
@@ -515,7 +513,7 @@ test_expect_success 'describe commits with disjoint bases 2' '
 		git tag B -a -m B &&
 		git merge --no-ff --allow-unrelated-histories main -m x &&
 
-		check_describe "B-3-*" HEAD
+		check_describe "B-3-gHASH" HEAD
 	)
 '
 
-- 
2.31.1.634.gb41287a30b0

[PATCH v2 3/5] describe tests: don't rely on err.actual from "check_describe"

[Ævar Arnfjörð Bjarmason]

Convert the one test that relied on the "err.actual" file produced by
check_describe() to instead do its own check of "git describe"
output.

This means that the two tests won't have an inter-dependency (e.g. if
the earlier test is skipped).

An earlier version of this patch instead asserted that no other test
had any output on stderr. We're not doing that here out of fear that
"gc --auto" or another future change to "git describe" will cause it
to legitimately emit output on stderr unexpectedly[1].

I'd think that inverting the test added in 3291fe4072e (Add
git-describe test for "verify annotated tag names on output",
2008-03-03) to make checking that we don't have warnings the rule
rather than the exception would be the sort of thing the describe
tests should be catching, but for now let's leave it as it is.

1. http://lore.kernel.org/git/xmqqwnuqo8ze.fsf@gitster.c.googlers.com

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 25 +++++++++++--------------
 1 file changed, 11 insertions(+), 14 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 13117bbcfb7..911b1928057 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -21,7 +21,7 @@ check_describe () {
 	shift
 	describe_opts="$@"
 	test_expect_success "describe $describe_opts" '
-		git describe $describe_opts 2>err.actual >raw &&
+		git describe $describe_opts >raw &&
 		sed -e "s/-g[0-9a-f]*\$/-gHASH/" <raw >actual &&
 		echo "$expect" >expect &&
 		test_cmp expect actual
@@ -90,20 +90,17 @@ test_expect_success 'describe --contains defaults to HEAD without commit-ish' '
 '
 
 check_describe tags/A --all A^0
-test_expect_success 'no warning was displayed for A' '
-	test_must_be_empty err.actual
-'
 
-test_expect_success 'rename tag A to Q locally' '
-	mv .git/refs/tags/A .git/refs/tags/Q
-'
-cat - >err.expect <<EOF
-warning: tag 'Q' is externally known as 'A'
-EOF
-check_describe A-8-gHASH HEAD
-test_expect_success 'warning was displayed for Q' '
-	test_cmp err.expect err.actual
-'
+test_expect_success 'renaming tag A to Q locally produces a warning' "
+	mv .git/refs/tags/A .git/refs/tags/Q &&
+	git describe HEAD 2>err >out &&
+	cat >expected <<-\EOF &&
+	warning: tag 'Q' is externally known as 'A'
+	EOF
+	test_cmp expected err &&
+	grep -E '^A-8-g[0-9a-f]+$' out
+"
+
 test_expect_success 'misnamed annotated tag forces long output' '
 	description=$(git describe --no-long Q^0) &&
 	expr "$description" : "A-0-g[0-9a-f]*$" &&
-- 
2.31.1.634.gb41287a30b0

[PATCH v2 4/5] describe tests: fix nested "test_expect_success" call

[Ævar Arnfjörð Bjarmason]

Fix a nested invocation of "test_expect_success", the
"check_describe()" function is a wrapper for calling
test_expect_success, and therefore needs to be called outside the body
of another "test_expect_success".

The two tests added in 30b1c7ad9d6 (describe: don't abort too early
when searching tags, 2020-02-26) were not testing for anything due to
this logic error. Without this fix reverting the C code changes in
that commit still has all tests passing, with this fix we're actually
testing the "describe" output. This is because "test_expect_success"
calls "test_finish_", whose last statement happens to be true.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 911b1928057..9dc07782ea6 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -469,7 +469,7 @@ test_expect_success 'name-rev covers all conditions while looking at parents' '
 #  o-----o---o----x
 #        A
 #
-test_expect_success 'describe commits with disjoint bases' '
+test_expect_success 'setup: describe commits with disjoint bases' '
 	git init disjoint1 &&
 	(
 		cd disjoint1 &&
@@ -482,19 +482,22 @@ test_expect_success 'describe commits with disjoint bases' '
 		git checkout --orphan branch && rm file &&
 		echo B > file2 && git add file2 && git commit -m B &&
 		git tag B -a -m B &&
-		git merge --no-ff --allow-unrelated-histories main -m x &&
-
-		check_describe "A-3-gHASH" HEAD
+		git merge --no-ff --allow-unrelated-histories main -m x
 	)
 '
 
+(
+	cd disjoint1 &&
+	check_describe "A-3-gHASH" HEAD
+)
+
 #           B
 #   o---o---o------------.
 #                         \
 #                  o---o---x
 #                  A
 #
-test_expect_success 'describe commits with disjoint bases 2' '
+test_expect_success 'setup: describe commits with disjoint bases 2' '
 	git init disjoint2 &&
 	(
 		cd disjoint2 &&
@@ -508,10 +511,13 @@ test_expect_success 'describe commits with disjoint bases 2' '
 		echo o >> file2 && git add file2 && GIT_COMMITTER_DATE="2020-01-01 15:01" git commit -m o &&
 		echo B >> file2 && git add file2 && GIT_COMMITTER_DATE="2020-01-01 15:02" git commit -m B &&
 		git tag B -a -m B &&
-		git merge --no-ff --allow-unrelated-histories main -m x &&
-
-		check_describe "B-3-gHASH" HEAD
+		git merge --no-ff --allow-unrelated-histories main -m x
 	)
 '
 
+(
+	cd disjoint2 &&
+	check_describe "B-3-gHASH" HEAD
+)
+
 test_done
-- 
2.31.1.634.gb41287a30b0

[PATCH v2 5/5] describe tests: support -C in "check_describe"

[Ævar Arnfjörð Bjarmason]

Change a subshell added in a preceding commit to instead use a new
"-C" option to "check_describe". The idiom for this is copied as-is
from the "test_commit" function in test-lib-functions.sh

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t6120-describe.sh | 27 ++++++++++++++++++---------
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 9dc07782ea6..1a501ee09e1 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -17,11 +17,26 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . ./test-lib.sh
 
 check_describe () {
+	indir= &&
+	while test $# != 0
+	do
+		case "$1" in
+		-C)
+			indir="$2"
+			shift
+			;;
+		*)
+			break
+			;;
+		esac
+		shift
+	done &&
+	indir=${indir:+"$indir"/} &&
 	expect="$1"
 	shift
 	describe_opts="$@"
 	test_expect_success "describe $describe_opts" '
-		git describe $describe_opts >raw &&
+		git ${indir:+ -C "$indir"} describe $describe_opts >raw &&
 		sed -e "s/-g[0-9a-f]*\$/-gHASH/" <raw >actual &&
 		echo "$expect" >expect &&
 		test_cmp expect actual
@@ -486,10 +501,7 @@ test_expect_success 'setup: describe commits with disjoint bases' '
 	)
 '
 
-(
-	cd disjoint1 &&
-	check_describe "A-3-gHASH" HEAD
-)
+check_describe -C disjoint1 "A-3-gHASH" HEAD
 
 #           B
 #   o---o---o------------.
@@ -515,9 +527,6 @@ test_expect_success 'setup: describe commits with disjoint bases 2' '
 	)
 '
 
-(
-	cd disjoint2 &&
-	check_describe "B-3-gHASH" HEAD
-)
+check_describe -C disjoint2 "B-3-gHASH" HEAD
 
 test_done
-- 
2.31.1.634.gb41287a30b0

[PATCH v2 0/2] svn tests: trivial "set -e" in main body of test fixes

[Ævar Arnfjörð Bjarmason]

As noted in 1. this set of trivial fixes has been split off from my
"describe tests fixes. The two patches were needed for a now-ejected
"make test_expect_{success,failure} return 1" (to catch such "set -e"
issues).

Even with that gone for now and thus the immediate incentive for these
fixes being gone, it makes sense to fix these tests to use a more
modern style.

The only other change since v1 is using ">" instead of "touch" per
Eric Wong's suggestion, and rewording the commit message(s) to not
promise a follow-up change to test_expect_{success,failure}.

Ævar Arnfjörð Bjarmason (2):
  svn tests: remove legacy re-setup from init-clone test
  svn tests: refactor away a "set -e" in test body

 t/t9117-git-svn-init-clone.sh |  6 ------
 t/t9148-git-svn-propset.sh    | 27 +++++++++++++++------------
 2 files changed, 15 insertions(+), 18 deletions(-)

Range-diff against v1:
1:  be5ed59dc61 = 1:  d08e098ea8d svn tests: remove legacy re-setup from init-clone test
2:  0b4238d012a ! 2:  8dd13c24994 svn tests: refactor away a "set -e" in test body
    @@ Commit message
         svn tests: refactor away a "set -e" in test body
     
         Refactor a test added in 83c9433e67 (git-svn: support for git-svn
    -    propset, 2014-12-07) to avoid using "set -e" in the test body. This
    -    would have broken in combination with a subsequent change to make
    -    "test_expect_success" return 1 to catch such cases.
    +    propset, 2014-12-07) to avoid using "set -e" in the test body. Let's
    +    move this into a setup test using "test_expect_success" instead.
     
    -    While I'm at it rewrite the test to conform to a modern style in our
    -    tests, using the "test_when_finished" function for the "rm -rf", and
    -    avoiding repeated "mkdir" in favor of "mkdir -p".
    +    While I'm at it refactor:
    +
    +     * Repeated "mkdir" to "mkdir -p"
    +     * Uses of "touch" to creating the files with ">" instead
    +     * The "rm -rf" at the end to happen in a "test_when_finished"
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    @@ t/t9148-git-svn-propset.sh: test_description='git svn propset tests'
     +	(
     +		cd import &&
     +		# for "add props top level"
    -+		touch foo &&
    ++		>foo &&
     +		# for "add props relative"
    -+		touch subdir/foo_subdir &&
    ++		>subdir/foo_subdir &&
     +		# for "add props subdir"
    -+		touch "$foo_subdir2" &&
    ++		>"$foo_subdir2" &&
     +		svn_cmd import -m "import for git svn" . "$svnrepo"
     +	)
     +'
3:  4f2c4f1fdd5 < -:  ----------- test-lib: return 1 from test_expect_{success,failure}
-- 
2.31.1.634.gb41287a30b0

[PATCH v2 1/2] svn tests: remove legacy re-setup from init-clone test

[Ævar Arnfjörð Bjarmason]

Remove the immediate "rm -rf .git" from the start of this test. This
was added back in 41337e22f0 (git-svn: add tests for command-line
usage of init and clone commands, 2007-11-17) when there was a "trash"
directory shared by all the tests, but ever since abc5d372ec (Enable
parallel tests, 2008-08-08) we've had per-test trash directories.

So this setup can simply be removed. We could use
TEST_NO_CREATE_REPO=true, but I don't think it's worth the effort to
go out of our way to be different. It doesn't matter that we now have
a redundant .git at the top-level.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t9117-git-svn-init-clone.sh | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh
index 044f65e9166..62de819a44e 100755
--- a/t/t9117-git-svn-init-clone.sh
+++ b/t/t9117-git-svn-init-clone.sh
@@ -7,12 +7,6 @@ test_description='git svn init/clone tests'
 
 . ./lib-git-svn.sh
 
-# setup, run inside tmp so we don't have any conflicts with $svnrepo
-set -e
-rm -r .git
-mkdir tmp
-cd tmp
-
 test_expect_success 'setup svnrepo' '
 	mkdir project project/trunk project/branches project/tags &&
 	echo foo > project/trunk/foo &&
-- 
2.31.1.634.gb41287a30b0

[PATCH v2 2/2] svn tests: refactor away a "set -e" in test body

[Ævar Arnfjörð Bjarmason]

Refactor a test added in 83c9433e67 (git-svn: support for git-svn
propset, 2014-12-07) to avoid using "set -e" in the test body. Let's
move this into a setup test using "test_expect_success" instead.

While I'm at it refactor:

 * Repeated "mkdir" to "mkdir -p"
 * Uses of "touch" to creating the files with ">" instead
 * The "rm -rf" at the end to happen in a "test_when_finished"

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t9148-git-svn-propset.sh | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/t/t9148-git-svn-propset.sh b/t/t9148-git-svn-propset.sh
index 102639090c1..aebb28995e5 100755
--- a/t/t9148-git-svn-propset.sh
+++ b/t/t9148-git-svn-propset.sh
@@ -7,19 +7,22 @@ test_description='git svn propset tests'
 
 . ./lib-git-svn.sh
 
-foo_subdir2="subdir/subdir2/foo_subdir2"
+test_expect_success 'setup propset via import' '
+	test_when_finished "rm -rf import" &&
 
-set -e
-mkdir import &&
-(set -e ; cd import
-	mkdir subdir
-	mkdir subdir/subdir2
-	touch foo 		# for 'add props top level'
-	touch subdir/foo_subdir # for 'add props relative'
-	touch "$foo_subdir2"	# for 'add props subdir'
-	svn_cmd import -m 'import for git svn' . "$svnrepo" >/dev/null
-)
-rm -rf import
+	foo_subdir2="subdir/subdir2/foo_subdir2" &&
+	mkdir -p import/subdir/subdir2 &&
+	(
+		cd import &&
+		# for "add props top level"
+		>foo &&
+		# for "add props relative"
+		>subdir/foo_subdir &&
+		# for "add props subdir"
+		>"$foo_subdir2" &&
+		svn_cmd import -m "import for git svn" . "$svnrepo"
+	)
+'
 
 test_expect_success 'initialize git svn' '
 	git svn init "$svnrepo"
-- 
2.31.1.634.gb41287a30b0