git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 1/2] t9902: verify that completion does not print anything

From
Patrick Steinhardt <ps@pks.im>
Date
Jan 12, 2024, 10:50 UTC
Message-ID
<ZaEZar9OTVgfkD9r@framework>
In-Reply-To
<d978494d-6c48-5923-4745-c42a39e1187a@gmx.de>
On Fri, Jan 12, 2024 at 11:08:21AM +0100, Johannes Schindelin wrote:
Show 58 quoted lines
> Hi Patrick,
> 
> 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@pks.im>
> > ---
> >  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. 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.

I'll reroll this patch series early next week.
Patrick
Previous: Johannes SchindelinNext: Johannes Schindelin
Message 5 of 18 in “completion: silence pseudo-ref existence check”
  1. 0/2 completion: silence pseudo-ref existence checkPatrick Steinhardt, Jan 11, 2024
  2. 1/2 t9902: verify that completion does not print anythingPatrick Steinhardt, Jan 11, 2024
  3. Junio C HamanoJan 11, 2024
  4. Johannes SchindelinJan 12, 2024
  5. Patrick SteinhardtJan 12, 2024
  6. Johannes SchindelinJan 12, 2024
  7. Jeff KingJan 12, 2024
  8. Junio C HamanoJan 12, 2024
  9. Patrick SteinhardtJan 15, 2024
  10. 2/2 completion: silence pseudoref existence checkPatrick Steinhardt, Jan 11, 2024
  11. Junio C HamanoJan 11, 2024
  12. SZEDER GáborJan 13, 2024
  13. 0/5 completion: silence pseudo-ref existence checkPatrick Steinhardt, Jan 15, 2024
  14. 1/5 completion: discover repo path in `__git_pseudoref_exists ()`Patrick Steinhardt, Jan 15, 2024
  15. 2/5 t9902: verify that completion does not print anythingPatrick Steinhardt, Jan 15, 2024
  16. 3/5 completion: improve existence check for pseudo-refsPatrick Steinhardt, Jan 15, 2024
  17. 4/5 completion: silence pseudoref existence checkPatrick Steinhardt, Jan 15, 2024
  18. 5/5 completion: treat dangling symrefs as existing pseudorefsPatrick Steinhardt, Jan 15, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.