Re: [PATCH] completion: complete remote branches with switch --track

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Apr 23, 2020, 02:03 UTC
Message-ID: <20200423020344.GI140314@google.com>
In-Reply-To: <20200422201541.3766173-1-jacob.e.keller@intel.com>

Hi,

Jacob Keller wrote:

Show 20 quoted lines

> If the --track option is supplied to git switch, then a new branch will
> be created tracking the specified remote branch.
>
> Fix git completion support so that remote branches will be completed
> when --track is enabled.
>
> Add a couple of simple test cases to help cover this new behavior. Note
> that ideally completion for --track would only allow remote branches,
> and would not complete all refs like HEAD, FETCH_HEAD, etc, so one of
> the new tests is a test_expect_failure to capture this.
>
> Fixes: ae36fe694180 ("completion: support switch")
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> I wasn't able to figure out how to get completion to ignore things like tags
> and similar, but I think this is still an improvement.
>
>  contrib/completion/git-completion.bash |  8 +++++---
>  t/t9902-completion.sh                  | 22 ++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 3 deletions(-)

Thanks for writing it.

One part I found a little confusing is that --track is being used in two ways. On one hand, it's an option to __git_complete_refs, meaning to complete remote-tracking branches. On the other hand, it's an option to git switch, meaning to create a branch set up to "git pull" from a remote-tracking branch.

Can the commit message give a motivating example to describe what improvement to the user's life this change brings? ("So now you can type 'git ... ' and hit TAB and see ....)

Some nitpicks:

[...]

Show 15 quoted lines

> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2235,12 +2235,14 @@ _git_switch ()
>  		if [ -n "$(__git_find_on_cmdline "--guess")" ]; then
>  			track_opt='--track'
>  		fi
> -		if [ -z "$(__git_find_on_cmdline "-d --detach")" ]; then
> -			only_local_ref=y
> -		else
> +		if [ -n "$(__git_find_on_cmdline "-d --detach")" ]; then
>  			# --guess --detach is invalid combination, no
>  			# dwim will be done when --detach is specified
>  			track_opt=
> +		elif [ -z "$(__git_find_on_cmdline "--track")" ]; then
> +			# if neither --detach or --track are specified then

language nits:

- s/or/nor/ (because the clause starts with "neither")
- s/are/is/ (because "either" and "neither" are singular)

English can be odd.

> +			# match only local refs.
> +			only_local_ref=y
>  		fi

Let me check that I understand correctly:

If --detach is passed, the <start-point> parameter is an arbitrary commit. So we want all refs (or even all commits), not just commits that are eligible for "git switch --guess" (the default mode) dwimery.

If --track is passed, the <start-point> parameter should be an arbitrary remote-tracking branch, not just a remote-tracking branch without corresponding local branch that would be eligible for --guess. A few lines up we handle this by setting track_opt to empty.

If neither --detach nor --track is passed, then...

... I'm not sure I understand the neither --detach nor --track passed case. Wouldn't this be --guess mode, where "$track_opt" is set, so the value of "$only_local_ref" isn't used? Or is this about the case where (1) --detach is not passed, (2) --track is not passed, and (3) --no-guess or GIT_COMPLETION_CHECKOUT_NO_GUESS is passed?

Yes, it must be about that case. In that case, only_local_ref is right.

In any case, this is getting difficult to understand, so I wonder if some refactoring is in order.

[...]

Show 7 quoted lines

> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -1760,6 +1760,28 @@ do
>  	'
>  done
>  
> +test_expect_success 'git switch - default local branches only' '

nit: "default to local branches only" or "the default is local branches only". In other words, this should be a sentence so the reader can understand what property we're testing for.

Show 14 quoted lines

> +	test_completion "git switch m" <<-\EOF
> +	master Z
> +	master-in-other Z
> +	mybranch Z
> +	mytag Z
> +	EOF
> +'
> +
> +test_expect_failure 'git switch - --track remote branches' '
> +	test_completion "git switch --track " <<-\EOF
> +	other/branch-in-other Z
> +	other/master-in-other Z
> +	EOF
> +'

Can this have a short comment describing the issue? If over time the behavior changes, we wouldn't have an easy place to see what the behavior was at the time this test was added.

> +
> +test_expect_success 'git switch - --track remote branches partial completion' '

"git switch --track: partially typed remote-tracking branch is completed"

Show 8 quoted lines

> +	test_completion "git switch --track other/master-in" <<-\EOF
> +	other/master-in-other Z
> +	EOF
> +'
> +
>  test_expect_success 'git config - section' '
>  	test_completion "git config br" <<-\EOF
>  	branch.Z

Thanks and hope that helps, Jonathan