[PATCHv2 00/21] completion: various __gitdir()-related improvements

[PATCHv2 00/21] completion: various __gitdir()-related improvements

[SZEDER Gábor]

This is a long overdue reroll of a bunch of bugfixes, cleanups and
optimizations related to how the completion script finds the path to
the repository and how it uses that path.  Most importantly this
series adds support for completion following 'git -C path', and it
eliminates a few subshells and git processes, for the sake of
fork()+exec() challenged OSes.

The first round is at [1].  It made its way to pu back then, but since
the reroll didn't come it was eventually discarded.

What did NOT change since v1 is that the new option added to 'git
rev-parse' is still called '--absolute-git-dir'.  There was a
suggestion [2] to turn it into an orthogonal '--absolute-dir' option
that works with other path-querying options, too, but I really doubt
it's worth it.  In short, regular scripts don't care, because a
relative path doesn't make any difference for them, and before we do
this orthogonal thing we have to decide a bunch of questions first,
see [3].

Changes since v1:

 - Use our real_path() instead of system realpath() to implement 'git
   rev-parse --absolute-git-dir'.
 - Refactored a bit how __git_refs() determines where it should list
   refs from.
 - Renamed a few refnames and remote in the tests (this accounts for
   the bulk of the interdiff).
 - Misc small adjustments: a few more comments, removed unnecessary
   disambiguating '--', typofix and more consistent quoting.
 - Improved commit messages.
 - Rebased to current master.

The interdiff below is compared to v1 rebased on top of current master.

This series is also available at

  https://github.com/szeder/git completion-gitdir-improvements


[1] - http://public-inbox.org/git/1456440650-32623-1-git-send-email-szeder@ira.uka.de/T/

[2] - http://public-inbox.org/git/CANoM8SXO_Rz_CVOz9ptsaVCzcQ2D1FQrSuFFW4vZ4SdRYMzD=w@mail.gmail.com/

[3] - http://public-inbox.org/git/20160518185825.Horde.EPd2nJNvqEW_VX4b01yWdIr@webmail.informatik.kit.edu/

SZEDER Gábor (21):
  completion: improve __git_refs()'s in-code documentation
  completion tests: don't add test cruft to the test repository
  completion tests: make the $cur variable local to the test helper
    functions
  completion tests: consolidate getting path of current working
    directory
  completion tests: check __gitdir()'s output in the error cases
  completion tests: add tests for the __git_refs() helper function
  completion: ensure that the repository path given on the command line
    exists
  completion: fix most spots not respecting 'git --git-dir=<path>'
  completion: respect 'git --git-dir=<path>' when listing remote refs
  completion: list refs from remote when remote's name matches a
    directory
  completion: don't list 'HEAD' when trying refs completion outside of a
    repo
  completion: list short refs from a remote given as a URL
  completion: don't offer commands when 'git --opt' needs an argument
  completion: fix completion after 'git -C <path>'
  rev-parse: add '--absolute-git-dir' option
  completion: respect 'git -C <path>'
  completion: don't use __gitdir() for git commands
  completion: consolidate silencing errors from git commands
  completion: don't guard git executions with __gitdir()
  completion: extract repository discovery from __gitdir()
  completion: cache the path to the repository

 Documentation/git-rev-parse.txt        |   4 +
 builtin/rev-parse.c                    |  26 +-
 contrib/completion/git-completion.bash | 250 ++++++++++-----
 t/t1500-rev-parse.sh                   |  17 +-
 t/t9902-completion.sh                  | 561 +++++++++++++++++++++++++++++----
 5 files changed, 690 insertions(+), 168 deletions(-)

-- 
2.11.0.555.g967c1bcb3

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 4040b3c86..1967bafba 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -820,10 +820,7 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 					if (!gitdir && !prefix)
 						gitdir = ".git";
 					if (gitdir) {
-						char absolute_path[PATH_MAX];
-						if (!realpath(gitdir, absolute_path))
-							die_errno(_("unable to get absolute path"));
-						puts(absolute_path);
+						puts(real_path(gitdir));
 						continue;
 					}
 				}
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 0184d0ebc..ed06cb621 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -350,41 +350,38 @@ __git_tags ()
 
 # Lists refs from the local (by default) or from a remote repository.
 # It accepts 0, 1 or 2 arguments:
-# 1: The remote to lists refs from (optional; ignored, if set but empty).
+# 1: The remote to list refs from (optional; ignored, if set but empty).
 #    Can be the name of a configured remote, a path, or a URL.
 # 2: In addition to local refs, list unique branches from refs/remotes/ for
 #    'git checkout's tracking DWIMery (optional; ignored, if set but empty).
 __git_refs ()
 {
 	local i hash dir track="${2-}"
-	local from_local=y remote="${1-}" named_remote=n
+	local list_refs_from=path remote="${1-}"
 	local format refs pfx
 
 	__git_find_repo_path
 	dir="$__git_repo_path"
 
-	if [ -z "$dir" ] && [ -z "$remote" ]; then
-		return
-	fi
-
-	if [ -n "$remote" ]; then
+	if [ -z "$remote" ]; then
+		if [ -z "$dir" ]; then
+			return
+		fi
+	else
 		if __git_is_configured_remote "$remote"; then
 			# configured remote takes precedence over a
 			# local directory with the same name
-			from_local=n
-			named_remote=y
+			list_refs_from=remote
+		elif [ -d "$remote/.git" ]; then
+			dir="$remote/.git"
+		elif [ -d "$remote" ]; then
+			dir="$remote"
 		else
-			if [ -d "$remote/.git" ]; then
-				dir="$remote/.git"
-			elif [ -d "$remote" ]; then
-				dir="$remote"
-			else
-				from_local=n
-			fi
+			list_refs_from=url
 		fi
 	fi
 
-	if [ "$from_local" = y ] && [ -d "$dir" ]; then
+	if [ "$list_refs_from" = path ]; then
 		case "$cur" in
 		refs|refs/*)
 			format="refname"
@@ -430,18 +427,18 @@ __git_refs ()
 		done
 		;;
 	*)
-		if [ "$named_remote" = y ]; then
+		if [ "$list_refs_from" = remote ]; then
 			echo "HEAD"
-			__git for-each-ref --format="%(refname:short)" -- \
+			__git for-each-ref --format="%(refname:short)" \
 				"refs/remotes/$remote/" | sed -e "s#^$remote/##"
 		else
 			__git ls-remote "$remote" HEAD \
-				'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' |
+				"refs/tags/*" "refs/heads/*" "refs/remotes/*" |
 			while read -r hash i; do
 				case "$i" in
 				*^{})	;;
 				refs/*)	echo "${i#refs/*/}" ;;
-				*)	echo "$i" ;;
+				*)	echo "$i" ;;  # symbolic refs
 				esac
 			done
 		fi
@@ -910,7 +907,7 @@ __git_get_option_value ()
 	done
 
 	if [ -n "$config_key" ] && [ -z "$result" ]; then
-		result="$(git --git-dir="$(__gitdir)" config "$config_key")"
+		result="$(__git config "$config_key")"
 	fi
 
 	echo "$result"
@@ -2830,9 +2827,12 @@ __git_main ()
 	if [ -z "$command" ]; then
 		case "$prev" in
 		--git-dir|-C|--work-tree)
+			# these need a path argument, let's fall back to
+			# Bash filename completion
 			return
 			;;
 		-c|--namespace)
+			# we don't support completing these options' arguments
 			return
 			;;
 		esac
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 1660759e3..d711bef24 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -519,22 +519,19 @@ test_expect_success '__git_is_configured_remote' '
 '
 
 test_expect_success 'setup for ref completion' '
-	echo content >file1 &&
-	echo more >file2 &&
-	git add file1 file2 &&
-	git commit -m one &&
-	git branch mybranch &&
-	git tag mytag &&
+	git commit --allow-empty -m initial &&
+	git branch matching-branch &&
+	git tag matching-tag &&
 	(
 		cd otherrepo &&
-		>file &&
-		git add file &&
-		git commit -m initial &&
-		git branch branch
+		git commit --allow-empty -m initial &&
+		git branch -m master master-in-other &&
+		git branch branch-in-other &&
+		git tag tag-in-other
 	) &&
-	git remote add remote "$ROOT/otherrepo/.git" &&
-	git update-ref refs/remotes/remote/branch master &&
-	git update-ref refs/remotes/remote/master master &&
+	git remote add other "$ROOT/otherrepo/.git" &&
+	git fetch --no-tags other &&
+	rm -f .git/FETCH_HEAD &&
 	git init thirdrepo
 '
 
@@ -542,10 +539,10 @@ test_expect_success '__git_refs - simple' '
 	cat >expected <<-EOF &&
 	HEAD
 	master
-	mybranch
-	remote/branch
-	remote/master
-	mytag
+	matching-branch
+	other/branch-in-other
+	other/master-in-other
+	matching-tag
 	EOF
 	(
 		cur= &&
@@ -557,7 +554,7 @@ test_expect_success '__git_refs - simple' '
 test_expect_success '__git_refs - full refs' '
 	cat >expected <<-EOF &&
 	refs/heads/master
-	refs/heads/mybranch
+	refs/heads/matching-branch
 	EOF
 	(
 		cur=refs/heads/ &&
@@ -569,8 +566,9 @@ test_expect_success '__git_refs - full refs' '
 test_expect_success '__git_refs - repo given on the command line' '
 	cat >expected <<-EOF &&
 	HEAD
-	branch
-	master
+	branch-in-other
+	master-in-other
+	tag-in-other
 	EOF
 	(
 		__git_dir="$ROOT/otherrepo/.git" &&
@@ -583,8 +581,9 @@ test_expect_success '__git_refs - repo given on the command line' '
 test_expect_success '__git_refs - remote on local file system' '
 	cat >expected <<-EOF &&
 	HEAD
-	branch
-	master
+	branch-in-other
+	master-in-other
+	tag-in-other
 	EOF
 	(
 		cur= &&
@@ -595,11 +594,12 @@ test_expect_success '__git_refs - remote on local file system' '
 
 test_expect_success '__git_refs - remote on local file system - full refs' '
 	cat >expected <<-EOF &&
-	refs/heads/branch
-	refs/heads/master
+	refs/heads/branch-in-other
+	refs/heads/master-in-other
+	refs/tags/tag-in-other
 	EOF
 	(
-		cur=refs/heads/ &&
+		cur=refs/ &&
 		__git_refs otherrepo >"$actual"
 	) &&
 	test_cmp expected "$actual"
@@ -608,24 +608,25 @@ test_expect_success '__git_refs - remote on local file system - full refs' '
 test_expect_success '__git_refs - configured remote' '
 	cat >expected <<-EOF &&
 	HEAD
-	branch
-	master
+	branch-in-other
+	master-in-other
 	EOF
 	(
 		cur= &&
-		__git_refs remote >"$actual"
+		__git_refs other >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success '__git_refs - configured remote - full refs' '
 	cat >expected <<-EOF &&
-	refs/heads/branch
-	refs/heads/master
+	refs/heads/branch-in-other
+	refs/heads/master-in-other
+	refs/tags/tag-in-other
 	EOF
 	(
-		cur=refs/heads/ &&
-		__git_refs remote >"$actual"
+		cur=refs/ &&
+		__git_refs other >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
@@ -633,28 +634,29 @@ test_expect_success '__git_refs - configured remote - full refs' '
 test_expect_success '__git_refs - configured remote - repo given on the command line' '
 	cat >expected <<-EOF &&
 	HEAD
-	branch
-	master
+	branch-in-other
+	master-in-other
 	EOF
 	(
 		cd thirdrepo &&
 		__git_dir="$ROOT/.git" &&
 		cur= &&
-		__git_refs remote >"$actual"
+		__git_refs other >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success '__git_refs - configured remote - full refs - repo given on the command line' '
 	cat >expected <<-EOF &&
-	refs/heads/branch
-	refs/heads/master
+	refs/heads/branch-in-other
+	refs/heads/master-in-other
+	refs/tags/tag-in-other
 	EOF
 	(
 		cd thirdrepo &&
 		__git_dir="$ROOT/.git" &&
-		cur=refs/heads/ &&
-		__git_refs remote >"$actual"
+		cur=refs/ &&
+		__git_refs other >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
@@ -662,14 +664,14 @@ test_expect_success '__git_refs - configured remote - full refs - repo given on
 test_expect_success '__git_refs - configured remote - remote name matches a directory' '
 	cat >expected <<-EOF &&
 	HEAD
-	branch
-	master
+	branch-in-other
+	master-in-other
 	EOF
-	mkdir remote &&
-	test_when_finished "rm -rf remote" &&
+	mkdir other &&
+	test_when_finished "rm -rf other" &&
 	(
 		cur= &&
-		__git_refs remote >"$actual"
+		__git_refs other >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
@@ -677,8 +679,9 @@ test_expect_success '__git_refs - configured remote - remote name matches a dire
 test_expect_success '__git_refs - URL remote' '
 	cat >expected <<-EOF &&
 	HEAD
-	branch
-	master
+	branch-in-other
+	master-in-other
+	tag-in-other
 	EOF
 	(
 		cur= &&
@@ -689,11 +692,12 @@ test_expect_success '__git_refs - URL remote' '
 
 test_expect_success '__git_refs - URL remote - full refs' '
 	cat >expected <<-EOF &&
-	refs/heads/branch
-	refs/heads/master
+	refs/heads/branch-in-other
+	refs/heads/master-in-other
+	refs/tags/tag-in-other
 	EOF
 	(
-		cur=refs/heads/ &&
+		cur=refs/ &&
 		__git_refs "file://$ROOT/otherrepo/.git" >"$actual"
 	) &&
 	test_cmp expected "$actual"
@@ -709,7 +713,7 @@ test_expect_success '__git_refs - non-existing remote' '
 
 test_expect_success '__git_refs - non-existing remote - full refs' '
 	(
-		cur=refs/heads/ &&
+		cur=refs/ &&
 		__git_refs non-existing >"$actual"
 	) &&
 	test_must_be_empty "$actual"
@@ -725,30 +729,41 @@ test_expect_success '__git_refs - non-existing URL remote' '
 
 test_expect_success '__git_refs - non-existing URL remote - full refs' '
 	(
-		cur=refs/heads/ &&
+		cur=refs/ &&
 		__git_refs "file://$ROOT/non-existing" >"$actual"
 	) &&
 	test_must_be_empty "$actual"
 '
 
+test_expect_success '__git_refs - not in a git repository' '
+	(
+		GIT_CEILING_DIRECTORIES="$ROOT" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd subdir &&
+		cur= &&
+		__git_refs >"$actual"
+	) &&
+	test_must_be_empty "$actual"
+'
+
 test_expect_success '__git_refs - unique remote branches for git checkout DWIMery' '
 	cat >expected <<-EOF &&
 	HEAD
 	master
-	mybranch
-	otherremote/ambiguous
-	otherremote/otherbranch
+	matching-branch
+	other/ambiguous
+	other/branch-in-other
+	other/master-in-other
 	remote/ambiguous
-	remote/branch
-	remote/master
-	mytag
-	branch
-	master
-	otherbranch
+	remote/branch-in-remote
+	matching-tag
+	branch-in-other
+	branch-in-remote
+	master-in-other
 	EOF
-	for remote_ref in refs/remotes/remote/ambiguous \
-		refs/remotes/otherremote/ambiguous \
-		refs/remotes/otherremote/otherbranch
+	for remote_ref in refs/remotes/other/ambiguous \
+		refs/remotes/remote/ambiguous \
+		refs/remotes/remote/branch-in-remote
 	do
 		git update-ref $remote_ref master &&
 		test_when_finished "git update-ref -d $remote_ref"
@@ -760,19 +775,10 @@ test_expect_success '__git_refs - unique remote branches for git checkout DWIMer
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__git_refs - not in a git repository' '
-	(
-		GIT_CEILING_DIRECTORIES="$ROOT" &&
-		export GIT_CEILING_DIRECTORIES &&
-		cd subdir &&
-		cur= &&
-		__git_refs >"$actual"
-	) &&
-	test_must_be_empty "$actual"
-'
-
-test_expect_success 'remove configured remote used for refs completion' '
-	git remote remove remote
+test_expect_success 'teardown after ref completion' '
+	git branch -d matching-branch &&
+	git tag -d matching-tag &&
+	git remote remove other
 '
 
 test_expect_success '__git_get_config_variables' '
@@ -898,6 +904,15 @@ test_expect_success 'git --help completion' '
 	test_completion "git --help core" "core-tutorial "
 '
 
+test_expect_success 'setup for integration tests' '
+	echo content >file1 &&
+	echo more >file2 &&
+	git add file1 file2 &&
+	git commit -m one &&
+	git branch mybranch &&
+	git tag mytag
+'
+
 test_expect_success 'checkout completes ref names' '
 	test_completion "git checkout m" <<-\EOF
 	master Z
@@ -908,7 +923,7 @@ test_expect_success 'checkout completes ref names' '
 
 test_expect_success 'git -C <path> checkout uses the right repo' '
 	test_completion "git -C subdir -C subsubdir -C .. -C ../otherrepo checkout b" <<-\EOF
-	branch Z
+	branch-in-other Z
 	EOF
 '
 

[PATCHv2 01/21] completion: improve __git_refs()'s in-code documentation

[SZEDER Gábor]

That "first argument is passed to __gitdir()" statement in particular
is not really helpful, and after this series it won't be the case
anyway.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6721ff80f..ee6fb2259 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -332,9 +332,11 @@ __git_tags ()
 	fi
 }
 
-# __git_refs accepts 0, 1 (to pass to __gitdir), or 2 arguments
-# presence of 2nd argument means use the guess heuristic employed
-# by checkout for tracking branches
+# Lists refs from the local (by default) or from a remote repository.
+# It accepts 0, 1 or 2 arguments:
+# 1: The remote to list refs from (optional; ignored, if set but empty).
+# 2: In addition to local refs, list unique branches from refs/remotes/ for
+#    'git checkout's tracking DWIMery (optional; ignored, if set but empty).
 __git_refs ()
 {
 	local i hash dir="$(__gitdir "${1-}")" track="${2-}"
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 02/21] completion tests: don't add test cruft to the test repository

[SZEDER Gábor]

While preparing commits, three tests added newly created files to the
index using 'git add .', which added not only the files in question
but leftover test cruft from previous tests like the files 'expected'
and 'actual' as well.  Luckily, this had no effect on the tests'
correctness.

Add only the files we are actually interested in.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9902-completion.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a34e55f87..f490c1d05 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -486,7 +486,7 @@ test_expect_success 'git --help completion' '
 test_expect_success 'setup for ref completion' '
 	echo content >file1 &&
 	echo more >file2 &&
-	git add . &&
+	git add file1 file2 &&
 	git commit -m one &&
 	git branch mybranch &&
 	git tag mytag
@@ -517,7 +517,7 @@ test_expect_success '<ref>: completes paths' '
 
 test_expect_success 'complete tree filename with spaces' '
 	echo content >"name with spaces" &&
-	git add . &&
+	git add "name with spaces" &&
 	git commit -m spaces &&
 	test_completion "git show HEAD:nam" <<-\EOF
 	name with spaces Z
@@ -526,7 +526,7 @@ test_expect_success 'complete tree filename with spaces' '
 
 test_expect_success 'complete tree filename with metacharacters' '
 	echo content >"name with \${meta}" &&
-	git add . &&
+	git add "name with \${meta}" &&
 	git commit -m meta &&
 	test_completion "git show HEAD:nam" <<-\EOF
 	name with ${meta} Z
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 03/21] completion tests: make the $cur variable local to the test helper functions

[SZEDER Gábor]

The test helper functions test_gitcomp() and test_gitcomp_nl() leak
the $cur variable into the test environment.  Since this variable has
a special role in the Bash completion script (it holds the word
currently being completed) it influences the behavior of most
completion functions and thus this leakage could interfere with
subsequent tests.  Although there are no such issues in the current
tests, early versions of the new tests that will be added later in
this series suffered because of this.

It's better to play safe and declare $cur local in those test helper
functions.  'local' is bashism, of course, but the tests of the Bash
completion script are run under Bash anyway, and there are already
other variables declared local in this test script.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9902-completion.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index f490c1d05..294a08cfe 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -98,7 +98,7 @@ test_gitcomp ()
 {
 	local -a COMPREPLY &&
 	sed -e 's/Z$//' >expected &&
-	cur="$1" &&
+	local cur="$1" &&
 	shift &&
 	__gitcomp "$@" &&
 	print_comp &&
@@ -113,7 +113,7 @@ test_gitcomp_nl ()
 {
 	local -a COMPREPLY &&
 	sed -e 's/Z$//' >expected &&
-	cur="$1" &&
+	local cur="$1" &&
 	shift &&
 	__gitcomp_nl "$@" &&
 	print_comp &&
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 04/21] completion tests: consolidate getting path of current working directory

[SZEDER Gábor]

Some tests of the __gitdir() helper function use the $TRASH_DIRECTORY
variable in direct path comparisons.  In general this should be
avoided, because it might contain symbolic links.  There happens to be
no issues with this here, however, because those tests use
$TRASH_DIRECTORY both for specifying the expected result and for
specifying input which in turn is just 'echo'ed verbatim.

Other __gitdir() tests ask for the path of the trash directory by
running $(pwd -P) in each test, sometimes even twice in a single test.

Run $(pwd) only once at the beginning of the test script to store the
path of the trash directory in a variable, and use that variable in
all __gitdir() tests.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9902-completion.sh | 44 +++++++++++++++++++++-----------------------
 1 file changed, 21 insertions(+), 23 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 294a08cfe..030a16e77 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -124,15 +124,22 @@ invalid_variable_name='${foo.bar}'
 
 actual="$TRASH_DIRECTORY/actual"
 
+if test_have_prereq MINGW
+then
+	ROOT="$(pwd -W)"
+else
+	ROOT="$(pwd)"
+fi
+
 test_expect_success 'setup for __gitdir tests' '
 	mkdir -p subdir/subsubdir &&
 	git init otherrepo
 '
 
 test_expect_success '__gitdir - from command line (through $__git_dir)' '
-	echo "$TRASH_DIRECTORY/otherrepo/.git" >expected &&
+	echo "$ROOT/otherrepo/.git" >expected &&
 	(
-		__git_dir="$TRASH_DIRECTORY/otherrepo/.git" &&
+		__git_dir="$ROOT/otherrepo/.git" &&
 		__gitdir >"$actual"
 	) &&
 	test_cmp expected "$actual"
@@ -157,7 +164,7 @@ test_expect_success '__gitdir - .git directory in cwd' '
 '
 
 test_expect_success '__gitdir - .git directory in parent' '
-	echo "$(pwd -P)/.git" >expected &&
+	echo "$ROOT/.git" >expected &&
 	(
 		cd subdir/subsubdir &&
 		__gitdir >"$actual"
@@ -175,7 +182,7 @@ test_expect_success '__gitdir - cwd is a .git directory' '
 '
 
 test_expect_success '__gitdir - parent is a .git directory' '
-	echo "$(pwd -P)/.git" >expected &&
+	echo "$ROOT/.git" >expected &&
 	(
 		cd .git/refs/heads &&
 		__gitdir >"$actual"
@@ -184,9 +191,9 @@ test_expect_success '__gitdir - parent is a .git directory' '
 '
 
 test_expect_success '__gitdir - $GIT_DIR set while .git directory in cwd' '
-	echo "$TRASH_DIRECTORY/otherrepo/.git" >expected &&
+	echo "$ROOT/otherrepo/.git" >expected &&
 	(
-		GIT_DIR="$TRASH_DIRECTORY/otherrepo/.git" &&
+		GIT_DIR="$ROOT/otherrepo/.git" &&
 		export GIT_DIR &&
 		__gitdir >"$actual"
 	) &&
@@ -194,9 +201,9 @@ test_expect_success '__gitdir - $GIT_DIR set while .git directory in cwd' '
 '
 
 test_expect_success '__gitdir - $GIT_DIR set while .git directory in parent' '
-	echo "$TRASH_DIRECTORY/otherrepo/.git" >expected &&
+	echo "$ROOT/otherrepo/.git" >expected &&
 	(
-		GIT_DIR="$TRASH_DIRECTORY/otherrepo/.git" &&
+		GIT_DIR="$ROOT/otherrepo/.git" &&
 		export GIT_DIR &&
 		cd subdir &&
 		__gitdir >"$actual"
@@ -206,24 +213,15 @@ test_expect_success '__gitdir - $GIT_DIR set while .git directory in parent' '
 
 test_expect_success '__gitdir - non-existing $GIT_DIR' '
 	(
-		GIT_DIR="$TRASH_DIRECTORY/non-existing" &&
+		GIT_DIR="$ROOT/non-existing" &&
 		export GIT_DIR &&
 		test_must_fail __gitdir
 	)
 '
 
-function pwd_P_W () {
-	if test_have_prereq MINGW
-	then
-		pwd -W
-	else
-		pwd -P
-	fi
-}
-
 test_expect_success '__gitdir - gitfile in cwd' '
-	echo "$(pwd_P_W)/otherrepo/.git" >expected &&
-	echo "gitdir: $(pwd_P_W)/otherrepo/.git" >subdir/.git &&
+	echo "$ROOT/otherrepo/.git" >expected &&
+	echo "gitdir: $ROOT/otherrepo/.git" >subdir/.git &&
 	test_when_finished "rm -f subdir/.git" &&
 	(
 		cd subdir &&
@@ -233,8 +231,8 @@ test_expect_success '__gitdir - gitfile in cwd' '
 '
 
 test_expect_success '__gitdir - gitfile in parent' '
-	echo "$(pwd_P_W)/otherrepo/.git" >expected &&
-	echo "gitdir: $(pwd_P_W)/otherrepo/.git" >subdir/.git &&
+	echo "$ROOT/otherrepo/.git" >expected &&
+	echo "gitdir: $ROOT/otherrepo/.git" >subdir/.git &&
 	test_when_finished "rm -f subdir/.git" &&
 	(
 		cd subdir/subsubdir &&
@@ -244,7 +242,7 @@ test_expect_success '__gitdir - gitfile in parent' '
 '
 
 test_expect_success SYMLINKS '__gitdir - resulting path avoids symlinks' '
-	echo "$(pwd -P)/otherrepo/.git" >expected &&
+	echo "$ROOT/otherrepo/.git" >expected &&
 	mkdir otherrepo/dir &&
 	test_when_finished "rm -rf otherrepo/dir" &&
 	ln -s otherrepo/dir link &&
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 05/21] completion tests: check __gitdir()'s output in the error cases

[SZEDER Gábor]

The __gitdir() helper function shouldn't output anything if not in a
git repository.  The relevant tests only checked its error code, so
extend them to ensure that there's no output.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9902-completion.sh | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 030a16e77..f7f7d49fb 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -215,8 +215,9 @@ test_expect_success '__gitdir - non-existing $GIT_DIR' '
 	(
 		GIT_DIR="$ROOT/non-existing" &&
 		export GIT_DIR &&
-		test_must_fail __gitdir
-	)
+		test_must_fail __gitdir >"$actual"
+	) &&
+	test_must_be_empty "$actual"
 '
 
 test_expect_success '__gitdir - gitfile in cwd' '
@@ -255,7 +256,8 @@ test_expect_success SYMLINKS '__gitdir - resulting path avoids symlinks' '
 '
 
 test_expect_success '__gitdir - not a git repository' '
-	nongit test_must_fail __gitdir
+	nongit test_must_fail __gitdir >"$actual" &&
+	test_must_be_empty "$actual"
 '
 
 test_expect_success '__gitcomp - trailing space - options' '
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 06/21] completion tests: add tests for the __git_refs() helper function

[SZEDER Gábor]

Check how __git_refs() lists refs in different scenarios, i.e.

  - short and full refs,
  - from a local or from a remote repository,
  - remote specified via path, name or URL,
  - with or without a repository specified on the command line,
  - non-existing remote,
  - unique remote branches for 'git checkout's tracking DWIMery,
  - not in a git repository, and
  - interesting combinations of the above.

Seven of these tests expect failure, mostly demonstrating bugs related
to listing refs from a remote repository:

  - ignoring the repository specified on the command line (2 tests),
  - listing refs from the wrong place when the name of a configured
    remote happens to match a directory,
  - listing only 'HEAD' but no short refs from a remote given as URL,
  - listing 'HEAD' even from non-existing remotes (2 tests), and
  - listing 'HEAD' when not in a repository.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 t/t9902-completion.sh | 265 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 264 insertions(+), 1 deletion(-)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index f7f7d49fb..7956cb9b1 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -365,6 +365,269 @@ test_expect_success '__git_remotes - list remotes from $GIT_DIR/remotes and from
 	test_cmp expect actual
 '
 
+test_expect_success 'setup for ref completion' '
+	git commit --allow-empty -m initial &&
+	git branch matching-branch &&
+	git tag matching-tag &&
+	(
+		cd otherrepo &&
+		git commit --allow-empty -m initial &&
+		git branch -m master master-in-other &&
+		git branch branch-in-other &&
+		git tag tag-in-other
+	) &&
+	git remote add other "$ROOT/otherrepo/.git" &&
+	git fetch --no-tags other &&
+	rm -f .git/FETCH_HEAD &&
+	git init thirdrepo
+'
+
+test_expect_success '__git_refs - simple' '
+	cat >expected <<-EOF &&
+	HEAD
+	master
+	matching-branch
+	other/branch-in-other
+	other/master-in-other
+	matching-tag
+	EOF
+	(
+		cur= &&
+		__git_refs >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__git_refs - full refs' '
+	cat >expected <<-EOF &&
+	refs/heads/master
+	refs/heads/matching-branch
+	EOF
+	(
+		cur=refs/heads/ &&
+		__git_refs >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__git_refs - repo given on the command line' '
+	cat >expected <<-EOF &&
+	HEAD
+	branch-in-other
+	master-in-other
+	tag-in-other
+	EOF
+	(
+		__git_dir="$ROOT/otherrepo/.git" &&
+		cur= &&
+		__git_refs >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__git_refs - remote on local file system' '
+	cat >expected <<-EOF &&
+	HEAD
+	branch-in-other
+	master-in-other
+	tag-in-other
+	EOF
+	(
+		cur= &&
+		__git_refs otherrepo >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__git_refs - remote on local file system - full refs' '
+	cat >expected <<-EOF &&
+	refs/heads/branch-in-other
+	refs/heads/master-in-other
+	refs/tags/tag-in-other
+	EOF
+	(
+		cur=refs/ &&
+		__git_refs otherrepo >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__git_refs - configured remote' '
+	cat >expected <<-EOF &&
+	HEAD
+	branch-in-other
+	master-in-other
+	EOF
+	(
+		cur= &&
+		__git_refs other >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__git_refs - configured remote - full refs' '
+	cat >expected <<-EOF &&
+	refs/heads/branch-in-other
+	refs/heads/master-in-other
+	refs/tags/tag-in-other
+	EOF
+	(
+		cur=refs/ &&
+		__git_refs other >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_failure '__git_refs - configured remote - repo given on the command line' '
+	cat >expected <<-EOF &&
+	HEAD
+	branch-in-other
+	master-in-other
+	EOF
+	(
+		cd thirdrepo &&
+		__git_dir="$ROOT/.git" &&
+		cur= &&
+		__git_refs other >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_failure '__git_refs - configured remote - full refs - repo given on the command line' '
+	cat >expected <<-EOF &&
+	refs/heads/branch-in-other
+	refs/heads/master-in-other
+	refs/tags/tag-in-other
+	EOF
+	(
+		cd thirdrepo &&
+		__git_dir="$ROOT/.git" &&
+		cur=refs/ &&
+		__git_refs other >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_failure '__git_refs - configured remote - remote name matches a directory' '
+	cat >expected <<-EOF &&
+	HEAD
+	branch-in-other
+	master-in-other
+	EOF
+	mkdir other &&
+	test_when_finished "rm -rf other" &&
+	(
+		cur= &&
+		__git_refs other >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_failure '__git_refs - URL remote' '
+	cat >expected <<-EOF &&
+	HEAD
+	branch-in-other
+	master-in-other
+	tag-in-other
+	EOF
+	(
+		cur= &&
+		__git_refs "file://$ROOT/otherrepo/.git" >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__git_refs - URL remote - full refs' '
+	cat >expected <<-EOF &&
+	refs/heads/branch-in-other
+	refs/heads/master-in-other
+	refs/tags/tag-in-other
+	EOF
+	(
+		cur=refs/ &&
+		__git_refs "file://$ROOT/otherrepo/.git" >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_failure '__git_refs - non-existing remote' '
+	(
+		cur= &&
+		__git_refs non-existing >"$actual"
+	) &&
+	test_must_be_empty "$actual"
+'
+
+test_expect_success '__git_refs - non-existing remote - full refs' '
+	(
+		cur=refs/ &&
+		__git_refs non-existing >"$actual"
+	) &&
+	test_must_be_empty "$actual"
+'
+
+test_expect_failure '__git_refs - non-existing URL remote' '
+	(
+		cur= &&
+		__git_refs "file://$ROOT/non-existing" >"$actual"
+	) &&
+	test_must_be_empty "$actual"
+'
+
+test_expect_success '__git_refs - non-existing URL remote - full refs' '
+	(
+		cur=refs/ &&
+		__git_refs "file://$ROOT/non-existing" >"$actual"
+	) &&
+	test_must_be_empty "$actual"
+'
+
+test_expect_failure '__git_refs - not in a git repository' '
+	(
+		GIT_CEILING_DIRECTORIES="$ROOT" &&
+		export GIT_CEILING_DIRECTORIES &&
+		cd subdir &&
+		cur= &&
+		__git_refs >"$actual"
+	) &&
+	test_must_be_empty "$actual"
+'
+
+test_expect_success '__git_refs - unique remote branches for git checkout DWIMery' '
+	cat >expected <<-EOF &&
+	HEAD
+	master
+	matching-branch
+	other/ambiguous
+	other/branch-in-other
+	other/master-in-other
+	remote/ambiguous
+	remote/branch-in-remote
+	matching-tag
+	branch-in-other
+	branch-in-remote
+	master-in-other
+	EOF
+	for remote_ref in refs/remotes/other/ambiguous \
+		refs/remotes/remote/ambiguous \
+		refs/remotes/remote/branch-in-remote
+	do
+		git update-ref $remote_ref master &&
+		test_when_finished "git update-ref -d $remote_ref"
+	done &&
+	(
+		cur= &&
+		__git_refs "" 1 >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'teardown after ref completion' '
+	git branch -d matching-branch &&
+	git tag -d matching-tag &&
+	git remote remove other
+'
+
 test_expect_success '__git_get_config_variables' '
 	cat >expect <<-EOF &&
 	name-1
@@ -483,7 +746,7 @@ test_expect_success 'git --help completion' '
 	test_completion "git --help core" "core-tutorial "
 '
 
-test_expect_success 'setup for ref completion' '
+test_expect_success 'setup for integration tests' '
 	echo content >file1 &&
 	echo more >file2 &&
 	git add file1 file2 &&
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 07/21] completion: ensure that the repository path given on the command line exists

[SZEDER Gábor]

The __gitdir() helper function prints the path to the git repository
to its stdout or stays silent and returns with error when it can't
find a repository or when the repository given via $GIT_DIR doesn't
exist.

This is not the case, however, when the path in $__git_dir, i.e. the
path to the repository specified on the command line via 'git
--git-dir=<path>', doesn't exist: __gitdir() still outputs it as if it
were a real existing repository, making some completion functions
believe that they operate on an existing repository.

Check that the path in $__git_dir exists and return with error without
printing anything to stdout if it doesn't.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 1 +
 t/t9902-completion.sh                  | 8 ++++++++
 2 files changed, 9 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ee6fb2259..5b2bd6721 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -40,6 +40,7 @@ __gitdir ()
 {
 	if [ -z "${1-}" ]; then
 		if [ -n "${__git_dir-}" ]; then
+			test -d "$__git_dir" || return 1
 			echo "$__git_dir"
 		elif [ -n "${GIT_DIR-}" ]; then
 			test -d "${GIT_DIR-}" || return 1
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 7956cb9b1..7667baabf 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -211,6 +211,14 @@ test_expect_success '__gitdir - $GIT_DIR set while .git directory in parent' '
 	test_cmp expected "$actual"
 '
 
+test_expect_success '__gitdir - non-existing path in $__git_dir' '
+	(
+		__git_dir="non-existing" &&
+		test_must_fail __gitdir >"$actual"
+	) &&
+	test_must_be_empty "$actual"
+'
+
 test_expect_success '__gitdir - non-existing $GIT_DIR' '
 	(
 		GIT_DIR="$ROOT/non-existing" &&
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 08/21] completion: fix most spots not respecting 'git --git-dir=<path>'

[SZEDER Gábor]

The completion script already respects the path to the repository
specified on the command line most of the time, here we add the
necessary '--git-dir=$(__gitdir)' options to most of the places where
git was executed without it.  The exceptions where said option is not
added are the git invocations:

  - in __git_refs() which are non-trivial and will be the subject of
    the following patch,

  - getting the list of git commands, merge strategies and archive
    formats, because these are independent from the repository and
    thus don't need it, and

  - the 'git rev-parse --git-dir' in __gitdir() itself.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5b2bd6721..295f6de24 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -283,11 +283,13 @@ __gitcomp_file ()
 # argument, and using the options specified in the second argument.
 __git_ls_files_helper ()
 {
+	local dir="$(__gitdir)"
+
 	if [ "$2" == "--committable" ]; then
-		git -C "$1" diff-index --name-only --relative HEAD
+		git --git-dir="$dir" -C "$1" diff-index --name-only --relative HEAD
 	else
 		# NOTE: $2 is not quoted in order to support multiple options
-		git -C "$1" ls-files --exclude-standard $2
+		git --git-dir="$dir" -C "$1" ls-files --exclude-standard $2
 	fi 2>/dev/null
 }
 
@@ -408,7 +410,7 @@ __git_refs2 ()
 __git_refs_remotes ()
 {
 	local i hash
-	git ls-remote "$1" 'refs/heads/*' 2>/dev/null | \
+	git --git-dir="$(__gitdir)" ls-remote "$1" 'refs/heads/*' 2>/dev/null | \
 	while read -r hash i; do
 		echo "$i:refs/remotes/$1/${i#refs/heads/}"
 	done
@@ -1186,7 +1188,7 @@ _git_commit ()
 		return
 	esac
 
-	if git rev-parse --verify --quiet HEAD >/dev/null; then
+	if git --git-dir="$(__gitdir)" rev-parse --verify --quiet HEAD >/dev/null; then
 		__git_complete_index_file "--committable"
 	else
 		# This is the first commit
@@ -1486,7 +1488,7 @@ _git_log ()
 {
 	__git_has_doubledash && return
 
-	local g="$(git rev-parse --git-dir 2>/dev/null)"
+	local g="$(__gitdir)"
 	local merge=""
 	if [ -f "$g/MERGE_HEAD" ]; then
 		merge="--merge"
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 09/21] completion: respect 'git --git-dir=<path>' when listing remote refs

[SZEDER Gábor]

In __git_refs() the git commands listing refs, both short and full,
from a given remote repository are run without giving them the path to
the git repository which might have been specified on the command line
via 'git --git-dir=<path>'.  This is bad, those git commands should
access the 'refs/remotes/<remote>/' hierarchy or the remote and
credentials configuration in that specified repository.

Use the __gitdir() helper only to find the path to the .git directory
and pass the resulting path to the 'git ls-remote' and 'for-each-ref'
executions that list remote refs.  While modifying that 'for-each-ref'
line, remove the superfluous disambiguating doubledash.

Don't use __gitdir() to check that the given remote is on the file
system: basically it performs only a single if statement for us at the
considerable cost of fork()ing a subshell for a command substitution.
We are better off to perform all the necessary checks of the remote in
__git_refs().

Though __git_refs() was the last remaining callsite that passed a
remote to __gitdir(), don't delete __gitdir()'s remote-handling part
yet, just in case some users' custom completion scriptlets depend on
it.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 22 +++++++++++++++++-----
 t/t9902-completion.sh                  |  4 ++--
 2 files changed, 19 insertions(+), 7 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 295f6de24..7d7e8b9d9 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -342,9 +342,21 @@ __git_tags ()
 #    'git checkout's tracking DWIMery (optional; ignored, if set but empty).
 __git_refs ()
 {
-	local i hash dir="$(__gitdir "${1-}")" track="${2-}"
+	local i hash dir="$(__gitdir)" track="${2-}"
+	local list_refs_from=path remote="${1-}"
 	local format refs pfx
-	if [ -d "$dir" ]; then
+
+	if [ -n "$remote" ]; then
+		if [ -d "$remote/.git" ]; then
+			dir="$remote/.git"
+		elif [ -d "$remote" ]; then
+			dir="$remote"
+		else
+			list_refs_from=remote
+		fi
+	fi
+
+	if [ "$list_refs_from" = path ] && [ -d "$dir" ]; then
 		case "$cur" in
 		refs|refs/*)
 			format="refname"
@@ -381,7 +393,7 @@ __git_refs ()
 	fi
 	case "$cur" in
 	refs|refs/*)
-		git ls-remote "$dir" "$cur*" 2>/dev/null | \
+		git --git-dir="$dir" ls-remote "$remote" "$cur*" 2>/dev/null | \
 		while read -r hash i; do
 			case "$i" in
 			*^{}) ;;
@@ -391,8 +403,8 @@ __git_refs ()
 		;;
 	*)
 		echo "HEAD"
-		git for-each-ref --format="%(refname:short)" -- \
-			"refs/remotes/$dir/" 2>/dev/null | sed -e "s#^$dir/##"
+		git --git-dir="$dir" for-each-ref --format="%(refname:short)" \
+			"refs/remotes/$remote/" 2>/dev/null | sed -e "s#^$remote/##"
 		;;
 	esac
 }
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 7667baabf..6e64cd6ba 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -486,7 +486,7 @@ test_expect_success '__git_refs - configured remote - full refs' '
 	test_cmp expected "$actual"
 '
 
-test_expect_failure '__git_refs - configured remote - repo given on the command line' '
+test_expect_success '__git_refs - configured remote - repo given on the command line' '
 	cat >expected <<-EOF &&
 	HEAD
 	branch-in-other
@@ -501,7 +501,7 @@ test_expect_failure '__git_refs - configured remote - repo given on the command
 	test_cmp expected "$actual"
 '
 
-test_expect_failure '__git_refs - configured remote - full refs - repo given on the command line' '
+test_expect_success '__git_refs - configured remote - full refs - repo given on the command line' '
 	cat >expected <<-EOF &&
 	refs/heads/branch-in-other
 	refs/heads/master-in-other
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 10/21] completion: list refs from remote when remote's name matches a directory

[SZEDER Gábor]

If the remote given to __git_refs() happens to match both the name of
a configured remote and the name of a directory in the current working
directory, then that directory is assumed to be a git repository, and
listing refs from that directory will be attempted.  This is wrong,
because in such a situation git commands (e.g. 'git fetch|pull|push
<remote>' whom these refs will eventually be passed to) give
precedence to the configured remote.  Therefore, __git_refs() should
list refs from the configured remote as well.

Add the helper function __git_is_configured_remote() that checks
whether its argument matches the name of a configured remote.  Use
this helper to decide how to handle the remote passed to __git_refs().

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 20 ++++++++++++++++++--
 t/t9902-completion.sh                  | 11 ++++++++++-
 2 files changed, 28 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7d7e8b9d9..fd0ba1f3b 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -347,12 +347,16 @@ __git_refs ()
 	local format refs pfx
 
 	if [ -n "$remote" ]; then
-		if [ -d "$remote/.git" ]; then
+		if __git_is_configured_remote "$remote"; then
+			# configured remote takes precedence over a
+			# local directory with the same name
+			list_refs_from=remote
+		elif [ -d "$remote/.git" ]; then
 			dir="$remote/.git"
 		elif [ -d "$remote" ]; then
 			dir="$remote"
 		else
-			list_refs_from=remote
+			list_refs_from=url
 		fi
 	fi
 
@@ -435,6 +439,18 @@ __git_remotes ()
 	git --git-dir="$d" remote
 }
 
+# Returns true if $1 matches the name of a configured remote, false otherwise.
+__git_is_configured_remote ()
+{
+	local remote
+	for remote in $(__git_remotes); do
+		if [ "$remote" = "$1" ]; then
+			return 0
+		fi
+	done
+	return 1
+}
+
 __git_list_merge_strategies ()
 {
 	git merge -s help 2>&1 |
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 6e64cd6ba..a201b5212 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -373,6 +373,15 @@ test_expect_success '__git_remotes - list remotes from $GIT_DIR/remotes and from
 	test_cmp expect actual
 '
 
+test_expect_success '__git_is_configured_remote' '
+	test_when_finished "git remote remove remote_1" &&
+	git remote add remote_1 git://remote_1 &&
+	test_when_finished "git remote remove remote_2" &&
+	git remote add remote_2 git://remote_2 &&
+	verbose __git_is_configured_remote remote_2 &&
+	test_must_fail __git_is_configured_remote non-existent
+'
+
 test_expect_success 'setup for ref completion' '
 	git commit --allow-empty -m initial &&
 	git branch matching-branch &&
@@ -516,7 +525,7 @@ test_expect_success '__git_refs - configured remote - full refs - repo given on
 	test_cmp expected "$actual"
 '
 
-test_expect_failure '__git_refs - configured remote - remote name matches a directory' '
+test_expect_success '__git_refs - configured remote - remote name matches a directory' '
 	cat >expected <<-EOF &&
 	HEAD
 	branch-in-other
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 11/21] completion: don't list 'HEAD' when trying refs completion outside of a repo

[SZEDER Gábor]

When refs completion is attempted while not in a git repository, the
completion script offers 'HEAD' erroneously.

Check early in __git_refs() that there is either a repository or a
remote to work on, and return early if neither is given.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 8 ++++++--
 t/t9902-completion.sh                  | 2 +-
 2 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index fd0ba1f3b..6b489b7c8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -346,7 +346,11 @@ __git_refs ()
 	local list_refs_from=path remote="${1-}"
 	local format refs pfx
 
-	if [ -n "$remote" ]; then
+	if [ -z "$remote" ]; then
+		if [ -z "$dir" ]; then
+			return
+		fi
+	else
 		if __git_is_configured_remote "$remote"; then
 			# configured remote takes precedence over a
 			# local directory with the same name
@@ -360,7 +364,7 @@ __git_refs ()
 		fi
 	fi
 
-	if [ "$list_refs_from" = path ] && [ -d "$dir" ]; then
+	if [ "$list_refs_from" = path ]; then
 		case "$cur" in
 		refs|refs/*)
 			format="refname"
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index a201b5212..5b4defaa5 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -599,7 +599,7 @@ test_expect_success '__git_refs - non-existing URL remote - full refs' '
 	test_must_be_empty "$actual"
 '
 
-test_expect_failure '__git_refs - not in a git repository' '
+test_expect_success '__git_refs - not in a git repository' '
 	(
 		GIT_CEILING_DIRECTORIES="$ROOT" &&
 		export GIT_CEILING_DIRECTORIES &&
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 12/21] completion: list short refs from a remote given as a URL

[SZEDER Gábor]

e832f5c09680 (completion: avoid ls-remote in certain scenarios,
2013-05-28) turned a 'git ls-remote <remote>' query into a 'git
for-each-ref refs/remotes/<remote>/' to improve responsiveness of
remote refs completion by avoiding potential network communication.
However, it inadvertently made impossible to complete short refs from
a remote given as a URL, e.g. 'git fetch git://server.com/repo.git
<TAB>', because there is, of course, no such thing as
'refs/remotes/git://server.com/repo.git'.

Since the previous commit we tell apart configured remotes, i.e. those
that can have a hierarchy under 'refs/remotes/', from others that
don't, including remotes given as URL, so we know when we can't use
the faster 'git for-each-ref'-based approach.

Resurrect the old, pre-e832f5c09680 'git ls-remote'-based code for the
latter case to support listing short refs from remotes given as a URL.
The code is slightly updated from the original to

  - take into account the path to the repository given on the command
    line (if any), and
  - omit 'ORIG_HEAD' from the query, as 'git ls-remote' will never
    list it anyway.

When the remote given to __git_refs() doesn't exist, then it will be
handled by this resurrected 'git ls-remote' query.  This code path
doesn't list 'HEAD' unconditionally, which has the nice side effect of
fixing two more expected test failures.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 19 ++++++++++++++++---
 t/t9902-completion.sh                  |  6 +++---
 2 files changed, 19 insertions(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 6b489b7c8..4ded44977 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -338,6 +338,7 @@ __git_tags ()
 # Lists refs from the local (by default) or from a remote repository.
 # It accepts 0, 1 or 2 arguments:
 # 1: The remote to list refs from (optional; ignored, if set but empty).
+#    Can be the name of a configured remote, a path, or a URL.
 # 2: In addition to local refs, list unique branches from refs/remotes/ for
 #    'git checkout's tracking DWIMery (optional; ignored, if set but empty).
 __git_refs ()
@@ -410,9 +411,21 @@ __git_refs ()
 		done
 		;;
 	*)
-		echo "HEAD"
-		git --git-dir="$dir" for-each-ref --format="%(refname:short)" \
-			"refs/remotes/$remote/" 2>/dev/null | sed -e "s#^$remote/##"
+		if [ "$list_refs_from" = remote ]; then
+			echo "HEAD"
+			git --git-dir="$dir" for-each-ref --format="%(refname:short)" \
+				"refs/remotes/$remote/" 2>/dev/null | sed -e "s#^$remote/##"
+		else
+			git --git-dir="$dir" ls-remote "$remote" HEAD \
+				"refs/tags/*" "refs/heads/*" "refs/remotes/*" 2>/dev/null |
+			while read -r hash i; do
+				case "$i" in
+				*^{})	;;
+				refs/*)	echo "${i#refs/*/}" ;;
+				*)	echo "$i" ;;  # symbolic refs
+				esac
+			done
+		fi
 		;;
 	esac
 }
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5b4defaa5..500505dca 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -540,7 +540,7 @@ test_expect_success '__git_refs - configured remote - remote name matches a dire
 	test_cmp expected "$actual"
 '
 
-test_expect_failure '__git_refs - URL remote' '
+test_expect_success '__git_refs - URL remote' '
 	cat >expected <<-EOF &&
 	HEAD
 	branch-in-other
@@ -567,7 +567,7 @@ test_expect_success '__git_refs - URL remote - full refs' '
 	test_cmp expected "$actual"
 '
 
-test_expect_failure '__git_refs - non-existing remote' '
+test_expect_success '__git_refs - non-existing remote' '
 	(
 		cur= &&
 		__git_refs non-existing >"$actual"
@@ -583,7 +583,7 @@ test_expect_success '__git_refs - non-existing remote - full refs' '
 	test_must_be_empty "$actual"
 '
 
-test_expect_failure '__git_refs - non-existing URL remote' '
+test_expect_success '__git_refs - non-existing URL remote' '
 	(
 		cur= &&
 		__git_refs "file://$ROOT/non-existing" >"$actual"
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 13/21] completion: don't offer commands when 'git --opt' needs an argument

[SZEDER Gábor]

The main git options '--git-dir', '-c', '-C', '--worktree' and
'--namespace' require an argument, but attempting completion right
after them lists git commands.

Don't offer anything right after these options, thus let Bash fall
back to filename completion, because

  - the three options '--git-dir', '-C' and '--worktree' do actually
    require a path argument, and

  - we don't complete the required argument of '-c' and '--namespace',
    and in that case the "standard" behavior of our completion script
    is to not offer anything, but fall back to filename completion.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 4ded44977..7d25b33b8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2808,6 +2808,17 @@ __git_main ()
 	done
 
 	if [ -z "$command" ]; then
+		case "$prev" in
+		--git-dir|-C|--work-tree)
+			# these need a path argument, let's fall back to
+			# Bash filename completion
+			return
+			;;
+		-c|--namespace)
+			# we don't support completing these options' arguments
+			return
+			;;
+		esac
 		case "$cur" in
 		--*)   __gitcomp "
 			--paginate
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 14/21] completion: fix completion after 'git -C <path>'

[SZEDER Gábor]

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 completion in various ways:

 - If 'path' happens to match one of the commands supported by the
   completion script, then options of that command 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 Bash in turn falls
   back to filename completion for all subsequent words.

 - Otherwise, if 'path' does contain such an unallowed character, then
   it leads to a more or less ugly error message 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, silence the standard error of the 'declare' builtins
looking for the completion function associated with the git command
and of the 'git config' query for the aliased command.  So if git ever
learns a new option with a mandatory argument in the future, then,
though the completion script will again misbehave, at least the
command line will not be utterly disrupted by those error messages.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 8 ++++----
 t/t9902-completion.sh                  | 7 ++++++-
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7d25b33b8..ac5eb9ced 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -816,7 +816,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)
@@ -2800,7 +2800,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
@@ -2844,13 +2844,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 500505dca..be2ed8704 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -755,7 +755,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.11.0.555.g967c1bcb3

[PATCHv2 15/21] rev-parse: add '--absolute-git-dir' option

[SZEDER Gábor]

The output of 'git rev-parse --git-dir' can be either a relative or an
absolute path, depending on whether the current working directory is
at the top of the worktree or the .git directory or not, or how the
path to the repository is specified via the '--git-dir=<path>' option
or the $GIT_DIR environment variable.  And if that output is a
relative path, then it is relative to the directory where any 'git
-C <path>' options might have led us.

This doesn't matter at all for regular scripts, because the git
wrapper automatically takes care of changing directories according to
the '-C <path>' options, and the scripts can then simply follow any
path returned by 'git rev-parse --git-dir', even if it's a relative
path.

Our Bash completion script, however, is unique in that it must run
directly in the user's interactive shell environment.  This means that
it's not executed through the git wrapper and would have to take care
of any '-C <path> options on its own, and it can't just change
directories as it pleases.  Consequently, adding support for taking
any '-C <path>' options on the command line into account during
completion turned out to be considerably more difficult, error prone
and required more subshells and git processes when it had to cope with
a relative path to the .git directory.

Help this rather special use case and teach 'git rev-parse' a new
'--absolute-git-dir' option which always outputs a canonicalized
absolute path to the .git directory, regardless of whether the path is
discovered automatically or is specified via $GIT_DIR or 'git
--git-dir=<path>'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/git-rev-parse.txt |  4 ++++
 builtin/rev-parse.c             | 26 ++++++++++++++++++--------
 t/t1500-rev-parse.sh            | 17 +++++++++--------
 3 files changed, 31 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 7241e9689..91c02b8c8 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -217,6 +217,10 @@ If `$GIT_DIR` is not defined and the current directory
 is not detected to lie in a Git repository or work tree
 print a message to stderr and exit with nonzero status.
 
+--absolute-git-dir::
+	Like `--git-dir`, but its output is always the canonicalized
+	absolute path.
+
 --git-common-dir::
 	Show `$GIT_COMMON_DIR` if defined, else `$GIT_DIR`.
 
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index ff13e59e1..1967bafba 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -802,17 +802,27 @@ int cmd_rev_parse(int argc, const char **argv, const char *prefix)
 				putchar('\n');
 				continue;
 			}
-			if (!strcmp(arg, "--git-dir")) {
+			if (!strcmp(arg, "--git-dir") ||
+			    !strcmp(arg, "--absolute-git-dir")) {
 				const char *gitdir = getenv(GIT_DIR_ENVIRONMENT);
 				char *cwd;
 				int len;
-				if (gitdir) {
-					puts(gitdir);
-					continue;
-				}
-				if (!prefix) {
-					puts(".git");
-					continue;
+				if (arg[2] == 'g') {	/* --git-dir */
+					if (gitdir) {
+						puts(gitdir);
+						continue;
+					}
+					if (!prefix) {
+						puts(".git");
+						continue;
+					}
+				} else {		/* --absolute-git-dir */
+					if (!gitdir && !prefix)
+						gitdir = ".git";
+					if (gitdir) {
+						puts(real_path(gitdir));
+						continue;
+					}
 				}
 				cwd = xgetcwd();
 				len = strlen(cwd);
diff --git a/t/t1500-rev-parse.sh b/t/t1500-rev-parse.sh
index 038e24c40..8b62ed85b 100755
--- a/t/t1500-rev-parse.sh
+++ b/t/t1500-rev-parse.sh
@@ -3,7 +3,7 @@
 test_description='test git rev-parse'
 . ./test-lib.sh
 
-# usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir
+# usage: [options] label is-bare is-inside-git is-inside-work prefix git-dir absolute-git-dir
 test_rev_parse () {
 	d=
 	bare=
@@ -29,7 +29,8 @@ test_rev_parse () {
 		 --is-inside-git-dir \
 		 --is-inside-work-tree \
 		 --show-prefix \
-		 --git-dir
+		 --git-dir \
+		 --absolute-git-dir
 	do
 		test $# -eq 0 && break
 		expect="$1"
@@ -62,26 +63,26 @@ test_expect_success 'setup' '
 	cp -R .git repo.git
 '
 
-test_rev_parse toplevel false false true '' .git
+test_rev_parse toplevel false false true '' .git "$ROOT/.git"
 
-test_rev_parse -C .git .git/ false true false '' .
-test_rev_parse -C .git/objects .git/objects/ false true false '' "$ROOT/.git"
+test_rev_parse -C .git .git/ false true false '' . "$ROOT/.git"
+test_rev_parse -C .git/objects .git/objects/ false true false '' "$ROOT/.git" "$ROOT/.git"
 
-test_rev_parse -C sub/dir subdirectory false false true sub/dir/ "$ROOT/.git"
+test_rev_parse -C sub/dir subdirectory false false true sub/dir/ "$ROOT/.git" "$ROOT/.git"
 
 test_rev_parse -b t 'core.bare = true' true false false
 
 test_rev_parse -b u 'core.bare undefined' false false true
 
 
-test_rev_parse -C work -g ../.git -b f 'GIT_DIR=../.git, core.bare = false' false false true ''
+test_rev_parse -C work -g ../.git -b f 'GIT_DIR=../.git, core.bare = false' false false true '' "../.git" "$ROOT/.git"
 
 test_rev_parse -C work -g ../.git -b t 'GIT_DIR=../.git, core.bare = true' true false false ''
 
 test_rev_parse -C work -g ../.git -b u 'GIT_DIR=../.git, core.bare undefined' false false true ''
 
 
-test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = false' false false true ''
+test_rev_parse -C work -g ../repo.git -b f 'GIT_DIR=../repo.git, core.bare = false' false false true '' "../repo.git" "$ROOT/repo.git"
 
 test_rev_parse -C work -g ../repo.git -b t 'GIT_DIR=../repo.git, core.bare = true' true false false ''
 
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 16/21] completion: respect 'git -C <path>'

[SZEDER Gábor]

'git -C <path>' option(s) on the command line should be taken into
account during completion, because

  - like '--git-dir=<path>', it can lead us to a different repository,

  - a few git commands executed in the completion script do care about
    in which directory they are executed, and

  - the command for which we are providing completion might care about
    in which directory it will be executed.

However, unlike '--git-dir=<path>', the '-C <path>' option can be
specified multiple times and their effect is cumulative, so we can't
just store a single '<path>' in a variable.  Nor can we simply
concatenate a path from '-C <path1> -C <path2> ...', because e.g. (in
an arguably pathological corner case) a relative path might be
followed by an absolute path.

Instead, store all '-C <path>' options word by word in the
$__git_C_args array in the main git completion function, and pass this
array, if present, to 'git rev-parse --absolute-git-dir' when
discovering the repository in __gitdir(), and let it take care of
multiple options, relative paths, absolute paths and everything.

Also pass all '-C <path> options via the $__git_C_args array to those
git executions which require a worktree and for which it matters from
which directory they are executed from.  There are only three such
cases:

  - 'git diff-index' and 'git ls-files' in __git_ls_files_helper()
    used for git-aware filename completion, and

  - the 'git ls-tree' used for completing the 'ref:path' notation.

The other git commands executed in the completion script don't need
these '-C <path>' options, because __gitdir() already took those
options into account.  It would not hurt them, either, but let's not
induce unnecessary code churn.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 19 ++++++--
 t/t9902-completion.sh                  | 87 ++++++++++++++++++++++++++++++++++
 2 files changed, 101 insertions(+), 5 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ac5eb9ced..e003afd54 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -39,7 +39,11 @@ esac
 __gitdir ()
 {
 	if [ -z "${1-}" ]; then
-		if [ -n "${__git_dir-}" ]; then
+		if [ -n "${__git_C_args-}" ]; then
+			git "${__git_C_args[@]}" \
+				${__git_dir:+--git-dir="$__git_dir"} \
+				rev-parse --absolute-git-dir 2>/dev/null
+		elif [ -n "${__git_dir-}" ]; then
 			test -d "$__git_dir" || return 1
 			echo "$__git_dir"
 		elif [ -n "${GIT_DIR-}" ]; then
@@ -286,10 +290,10 @@ __git_ls_files_helper ()
 	local dir="$(__gitdir)"
 
 	if [ "$2" == "--committable" ]; then
-		git --git-dir="$dir" -C "$1" diff-index --name-only --relative HEAD
+		git ${__git_C_args:+"${__git_C_args[@]}"} --git-dir="$dir" -C "$1" diff-index --name-only --relative HEAD
 	else
 		# NOTE: $2 is not quoted in order to support multiple options
-		git --git-dir="$dir" -C "$1" ls-files --exclude-standard $2
+		git ${__git_C_args:+"${__git_C_args[@]}"} --git-dir="$dir" -C "$1" ls-files --exclude-standard $2
 	fi 2>/dev/null
 }
 
@@ -519,7 +523,7 @@ __git_complete_revlist_file ()
 		*)   pfx="$ref:$pfx" ;;
 		esac
 
-		__gitcomp_nl "$(git --git-dir="$(__gitdir)" ls-tree "$ls" 2>/dev/null \
+		__gitcomp_nl "$(git ${__git_C_args:+"${__git_C_args[@]}"} --git-dir="$(__gitdir)" ls-tree "$ls" 2>/dev/null \
 				| sed '/^100... blob /{
 				           s,^.*	,,
 				           s,$, ,
@@ -2792,6 +2796,7 @@ _git_worktree ()
 __git_main ()
 {
 	local i c=1 command __git_dir
+	local __git_C_args C_args_count=0
 
 	while [ $c -lt $cword ]; do
 		i="${words[c]}"
@@ -2800,7 +2805,11 @@ __git_main ()
 		--git-dir)   ((c++)) ; __git_dir="${words[c]}" ;;
 		--bare)      __git_dir="." ;;
 		--help) command="help"; break ;;
-		-c|-C|--work-tree|--namespace) ((c++)) ;;
+		-c|--work-tree|--namespace) ((c++)) ;;
+		-C)	__git_C_args[C_args_count++]=-C
+			((c++))
+			__git_C_args[C_args_count++]="${words[c]}"
+			;;
 		-*) ;;
 		*) command="$i"; break ;;
 		esac
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index be2ed8704..984df05b2 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -211,6 +211,87 @@ test_expect_success '__gitdir - $GIT_DIR set while .git directory in parent' '
 	test_cmp expected "$actual"
 '
 
+test_expect_success '__gitdir - from command line while "git -C"' '
+	echo "$ROOT/.git" >expected &&
+	(
+		__git_dir="$ROOT/.git" &&
+		__git_C_args=(-C otherrepo) &&
+		__gitdir >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__gitdir - relative dir from command line and "git -C"' '
+	echo "$ROOT/otherrepo/.git" >expected &&
+	(
+		cd subdir &&
+		__git_dir="otherrepo/.git" &&
+		__git_C_args=(-C ..) &&
+		__gitdir >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__gitdir - $GIT_DIR set while "git -C"' '
+	echo "$ROOT/.git" >expected &&
+	(
+		GIT_DIR="$ROOT/.git" &&
+		export GIT_DIR &&
+		__git_C_args=(-C otherrepo) &&
+		__gitdir >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__gitdir - relative dir in $GIT_DIR and "git -C"' '
+	echo "$ROOT/otherrepo/.git" >expected &&
+	(
+		cd subdir &&
+		GIT_DIR="otherrepo/.git" &&
+		export GIT_DIR &&
+		__git_C_args=(-C ..) &&
+		__gitdir >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__gitdir - "git -C" while .git directory in cwd' '
+	echo "$ROOT/otherrepo/.git" >expected &&
+	(
+		__git_C_args=(-C otherrepo) &&
+		__gitdir >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__gitdir - "git -C" while cwd is a .git directory' '
+	echo "$ROOT/otherrepo/.git" >expected &&
+	(
+		cd .git &&
+		__git_C_args=(-C .. -C otherrepo) &&
+		__gitdir >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__gitdir - "git -C" while .git directory in parent' '
+	echo "$ROOT/otherrepo/.git" >expected &&
+	(
+		cd subdir &&
+		__git_C_args=(-C .. -C otherrepo) &&
+		__gitdir >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__gitdir - non-existing path in "git -C"' '
+	(
+		__git_C_args=(-C non-existing) &&
+		test_must_fail __gitdir >"$actual"
+	) &&
+	test_must_be_empty "$actual"
+'
+
 test_expect_success '__gitdir - non-existing path in $__git_dir' '
 	(
 		__git_dir="non-existing" &&
@@ -785,6 +866,12 @@ test_expect_success 'checkout completes ref names' '
 	EOF
 '
 
+test_expect_success 'git -C <path> checkout uses the right repo' '
+	test_completion "git -C subdir -C subsubdir -C .. -C ../otherrepo checkout b" <<-\EOF
+	branch-in-other Z
+	EOF
+'
+
 test_expect_success 'show completes all refs' '
 	test_completion "git show m" <<-\EOF
 	master Z
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 17/21] completion: don't use __gitdir() for git commands

[SZEDER Gábor]

Several completion functions contain the following pattern to run git
commands respecting the path to the repository specified on the
command line:

  git --git-dir="$(__gitdir)" <cmd> <options>

This imposes the overhead of fork()ing a subshell for the command
substitution and potentially fork()+exec()ing 'git rev-parse' inside
__gitdir().

Now, if neither '--gitdir=<path>' nor '-C <path>' options are
specified on the command line, then those git commands are perfectly
capable to discover the repository on their own.  If either one or
both of those options are specified on the command line, then, again,
the git commands could discover the repository, if we pass them all of
those options from the command line.

This means we don't have to run __gitdir() at all for git commands and
can spare its fork()+exec() overhead.

Use Bash parameter expansions to check the $__git_dir variable and
$__git_C_args array and to assemble the appropriate '--git-dir=<path>'
and '-C <path>' options if either one or both are present on the
command line.  These parameter expansions are, however, rather long,
so instead of changing all git executions and make already long lines
even longer, encapsulate running git with '--git-dir=<path> -C <path>'
options into the new __git() wrapper function.  Furthermore, this
wrapper function will also enable us to silence error messages from
git commands uniformly in one place in a later commit.

There's one tricky case, though: in __git_refs() local refs are listed
with 'git for-each-ref', where "local" is not necessarily the
repository we are currently in, but it might mean a remote repository
in the filesystem (e.g. listing refs for 'git fetch /some/other/repo
<TAB>').  Use one-shot variable assignment to override $__git_dir with
the path of the repository where the refs should come from.  Although
one-shot variable assignments in front of shell functions are to be
avoided in our scripts in general, in the Bash completion script we
can do that safely.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 60 ++++++++++++++++++----------------
 1 file changed, 31 insertions(+), 29 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e003afd54..e7c0b56ea 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -61,6 +61,14 @@ __gitdir ()
 	fi
 }
 
+# Runs git with all the options given as argument, respecting any
+# '--git-dir=<path>' and '-C <path>' options present on the command line
+__git ()
+{
+	git ${__git_C_args:+"${__git_C_args[@]}"} \
+		${__git_dir:+--git-dir="$__git_dir"} "$@"
+}
+
 # The following function is based on code from:
 #
 #   bash_completion - programmable completion functions for bash 3.2+
@@ -287,13 +295,11 @@ __gitcomp_file ()
 # argument, and using the options specified in the second argument.
 __git_ls_files_helper ()
 {
-	local dir="$(__gitdir)"
-
 	if [ "$2" == "--committable" ]; then
-		git ${__git_C_args:+"${__git_C_args[@]}"} --git-dir="$dir" -C "$1" diff-index --name-only --relative HEAD
+		__git -C "$1" diff-index --name-only --relative HEAD
 	else
 		# NOTE: $2 is not quoted in order to support multiple options
-		git ${__git_C_args:+"${__git_C_args[@]}"} --git-dir="$dir" -C "$1" ls-files --exclude-standard $2
+		__git -C "$1" ls-files --exclude-standard $2
 	fi 2>/dev/null
 }
 
@@ -323,8 +329,7 @@ __git_heads ()
 {
 	local dir="$(__gitdir)"
 	if [ -d "$dir" ]; then
-		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
-			refs/heads
+		__git for-each-ref --format='%(refname:short)' refs/heads
 		return
 	fi
 }
@@ -333,8 +338,7 @@ __git_tags ()
 {
 	local dir="$(__gitdir)"
 	if [ -d "$dir" ]; then
-		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
-			refs/tags
+		__git for-each-ref --format='%(refname:short)' refs/tags
 		return
 	fi
 }
@@ -385,14 +389,14 @@ __git_refs ()
 			refs="refs/tags refs/heads refs/remotes"
 			;;
 		esac
-		git --git-dir="$dir" for-each-ref --format="$pfx%($format)" \
+		__git_dir="$dir" __git for-each-ref --format="$pfx%($format)" \
 			$refs
 		if [ -n "$track" ]; then
 			# employ the heuristic used by git checkout
 			# Try to find a remote branch that matches the completion word
 			# but only output if the branch name is unique
 			local ref entry
-			git --git-dir="$dir" for-each-ref --shell --format="ref=%(refname:short)" \
+			__git for-each-ref --shell --format="ref=%(refname:short)" \
 				"refs/remotes/" | \
 			while read -r entry; do
 				eval "$entry"
@@ -406,7 +410,7 @@ __git_refs ()
 	fi
 	case "$cur" in
 	refs|refs/*)
-		git --git-dir="$dir" ls-remote "$remote" "$cur*" 2>/dev/null | \
+		__git ls-remote "$remote" "$cur*" 2>/dev/null | \
 		while read -r hash i; do
 			case "$i" in
 			*^{}) ;;
@@ -417,10 +421,10 @@ __git_refs ()
 	*)
 		if [ "$list_refs_from" = remote ]; then
 			echo "HEAD"
-			git --git-dir="$dir" for-each-ref --format="%(refname:short)" \
+			__git for-each-ref --format="%(refname:short)" \
 				"refs/remotes/$remote/" 2>/dev/null | sed -e "s#^$remote/##"
 		else
-			git --git-dir="$dir" ls-remote "$remote" HEAD \
+			__git ls-remote "$remote" HEAD \
 				"refs/tags/*" "refs/heads/*" "refs/remotes/*" 2>/dev/null |
 			while read -r hash i; do
 				case "$i" in
@@ -447,7 +451,7 @@ __git_refs2 ()
 __git_refs_remotes ()
 {
 	local i hash
-	git --git-dir="$(__gitdir)" ls-remote "$1" 'refs/heads/*' 2>/dev/null | \
+	__git ls-remote "$1" 'refs/heads/*' 2>/dev/null | \
 	while read -r hash i; do
 		echo "$i:refs/remotes/$1/${i#refs/heads/}"
 	done
@@ -457,7 +461,7 @@ __git_remotes ()
 {
 	local d="$(__gitdir)"
 	test -d "$d/remotes" && ls -1 "$d/remotes"
-	git --git-dir="$d" remote
+	__git remote
 }
 
 # Returns true if $1 matches the name of a configured remote, false otherwise.
@@ -523,7 +527,7 @@ __git_complete_revlist_file ()
 		*)   pfx="$ref:$pfx" ;;
 		esac
 
-		__gitcomp_nl "$(git ${__git_C_args:+"${__git_C_args[@]}"} --git-dir="$(__gitdir)" ls-tree "$ls" 2>/dev/null \
+		__gitcomp_nl "$(__git ls-tree "$ls" 2>/dev/null \
 				| sed '/^100... blob /{
 				           s,^.*	,,
 				           s,$, ,
@@ -801,7 +805,7 @@ __git_compute_porcelain_commands ()
 __git_get_config_variables ()
 {
 	local section="$1" i IFS=$'\n'
-	for i in $(git --git-dir="$(__gitdir)" config --name-only --get-regexp "^$section\..*" 2>/dev/null); do
+	for i in $(__git config --name-only --get-regexp "^$section\..*" 2>/dev/null); do
 		echo "${i#$section.}"
 	done
 }
@@ -819,8 +823,7 @@ __git_aliases ()
 # __git_aliased_command requires 1 argument
 __git_aliased_command ()
 {
-	local word cmdline=$(git --git-dir="$(__gitdir)" \
-		config --get "alias.$1" 2>/dev/null)
+	local word cmdline=$(__git config --get "alias.$1" 2>/dev/null)
 	for word in $cmdline; do
 		case "$word" in
 		\!gitk|gitk)
@@ -896,7 +899,7 @@ __git_get_option_value ()
 	done
 
 	if [ -n "$config_key" ] && [ -z "$result" ]; then
-		result="$(git --git-dir="$(__gitdir)" config "$config_key")"
+		result="$(__git config "$config_key")"
 	fi
 
 	echo "$result"
@@ -1237,7 +1240,7 @@ _git_commit ()
 		return
 	esac
 
-	if git --git-dir="$(__gitdir)" rev-parse --verify --quiet HEAD >/dev/null; then
+	if __git rev-parse --verify --quiet HEAD >/dev/null; then
 		__git_complete_index_file "--committable"
 	else
 		# This is the first commit
@@ -1839,7 +1842,7 @@ _git_send_email ()
 	case "$prev" in
 	--to|--cc|--bcc|--from)
 		__gitcomp "
-		$(git --git-dir="$(__gitdir)" send-email --dump-aliases 2>/dev/null)
+		$(__git send-email --dump-aliases 2>/dev/null)
 		"
 		return
 		;;
@@ -1871,7 +1874,7 @@ _git_send_email ()
 		;;
 	--to=*|--cc=*|--bcc=*|--from=*)
 		__gitcomp "
-		$(git --git-dir="$(__gitdir)" send-email --dump-aliases 2>/dev/null)
+		$(__git send-email --dump-aliases 2>/dev/null)
 		" "" "${cur#--*=}"
 		return
 		;;
@@ -1966,7 +1969,7 @@ __git_config_get_set_variables ()
 		c=$((--c))
 	done
 
-	git --git-dir="$(__gitdir)" config $config_file --name-only --list 2>/dev/null
+	__git config $config_file --name-only --list 2>/dev/null
 }
 
 _git_config ()
@@ -2001,9 +2004,8 @@ _git_config ()
 	remote.*.push)
 		local remote="${prev#remote.}"
 		remote="${remote%.push}"
-		__gitcomp_nl "$(git --git-dir="$(__gitdir)" \
-			for-each-ref --format='%(refname):%(refname)' \
-			refs/heads)"
+		__gitcomp_nl "$(__git for-each-ref
+			--format='%(refname):%(refname)' refs/heads)"
 		return
 		;;
 	pull.twohead|pull.octopus)
@@ -2591,12 +2593,12 @@ _git_stash ()
 			if [ $cword -eq 3 ]; then
 				__gitcomp_nl "$(__git_refs)";
 			else
-				__gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
+				__gitcomp_nl "$(__git stash list \
 						| sed -n -e 's/:.*//p')"
 			fi
 			;;
 		show,*|apply,*|drop,*|pop,*)
-			__gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
+			__gitcomp_nl "$(__git stash list \
 					| sed -n -e 's/:.*//p')"
 			;;
 		*)
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 18/21] completion: consolidate silencing errors from git commands

[SZEDER Gábor]

Outputting error messages during completion is bad: they disrupt the
command line, can't be deleted, and the user is forced to Ctrl-C and
start over most of the time.  We already silence stderr of many git
commands in our Bash completion script, but there are still some in
there that can spew error messages when something goes wrong.

We could add the missing stderr redirections to all the remaining
places, but instead let's leverage that git commands are now executed
through the previously introduced __git() wrapper function, and
redirect standard error to /dev/null only in that function.  This way
we need only one redirection to take care of errors from almost all
git commands.  Redirecting standard error of the __git() wrapper
function thus became redundant, remove them.

The exceptions, i.e. the repo-independent git executions and those in
the __gitdir() function that don't go through __git() already have
their standard error silenced.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e7c0b56ea..1a849761f 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -66,7 +66,7 @@ __gitdir ()
 __git ()
 {
 	git ${__git_C_args:+"${__git_C_args[@]}"} \
-		${__git_dir:+--git-dir="$__git_dir"} "$@"
+		${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null
 }
 
 # The following function is based on code from:
@@ -300,7 +300,7 @@ __git_ls_files_helper ()
 	else
 		# NOTE: $2 is not quoted in order to support multiple options
 		__git -C "$1" ls-files --exclude-standard $2
-	fi 2>/dev/null
+	fi
 }
 
 
@@ -410,7 +410,7 @@ __git_refs ()
 	fi
 	case "$cur" in
 	refs|refs/*)
-		__git ls-remote "$remote" "$cur*" 2>/dev/null | \
+		__git ls-remote "$remote" "$cur*" | \
 		while read -r hash i; do
 			case "$i" in
 			*^{}) ;;
@@ -422,10 +422,10 @@ __git_refs ()
 		if [ "$list_refs_from" = remote ]; then
 			echo "HEAD"
 			__git for-each-ref --format="%(refname:short)" \
-				"refs/remotes/$remote/" 2>/dev/null | sed -e "s#^$remote/##"
+				"refs/remotes/$remote/" | sed -e "s#^$remote/##"
 		else
 			__git ls-remote "$remote" HEAD \
-				"refs/tags/*" "refs/heads/*" "refs/remotes/*" 2>/dev/null |
+				"refs/tags/*" "refs/heads/*" "refs/remotes/*" |
 			while read -r hash i; do
 				case "$i" in
 				*^{})	;;
@@ -451,7 +451,7 @@ __git_refs2 ()
 __git_refs_remotes ()
 {
 	local i hash
-	__git ls-remote "$1" 'refs/heads/*' 2>/dev/null | \
+	__git ls-remote "$1" 'refs/heads/*' | \
 	while read -r hash i; do
 		echo "$i:refs/remotes/$1/${i#refs/heads/}"
 	done
@@ -527,7 +527,7 @@ __git_complete_revlist_file ()
 		*)   pfx="$ref:$pfx" ;;
 		esac
 
-		__gitcomp_nl "$(__git ls-tree "$ls" 2>/dev/null \
+		__gitcomp_nl "$(__git ls-tree "$ls" \
 				| sed '/^100... blob /{
 				           s,^.*	,,
 				           s,$, ,
@@ -805,7 +805,7 @@ __git_compute_porcelain_commands ()
 __git_get_config_variables ()
 {
 	local section="$1" i IFS=$'\n'
-	for i in $(__git config --name-only --get-regexp "^$section\..*" 2>/dev/null); do
+	for i in $(__git config --name-only --get-regexp "^$section\..*"); do
 		echo "${i#$section.}"
 	done
 }
@@ -823,7 +823,7 @@ __git_aliases ()
 # __git_aliased_command requires 1 argument
 __git_aliased_command ()
 {
-	local word cmdline=$(__git config --get "alias.$1" 2>/dev/null)
+	local word cmdline=$(__git config --get "alias.$1")
 	for word in $cmdline; do
 		case "$word" in
 		\!gitk|gitk)
@@ -1841,9 +1841,7 @@ _git_send_email ()
 {
 	case "$prev" in
 	--to|--cc|--bcc|--from)
-		__gitcomp "
-		$(__git send-email --dump-aliases 2>/dev/null)
-		"
+		__gitcomp "$(__git send-email --dump-aliases)"
 		return
 		;;
 	esac
@@ -1873,9 +1871,7 @@ _git_send_email ()
 		return
 		;;
 	--to=*|--cc=*|--bcc=*|--from=*)
-		__gitcomp "
-		$(__git send-email --dump-aliases 2>/dev/null)
-		" "" "${cur#--*=}"
+		__gitcomp "$(__git send-email --dump-aliases)" "" "${cur#--*=}"
 		return
 		;;
 	--*)
@@ -1969,7 +1965,7 @@ __git_config_get_set_variables ()
 		c=$((--c))
 	done
 
-	__git config $config_file --name-only --list 2>/dev/null
+	__git config $config_file --name-only --list
 }
 
 _git_config ()
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 19/21] completion: don't guard git executions with __gitdir()

[SZEDER Gábor]

Three completion functions, namely __git_index_files(), __git_heads()
and __git_tags(), first run __gitdir() and check that the path it
outputs exists, i.e. that there is a git repository, and run a git
command only if there is one.

After the previous changes in this series there are no further uses of
__gitdir()'s output in these functions besides those checks.  And
those checks are unnecessary, because we can just execute those git
commands outside of a repository and let them error out.  We don't
perform such a check in other places either.

Remove this check and the __gitdir() call from these functions,
sparing the fork()+exec() overhead of the command substitution and the
potential 'git rev-parse' execution.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 32 +++++++++++---------------------
 1 file changed, 11 insertions(+), 21 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1a849761f..1c7493ff2 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -312,35 +312,25 @@ __git_ls_files_helper ()
 #    slash.
 __git_index_files ()
 {
-	local dir="$(__gitdir)" root="${2-.}" file
-
-	if [ -d "$dir" ]; then
-		__git_ls_files_helper "$root" "$1" |
-		while read -r file; do
-			case "$file" in
-			?*/*) echo "${file%%/*}" ;;
-			*) echo "$file" ;;
-			esac
-		done | sort | uniq
-	fi
+	local root="${2-.}" file
+
+	__git_ls_files_helper "$root" "$1" |
+	while read -r file; do
+		case "$file" in
+		?*/*) echo "${file%%/*}" ;;
+		*) echo "$file" ;;
+		esac
+	done | sort | uniq
 }
 
 __git_heads ()
 {
-	local dir="$(__gitdir)"
-	if [ -d "$dir" ]; then
-		__git for-each-ref --format='%(refname:short)' refs/heads
-		return
-	fi
+	__git for-each-ref --format='%(refname:short)' refs/heads
 }
 
 __git_tags ()
 {
-	local dir="$(__gitdir)"
-	if [ -d "$dir" ]; then
-		__git for-each-ref --format='%(refname:short)' refs/tags
-		return
-	fi
+	__git for-each-ref --format='%(refname:short)' refs/tags
 }
 
 # Lists refs from the local (by default) or from a remote repository.
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 20/21] completion: extract repository discovery from __gitdir()

[SZEDER Gábor]

To prepare for caching the path to the repository in the following
commit, extract the repository discovering part of __gitdir() into the
__git_find_repo_path() helper function, which stores the found path in
the $__git_repo_path variable instead of printing it.  Make __gitdir()
a wrapper around this new function.  Declare $__git_repo_path local in
the toplevel completion functions __git_main() and __gitk_main() to
ensure that it never leaks into the environment and influences
subsequent completions (though this isn't necessary right now, as
__gitdir() is still only executed in subshells, but will matter for
the following commit).

Adjust tests checking __gitdir() or any other completion function
calling __gitdir() to perform those checks in a subshell to prevent
$__git_repo_path from leaking into the test environment.  Otherwise
leave the tests unchanged to demonstrate that this change doesn't
alter __gitdir()'s behavior.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash | 42 +++++++++++++++++++++-------------
 t/t9902-completion.sh                  | 22 +++++++++++++-----
 2 files changed, 42 insertions(+), 22 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 1c7493ff2..7775411cd 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -34,26 +34,35 @@ case "$COMP_WORDBREAKS" in
 *)   COMP_WORDBREAKS="$COMP_WORDBREAKS:"
 esac
 
+# Discovers the path to the git repository taking any '--git-dir=<path>' and
+# '-C <path>' options into account and stores it in the $__git_repo_path
+# variable.
+__git_find_repo_path ()
+{
+	if [ -n "${__git_C_args-}" ]; then
+		__git_repo_path="$(git "${__git_C_args[@]}" \
+			${__git_dir:+--git-dir="$__git_dir"} \
+			rev-parse --absolute-git-dir 2>/dev/null)"
+	elif [ -n "${__git_dir-}" ]; then
+		test -d "$__git_dir" &&
+		__git_repo_path="$__git_dir"
+	elif [ -n "${GIT_DIR-}" ]; then
+		test -d "${GIT_DIR-}" &&
+		__git_repo_path="$GIT_DIR"
+	elif [ -d .git ]; then
+		__git_repo_path=.git
+	else
+		__git_repo_path="$(git rev-parse --git-dir 2>/dev/null)"
+	fi
+}
+
 # __gitdir accepts 0 or 1 arguments (i.e., location)
 # returns location of .git repo
 __gitdir ()
 {
 	if [ -z "${1-}" ]; then
-		if [ -n "${__git_C_args-}" ]; then
-			git "${__git_C_args[@]}" \
-				${__git_dir:+--git-dir="$__git_dir"} \
-				rev-parse --absolute-git-dir 2>/dev/null
-		elif [ -n "${__git_dir-}" ]; then
-			test -d "$__git_dir" || return 1
-			echo "$__git_dir"
-		elif [ -n "${GIT_DIR-}" ]; then
-			test -d "${GIT_DIR-}" || return 1
-			echo "$GIT_DIR"
-		elif [ -d .git ]; then
-			echo .git
-		else
-			git rev-parse --git-dir 2>/dev/null
-		fi
+		__git_find_repo_path || return 1
+		echo "$__git_repo_path"
 	elif [ -d "$1/.git" ]; then
 		echo "$1/.git"
 	else
@@ -2783,7 +2792,7 @@ _git_worktree ()
 
 __git_main ()
 {
-	local i c=1 command __git_dir
+	local i c=1 command __git_dir __git_repo_path
 	local __git_C_args C_args_count=0
 
 	while [ $c -lt $cword ]; do
@@ -2855,6 +2864,7 @@ __gitk_main ()
 {
 	__git_has_doubledash && return
 
+	local __git_repo_path
 	local g="$(__gitdir)"
 	local merge=""
 	if [ -f "$g/MERGE_HEAD" ]; then
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 984df05b2..1816ed2e0 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -147,19 +147,25 @@ test_expect_success '__gitdir - from command line (through $__git_dir)' '
 
 test_expect_success '__gitdir - repo as argument' '
 	echo "otherrepo/.git" >expected &&
-	__gitdir "otherrepo" >"$actual" &&
+	(
+		__gitdir "otherrepo" >"$actual"
+	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success '__gitdir - remote as argument' '
 	echo "remote" >expected &&
-	__gitdir "remote" >"$actual" &&
+	(
+		__gitdir "remote" >"$actual"
+	) &&
 	test_cmp expected "$actual"
 '
 
 test_expect_success '__gitdir - .git directory in cwd' '
 	echo ".git" >expected &&
-	__gitdir >"$actual" &&
+	(
+		__gitdir >"$actual"
+	) &&
 	test_cmp expected "$actual"
 '
 
@@ -450,7 +456,9 @@ test_expect_success '__git_remotes - list remotes from $GIT_DIR/remotes and from
 	git remote add remote_in_config_1 git://remote_1 &&
 	test_when_finished "git remote remove remote_in_config_2" &&
 	git remote add remote_in_config_2 git://remote_2 &&
-	__git_remotes >actual &&
+	(
+		__git_remotes >actual
+	) &&
 	test_cmp expect actual
 '
 
@@ -459,8 +467,10 @@ test_expect_success '__git_is_configured_remote' '
 	git remote add remote_1 git://remote_1 &&
 	test_when_finished "git remote remove remote_2" &&
 	git remote add remote_2 git://remote_2 &&
-	verbose __git_is_configured_remote remote_2 &&
-	test_must_fail __git_is_configured_remote non-existent
+	(
+		verbose __git_is_configured_remote remote_2 &&
+		test_must_fail __git_is_configured_remote non-existent
+	)
 '
 
 test_expect_success 'setup for ref completion' '
-- 
2.11.0.555.g967c1bcb3

[PATCHv2 21/21] completion: cache the path to the repository

[SZEDER Gábor]

After the previous changes in this series there are only a handful of
$(__gitdir) command substitutions left in the completion script, but
there is still a bit of room for improvements:

  1. The command substitution involves the forking of a subshell,
     which has considerable overhead on some platforms.

  2. There are a few cases, where this command substitution is
     executed more than once during a single completion, which means
     multiple subshells and possibly multiple 'git rev-parse'
     executions.  __gitdir() is invoked twice while completing refs
     for e.g. 'git log', 'git rebase', 'gitk', or while completing
     remote refs for 'git fetch' or 'git push'.

Both of these points can be addressed by using the
__git_find_repo_path() helper function introduced in the previous
commit:

  1. __git_find_repo_path() stores the path to the repository in a
     variable instead of printing it, so the command substitution
     around the function can be avoided.  Or rather: the command
     substitution should be avoided to make the new value of the
     variable set inside the function visible to the callers.
     (Yes, there is now a command substitution inside
     __git_find_repo_path() around each 'git rev-parse', but that's
     executed only if necessary, and only once per completion, see
     point 2. below.)

  2. $__git_repo_path, the variable holding the path to the
     repository, is declared local in the toplevel completion
     functions __git_main() and __gitk_main().  Thus, once set, the
     path is visible in all completion functions, including all
     subsequent calls to __git_find_repo_path(), meaning that they
     wouldn't have to re-discover the path to the repository.

So call __git_find_repo_path() and use $__git_repo_path instead of the
$(__gitdir) command substitution to access paths in the .git
directory.  Turn tests checking __gitdir()'s repository discovery into
tests of __git_find_repo_path() such that only the tested function
changes but the expected results don't, ensuring that repo discovery
keeps working as it did before.

As __gitdir() is not used anymore in the completion script, mark it as
deprecated and direct users' attention to __git_find_repo_path() and
$__git_repo_path.  Yet keep four __gitdir() tests to ensure that it
handles success and failure of __git_find_repo_path() and that it
still handles its optional remote argument, because users' custom
completion scriptlets might depend on it.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 contrib/completion/git-completion.bash |  46 ++++++----
 t/t9902-completion.sh                  | 161 +++++++++++++++++++++------------
 2 files changed, 132 insertions(+), 75 deletions(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 7775411cd..ed06cb621 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -39,6 +39,11 @@ esac
 # variable.
 __git_find_repo_path ()
 {
+	if [ -n "$__git_repo_path" ]; then
+		# we already know where it is
+		return
+	fi
+
 	if [ -n "${__git_C_args-}" ]; then
 		__git_repo_path="$(git "${__git_C_args[@]}" \
 			${__git_dir:+--git-dir="$__git_dir"} \
@@ -56,6 +61,7 @@ __git_find_repo_path ()
 	fi
 }
 
+# Deprecated: use __git_find_repo_path() and $__git_repo_path instead
 # __gitdir accepts 0 or 1 arguments (i.e., location)
 # returns location of .git repo
 __gitdir ()
@@ -350,10 +356,13 @@ __git_tags ()
 #    'git checkout's tracking DWIMery (optional; ignored, if set but empty).
 __git_refs ()
 {
-	local i hash dir="$(__gitdir)" track="${2-}"
+	local i hash dir track="${2-}"
 	local list_refs_from=path remote="${1-}"
 	local format refs pfx
 
+	__git_find_repo_path
+	dir="$__git_repo_path"
+
 	if [ -z "$remote" ]; then
 		if [ -z "$dir" ]; then
 			return
@@ -458,8 +467,8 @@ __git_refs_remotes ()
 
 __git_remotes ()
 {
-	local d="$(__gitdir)"
-	test -d "$d/remotes" && ls -1 "$d/remotes"
+	__git_find_repo_path
+	test -d "$__git_repo_path/remotes" && ls -1 "$__git_repo_path/remotes"
 	__git remote
 }
 
@@ -957,8 +966,8 @@ __git_whitespacelist="nowarn warn error error-all fix"
 
 _git_am ()
 {
-	local dir="$(__gitdir)"
-	if [ -d "$dir"/rebase-apply ]; then
+	__git_find_repo_path
+	if [ -d "$__git_repo_path"/rebase-apply ]; then
 		__gitcomp "--skip --continue --resolved --abort"
 		return
 	fi
@@ -1041,7 +1050,8 @@ _git_bisect ()
 	local subcommands="start bad good skip reset visualize replay log run"
 	local subcommand="$(__git_find_on_cmdline "$subcommands")"
 	if [ -z "$subcommand" ]; then
-		if [ -f "$(__gitdir)"/BISECT_START ]; then
+		__git_find_repo_path
+		if [ -f "$__git_repo_path"/BISECT_START ]; then
 			__gitcomp "$subcommands"
 		else
 			__gitcomp "replay start"
@@ -1146,8 +1156,8 @@ _git_cherry ()
 
 _git_cherry_pick ()
 {
-	local dir="$(__gitdir)"
-	if [ -f "$dir"/CHERRY_PICK_HEAD ]; then
+	__git_find_repo_path
+	if [ -f "$__git_repo_path"/CHERRY_PICK_HEAD ]; then
 		__gitcomp "--continue --quit --abort"
 		return
 	fi
@@ -1538,10 +1548,10 @@ __git_log_date_formats="relative iso8601 rfc2822 short local default raw"
 _git_log ()
 {
 	__git_has_doubledash && return
+	__git_find_repo_path
 
-	local g="$(__gitdir)"
 	local merge=""
-	if [ -f "$g/MERGE_HEAD" ]; then
+	if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
 		merge="--merge"
 	fi
 	case "$cur" in
@@ -1788,11 +1798,12 @@ _git_push ()
 
 _git_rebase ()
 {
-	local dir="$(__gitdir)"
-	if [ -f "$dir"/rebase-merge/interactive ]; then
+	__git_find_repo_path
+	if [ -f "$__git_repo_path"/rebase-merge/interactive ]; then
 		__gitcomp "--continue --skip --abort --quit --edit-todo"
 		return
-	elif [ -d "$dir"/rebase-apply ] || [ -d "$dir"/rebase-merge ]; then
+	elif [ -d "$__git_repo_path"/rebase-apply ] || \
+	     [ -d "$__git_repo_path"/rebase-merge ]; then
 		__gitcomp "--continue --skip --abort --quit"
 		return
 	fi
@@ -2467,8 +2478,8 @@ _git_reset ()
 
 _git_revert ()
 {
-	local dir="$(__gitdir)"
-	if [ -f "$dir"/REVERT_HEAD ]; then
+	__git_find_repo_path
+	if [ -f "$__git_repo_path"/REVERT_HEAD ]; then
 		__gitcomp "--continue --quit --abort"
 		return
 	fi
@@ -2865,9 +2876,10 @@ __gitk_main ()
 	__git_has_doubledash && return
 
 	local __git_repo_path
-	local g="$(__gitdir)"
+	__git_find_repo_path
+
 	local merge=""
-	if [ -f "$g/MERGE_HEAD" ]; then
+	if [ -f "$__git_repo_path/MERGE_HEAD" ]; then
 		merge="--merge"
 	fi
 	case "$cur" in
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 1816ed2e0..d711bef24 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -131,213 +131,217 @@ else
 	ROOT="$(pwd)"
 fi
 
-test_expect_success 'setup for __gitdir tests' '
+test_expect_success 'setup for __git_find_repo_path/__gitdir tests' '
 	mkdir -p subdir/subsubdir &&
+	mkdir -p non-repo &&
 	git init otherrepo
 '
 
-test_expect_success '__gitdir - from command line (through $__git_dir)' '
+test_expect_success '__git_find_repo_path - from command line (through $__git_dir)' '
 	echo "$ROOT/otherrepo/.git" >expected &&
 	(
 		__git_dir="$ROOT/otherrepo/.git" &&
-		__gitdir >"$actual"
-	) &&
-	test_cmp expected "$actual"
-'
-
-test_expect_success '__gitdir - repo as argument' '
-	echo "otherrepo/.git" >expected &&
-	(
-		__gitdir "otherrepo" >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - remote as argument' '
-	echo "remote" >expected &&
-	(
-		__gitdir "remote" >"$actual"
-	) &&
-	test_cmp expected "$actual"
-'
-
-test_expect_success '__gitdir - .git directory in cwd' '
+test_expect_success '__git_find_repo_path - .git directory in cwd' '
 	echo ".git" >expected &&
 	(
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - .git directory in parent' '
+test_expect_success '__git_find_repo_path - .git directory in parent' '
 	echo "$ROOT/.git" >expected &&
 	(
 		cd subdir/subsubdir &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - cwd is a .git directory' '
+test_expect_success '__git_find_repo_path - cwd is a .git directory' '
 	echo "." >expected &&
 	(
 		cd .git &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - parent is a .git directory' '
+test_expect_success '__git_find_repo_path - parent is a .git directory' '
 	echo "$ROOT/.git" >expected &&
 	(
 		cd .git/refs/heads &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - $GIT_DIR set while .git directory in cwd' '
+test_expect_success '__git_find_repo_path - $GIT_DIR set while .git directory in cwd' '
 	echo "$ROOT/otherrepo/.git" >expected &&
 	(
 		GIT_DIR="$ROOT/otherrepo/.git" &&
 		export GIT_DIR &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - $GIT_DIR set while .git directory in parent' '
+test_expect_success '__git_find_repo_path - $GIT_DIR set while .git directory in parent' '
 	echo "$ROOT/otherrepo/.git" >expected &&
 	(
 		GIT_DIR="$ROOT/otherrepo/.git" &&
 		export GIT_DIR &&
 		cd subdir &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - from command line while "git -C"' '
+test_expect_success '__git_find_repo_path - from command line while "git -C"' '
 	echo "$ROOT/.git" >expected &&
 	(
 		__git_dir="$ROOT/.git" &&
 		__git_C_args=(-C otherrepo) &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - relative dir from command line and "git -C"' '
+test_expect_success '__git_find_repo_path - relative dir from command line and "git -C"' '
 	echo "$ROOT/otherrepo/.git" >expected &&
 	(
 		cd subdir &&
 		__git_dir="otherrepo/.git" &&
 		__git_C_args=(-C ..) &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - $GIT_DIR set while "git -C"' '
+test_expect_success '__git_find_repo_path - $GIT_DIR set while "git -C"' '
 	echo "$ROOT/.git" >expected &&
 	(
 		GIT_DIR="$ROOT/.git" &&
 		export GIT_DIR &&
 		__git_C_args=(-C otherrepo) &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - relative dir in $GIT_DIR and "git -C"' '
+test_expect_success '__git_find_repo_path - relative dir in $GIT_DIR and "git -C"' '
 	echo "$ROOT/otherrepo/.git" >expected &&
 	(
 		cd subdir &&
 		GIT_DIR="otherrepo/.git" &&
 		export GIT_DIR &&
 		__git_C_args=(-C ..) &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - "git -C" while .git directory in cwd' '
+test_expect_success '__git_find_repo_path - "git -C" while .git directory in cwd' '
 	echo "$ROOT/otherrepo/.git" >expected &&
 	(
 		__git_C_args=(-C otherrepo) &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - "git -C" while cwd is a .git directory' '
+test_expect_success '__git_find_repo_path - "git -C" while cwd is a .git directory' '
 	echo "$ROOT/otherrepo/.git" >expected &&
 	(
 		cd .git &&
 		__git_C_args=(-C .. -C otherrepo) &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - "git -C" while .git directory in parent' '
+test_expect_success '__git_find_repo_path - "git -C" while .git directory in parent' '
 	echo "$ROOT/otherrepo/.git" >expected &&
 	(
 		cd subdir &&
 		__git_C_args=(-C .. -C otherrepo) &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - non-existing path in "git -C"' '
+test_expect_success '__git_find_repo_path - non-existing path in "git -C"' '
 	(
 		__git_C_args=(-C non-existing) &&
-		test_must_fail __gitdir >"$actual"
+		test_must_fail __git_find_repo_path &&
+		printf "$__git_repo_path" >"$actual"
 	) &&
 	test_must_be_empty "$actual"
 '
 
-test_expect_success '__gitdir - non-existing path in $__git_dir' '
+test_expect_success '__git_find_repo_path - non-existing path in $__git_dir' '
 	(
 		__git_dir="non-existing" &&
-		test_must_fail __gitdir >"$actual"
+		test_must_fail __git_find_repo_path &&
+		printf "$__git_repo_path" >"$actual"
 	) &&
 	test_must_be_empty "$actual"
 '
 
-test_expect_success '__gitdir - non-existing $GIT_DIR' '
+test_expect_success '__git_find_repo_path - non-existing $GIT_DIR' '
 	(
 		GIT_DIR="$ROOT/non-existing" &&
 		export GIT_DIR &&
-		test_must_fail __gitdir >"$actual"
+		test_must_fail __git_find_repo_path &&
+		printf "$__git_repo_path" >"$actual"
 	) &&
 	test_must_be_empty "$actual"
 '
 
-test_expect_success '__gitdir - gitfile in cwd' '
+test_expect_success '__git_find_repo_path - gitfile in cwd' '
 	echo "$ROOT/otherrepo/.git" >expected &&
 	echo "gitdir: $ROOT/otherrepo/.git" >subdir/.git &&
 	test_when_finished "rm -f subdir/.git" &&
 	(
 		cd subdir &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - gitfile in parent' '
+test_expect_success '__git_find_repo_path - gitfile in parent' '
 	echo "$ROOT/otherrepo/.git" >expected &&
 	echo "gitdir: $ROOT/otherrepo/.git" >subdir/.git &&
 	test_when_finished "rm -f subdir/.git" &&
 	(
 		cd subdir/subsubdir &&
-		__gitdir >"$actual"
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success SYMLINKS '__gitdir - resulting path avoids symlinks' '
+test_expect_success SYMLINKS '__git_find_repo_path - resulting path avoids symlinks' '
 	echo "$ROOT/otherrepo/.git" >expected &&
 	mkdir otherrepo/dir &&
 	test_when_finished "rm -rf otherrepo/dir" &&
@@ -345,16 +349,57 @@ test_expect_success SYMLINKS '__gitdir - resulting path avoids symlinks' '
 	test_when_finished "rm -f link" &&
 	(
 		cd link &&
+		__git_find_repo_path &&
+		echo "$__git_repo_path" >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__git_find_repo_path - not a git repository' '
+	(
+		cd non-repo &&
+		GIT_CEILING_DIRECTORIES="$ROOT" &&
+		export GIT_CEILING_DIRECTORIES &&
+		test_must_fail __git_find_repo_path &&
+		printf "$__git_repo_path" >"$actual"
+	) &&
+	test_must_be_empty "$actual"
+'
+
+test_expect_success '__gitdir - finds repo' '
+	echo "$ROOT/.git" >expected &&
+	(
+		cd subdir/subsubdir &&
 		__gitdir >"$actual"
 	) &&
 	test_cmp expected "$actual"
 '
 
-test_expect_success '__gitdir - not a git repository' '
-	nongit test_must_fail __gitdir >"$actual" &&
+
+test_expect_success '__gitdir - returns error when cant find repo' '
+	(
+		__git_dir="non-existing" &&
+		test_must_fail __gitdir >"$actual"
+	) &&
 	test_must_be_empty "$actual"
 '
 
+test_expect_success '__gitdir - repo as argument' '
+	echo "otherrepo/.git" >expected &&
+	(
+		__gitdir "otherrepo" >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success '__gitdir - remote as argument' '
+	echo "remote" >expected &&
+	(
+		__gitdir "remote" >"$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
 test_expect_success '__gitcomp - trailing space - options' '
 	test_gitcomp "--re" "--dry-run --reuse-message= --reedit-message=
 		--reset-author" <<-EOF
-- 
2.11.0.555.g967c1bcb3

Re: [PATCHv2 00/21] completion: various __gitdir()-related improvements

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> This is a long overdue reroll of a bunch of bugfixes, cleanups and
> optimizations related to how the completion script finds the path to
> the repository and how it uses that path.  Most importantly this
> series adds support for completion following 'git -C path', and it
> eliminates a few subshells and git processes, for the sake of
> fork()+exec() challenged OSes.

Thanks. Queued.

[PATCH] completion: restore removed line continuating backslash

[SZEDER Gábor]

Recent commit 1cd23e9e0 (completion: don't use __gitdir() for git
commands, 2017-02-03) rewrapped a couple of long lines, and while
doing so it inadvertently removed a '\' from the end of a line, thus
breaking completion for 'git config remote.name.push <TAB>'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---

I wanted this to be a fixup!, but then noticed that this series is
already in next, hence the proper commit message.

I see the last What's cooking marks this series as "will merge to
master".  That doesn't mean that it will be in v2.12, does it?

 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 993085af6..f2b294aac 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2086,7 +2086,7 @@ _git_config ()
 	remote.*.push)
 		local remote="${prev#remote.}"
 		remote="${remote%.push}"
-		__gitcomp_nl "$(__git for-each-ref
+		__gitcomp_nl "$(__git for-each-ref \
 			--format='%(refname):%(refname)' refs/heads)"
 		return
 		;;
-- 
2.11.1.499.ge87add2e7

Re: [PATCH] completion: restore removed line continuating backslash

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

> Recent commit 1cd23e9e0 (completion: don't use __gitdir() for git
> commands, 2017-02-03) rewrapped a couple of long lines, and while
> doing so it inadvertently removed a '\' from the end of a line, thus
> breaking completion for 'git config remote.name.push <TAB>'.
>
> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
> ---
>
> I wanted this to be a fixup!, but then noticed that this series is
> already in next, hence the proper commit message.

We get "--format=... : command not found"?
Thanks for a set of sharp eyes.

> I see the last What's cooking marks this series as "will merge to
> master".  That doesn't mean that it will be in v2.12, does it?

I actually was hoping that these can go in.  Others that can (or
should) wait are marked as "Will cook in 'next'".

If you feel uncomfortable and want these to cook longer, please tell
me so.

Thanks.

>  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 993085af6..f2b294aac 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -2086,7 +2086,7 @@ _git_config ()
>  	remote.*.push)
>  		local remote="${prev#remote.}"
>  		remote="${remote%.push}"
> -		__gitcomp_nl "$(__git for-each-ref
> +		__gitcomp_nl "$(__git for-each-ref \
>  			--format='%(refname):%(refname)' refs/heads)"
>  		return
>  		;;

Re: [PATCH] completion: restore removed line continuating backslash

[SZEDER Gábor]

On Mon, Feb 13, 2017 at 9:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>> Recent commit 1cd23e9e0 (completion: don't use __gitdir() for git
>> commands, 2017-02-03) rewrapped a couple of long lines, and while
>> doing so it inadvertently removed a '\' from the end of a line, thus
>> breaking completion for 'git config remote.name.push <TAB>'.
>>
>> Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
>> ---
>>
>> I wanted this to be a fixup!, but then noticed that this series is
>> already in next, hence the proper commit message.
>
> We get "--format=... : command not found"?

Yeah, that's the one.

> Thanks for a set of sharp eyes.

Heh.  Sharp?!  It took over five minutes to notice after I first got
that error...

Furthermore, that '\' was already missing in v1, almost a year ago :)

>> I see the last What's cooking marks this series as "will merge to
>> master".  That doesn't mean that it will be in v2.12, does it?
>
> I actually was hoping that these can go in.  Others that can (or
> should) wait are marked as "Will cook in 'next'".
>
> If you feel uncomfortable and want these to cook longer, please tell
> me so.

Well, it was mainly my surprise that a 20+ patch series arriving so
late that it gets queued on top of -rc0 would still make it into the
release.  However, I have been using this series with only minor
modifications for the better part of a year now, so it's unlikely that
there will be any big issues with it.  Maybe something small, like
this missing '\', but we will deal with it when it arises.

Gábor

Re: [PATCH] completion: restore removed line continuating backslash

[Junio C Hamano]

SZEDER Gábor <szeder.dev@gmail.com> writes:

>> If you feel uncomfortable and want these to cook longer, please tell
>> me so.
>
> Well, it was mainly my surprise that a 20+ patch series arriving so
> late that it gets queued on top of -rc0 would still make it into the
> release.

It all depends on what area the changes are about ;-)

Not meaning to make light of your contribution, but if the upcoming
version of Git shipped with a slightly broken completion, and the
breakage is severe enough, the only thing we need to do is to do a
maintenance release that reverts a single script.  Distro packagers
may do that for us.  While waiting, the user can choose not to load
the completion and can keep using Git just fine.  A broken
"mergetool" would be similar in that a workaround to inspect "git
diff" is available.  If we break "pull" or "commit" on the other
hand, the necessary workaround would be a lot more involved.

Some changes in low-impact areas can wait without reducing the
end-user value of the new release, but if the risk of regression is
small, I favour merging them, rather than postponing them for one
cycle, if only to reduce the number of patches that I need to hold
onto.  Patches to clarify existing documentation fall into this
category.

My perception of "risk of regression" obviously is affected by the
size of the series, and 20+ is certainly on the larger side.  But
other things also come into the picture.  Patches from an author who
knows the area the patches touch very well and with track record of
not causing embarrassing regressions, would make me feel safer than
patches from others, for example.

Re: [PATCH] completion: restore removed line continuating backslash

[SZEDER Gábor]

On Wed, Feb 15, 2017 at 3:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> SZEDER Gábor <szeder.dev@gmail.com> writes:
>
>>> If you feel uncomfortable and want these to cook longer, please tell
>>> me so.
>>
>> Well, it was mainly my surprise that a 20+ patch series arriving so
>> late that it gets queued on top of -rc0 would still make it into the
>> release.
>
> It all depends on what area the changes are about ;-)

Sure.  I actually wanted to end that email with something like "and
it's in contrib anyway :)", but by the time I got there I forgot about
it.