threads / patch / 54158

patcht3200: clean side effect of git checkout --orphan

Subject: [PATCH] t3200: clean side effect of git checkout --orphan

## tl;dr

4 messages between Aug 29, 2020 and Aug 31, 2020. Diffs are folded; open one to read it.

replies: 3people: 3as markdown or json

Aaron Lipman· Aug 29, 2020, 22:56 UTC · lore

The "refuse --edit-description on unborn branch for now" test in t3200 switches to an orphan branch, causing subsequent git commands referencing HEAD to fail. Avoid this side-effect by switching back to master after that test finishes.

This has gone undetected, as the next effected test expects failure - but it currently fails for the wrong reason.

Verbose output of the next test referencing HEAD, "--merged is incompatible with --no-merged":

  fatal: malformed object name HEAD
Which this commit corrects to:
  error: option `no-merged' is incompatible with --merged
Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
We might considered updating "--merged is incompatible with --no-merged"
tests to not only test for failure, but check the contents of the error
message to ensure they fail for the correct reason, e.g.:
test_expect_success '--merged is incompatible with --no-merged' '
  test_must_fail git branch --merged HEAD --no-merged HEAD 2>error &&
  test_i18ngrep "is incompatible with --merged" error
'

However, I intend to submit a patch enabling ref-filter to accept multiple merged/no-merged filters, which would make said updates irrelevant.

 t/t3200-branch.sh | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/t3200-branch.sh +1 −0
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 4c0734157b..028c88d1b2 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1287,6 +1287,7 @@ test_expect_success 'detect typo in branch name when using --edit-description' '
 '
 
 test_expect_success 'refuse --edit-description on unborn branch for now' '
+	test_when_finished "git checkout master" &&
 	write_script editor <<-\EOF &&
 		echo "New contents" >"$1"
 	EOF

-- 
2.24.3 (Apple Git-128)
Eric Sunshine· Aug 30, 2020, 05:35 UTC · re: Aaron Lipman · lore

Re: [PATCH] t3200: clean side effect of git checkout --orphan

On Sat, Aug 29, 2020 at 6:57 PM Aaron Lipman <alipman88@gmail.com> wrote:
Show 7 quoted lines
> The "refuse --edit-description on unborn branch for now" test in t3200
> switches to an orphan branch, causing subsequent git commands
> referencing HEAD to fail. Avoid this side-effect by switching back to
> master after that test finishes.
>
> This has gone undetected, as the next effected test expects failure -
> but it currently fails for the wrong reason.
s/effected/affected

In fact, the three tests following the orphan test all expect failure (though I didn't check if they also fail for the wrong reason), and the following test which doesn't expect failure has an explicit "git checkout master" early on, which explains why it was not impacted by this problem.

Show 10 quoted lines
> Verbose output of the next test referencing HEAD,
> "--merged is incompatible with --no-merged":
>
>   fatal: malformed object name HEAD
>
> Which this commit corrects to:
>
>   error: option `no-merged' is incompatible with --merged
>
> Signed-off-by: Aaron Lipman <alipman88@gmail.com>
Description and actual fix make perfect sense.
Junio C Hamano· Aug 31, 2020, 18:21 UTC · re: Eric Sunshine · lore

Re: [PATCH] t3200: clean side effect of git checkout --orphan

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 29 quoted lines
> On Sat, Aug 29, 2020 at 6:57 PM Aaron Lipman <alipman88@gmail.com> wrote:
>> The "refuse --edit-description on unborn branch for now" test in t3200
>> switches to an orphan branch, causing subsequent git commands
>> referencing HEAD to fail. Avoid this side-effect by switching back to
>> master after that test finishes.
>>
>> This has gone undetected, as the next effected test expects failure -
>> but it currently fails for the wrong reason.
>
> s/effected/affected
>
> In fact, the three tests following the orphan test all expect failure
> (though I didn't check if they also fail for the wrong reason), and
> the following test which doesn't expect failure has an explicit "git
> checkout master" early on, which explains why it was not impacted by
> this problem.
>
>> Verbose output of the next test referencing HEAD,
>> "--merged is incompatible with --no-merged":
>>
>>   fatal: malformed object name HEAD
>>
>> Which this commit corrects to:
>>
>>   error: option `no-merged' is incompatible with --merged
>>
>> Signed-off-by: Aaron Lipman <alipman88@gmail.com>
>
> Description and actual fix make perfect sense.

Yeah, looks good. Of course, the affected test can be made more defensive to protect the precondition it relies on from getting broken by other tests (i.e. if it refers to HEAD, it should make sure it is on an actual commit). Each test cleaning up after itself is a good discipline to have, but what is "clean" is quite subjective and depends on each test piece in the script X-<.

Thanks, will queue.
Aaron Lipman· Aug 30, 2020, 22:42 UTC · re: Aaron Lipman · lore

[PATCH v2] t3200: clean side effect of git checkout --orphan

The "refuse --edit-description on unborn branch for now" test in t3200 switches to an orphan branch, causing subsequent git commands referencing HEAD to fail. Avoid this side-effect by switching back to master after the test finishes.

This has gone undetected, as the next affected test expects failure - but it currently fails for the wrong reason.

Verbose output of the next test referencing HEAD, "--merged is incompatible with --no-merged":

  fatal: malformed object name HEAD
Which this commit corrects to:
  error: option `no-merged' is incompatible with --merged
Signed-off-by: Aaron Lipman <alipman88@gmail.com>
---
Whoops, fixed the effected/affected error.
Thanks for catching that, Eric!
 t/t3200-branch.sh | 1 +
 1 file changed, 1 insertion(+)
Show changes to t/t3200-branch.sh +1 −0
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index 4c0734157b..028c88d1b2 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -1287,6 +1287,7 @@ test_expect_success 'detect typo in branch name when using --edit-description' '
 '
 
 test_expect_success 'refuse --edit-description on unborn branch for now' '
+	test_when_finished "git checkout master" &&
 	write_script editor <<-\EOF &&
 		echo "New contents" >"$1"
 	EOF

-- 
2.24.3 (Apple Git-128)

← back to recent threads