* [PATCH] completion: complete remote branches with switch --track
@ 2020-04-22 20:15 Jacob Keller
2020-04-23 2:03 ` Jonathan Nieder
0 siblings, 1 reply; 4+ messages in thread
From: Jacob Keller @ 2020-04-22 20:15 UTC (permalink / raw)
To: git; +Cc: Jacob Keller
From: Jacob Keller <jacob.keller@gmail.com>
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(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c21786f2fd00..6de341fd1587 100644
--- 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
+ # match only local refs.
+ only_local_ref=y
fi
if [ $only_local_ref = y -a -z "$track_opt" ]; then
__gitcomp_direct "$(__git_heads "" "$cur" " ")"
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5505e5aa249e..a2478eae6933 100755
--- 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' '
+ 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
+'
+
+test_expect_success 'git switch - --track remote branches partial completion' '
+ 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
--
2.25.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] completion: complete remote branches with switch --track
2020-04-22 20:15 [PATCH] completion: complete remote branches with switch --track Jacob Keller
@ 2020-04-23 2:03 ` Jonathan Nieder
2020-04-23 4:33 ` Jacob Keller
2020-04-23 23:46 ` Jacob Keller
0 siblings, 2 replies; 4+ messages in thread
From: Jonathan Nieder @ 2020-04-23 2:03 UTC (permalink / raw)
To: Jacob Keller; +Cc: git, Jacob Keller
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.
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.
> + # 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.
[...]
> --- 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.
> + 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"
> + 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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] completion: complete remote branches with switch --track
2020-04-23 2:03 ` Jonathan Nieder
@ 2020-04-23 4:33 ` Jacob Keller
2020-04-23 23:46 ` Jacob Keller
1 sibling, 0 replies; 4+ messages in thread
From: Jacob Keller @ 2020-04-23 4:33 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jacob Keller, Git mailing list
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] completion: complete remote branches with switch --track
2020-04-23 2:03 ` Jonathan Nieder
2020-04-23 4:33 ` Jacob Keller
@ 2020-04-23 23:46 ` Jacob Keller
1 sibling, 0 replies; 4+ messages in thread
From: Jacob Keller @ 2020-04-23 23:46 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Jacob Keller, Git mailing list
On Wed, Apr 22, 2020 at 7:03 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> Let me check that I understand correctly:
>
I do not believe your understanding here is correct. This is about the
case where you try to complete
git switch --track <TAB>
I expect this to complete to the list of all remote branches, and
*not* to include local branches that already exist.
Today, on git version 2.25.2,
git switch --track <TAB> completes only to local branches.
git switch <TAB> appears to complete to basically any reference.
> 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.
>
Yep, I agree here.
> 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.
>
Sort of. Yes, we do disable "track_opt", but, because --detach is
*not* set, we also set only_local_ref=y.
In this case, when only_local_ref=y, and when track_opt is not set, we
trigger the call to __git_heads instead of __git_complete_refs.
I *think* the problem is that we actually got something inverted in a sense:
We probably do want git switch to complete only local branches and
dwims, not arbitrary references.
> 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.
>
that seems to behave properly, i.e.
git switch --no-guess <TAB> will complete only local branches.
> In any case, this is getting difficult to understand, so I wonder if
> some refactoring is in order.
>
The trouble here is that we accidentally trigger that behavior if you do
git switch --track
because we set track_opt to none, *and* we set only_local_refs to y,
resulting in just calling git_heads.
I think the trouble is that __git_complete_refs prints far more than
we want, so it needs some new options to avoid printing certain
settings.
I *think* the problem is that --dwim lists *both* local refs *and*
unique branches from remotes, and because it lists local references,
we end up displaying a bunch of unnecessary refs like tags and such.
Hmm.
Thanks,
Jake
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-04-23 23:46 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-22 20:15 [PATCH] completion: complete remote branches with switch --track Jacob Keller
2020-04-23 2:03 ` Jonathan Nieder
2020-04-23 4:33 ` Jacob Keller
2020-04-23 23:46 ` Jacob Keller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.