threads / patch / 52871

patchdescribe: dont abort too early when searching tags

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

## tl;dr

34 messages between Feb 23, 2020 and Apr 12, 2021. Diffs are folded; open one to read it.

replies: 33people: 4as markdown or json

Benno Evers· Feb 23, 2020, 12:51 UTC · lore

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(-)
Show changes to 2 files +70 −5

builtin/describe.c, t/t6120-describe.sh

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
Junio C Hamano· Feb 24, 2020, 20:52 UTC · re: Benno Evers · lore

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

Benno Evers <benno@bmevers.de> writes:
Show 23 quoted lines
> 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.
benno@bmevers.de· Feb 25, 2020, 19:07 UTC · re: Junio C Hamano · lore

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

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.
 
Junio C Hamano· Feb 25, 2020, 20:10 UTC · re: benno@bmevers.de · lore

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

benno@bmevers.de writes:
Show 11 quoted lines
> 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.
Ævar Arnfjörð Bjarmason· Feb 28, 2021, 19:54 UTC · re: Benno Evers · lore

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

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
Junio C Hamano· Mar 9, 2021, 00:47 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 8 quoted lines
> 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 ;-)

Show 18 quoted lines
> Æ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(-)
Junio C Hamano· Mar 12, 2021, 23:59 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Æ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.
Ævar Arnfjörð Bjarmason· Apr 12, 2021, 11:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 0/5] describe test fixes

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
Ævar Arnfjörð Bjarmason· Apr 12, 2021, 11:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to t/t6120-describe.sh +6 −6
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
Ævar Arnfjörð Bjarmason· Apr 12, 2021, 11:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to t/t6120-describe.sh +11 −14
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
Ævar Arnfjörð Bjarmason· Apr 12, 2021, 11:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to t/t6120-describe.sh +38 −40
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
Ævar Arnfjörð Bjarmason· Apr 12, 2021, 11:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to t/t6120-describe.sh +14 −8
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
Ævar Arnfjörð Bjarmason· Apr 12, 2021, 11:21 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to t/t6120-describe.sh +18 −9
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
Ævar Arnfjörð Bjarmason· Apr 12, 2021, 11:33 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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
Ævar Arnfjörð Bjarmason· Apr 12, 2021, 11:33 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to t/t9117-git-svn-init-clone.sh +0 −6
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
Ævar Arnfjörð Bjarmason· Apr 12, 2021, 11:33 UTC · re: Ævar Arnfjörð Bjarmason · lore

[PATCH v2 2/2] 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. 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(-)
Show changes to t/t9148-git-svn-propset.sh +15 −12
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
Ævar Arnfjörð Bjarmason· Feb 28, 2021, 19:54 UTC · re: Benno Evers · lore

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

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(-)
Show changes to t/t6120-describe.sh +6 −6
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
Ævar Arnfjörð Bjarmason· Feb 28, 2021, 19:54 UTC · re: Benno Evers · lore

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

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(-)
Show changes to t/t6120-describe.sh +38 −40
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
Junio C Hamano· Mar 1, 2021, 21:26 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 8 quoted lines
> 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.

Show 12 quoted lines
> 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
>  	'
>  }
Ævar Arnfjörð Bjarmason· Feb 28, 2021, 19:54 UTC · re: Benno Evers · lore

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

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(-)
Show changes to 2 files +19 −5

t/t1403-show-ref.sh, t/test-lib-functions.sh

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
Junio C Hamano· Mar 1, 2021, 21:41 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 7 quoted lines
> @@ -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.
Show 16 quoted lines
> @@ -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
>  }
Ævar Arnfjörð Bjarmason· Mar 2, 2021, 09:34 UTC · re: Junio C Hamano · lore

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

On Mon, Mar 01 2021, Junio C Hamano wrote:
Show 47 quoted lines
> Æ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".

Show 16 quoted lines
>> @@ -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
>>  }
Junio C Hamano· Mar 3, 2021, 06:35 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 11 quoted lines
> 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 ;-)

Ævar Arnfjörð Bjarmason· Feb 28, 2021, 19:54 UTC · re: Benno Evers · lore

[PATCH 03/10] describe tests: always assert empty stderr from "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.

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(-)
Show changes to t/t6120-describe.sh +12 −14
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
Junio C Hamano· Mar 1, 2021, 21:32 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 7 quoted lines
> 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.

Show 50 quoted lines
> 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]*$" &&
Ævar Arnfjörð Bjarmason· Feb 28, 2021, 19:54 UTC · re: Benno Evers · lore

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

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(-)
Show changes to t/t6120-describe.sh +13 −45
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
Junio C Hamano· Mar 1, 2021, 21:42 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Æ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(-)
Ævar Arnfjörð Bjarmason· Feb 28, 2021, 19:54 UTC · re: Benno Evers · lore

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

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(-)
Show changes to t/t6120-describe.sh +14 −8
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
Ævar Arnfjörð Bjarmason· Feb 28, 2021, 19:54 UTC · re: Benno Evers · lore

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

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(-)
Show changes to t/t9117-git-svn-init-clone.sh +0 −6
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
Ævar Arnfjörð Bjarmason· Feb 28, 2021, 19:54 UTC · re: Benno Evers · lore

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

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(-)
Show changes to t/t6120-describe.sh +18 −9
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
Ævar Arnfjörð Bjarmason· Feb 28, 2021, 19:54 UTC · re: Benno Evers · lore

[PATCH 09/10] 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.

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(-)
Show changes to t/t9148-git-svn-propset.sh +15 −12
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
Eric Wong· Feb 28, 2021, 21:14 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 8 quoted lines
> 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.

Show 32 quoted lines
> --- 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 :>

Ævar Arnfjörð Bjarmason· Feb 28, 2021, 19:54 UTC · re: Benno Evers · lore

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

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(+)
Show changes to t/test-lib-functions.sh +2 −0
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
Junio C Hamano· Mar 1, 2021, 21:43 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
Show 20 quoted lines
> 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.

← back to recent threads