Re: [PATCH 2/2] completion: silence pseudoref existence check

["SZEDER Gábor" <szeder.dev@gmail.com>]

On Thu, Jan 11, 2024 at 11:41:59AM +0100, Patrick Steinhardt wrote:
> 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 <ps@pks.im>
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  t/t9902-completion.sh                  | 8 ++++++++
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 8c40ade494..8872192e2b 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -146,7 +146,7 @@ __git_pseudoref_exists ()
>  	if __git_eread "$__git_repo_path/HEAD" head; then
>  		b="${head#ref: }"
>  		if [ "$b" == "refs/heads/.invalid" ]; then

Nit: I guess these last two lines above came from the git prompt
script, where we do need the name of the ref, but here we don't need
that, so the condition could have simply been

  if [ "$head" = "ref: refs/heads/.invalid" ]

With a single = instead of ==, because there is no pattern matching
here.

> -			__git -C "$__git_repo_path" rev-parse --verify --quiet "$ref" 2>/dev/null
> +			__git -C "$__git_repo_path" rev-parse --verify --quiet "$ref" >/dev/null 2>&1

You don't need the '-C $__git_repo_path' option and you don't have to
redirect stderr either, because the purpose of the __git wrapper
function is to take care of both.

>  			return $?
>  		fi
>  	fi
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 78cb93bea7..b14ae4de14 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1927,6 +1927,14 @@ test_expect_success 'git checkout - --orphan with branch already provided comple
>  	EOF
>  '
>  
> +test_expect_success 'git reset completes modified files' '

The description of the test case mentions 'git reset' ...

> +	test_commit A a.file &&
> +	echo B >a.file &&
> +	test_completion "git restore a." <<-\EOF

... but it invokes 'git restore'.

Anyway, I think it would be better to add a dedicated test or two to
exercise the __git_pseudoref_exists helper function instead (or
perhaps in addition).

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