[PATCH 0/4] Sparse checkout completion fixes

[PATCH 0/4] Sparse checkout completion fixes

[Elijah Newren via GitGitGadget]

This fixes a few issues with tab completion for the sparse-checkout command,
specifically with the "add" and "set" subcommands.

The 4th patch should probably be considered RFC; it is notable in that we
previously discussed a much different proposal and the general problem
area[1], though our discussion was from a limited vantage point and none of
us (myself included) were aware of the full context at the time. In that
thread, Junio gave some helpful general high-level guidelines for
completion. I believe the existing completion rules fail Junio's guidelines
pretty badly and that we thus need to do something else. See the lengthy
commit message. I implement a simple though somewhat suboptimal choice for
that something else (while arguing that it's at least much better than our
current solution), while also documenting with a code comment a much more
involved alternative solution that we could consider in the future. Comments
on this choice welcome.

[1] https://lore.kernel.org/git/xmqqv8yjz5us.fsf@gitster.g/

Elijah Newren (4):
  completion: squelch stray errors in sparse-checkout completion
  completion: fix logic for determining whether cone mode is active
  completion: avoid misleading completions in cone mode
  completion: avoid user confusion in non-cone mode

 contrib/completion/git-completion.bash | 95 +++++++++++++++++++++++++-
 1 file changed, 92 insertions(+), 3 deletions(-)


base-commit: 79f2338b3746d23454308648b2491e5beba4beff
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1349%2Fnewren%2Fsparse-checkout-completion-fixes-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1349/newren/sparse-checkout-completion-fixes-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1349
-- 
gitgitgadget

[PATCH 1/4] completion: squelch stray errors in sparse-checkout completion

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

If, in the root of a project, one types

    git sparse-checkout set --cone ../<TAB>

then an error message of the form

    fatal: ../: '../' is outside repository at '/home/newren/floss/git'

is written to stderr, which munges the users view of their own command.
Squelch such messages.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ba5c395d2d8..6fced40d04c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3014,7 +3014,7 @@ __gitcomp_directories ()
 			COMPREPLY+=("$c/")
 			_found=1
 		fi
-	done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir)
+	done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir 2>/dev/null)
 
 	if [[ $_found == 0 ]] && [[ "$cur" =~ /$ ]]; then
 		# No possible further completions any deeper, so assume we're at
-- 
gitgitgadget

[PATCH 2/4] completion: fix logic for determining whether cone mode is active

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

_git_sparse_checkout() was checking whether we were in cone mode by
checking whether either:

    A) core.sparseCheckoutCone was "true"
    B) "--cone" was specified on the command line

This code has 2 bugs I didn't catch in my review at the time

    1) core.sparseCheckout must be "true" for core.sparseCheckoutCone to
       be relevant (which matters since "git sparse-checkout disable"
       only unsets core.sparseCheckout, not core.sparseCheckoutCone)
    2) The presence of "--no-cone" should override any config setting

Further, I forgot to update this logic as part of 2d95707a02
("sparse-checkout: make --cone the default", 2022-04-22) for the new
default.

Update the code for the new default and make it be more careful in
determining whether to complete based on cone mode or non-cone mode.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6fced40d04c..42e9e0cba8f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3027,6 +3027,7 @@ _git_sparse_checkout ()
 {
 	local subcommands="list init set disable add reapply"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+	local using_cone=true
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
 		return
@@ -3037,8 +3038,15 @@ _git_sparse_checkout ()
 		__gitcomp_builtin sparse-checkout_$subcommand "" "--"
 		;;
 	set,*|add,*)
-		if [ "$(__git config core.sparseCheckoutCone)" == "true" ] ||
-		[ -n "$(__git_find_on_cmdline --cone)" ]; then
+		if [[ "$(__git config core.sparseCheckout)" == "true" &&
+		      "$(__git config core.sparseCheckoutCone)" == "false" &&
+		      -z "$(__git_find_on_cmdline --cone)" ]]; then
+			using_cone=false
+		fi
+		if [[ -n "$(__git_find_on_cmdline --no-cone)" ]]; then
+			using_cone=false
+		fi
+		if [[ "$using_cone" == "true" ]]; then
 			__gitcomp_directories
 		fi
 	esac
-- 
gitgitgadget

[PATCH 3/4] completion: avoid misleading completions in cone mode

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The "set" and "add" subcommands of "sparse-checkout", when in cone mode,
should only complete on directories.  For bash_completion in general,
when no completions are returned for any subcommands, it will often fall
back to standard completion of files and directories as a substitute.
That is not helpful here.  Since we have already looked for all valid
completions, if none are found then falling back to standard bash file
and directory completion is at best actively misleading.  In fact, there
are three different ways it can be actively misleading.  Add a long
comment in the code about how that fallback behavior can deceive, and
disable the fallback by returning a fake result as the sole completion.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 42e9e0cba8f..136faeca1e9 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3020,6 +3020,26 @@ __gitcomp_directories ()
 		# No possible further completions any deeper, so assume we're at
 		# a leaf directory and just consider it complete
 		__gitcomp_direct_append "$cur "
+	elif [[ $_found == 0 ]]; then
+		# No possible completions found.  Avoid falling back to
+		# bash's default file and directory completion, because all
+		# valid completions have already been searched and the
+		# fallbacks can do nothing but mislead.  In fact, they can
+		# mislead in three different ways:
+		#    1) Fallback file completion makes no sense when asking
+		#       for directory completions, as this function does.
+		#    2) Fallback directory completion is bad because
+		#       e.g. "/pro" is invalid and should NOT complete to
+		#       "/proc".
+		#    3) Fallback file/directory completion only completes
+		#       on paths that exist in the current working tree,
+		#       i.e. which are *already* part of their
+		#       sparse-checkout.  Thus, normal file and directory
+		#       completion is always useless for "git
+		#       sparse-checkout add" and is also probelmatic for
+		#       "git sparse-checkout set" unless using it to
+		#       strictly narrow the checkout.
+		COMPREPLY=( "" )
 	fi
 }
 
-- 
gitgitgadget

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

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

It is tempting to think of "files and directories" of the current
directory as valid inputs to the add and set subcommands of git
sparse-checkout.  However, in non-cone mode, they often aren't and using
them as potential completions leads to *many* forms of confusion:

Issue #1. It provides the *wrong* files and directories.

For
    git sparse-checkout add
we always want to add files and directories not currently in our sparse
checkout, which means we want file and directories not currently present
in the current working tree.  Providing the files and directories
currently present is thus always wrong.

For
    git sparse-checkout set
we have a similar problem except in the subset of cases where we are
trying to narrow our checkout to a strict subset of what we already
have.  That is not a very common scenario, especially since it often
does not even happen to be true for the first use of the command; for
years we required users to create a sparse-checkout via
    git sparse-checkout init
    git sparse-checkout set <args...>
(or use a clone option that did the init step for you at clone time).
The init command creates a minimal sparse-checkout with just the
top-level directory present, meaning the set command has to be used to
expand the checkout.  Thus, only in a special and perhaps unusual cases
would any of the suggestions from normal file and directory completion
be appropriate.

Issue #2: Suggesting patterns that lead to warnings is unfriendly.

If the user specifies any regular file and omits the leading '/', then
the sparse-checkout command will warn the user that their command is
problematic and suggest they use a leading slash instead.

Issue #3: Completion gets confused by leading '/', and provides wrong paths.

Users often want to anchor their patterns to the toplevel of the
repository, especially when listing individual files.  There are a
number of reasons for this, but notably even sparse-checkout encourages
them to do so (as noted above).  However, if users do so (via adding a
leading '/' to their pattern), then bash completion will interpret the
leading slash not as a request for a path at the toplevel of the
repository, but as a request for a path at the root of the filesytem.
That means at best that completion cannot help with such paths, and if
it does find any completions, they are almost guaranteed to be wrong.

Issue #4: Suggesting invalid patterns from subdirectories is unfriendly.

There is no per-directory equivalent to .gitignore with
sparse-checkouts.  There is only a single worktree-global
$GIT_DIR/info/sparse-checkout file.  As such, paths to files must be
specified relative to the toplevel of a repository.  Providing
suggestions of paths that are relative to the current working directory,
as bash completion defaults to, is wrong when the current working
directory is not the worktree toplevel directory.

Issue #5: Paths with special characters will be interpreted incorrectly

The entries in the sparse-checkout file are patterns, not paths.  While
most paths also qualify as patterns (though even in such cases it would
be better for users to not use them directly but prefix them with a
leading '/'), there are a variety of special characters that would need
special escaping beyond the normal shell escaping: '*', '?', '\', '[',
']', and any leading '#' or '!'.  If completion suggests any such paths,
users will likely expect them to be treated as an exact path rather than
as a pattern that might match some number of files other than 1.

Because of the combination of the above issues, turn completion off for
the `set` and `add` subcommands of `sparse-checkout` when in non-cone
mode, but leave a NEEDSWORK comment specifying what could theoretically
be done if someone wanted to provide completion rules that were more
helpful than harmful.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 61 ++++++++++++++++++++++++++
 1 file changed, 61 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 136faeca1e9..7d460da2fab 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3068,6 +3068,67 @@ _git_sparse_checkout ()
 		fi
 		if [[ "$using_cone" == "true" ]]; then
 			__gitcomp_directories
+		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.  Bash's default file and directory
+			#        completion is totally useless for "git
+			#        sparse-checkout add" because of this.  It is
+			#        likewise problematic for "git
+			#        sparse-checkout set" except in those subset
+			#        of cases when trying to narrow scope to a
+			#        strict subset of what you already have
+			#        checked out.
+			#
+			#     2. Always provides file/directory completions
+			#        with a prepended leading '/', so that
+			#        files/directories are only searched at the
+			#        relevant level rather than throughout all
+			#        trees in the hierarchy.  Doing this also
+			#        avoids suggesting the user run a
+			#        sparse-checkout command that will result in
+			#        a warning be thrown at the user.
+			#
+			#     3. Does not accidentally search the root of
+			#        the filesystem when a path with a leading
+			#        slash is specified.  ("git sparse-checkout
+			#        add /ho<TAB>" should not complete to
+			#        "/home" but to e.g. "/hooks" if there is a
+			#        "hooks" in the top of the repository.)
+			#
+			#     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.)
+			#
+			#     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
 }
-- 
gitgitgadget

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

[Junio C Hamano]

"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?

> +		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.

> +			#     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/

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.

> +			#     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
>  }

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

[Elijah Newren]

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
> >  }

Re: [PATCH 1/4] completion: squelch stray errors in sparse-checkout completion

[SZEDER Gábor]

On Thu, Nov 23, 2023 at 05:44:05PM +0000, Elijah Newren via GitGitGadget wrote:
> From: Elijah Newren <newren@gmail.com>
> 
> If, in the root of a project, one types
> 
>     git sparse-checkout set --cone ../<TAB>
> 
> then an error message of the form
> 
>     fatal: ../: '../' is outside repository at '/home/newren/floss/git'
> 
> is written to stderr, which munges the users view of their own command.
> Squelch such messages.
> 
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index ba5c395d2d8..6fced40d04c 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -3014,7 +3014,7 @@ __gitcomp_directories ()
>  			COMPREPLY+=("$c/")
>  			_found=1
>  		fi
> -	done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir)
> +	done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir 2>/dev/null)

It would be better to use the __git wrapper instead, like the wast
majority of git invocations in our completion script, because it not
only takes care of squelching standard error, but also takes into
account any -C dir and/or --git-dir options present on the command
line.

e15098a314 (completion: consolidate silencing errors from git
commands, 2017-02-03)


>  
>  	if [[ $_found == 0 ]] && [[ "$cur" =~ /$ ]]; then
>  		# No possible further completions any deeper, so assume we're at
> -- 
> gitgitgadget
> 
> 

Re: [PATCH 1/4] completion: squelch stray errors in sparse-checkout completion

[Elijah Newren]

On Fri, Nov 24, 2023 at 10:39 AM SZEDER Gábor <szeder.dev@gmail.com> wrote:
>
> On Thu, Nov 23, 2023 at 05:44:05PM +0000, Elijah Newren via GitGitGadget wrote:
> > From: Elijah Newren <newren@gmail.com>
> >
> > If, in the root of a project, one types
> >
> >     git sparse-checkout set --cone ../<TAB>
> >
> > then an error message of the form
> >
> >     fatal: ../: '../' is outside repository at '/home/newren/floss/git'
> >
> > is written to stderr, which munges the users view of their own command.
> > Squelch such messages.
> >
> > Signed-off-by: Elijah Newren <newren@gmail.com>
> > ---
> >  contrib/completion/git-completion.bash | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index ba5c395d2d8..6fced40d04c 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -3014,7 +3014,7 @@ __gitcomp_directories ()
> >                       COMPREPLY+=("$c/")
> >                       _found=1
> >               fi
> > -     done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir)
> > +     done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir 2>/dev/null)
>
> It would be better to use the __git wrapper instead, like the wast
> majority of git invocations in our completion script, because it not
> only takes care of squelching standard error, but also takes into
> account any -C dir and/or --git-dir options present on the command
> line.
>
> e15098a314 (completion: consolidate silencing errors from git
> commands, 2017-02-03)

Ooh, nice!  Thanks for the pointer, I was unaware.  I'll make that change.

[PATCH v2 0/4] Sparse checkout completion fixes

[Elijah Newren via GitGitGadget]

This fixes a few issues with tab completion for the sparse-checkout command,
specifically with the "add" and "set" subcommands.

As noted in v1, the 4th patch implements a somewhat suboptimal solution that
at least improves the situation, while also documenting with a code comment
a much more involved alternative solution that we could consider in the
future.

Changes since v1:

 * Use __git wrapper function to squelch errors, as suggested by SZEDER
   Gábor
 * note that we could use the index or HEAD for the more involved solution
   in patch 4.

[1] https://lore.kernel.org/git/xmqqv8yjz5us.fsf@gitster.g/

Elijah Newren (4):
  completion: squelch stray errors in sparse-checkout completion
  completion: fix logic for determining whether cone mode is active
  completion: avoid misleading completions in cone mode
  completion: avoid user confusion in non-cone mode

 contrib/completion/git-completion.bash | 96 +++++++++++++++++++++++++-
 1 file changed, 93 insertions(+), 3 deletions(-)


base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1349%2Fnewren%2Fsparse-checkout-completion-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1349/newren/sparse-checkout-completion-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1349

Range-diff vs v1:

 1:  591c7b8d73b ! 1:  97e20e3b99d completion: squelch stray errors in sparse-checkout completion
     @@ Commit message
              fatal: ../: '../' is outside repository at '/home/newren/floss/git'
      
          is written to stderr, which munges the users view of their own command.
     -    Squelch such messages.
     +    Squelch such messages by using the __git() wrapper, designed for this
     +    purpose; see commit e15098a314 (completion: consolidate silencing errors
     +    from git commands, 2017-02-03) for more on the wrapper.
      
          Signed-off-by: Elijah Newren <newren@gmail.com>
      
     @@ contrib/completion/git-completion.bash: __gitcomp_directories ()
       			_found=1
       		fi
      -	done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir)
     -+	done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir 2>/dev/null)
     ++	done < <(__git ls-tree -z -d --name-only HEAD $_tmp_dir)
       
       	if [[ $_found == 0 ]] && [[ "$cur" =~ /$ ]]; then
       		# No possible further completions any deeper, so assume we're at
 2:  a15fb054579 = 2:  212ba35ed46 completion: fix logic for determining whether cone mode is active
 3:  e8cc5c54e60 = 3:  1cbbcd9097c completion: avoid misleading completions in cone mode
 4:  fe8669a3f4f ! 4:  604f21dc827 completion: avoid user confusion in non-cone mode
     @@ contrib/completion/git-completion.bash: _git_sparse_checkout ()
      +			# completion function which:
      +			#
      +			#     1. Provides completions based on
     -+			#        files/directories that exist in HEAD, not
     -+			#        just those currently present in the working
     -+			#        tree.  Bash's default file and directory
     -+			#        completion is totally useless for "git
     -+			#        sparse-checkout add" because of this.  It is
     -+			#        likewise problematic for "git
     -+			#        sparse-checkout set" except in those subset
     -+			#        of cases when trying to narrow scope to a
     -+			#        strict subset of what you already have
     -+			#        checked out.
     ++			#        files/directories that exist in HEAD (or in
     ++			#        the index since sparse-index isn't possible
     ++			#        in non-cone mode), not just those currently
     ++			#        present in the working tree.  Bash's
     ++			#        default file and directory completion is
     ++			#        totally useless for "git sparse-checkout
     ++			#        add" because of this.  It is likewise
     ++			#        problematic for "git sparse-checkout set"
     ++			#        except in those subset of cases when trying
     ++			#        to narrow scope to a strict subset of what
     ++			#        you already have checked out.
      +			#
      +			#     2. Always provides file/directory completions
      +			#        with a prepended leading '/', so that

-- 
gitgitgadget

[PATCH v2 1/4] completion: squelch stray errors in sparse-checkout completion

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

If, in the root of a project, one types

    git sparse-checkout set --cone ../<TAB>

then an error message of the form

    fatal: ../: '../' is outside repository at '/home/newren/floss/git'

is written to stderr, which munges the users view of their own command.
Squelch such messages by using the __git() wrapper, designed for this
purpose; see commit e15098a314 (completion: consolidate silencing errors
from git commands, 2017-02-03) for more on the wrapper.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 13a39ebd2e7..b8661701718 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3084,7 +3084,7 @@ __gitcomp_directories ()
 			COMPREPLY+=("$c/")
 			_found=1
 		fi
-	done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir)
+	done < <(__git ls-tree -z -d --name-only HEAD $_tmp_dir)
 
 	if [[ $_found == 0 ]] && [[ "$cur" =~ /$ ]]; then
 		# No possible further completions any deeper, so assume we're at
-- 
gitgitgadget

[PATCH v2 2/4] completion: fix logic for determining whether cone mode is active

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

_git_sparse_checkout() was checking whether we were in cone mode by
checking whether either:

    A) core.sparseCheckoutCone was "true"
    B) "--cone" was specified on the command line

This code has 2 bugs I didn't catch in my review at the time

    1) core.sparseCheckout must be "true" for core.sparseCheckoutCone to
       be relevant (which matters since "git sparse-checkout disable"
       only unsets core.sparseCheckout, not core.sparseCheckoutCone)
    2) The presence of "--no-cone" should override any config setting

Further, I forgot to update this logic as part of 2d95707a02
("sparse-checkout: make --cone the default", 2022-04-22) for the new
default.

Update the code for the new default and make it be more careful in
determining whether to complete based on cone mode or non-cone mode.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b8661701718..7aa66c19ede 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3097,6 +3097,7 @@ _git_sparse_checkout ()
 {
 	local subcommands="list init set disable add reapply"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+	local using_cone=true
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
 		return
@@ -3107,8 +3108,15 @@ _git_sparse_checkout ()
 		__gitcomp_builtin sparse-checkout_$subcommand "" "--"
 		;;
 	set,*|add,*)
-		if [ "$(__git config core.sparseCheckoutCone)" == "true" ] ||
-		[ -n "$(__git_find_on_cmdline --cone)" ]; then
+		if [[ "$(__git config core.sparseCheckout)" == "true" &&
+		      "$(__git config core.sparseCheckoutCone)" == "false" &&
+		      -z "$(__git_find_on_cmdline --cone)" ]]; then
+			using_cone=false
+		fi
+		if [[ -n "$(__git_find_on_cmdline --no-cone)" ]]; then
+			using_cone=false
+		fi
+		if [[ "$using_cone" == "true" ]]; then
 			__gitcomp_directories
 		fi
 	esac
-- 
gitgitgadget

[PATCH v2 3/4] completion: avoid misleading completions in cone mode

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The "set" and "add" subcommands of "sparse-checkout", when in cone mode,
should only complete on directories.  For bash_completion in general,
when no completions are returned for any subcommands, it will often fall
back to standard completion of files and directories as a substitute.
That is not helpful here.  Since we have already looked for all valid
completions, if none are found then falling back to standard bash file
and directory completion is at best actively misleading.  In fact, there
are three different ways it can be actively misleading.  Add a long
comment in the code about how that fallback behavior can deceive, and
disable the fallback by returning a fake result as the sole completion.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7aa66c19ede..c614e5d4f07 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3090,6 +3090,26 @@ __gitcomp_directories ()
 		# No possible further completions any deeper, so assume we're at
 		# a leaf directory and just consider it complete
 		__gitcomp_direct_append "$cur "
+	elif [[ $_found == 0 ]]; then
+		# No possible completions found.  Avoid falling back to
+		# bash's default file and directory completion, because all
+		# valid completions have already been searched and the
+		# fallbacks can do nothing but mislead.  In fact, they can
+		# mislead in three different ways:
+		#    1) Fallback file completion makes no sense when asking
+		#       for directory completions, as this function does.
+		#    2) Fallback directory completion is bad because
+		#       e.g. "/pro" is invalid and should NOT complete to
+		#       "/proc".
+		#    3) Fallback file/directory completion only completes
+		#       on paths that exist in the current working tree,
+		#       i.e. which are *already* part of their
+		#       sparse-checkout.  Thus, normal file and directory
+		#       completion is always useless for "git
+		#       sparse-checkout add" and is also probelmatic for
+		#       "git sparse-checkout set" unless using it to
+		#       strictly narrow the checkout.
+		COMPREPLY=( "" )
 	fi
 }
 
-- 
gitgitgadget

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

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

It is tempting to think of "files and directories" of the current
directory as valid inputs to the add and set subcommands of git
sparse-checkout.  However, in non-cone mode, they often aren't and using
them as potential completions leads to *many* forms of confusion:

Issue #1. It provides the *wrong* files and directories.

For
    git sparse-checkout add
we always want to add files and directories not currently in our sparse
checkout, which means we want file and directories not currently present
in the current working tree.  Providing the files and directories
currently present is thus always wrong.

For
    git sparse-checkout set
we have a similar problem except in the subset of cases where we are
trying to narrow our checkout to a strict subset of what we already
have.  That is not a very common scenario, especially since it often
does not even happen to be true for the first use of the command; for
years we required users to create a sparse-checkout via
    git sparse-checkout init
    git sparse-checkout set <args...>
(or use a clone option that did the init step for you at clone time).
The init command creates a minimal sparse-checkout with just the
top-level directory present, meaning the set command has to be used to
expand the checkout.  Thus, only in a special and perhaps unusual cases
would any of the suggestions from normal file and directory completion
be appropriate.

Issue #2: Suggesting patterns that lead to warnings is unfriendly.

If the user specifies any regular file and omits the leading '/', then
the sparse-checkout command will warn the user that their command is
problematic and suggest they use a leading slash instead.

Issue #3: Completion gets confused by leading '/', and provides wrong paths.

Users often want to anchor their patterns to the toplevel of the
repository, especially when listing individual files.  There are a
number of reasons for this, but notably even sparse-checkout encourages
them to do so (as noted above).  However, if users do so (via adding a
leading '/' to their pattern), then bash completion will interpret the
leading slash not as a request for a path at the toplevel of the
repository, but as a request for a path at the root of the filesytem.
That means at best that completion cannot help with such paths, and if
it does find any completions, they are almost guaranteed to be wrong.

Issue #4: Suggesting invalid patterns from subdirectories is unfriendly.

There is no per-directory equivalent to .gitignore with
sparse-checkouts.  There is only a single worktree-global
$GIT_DIR/info/sparse-checkout file.  As such, paths to files must be
specified relative to the toplevel of a repository.  Providing
suggestions of paths that are relative to the current working directory,
as bash completion defaults to, is wrong when the current working
directory is not the worktree toplevel directory.

Issue #5: Paths with special characters will be interpreted incorrectly

The entries in the sparse-checkout file are patterns, not paths.  While
most paths also qualify as patterns (though even in such cases it would
be better for users to not use them directly but prefix them with a
leading '/'), there are a variety of special characters that would need
special escaping beyond the normal shell escaping: '*', '?', '\', '[',
']', and any leading '#' or '!'.  If completion suggests any such paths,
users will likely expect them to be treated as an exact path rather than
as a pattern that might match some number of files other than 1.

Because of the combination of the above issues, turn completion off for
the `set` and `add` subcommands of `sparse-checkout` when in non-cone
mode, but leave a NEEDSWORK comment specifying what could theoretically
be done if someone wanted to provide completion rules that were more
helpful than harmful.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 62 ++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c614e5d4f07..1e07dc8b73e 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3138,6 +3138,68 @@ _git_sparse_checkout ()
 		fi
 		if [[ "$using_cone" == "true" ]]; then
 			__gitcomp_directories
+		else
+			# NEEDSWORK: It might be useful to provide a
+			# completion function which:
+			#
+			#     1. Provides completions based on
+			#        files/directories that exist in HEAD (or in
+			#        the index since sparse-index isn't possible
+			#        in non-cone mode), not just those currently
+			#        present in the working tree.  Bash's
+			#        default file and directory completion is
+			#        totally useless for "git sparse-checkout
+			#        add" because of this.  It is likewise
+			#        problematic for "git sparse-checkout set"
+			#        except in those subset of cases when trying
+			#        to narrow scope to a strict subset of what
+			#        you already have checked out.
+			#
+			#     2. Always provides file/directory completions
+			#        with a prepended leading '/', so that
+			#        files/directories are only searched at the
+			#        relevant level rather than throughout all
+			#        trees in the hierarchy.  Doing this also
+			#        avoids suggesting the user run a
+			#        sparse-checkout command that will result in
+			#        a warning be thrown at the user.
+			#
+			#     3. Does not accidentally search the root of
+			#        the filesystem when a path with a leading
+			#        slash is specified.  ("git sparse-checkout
+			#        add /ho<TAB>" should not complete to
+			#        "/home" but to e.g. "/hooks" if there is a
+			#        "hooks" in the top of the repository.)
+			#
+			#     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.)
+			#
+			#     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
 }
-- 
gitgitgadget

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

[Junio C Hamano]

Elijah Newren <newren@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`.

Thanks; what I missed was exactly what __gitcomp_directories does
not do you explained above.

>> > +                     #     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?

To my mind, the completion is what I as an end user get when I type
<TAB> to help me formulate input that is acceptable by the command.
As I said, I consider it a bug (or UI mistake) in the a command if
it pretends to work inside a subdirecctory but complains when it is
given a path relative to the current directory, so I'd rather prefer
the approach to "fix" the underlying command, but if that is too
much work or cannot be done for whatever reason, the second best
would be to turn whatever we can do to help the end-user input into
a form that is accepted by the command without changing what the
input means.  If it takes more than "appending at the end", that is
fine, at least by me as an end user.

If you are saying "completion code can only append at the end
because we can only return strings to be appended, not the entire
strings, to the readline machinery, so mucking with the start of the
string is not doable", then sorry---I accept that what we cannot do
cannot be done, and in that case you are "not mistaken".

But from the existing use of COMPREPLY[], it didn't look that way
(it seems __gitcomp is equipped to take fixed prefix to all
candidates by passing it in $2 and used to complete names of
configuration variables in a section, but it seems to me that it can
be repurposed when prefixing "$(git rev-parse --show-prefix)" to a
given pathname relative to the $cwd).  And if that can be done, then
you are "not mistaken", but merely being dogmatic and limiting what
your code can do yourself.

>> 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`?

If "set" has a similar UI issue that confuses end-users, then sure,
I do not see a reason why we want to leave it confusing---the
experimental labelling is to allow us to fix these warts more
easily, no?

>   * 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?

I do not know.

>   * In cone mode (the default), should `add-patterns` just be an
> error, since no pattern specification is allowed?

I do not really care.  "add-patterns" is a potential tool you can
use to reduce friction while fixing the UI warts in an experimental
command.

>   * 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`?

Very likely.  If the desired behaviour from the command can only be
had by castrating features, then such a recommendation would not
mean much to end-users anyway, though.

> If so, what have we
> accomplished by adding the new names?

It is valuable for those who do need to go against recommendation
(because the recommendation robs usability from them way too much),
will have much less confusing and working completion when they use
'add' or 'set', no?

> 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.

Oh, if we do honor the $(git rev-parse --show-prefix), then that
changes the equation somewhat.  I got an impression from your log
message or cover letter that it wasn't the case, and that was where
the "if the command is so broken, then completion can add it for the
user" and "if the command is so broken, then fix it to take relative
paths" came from.

[PATCH v3 0/4] Sparse checkout completion fixes

[Elijah Newren via GitGitGadget]

This fixes a few issues with tab completion for the sparse-checkout command,
specifically with the "add" and "set" subcommands.

Changes since v2:

 * For patch 4, provide completion (and startings) of arguments into
   patterns that match files in the index -- as suggested by Junio.

Changes since v1:

 * Use __git wrapper function to squelch errors, as suggested by SZEDER
   Gábor
 * note that we could use the index or HEAD for the more involved solution
   in patch 4.

[1] https://lore.kernel.org/git/xmqqv8yjz5us.fsf@gitster.g/

Elijah Newren (4):
  completion: squelch stray errors in sparse-checkout completion
  completion: fix logic for determining whether cone mode is active
  completion: avoid misleading completions in cone mode
  completion: avoid user confusion in non-cone mode

 contrib/completion/git-completion.bash | 123 ++++++++++++++++++++++++-
 t/t9902-completion.sh                  |   9 +-
 2 files changed, 127 insertions(+), 5 deletions(-)


base-commit: 564d0252ca632e0264ed670534a51d18a689ef5d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1349%2Fnewren%2Fsparse-checkout-completion-fixes-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1349/newren/sparse-checkout-completion-fixes-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1349

Range-diff vs v2:

 1:  97e20e3b99d = 1:  97e20e3b99d completion: squelch stray errors in sparse-checkout completion
 2:  212ba35ed46 = 2:  212ba35ed46 completion: fix logic for determining whether cone mode is active
 3:  1cbbcd9097c = 3:  1cbbcd9097c completion: avoid misleading completions in cone mode
 4:  604f21dc827 < -:  ----------- completion: avoid user confusion in non-cone mode
 -:  ----------- > 4:  89501b366ff completion: avoid user confusion in non-cone mode

-- 
gitgitgadget

[PATCH v3 1/4] completion: squelch stray errors in sparse-checkout completion

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

If, in the root of a project, one types

    git sparse-checkout set --cone ../<TAB>

then an error message of the form

    fatal: ../: '../' is outside repository at '/home/newren/floss/git'

is written to stderr, which munges the users view of their own command.
Squelch such messages by using the __git() wrapper, designed for this
purpose; see commit e15098a314 (completion: consolidate silencing errors
from git commands, 2017-02-03) for more on the wrapper.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 13a39ebd2e7..b8661701718 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3084,7 +3084,7 @@ __gitcomp_directories ()
 			COMPREPLY+=("$c/")
 			_found=1
 		fi
-	done < <(git ls-tree -z -d --name-only HEAD $_tmp_dir)
+	done < <(__git ls-tree -z -d --name-only HEAD $_tmp_dir)
 
 	if [[ $_found == 0 ]] && [[ "$cur" =~ /$ ]]; then
 		# No possible further completions any deeper, so assume we're at
-- 
gitgitgadget

[PATCH v3 2/4] completion: fix logic for determining whether cone mode is active

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

_git_sparse_checkout() was checking whether we were in cone mode by
checking whether either:

    A) core.sparseCheckoutCone was "true"
    B) "--cone" was specified on the command line

This code has 2 bugs I didn't catch in my review at the time

    1) core.sparseCheckout must be "true" for core.sparseCheckoutCone to
       be relevant (which matters since "git sparse-checkout disable"
       only unsets core.sparseCheckout, not core.sparseCheckoutCone)
    2) The presence of "--no-cone" should override any config setting

Further, I forgot to update this logic as part of 2d95707a02
("sparse-checkout: make --cone the default", 2022-04-22) for the new
default.

Update the code for the new default and make it be more careful in
determining whether to complete based on cone mode or non-cone mode.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index b8661701718..7aa66c19ede 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3097,6 +3097,7 @@ _git_sparse_checkout ()
 {
 	local subcommands="list init set disable add reapply"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
+	local using_cone=true
 	if [ -z "$subcommand" ]; then
 		__gitcomp "$subcommands"
 		return
@@ -3107,8 +3108,15 @@ _git_sparse_checkout ()
 		__gitcomp_builtin sparse-checkout_$subcommand "" "--"
 		;;
 	set,*|add,*)
-		if [ "$(__git config core.sparseCheckoutCone)" == "true" ] ||
-		[ -n "$(__git_find_on_cmdline --cone)" ]; then
+		if [[ "$(__git config core.sparseCheckout)" == "true" &&
+		      "$(__git config core.sparseCheckoutCone)" == "false" &&
+		      -z "$(__git_find_on_cmdline --cone)" ]]; then
+			using_cone=false
+		fi
+		if [[ -n "$(__git_find_on_cmdline --no-cone)" ]]; then
+			using_cone=false
+		fi
+		if [[ "$using_cone" == "true" ]]; then
 			__gitcomp_directories
 		fi
 	esac
-- 
gitgitgadget

[PATCH v3 3/4] completion: avoid misleading completions in cone mode

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

The "set" and "add" subcommands of "sparse-checkout", when in cone mode,
should only complete on directories.  For bash_completion in general,
when no completions are returned for any subcommands, it will often fall
back to standard completion of files and directories as a substitute.
That is not helpful here.  Since we have already looked for all valid
completions, if none are found then falling back to standard bash file
and directory completion is at best actively misleading.  In fact, there
are three different ways it can be actively misleading.  Add a long
comment in the code about how that fallback behavior can deceive, and
disable the fallback by returning a fake result as the sole completion.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7aa66c19ede..c614e5d4f07 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3090,6 +3090,26 @@ __gitcomp_directories ()
 		# No possible further completions any deeper, so assume we're at
 		# a leaf directory and just consider it complete
 		__gitcomp_direct_append "$cur "
+	elif [[ $_found == 0 ]]; then
+		# No possible completions found.  Avoid falling back to
+		# bash's default file and directory completion, because all
+		# valid completions have already been searched and the
+		# fallbacks can do nothing but mislead.  In fact, they can
+		# mislead in three different ways:
+		#    1) Fallback file completion makes no sense when asking
+		#       for directory completions, as this function does.
+		#    2) Fallback directory completion is bad because
+		#       e.g. "/pro" is invalid and should NOT complete to
+		#       "/proc".
+		#    3) Fallback file/directory completion only completes
+		#       on paths that exist in the current working tree,
+		#       i.e. which are *already* part of their
+		#       sparse-checkout.  Thus, normal file and directory
+		#       completion is always useless for "git
+		#       sparse-checkout add" and is also probelmatic for
+		#       "git sparse-checkout set" unless using it to
+		#       strictly narrow the checkout.
+		COMPREPLY=( "" )
 	fi
 }
 
-- 
gitgitgadget

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

[Elijah Newren via GitGitGadget]

From: Elijah Newren <newren@gmail.com>

It is tempting to think of "files and directories" of the current
directory as valid inputs to the add and set subcommands of git
sparse-checkout.  However, in non-cone mode, they often aren't and using
them as potential completions leads to *many* forms of confusion:

Issue #1. It provides the *wrong* files and directories.

For
    git sparse-checkout add
we always want to add files and directories not currently in our sparse
checkout, which means we want file and directories not currently present
in the current working tree.  Providing the files and directories
currently present is thus always wrong.

For
    git sparse-checkout set
we have a similar problem except in the subset of cases where we are
trying to narrow our checkout to a strict subset of what we already
have.  That is not a very common scenario, especially since it often
does not even happen to be true for the first use of the command; for
years we required users to create a sparse-checkout via
    git sparse-checkout init
    git sparse-checkout set <args...>
(or use a clone option that did the init step for you at clone time).
The init command creates a minimal sparse-checkout with just the
top-level directory present, meaning the set command has to be used to
expand the checkout.  Thus, only in a special and perhaps unusual cases
would any of the suggestions from normal file and directory completion
be appropriate.

Issue #2: Suggesting patterns that lead to warnings is unfriendly.

If the user specifies any regular file and omits the leading '/', then
the sparse-checkout command will warn the user that their command is
problematic and suggest they use a leading slash instead.

Issue #3: Completion gets confused by leading '/', and provides wrong paths.

Users often want to anchor their patterns to the toplevel of the
repository, especially when listing individual files.  There are a
number of reasons for this, but notably even sparse-checkout encourages
them to do so (as noted above).  However, if users do so (via adding a
leading '/' to their pattern), then bash completion will interpret the
leading slash not as a request for a path at the toplevel of the
repository, but as a request for a path at the root of the filesytem.
That means at best that completion cannot help with such paths, and if
it does find any completions, they are almost guaranteed to be wrong.

Issue #4: Suggesting invalid patterns from subdirectories is unfriendly.

There is no per-directory equivalent to .gitignore with
sparse-checkouts.  There is only a single worktree-global
$GIT_DIR/info/sparse-checkout file.  As such, paths to files must be
specified relative to the toplevel of a repository.  Providing
suggestions of paths that are relative to the current working directory,
as bash completion defaults to, is wrong when the current working
directory is not the worktree toplevel directory.

Issue #5: Paths with special characters will be interpreted incorrectly

The entries in the sparse-checkout file are patterns, not paths.  While
most paths also qualify as patterns (though even in such cases it would
be better for users to not use them directly but prefix them with a
leading '/'), there are a variety of special characters that would need
special escaping beyond the normal shell escaping: '*', '?', '\', '[',
']', and any leading '#' or '!'.  If completion suggests any such paths,
users will likely expect them to be treated as an exact path rather than
as a pattern that might match some number of files other than 1.

However, despite the first four issues, we can note that _if_ users are
using tab completion, then they are probably trying to specify a path in
the index.  As such, we transform their argument into a top-level-rooted
pattern that matches such a file.  For example, if they type:
   git sparse-checkout add Make<TAB>
we could "complete" to
   git sparse-checkout add /Makefile
or, if they ran from the Documentation/technical/ subdirectory:
   git sparse-checkout add m<TAB>
we could "complete" it to:
   git sparse-checkout add /Documentation/technical/multi-pack-index.txt
Note in both cases I use "complete" in quotes, because we actually add
characters both before and after the argument in question, so we are
kind of abusing "bash completions" to be "bash completions AND
beginnings".

The fifth issue is a bit stickier, especially when you consider that we
not only need to deal with escaping issues because of special meanings
of patterns in sparse-checkout & gitignore files, but also that we need
to consider escaping issues due to ls-files needing to sometimes quote
or escape characters, and because the shell needs to escape some
characters.  The multiple interacting forms of escaping could get ugly;
this patch makes no attempt to do so and simply documents that we
decided to not deal with those corner cases for now but at least get the
common cases right.

Signed-off-by: Elijah Newren <newren@gmail.com>
---
 contrib/completion/git-completion.bash | 89 ++++++++++++++++++++++++++
 t/t9902-completion.sh                  |  9 ++-
 2 files changed, 96 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c614e5d4f07..185b47d802f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -3113,6 +3113,93 @@ __gitcomp_directories ()
 	fi
 }
 
+# In non-cone mode, the arguments to {set,add} are supposed to be
+# patterns, relative to the toplevel directory.  These can be any kind
+# of general pattern, like 'subdir/*.c' and we can't complete on all
+# of those.  However, if the user presses Tab to get tab completion, we
+# presume that they are trying to provide a pattern that names a specific
+# path.
+__gitcomp_slash_leading_paths ()
+{
+	local dequoted_word pfx="" cur_ toplevel
+
+	# Since we are dealing with a sparse-checkout, subdirectories may not
+	# exist in the local working copy.  Therefore, we want to run all
+	# ls-files commands relative to the repository toplevel.
+	toplevel="$(git rev-parse --show-toplevel)/"
+
+	__git_dequote "$cur"
+
+	# If the paths provided by the user already start with '/', then
+	# they are considered relative to the toplevel of the repository
+	# already.  If they do not start with /, then we need to adjust
+	# them to start with the appropriate prefix.
+	case "$cur" in
+	/*)
+		cur="${cur:1}"
+		;;
+	*)
+		pfx="$(__git rev-parse --show-prefix)"
+	esac
+
+	# Since sparse-index is limited to cone-mode, in non-cone-mode the
+	# list of valid paths is precisely the cached files in the index.
+	#
+	# NEEDSWORK:
+	#   1) We probably need to take care of cases where ls-files
+	#      responds with special quoting.
+	#   2) We probably need to take care of cases where ${cur} has
+	#      some kind of special quoting.
+	#   3) On top of any quoting from 1 & 2, we have to provide an extra
+	#      level of quoting for any paths that contain a '*', '?', '\',
+	#      '[', ']', or leading '#' or '!' since those will be
+	#      interpreted by sparse-checkout as something other than a
+	#      literal path character.
+	# Since there are two types of quoting here, this might get really
+	# complex.  For now, just punt on all of this...
+	completions="$(__git -C "${toplevel}" -c core.quotePath=false \
+			 ls-files --cached -- "${pfx}${cur}*" \
+			 | sed -e s%^%/% -e 's%$% %')"
+	# Note, above, though that we needed all of the completions to be
+	# prefixed with a '/', and we want to add a space so that bash
+	# completion will actually complete an entry and let us move on to
+	# the next one.
+
+	# Return what we've found.
+	if test -n "$completions"; then
+		# We found some completions; return them
+		local IFS=$'\n'
+		COMPREPLY=($completions)
+	else
+		# Do NOT fall back to bash-style all-local-files-and-dirs
+		# when we find no match.  Such options are worse than
+		# useless:
+		#     1. "git sparse-checkout add" needs paths that are NOT
+		#        currently in the working copy.  "git
+		#        sparse-checkout set" does as well, except in the
+		#        special cases when users are only trying to narrow
+		#        their sparse checkout to a subset of what they
+		#        already have.
+		#
+		#     2. A path like '.config' is ambiguous as to whether
+		#        the user wants all '.config' files throughout the
+		#        tree, or just the one under the current directory.
+		#        It would result in a warning from the
+		#        sparse-checkout command due to this.  As such, all
+		#        completions of paths should be prefixed with a
+		#        '/'.
+		#
+		#     3. We don't want paths prefixed with a '/' to
+		#        complete files in the system root directory, we
+		#        want it to complete on files relative to the
+		#        repository root.
+		#
+		# As such, make sure that NO completions are offered rather
+		# than falling back to bash's default completions.
+		COMPREPLY=( "" )
+	fi
+}
+
 _git_sparse_checkout ()
 {
 	local subcommands="list init set disable add reapply"
@@ -3138,6 +3225,8 @@ _git_sparse_checkout ()
 		fi
 		if [[ "$using_cone" == "true" ]]; then
 			__gitcomp_directories
+		else
+			 __gitcomp_slash_leading_paths
 		fi
 	esac
 }
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a7c3b4eb63a..bbd17f296e4 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1571,7 +1571,7 @@ test_expect_success FUNNYNAMES,!CYGWIN 'cone mode sparse-checkout completes dire
 	)
 '
 
-test_expect_success 'non-cone mode sparse-checkout uses bash completion' '
+test_expect_success 'non-cone mode sparse-checkout gives rooted paths' '
 	# reset sparse-checkout repo to non-cone mode
 	git -C sparse-checkout sparse-checkout disable &&
 	git -C sparse-checkout sparse-checkout set --no-cone &&
@@ -1581,7 +1581,12 @@ test_expect_success 'non-cone mode sparse-checkout uses bash completion' '
 		# expected to be empty since we have not configured
 		# custom completion for non-cone mode
 		test_completion "git sparse-checkout set f" <<-\EOF
-
+		/folder1/0/1/t.txt 
+		/folder1/expected 
+		/folder1/out 
+		/folder1/out_sorted 
+		/folder2/0/t.txt 
+		/folder3/t.txt 
 		EOF
 	)
 '
-- 
gitgitgadget

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

[Elijah Newren]

On Sun, Nov 26, 2023 at 5:39 PM Junio C Hamano <gitster@pobox.com> wrote:
[...]
> >> 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?
>
> To my mind, the completion is what I as an end user get when I type
> <TAB> to help me formulate input that is acceptable by the command.
> As I said, I consider it a bug (or UI mistake) in the a command if
> it pretends to work inside a subdirecctory but complains when it is
> given a path relative to the current directory, so I'd rather prefer
> the approach to "fix" the underlying command, but if that is too
> much work or cannot be done for whatever reason, the second best
> would be to turn whatever we can do to help the end-user input into
> a form that is accepted by the command without changing what the
> input means.  If it takes more than "appending at the end", that is
> fine, at least by me as an end user.
>
> If you are saying "completion code can only append at the end
> because we can only return strings to be appended, not the entire
> strings, to the readline machinery, so mucking with the start of the
> string is not doable", then sorry---I accept that what we cannot do
> cannot be done, and in that case you are "not mistaken".

This was what I thought; that bash completion didn't support this.

> But from the existing use of COMPREPLY[], it didn't look that way
> (it seems __gitcomp is equipped to take fixed prefix to all
> candidates by passing it in $2 and used to complete names of
> configuration variables in a section, but it seems to me that it can
> be repurposed when prefixing "$(git rev-parse --show-prefix)" to a
> given pathname relative to the $cwd).

Ooh, that's really interesting; I had no idea it had this kind of
flexibility.  It does feel like we're abusing "bash completions" to be
both "bash completions AND startings", but I agree that this is a case
where it makes sense to do so.

I changed patch 4 to implement this for non-cone mode, and submitted
v3 with that change.

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

[Junio C Hamano]

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> @@ -1581,7 +1581,12 @@ test_expect_success 'non-cone mode sparse-checkout uses bash completion' '
>  		# expected to be empty since we have not configured
>  		# custom completion for non-cone mode
>  		test_completion "git sparse-checkout set f" <<-\EOF
> -
> +		/folder1/0/1/t.txt 
> +		/folder1/expected 
> +		/folder1/out 
> +		/folder1/out_sorted 
> +		/folder2/0/t.txt 
> +		/folder3/t.txt 
>  		EOF

The "test_completion" helper strips "Z" at the end of its input
lines so that a hunk like this can be written without having to
worry about mail transport eating trailing whitespaces.

I'll squash the following while queuing.

Thanks.

 t/t9902-completion.sh | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5634b78fc5..aa9a614de3 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -1581,12 +1581,12 @@ test_expect_success 'non-cone mode sparse-checkout gives rooted paths' '
 		# expected to be empty since we have not configured
 		# custom completion for non-cone mode
 		test_completion "git sparse-checkout set f" <<-\EOF
-		/folder1/0/1/t.txt
-		/folder1/expected
-		/folder1/out
-		/folder1/out_sorted
-		/folder2/0/t.txt
-		/folder3/t.txt
+		/folder1/0/1/t.txt Z
+		/folder1/expected Z
+		/folder1/out Z
+		/folder1/out_sorted Z
+		/folder2/0/t.txt Z
+		/folder3/t.txt Z
 		EOF
 	)
 '