{"thread":{"id":"52871","subject":"[PATCH] describe: dont abort too early when searching tags","startedAt":"2020-02-23T12:58:00Z","lastAt":"2021-04-12T11:33:41Z","messageCount":34,"participants":["Benno Evers","Junio C Hamano","benno@bmevers.de","Ævar Arnfjörð Bjarmason","Eric Wong"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"392354","messageId":"20200223125102.6697-1-benno@bmevers.de","threadId":"52871","inReplyTo":null,"subject":"[PATCH] describe: dont abort too early when searching tags","fromName":"Benno Evers","fromEmail":"benno@bmevers.de","sentAt":"2020-02-23T12:51:02Z","receivedAt":"2020-02-23T12:58:00Z","isPatch":true,"sender":{"key":"benno@bmevers.de","avatar":"https://avatars.githubusercontent.com/u/51410?v=4"},"body":"When searching the commit graph for tag candidates, `git-describe`\nwill stop as soon as there is only one active branch left and\nit already found an annotated tag as a candidate.\n\nThis works well as long as all branches eventually connect back\nto a common root, but if the tags are found across branches\nwith no common ancestor\n\n B\n o----.\n \\\n o-----o---o----x\n A\n\nit can happen that the search on one branch terminates prematurely\nbecause a tag was found on another, independent branch. This scenario\nisn't quite as obscure as it sounds, since cloning with a limited\ndepth often introduces many independent \"dead ends\" into the commit\ngraph.\n\nThe help text of `git-describe` states pretty clearly that when\ndescribing a commit D, the number appended to the emitted tag X should\ncorrespond to the number of commits found by `git log X..D`.\n\nThus, this commit modifies the stopping condition to only abort\nthe search when only one branch is left to search *and* all current\nbest candidates are descendants from that branch.\n\nSigned-off-by: Benno Evers \n---\nWe originally found this issue in one of our internal CI jobs,\nwhich relied on `git-describe` to assign filenames to the generated\nartifacts.\n---\n builtin/describe.c | 22 +++++++++++++++----\n t/t6120-describe.sh | 53 ++++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 70 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex b6df81d8d0..420f4c6401 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -376,11 +376,25 @@ static void describe_commit(struct object_id *oid, struct strbuf *dst)\n \t\t\tif (!(c->object.flags & t->flag_within))\n \t\t\t\tt->depth++;\n \t\t}\n+\t\t/* Stop if last remaining path already covered by best candidate(s) */\n \t\tif (annotated_cnt && !list) {\n-\t\t\tif (debug)\n-\t\t\t\tfprintf(stderr, _(\"finished search at %s\\n\"),\n-\t\t\t\t\toid_to_hex(&c->object.oid));\n-\t\t\tbreak;\n+\t\t\tint best_depth = INT_MAX;\n+\t\t\tunsigned best_within = 0;\n+\t\t\tfor (cur_match = 0; cur_match < match_cnt; cur_match++) {\n+\t\t\t\tstruct possible_tag *t = &all_matches[cur_match];\n+\t\t\t\tif (t->depth < best_depth) {\n+\t\t\t\t\tbest_depth = t->depth;\n+\t\t\t\t\tbest_within = t->flag_within;\n+\t\t\t\t} else if (t->depth == best_depth) {\n+\t\t\t\t\tbest_within |= t->flag_within;\n+\t\t\t\t}\n+\t\t\t}\n+\t\t\tif ((c->object.flags & best_within) == best_within) {\n+\t\t\t\tif (debug)\n+\t\t\t\t\tfprintf(stderr, _(\"finished search at %s\\n\"),\n+\t\t\t\t\t\toid_to_hex(&c->object.oid));\n+\t\t\t\tbreak;\n+\t\t\t}\n \t\t}\n \t\twhile (parents) {\n \t\t\tstruct commit *p = parents->item;\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 09c50f3f04..d8cc08258e 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -479,4 +479,55 @@ test_expect_success 'name-rev covers all conditions while looking at parents' '\n \t)\n '\n \n-test_done\n+# B\n+# o\n+# \\\n+# o-----o---o----x\n+# A\n+#\n+test_expect_success 'describe commits with disjoint bases' '\n+\tgit init disjoint1 &&\n+\t(\n+\t\tcd disjoint1 &&\n+\n+\t\techo o >> file && git add file && git commit -m o &&\n+\t\techo A >> file && git add file && git commit -m A &&\n+\t\tgit tag A -a -m A &&\n+\t\techo o >> file && git add file && git commit -m o &&\n+\n+\t\tgit checkout --orphan branch && rm file &&\n+\t\techo B > file2 && git add file2 && git commit -m B &&\n+\t\tgit tag B -a -m B &&\n+\t\tgit merge --no-ff --allow-unrelated-histories master -m x &&\n+\n+\t\tcheck_describe \"A-3-*\" HEAD\n+\t)\n+'\n+\n+# B\n+# o---o---o------------.\n+# \\\n+# o---o---x\n+# A\n+#\n+test_expect_success 'describe commits with disjoint bases 2' '\n+\tgit init disjoint2 &&\n+\t(\n+\t\tcd disjoint2 &&\n+\n+\t\techo A >> file && git add file && GIT_COMMITTER_DATE=\"2020-01-01 18:00\" git commit -m A &&\n+\t\tgit tag A -a -m A &&\n+\t\techo o >> file && git add file && GIT_COMMITTER_DATE=\"2020-01-01 18:01\" git commit -m o &&\n+\n+\t\tgit checkout --orphan branch &&\n+\t\techo o >> file2 && git add file2 && GIT_COMMITTER_DATE=\"2020-01-01 15:00\" git commit -m o &&\n+\t\techo o >> file2 && git add file2 && GIT_COMMITTER_DATE=\"2020-01-01 15:01\" git commit -m o &&\n+\t\techo B >> file2 && git add file2 && GIT_COMMITTER_DATE=\"2020-01-01 15:02\" git commit -m B &&\n+\t\tgit tag B -a -m B &&\n+\t\tgit merge --no-ff --allow-unrelated-histories master -m x &&\n+\n+\t\tcheck_describe \"B-3-*\" HEAD\n+\t)\n+'\n+\n+test_done\n\\ No newline at end of file\n-- \n2.20.1\n\n"},{"id":"392438","messageId":"xmqq4kvfpv17.fsf@gitster-ct.c.googlers.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"Re: [PATCH] describe: dont abort too early when searching tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-24T20:52:52Z","receivedAt":"2020-02-24T20:52:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Benno Evers writes:\n\n> When searching the commit graph for tag candidates, `git-describe`\n> will stop as soon as there is only one active branch left and\n> it already found an annotated tag as a candidate.\n>\n> This works well as long as all branches eventually connect back\n> to a common root, but if the tags are found across branches\n> with no common ancestor\n>\n> B\n> o----.\n> \\\n> o-----o---o----x\n> A\n>\n> it can happen that the search on one branch terminates prematurely\n> because a tag was found on another, independent branch. This scenario\n> isn't quite as obscure as it sounds, since cloning with a limited\n> depth often introduces many independent \"dead ends\" into the commit\n> graph.\n>\n> The help text of `git-describe` states pretty clearly that when\n> describing a commit D, the number appended to the emitted tag X should\n> correspond to the number of commits found by `git log X..D`.\n\nTo be fair, that description was written for the case in a normal\nhistory with only a single root.\n\nHow much, if any, does this change hurt the performance by forcing\nthe code to dig further if there aren't multiple roots? If there is\nsuch an unnecessary overhead that degrades performance for the more\ncommon case, can we improve it further to avoid it?\n\nThanks.\n\n\n\n"},{"id":"392476","messageId":"20200225190751.5898-1-benno@bmevers.de","threadId":"52871","inReplyTo":"xmqq4kvfpv17.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] describe: dont abort too early when searching tags","fromName":"","fromEmail":"benno@bmevers.de","sentAt":"2020-02-25T19:07:51Z","receivedAt":"2020-02-25T19:08:22Z","isPatch":true,"sender":{"key":"benno@bmevers.de","avatar":"https://avatars.githubusercontent.com/u/51410?v=4"},"body":"From: Benno Evers \n\n> How much, if any, does this change hurt the performance by forcing\n> the code to dig further if there aren't multiple roots? If there is\n> such an unnecessary overhead that degrades performance for the more\n> common case, can we improve it further to avoid it?\n\nIf there aren't multiple roots, then this should be visiting exactly\nthe same number of commits as before. This is because in this case, if\nwe're down to a single branch, the current commit must be an ancestor\nof *all* tag candidates so the stopping condition is always true.\n\nIt's actually a bit challenging to find good test repositories for\nthis in the wild, as the big ones that use a merging workflow (like git\nitself, the kernel, etc.) usually have so many branches active at any\npoint in time that the search will only stop when it hits the max number\nof candidates. So I might be missing some edge cases. However, for the\n\"normal\" repositories that I tested the number of traversed commits was\nalways the same before and after the change.\n \n"},{"id":"392481","messageId":"xmqq7e0amnrq.fsf@gitster-ct.c.googlers.com","threadId":"52871","inReplyTo":"20200225190751.5898-1-benno@bmevers.de","subject":"Re: [PATCH] describe: dont abort too early when searching tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-25T20:10:17Z","receivedAt":"2020-02-25T20:10:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"benno@bmevers.de writes:\n\n> From: Benno Evers \n>\n>> How much, if any, does this change hurt the performance by forcing\n>> the code to dig further if there aren't multiple roots? If there is\n>> such an unnecessary overhead that degrades performance for the more\n>> common case, can we improve it further to avoid it?\n>\n> If there aren't multiple roots, then this should be visiting exactly\n> the same number of commits as before. This is because in this case, if\n> we're down to a single branch, the current commit must be an ancestor\n> of *all* tag candidates so the stopping condition is always true.\n\nSounds good. Can some of that analysis go in the proposed log\nmessage text, so that other people do not have to ask the same\nquestion later?\n\nThanks.\n"},{"id":"417985","messageId":"20210228195414.21372-1-avarab@gmail.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"[PATCH 00/10] describe tests: refactor & fix recent broken tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-28T19:54:04Z","receivedAt":"2021-02-28T19:55:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"While looking at the \"git describe\" tests due to the on-list\n%(describe) discussion I discovered that the feature added in\n30b1c7ad9d (describe: don't abort too early when searching tags,\n2020-02-26) has never been tested for.\n\nThis is because it defined a custom test function that called\ntest_expect_success, which then got called inside another top-level\ntest_expect_success. Thus even if it failed we'd pass the test.\n\nThis series fixes that issue, and makes some general improvements to\nthe \"describe\" tests. I then make test_expect_{success,failure} return\n1 to catch these sorts of issues in the future, which required fixes\nto a couple of svn tests that ran with \"set -e\".\n\nI was on the fence about whether to send this after the recent rc0,\nbut figured that since it's test-only Junio might want to pick it up,\nand possibly for the next rc in case we'd like to do some pre-release\ntesting for this never-before-tested feature added in 2.26.0 (although\nthe actual implementation looks fine to me).\n\nÆvar Arnfjörð Bjarmason (10):\n describe tests: improve test for --work-tree & --dirty\n describe tests: refactor away from glob matching\n describe tests: always assert empty stderr from \"describe\"\n test-lib functions: add an --annotated-tag option to \"test_commit\"\n describe tests: convert setup to use test_commit\n describe tests: fix nested \"test_expect_success\" call\n describe tests: support -C in \"check_describe\"\n svn tests: remove legacy re-setup from init-clone test\n svn tests: refactor away a \"set -e\" in test body\n test-lib: return 1 from test_expect_{success,failure}\n\n t/t1403-show-ref.sh | 6 +-\n t/t6120-describe.sh | 193 +++++++++++++++-------------------\n t/t9117-git-svn-init-clone.sh | 6 --\n t/t9148-git-svn-propset.sh | 27 ++---\n t/test-lib-functions.sh | 20 +++-\n 5 files changed, 122 insertions(+), 130 deletions(-)\n\n-- \n2.31.0.rc0.116.g45ec00aa00\n\n"},{"id":"417986","messageId":"20210228195414.21372-2-avarab@gmail.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"[PATCH 01/10] describe tests: improve test for --work-tree & --dirty","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-28T19:54:05Z","receivedAt":"2021-02-28T19:55:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Improve tests added in 9f67d2e8279 (Teach \"git describe\" --dirty\noption, 2009-10-21) and 2ed5c8e174d (describe: setup working tree for\n--dirty, 2019-02-03) so that they make sense in combination with each\nother.\n\nThe \"check_describe\" being removed here was the earlier test, we then\nlater added these --work-tree tests which really just wanted to check\nif we got the exact same output from \"describe\", but the test wasn't\nstructured to test for that.\n\nLet's change it to do that, which both improves test coverage and\nmakes it more obvious what's going on here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6120-describe.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex e89b6747be..7bc2aaa46e 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -183,24 +183,24 @@ test_expect_success 'set-up dirty work tree' '\n \techo >>file\n '\n \n-check_describe \"A-*[0-9a-f]-dirty\" --dirty\n-\n test_expect_success 'describe --dirty with --work-tree (dirty)' '\n+\tgit describe --dirty >expected &&\n \t(\n \t\tcd \"$TEST_DIRECTORY\" &&\n \t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty >\"$TRASH_DIRECTORY/out\"\n \t) &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+-dirty$\" out\n+\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+-dirty$\" out &&\n+\ttest_cmp expected out\n '\n \n-check_describe \"A-*[0-9a-f].mod\" --dirty=.mod\n-\n test_expect_success 'describe --dirty=.mod with --work-tree (dirty)' '\n+\tgit describe --dirty=.mod >expected &&\n \t(\n \t\tcd \"$TEST_DIRECTORY\" &&\n \t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty=.mod >\"$TRASH_DIRECTORY/out\"\n \t) &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+.mod$\" out\n+\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+.mod$\" out &&\n+\ttest_cmp expected out\n '\n \n test_expect_success 'describe --dirty HEAD' '\n-- \n2.31.0.rc0.116.g45ec00aa00\n\n"},{"id":"417987","messageId":"20210228195414.21372-3-avarab@gmail.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"[PATCH 02/10] describe tests: refactor away from glob matching","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-28T19:54:06Z","receivedAt":"2021-02-28T19:55:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the glob matching via a \"case\" statement to a \"test_cmp\" after\nwe've stripped out the hash-specific g\nsuffix. 5312ab11fbf (Add describe test., 2007-01-13).\n\nThis means that we can use test_cmp to compare the output. I could\nomit the \"-8\" change of e.g. \"A-*\" to \"A-8-gHASH\", but I think it\nmakes sense to test that here explicitly. It means you need to add new\ntests to the bottom of the file, but that's not a burden in this case.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6120-describe.sh | 78 ++++++++++++++++++++++-----------------------\n 1 file changed, 38 insertions(+), 40 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 7bc2aaa46e..e4fd5d567f 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -21,12 +21,10 @@ check_describe () {\n \tshift\n \tdescribe_opts=\"$@\"\n \ttest_expect_success \"describe $describe_opts\" '\n-\tR=$(git describe $describe_opts 2>err.actual) &&\n-\tcase \"$R\" in\n-\t$expect)\techo happy ;;\n-\t*)\techo \"Oops - $R is not $expect\" &&\n-\t\tfalse ;;\n-\tesac\n+\t\tgit describe $describe_opts 2>err.actual >raw &&\n+\t\tsed -e \"s/-g[0-9a-f]*\\$/-gHASH/\" actual &&\n+\t\techo $expect >expect &&\n+\t\ttest_cmp expect actual\n \t'\n }\n \n@@ -91,29 +89,29 @@ test_expect_success setup '\n \n '\n \n-check_describe A-* HEAD\n-check_describe A-* HEAD^\n-check_describe R-* HEAD^^\n-check_describe A-* HEAD^^2\n+check_describe A-8-gHASH HEAD\n+check_describe A-7-gHASH HEAD^\n+check_describe R-2-gHASH HEAD^^\n+check_describe A-3-gHASH HEAD^^2\n check_describe B HEAD^^2^\n-check_describe R-* HEAD^^^\n+check_describe R-1-gHASH HEAD^^^\n \n-check_describe c-* --tags HEAD\n-check_describe c-* --tags HEAD^\n-check_describe e-* --tags HEAD^^\n-check_describe c-* --tags HEAD^^2\n+check_describe c-7-gHASH --tags HEAD\n+check_describe c-6-gHASH --tags HEAD^\n+check_describe e-1-gHASH --tags HEAD^^\n+check_describe c-2-gHASH --tags HEAD^^2\n check_describe B --tags HEAD^^2^\n check_describe e --tags HEAD^^^\n \n check_describe heads/main --all HEAD\n-check_describe tags/c-* --all HEAD^\n+check_describe tags/c-6-gHASH --all HEAD^\n check_describe tags/e --all HEAD^^^\n \n-check_describe B-0-* --long HEAD^^2^\n-check_describe A-3-* --long HEAD^^2\n+check_describe B-0-gHASH --long HEAD^^2^\n+check_describe A-3-gHASH --long HEAD^^2\n \n-check_describe c-7-* --tags\n-check_describe e-3-* --first-parent --tags\n+check_describe c-7-gHASH --tags\n+check_describe e-3-gHASH --first-parent --tags\n \n test_expect_success 'describe --contains defaults to HEAD without commit-ish' '\n \techo \"A^0\" >expect &&\n@@ -134,7 +132,7 @@ test_expect_success 'rename tag A to Q locally' '\n cat - >err.expect <out &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+$\" out\n+\tgrep -E \"^A-8-g[0-9a-f]+$\" out\n '\n \n-check_describe \"A-*[0-9a-f]\" --dirty\n+check_describe \"A-8-gHASH\" --dirty\n \n test_expect_success 'describe --dirty with --work-tree' '\n \t(\n \t\tcd \"$TEST_DIRECTORY\" &&\n \t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty >\"$TRASH_DIRECTORY/out\"\n \t) &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+$\" out\n+\tgrep -E \"^A-8-g[0-9a-f]+$\" out\n '\n \n test_expect_success 'set-up dirty work tree' '\n@@ -189,7 +187,7 @@ test_expect_success 'describe --dirty with --work-tree (dirty)' '\n \t\tcd \"$TEST_DIRECTORY\" &&\n \t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty >\"$TRASH_DIRECTORY/out\"\n \t) &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+-dirty$\" out &&\n+\tgrep -E \"^A-8-g[0-9a-f]+-dirty$\" out &&\n \ttest_cmp expected out\n '\n \n@@ -199,7 +197,7 @@ test_expect_success 'describe --dirty=.mod with --work-tree (dirty)' '\n \t\tcd \"$TEST_DIRECTORY\" &&\n \t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty=.mod >\"$TRASH_DIRECTORY/out\"\n \t) &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+.mod$\" out &&\n+\tgrep -E \"^A-8-g[0-9a-f]+.mod$\" out &&\n \ttest_cmp expected out\n '\n \n@@ -223,21 +221,21 @@ test_expect_success 'set-up matching pattern tests' '\n \n '\n \n-check_describe \"test-annotated-*\" --match=\"test-*\"\n+check_describe \"test-annotated-3-gHASH\" --match=\"test-*\"\n \n-check_describe \"test1-lightweight-*\" --tags --match=\"test1-*\"\n+check_describe \"test1-lightweight-2-gHASH\" --tags --match=\"test1-*\"\n \n-check_describe \"test2-lightweight-*\" --tags --match=\"test2-*\"\n+check_describe \"test2-lightweight-1-gHASH\" --tags --match=\"test2-*\"\n \n-check_describe \"test2-lightweight-*\" --long --tags --match=\"test2-*\" HEAD^\n+check_describe \"test2-lightweight-0-gHASH\" --long --tags --match=\"test2-*\" HEAD^\n \n-check_describe \"test2-lightweight-*\" --long --tags --match=\"test1-*\" --match=\"test2-*\" HEAD^\n+check_describe \"test2-lightweight-0-gHASH\" --long --tags --match=\"test1-*\" --match=\"test2-*\" HEAD^\n \n-check_describe \"test2-lightweight-*\" --long --tags --match=\"test1-*\" --no-match --match=\"test2-*\" HEAD^\n+check_describe \"test2-lightweight-0-gHASH\" --long --tags --match=\"test1-*\" --no-match --match=\"test2-*\" HEAD^\n \n-check_describe \"test1-lightweight-*\" --long --tags --match=\"test1-*\" --match=\"test3-*\" HEAD\n+check_describe \"test1-lightweight-2-gHASH\" --long --tags --match=\"test1-*\" --match=\"test3-*\" HEAD\n \n-check_describe \"test1-lightweight-*\" --long --tags --match=\"test3-*\" --match=\"test1-*\" HEAD\n+check_describe \"test1-lightweight-2-gHASH\" --long --tags --match=\"test3-*\" --match=\"test1-*\" HEAD\n \n test_expect_success 'set-up branches' '\n \tgit branch branch_A A &&\n@@ -247,11 +245,11 @@ test_expect_success 'set-up branches' '\n \tgit update-ref refs/original/original_branch_A test-annotated~2\n '\n \n-check_describe \"heads/branch_A*\" --all --match=\"branch_*\" --exclude=\"branch_C\" HEAD\n+check_describe \"heads/branch_A-11-gHASH\" --all --match=\"branch_*\" --exclude=\"branch_C\" HEAD\n \n-check_describe \"remotes/origin/remote_branch_A*\" --all --match=\"origin/remote_branch_*\" --exclude=\"origin/remote_branch_C\" HEAD\n+check_describe \"remotes/origin/remote_branch_A-11-gHASH\" --all --match=\"origin/remote_branch_*\" --exclude=\"origin/remote_branch_C\" HEAD\n \n-check_describe \"original/original_branch_A*\" --all test-annotated~1\n+check_describe \"original/original_branch_A-6-gHASH\" --all test-annotated~1\n \n test_expect_success '--match does not work for other types' '\n \ttest_must_fail git describe --all --match=\"*original_branch_*\" test-annotated~1\n@@ -521,7 +519,7 @@ test_expect_success 'describe commits with disjoint bases' '\n \t\tgit tag B -a -m B &&\n \t\tgit merge --no-ff --allow-unrelated-histories main -m x &&\n \n-\t\tcheck_describe \"A-3-*\" HEAD\n+\t\tcheck_describe \"A-3-gHASH\" HEAD\n \t)\n '\n \n@@ -547,7 +545,7 @@ test_expect_success 'describe commits with disjoint bases 2' '\n \t\tgit tag B -a -m B &&\n \t\tgit merge --no-ff --allow-unrelated-histories main -m x &&\n \n-\t\tcheck_describe \"B-3-*\" HEAD\n+\t\tcheck_describe \"B-3-gHASH\" HEAD\n \t)\n '\n \n-- \n2.31.0.rc0.116.g45ec00aa00\n\n"},{"id":"417988","messageId":"20210228195414.21372-5-avarab@gmail.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"[PATCH 04/10] test-lib functions: add an --annotated-tag option to \"test_commit\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-28T19:54:08Z","receivedAt":"2021-02-28T19:55:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add an --annotated-tag option to test_commit. The tag will share the\nsame message as the commit, and we'll call test_tick before creating\nit (unless --notick) is provided.\n\nThere's quite a few tests that could be simplified with this\nconstruct. I've picked one to convert in this change as a\ndemonstration.\n\nThe placement of --annotated-tag after \"notick\" in the case of the\ndocumentation, and then after \"no_tag\" in the case of the code is\nslightly inconsistent. It's to evade a merge conflict with two other\ncommits adding a --printf option, and another one adding documentation\nfor --no-tag.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1403-show-ref.sh | 6 ++----\n t/test-lib-functions.sh | 18 +++++++++++++++++-\n 2 files changed, 19 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh\nindex 6ce62f878c..7c873033e9 100755\n--- a/t/t1403-show-ref.sh\n+++ b/t/t1403-show-ref.sh\n@@ -7,11 +7,9 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n \n test_expect_success setup '\n-\ttest_commit A &&\n-\tgit tag -f -a -m \"annotated A\" A &&\n+\ttest_commit --annotated-tag A &&\n \tgit checkout -b side &&\n-\ttest_commit B &&\n-\tgit tag -f -a -m \"annotated B\" B &&\n+\ttest_commit --annotated-tag B &&\n \tgit checkout main &&\n \ttest_commit C &&\n \tgit branch B A^0\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 6348e8d733..c6cdabf53e 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -171,6 +171,10 @@ debug () {\n #\tRun all git commands in directory \n # --notick\n #\tDo not call test_tick before making a commit\n+# --annotated-tag\n+#\tCreate an annotated tag with \"-a -m \". Calls\n+#\ttest_tick between making the commit and tag unless --notick is\n+#\tgiven.\n # --append\n #\tUse \"echo >>\" instead of \"echo >\" when writing \"\" to\n #\t\"\"\n@@ -191,6 +195,7 @@ test_commit () {\n \tsignoff= &&\n \tindir= &&\n \tno_tag= &&\n+\tannotated_tag= &&\n \twhile test $# != 0\n \tdo\n \t\tcase \"$1\" in\n@@ -220,6 +225,9 @@ test_commit () {\n \t\t--no-tag)\n \t\t\tno_tag=yes\n \t\t\t;;\n+\t\t--annotated-tag)\n+\t\t\tannotated_tag=yes\n+\t\t\t;;\n \t\t*)\n \t\t\tbreak\n \t\t\t;;\n@@ -244,7 +252,15 @@ test_commit () {\n \t $signoff -m \"$1\" &&\n \tif test -z \"$no_tag\"\n \tthen\n-\t\tgit ${indir:+ -C \"$indir\"} tag \"${4:-$1}\"\n+\t\tif test -n \"$annotated_tag\"\n+\t\tthen\n+\t\t\tif test -z \"$notick\"\n+\t\t\tthen\n+\t\t\t\ttest_tick\n+\t\t\tfi &&\n+\t\t\ttest_tick\n+\t\tfi &&\n+\t\tgit ${indir:+ -C \"$indir\"} tag ${annotated_tag:+ -a -m \"$1\"} \"${4:-$1}\"\n \tfi\n }\n \n-- \n2.31.0.rc0.116.g45ec00aa00\n\n"},{"id":"417989","messageId":"20210228195414.21372-4-avarab@gmail.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"[PATCH 03/10] describe tests: always assert empty stderr from \"describe\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-28T19:54:07Z","receivedAt":"2021-02-28T19:55:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Invert a test added in 3291fe4072e (Add git-describe test for \"verify\nannotated tag names on output\", 2008-03-03) to make checking that we\ndon't have warnings the rule rather than the exception.\n\nThere was only one case where we expected and got a warning. Let's\ntest for that case explicitly, and assert no warnings or other stderr\noutput for all the rest.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6120-describe.sh | 26 ++++++++++++--------------\n 1 file changed, 12 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex e4fd5d567f..ef70c695be 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -21,7 +21,8 @@ check_describe () {\n \tshift\n \tdescribe_opts=\"$@\"\n \ttest_expect_success \"describe $describe_opts\" '\n-\t\tgit describe $describe_opts 2>err.actual >raw &&\n+\t\tgit describe $describe_opts 2>err >raw &&\n+\t\ttest_must_be_empty err &&\n \t\tsed -e \"s/-g[0-9a-f]*\\$/-gHASH/\" actual &&\n \t\techo $expect >expect &&\n \t\ttest_cmp expect actual\n@@ -122,20 +123,17 @@ test_expect_success 'describe --contains defaults to HEAD without commit-ish' '\n '\n \n check_describe tags/A --all A^0\n-test_expect_success 'no warning was displayed for A' '\n-\ttest_must_be_empty err.actual\n-'\n \n-test_expect_success 'rename tag A to Q locally' '\n-\tmv .git/refs/tags/A .git/refs/tags/Q\n-'\n-cat - >err.expect <actual >out &&\n+\tcat >expected <<-\\EOF &&\n+\twarning: tag 'Q' is externally known as 'A'\n+\tEOF\n+\ttest_cmp expected actual &&\n+\tgrep -E '^A-8-g[0-9a-f]+$' out\n+\"\n+\n test_expect_success 'misnamed annotated tag forces long output' '\n \tdescription=$(git describe --no-long Q^0) &&\n \texpr \"$description\" : \"A-0-g[0-9a-f]*$\" &&\n-- \n2.31.0.rc0.116.g45ec00aa00\n\n"},{"id":"417990","messageId":"20210228195414.21372-6-avarab@gmail.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"[PATCH 05/10] describe tests: convert setup to use test_commit","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-28T19:54:09Z","receivedAt":"2021-02-28T19:56:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Convert the setup of the describe tests to use test_commit when\npossible. This makes use of the new --annotated-tag option to\ntest_commit.\n\nSome of the setup here could simply be removed since the data being\ncreated wasn't important to any of the subsequent tests, so I've done\nso. E.g. assigning to the \"one\" variable was always useless, and just\nchecking that we can describe HEAD after the first commit wasn't\nuseful.\n\nIn the case of the \"two\" variable we could instead use the tag we just\ncreated. See 5312ab11fbf (Add describe test., 2007-01-13) for the\ninitial version of this code. There's other cases here like redundant\n\"test_tick\" invocations, or the simplification of not echoing \"X\" to a\nfile we're about to tag as \"x\", now we just use \"x\" in both cases.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6120-describe.sh | 58 ++++++++++-----------------------------------\n 1 file changed, 13 insertions(+), 45 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex ef70c695be..8dc76f8e9e 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -30,64 +30,32 @@ check_describe () {\n }\n \n test_expect_success setup '\n+\ttest_commit initial file one &&\n+\ttest_commit second file two &&\n+\ttest_commit third file three &&\n+\ttest_commit --annotated-tag A file A &&\n+\ttest_commit c file c &&\n \n-\ttest_tick &&\n-\techo one >file && git add file && git commit -m initial &&\n-\tone=$(git rev-parse HEAD) &&\n-\n-\tgit describe --always HEAD &&\n-\n-\ttest_tick &&\n-\techo two >file && git add file && git commit -m second &&\n-\ttwo=$(git rev-parse HEAD) &&\n-\n-\ttest_tick &&\n-\techo three >file && git add file && git commit -m third &&\n-\n-\ttest_tick &&\n-\techo A >file && git add file && git commit -m A &&\n-\ttest_tick &&\n-\tgit tag -a -m A A &&\n-\n-\ttest_tick &&\n-\techo c >file && git add file && git commit -m c &&\n-\ttest_tick &&\n-\tgit tag c &&\n-\n-\tgit reset --hard $two &&\n-\ttest_tick &&\n-\techo B >side && git add side && git commit -m B &&\n-\ttest_tick &&\n-\tgit tag -a -m B B &&\n+\tgit reset --hard second &&\n+\ttest_commit --annotated-tag B side B &&\n \n \ttest_tick &&\n \tgit merge -m Merged c &&\n \tmerged=$(git rev-parse HEAD) &&\n \n-\tgit reset --hard $two &&\n-\ttest_tick &&\n-\techo D >another && git add another && git commit -m D &&\n-\ttest_tick &&\n-\tgit tag -a -m D D &&\n-\ttest_tick &&\n-\tgit tag -a -m R R &&\n-\n-\ttest_tick &&\n-\techo DD >another && git commit -a -m another &&\n+\tgit reset --hard second &&\n+\ttest_commit --no-tag D another D &&\n \n \ttest_tick &&\n-\tgit tag e &&\n+\tgit tag -a -m R R &&\n \n-\ttest_tick &&\n-\techo DDD >another && git commit -a -m \"yet another\" &&\n+\ttest_commit e another DD &&\n+\ttest_commit --no-tag \"yet another\" another DDD &&\n \n \ttest_tick &&\n \tgit merge -m Merged $merged &&\n \n-\ttest_tick &&\n-\techo X >file && echo X >side && git add file side &&\n-\tgit commit -m x\n-\n+\ttest_commit --no-tag x file\n '\n \n check_describe A-8-gHASH HEAD\n-- \n2.31.0.rc0.116.g45ec00aa00\n\n"},{"id":"417991","messageId":"20210228195414.21372-7-avarab@gmail.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"[PATCH 06/10] describe tests: fix nested \"test_expect_success\" call","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-28T19:54:10Z","receivedAt":"2021-02-28T19:56:00Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a nested invocation of \"test_expect_success\", the\n\"check_describe()\" function is a wrapper for calling\ntest_expect_success, and therefore needs to be called outside the body\nof another \"test_expect_success\".\n\nThe two tests added in 30b1c7ad9d6 (describe: don't abort too early\nwhen searching tags, 2020-02-26) were not testing for anything due to\nthis logic error. Without this fix reverting the C code changes in\nthat commit still has all tests passing, with this fix we're actually\ntesting the \"describe\" output. This is because \"test_expect_success\"\ncalls \"test_finish_\", whose last statement happens to be true.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6120-describe.sh | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 8dc76f8e9e..ae801c740b 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -470,7 +470,7 @@ test_expect_success 'name-rev covers all conditions while looking at parents' '\n # o-----o---o----x\n # A\n #\n-test_expect_success 'describe commits with disjoint bases' '\n+test_expect_success 'setup: describe commits with disjoint bases' '\n \tgit init disjoint1 &&\n \t(\n \t\tcd disjoint1 &&\n@@ -483,19 +483,22 @@ test_expect_success 'describe commits with disjoint bases' '\n \t\tgit checkout --orphan branch && rm file &&\n \t\techo B > file2 && git add file2 && git commit -m B &&\n \t\tgit tag B -a -m B &&\n-\t\tgit merge --no-ff --allow-unrelated-histories main -m x &&\n-\n-\t\tcheck_describe \"A-3-gHASH\" HEAD\n+\t\tgit merge --no-ff --allow-unrelated-histories main -m x\n \t)\n '\n \n+(\n+\tcd disjoint1 &&\n+\tcheck_describe \"A-3-gHASH\" HEAD\n+)\n+\n # B\n # o---o---o------------.\n # \\\n # o---o---x\n # A\n #\n-test_expect_success 'describe commits with disjoint bases 2' '\n+test_expect_success 'setup: describe commits with disjoint bases 2' '\n \tgit init disjoint2 &&\n \t(\n \t\tcd disjoint2 &&\n@@ -509,10 +512,13 @@ test_expect_success 'describe commits with disjoint bases 2' '\n \t\techo o >> file2 && git add file2 && GIT_COMMITTER_DATE=\"2020-01-01 15:01\" git commit -m o &&\n \t\techo B >> file2 && git add file2 && GIT_COMMITTER_DATE=\"2020-01-01 15:02\" git commit -m B &&\n \t\tgit tag B -a -m B &&\n-\t\tgit merge --no-ff --allow-unrelated-histories main -m x &&\n-\n-\t\tcheck_describe \"B-3-gHASH\" HEAD\n+\t\tgit merge --no-ff --allow-unrelated-histories main -m x\n \t)\n '\n \n+(\n+\tcd disjoint2 &&\n+\tcheck_describe \"B-3-gHASH\" HEAD\n+)\n+\n test_done\n-- \n2.31.0.rc0.116.g45ec00aa00\n\n"},{"id":"417992","messageId":"20210228195414.21372-9-avarab@gmail.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"[PATCH 08/10] svn tests: remove legacy re-setup from init-clone test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-28T19:54:12Z","receivedAt":"2021-02-28T19:56:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the immediate \"rm -rf .git\" from the start of this test. This\nwas added back in 41337e22f0 (git-svn: add tests for command-line\nusage of init and clone commands, 2007-11-17) when there was a \"trash\"\ndirectory shared by all the tests, but ever since abc5d372ec (Enable\nparallel tests, 2008-08-08) we've had per-test trash directories.\n\nSo this setup can simply be removed. We could use\nTEST_NO_CREATE_REPO=true, but I don't think it's worth the effort to\ngo out of our way to be different. It doesn't matter that we now have\na redundant .git at the top-level.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t9117-git-svn-init-clone.sh | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh\nindex 044f65e916..62de819a44 100755\n--- a/t/t9117-git-svn-init-clone.sh\n+++ b/t/t9117-git-svn-init-clone.sh\n@@ -7,12 +7,6 @@ test_description='git svn init/clone tests'\n \n . ./lib-git-svn.sh\n \n-# setup, run inside tmp so we don't have any conflicts with $svnrepo\n-set -e\n-rm -r .git\n-mkdir tmp\n-cd tmp\n-\n test_expect_success 'setup svnrepo' '\n \tmkdir project project/trunk project/branches project/tags &&\n \techo foo > project/trunk/foo &&\n-- \n2.31.0.rc0.116.g45ec00aa00\n\n"},{"id":"417993","messageId":"20210228195414.21372-8-avarab@gmail.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"[PATCH 07/10] describe tests: support -C in \"check_describe\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-28T19:54:11Z","receivedAt":"2021-02-28T19:56:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change a subshell added in a preceding commit to instead use a new\n\"-C\" option to \"check_describe\". The idiom for this is copied as-is\nfrom the \"test_commit\" function in test-lib-functions.sh\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6120-describe.sh | 27 ++++++++++++++++++---------\n 1 file changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex ae801c740b..e48c4f604b 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -17,11 +17,26 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n \n check_describe () {\n+\tindir= &&\n+\twhile test $# != 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-C)\n+\t\t\tindir=\"$2\"\n+\t\t\tshift\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone &&\n+\tindir=${indir:+\"$indir\"/} &&\n \texpect=\"$1\"\n \tshift\n \tdescribe_opts=\"$@\"\n \ttest_expect_success \"describe $describe_opts\" '\n-\t\tgit describe $describe_opts 2>err >raw &&\n+\t\tgit ${indir:+ -C \"$indir\"} describe $describe_opts 2>err >raw &&\n \t\ttest_must_be_empty err &&\n \t\tsed -e \"s/-g[0-9a-f]*\\$/-gHASH/\" actual &&\n \t\techo $expect >expect &&\n@@ -487,10 +502,7 @@ test_expect_success 'setup: describe commits with disjoint bases' '\n \t)\n '\n \n-(\n-\tcd disjoint1 &&\n-\tcheck_describe \"A-3-gHASH\" HEAD\n-)\n+check_describe -C disjoint1 \"A-3-gHASH\" HEAD\n \n # B\n # o---o---o------------.\n@@ -516,9 +528,6 @@ test_expect_success 'setup: describe commits with disjoint bases 2' '\n \t)\n '\n \n-(\n-\tcd disjoint2 &&\n-\tcheck_describe \"B-3-gHASH\" HEAD\n-)\n+check_describe -C disjoint2 \"B-3-gHASH\" HEAD\n \n test_done\n-- \n2.31.0.rc0.116.g45ec00aa00\n\n"},{"id":"417994","messageId":"20210228195414.21372-10-avarab@gmail.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"[PATCH 09/10] svn tests: refactor away a \"set -e\" in test body","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-28T19:54:13Z","receivedAt":"2021-02-28T19:56:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor a test added in 83c9433e67 (git-svn: support for git-svn\npropset, 2014-12-07) to avoid using \"set -e\" in the test body. This\nwould have broken in combination with a subsequent change to make\n\"test_expect_success\" return 1 to catch such cases.\n\nWhile I'm at it rewrite the test to conform to a modern style in our\ntests, using the \"test_when_finished\" function for the \"rm -rf\", and\navoiding repeated \"mkdir\" in favor of \"mkdir -p\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t9148-git-svn-propset.sh | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t9148-git-svn-propset.sh b/t/t9148-git-svn-propset.sh\nindex 102639090c..2b55e76be6 100755\n--- a/t/t9148-git-svn-propset.sh\n+++ b/t/t9148-git-svn-propset.sh\n@@ -7,19 +7,22 @@ test_description='git svn propset tests'\n \n . ./lib-git-svn.sh\n \n-foo_subdir2=\"subdir/subdir2/foo_subdir2\"\n+test_expect_success 'setup propset via import' '\n+\ttest_when_finished \"rm -rf import\" &&\n \n-set -e\n-mkdir import &&\n-(set -e ; cd import\n-\tmkdir subdir\n-\tmkdir subdir/subdir2\n-\ttouch foo \t\t# for 'add props top level'\n-\ttouch subdir/foo_subdir # for 'add props relative'\n-\ttouch \"$foo_subdir2\"\t# for 'add props subdir'\n-\tsvn_cmd import -m 'import for git svn' . \"$svnrepo\" >/dev/null\n-)\n-rm -rf import\n+\tfoo_subdir2=\"subdir/subdir2/foo_subdir2\" &&\n+\tmkdir -p import/subdir/subdir2 &&\n+\t(\n+\t\tcd import &&\n+\t\t# for \"add props top level\"\n+\t\ttouch foo &&\n+\t\t# for \"add props relative\"\n+\t\ttouch subdir/foo_subdir &&\n+\t\t# for \"add props subdir\"\n+\t\ttouch \"$foo_subdir2\" &&\n+\t\tsvn_cmd import -m \"import for git svn\" . \"$svnrepo\"\n+\t)\n+'\n \n test_expect_success 'initialize git svn' '\n \tgit svn init \"$svnrepo\"\n-- \n2.31.0.rc0.116.g45ec00aa00\n\n"},{"id":"417995","messageId":"20210228195414.21372-11-avarab@gmail.com","threadId":"52871","inReplyTo":"20200223125102.6697-1-benno@bmevers.de","subject":"[PATCH 10/10] test-lib: return 1 from test_expect_{success,failure}","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-28T19:54:14Z","receivedAt":"2021-02-28T19:56:14Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"An earlier commit fixed an issue in \"check_describe\" with\n\"test_expect_success\" being called within another\n\"test_expect_success\", causing the test to succeed even if it should\nfail.\n\nLet's try to guard against this in the test library by returning 1\nfrom these two functions. This change would have caught the issue I've\nnow fixed in the \"check_describe\" function.\n\nI could equivalently add this \"return 1\" to the \"test_finish_\"\nfunction itself, but I think doing it here is more readable.\n\nBecause of this change any tests which ran under \"set -e\" needed to be\nrefactored not to use \"set -e\". Luckily there were only two such\ntests, earlier commits did that refactoring.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/test-lib-functions.sh | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex c6cdabf53e..3dd68091bb 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -636,6 +636,7 @@ test_expect_failure () {\n \t\tfi\n \tfi\n \ttest_finish_\n+\treturn 1\n }\n \n test_expect_success () {\n@@ -656,6 +657,7 @@ test_expect_success () {\n \t\tfi\n \tfi\n \ttest_finish_\n+\treturn 1\n }\n \n # test_external runs external test scripts that provide continuous\n-- \n2.31.0.rc0.116.g45ec00aa00\n\n"},{"id":"417999","messageId":"20210228211407.GA3214@dcvr","threadId":"52871","inReplyTo":"20210228195414.21372-10-avarab@gmail.com","subject":"Re: [PATCH 09/10] svn tests: refactor away a \"set -e\" in test body","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2021-02-28T21:14:07Z","receivedAt":"2021-02-28T21:15:04Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Ævar Arnfjörð Bjarmason wrote:\n> Refactor a test added in 83c9433e67 (git-svn: support for git-svn\n> propset, 2014-12-07) to avoid using \"set -e\" in the test body. This\n> would have broken in combination with a subsequent change to make\n> \"test_expect_success\" return 1 to catch such cases.\n> \n> While I'm at it rewrite the test to conform to a modern style in our\n> tests, using the \"test_when_finished\" function for the \"rm -rf\", and\n> avoiding repeated \"mkdir\" in favor of \"mkdir -p\".\n\nThank you for working on these (and sorry for making a mess all\nthose years ago!).\n\nI had it in my mind that \"mkdir -p\" wasn't portable, but systems\nwithout it probably haven't been relevant since pre-git times.\n\n> --- a/t/t9148-git-svn-propset.sh\n> +++ b/t/t9148-git-svn-propset.sh\n> @@ -7,19 +7,22 @@ test_description='git svn propset tests'\n> \n> . ./lib-git-svn.sh\n> \n> -foo_subdir2=\"subdir/subdir2/foo_subdir2\"\n> +test_expect_success 'setup propset via import' '\n> +\ttest_when_finished \"rm -rf import\" &&\n> \n> -set -e\n> -mkdir import &&\n> -(set -e ; cd import\n> -\tmkdir subdir\n> -\tmkdir subdir/subdir2\n> -\ttouch foo \t\t# for 'add props top level'\n> -\ttouch subdir/foo_subdir # for 'add props relative'\n> -\ttouch \"$foo_subdir2\"\t# for 'add props subdir'\n> -\tsvn_cmd import -m 'import for git svn' . \"$svnrepo\" >/dev/null\n> -)\n> -rm -rf import\n> +\tfoo_subdir2=\"subdir/subdir2/foo_subdir2\" &&\n> +\tmkdir -p import/subdir/subdir2 &&\n> +\t(\n> +\t\tcd import &&\n> +\t\t# for \"add props top level\"\n> +\t\ttouch foo &&\n> +\t\t# for \"add props relative\"\n> +\t\ttouch subdir/foo_subdir &&\n> +\t\t# for \"add props subdir\"\n> +\t\ttouch \"$foo_subdir2\" &&\n> +\t\tsvn_cmd import -m \"import for git svn\" . \"$svnrepo\"\n\nI've noticed '>' could be used instead of touch, but that's\nprobably better as another patch :>\n"},{"id":"418069","messageId":"xmqq1rcypnua.fsf@gitster.c.googlers.com","threadId":"52871","inReplyTo":"20210228195414.21372-3-avarab@gmail.com","subject":"Re: [PATCH 02/10] describe tests: refactor away from glob matching","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T21:26:37Z","receivedAt":"2021-03-01T21:30:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the glob matching via a \"case\" statement to a \"test_cmp\" after\n> we've stripped out the hash-specific g\n> suffix. 5312ab11fbf (Add describe test., 2007-01-13).\n>\n> This means that we can use test_cmp to compare the output. I could\n> omit the \"-8\" change of e.g. \"A-*\" to \"A-8-gHASH\", but I think it\n> makes sense to test that here explicitly. It means you need to add new\n> tests to the bottom of the file, but that's not a burden in this case.\n\nI think the point in these tests are that they consider \"which tip\nthe tested commit is the closest\" is the only piece of information\nthat matter, and allows the numbers (\"number of commits on top of\")\nto be redefined in the future without having to change the tests,\nbut I tend to agree that such a change should be accompanied by and\ndocumented with changes to these tests. \n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t6120-describe.sh | 78 ++++++++++++++++++++++-----------------------\n> 1 file changed, 38 insertions(+), 40 deletions(-)\n>\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index 7bc2aaa46e..e4fd5d567f 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -21,12 +21,10 @@ check_describe () {\n> \tshift\n> \tdescribe_opts=\"$@\"\n\nJust a style thing, when we are not invoking the \"each positional\narg is double-quoted individually against being split at $IFS\" magic,\nwe prefer to spell this as \"$*\".\n\n> \ttest_expect_success \"describe $describe_opts\" '\n> +\t\tgit describe $describe_opts 2>err.actual >raw &&\n> +\t\tsed -e \"s/-g[0-9a-f]*\\$/-gHASH/\" actual &&\n\nThe exact ones would be passed as-is (i.e. \"test_cmp raw actual\"\ncould pass), which is what we want anyway.\n\nIf we are planning to further extend this helper to make it more\ncapable, we might want to consider quoting $describe to be evaled\nproperly so that we can do an equivalent of\n\n\tcheck_describe A-8-gHASH --dirty='.d i r t y' HEAD\n\nwhen we gain new option that is more intresting than --dirty=\nthat legitimately would benefit from being able to pass arguments\nwith $IFS whitespace in them.\n\nBut that is outside the scope of this step. I haven't seen the\noverall topic yet, so it may or may not be within the scope of this\nseries. We'll see.\n\n> +\t\techo $expect >expect &&\n\nLet's double-quote to relieve readers from having to wonder if you\nare expecting the callers to feed crazy things like \"a b\" and this\necho to normalize it to \"a b\".\n\n> +\t\ttest_cmp expect actual\n> \t'\n> }\n"},{"id":"418070","messageId":"xmqqwnuqo8ze.fsf@gitster.c.googlers.com","threadId":"52871","inReplyTo":"20210228195414.21372-4-avarab@gmail.com","subject":"Re: [PATCH 03/10] describe tests: always assert empty stderr from \"describe\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T21:32:53Z","receivedAt":"2021-03-01T21:37:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Invert a test added in 3291fe4072e (Add git-describe test for \"verify\n> annotated tag names on output\", 2008-03-03) to make checking that we\n> don't have warnings the rule rather than the exception.\n>\n> There was only one case where we expected and got a warning. Let's\n> test for that case explicitly, and assert no warnings or other stderr\n> output for all the rest.\n\nWhen we are expecting an error from \"describe\", we would want to\nmake sure that we will see an expected explanation of the failure\n(e.g. \"you passed a non-commit\") in the standard error stream, but I\nam somewhat skeptical about the value of a change like this that\ninsists that there is nothing on the standard error stream when the\ncommand succeeds.\n\nIt is unlikely to trigger auto GC during the operation of \"git\ndescribe\" and seeing some output in the standard error stream, but\nit is easy to imagine that we may add an \"automatically cache\nprecomputed topology information\" feature and trigger during a\nhistory traversal operation like this one, with some note to the\nstandard error output.\n\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t6120-describe.sh | 26 ++++++++++++--------------\n> 1 file changed, 12 insertions(+), 14 deletions(-)\n>\n> diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\n> index e4fd5d567f..ef70c695be 100755\n> --- a/t/t6120-describe.sh\n> +++ b/t/t6120-describe.sh\n> @@ -21,7 +21,8 @@ check_describe () {\n> \tshift\n> \tdescribe_opts=\"$@\"\n> \ttest_expect_success \"describe $describe_opts\" '\n> -\t\tgit describe $describe_opts 2>err.actual >raw &&\n> +\t\tgit describe $describe_opts 2>err >raw &&\n> +\t\ttest_must_be_empty err &&\n> \t\tsed -e \"s/-g[0-9a-f]*\\$/-gHASH/\" actual &&\n> \t\techo $expect >expect &&\n> \t\ttest_cmp expect actual\n> @@ -122,20 +123,17 @@ test_expect_success 'describe --contains defaults to HEAD without commit-ish' '\n> '\n> \n> check_describe tags/A --all A^0\n> -test_expect_success 'no warning was displayed for A' '\n> -\ttest_must_be_empty err.actual\n> -'\n> \n> -test_expect_success 'rename tag A to Q locally' '\n> -\tmv .git/refs/tags/A .git/refs/tags/Q\n> -'\n> -cat - >err.expect < -warning: tag 'Q' is externally known as 'A'\n> -EOF\n> -check_describe A-8-gHASH HEAD\n> -test_expect_success 'warning was displayed for Q' '\n> -\ttest_cmp err.expect err.actual\n> -'\n> +test_expect_success 'renaming tag A to Q locally produces a warning' \"\n> +\tmv .git/refs/tags/A .git/refs/tags/Q &&\n> +\tgit describe HEAD 2>actual >out &&\n> +\tcat >expected <<-\\EOF &&\n> +\twarning: tag 'Q' is externally known as 'A'\n> +\tEOF\n> +\ttest_cmp expected actual &&\n> +\tgrep -E '^A-8-g[0-9a-f]+$' out\n> +\"\n> +\n> test_expect_success 'misnamed annotated tag forces long output' '\n> \tdescription=$(git describe --no-long Q^0) &&\n> \texpr \"$description\" : \"A-0-g[0-9a-f]*$\" &&\n"},{"id":"418071","messageId":"xmqqr1kyo8l8.fsf@gitster.c.googlers.com","threadId":"52871","inReplyTo":"20210228195414.21372-5-avarab@gmail.com","subject":"Re: [PATCH 04/10] test-lib functions: add an --annotated-tag option to \"test_commit\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T21:41:23Z","receivedAt":"2021-03-01T21:46:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> @@ -220,6 +225,9 @@ test_commit () {\n> \t\t--no-tag)\n> \t\t\tno_tag=yes\n> \t\t\t;;\n> +\t\t--annotated-tag)\n> +\t\t\tannotated_tag=yes\n> +\t\t\t;;\n\nA new option is welcome, but shouldn't we be straightening out the\nvariables that keep track of the options around tagging? It's not\nlike it is possible to have 4 (two variables that can independently\nset to 'yes') possibilities, so something along the lines of ...\n\n test_commit () {\n +\ttag=light &&\n notick= &&\n ...\n\t\twhile test $# != 0\n\t\tdo\n\t\t\tcase \"$1\" in\n\t\t\t...\n\t\t\t--no-tag)\n\t-\t\t\tno_tag=yes\n\t+\t\t\ttag=none\n\t\t\t\t;;\n\t+\t\t--annotated)\n\t+\t\t\ttag=annotated\n\t+\t\t\t;;\n\t\t...\n\t-\tif test -z \"$no_tag\"\n\t-\tthen\n\t+\tcase \"$tag\" in\n\t+\tnone)\n\t+\t\t;;\n\t+\tlight)\n\t\t\tgit ${indir:+ -C \"$indir\") tag \"${4:-$1}\"\n\t+\t\t;;\n\t+\tannotated)\n\t+\t\tgit ${indir:+ -C \"$indir\") tag -m \"$1\" \"${4:-$1}\"\n\t+\t\t;;\n\t+\tesac\n\t\t...\n\nafter this step (meaning, we may want to start from fixing the no_tag=yes\nto tag=none before introducing this new feature).\n\nThanks.\n\n> @@ -244,7 +252,15 @@ test_commit () {\n> \t $signoff -m \"$1\" &&\n> \tif test -z \"$no_tag\"\n> \tthen\n> -\t\tgit ${indir:+ -C \"$indir\"} tag \"${4:-$1}\"\n> +\t\tif test -n \"$annotated_tag\"\n> +\t\tthen\n> +\t\t\tif test -z \"$notick\"\n> +\t\t\tthen\n> +\t\t\t\ttest_tick\n> +\t\t\tfi &&\n> +\t\t\ttest_tick\n> +\t\tfi &&\n> +\t\tgit ${indir:+ -C \"$indir\"} tag ${annotated_tag:+ -a -m \"$1\"} \"${4:-$1}\"\n> \tfi\n> }\n"},{"id":"418073","messageId":"xmqqmtvmo8il.fsf@gitster.c.googlers.com","threadId":"52871","inReplyTo":"20210228195414.21372-6-avarab@gmail.com","subject":"Re: [PATCH 05/10] describe tests: convert setup to use test_commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T21:42:58Z","receivedAt":"2021-03-01T21:47:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Convert the setup of the describe tests to use test_commit when\n> possible. This makes use of the new --annotated-tag option to\n> test_commit.\n\nMuch nicer ;-)\n\n> t/t6120-describe.sh | 58 ++++++++++-----------------------------------\n> 1 file changed, 13 insertions(+), 45 deletions(-)\n"},{"id":"418074","messageId":"xmqqft1eo8hs.fsf@gitster.c.googlers.com","threadId":"52871","inReplyTo":"20210228195414.21372-11-avarab@gmail.com","subject":"Re: [PATCH 10/10] test-lib: return 1 from test_expect_{success,failure}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-01T21:43:27Z","receivedAt":"2021-03-01T21:47:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index c6cdabf53e..3dd68091bb 100644\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -636,6 +636,7 @@ test_expect_failure () {\n> \t\tfi\n> \tfi\n> \ttest_finish_\n> +\treturn 1\n> }\n> \n> test_expect_success () {\n> @@ -656,6 +657,7 @@ test_expect_success () {\n> \t\tfi\n> \tfi\n> \ttest_finish_\n> +\treturn 1\n> }\n> \n> # test_external runs external test scripts that provide continuous\n\nHmph.\n\nThis does not catch if the outer expect_success does not catch a\nfailure in the inner expect_success and signal a failure.\n\nWhen I asked if this kind of breakage is an easy mistake to catch by\nthe test lint, I had something along this in mind:\n\n\ttest_expect_success () {\n\t\tif test -n \"$GIT_IN_TEST_EXPECT\"\n\t\tthen\n\t\t\tBUG caling \"$GIT_IN_TEST_EXPECT\" inside test_expect_success\n\t\tfi\n\t\tGIT_IN_TEST_EXPECT=test_expect_success\n\n\t\t... do the 'eval the given test body' thing ..\n\n\t\tGIT_IN_TEST_EXPECT=\n\t}\n\nAfter all, the error is in the outer expect_success in that it\ncalled another one, so it feels more natural that the called inner\nexpect_success to notice the situation and barf.\n\nThanks.\n"},{"id":"418103","messageId":"87czwhkif6.fsf@evledraar.gmail.com","threadId":"52871","inReplyTo":"xmqqr1kyo8l8.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 04/10] test-lib functions: add an --annotated-tag option to \"test_commit\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-03-02T09:34:53Z","receivedAt":"2021-03-02T15:28:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Mar 01 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> @@ -220,6 +225,9 @@ test_commit () {\n>> \t\t--no-tag)\n>> \t\t\tno_tag=yes\n>> \t\t\t;;\n>> +\t\t--annotated-tag)\n>> +\t\t\tannotated_tag=yes\n>> +\t\t\t;;\n>\n> A new option is welcome, but shouldn't we be straightening out the\n> variables that keep track of the options around tagging? It's not\n> like it is possible to have 4 (two variables that can independently\n> set to 'yes') possibilities, so something along the lines of ...\n>\n> test_commit () {\n> +\ttag=light &&\n> notick= &&\n> ...\n> \t\twhile test $# != 0\n> \t\tdo\n> \t\t\tcase \"$1\" in\n> \t\t\t...\n> \t\t\t--no-tag)\n> \t-\t\t\tno_tag=yes\n> \t+\t\t\ttag=none\n> \t\t\t\t;;\n> \t+\t\t--annotated)\n> \t+\t\t\ttag=annotated\n> \t+\t\t\t;;\n> \t\t...\n> \t-\tif test -z \"$no_tag\"\n> \t-\tthen\n> \t+\tcase \"$tag\" in\n> \t+\tnone)\n> \t+\t\t;;\n> \t+\tlight)\n> \t\t\tgit ${indir:+ -C \"$indir\") tag \"${4:-$1}\"\n> \t+\t\t;;\n> \t+\tannotated)\n> \t+\t\tgit ${indir:+ -C \"$indir\") tag -m \"$1\" \"${4:-$1}\"\n> \t+\t\t;;\n> \t+\tesac\n> \t\t...\n>\n> after this step (meaning, we may want to start from fixing the no_tag=yes\n> to tag=none before introducing this new feature).\n\nYeah, as noted in the last paragraph of the commit message:\n \n The placement of --annotated-tag after \"notick\" in the case of the\n documentation, and then after \"no_tag\" in the case of the code is\n slightly inconsistent. It's to evade a merge conflict with two other\n commits adding a --printf option, and another one adding documentation\n for --no-tag.\n\nSo the existing patch + not doing a bigger refactoring is because I\ndidn't want to cause conflicts for you to solve with other in-flight\ntopics. It would be easier with ab/pickaxe-pcre2 merged down :)\n\nI'd prefer to keep this as-is for now, and just refactor this function a\nbit after the current topics land.\n\nIn particular I'd like to make the the \"message file contents tags\"\narguments optionally have --long-option versions, so e.g. tests that\nneed a separate --commit-message and --tag-message can use the helper,\nthere's also quite a few that could use a --file=\nv.s. echo-ing a \"message\" to a \"file\".\n\n\n>> @@ -244,7 +252,15 @@ test_commit () {\n>> \t $signoff -m \"$1\" &&\n>> \tif test -z \"$no_tag\"\n>> \tthen\n>> -\t\tgit ${indir:+ -C \"$indir\"} tag \"${4:-$1}\"\n>> +\t\tif test -n \"$annotated_tag\"\n>> +\t\tthen\n>> +\t\t\tif test -z \"$notick\"\n>> +\t\t\tthen\n>> +\t\t\t\ttest_tick\n>> +\t\t\tfi &&\n>> +\t\t\ttest_tick\n>> +\t\tfi &&\n>> +\t\tgit ${indir:+ -C \"$indir\"} tag ${annotated_tag:+ -a -m \"$1\"} \"${4:-$1}\"\n>> \tfi\n>> }\n\n"},{"id":"418129","messageId":"xmqqmtvklp7p.fsf@gitster.c.googlers.com","threadId":"52871","inReplyTo":"87czwhkif6.fsf@evledraar.gmail.com","subject":"Re: [PATCH 04/10] test-lib functions: add an --annotated-tag option to \"test_commit\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-03T06:35:06Z","receivedAt":"2021-03-03T06:51:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Yeah, as noted in the last paragraph of the commit message:\n> \n> The placement of --annotated-tag after \"notick\" in the case of the\n> documentation, and then after \"no_tag\" in the case of the code is\n> slightly inconsistent. It's to evade a merge conflict with two other\n> commits adding a --printf option, and another one adding documentation\n> for --no-tag.\n>\n> So the existing patch + not doing a bigger refactoring is because I\n> didn't want to cause conflicts for you to solve with other in-flight\n> topics. It would be easier with ab/pickaxe-pcre2 merged down :)\n\nAs things that are not yet in 'master' will not be merged down for a\nfew weeks anyway, the conflicts don't matter that much---it is a\nvalid choice not to accept new topics that conflicts with topics\ncooking in 'next' after all. That way, the number of topics that\nfolks have to look at will not increase, which gives more opportunity\nfor topics that are already in 'seen' to get reviewed ;-)\n\n"},{"id":"418563","messageId":"xmqqtupl2lwp.fsf@gitster.c.googlers.com","threadId":"52871","inReplyTo":"20210228195414.21372-1-avarab@gmail.com","subject":"Re: [PATCH 00/10] describe tests: refactor & fix recent broken tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-09T00:47:18Z","receivedAt":"2021-03-09T00:48:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> While looking at the \"git describe\" tests due to the on-list\n> %(describe) discussion I discovered that the feature added in\n> 30b1c7ad9d (describe: don't abort too early when searching tags,\n> 2020-02-26) has never been tested for.\n>\n> This is because it defined a custom test function that called\n> test_expect_success, which then got called inside another top-level\n> test_expect_success. Thus even if it failed we'd pass the test.\n\nThanks. I think I've seen something similar recently. I wonder if\nit is an easy mistake our test linter can catch.\n\n> This series fixes that issue, and makes some general improvements to\n> the \"describe\" tests. I then make test_expect_{success,failure} return\n> 1 to catch these sorts of issues in the future, which required fixes\n> to a couple of svn tests that ran with \"set -e\".\n\nOK.\n\n> I was on the fence about whether to send this after the recent rc0,\n> but figured that since it's test-only Junio might want to pick it up,\n\nEven if it is too late, giving earlier exposure would by itself be a\ngood thing (just don't forget to resend after the dust settles, so\nthat it would land eventually).\n\n> and possibly for the next rc in case we'd like to do some pre-release\n> testing for this never-before-tested feature added in 2.26.0 (although\n> the actual implementation looks fine to me).\n\nThanks; it obviously is a lower priority as we have survived with\nthe broken test for a long time since 2.26 ;-)\n\n> Ævar Arnfjörð Bjarmason (10):\n> describe tests: improve test for --work-tree & --dirty\n> describe tests: refactor away from glob matching\n> describe tests: always assert empty stderr from \"describe\"\n> test-lib functions: add an --annotated-tag option to \"test_commit\"\n> describe tests: convert setup to use test_commit\n> describe tests: fix nested \"test_expect_success\" call\n> describe tests: support -C in \"check_describe\"\n> svn tests: remove legacy re-setup from init-clone test\n> svn tests: refactor away a \"set -e\" in test body\n> test-lib: return 1 from test_expect_{success,failure}\n>\n> t/t1403-show-ref.sh | 6 +-\n> t/t6120-describe.sh | 193 +++++++++++++++-------------------\n> t/t9117-git-svn-init-clone.sh | 6 --\n> t/t9148-git-svn-propset.sh | 27 ++---\n> t/test-lib-functions.sh | 20 +++-\n> 5 files changed, 122 insertions(+), 130 deletions(-)\n"},{"id":"419021","messageId":"xmqq1rcj6hzr.fsf@gitster.g","threadId":"52871","inReplyTo":"20210228195414.21372-1-avarab@gmail.com","subject":"Re: [PATCH 00/10] describe tests: refactor & fix recent broken tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-03-12T23:59:36Z","receivedAt":"2021-03-13T00:00:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> While looking at the \"git describe\" tests due to the on-list\n> %(describe) discussion I discovered that the feature added in\n> 30b1c7ad9d (describe: don't abort too early when searching tags,\n> 2020-02-26) has never been tested for.\n\nThis came during -rc freeze and got reviewed, so I'd expect a reroll\nsometime but does not have to be done in a hurry before the final\nwhich is expected to happen on the 15th.\n\nThanks.\n"},{"id":"421666","messageId":"patch-1.5-c41a777462e-20210412T111601Z-avarab@gmail.com","threadId":"52871","inReplyTo":"cover-0.5-00000000000-20210412T111601Z-avarab@gmail.com","subject":"[PATCH v2 1/5] describe tests: improve test for --work-tree & --dirty","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-12T11:21:43Z","receivedAt":"2021-04-12T11:22:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Improve tests added in 9f67d2e8279 (Teach \"git describe\" --dirty\noption, 2009-10-21) and 2ed5c8e174d (describe: setup working tree for\n--dirty, 2019-02-03) so that they make sense in combination with each\nother.\n\nThe \"check_describe\" being removed here was the earlier test, we then\nlater added these --work-tree tests which really just wanted to check\nif we got the exact same output from \"describe\", but the test wasn't\nstructured to test for that.\n\nLet's change it to do that, which both improves test coverage and\nmakes it more obvious what's going on here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6120-describe.sh | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 88fddc91424..a83ea15faaf 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -151,24 +151,24 @@ test_expect_success 'set-up dirty work tree' '\n \techo >>file\n '\n \n-check_describe \"A-*[0-9a-f]-dirty\" --dirty\n-\n test_expect_success 'describe --dirty with --work-tree (dirty)' '\n+\tgit describe --dirty >expected &&\n \t(\n \t\tcd \"$TEST_DIRECTORY\" &&\n \t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty >\"$TRASH_DIRECTORY/out\"\n \t) &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+-dirty$\" out\n+\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+-dirty$\" out &&\n+\ttest_cmp expected out\n '\n \n-check_describe \"A-*[0-9a-f].mod\" --dirty=.mod\n-\n test_expect_success 'describe --dirty=.mod with --work-tree (dirty)' '\n+\tgit describe --dirty=.mod >expected &&\n \t(\n \t\tcd \"$TEST_DIRECTORY\" &&\n \t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty=.mod >\"$TRASH_DIRECTORY/out\"\n \t) &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+.mod$\" out\n+\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+.mod$\" out &&\n+\ttest_cmp expected out\n '\n \n test_expect_success 'describe --dirty HEAD' '\n-- \n2.31.1.634.gb41287a30b0\n\n"},{"id":"421667","messageId":"cover-0.5-00000000000-20210412T111601Z-avarab@gmail.com","threadId":"52871","inReplyTo":"20210228195414.21372-1-avarab@gmail.com","subject":"[PATCH v2 0/5] describe test fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-12T11:21:42Z","receivedAt":"2021-04-12T11:22:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in v1 we have a test for \"git describe\" test that tests for\nnothing at all, due to being a nested test_expect_success.\n\nThis series is based on my \"test-lib.sh fixes\" series[2] and fixes\nthat bug, and should address all feedback on v1. In particular:\n\n * Shell quoting fixes\n * The \"nothing should have stderr\" general testing is gone per\n Junio's request.\n * Required test-lib.sh fixes moved to [1]\n * We no longer change the return value of\n test_expect_{success,failure} to narrowly catch the bug being fixed\n here.\n * Ejected SVN test fixup patches, needed for the now-ejected\n test_expect_{success,failure} change. Those fixes still make sense,\n but I'll submit them separately (they don't depend on anything\n else).\n\nI think the \"catch the bug\" is probably a good idea, but Junio's\nsuggestion of tracking this via some env variable \"stack depth\" is\nsomething that would probably collide with t0000*.sh changes I have\nunsubmitted/outstanding, and I don't have time to pursue it now. So\nI've left that out.\n\n1. https://lore.kernel.org/git/20210228195414.21372-1-avarab@gmail.com/#t\n2. https://lore.kernel.org/git/cover-00.16-00000000000-20210412T110456Z-avarab@gmail.com/\n\nÆvar Arnfjörð Bjarmason (5):\n describe tests: improve test for --work-tree & --dirty\n describe tests: refactor away from glob matching\n describe tests: don't rely on err.actual from \"check_describe\"\n describe tests: fix nested \"test_expect_success\" call\n describe tests: support -C in \"check_describe\"\n\n t/t6120-describe.sh | 134 ++++++++++++++++++++++++--------------------\n 1 file changed, 72 insertions(+), 62 deletions(-)\n\nRange-diff against v1:\n 1: d76214a9171 = 1: c41a777462e describe tests: improve test for --work-tree & --dirty\n 2: c1b8625de15 ! 2: b428f468d68 describe tests: refactor away from glob matching\n @@ t/t6120-describe.sh: check_describe () {\n -\tesac\n +\t\tgit describe $describe_opts 2>err.actual >raw &&\n +\t\tsed -e \"s/-g[0-9a-f]*\\$/-gHASH/\" actual &&\n -+\t\techo $expect >expect &&\n ++\t\techo \"$expect\" >expect &&\n +\t\ttest_cmp expect actual\n \t'\n }\n \n @@ t/t6120-describe.sh: test_expect_success setup '\n - \n + \ttest_commit --no-tag x file\n '\n \n -check_describe A-* HEAD\n 3: ac1a658d07f ! 3: 50b5a41f88d describe tests: always assert empty stderr from \"describe\"\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - describe tests: always assert empty stderr from \"describe\"\n + describe tests: don't rely on err.actual from \"check_describe\"\n \n - Invert a test added in 3291fe4072e (Add git-describe test for \"verify\n - annotated tag names on output\", 2008-03-03) to make checking that we\n - don't have warnings the rule rather than the exception.\n + Convert the one test that relied on the \"err.actual\" file produced by\n + check_describe() to instead do its own check of \"git describe\"\n + output.\n \n - There was only one case where we expected and got a warning. Let's\n - test for that case explicitly, and assert no warnings or other stderr\n - output for all the rest.\n + This means that the two tests won't have an inter-dependency (e.g. if\n + the earlier test is skipped).\n +\n + An earlier version of this patch instead asserted that no other test\n + had any output on stderr. We're not doing that here out of fear that\n + \"gc --auto\" or another future change to \"git describe\" will cause it\n + to legitimately emit output on stderr unexpectedly[1].\n +\n + I'd think that inverting the test added in 3291fe4072e (Add\n + git-describe test for \"verify annotated tag names on output\",\n + 2008-03-03) to make checking that we don't have warnings the rule\n + rather than the exception would be the sort of thing the describe\n + tests should be catching, but for now let's leave it as it is.\n +\n + 1. http://lore.kernel.org/git/xmqqwnuqo8ze.fsf@gitster.c.googlers.com\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ t/t6120-describe.sh: check_describe () {\n \tdescribe_opts=\"$@\"\n \ttest_expect_success \"describe $describe_opts\" '\n -\t\tgit describe $describe_opts 2>err.actual >raw &&\n -+\t\tgit describe $describe_opts 2>err >raw &&\n -+\t\ttest_must_be_empty err &&\n ++\t\tgit describe $describe_opts >raw &&\n \t\tsed -e \"s/-g[0-9a-f]*\\$/-gHASH/\" actual &&\n - \t\techo $expect >expect &&\n + \t\techo \"$expect\" >expect &&\n \t\ttest_cmp expect actual\n @@ t/t6120-describe.sh: test_expect_success 'describe --contains defaults to HEAD without commit-ish' '\n '\n @@ t/t6120-describe.sh: test_expect_success 'describe --contains defaults to HEAD w\n -'\n +test_expect_success 'renaming tag A to Q locally produces a warning' \"\n +\tmv .git/refs/tags/A .git/refs/tags/Q &&\n -+\tgit describe HEAD 2>actual >out &&\n ++\tgit describe HEAD 2>err >out &&\n +\tcat >expected <<-\\EOF &&\n +\twarning: tag 'Q' is externally known as 'A'\n +\tEOF\n -+\ttest_cmp expected actual &&\n ++\ttest_cmp expected err &&\n +\tgrep -E '^A-8-g[0-9a-f]+$' out\n +\"\n +\n 4: 15efc2c6242 < -: ----------- test-lib functions: add an --annotated-tag option to \"test_commit\"\n 5: 06a8904d693 < -: ----------- describe tests: convert setup to use test_commit\n 6: 91424c8392b = 4: 5c81358d6bb describe tests: fix nested \"test_expect_success\" call\n 7: ecb8f6fb02f ! 5: 798f6cd63c8 describe tests: support -C in \"check_describe\"\n @@ t/t6120-describe.sh: export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n \tshift\n \tdescribe_opts=\"$@\"\n \ttest_expect_success \"describe $describe_opts\" '\n --\t\tgit describe $describe_opts 2>err >raw &&\n -+\t\tgit ${indir:+ -C \"$indir\"} describe $describe_opts 2>err >raw &&\n - \t\ttest_must_be_empty err &&\n +-\t\tgit describe $describe_opts >raw &&\n ++\t\tgit ${indir:+ -C \"$indir\"} describe $describe_opts >raw &&\n \t\tsed -e \"s/-g[0-9a-f]*\\$/-gHASH/\" actual &&\n - \t\techo $expect >expect &&\n + \t\techo \"$expect\" >expect &&\n + \t\ttest_cmp expect actual\n @@ t/t6120-describe.sh: test_expect_success 'setup: describe commits with disjoint bases' '\n \t)\n '\n 8: be5ed59dc61 < -: ----------- svn tests: remove legacy re-setup from init-clone test\n 9: 0b4238d012a < -: ----------- svn tests: refactor away a \"set -e\" in test body\n10: 4f2c4f1fdd5 < -: ----------- test-lib: return 1 from test_expect_{success,failure}\n-- \n2.31.1.634.gb41287a30b0\n\n"},{"id":"421668","messageId":"patch-3.5-50b5a41f88d-20210412T111601Z-avarab@gmail.com","threadId":"52871","inReplyTo":"cover-0.5-00000000000-20210412T111601Z-avarab@gmail.com","subject":"[PATCH v2 3/5] describe tests: don't rely on err.actual from \"check_describe\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-12T11:21:45Z","receivedAt":"2021-04-12T11:22:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Convert the one test that relied on the \"err.actual\" file produced by\ncheck_describe() to instead do its own check of \"git describe\"\noutput.\n\nThis means that the two tests won't have an inter-dependency (e.g. if\nthe earlier test is skipped).\n\nAn earlier version of this patch instead asserted that no other test\nhad any output on stderr. We're not doing that here out of fear that\n\"gc --auto\" or another future change to \"git describe\" will cause it\nto legitimately emit output on stderr unexpectedly[1].\n\nI'd think that inverting the test added in 3291fe4072e (Add\ngit-describe test for \"verify annotated tag names on output\",\n2008-03-03) to make checking that we don't have warnings the rule\nrather than the exception would be the sort of thing the describe\ntests should be catching, but for now let's leave it as it is.\n\n1. http://lore.kernel.org/git/xmqqwnuqo8ze.fsf@gitster.c.googlers.com\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6120-describe.sh | 25 +++++++++++--------------\n 1 file changed, 11 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 13117bbcfb7..911b1928057 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -21,7 +21,7 @@ check_describe () {\n \tshift\n \tdescribe_opts=\"$@\"\n \ttest_expect_success \"describe $describe_opts\" '\n-\t\tgit describe $describe_opts 2>err.actual >raw &&\n+\t\tgit describe $describe_opts >raw &&\n \t\tsed -e \"s/-g[0-9a-f]*\\$/-gHASH/\" actual &&\n \t\techo \"$expect\" >expect &&\n \t\ttest_cmp expect actual\n@@ -90,20 +90,17 @@ test_expect_success 'describe --contains defaults to HEAD without commit-ish' '\n '\n \n check_describe tags/A --all A^0\n-test_expect_success 'no warning was displayed for A' '\n-\ttest_must_be_empty err.actual\n-'\n \n-test_expect_success 'rename tag A to Q locally' '\n-\tmv .git/refs/tags/A .git/refs/tags/Q\n-'\n-cat - >err.expect <err >out &&\n+\tcat >expected <<-\\EOF &&\n+\twarning: tag 'Q' is externally known as 'A'\n+\tEOF\n+\ttest_cmp expected err &&\n+\tgrep -E '^A-8-g[0-9a-f]+$' out\n+\"\n+\n test_expect_success 'misnamed annotated tag forces long output' '\n \tdescription=$(git describe --no-long Q^0) &&\n \texpr \"$description\" : \"A-0-g[0-9a-f]*$\" &&\n-- \n2.31.1.634.gb41287a30b0\n\n"},{"id":"421670","messageId":"patch-2.5-b428f468d68-20210412T111601Z-avarab@gmail.com","threadId":"52871","inReplyTo":"cover-0.5-00000000000-20210412T111601Z-avarab@gmail.com","subject":"[PATCH v2 2/5] describe tests: refactor away from glob matching","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-12T11:21:44Z","receivedAt":"2021-04-12T11:22:04Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the glob matching via a \"case\" statement to a \"test_cmp\" after\nwe've stripped out the hash-specific g\nsuffix. 5312ab11fbf (Add describe test., 2007-01-13).\n\nThis means that we can use test_cmp to compare the output. I could\nomit the \"-8\" change of e.g. \"A-*\" to \"A-8-gHASH\", but I think it\nmakes sense to test that here explicitly. It means you need to add new\ntests to the bottom of the file, but that's not a burden in this case.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6120-describe.sh | 78 ++++++++++++++++++++++-----------------------\n 1 file changed, 38 insertions(+), 40 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex a83ea15faaf..13117bbcfb7 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -21,12 +21,10 @@ check_describe () {\n \tshift\n \tdescribe_opts=\"$@\"\n \ttest_expect_success \"describe $describe_opts\" '\n-\tR=$(git describe $describe_opts 2>err.actual) &&\n-\tcase \"$R\" in\n-\t$expect)\techo happy ;;\n-\t*)\techo \"Oops - $R is not $expect\" &&\n-\t\tfalse ;;\n-\tesac\n+\t\tgit describe $describe_opts 2>err.actual >raw &&\n+\t\tsed -e \"s/-g[0-9a-f]*\\$/-gHASH/\" actual &&\n+\t\techo \"$expect\" >expect &&\n+\t\ttest_cmp expect actual\n \t'\n }\n \n@@ -59,29 +57,29 @@ test_expect_success setup '\n \ttest_commit --no-tag x file\n '\n \n-check_describe A-* HEAD\n-check_describe A-* HEAD^\n-check_describe R-* HEAD^^\n-check_describe A-* HEAD^^2\n+check_describe A-8-gHASH HEAD\n+check_describe A-7-gHASH HEAD^\n+check_describe R-2-gHASH HEAD^^\n+check_describe A-3-gHASH HEAD^^2\n check_describe B HEAD^^2^\n-check_describe R-* HEAD^^^\n+check_describe R-1-gHASH HEAD^^^\n \n-check_describe c-* --tags HEAD\n-check_describe c-* --tags HEAD^\n-check_describe e-* --tags HEAD^^\n-check_describe c-* --tags HEAD^^2\n+check_describe c-7-gHASH --tags HEAD\n+check_describe c-6-gHASH --tags HEAD^\n+check_describe e-1-gHASH --tags HEAD^^\n+check_describe c-2-gHASH --tags HEAD^^2\n check_describe B --tags HEAD^^2^\n check_describe e --tags HEAD^^^\n \n check_describe heads/main --all HEAD\n-check_describe tags/c-* --all HEAD^\n+check_describe tags/c-6-gHASH --all HEAD^\n check_describe tags/e --all HEAD^^^\n \n-check_describe B-0-* --long HEAD^^2^\n-check_describe A-3-* --long HEAD^^2\n+check_describe B-0-gHASH --long HEAD^^2^\n+check_describe A-3-gHASH --long HEAD^^2\n \n-check_describe c-7-* --tags\n-check_describe e-3-* --first-parent --tags\n+check_describe c-7-gHASH --tags\n+check_describe e-3-gHASH --first-parent --tags\n \n test_expect_success 'describe --contains defaults to HEAD without commit-ish' '\n \techo \"A^0\" >expect &&\n@@ -102,7 +100,7 @@ test_expect_success 'rename tag A to Q locally' '\n cat - >err.expect <out &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+$\" out\n+\tgrep -E \"^A-8-g[0-9a-f]+$\" out\n '\n \n-check_describe \"A-*[0-9a-f]\" --dirty\n+check_describe \"A-8-gHASH\" --dirty\n \n test_expect_success 'describe --dirty with --work-tree' '\n \t(\n \t\tcd \"$TEST_DIRECTORY\" &&\n \t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty >\"$TRASH_DIRECTORY/out\"\n \t) &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+$\" out\n+\tgrep -E \"^A-8-g[0-9a-f]+$\" out\n '\n \n test_expect_success 'set-up dirty work tree' '\n@@ -157,7 +155,7 @@ test_expect_success 'describe --dirty with --work-tree (dirty)' '\n \t\tcd \"$TEST_DIRECTORY\" &&\n \t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty >\"$TRASH_DIRECTORY/out\"\n \t) &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+-dirty$\" out &&\n+\tgrep -E \"^A-8-g[0-9a-f]+-dirty$\" out &&\n \ttest_cmp expected out\n '\n \n@@ -167,7 +165,7 @@ test_expect_success 'describe --dirty=.mod with --work-tree (dirty)' '\n \t\tcd \"$TEST_DIRECTORY\" &&\n \t\tgit --git-dir \"$TRASH_DIRECTORY/.git\" --work-tree \"$TRASH_DIRECTORY\" describe --dirty=.mod >\"$TRASH_DIRECTORY/out\"\n \t) &&\n-\tgrep -E \"^A-[1-9][0-9]?-g[0-9a-f]+.mod$\" out &&\n+\tgrep -E \"^A-8-g[0-9a-f]+.mod$\" out &&\n \ttest_cmp expected out\n '\n \n@@ -191,21 +189,21 @@ test_expect_success 'set-up matching pattern tests' '\n \n '\n \n-check_describe \"test-annotated-*\" --match=\"test-*\"\n+check_describe \"test-annotated-3-gHASH\" --match=\"test-*\"\n \n-check_describe \"test1-lightweight-*\" --tags --match=\"test1-*\"\n+check_describe \"test1-lightweight-2-gHASH\" --tags --match=\"test1-*\"\n \n-check_describe \"test2-lightweight-*\" --tags --match=\"test2-*\"\n+check_describe \"test2-lightweight-1-gHASH\" --tags --match=\"test2-*\"\n \n-check_describe \"test2-lightweight-*\" --long --tags --match=\"test2-*\" HEAD^\n+check_describe \"test2-lightweight-0-gHASH\" --long --tags --match=\"test2-*\" HEAD^\n \n-check_describe \"test2-lightweight-*\" --long --tags --match=\"test1-*\" --match=\"test2-*\" HEAD^\n+check_describe \"test2-lightweight-0-gHASH\" --long --tags --match=\"test1-*\" --match=\"test2-*\" HEAD^\n \n-check_describe \"test2-lightweight-*\" --long --tags --match=\"test1-*\" --no-match --match=\"test2-*\" HEAD^\n+check_describe \"test2-lightweight-0-gHASH\" --long --tags --match=\"test1-*\" --no-match --match=\"test2-*\" HEAD^\n \n-check_describe \"test1-lightweight-*\" --long --tags --match=\"test1-*\" --match=\"test3-*\" HEAD\n+check_describe \"test1-lightweight-2-gHASH\" --long --tags --match=\"test1-*\" --match=\"test3-*\" HEAD\n \n-check_describe \"test1-lightweight-*\" --long --tags --match=\"test3-*\" --match=\"test1-*\" HEAD\n+check_describe \"test1-lightweight-2-gHASH\" --long --tags --match=\"test3-*\" --match=\"test1-*\" HEAD\n \n test_expect_success 'set-up branches' '\n \tgit branch branch_A A &&\n@@ -215,11 +213,11 @@ test_expect_success 'set-up branches' '\n \tgit update-ref refs/original/original_branch_A test-annotated~2\n '\n \n-check_describe \"heads/branch_A*\" --all --match=\"branch_*\" --exclude=\"branch_C\" HEAD\n+check_describe \"heads/branch_A-11-gHASH\" --all --match=\"branch_*\" --exclude=\"branch_C\" HEAD\n \n-check_describe \"remotes/origin/remote_branch_A*\" --all --match=\"origin/remote_branch_*\" --exclude=\"origin/remote_branch_C\" HEAD\n+check_describe \"remotes/origin/remote_branch_A-11-gHASH\" --all --match=\"origin/remote_branch_*\" --exclude=\"origin/remote_branch_C\" HEAD\n \n-check_describe \"original/original_branch_A*\" --all test-annotated~1\n+check_describe \"original/original_branch_A-6-gHASH\" --all test-annotated~1\n \n test_expect_success '--match does not work for other types' '\n \ttest_must_fail git describe --all --match=\"*original_branch_*\" test-annotated~1\n@@ -489,7 +487,7 @@ test_expect_success 'describe commits with disjoint bases' '\n \t\tgit tag B -a -m B &&\n \t\tgit merge --no-ff --allow-unrelated-histories main -m x &&\n \n-\t\tcheck_describe \"A-3-*\" HEAD\n+\t\tcheck_describe \"A-3-gHASH\" HEAD\n \t)\n '\n \n@@ -515,7 +513,7 @@ test_expect_success 'describe commits with disjoint bases 2' '\n \t\tgit tag B -a -m B &&\n \t\tgit merge --no-ff --allow-unrelated-histories main -m x &&\n \n-\t\tcheck_describe \"B-3-*\" HEAD\n+\t\tcheck_describe \"B-3-gHASH\" HEAD\n \t)\n '\n \n-- \n2.31.1.634.gb41287a30b0\n\n"},{"id":"421669","messageId":"patch-4.5-5c81358d6bb-20210412T111601Z-avarab@gmail.com","threadId":"52871","inReplyTo":"cover-0.5-00000000000-20210412T111601Z-avarab@gmail.com","subject":"[PATCH v2 4/5] describe tests: fix nested \"test_expect_success\" call","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-12T11:21:46Z","receivedAt":"2021-04-12T11:22:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a nested invocation of \"test_expect_success\", the\n\"check_describe()\" function is a wrapper for calling\ntest_expect_success, and therefore needs to be called outside the body\nof another \"test_expect_success\".\n\nThe two tests added in 30b1c7ad9d6 (describe: don't abort too early\nwhen searching tags, 2020-02-26) were not testing for anything due to\nthis logic error. Without this fix reverting the C code changes in\nthat commit still has all tests passing, with this fix we're actually\ntesting the \"describe\" output. This is because \"test_expect_success\"\ncalls \"test_finish_\", whose last statement happens to be true.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6120-describe.sh | 22 ++++++++++++++--------\n 1 file changed, 14 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 911b1928057..9dc07782ea6 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -469,7 +469,7 @@ test_expect_success 'name-rev covers all conditions while looking at parents' '\n # o-----o---o----x\n # A\n #\n-test_expect_success 'describe commits with disjoint bases' '\n+test_expect_success 'setup: describe commits with disjoint bases' '\n \tgit init disjoint1 &&\n \t(\n \t\tcd disjoint1 &&\n@@ -482,19 +482,22 @@ test_expect_success 'describe commits with disjoint bases' '\n \t\tgit checkout --orphan branch && rm file &&\n \t\techo B > file2 && git add file2 && git commit -m B &&\n \t\tgit tag B -a -m B &&\n-\t\tgit merge --no-ff --allow-unrelated-histories main -m x &&\n-\n-\t\tcheck_describe \"A-3-gHASH\" HEAD\n+\t\tgit merge --no-ff --allow-unrelated-histories main -m x\n \t)\n '\n \n+(\n+\tcd disjoint1 &&\n+\tcheck_describe \"A-3-gHASH\" HEAD\n+)\n+\n # B\n # o---o---o------------.\n # \\\n # o---o---x\n # A\n #\n-test_expect_success 'describe commits with disjoint bases 2' '\n+test_expect_success 'setup: describe commits with disjoint bases 2' '\n \tgit init disjoint2 &&\n \t(\n \t\tcd disjoint2 &&\n@@ -508,10 +511,13 @@ test_expect_success 'describe commits with disjoint bases 2' '\n \t\techo o >> file2 && git add file2 && GIT_COMMITTER_DATE=\"2020-01-01 15:01\" git commit -m o &&\n \t\techo B >> file2 && git add file2 && GIT_COMMITTER_DATE=\"2020-01-01 15:02\" git commit -m B &&\n \t\tgit tag B -a -m B &&\n-\t\tgit merge --no-ff --allow-unrelated-histories main -m x &&\n-\n-\t\tcheck_describe \"B-3-gHASH\" HEAD\n+\t\tgit merge --no-ff --allow-unrelated-histories main -m x\n \t)\n '\n \n+(\n+\tcd disjoint2 &&\n+\tcheck_describe \"B-3-gHASH\" HEAD\n+)\n+\n test_done\n-- \n2.31.1.634.gb41287a30b0\n\n"},{"id":"421671","messageId":"patch-5.5-798f6cd63c8-20210412T111601Z-avarab@gmail.com","threadId":"52871","inReplyTo":"cover-0.5-00000000000-20210412T111601Z-avarab@gmail.com","subject":"[PATCH v2 5/5] describe tests: support -C in \"check_describe\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-12T11:21:47Z","receivedAt":"2021-04-12T11:22:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change a subshell added in a preceding commit to instead use a new\n\"-C\" option to \"check_describe\". The idiom for this is copied as-is\nfrom the \"test_commit\" function in test-lib-functions.sh\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t6120-describe.sh | 27 ++++++++++++++++++---------\n 1 file changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/t/t6120-describe.sh b/t/t6120-describe.sh\nindex 9dc07782ea6..1a501ee09e1 100755\n--- a/t/t6120-describe.sh\n+++ b/t/t6120-describe.sh\n@@ -17,11 +17,26 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n . ./test-lib.sh\n \n check_describe () {\n+\tindir= &&\n+\twhile test $# != 0\n+\tdo\n+\t\tcase \"$1\" in\n+\t\t-C)\n+\t\t\tindir=\"$2\"\n+\t\t\tshift\n+\t\t\t;;\n+\t\t*)\n+\t\t\tbreak\n+\t\t\t;;\n+\t\tesac\n+\t\tshift\n+\tdone &&\n+\tindir=${indir:+\"$indir\"/} &&\n \texpect=\"$1\"\n \tshift\n \tdescribe_opts=\"$@\"\n \ttest_expect_success \"describe $describe_opts\" '\n-\t\tgit describe $describe_opts >raw &&\n+\t\tgit ${indir:+ -C \"$indir\"} describe $describe_opts >raw &&\n \t\tsed -e \"s/-g[0-9a-f]*\\$/-gHASH/\" actual &&\n \t\techo \"$expect\" >expect &&\n \t\ttest_cmp expect actual\n@@ -486,10 +501,7 @@ test_expect_success 'setup: describe commits with disjoint bases' '\n \t)\n '\n \n-(\n-\tcd disjoint1 &&\n-\tcheck_describe \"A-3-gHASH\" HEAD\n-)\n+check_describe -C disjoint1 \"A-3-gHASH\" HEAD\n \n # B\n # o---o---o------------.\n@@ -515,9 +527,6 @@ test_expect_success 'setup: describe commits with disjoint bases 2' '\n \t)\n '\n \n-(\n-\tcd disjoint2 &&\n-\tcheck_describe \"B-3-gHASH\" HEAD\n-)\n+check_describe -C disjoint2 \"B-3-gHASH\" HEAD\n \n test_done\n-- \n2.31.1.634.gb41287a30b0\n\n"},{"id":"421672","messageId":"patch-1.2-d08e098ea8d-20210412T113247Z-avarab@gmail.com","threadId":"52871","inReplyTo":"cover-0.2-00000000000-20210412T113247Z-avarab@gmail.com","subject":"[PATCH v2 1/2] svn tests: remove legacy re-setup from init-clone test","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-12T11:33:25Z","receivedAt":"2021-04-12T11:33:39Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the immediate \"rm -rf .git\" from the start of this test. This\nwas added back in 41337e22f0 (git-svn: add tests for command-line\nusage of init and clone commands, 2007-11-17) when there was a \"trash\"\ndirectory shared by all the tests, but ever since abc5d372ec (Enable\nparallel tests, 2008-08-08) we've had per-test trash directories.\n\nSo this setup can simply be removed. We could use\nTEST_NO_CREATE_REPO=true, but I don't think it's worth the effort to\ngo out of our way to be different. It doesn't matter that we now have\na redundant .git at the top-level.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t9117-git-svn-init-clone.sh | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/t/t9117-git-svn-init-clone.sh b/t/t9117-git-svn-init-clone.sh\nindex 044f65e9166..62de819a44e 100755\n--- a/t/t9117-git-svn-init-clone.sh\n+++ b/t/t9117-git-svn-init-clone.sh\n@@ -7,12 +7,6 @@ test_description='git svn init/clone tests'\n \n . ./lib-git-svn.sh\n \n-# setup, run inside tmp so we don't have any conflicts with $svnrepo\n-set -e\n-rm -r .git\n-mkdir tmp\n-cd tmp\n-\n test_expect_success 'setup svnrepo' '\n \tmkdir project project/trunk project/branches project/tags &&\n \techo foo > project/trunk/foo &&\n-- \n2.31.1.634.gb41287a30b0\n\n"},{"id":"421673","messageId":"cover-0.2-00000000000-20210412T113247Z-avarab@gmail.com","threadId":"52871","inReplyTo":"20210228195414.21372-1-avarab@gmail.com","subject":"[PATCH v2 0/2] svn tests: trivial \"set -e\" in main body of test fixes","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-12T11:33:24Z","receivedAt":"2021-04-12T11:33:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"As noted in 1. this set of trivial fixes has been split off from my\n\"describe tests fixes. The two patches were needed for a now-ejected\n\"make test_expect_{success,failure} return 1\" (to catch such \"set -e\"\nissues).\n\nEven with that gone for now and thus the immediate incentive for these\nfixes being gone, it makes sense to fix these tests to use a more\nmodern style.\n\nThe only other change since v1 is using \">\" instead of \"touch\" per\nEric Wong's suggestion, and rewording the commit message(s) to not\npromise a follow-up change to test_expect_{success,failure}.\n\nÆvar Arnfjörð Bjarmason (2):\n svn tests: remove legacy re-setup from init-clone test\n svn tests: refactor away a \"set -e\" in test body\n\n t/t9117-git-svn-init-clone.sh | 6 ------\n t/t9148-git-svn-propset.sh | 27 +++++++++++++++------------\n 2 files changed, 15 insertions(+), 18 deletions(-)\n\nRange-diff against v1:\n1: be5ed59dc61 = 1: d08e098ea8d svn tests: remove legacy re-setup from init-clone test\n2: 0b4238d012a ! 2: 8dd13c24994 svn tests: refactor away a \"set -e\" in test body\n @@ Commit message\n svn tests: refactor away a \"set -e\" in test body\n \n Refactor a test added in 83c9433e67 (git-svn: support for git-svn\n - propset, 2014-12-07) to avoid using \"set -e\" in the test body. This\n - would have broken in combination with a subsequent change to make\n - \"test_expect_success\" return 1 to catch such cases.\n + propset, 2014-12-07) to avoid using \"set -e\" in the test body. Let's\n + move this into a setup test using \"test_expect_success\" instead.\n \n - While I'm at it rewrite the test to conform to a modern style in our\n - tests, using the \"test_when_finished\" function for the \"rm -rf\", and\n - avoiding repeated \"mkdir\" in favor of \"mkdir -p\".\n + While I'm at it refactor:\n +\n + * Repeated \"mkdir\" to \"mkdir -p\"\n + * Uses of \"touch\" to creating the files with \">\" instead\n + * The \"rm -rf\" at the end to happen in a \"test_when_finished\"\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n @@ t/t9148-git-svn-propset.sh: test_description='git svn propset tests'\n +\t(\n +\t\tcd import &&\n +\t\t# for \"add props top level\"\n -+\t\ttouch foo &&\n ++\t\t>foo &&\n +\t\t# for \"add props relative\"\n -+\t\ttouch subdir/foo_subdir &&\n ++\t\t>subdir/foo_subdir &&\n +\t\t# for \"add props subdir\"\n -+\t\ttouch \"$foo_subdir2\" &&\n ++\t\t>\"$foo_subdir2\" &&\n +\t\tsvn_cmd import -m \"import for git svn\" . \"$svnrepo\"\n +\t)\n +'\n3: 4f2c4f1fdd5 < -: ----------- test-lib: return 1 from test_expect_{success,failure}\n-- \n2.31.1.634.gb41287a30b0\n\n"},{"id":"421674","messageId":"patch-2.2-8dd13c24994-20210412T113247Z-avarab@gmail.com","threadId":"52871","inReplyTo":"cover-0.2-00000000000-20210412T113247Z-avarab@gmail.com","subject":"[PATCH v2 2/2] svn tests: refactor away a \"set -e\" in test body","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-04-12T11:33:26Z","receivedAt":"2021-04-12T11:33:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refactor a test added in 83c9433e67 (git-svn: support for git-svn\npropset, 2014-12-07) to avoid using \"set -e\" in the test body. Let's\nmove this into a setup test using \"test_expect_success\" instead.\n\nWhile I'm at it refactor:\n\n * Repeated \"mkdir\" to \"mkdir -p\"\n * Uses of \"touch\" to creating the files with \">\" instead\n * The \"rm -rf\" at the end to happen in a \"test_when_finished\"\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t9148-git-svn-propset.sh | 27 +++++++++++++++------------\n 1 file changed, 15 insertions(+), 12 deletions(-)\n\ndiff --git a/t/t9148-git-svn-propset.sh b/t/t9148-git-svn-propset.sh\nindex 102639090c1..aebb28995e5 100755\n--- a/t/t9148-git-svn-propset.sh\n+++ b/t/t9148-git-svn-propset.sh\n@@ -7,19 +7,22 @@ test_description='git svn propset tests'\n \n . ./lib-git-svn.sh\n \n-foo_subdir2=\"subdir/subdir2/foo_subdir2\"\n+test_expect_success 'setup propset via import' '\n+\ttest_when_finished \"rm -rf import\" &&\n \n-set -e\n-mkdir import &&\n-(set -e ; cd import\n-\tmkdir subdir\n-\tmkdir subdir/subdir2\n-\ttouch foo \t\t# for 'add props top level'\n-\ttouch subdir/foo_subdir # for 'add props relative'\n-\ttouch \"$foo_subdir2\"\t# for 'add props subdir'\n-\tsvn_cmd import -m 'import for git svn' . \"$svnrepo\" >/dev/null\n-)\n-rm -rf import\n+\tfoo_subdir2=\"subdir/subdir2/foo_subdir2\" &&\n+\tmkdir -p import/subdir/subdir2 &&\n+\t(\n+\t\tcd import &&\n+\t\t# for \"add props top level\"\n+\t\t>foo &&\n+\t\t# for \"add props relative\"\n+\t\t>subdir/foo_subdir &&\n+\t\t# for \"add props subdir\"\n+\t\t>\"$foo_subdir2\" &&\n+\t\tsvn_cmd import -m \"import for git svn\" . \"$svnrepo\"\n+\t)\n+'\n \n test_expect_success 'initialize git svn' '\n \tgit svn init \"$svnrepo\"\n-- \n2.31.1.634.gb41287a30b0\n\n"}]}