threads / patch / 40483

patchcompletion: fix completion after 'git -C path'

Subject: [PATCH] completion: fix completion after 'git -C path'

## tl;dr

3 messages between Oct 5, 2015 and Oct 6, 2015. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

SZEDER Gábor· Oct 5, 2015, 12:02 UTC · lore

The main completion function finds the name of the git command by iterating through all the words on the command line in search for the first non-option-looking word. As it is not aware of 'git -C's mandatory path argument, if the '-C path' option is present, 'path' will be the first such word and it will be mistaken for a git command. This breaks the completion script in various ways:

 - If 'path' happens to match one of the commands supported by the
   completion script, then its options will be offered.
 - If 'path' doesn't match a supported command and doesn't contain any
   characters not allowed in Bash identifier names, then the completion
   script does basically nothing and lets Bash to fall back to filename
   completion.
 - Otherwise, if 'path' contains such unallowed characters, then it
   leads to a more or less ugly error in the middle of the command line.
   The standard '/' directory separator is such a character, and it
   happens to trigger one of the uglier errors:
     $ git -C some/path <TAB>sh.exe": declare: `_git_some/path': not a valid identifier
     error: invalid key: alias.some/path

Fix this by skipping 'git -C's mandatory path argument while iterating over the words on the command line. Extend the relevant test with this case and, while at it, with cases that needed similar treatment in the past ('--git-dir', '-c', '--work-tree' and '--namespace'). Additionally, shut up standard error of the 'declare' commands looking for the associated completion function and of the 'git config' query for the aliased command, so if git learns a new option with a mandatory argument in the future, then at least the command line will not be utterly disrupted by those error messages.

Note, that this change merely fixes the breakage related to 'git -C path', but the completion script will not take it into account as it does '--git-dir path'.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 8 ++++----
 t/t9902-completion.sh                  | 7 ++++++-
 2 files changed, 10 insertions(+), 5 deletions(-)
Show changes to 2 files +10 −5

contrib/completion/git-completion.bash, t/t9902-completion.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 482ca84b45..80dc717fe2 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -763,7 +763,7 @@ __git_aliases ()
 __git_aliased_command ()
 {
 	local word cmdline=$(git --git-dir="$(__gitdir)" \
-		config --get "alias.$1")
+		config --get "alias.$1" 2>/dev/null)
 	for word in $cmdline; do
 		case "$word" in
 		\!gitk|gitk)
@@ -2571,7 +2571,7 @@ __git_main ()
 		--git-dir)   ((c++)) ; __git_dir="${words[c]}" ;;
 		--bare)      __git_dir="." ;;
 		--help) command="help"; break ;;
-		-c|--work-tree|--namespace) ((c++)) ;;
+		-c|-C|--work-tree|--namespace) ((c++)) ;;
 		-*) ;;
 		*) command="$i"; break ;;
 		esac
@@ -2604,13 +2604,13 @@ __git_main ()
 	fi
 
 	local completion_func="_git_${command//-/_}"
-	declare -f $completion_func >/dev/null && $completion_func && return
+	declare -f $completion_func >/dev/null 2>/dev/null && $completion_func && return
 
 	local expansion=$(__git_aliased_command "$command")
 	if [ -n "$expansion" ]; then
 		words[1]=$expansion
 		completion_func="_git_${expansion//-/_}"
-		declare -f $completion_func >/dev/null && $completion_func
+		declare -f $completion_func >/dev/null 2>/dev/null && $completion_func
 	fi
 }
 
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 2ba62fbc17..f5a669918d 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -480,7 +480,12 @@ test_expect_success 'general options plus command' '
 	test_completion "git --namespace=foo check" "checkout " &&
 	test_completion "git --paginate check" "checkout " &&
 	test_completion "git --info-path check" "checkout " &&
-	test_completion "git --no-replace-objects check" "checkout "
+	test_completion "git --no-replace-objects check" "checkout " &&
+	test_completion "git --git-dir some/path check" "checkout " &&
+	test_completion "git -c conf.var=value check" "checkout " &&
+	test_completion "git -C some/path check" "checkout " &&
+	test_completion "git --work-tree some/path check" "checkout " &&
+	test_completion "git --namespace name/space check" "checkout "
 '
 
 test_expect_success 'git --help completion' '
-- 
2.6.0.rc2.22.g7128296
Michael J Gruber· Oct 6, 2015, 08:52 UTC · re: SZEDER Gábor · lore

Re: [PATCH] completion: fix completion after 'git -C path'

SZEDER Gábor venit, vidit, dixit 05.10.2015 14:02:
Show 36 quoted lines
> The main completion function finds the name of the git command by
> iterating through all the words on the command line in search for the
> first non-option-looking word.  As it is not aware of 'git -C's
> mandatory path argument, if the '-C path' option is present, 'path' will
> be the first such word and it will be mistaken for a git command.  This
> breaks the completion script in various ways:
> 
>  - If 'path' happens to match one of the commands supported by the
>    completion script, then its options will be offered.
> 
>  - If 'path' doesn't match a supported command and doesn't contain any
>    characters not allowed in Bash identifier names, then the completion
>    script does basically nothing and lets Bash to fall back to filename
>    completion.
> 
>  - Otherwise, if 'path' contains such unallowed characters, then it
>    leads to a more or less ugly error in the middle of the command line.
>    The standard '/' directory separator is such a character, and it
>    happens to trigger one of the uglier errors:
> 
>      $ git -C some/path <TAB>sh.exe": declare: `_git_some/path': not a valid identifier
>      error: invalid key: alias.some/path
> 
> Fix this by skipping 'git -C's mandatory path argument while iterating
> over the words on the command line.  Extend the relevant test with this
> case and, while at it, with cases that needed similar treatment in the
> past ('--git-dir', '-c', '--work-tree' and '--namespace').
> Additionally, shut up standard error of the 'declare' commands looking
> for the associated completion function and of the 'git config' query for
> the aliased command, so if git learns a new option with a mandatory
> argument in the future, then at least the command line will not be
> utterly disrupted by those error messages.
> 
> Note, that this change merely fixes the breakage related to 'git -C
> path', but the completion script will not take it into account as it
> does '--git-dir path'.

I don't understand the "as it does" part. Do you mean that the completion script does '--git-dir path', or that git does it?

In any case, "git -C path ..." is more like "cd path && git ...". That is, completion should take it into account at least when determining GIT_DIR (though -C does not specifiy the git-dir directly), and possibly also for completion of untracked files. Otherwise, it's going by the wrong repo (unless path is a subdir of cwd).

Michael
SZEDER Gábor· Oct 6, 2015, 10:12 UTC · re: Michael J Gruber · lore

Re: [PATCH] completion: fix completion after 'git -C path'

Quoting Michael J Gruber <git@drmicha.warpmail.net>:
Show 41 quoted lines
> SZEDER Gábor venit, vidit, dixit 05.10.2015 14:02:
>> The main completion function finds the name of the git command by
>> iterating through all the words on the command line in search for the
>> first non-option-looking word.  As it is not aware of 'git -C's
>> mandatory path argument, if the '-C path' option is present, 'path' will
>> be the first such word and it will be mistaken for a git command.  This
>> breaks the completion script in various ways:
>>
>>  - If 'path' happens to match one of the commands supported by the
>>    completion script, then its options will be offered.
>>
>>  - If 'path' doesn't match a supported command and doesn't contain any
>>    characters not allowed in Bash identifier names, then the completion
>>    script does basically nothing and lets Bash to fall back to filename
>>    completion.
>>
>>  - Otherwise, if 'path' contains such unallowed characters, then it
>>    leads to a more or less ugly error in the middle of the command line.
>>    The standard '/' directory separator is such a character, and it
>>    happens to trigger one of the uglier errors:
>>
>>      $ git -C some/path <TAB>sh.exe": declare: `_git_some/path':  
>> not a valid identifier
>>      error: invalid key: alias.some/path
>>
>> Fix this by skipping 'git -C's mandatory path argument while iterating
>> over the words on the command line.  Extend the relevant test with this
>> case and, while at it, with cases that needed similar treatment in the
>> past ('--git-dir', '-c', '--work-tree' and '--namespace').
>> Additionally, shut up standard error of the 'declare' commands looking
>> for the associated completion function and of the 'git config' query for
>> the aliased command, so if git learns a new option with a mandatory
>> argument in the future, then at least the command line will not be
>> utterly disrupted by those error messages.
>>
>> Note, that this change merely fixes the breakage related to 'git -C
>> path', but the completion script will not take it into account as it
>> does '--git-dir path'.
>
> I don't understand the "as it does" part. Do you mean that the
> completion script does '--git-dir path', or that git does it?

When the user specifies '--git-dir path' on the command line the completion script respects that (most of the time, I noticed a few missing spots), i.e.

   git --git-dir /somewhere/else/.git log <TAB>

gives you all the refs from the specified repository, which is good. However, that's not the case with '-C /somewhere/else', as it will lead to the error mentioned in the commit message on current git, or will be ignored with this patch.

Show 5 quoted lines
> In any case, "git -C path ..." is more like "cd path && git ...". That
> is, completion should take it into account at least when determining
> GIT_DIR (though -C does not specifiy the git-dir directly), and possibly
> also for completion of untracked files. Otherwise, it's going by the
> wrong repo (unless path is a subdir of cwd).

I agree, it should, but it doesn't. That will be the next step in some future patches.

Gábor

← back to recent threads