Hi Patrick, On Fri, 12 Jan 2024, Patrick Steinhardt wrote: > On Fri, Jan 12, 2024 at 11:08:21AM +0100, Johannes Schindelin wrote: > > > On Thu, 11 Jan 2024, Patrick Steinhardt wrote: > > > > > 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 <ps@xxxxxx> > > > --- > > > t/t9902-completion.sh | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh > > > index aa9a614de3..78cb93bea7 100755 > > > --- a/t/t9902-completion.sh > > > +++ b/t/t9902-completion.sh > > > @@ -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? > > 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. Hmm. I had a closer look and now I am thoroughly confused. In `t/lib-bash.exe`, we make sure that this test is run in Bash. And indeed, when I call BASH=1 POSIXLY_CORRECT=1 TEST_SHELL_PATH=/bin/dash dash t9902-completion.sh -iVx I am greeted by this error message: git-completion.bash: Syntax error: "(" unexpected (expecting "fi") So something else must be going on here. > 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. Personally, I would go for option 2, filtering out the xtrace output. This here seems to work: -- snip -- diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index b14ae4de14e..23cd1cd9508 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -87,11 +87,14 @@ test_completion () else sed -e 's/Z$//' |sort >expected fi && + PS4=+ && run_completion "$1" >"$TRASH_DIRECTORY"/output 2>&1 && + sed "/^+/{:1;s/'[^']*'//g;/'/{N;b1};d}" \ + <"$TRASH_DIRECTORY"/output >"$TRASH_DIRECTORY"/output.x && + test_must_be_empty "$TRASH_DIRECTORY"/output.x && + rm "$TRASH_DIRECTORY"/output "$TRASH_DIRECTORY"/output.x && sort out >out_sorted && - test_cmp expected out_sorted && - test_must_be_empty "$TRASH_DIRECTORY"/output && - rm "$TRASH_DIRECTORY"/output + test_cmp expected out_sorted } # Test __gitcomp. -- snap -- It is a bit ugly, in particular the `sed` expression (which is a bit complex because the `output` file can contain multi-line strings enclosed in single-quotes), and we could probably lose the `"$TRASH_DIRECTORY"/` part to improve the reading experience somewhat. 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? Ciao, Johannes
