patchcompletion: fix __git_complete_worktree_paths

Use __git to invoke "worktree list" in __git_complete_worktree_paths, to respect any "-C" and "--git-dir" options present on the command line.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

I stumbled upon this in a situation like:

   $ git init /tmp/foo && cd /tmp/foo
   $ git worktree add --orphan foo_wt

   $ git init /tmp/bar && cd /tmp/bar
   $ git worktree add --orphan bar_wt

   $ cd /tmp/foo
   $ git -C /tmp/bar worktree remove <TAB>
   ... expecting /tmp/bar/wt, but ...
   $ git -C /tmp/bar worktree remove /tmp/foo_wt

 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Use __git to invoke "worktree list" in __git_complete_worktree_paths, to respect any "-C" and "--git-dir" options present on the command line.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

I stumbled upon this in a situation like:

   $ git init /tmp/foo && cd /tmp/foo
   $ git worktree add --orphan foo_wt

   $ git init /tmp/bar && cd /tmp/bar
   $ git worktree add --orphan bar_wt

   $ cd /tmp/foo
   $ git -C /tmp/bar worktree remove <TAB>
   ... expecting /tmp/bar/wt, but ...
   $ git -C /tmp/bar worktree remove /tmp/foo_wt

In this iteration, v2, some tests are included.

The function __git was introduced in 1cd23e9e05 (completion: don't use __gitdir() for git commands, 2017-02-03). It is a small function, so I'll include here to ease the review of this patch:

	# Runs git with all the options given as argument, respecting any
	# '--git-dir=<path>' and '-C <path>' options present on the command line
	__git ()
	{
		git ${__git_C_args:+"${__git_C_args[@]}"} \
			${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null
	}

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

On Mon, Feb 26, 2024 at 12:53:31AM +0100, Rubén Justo wrote:

If I understand correctly, we assume that the repo isn't detected here, and thus we will fail to complete the command. We don't want an error message though, which we assert. But do we also want to assert that there is no output on stdout?

What does this delete? I don't see "to_delete" being created as part of this test.

And as far as I can see, we don't write to "out" in this test, either. So I think we're accidentally relying on state by the first test here.

Patrick

On Tue, Feb 27, 2024 at 09:21:48AM +0100, Patrick Steinhardt wrote:

Correct.

To me, the check makes sense; to notice if we leak a message in such a circumstance, for instance. I can drop it if you think it does not add value.

The test for stderr is my main goal here.

Good eyes.  It's noise.  I'll drop this line.  Thanks.

The function run_completion leaves the result of the completion in the file "out". So we're checking here if "otherrepo_wt" is present in what "git -C otherrepo worktree remove <TAB>" returns.

Maybe a new function: grep_completion, similar to test_completion, could make this clearer?

Thanks.

Use __git to invoke "worktree list" in __git_complete_worktree_paths, to respect any "-C" and "--git-dir" options present on the command line.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

I stumbled upon this in a situation like:

   $ git init /tmp/foo && cd /tmp/foo
   $ git worktree add --orphan foo_wt

   $ git init /tmp/bar && cd /tmp/bar
   $ git worktree add --orphan bar_wt

   $ cd /tmp/foo
   $ git -C /tmp/bar worktree remove <TAB>
   ... expecting /tmp/bar/wt, but ...
   $ git -C /tmp/bar worktree remove /tmp/foo_wt

The function __git was introduced in 1cd23e9e05 (completion: don't use __gitdir() for git commands, 2017-02-03). It is a small function, so I'll include here to ease the review of this patch:

        # Runs git with all the options given as argument, respecting any
        # '--git-dir=<path>' and '-C <path>' options present on the command line
        __git ()
        {
                git ${__git_C_args:+"${__git_C_args[@]}"} \
                        ${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null
        }

Here are the changes since v2:

"test_completion" already asserts if something is printed on stderr. Therefore "test_must_be_empty err" does not make sense. Removed. This makes the test_completion with an empty response, more valuable, IMO. But I'm open to change it.

The second test_when_finished has been fixed.

Thanks in advance for your review.

Range-diff against v2:
1:  4cc071fb8e ! 1:  de5d2ca1d3 completion: fix __git_complete_worktree_paths
    @@ t/t9902-completion.sh: test_expect_success '__git_complete_fetch_refspecs - full
     +		cd non-repo &&
     +		GIT_CEILING_DIRECTORIES="$ROOT" &&
     +		export GIT_CEILING_DIRECTORIES &&
    -+		test_completion "git worktree remove " "" 2>err &&
    -+		test_must_be_empty err
    ++		test_completion "git worktree remove " ""
     +	)
     +'
     +
     +test_expect_success '__git_complete_worktree_paths with -C' '
    -+	test_when_finished "rm -rf to_delete" &&
    ++	test_when_finished "git -C otherrepo worktree remove otherrepo_wt" &&
     +	git -C otherrepo worktree add --orphan otherrepo_wt &&
     +	run_completion "git -C otherrepo worktree remove " &&
     +	grep otherrepo_wt out

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