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

From: Elijah Newren <newren@gmail.com>
Date: Jan 17, 2022, 18:14 UTC
Message-ID: <CABPp-BErg-RtyycXaRXYfQHEQXA4q-FU9Q6nYkSHJsqL-04oXw@mail.gmail.com>
In-Reply-To: <xmqqv8yjz5us.fsf@gitster.g>

On Sun, Jan 16, 2022 at 2:13 PM Junio C Hamano <gitster@pobox.com> wrote:

Show 11 quoted lines

>
> Elijah Newren <newren@gmail.com> writes:
>
> > ...  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 am not sure if I follow.  I do not think it makes sense to aim for
> enumerating EVERYTHING the command would accept.  But I do not know
> that is the same as "merely what the command swallows".

I don't understand your distinction; I think your second sentence is the same thing I said.

Show 10 quoted lines

> > I don't think that's a useful guide or starting point, so we
> > instead need to discuss what are reasonable completions.
>
> I do not think it is a good idea to refrain from suggesting anything
> that has a possibility of being wring, either, though.  If a path
> that is not a directory (either because it is a file in the current
> checkout, or because it is a directory only in a different branch)
> is given, it might not make sense in the cone-mode for the current
> checkout, but it may well make sense when a different branch is
> checked out.

Completing on files and directories would be a reasonable first cut,
but that's what we already had before this series was proposed.  It
has the downsides of
  * [cone-mode] making the majority of suggested completions wrong
  * [non-cone mode] subtly recommending individually adding all wanted
files, increasing the number of patterns and exacerbating the
quadratic behavior that non-cone mode experiences with every
unpack_trees() call that touches the working tree.
  * [both modes] potentially making it hard to find or select the
valid choices you do want (there are many more files than directories)

But, despite all that, completing on files and directories is a somewhat reasonable first cut.

Lessley was trying to aim higher with her submission, though, and I don't see why she should be dissuaded. Something better would be nice.

>  Or you may not even in the cone-mode, and in which
> case, as SZEDER suggested, a single filename may perfectly make
> sense.  A user who said READM<TAB> and does not see it completed to
> README.md would be quite confused.

I'm not sure I follow.  READM<TAB> already doesn't complete to
README.md in the following example command lines:
   'cd READM<TAB>'
   'ssh READM<TAB>'
That doesn't seem to cause user confusion, so I don't think
disallowing it in cone-mode would cause confusion.  Suggesting it, on
the other hand, may well cause confusion given that cone-mode is
explicitly about directories and is documented as such.

If you're only talking about non-cone mode, then this may be a reasonable objection, though I'm not sure I agree with it even then. In addition to the fact that individual file completion can be detrimental to the user in non-cone mode for performance reasons, the documentation never explicitly states files are acceptable to sparse-checkout {set,add}. It always mentions "directories" and "patterns". We can't complete all patterns, so we have to pick a subset. I don't see why "directories and files" is a better subset to pick than "just directories".

> Are we limiting ourselves to directories only when we know we are in
> the cone-mode and showing both directories and files otherwise?

That is one possibility, though not the one Lessley proposed.

Show 7 quoted lines

> I think the guiding principle ought to be that we show completion
> that
>
>  - is cheap enough to compute in interactive use (e.g. we should
>    refrain from looking for directories in all possible branches,
>    but instead just look at the working tree and possibly in the
>    index),

I agree, except with the suggestion to use the working tree or index
in the case of sparse-checkouts.  The working tree shouldn't be used
because it doesn't have the needed information:
  * `git clone --sparse` starts with zero subdirectories present
  * `set` and `add` are likely being used to change sparsity to
include something not already present

The index shouldn't be used because it is not cheap for interactive use:
  * it contains recursive entries below directories of interest that
have to be filtered out
  * with the sparse-index, it may have sparse-directories hiding what
we want, forcing us to fully inflate the index to find the pieces we
do want

I agree with Lessley's choice of using ls-tree and HEAD to achieve cheap interactive use.

>  - is simple enough to explain to the users to understand what the
>    rules are, and
>
>  - gives useful enough candidates.

I agree with these 3 criteria.

Show 5 quoted lines

> "We only look for directories (without going recursive) in the
> working tree" does satisfy the first two, but I am not sure it is
> more useful than "We show files and directories (without going
> recursive) in the working tree", which also satisfies the first
> two.

Well, Lessley certainly thought directories-only was more useful, and in fact labelled "files and directories" as a bug/issue in her cover letter. Stolee commented on the series and also didn't see anything wrong with that claim.

> Of course, if the completion limits to directories only in a
> repository in the cone-mode, I would not worry about the exclusion
> of non-directories will confuse users.

I'd prefer directories-only for both modes, but could accept directories-only for cone mode and both-files-and-directories for non-cone mode.

Especially since I think we're going to deprecate non-cone mode regardless.