[PATCH] completion: complete paths for git send-email

[PATCH] completion: complete paths for git send-email

[Yury Norov (NVIDIA)]

From: Yury Norov <ynorov@nvidia.com>

git send-email accepts either revisions or paths to patch files, but its
Bash completion only offers revisions. This prevents patch files from
being completed. It can also make a prefix such as "0" expand to an
unrelated hexadecimal ref even when matching 0001-*.patch files exist.

In my Linux tree, an attempt to autocomplete the standard-named patch
brings a random hashtag:

 $ ls 0*
 0001-bitmap-drop-bitmap_next_set_region.patch
 $ git send-email 0<Tab>
 $ git send-email 05c69d298c96703741cac9a5cbbf6c53bd55a6e2

Introduce an append variant of __gitcomp_file() and use it to add
filesystem candidates after the existing revision candidates.  Keep the
latter because revisions remain valid send-email arguments.

Add a regression test covering patch files alongside a 40-hex ref.

Assisted-by: Codex <codex@openai.com>
Signed-off-by: Yury Norov <ynorov@nvidia.com>
---
 contrib/completion/git-completion.bash | 29 +++++++++++++++++++-------
 t/t9902-completion.sh                  | 12 ++++++++++-
 2 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e87578771..b7017488d 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -579,21 +579,18 @@ __gitcomp_file_direct ()
 }
 
 # Generates completion reply with compgen from newline-separated possible
-# completion filenames.
+# completion filenames by appending them to the existing list of completion
+# candidates, COMPREPLY.
 # It accepts 1 to 3 arguments:
 # 1: List of possible completion filenames, separated by a single newline.
 # 2: A directory prefix to be added to each possible completion filename
 #    (optional).
 # 3: Generate possible completion matches for this word (optional).
-__gitcomp_file ()
+__gitcomp_file_append ()
 {
 	local IFS=$'\n'
 
-	# XXX does not work when the directory prefix contains a tilde,
-	# since tilde expansion is not applied.
-	# This means that COMPREPLY will be empty and Bash default
-	# completion will be used.
-	__gitcompadd "$1" "${2-}" "${3-$cur}" ""
+	__gitcompappend "$1" "${2-}" "${3-$cur}" ""
 
 	# use a hack to enable file mode in bash < 4
 	compopt -o filenames +o nospace 2>/dev/null ||
@@ -601,6 +598,23 @@ __gitcomp_file ()
 	true
 }
 
+# Generates completion reply with compgen from newline-separated possible
+# completion filenames.
+# It accepts 1 to 3 arguments:
+# 1: List of possible completion filenames, separated by a single newline.
+# 2: A directory prefix to be added to each possible completion filename
+#    (optional).
+# 3: Generate possible completion matches for this word (optional).
+__gitcomp_file ()
+{
+	# XXX does not work when the directory prefix contains a tilde,
+	# since tilde expansion is not applied.
+	# This means that COMPREPLY will be empty and Bash default
+	# completion will be used.
+	COMPREPLY=()
+	__gitcomp_file_append "$@"
+}
+
 # Find the current subcommand for commands that follow the syntax:
 #
 #    git <command> <subcommand>
@@ -2634,6 +2648,7 @@ _git_send_email ()
 		;;
 	esac
 	__git_complete_revlist
+	__gitcomp_file_append "$(compgen -f -- "$cur")"
 }
 
 _git_stage ()
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 55dc9eabf..e87827f21 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -2777,7 +2777,17 @@ test_expect_success PERL 'send-email' '
 	test_completion "git send-email --val" <<-\EOF &&
 	--validate Z
 	EOF
-	test_completion "git send-email ma" "main "
+	test_completion "git send-email ma" "main " &&
+
+	git tag 05c69d298c96703741cac9a5cbbf6c53bd55a6e2 &&
+	test_when_finished "git tag -d 05c69d298c96703741cac9a5cbbf6c53bd55a6e2 &&
+		rm -f 0001-example.patch 0002-example.patch" &&
+	touch 0001-example.patch 0002-example.patch &&
+	test_completion "git send-email 0" <<-\EOF
+	0001-example.patch
+	0002-example.patch
+	05c69d298c96703741cac9a5cbbf6c53bd55a6e2 Z
+	EOF
 '
 
 test_expect_success 'complete files' '
-- 
2.53.0

Re: [PATCH] completion: complete paths for git send-email

[Junio C Hamano]

"Yury Norov (NVIDIA)" <yury.norov@gmail.com> writes:

> From: Yury Norov <ynorov@nvidia.com>
>
> git send-email accepts either revisions or paths to patch files, but its
> Bash completion only offers revisions. This prevents patch files from
> being completed. It can also make a prefix such as "0" expand to an
> unrelated hexadecimal ref even when matching 0001-*.patch files exist.
>
> In my Linux tree, an attempt to autocomplete the standard-named patch
> brings a random hashtag:
>
>  $ ls 0*
>  0001-bitmap-drop-bitmap_next_set_region.patch
>  $ git send-email 0<Tab>
>  $ git send-email 05c69d298c96703741cac9a5cbbf6c53bd55a6e2

Wow.  Even though I use nothing but 'git send-email' when sending my
own patches, I have never noticed this behavior.  I guess that is
primarily because I only use the command via my own wrapper script,
so the usual bash completion kicks in only for filenames in my
workflow.  Since I store my patches two levels deep in my working
tree (for example, '+outgo/topic/0000-cover-letter.txt'), I suspect
that even if I got rid of my wrapper, I would not suffer from this
issue.  An attempt to run 'git send-email +outgo/contrib-doc/0<TAB>'
expanding the trailing '0' into a hexadecimal object name would
indeed be quite annoying.

Good find.

> Introduce an append variant of __gitcomp_file() and use it to add
> filesystem candidates after the existing revision candidates.  Keep the
> latter because revisions remain valid send-email arguments.

OK.  I will need help from those who are more familiar with our
completion code than I am to properly assess this change.  Any
assistance in reviewing this would be appreciated.

> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 55dc9eabf..e87827f21 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -2777,7 +2777,17 @@ test_expect_success PERL 'send-email' '
>  	test_completion "git send-email --val" <<-\EOF &&
>  	--validate Z
>  	EOF
> -	test_completion "git send-email ma" "main "
> +	test_completion "git send-email ma" "main " &&
> +
> +	git tag 05c69d298c96703741cac9a5cbbf6c53bd55a6e2 &&
> +	test_when_finished "git tag -d 05c69d298c96703741cac9a5cbbf6c53bd55a6e2 &&
> +		rm -f 0001-example.patch 0002-example.patch" &&

If the initial 'git tag' fails, 'test_when_finished' is never
registered, and we end up failing to remove the '000?-example.patch'
files.  The usual way to write this is:

 - set up 'test_when_finished' with a body that is written to
   succeed even if the clean-up target is not present (your '-f' in
   'rm -f' is good, as it prevents 'rm' from failing even if
   '0001-example.patch' does not get created); then

 - write the test code that dirties the state (requiring clean-up)
   after registering the 'test_when_finished' handler.

That is, "Prepare the clean-up first, and then you do not have to
worry about making a mess."

By the way, the use of a purely hexadecimal string as a tag or
branch name is highly misleading.  What happens if an object exists
whose name is identical to that tag?  Git offers ways to
disambiguate if you really want to, but I do not see any reason for
a sensible person or workflow to deliberately place oneself in a
situation where such disambiguation becomes necessary.

Of course, that is no excuse for the bug.  Our completion script
should not misbehave, even when confronted with a workflow that uses
funny-looking tags.