Re: [PATCH v3 2/3] sparse-checkout: custom tab completion

> On Sat, Jan 15, 2022 at 1:57 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
> >
> > On Mon, Jan 10, 2022 at 06:59:51PM +0000, Lessley Dennington via GitGitGadget wrote:
> > > Subject: Re: [PATCH v3 2/3] sparse-checkout: custom tab completion
> >
> > None of these patches touch sparse-checkout, but only the completion
> > script and its tests.  Therefore "completion:" would be a better
> > matching area prefix.
> 
> Thanks for the detailed feedback and guidance in your review.  Very
> helpful.  I'll omit quoting most of it here, but I do want to comment
> on the point about directories.
> 
> ...
> > > 4. A list of directories (but not files) is provided when users enter git
> > > sparse-checkout add <TAB> or git sparse-checkout set <TAB>.
> >
> > Why limit completion only to directories?  Both of those subcommands
> > accept files,
> 
> Discussed in part at [1], but let me give a more detailed answer.

> Both of these commands accept not only directories and files, but also
> nearly arbitrary input as far as I can tell.  (In cone-mode, it'll
> accept anything so long as it doesn't look like a relative path that
> tries to reach above the toplevel directory with '../' sequences.  In
> non-cone mode, I think it accepts completely arbitrary input).  If our
> guide is merely what the command swallows, then we should forgo
> completion for these subcommands, because it's not possible to
> enumerate all possible completions.  I don't think that's a useful
> guide or starting point, so we instead need to discuss what are
> reasonable completions.
> 
> cone-mode works exclusively on directories.  So, in that mode,
> directories are what we want to complete on.  (And if a file is
> specified, cone-mode will treat it as a directory and add expressions
> for including all the files under that "directory", which might be
> confusing.  sparse-checkout doesn't verify it is a diretory, because
> it *might* name a directory in a different branch, including one not
> yet downloaded.  But "might name a directory on another branch" is no
> reason to suggest picking that pathname with completion.)
> 
> In non-cone mode, arbitrary expressions are valid and will be treated
> as gitignore-style expressions.  That again leaves us with either not
> providing completions, or choosing a subset of possible inputs that
> are reasonable suggestions for users.  I prefer the latter, and in
> particular I feel that directories are reasonable suggestions.  In
> contrast, I don't think providing files is helpful, because it
> reinforces the design flaw of non-cone mode.  Non-cone mode has
> quadratic performance baked into its design, and since
> sparse-checkouts are about performance, non-cone mode kind of defeats
> the purpose of the command.

>  (In addition to other problems[2].)  So,
> I think non-cone mode should be deprecated and excised.  Patches
> elsewhere are moving in the direction of deprecation already[3], and
> we've already discussed multiple steps we'll likely take soon
> continuing in that direction.  In the meantime, providing just
> directories for completion seems like a good direction to me.
> 
> [1] https://lore.kernel.org/git/CABPp-BG=wr81CPtW1M12xFN_0dyS8mAZjM6o=77LA20Zge8Xng@mail.gmail.com/
> [2] https://lore.kernel.org/git/CABPp-BF=-1aZd=nFHF6spo7Ksa7f7Wb7ervCt0QvtNitMY=ZBA@mail.gmail.com/
> [3] https://lore.kernel.org/git/0af00779128e594aff0ee4ec5378addeac8e88a2.1642175983.git.gitgitgadget@gmail.com/
> ("This mode is harder to use and less performant, and is thus not
> recommended.")
> 
> > and I think 'git sparse-checkout set README.md' is a
> > perfectly reasonable command.
> 
> Reasonable in what sense?  That it makes it (vastly) easier to
> implement the completion and sparse-checkout set|add will swallow it,
> or that it's something that should actually be recommended for users
> doing sparse-checkouts?  While the former certainly holds, I don't
> think the latter does.