Re: [PATCH 4/4] completion: avoid user confusion in non-cone mode
- From
Elijah Newren <newren@gmail.com>
- Date
- Nov 24, 2023, 15:28 UTC
- Message-ID
- <CABPp-BE73cDq-DBKzsw3R0awiKn5J5LCfdXRULHyPbrS9GEn4Q@mail.gmail.com>
- In-Reply-To
- <xmqqo7fk9cdt.fsf@gitster.g>
On Thu, Nov 23, 2023 at 5:19 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 8 quoted lines
> > "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.
Show 11 quoted lines
> > + 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.
Show 24 quoted lines
> > + # 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?
Show 16 quoted lines
> 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).
Show 18 quoted lines
> > + # 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 > > }