Re: [PATCH 4/4] completion: avoid user confusion in non-cone mode

[Elijah Newren <newren@gmail.com>]

On Thu, Nov 23, 2023 at 5:19 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> >               if [[ "$using_cone" == "true" ]]; then
> >                       __gitcomp_directories
>
> Hmph, doesn't "Providing the files and directories currently present
> is thus always wrong." apply equally to cone mode?

Absolutely, it definitely applies to cone mode.  We (mostly) fixed
that a long time ago, making it not complete on the files &
directories currently present.  In particular, the
__gitcomp_directories() function highlighted here completes on the
output of `git ls-tree -z -d --name-only HEAD`.

However, before this series, there was a problem when
__gitcomp_directories() finds no possible completions.  In that case,
the code would fall back to bash-completion's default of completing on
all files and directories currently present.  But that was fixed in
patch 3 of this series to avoid that fallback.

This patch, though, isn't about cone mode.  It's about fixing (or at
least improving) non-cone mode.

> > +             else
> > +                     # NEEDSWORK: It might be useful to provide a
> > +                     # completion function which:
> > +                     #
> > +                     #     1. Provides completions based on
> > +                     #        files/directories that exist in HEAD, not
> > +                     #        just those currently present in the working
> > +                     #        tree.
>
> This makes a lot of sense.  May make even more sense with
> s/HEAD/index/, though.

Ooh, interesting.  That wouldn't work with the sparse index (where
paths we want to complete on are currently missing from the index
too), but sparse index is restricted to cone mode, and we're
discussing non-cone-mode here.  So, this might be a basis for a good
alternative.

> > +                     #     4. Provides no completions when run from a
> > +                     #        subdirectory of the repository root.  (If we
> > +                     #        did provide file/directory completions, the
> > +                     #        user would just get a "please run from the
> > +                     #        toplevel directory" error message when they
> > +                     #        ran it.  *Further*, if the user did rerun
> > +                     #        the command from the toplevel, the
> > +                     #        completions we previously provided would
> > +                     #        likely be wrong as they'd be relative to the
> > +                     #        subdirectory rather than the repository
> > +                     #        root.  That could lead to users getting a
> > +                     #        nasty surprise based on trying to use a
> > +                     #        command we helped them create.)
>
> Hmph, would an obvious alternative to (1) check against the HEAD (or
> the index) to see if the prefix string matches an entity at the
> current directory level, and then (2) to prefix the result of the
> previous step with "/$(git rev-parse --show-prefix)" work?  That is
> something like this:
>
>     $ cd t
>     $ git sparse-checkout add help<TAB>
>     ->
>     $ git sparse-checkout add /t/helper/

I thought bash-completion was only for completions, not for startings
as well.  Was I mistaken?

> and when the user gave the full path from the root level, do the
> obvious:
>
>     $ cd t
>     $ git sparse-checkout add /t/help<TAB>
>     ->
>     $ git sparse-checkout add /t/helper/
>
> Another more fundamental approach to avoid "confusion" this bullet
> item tries to side step might be to *fix* the command that gets
> completed.  As "git sparse-checkout --help" is marked as
> EXPERIMENTAL in capital letters, we should be able to say "what was
> traditionally known as 'add' is from now on called 'add-pattern' and
> command line completion would not get in the way; the 'add'
> subcommand now takes only literal paths, not patterns, that are
> relative to the current directory" if we wanted to.

That's interesting...but it opens up a new can of worms:
  * Would we also need both `set-patterns` and `set`, in addition to
`add-patterns` and `add`?
  * In cone mode, the paths passed are literal directories (and only
directories; no individual files), but the thing added is a
telescoping "cone" of leading directories as well.  Does this make it
potentially confusing to users to say that `add` only takes literal
paths?
  * In cone mode (the default), should `add-patterns` just be an
error, since no pattern specification is allowed?
  * In the git-sparse-checkout manual, for performance reasons, we
recommend users _not_ specify individual paths in non-cone mode.
Would our recommendation then be to just not use `add` or `set` and
only use `add-patterns` and `set-patterns`?  If so, what have we
accomplished by adding the new names?

Maybe I'm missing something about your suggestion, but this seems much
more complex than the simple solution we implemented in bb8b5e9a90d
("sparse-checkout: pay attention to prefix for {set, add}",
2022-02-19) for the !core_sparse_checkout_cone case.  I like the
simple solution there, though that simple solution omitted modifying
the completion rules in a way that was consistent (i.e. that returns
nothing when the user is running from a subdirectory).


> > +                     #     5. Provides escaped completions for any paths
> > +                     #        containing a '*', '?', '\', '[', ']', or
> > +                     #        leading '#' or '!'.  (These characters might
> > +                     #        already be escaped to protect from the
> > +                     #        shell, but they need an *extra* layer of
> > +                     #        escaping to prevent the pattern parsing in
> > +                     #        Git from seeing them as special characters.)
> > +                     #
> > +                     # Of course, this would be a lot of work, so for now,
> > +                     # just avoid the many forms of user confusion that
> > +                     # could be caused by providing bad completions by
> > +                     # providing a fake completion to avoid falling back to
> > +                     # bash's normal file and directory completion.
>
> > +                     COMPREPLY=( "" )
> >               fi
> >       esac
> >  }