{"thread":{"id":"53288","subject":"[PATCH] completion: complete remote branches with switch --track","startedAt":"2020-04-22T20:17:03Z","lastAt":"2020-04-23T23:46:22Z","messageCount":4,"participants":["Jacob Keller","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"395940","messageId":"20200422201541.3766173-1-jacob.e.keller@intel.com","threadId":"53288","inReplyTo":null,"subject":"[PATCH] completion: complete remote branches with switch --track","fromName":"Jacob Keller","fromEmail":"jacob.e.keller@intel.com","sentAt":"2020-04-22T20:15:42Z","receivedAt":"2020-04-22T20:17:03Z","isPatch":true,"sender":{"key":"jacob.e.keller@intel.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"From: Jacob Keller <jacob.keller@gmail.com>\n\nIf the --track option is supplied to git switch, then a new branch will\nbe created tracking the specified remote branch.\n\nFix git completion support so that remote branches will be completed\nwhen --track is enabled.\n\nAdd a couple of simple test cases to help cover this new behavior. Note\nthat ideally completion for --track would only allow remote branches,\nand would not complete all refs like HEAD, FETCH_HEAD, etc, so one of\nthe new tests is a test_expect_failure to capture this.\n\nFixes: ae36fe694180 (\"completion: support switch\")\nSigned-off-by: Jacob Keller <jacob.keller@gmail.com>\n---\nI wasn't able to figure out how to get completion to ignore things like tags\nand similar, but I think this is still an improvement.\n\n contrib/completion/git-completion.bash |  8 +++++---\n t/t9902-completion.sh                  | 22 ++++++++++++++++++++++\n 2 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex c21786f2fd00..6de341fd1587 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -2235,12 +2235,14 @@ _git_switch ()\n \t\tif [ -n \"$(__git_find_on_cmdline \"--guess\")\" ]; then\n \t\t\ttrack_opt='--track'\n \t\tfi\n-\t\tif [ -z \"$(__git_find_on_cmdline \"-d --detach\")\" ]; then\n-\t\t\tonly_local_ref=y\n-\t\telse\n+\t\tif [ -n \"$(__git_find_on_cmdline \"-d --detach\")\" ]; then\n \t\t\t# --guess --detach is invalid combination, no\n \t\t\t# dwim will be done when --detach is specified\n \t\t\ttrack_opt=\n+\t\telif [ -z \"$(__git_find_on_cmdline \"--track\")\" ]; then\n+\t\t\t# if neither --detach or --track are specified then\n+\t\t\t# match only local refs.\n+\t\t\tonly_local_ref=y\n \t\tfi\n \t\tif [ $only_local_ref = y -a -z \"$track_opt\" ]; then\n \t\t\t__gitcomp_direct \"$(__git_heads \"\" \"$cur\" \" \")\"\ndiff --git a/t/t9902-completion.sh b/t/t9902-completion.sh\nindex 5505e5aa249e..a2478eae6933 100755\n--- a/t/t9902-completion.sh\n+++ b/t/t9902-completion.sh\n@@ -1760,6 +1760,28 @@ do\n \t'\n done\n \n+test_expect_success 'git switch - default local branches only' '\n+\ttest_completion \"git switch m\" <<-\\EOF\n+\tmaster Z\n+\tmaster-in-other Z\n+\tmybranch Z\n+\tmytag Z\n+\tEOF\n+'\n+\n+test_expect_failure 'git switch - --track remote branches' '\n+\ttest_completion \"git switch --track \" <<-\\EOF\n+\tother/branch-in-other Z\n+\tother/master-in-other Z\n+\tEOF\n+'\n+\n+test_expect_success 'git switch - --track remote branches partial completion' '\n+\ttest_completion \"git switch --track other/master-in\" <<-\\EOF\n+\tother/master-in-other Z\n+\tEOF\n+'\n+\n test_expect_success 'git config - section' '\n \ttest_completion \"git config br\" <<-\\EOF\n \tbranch.Z\n-- \n2.25.2\n\n"},{"id":"395976","messageId":"20200423020344.GI140314@google.com","threadId":"53288","inReplyTo":"20200422201541.3766173-1-jacob.e.keller@intel.com","subject":"Re: [PATCH] completion: complete remote branches with switch --track","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-04-23T02:03:44Z","receivedAt":"2020-04-23T02:03:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJacob Keller wrote:\n\n> If the --track option is supplied to git switch, then a new branch will\n> be created tracking the specified remote branch.\n>\n> Fix git completion support so that remote branches will be completed\n> when --track is enabled.\n>\n> Add a couple of simple test cases to help cover this new behavior. Note\n> that ideally completion for --track would only allow remote branches,\n> and would not complete all refs like HEAD, FETCH_HEAD, etc, so one of\n> the new tests is a test_expect_failure to capture this.\n>\n> Fixes: ae36fe694180 (\"completion: support switch\")\n> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> ---\n> I wasn't able to figure out how to get completion to ignore things like tags\n> and similar, but I think this is still an improvement.\n>\n>  contrib/completion/git-completion.bash |  8 +++++---\n>  t/t9902-completion.sh                  | 22 ++++++++++++++++++++++\n>  2 files changed, 27 insertions(+), 3 deletions(-)\n\nThanks for writing it.\n\nOne part I found a little confusing is that --track is being used in\ntwo ways.  On one hand, it's an option to __git_complete_refs, meaning\nto complete remote-tracking branches.  On the other hand, it's an option\nto git switch, meaning to create a branch set up to \"git pull\" from a\nremote-tracking branch.\n\nCan the commit message give a motivating example to describe what\nimprovement to the user's life this change brings?  (\"So now you can\ntype 'git ... ' and hit TAB and see ....)\n\nSome nitpicks:\n\n[...]\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -2235,12 +2235,14 @@ _git_switch ()\n>  \t\tif [ -n \"$(__git_find_on_cmdline \"--guess\")\" ]; then\n>  \t\t\ttrack_opt='--track'\n>  \t\tfi\n> -\t\tif [ -z \"$(__git_find_on_cmdline \"-d --detach\")\" ]; then\n> -\t\t\tonly_local_ref=y\n> -\t\telse\n> +\t\tif [ -n \"$(__git_find_on_cmdline \"-d --detach\")\" ]; then\n>  \t\t\t# --guess --detach is invalid combination, no\n>  \t\t\t# dwim will be done when --detach is specified\n>  \t\t\ttrack_opt=\n> +\t\telif [ -z \"$(__git_find_on_cmdline \"--track\")\" ]; then\n> +\t\t\t# if neither --detach or --track are specified then\n\nlanguage nits:\n\n- s/or/nor/ (because the clause starts with \"neither\")\n- s/are/is/ (because \"either\" and \"neither\" are singular)\n\nEnglish can be odd.\n\n> +\t\t\t# match only local refs.\n> +\t\t\tonly_local_ref=y\n>  \t\tfi\n\nLet me check that I understand correctly:\n\nIf --detach is passed, the <start-point> parameter is an arbitrary\ncommit.  So we want all refs (or even all commits), not just commits\nthat are eligible for \"git switch --guess\" (the default mode) dwimery.\n\nIf --track is passed, the <start-point> parameter should be an\narbitrary remote-tracking branch, not just a remote-tracking branch\nwithout corresponding local branch that would be eligible for --guess.\nA few lines up we handle this by setting track_opt to empty.\n\nIf neither --detach nor --track is passed, then...\n\n... I'm not sure I understand the neither --detach nor --track passed\ncase.  Wouldn't this be --guess mode, where \"$track_opt\" is set, so the\nvalue of \"$only_local_ref\" isn't used?  Or is this about the case\nwhere (1) --detach is not passed, (2) --track is not passed, and (3)\n--no-guess or GIT_COMPLETION_CHECKOUT_NO_GUESS is passed?\n\nYes, it must be about that case.  In that case, only_local_ref is\nright.\n\nIn any case, this is getting difficult to understand, so I wonder if\nsome refactoring is in order.\n\n[...]\n> --- a/t/t9902-completion.sh\n> +++ b/t/t9902-completion.sh\n> @@ -1760,6 +1760,28 @@ do\n>  \t'\n>  done\n>  \n> +test_expect_success 'git switch - default local branches only' '\n\nnit: \"default to local branches only\" or \"the default is local\nbranches only\".  In other words, this should be a sentence so the\nreader can understand what property we're testing for.\n\n> +\ttest_completion \"git switch m\" <<-\\EOF\n> +\tmaster Z\n> +\tmaster-in-other Z\n> +\tmybranch Z\n> +\tmytag Z\n> +\tEOF\n> +'\n> +\n> +test_expect_failure 'git switch - --track remote branches' '\n> +\ttest_completion \"git switch --track \" <<-\\EOF\n> +\tother/branch-in-other Z\n> +\tother/master-in-other Z\n> +\tEOF\n> +'\n\nCan this have a short comment describing the issue?  If over time the\nbehavior changes, we wouldn't have an easy place to see what the\nbehavior was at the time this test was added.\n\n> +\n> +test_expect_success 'git switch - --track remote branches partial completion' '\n\n\"git switch --track: partially typed remote-tracking branch is completed\"\n\n> +\ttest_completion \"git switch --track other/master-in\" <<-\\EOF\n> +\tother/master-in-other Z\n> +\tEOF\n> +'\n> +\n>  test_expect_success 'git config - section' '\n>  \ttest_completion \"git config br\" <<-\\EOF\n>  \tbranch.Z\n\nThanks and hope that helps,\nJonathan\n"},{"id":"395978","messageId":"CA+P7+xqTONc_WKxswhrTFy=W3R_7GM+ySCd+gQN=Yw4fbMSSEA@mail.gmail.com","threadId":"53288","inReplyTo":"20200423020344.GI140314@google.com","subject":"Re: [PATCH] completion: complete remote branches with switch --track","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2020-04-23T04:33:50Z","receivedAt":"2020-04-23T04:34:03Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Apr 22, 2020 at 7:03 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n> Hi,\n>\n> Jacob Keller wrote:\n>\n> > If the --track option is supplied to git switch, then a new branch will\n> > be created tracking the specified remote branch.\n> >\n> > Fix git completion support so that remote branches will be completed\n> > when --track is enabled.\n> >\n> > Add a couple of simple test cases to help cover this new behavior. Note\n> > that ideally completion for --track would only allow remote branches,\n> > and would not complete all refs like HEAD, FETCH_HEAD, etc, so one of\n> > the new tests is a test_expect_failure to capture this.\n> >\n> > Fixes: ae36fe694180 (\"completion: support switch\")\n> > Signed-off-by: Jacob Keller <jacob.keller@gmail.com>\n> > ---\n> > I wasn't able to figure out how to get completion to ignore things like tags\n> > and similar, but I think this is still an improvement.\n> >\n> >  contrib/completion/git-completion.bash |  8 +++++---\n> >  t/t9902-completion.sh                  | 22 ++++++++++++++++++++++\n> >  2 files changed, 27 insertions(+), 3 deletions(-)\n>\n> Thanks for writing it.\n>\n> One part I found a little confusing is that --track is being used in\n> two ways.  On one hand, it's an option to __git_complete_refs, meaning\n> to complete remote-tracking branches.  On the other hand, it's an option\n> to git switch, meaning to create a branch set up to \"git pull\" from a\n> remote-tracking branch.\n>\n\nSure, I might actually just go and write a patch to switch --track to\n--dwim and allow --track only for backwards compatibility with\nexternal completions.\n\n> Can the commit message give a motivating example to describe what\n> improvement to the user's life this change brings?  (\"So now you can\n> type 'git ... ' and hit TAB and see ....)\n>\n> Some nitpicks:\n>\n> [...]\n> > --- a/contrib/completion/git-completion.bash\n> > +++ b/contrib/completion/git-completion.bash\n> > @@ -2235,12 +2235,14 @@ _git_switch ()\n> >               if [ -n \"$(__git_find_on_cmdline \"--guess\")\" ]; then\n> >                       track_opt='--track'\n> >               fi\n> > -             if [ -z \"$(__git_find_on_cmdline \"-d --detach\")\" ]; then\n> > -                     only_local_ref=y\n> > -             else\n> > +             if [ -n \"$(__git_find_on_cmdline \"-d --detach\")\" ]; then\n> >                       # --guess --detach is invalid combination, no\n> >                       # dwim will be done when --detach is specified\n> >                       track_opt=\n> > +             elif [ -z \"$(__git_find_on_cmdline \"--track\")\" ]; then\n> > +                     # if neither --detach or --track are specified then\n>\n> language nits:\n>\n> - s/or/nor/ (because the clause starts with \"neither\")\n> - s/are/is/ (because \"either\" and \"neither\" are singular)\n>\n> English can be odd.\n>\n\nSure. It's easy to get this wrong, can fix.\n\n> > +                     # match only local refs.\n> > +                     only_local_ref=y\n> >               fi\n>\n> Let me check that I understand correctly:\n>\n> If --detach is passed, the <start-point> parameter is an arbitrary\n> commit.  So we want all refs (or even all commits), not just commits\n> that are eligible for \"git switch --guess\" (the default mode) dwimery.\n>\n\nYes.\n\n> If --track is passed, the <start-point> parameter should be an\n> arbitrary remote-tracking branch, not just a remote-tracking branch\n> without corresponding local branch that would be eligible for --guess.\n> A few lines up we handle this by setting track_opt to empty.\n>\n> If neither --detach nor --track is passed, then..\n>\n> ... I'm not sure I understand the neither --detach nor --track passed\n> case.  Wouldn't this be --guess mode, where \"$track_opt\" is set, so the\n> value of \"$only_local_ref\" isn't used?  Or is this about the case\n> where (1) --detach is not passed, (2) --track is not passed, and (3)\n> --no-guess or GIT_COMPLETION_CHECKOUT_NO_GUESS is passed?\n>\n> Yes, it must be about that case.  In that case, only_local_ref is\n> right.\n>\n> In any case, this is getting difficult to understand, so I wonder if\n> some refactoring is in order.\n>\n\nI think so. There's also room for improvement, because currently with\nthis patch:\n\ngit switch --track <TAB>\n\ncompletes all refs including stuff like FETCH_HEAD, etc.\n\nI think the most obvious thing would be to complete remote branches\nonly. We could complete more points if both track and -c are present,\nindicating the user wants to track with a known name. (Since --track's\nautomatic naming is based on extracting the origin.\n\nI don't think the current functions we use can quite handle what we\nwant so maybe it's time I try to dig into the gitref code to see if we\ncan extract the right refs with new options..\n\n> [...]\n> > --- a/t/t9902-completion.sh\n> > +++ b/t/t9902-completion.sh\n> > @@ -1760,6 +1760,28 @@ do\n> >       '\n> >  done\n> >\n> > +test_expect_success 'git switch - default local branches only' '\n>\n> nit: \"default to local branches only\" or \"the default is local\n> branches only\".  In other words, this should be a sentence so the\n> reader can understand what property we're testing for.\n>\n\nYea makes sense.\n\n> > +     test_completion \"git switch m\" <<-\\EOF\n> > +     master Z\n> > +     master-in-other Z\n> > +     mybranch Z\n> > +     mytag Z\n> > +     EOF\n> > +'\n> > +\n> > +test_expect_failure 'git switch - --track remote branches' '\n> > +     test_completion \"git switch --track \" <<-\\EOF\n> > +     other/branch-in-other Z\n> > +     other/master-in-other Z\n> > +     EOF\n> > +'\n>\n> Can this have a short comment describing the issue?  If over time the\n> behavior changes, we wouldn't have an easy place to see what the\n> behavior was at the time this test was added.\n>\n\nYes, I'll add a sentence.\n\n> > +\n> > +test_expect_success 'git switch - --track remote branches partial completion' '\n>\n> \"git switch --track: partially typed remote-tracking branch is completed\"\n>\n> > +     test_completion \"git switch --track other/master-in\" <<-\\EOF\n> > +     other/master-in-other Z\n> > +     EOF\n> > +'\n> > +\n> >  test_expect_success 'git config - section' '\n> >       test_completion \"git config br\" <<-\\EOF\n> >       branch.Z\n>\n> Thanks and hope that helps,\n> Jonathan\n\nThanks. I greatly appreciate the review!\n\nRegards,\nJake\n"},{"id":"396070","messageId":"CA+P7+xqgW0tfLcy8R4gy2X_8apvXOaibvd3XWWdKSk8yMi+4Bg@mail.gmail.com","threadId":"53288","inReplyTo":"20200423020344.GI140314@google.com","subject":"Re: [PATCH] completion: complete remote branches with switch --track","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2020-04-23T23:46:08Z","receivedAt":"2020-04-23T23:46:22Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Apr 22, 2020 at 7:03 PM Jonathan Nieder <jrnieder@gmail.com> wrote:\n>\n> Let me check that I understand correctly:\n>\n\nI do not believe your understanding here is correct. This is about the\ncase where you try to complete\n\ngit switch --track <TAB>\n\nI expect this to complete to the list of all remote branches, and\n*not* to include local branches that already exist.\n\nToday, on git version 2.25.2,\n\ngit switch --track <TAB> completes only to local branches.\n\ngit switch <TAB> appears to complete to basically any reference.\n\n> If --detach is passed, the <start-point> parameter is an arbitrary\n> commit.  So we want all refs (or even all commits), not just commits\n> that are eligible for \"git switch --guess\" (the default mode) dwimery.\n>\n\nYep, I agree here.\n\n> If --track is passed, the <start-point> parameter should be an\n> arbitrary remote-tracking branch, not just a remote-tracking branch\n> without corresponding local branch that would be eligible for --guess.\n> A few lines up we handle this by setting track_opt to empty.\n>\n\nSort of. Yes, we do disable \"track_opt\", but, because --detach is\n*not* set, we also set only_local_ref=y.\n\nIn this case, when only_local_ref=y, and when track_opt is not set, we\ntrigger the call to __git_heads instead of __git_complete_refs.\n\nI *think* the problem is that we actually got something inverted in a sense:\n\nWe probably do want git switch to complete only local branches and\ndwims, not arbitrary references.\n\n> If neither --detach nor --track is passed, then...\n>\n> ... I'm not sure I understand the neither --detach nor --track passed\n> case.  Wouldn't this be --guess mode, where \"$track_opt\" is set, so the\n> value of \"$only_local_ref\" isn't used?  Or is this about the case\n> where (1) --detach is not passed, (2) --track is not passed, and (3)\n> --no-guess or GIT_COMPLETION_CHECKOUT_NO_GUESS is passed?\n>\n> Yes, it must be about that case.  In that case, only_local_ref is\n> right.\n>\n\nthat seems to behave properly, i.e.\n\ngit switch --no-guess <TAB> will complete only local branches.\n\n> In any case, this is getting difficult to understand, so I wonder if\n> some refactoring is in order.\n>\n\nThe trouble here is that we accidentally trigger that behavior if you do\n\ngit switch --track\n\nbecause we set track_opt to none, *and* we set only_local_refs to y,\nresulting in just calling git_heads.\n\nI think the trouble is that __git_complete_refs prints far more than\nwe want, so it needs some new options to avoid printing certain\nsettings.\n\nI *think* the problem is that --dwim lists *both* local refs *and*\nunique branches from remotes, and because it lists local references,\nwe end up displaying a bunch of unnecessary refs like tags and such.\n\nHmm.\n\nThanks,\nJake\n"}]}