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

[Jacob Keller <jacob.keller@gmail.com>]

On Wed, Apr 22, 2020 at 7:03 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Hi,
>
> Jacob Keller wrote:
>
> > 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.
>

Sure, I might actually just go and write a patch to switch --track to
--dwim and allow --track only for backwards compatibility with
external completions.

> 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:
>
> [...]
> > --- 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.
>

Sure. It's easy to get this wrong, can fix.

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

Yes.

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

I think so. There's also room for improvement, because currently with
this patch:

git switch --track <TAB>

completes all refs including stuff like FETCH_HEAD, etc.

I think the most obvious thing would be to complete remote branches
only. We could complete more points if both track and -c are present,
indicating the user wants to track with a known name. (Since --track's
automatic naming is based on extracting the origin.

I don't think the current functions we use can quite handle what we
want so maybe it's time I try to dig into the gitref code to see if we
can extract the right refs with new options..

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

Yea makes sense.

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

Yes, I'll add a sentence.

> > +
> > +test_expect_success 'git switch - --track remote branches partial completion' '
>
> "git switch --track: partially typed remote-tracking branch is completed"
>
> > +     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

Thanks. I greatly appreciate the review!

Regards,
Jake