git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Lessley Dennington <lessleydennington@gmail.com>
Cc: "Lessley Dennington via GitGitGadget" <gitgitgadget@gmail.com>,
	"Git Mailing List" <git@vger.kernel.org>,
	"Derrick Stolee" <stolee@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	johannes.schindelin@gmail.com, "Taylor Blau" <me@ttaylorr.com>,
	"SZEDER Gábor" <szeder.dev@gmail.com>
Subject: Re: [PATCH v4 2/3] completion: sparse-checkout updates
Date: Mon, 31 Jan 2022 13:37:40 -0800	[thread overview]
Message-ID: <CABPp-BGyH029hidv2t5hmx6=LLt=9m1R_7vZLK7JYizybmaFgQ@mail.gmail.com> (raw)
In-Reply-To: <82bb12d0-0505-1997-21ba-b9863bfdf2c8@gmail.com>

On Mon, Jan 31, 2022 at 12:03 PM Lessley Dennington
<lessleydennington@gmail.com> wrote:
>
> On 1/27/22 5:21 PM, Elijah Newren wrote:
> > On Thu, Jan 27, 2022 at 1:21 PM Lessley Dennington via GitGitGadget
> > <gitgitgadget@gmail.com> wrote:
> >>
> >> From: Lessley Dennington <lessleydennington@gmail.com>
> >>
> >> Fix custom tab completion for the sparse-checkout command. This will
> >> ensure:
> >>
> >> 1. The full list of subcommands is provided when users enter git
> >> sparse-checkout <TAB>.
> >> 2. Subcommand options are tab-completable.
> >> 3. A list of directories (but not files) is provided when users enter git
> >> sparse-checkout add <TAB> or git sparse-checkout set <TAB> in cone mode
> >> only. In non-cone mode, these commands will continue to complete both
> >> directory and file names.
> >>
> >> Failing tests that were added in the previous commit to verify these
> >> scenarios are now passing with these updates.
> >>
> >> Signed-off-by: Lessley Dennington <lessleydennington@gmail.com>
> >> ---
> >>   contrib/completion/git-completion.bash | 15 ++++++++-------
> >>   t/t9902-completion.sh                  |  4 ++--
> >>   2 files changed, 10 insertions(+), 9 deletions(-)
> >>
> >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> >> index c82ccaebcc7..a920cdab201 100644
> >> --- a/contrib/completion/git-completion.bash
> >> +++ b/contrib/completion/git-completion.bash
> >> @@ -2988,21 +2988,22 @@ _git_show_branch ()
> >>
> >>   _git_sparse_checkout ()
> >>   {
> >> -       local subcommands="list init set disable"
> >> +       local subcommands="list init set disable add reapply"
> >>          local subcommand="$(__git_find_on_cmdline "$subcommands")"
> >> +
> >>          if [ -z "$subcommand" ]; then
> >>                  __gitcomp "$subcommands"
> >>                  return
> >>          fi
> >>
> >>          case "$subcommand,$cur" in
> >> -       init,--*)
> >> -               __gitcomp "--cone"
> >> -               ;;
> >> -       set,--*)
> >> -               __gitcomp "--stdin"
> >> +               *,--*)
> >> +                       __gitcomp_builtin sparse-checkout_$subcommand "" "--"
> >>                  ;;
> >> -       *)
> >> +               set,*|add,*)
> >> +                       if [ $(__git config core.sparseCheckoutCone) ]; then
> >> +                               __git_complete_index_file "--directory"
> >> +                       fi
> >
> > --directory only affects how untracked files are presented in `git
> > ls-files` (which _git_complete_index_file uses), and you're not asking
> > for untracked paths at all here.  So you've got a useless flag.
> >
> > Testing this commit out, I can verify that it completes on all tracked
> > paths in the index.  Perhaps that was an intended intermediate step,
> > but if so the commit message needs to be changed since it claims this
> > commit is doing something else.
> >
> > There's also a problem in that `git sparse-checkout --cone path<TAB>`
> > is not using this codepath.  If the user specifies --cone, then we
> > should give them cone-mode-style completion the same as if they
> > already have the config setting set.  Or, we at least need to mention
> > in the commit message that we don't yet correctly check for the
> > `--cone` option to get the right style of completion and are leaving
> > it for future work.
>
> >
> Thank you for your feedback, it is very helpful. As I've been thinking
> about this, it feels like this commit should perhaps be dropped in favor
> of the __gitcomp_directories commit (with some modifications per [1]). I
> know you initially said to add the __gitcomp_directories commit in
> addition to what I had, though, so I wanted to confirm whether removing
> this is the right course of action.
>
> [1]:
> https://lore.kernel.org/git/CABPp-BEq9pTqsy_R_SR1DSgUK58ubNR1Gk4G1RoL8wkadyo6zw@mail.gmail.com/

I think the link you meant was
https://lore.kernel.org/git/CABPp-BG_Jgyr89z_D355Ytzz31J40nBGX=2cr+aXtcf3U1y6Dg@mail.gmail.com/.
Anyway...

I thought the "git ls-tree -d -r --name-only HEAD" step you did
previously was a useful intermediate step because it was really simple
and it highlighted the direction we wanted to go for cone mode -- it
just had some caveats (it was potentially slow, and it has the same
quoting issues that my patch had).  I feel that sometimes an
intermediate step helps reviewers -- both current ones and folks
digging through the code years from now.

I think __git_complete_index_file() is a step in the wrong direction,
and as such adding a patch that uses it does not help people
understand the path to the end goal.  So, if the choice is about
__git_complete_index_file(), then yeah, we shouldn't include that
step.  But your old step 2 might have value.

I think there's a few possible choices you could make to replace your
current patches 2 & 3; including:
  * 1 patch: Just use the __gitcomp_directories commit, modified to
handle the special quoting needed
  * 2 patches: Insert your old "git ls-tree -d -r --name-only HEAD"
patch, made specific to cone mode, mentioning the performance and
quoting shortcomings in the commit message and stating that a
subsequent commit will address those issues.  Then add the
__gitcomp_directories commit, modified to handle special quoting
  * 3 patches: First commit same as the case above.  Then add the
__gitcomp_directories commit without changing it to handle special
path quoting (i.e. the second patch only addresses the performance
shortcoming).  Then add a 3rd commit handling the special path quoting
(addressing the remaining issue).
  * some other split that occurs to you while you do the work.

Of the first three, I have a small preference for replacing patches 2
& 3 from this round with the 3-patch replacement above (making your
series be 4 patches total), because it provides a "10,000 ft overview"
before jumping into the weeds, it naturally documents why a simpler
solution that might occur to folks isn't good enough, it allows
reviewers to focus on just performance or on just special quoting when
reviewing so they don't have to try to guess which end goal each line
of code is working to achieve as they attempt to review.  But I'm
partially guessing at how much work is involved in the special path
quoting, and at how localized each part of the solution might be.  If
either of the performance or special quoting concerns are nicely
localized in the patch anyway, then the separation of commits becomes
less important.  And perhaps a comment in the commit message is good
enough to provide the 10,000 ft overview so you don't need the
explicit "git ls-tree -d -r --name-only HEAD" step.

Anyway, that's how I think about it.  Other than the fact that
__git_complete_index_file() should not be included as a step, any of
the choices could be fine so I'll leave it up to you.

  reply	other threads:[~2022-01-31 21:37 UTC|newest]

Thread overview: 95+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-12-30  0:32 [PATCH 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
2021-12-30  0:32 ` [PATCH 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
2021-12-30 13:43   ` Derrick Stolee
2021-12-30 16:19     ` Lessley Dennington
2021-12-30 17:43       ` Derrick Stolee
2021-12-31 19:27         ` Elijah Newren
2022-01-04 19:19           ` Lessley Dennington
2021-12-30  0:32 ` [PATCH 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
2021-12-30 13:50   ` Derrick Stolee
2021-12-30 16:24     ` Lessley Dennington
2021-12-30 19:26 ` [PATCH v2 0/2] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
2021-12-30 19:26   ` [PATCH v2 1/2] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
2021-12-31 20:03     ` Elijah Newren
2021-12-31 22:20       ` Junio C Hamano
2021-12-31 22:25         ` Elijah Newren
2022-01-04 19:25         ` Lessley Dennington
2022-01-04 20:25           ` Elijah Newren
2022-01-05 14:05             ` Lessley Dennington
2022-01-04 19:24       ` Lessley Dennington
2021-12-30 19:26   ` [PATCH v2 2/2] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
2021-12-31 22:52     ` Elijah Newren
2022-01-04 19:41       ` Lessley Dennington
2022-01-04 20:42         ` Elijah Newren
2022-01-05 20:20           ` Lessley Dennington
2022-01-05 22:55             ` Elijah Newren
2022-01-10 18:59   ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Lessley Dennington via GitGitGadget
2022-01-10 18:59     ` [PATCH v3 1/3] sparse-checkout: custom tab completion tests Lessley Dennington via GitGitGadget
2022-01-10 18:59     ` [PATCH v3 2/3] sparse-checkout: custom tab completion Lessley Dennington via GitGitGadget
2022-01-15  9:57       ` SZEDER Gábor
2022-01-16  1:03         ` Elijah Newren
2022-01-16 22:13           ` Junio C Hamano
2022-01-17 18:14             ` Elijah Newren
2022-01-17 19:40               ` Junio C Hamano
2022-01-18 17:56                 ` Lessley Dennington
2022-01-22  1:07                   ` Lessley Dennington
2022-01-22  1:08                     ` Lessley Dennington
2022-01-22  2:11                       ` Lessley Dennington
2022-01-18 21:02               ` SZEDER Gábor
2022-01-18 21:43                 ` Elijah Newren
2022-01-18 17:59           ` Lessley Dennington
2022-01-18 22:22           ` SZEDER Gábor
2022-01-18 23:30             ` Elijah Newren
2022-01-10 18:59     ` [PATCH v3 3/3] sparse-checkout: limit tab completion to a single level Lessley Dennington via GitGitGadget
2022-01-12 23:43       ` Lessley Dennington
2022-01-13  0:00         ` Junio C Hamano
2022-01-13  0:38         ` Elijah Newren
2022-01-13 19:02           ` Lessley Dennington
2022-01-10 20:38     ` [PATCH v3 0/3] sparse checkout: custom bash completion updates Elijah Newren
2022-01-11 17:17       ` Lessley Dennington
2022-01-11 19:45         ` Taylor Blau
2022-01-12 18:35           ` Lessley Dennington
2022-01-27 21:21     ` [PATCH v4 0/3] completion: sparse-checkout updates Lessley Dennington via GitGitGadget
2022-01-27 21:21       ` [PATCH v4 1/3] completion: add sparse-checkout tests Lessley Dennington via GitGitGadget
2022-01-28  0:08         ` Elijah Newren
2022-01-28  1:56           ` Junio C Hamano
2022-01-28  2:04             ` Elijah Newren
2022-01-27 21:21       ` [PATCH v4 2/3] completion: sparse-checkout updates Lessley Dennington via GitGitGadget
2022-01-28  1:21         ` Elijah Newren
2022-01-31 20:03           ` Lessley Dennington
2022-01-31 21:37             ` Elijah Newren [this message]
2022-01-27 21:21       ` [PATCH v4 3/3] completion: ensure cone mode completion with multiple <TAB>s Lessley Dennington via GitGitGadget
2022-01-28  1:53         ` Elijah Newren
2022-02-03 20:44       ` [PATCH v5 0/3] completion: sparse-checkout updates Lessley Dennington via GitGitGadget
2022-02-03 20:44         ` [PATCH v5 1/3] completion: address sparse-checkout issues Lessley Dennington via GitGitGadget
2022-02-03 23:52           ` Elijah Newren
2022-02-04  0:34             ` Lessley Dennington
2022-02-03 20:44         ` [PATCH v5 2/3] completion: improve sparse-checkout cone mode directory completion Lessley Dennington via GitGitGadget
2022-02-03 20:44         ` [PATCH v5 3/3] completion: handle unusual characters for sparse-checkout Lessley Dennington via GitGitGadget
2022-02-03 23:58           ` Elijah Newren
2022-02-04  0:37             ` Lessley Dennington
2022-02-04  4:25             ` Ævar Arnfjörð Bjarmason
2022-02-04 21:55           ` Junio C Hamano
2022-02-03 21:48         ` [PATCH v5 0/3] completion: sparse-checkout updates Junio C Hamano
2022-02-03 22:17           ` Lessley Dennington
2022-02-03 23:28             ` Junio C Hamano
2022-02-03 23:59               ` Lessley Dennington
2022-02-04  2:43                 ` Lessley Dennington
2022-02-04  3:28                   ` Lessley Dennington
2022-02-04  4:21                   ` Ævar Arnfjörð Bjarmason
2022-02-04  3:26         ` [PATCH v6 " Lessley Dennington via GitGitGadget
2022-02-04  3:26           ` [PATCH v6 1/3] completion: address sparse-checkout issues Lessley Dennington via GitGitGadget
2022-02-04  3:26           ` [PATCH v6 2/3] completion: improve sparse-checkout cone mode directory completion Lessley Dennington via GitGitGadget
2022-02-04  3:26           ` [PATCH v6 3/3] completion: handle unusual characters for sparse-checkout Lessley Dennington via GitGitGadget
2022-02-04  6:05           ` [PATCH v6 0/3] completion: sparse-checkout updates Elijah Newren
2022-02-04 17:04             ` Junio C Hamano
2022-02-04 17:55               ` Elijah Newren
2022-02-04 19:54                 ` Junio C Hamano
2022-02-04 20:01                   ` Elijah Newren
2022-02-04 21:47                     ` Junio C Hamano
2022-02-07 17:31           ` [PATCH v7 " Lessley Dennington via GitGitGadget
2022-02-07 17:31             ` [PATCH v7 1/3] completion: address sparse-checkout issues Lessley Dennington via GitGitGadget
2022-02-07 17:31             ` [PATCH v7 2/3] completion: improve sparse-checkout cone mode directory completion Lessley Dennington via GitGitGadget
2022-02-07 17:31             ` [PATCH v7 3/3] completion: handle unusual characters for sparse-checkout Lessley Dennington via GitGitGadget
2022-04-06  9:42               ` Adam Dinwoodie
2022-02-08  4:16             ` [PATCH v7 0/3] completion: sparse-checkout updates Elijah Newren

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CABPp-BGyH029hidv2t5hmx6=LLt=9m1R_7vZLK7JYizybmaFgQ@mail.gmail.com' \
    --to=newren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=gitster@pobox.com \
    --cc=johannes.schindelin@gmail.com \
    --cc=lessleydennington@gmail.com \
    --cc=me@ttaylorr.com \
    --cc=stolee@gmail.com \
    --cc=szeder.dev@gmail.com \
    --subject='Re: [PATCH v4 2/3] completion: sparse-checkout updates' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).