completion: silence pseudo-ref existence check

The Bash completion script must not print anything to either stdout or stderr. Instead, it is only expected to populate certain variables. Tighten our test_completion () test helper to verify this requirement.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t9902-completion.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

t/t9902-completion.sh +0 -0

index aa9a614de3..78cb93bea7 100755

t/t9902-completion.sh +3 -1

 	sort out >out_sorted &&
-	test_cmp expected out_sorted
+	test_cmp expected out_sorted &&
+	test_must_be_empty "$TRASH_DIRECTORY"/output &&
+	rm "$TRASH_DIRECTORY"/output
 }
 
 # Test __gitcomp.

In 44dbb3bf29 (completion: support pseudoref existence checks for reftables, 2023-12-19), we have extended the Bash completion script to support future ref backends better by using git-rev-parse(1) to check for pseudo-ref existence. This conversion has introduced a bug, because even though we pass --quiet to git-rev-parse(1) it would still output the resolved object ID of the ref in question if it exists.

Fix this by redirecting its stdout to /dev/null and add a test that catches this behaviour. Note that the test passes even without the fix for the "files" backend because we parse pseudo refs via the filesystem directly in that case. But the test will fail with the "reftable" backend.

The Bash completion script must not print anything to either stdout or stderr. Instead, it is only expected to populate certain variables. Tighten our test_completion () test helper to verify this requirement.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t9902-completion.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

t/t9902-completion.sh +0 -0

index aa9a614de3..78cb93bea7 100755

Hi Patrick,

Quoted 14 lines · Patrick Steinhardt

The Bash completion script must not print anything to either stdout or stderr. Instead, it is only expected to populate certain variables. Tighten our test_completion () test helper to verify this requirement.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t9902-completion.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

t/t9902-completion.sh +0 -0

index aa9a614de3..78cb93bea7 100755

t/t9902-completion.sh test_completion () · line 93

Quoted 9 lines

t/t9902-completion.sh +3 -2

@@ -87,9 +87,11 @@ test_completion ()
 	else
 		sed -e 's/Z$//' |sort >expected
 	fi &&
-	run_completion "$1" &&
+	run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
 	sort out >out_sorted &&
-	test_cmp expected out_sorted
+	test_cmp expected out_sorted &&
+	test_must_be_empty "$TRASH_DIRECTORY"/output &&

It seems that this fails CI on macOS, most likely because we're running with set -x and that output somehow ends up in output, see e.g. here:
https://github.com/git/git/actions/runs/7496790359/job/20409405194#step:4:1880

[...]
++ test_completion 'git switch '
++ test 1 -gt 1
++ sed -e 's/Z$//'
++ sort
++ run_completion 'git switch '
++ sort out
++ test_cmp expected out_sorted
++ test 2 -ne 2
++ eval 'diff -u' '"$@"'
+++ diff -u expected out_sorted
++ test_must_be_empty '/Users/runner/work/git/git/t/trash directory.t9902-completion/output' ++ test 1 -ne 1
++ test_path_is_file '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ test 1 -ne 1
++ test -f '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ test -s '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ echo ''\''/Users/runner/work/git/git/t/trash directory.t9902-completion/output'\'' is not empty, it contains:' '/Users/runner/work/git/git/t/trash directory.t9902-completion/output' is not empty, it contains:
++ cat '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ local -a COMPREPLY _words
++ local _cword
[...]

Maybe this is running in Dash and therefore BASH_XTRACEFD=4 in test-lib.sh has not the intended effect?

Quoted 54 lines · Johannes Schindelin

Quoted 14 lines · Patrick Steinhardt

The Bash completion script must not print anything to either stdout or stderr. Instead, it is only expected to populate certain variables. Tighten our test_completion () test helper to verify this requirement.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 t/t9902-completion.sh | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

t/t9902-completion.sh +0 -0

index aa9a614de3..78cb93bea7 100755

t/t9902-completion.sh test_completion () · line 93

Quoted 9 lines

t/t9902-completion.sh +3 -2

@@ -87,9 +87,11 @@ test_completion ()
 	else
 		sed -e 's/Z$//' |sort >expected
 	fi &&
-	run_completion "$1" &&
+	run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 &&
 	sort out >out_sorted &&
-	test_cmp expected out_sorted
+	test_cmp expected out_sorted &&
+	test_must_be_empty "$TRASH_DIRECTORY"/output &&

It seems that this fails CI on macOS, most likely because we're running with set -x and that output somehow ends up in output, see e.g. here:
https://github.com/git/git/actions/runs/7496790359/job/20409405194#step:4:1880

[...]
++ test_completion 'git switch '
++ test 1 -gt 1
++ sed -e 's/Z$//'
++ sort
++ run_completion 'git switch '
++ sort out
++ test_cmp expected out_sorted
++ test 2 -ne 2
++ eval 'diff -u' '"$@"'
+++ diff -u expected out_sorted
++ test_must_be_empty '/Users/runner/work/git/git/t/trash directory.t9902-completion/output' ++ test 1 -ne 1
++ test_path_is_file '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ test 1 -ne 1
++ test -f '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ test -s '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ echo ''\''/Users/runner/work/git/git/t/trash directory.t9902-completion/output'\'' is not empty, it contains:' '/Users/runner/work/git/git/t/trash directory.t9902-completion/output' is not empty, it contains:
++ cat '/Users/runner/work/git/git/t/trash directory.t9902-completion/output'
++ local -a COMPREPLY _words
++ local _cword
[...]

Quoted 2 lines

Maybe this is running in Dash and therefore BASH_XTRACEFD=4 in test-lib.sh has not the intended effect?

Meh, thanks for the heads up. Another test gap in GitLab CI which I'm going to address soon via a new macOS job.

In any case, Dash indeed does not honor the above envvar.

I also could not find any alternate solutions to redirect the tracing output. So for all I can see there are a few ways to handle this:

- set -x and then restore the previous value after having called run_completion.

- Filter the output so that any line starting with "${PS4}" gets removed.

- Don't test for this bug.

Not sure which way to go, but the first alternative feels a bit more sensible to me. It does remove the ability to see what's going on in the completion script though in case one wants to debug it.

But my main concern is: Why does this happen in the first place? If we are running with Bash, why does BASH_XTRACEFD to work as intended here and makes it necessary to filter out the traced commands?

Quoted 3 lines · Johannes Schindelin

But my main concern is: Why does this happen in the first place? If we are running with Bash, why does BASH_XTRACEFD to work as intended here and makes it necessary to filter out the traced commands?

BASH_XTRACEFD was introduced in bash 4.1. macOS ships with the ancient bash 3.2.57, which is the last GPLv2 version.

One simple solution is to mark the script with test_untraceable. See 5fc98e79fc (t: add means to disable '-x' tracing for individual test scripts, 2018-02-24) and 5827506928 (t1510-repo-setup: mark as untraceable with '-x', 2018-02-24).

That will disable tracing entirely in the script for older versions of bash, which could make debugging harder. But it will still work as expected for people on reasonable versions of bash, and doesn't introduce any complicated code.

In 44dbb3bf29 (completion: support pseudoref existence checks for reftables, 2023-12-19), we have extended the Bash completion script to support future ref backends better by using git-rev-parse(1) to check for pseudo-ref existence. This conversion has introduced a bug, because even though we pass --quiet to git-rev-parse(1) it would still output the resolved object ID of the ref in question if it exists.

Fix this by redirecting its stdout to /dev/null and add a test that catches this behaviour. Note that the test passes even without the fix for the "files" backend because we parse pseudo refs via the filesystem directly in that case. But the test will fail with the "reftable" backend.

Signed-off-by Patrick Steinhardt <[email protected]>

---
 contrib/completion/git-completion.bash | 2 +-
 t/t9902-completion.sh                  | 8 ++++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

contrib/completion/git-completion.bash +0 -0

index 8c40ade494..8872192e2b 100644

t/t9902-completion.sh +4 -0

+	a.file
+	EOF
+'
+
 test_expect_success 'teardown after ref completion' '
 	git branch -d matching-branch &&
 	git tag -d matching-tag &&

Signature · 1 line

2.43.GIT

Quoted 3 lines · Johannes Schindelin

But my main concern is: Why does this happen in the first place? If we are running with Bash, why does BASH_XTRACEFD to work as intended here and makes it necessary to filter out the traced commands?

BASH_XTRACEFD was introduced in bash 4.1. macOS ships with the ancient bash 3.2.57, which is the last GPLv2 version.

One simple solution is to mark the script with test_untraceable. See 5fc98e79fc (t: add means to disable '-x' tracing for individual test scripts, 2018-02-24) and 5827506928 (t1510-repo-setup: mark as untraceable with '-x', 2018-02-24).

That will disable tracing entirely in the script for older versions of bash, which could make debugging harder. But it will still work as expected for people on reasonable versions of bash, and doesn't introduce any complicated code.

-Peff