Re: [PATCH v2] completion: fix __git_complete_worktree_paths

From: Rubén Justo <rjusto@gmail.com>
Date: Feb 27, 2024, 18:09 UTC
Message-ID: <0928d29b-41df-40a4-9536-4a8d097faa83@gmail.com>
In-Reply-To: <Zd2bnJZtgW2tFMFn@tanuki>

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

Show 13 quoted lines

> > +test_expect_success '__git_complete_worktree_paths - not a git repository' '
> > +	(
> > +		cd non-repo &&
> > +		GIT_CEILING_DIRECTORIES="$ROOT" &&
> > +		export GIT_CEILING_DIRECTORIES &&
> > +		test_completion "git worktree remove " "" 2>err &&
> > +		test_must_be_empty err
> > +	)
> > +'
> 
> 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.

Correct.

> But do we also want to assert that
> there is no output on stdout?

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.

Show 7 quoted lines

> 
> > +
> > +test_expect_success '__git_complete_worktree_paths with -C' '
> > +	test_when_finished "rm -rf to_delete" &&
> 
> What does this delete? I don't see "to_delete" being created as part of
> this test.

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

Show 7 quoted lines

> 
> > +	git -C otherrepo worktree add --orphan otherrepo_wt &&
> > +	run_completion "git -C otherrepo worktree remove " &&
> > +	grep otherrepo_wt out
> 
> 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.

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?

> 
> Patrick

Thanks.

Show 9 quoted lines

> 
> > +'
> > +
> >  test_expect_success 'git switch - with no options, complete local branches and unique remote branch names for DWIM logic' ' ow
> >  	test_completion "git switch " <<-\EOF
> >  	branch-in-other Z
> > -- 
> > 2.44.0.1.g0da3aa8f7f
> >