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

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Jan 13, 2024, 19:17 UTC
Message-ID: <20240113191749.GB3000857@szeder.dev>
In-Reply-To: <24563975fca8df6ae73917e9ee3534933d47c429.1704969119.git.ps@pks.im>

On Thu, Jan 11, 2024 at 11:41:59AM +0100, Patrick Steinhardt wrote:

Show 27 quoted lines

> 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.

Show 12 quoted lines

>  			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).

Show 10 quoted lines

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