| Message ID | 20200422201541.3766173-1-jacob.e.keller@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | completion: complete remote branches with switch --track | expand |
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
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
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
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