threads / patch / 30479

patch, 19 partsBash prompt speedup

Subject: [PATCH 00/19] Bash prompt speedup

## tl;dr

50 messages between May 9, 2012 and May 10, 2012. Diffs are folded; open one to read it.

replies: 49people: 4as markdown or json

SZEDER Gábor· May 9, 2012, 00:44 UTC · lore
Hi,

It was pointed out about a year ago that displaying the git-specific bash prompt on Windows/MinGW takes quite long, long enough to be noticeable. And indeed, while in a subdirectory of a repository and stash indicator enabled and my laptop running from battery I get this:

  $ time prompt=$(__git_ps1)
  real    0m0.412s
  user    0m0.048s
  sys     0m0.210s

This is mainly caused by the numerous fork()s and exec()s to create subshells and run git commands, which are rather expensive on Windows.

This patch series eliminates many command substitutions and git commands from __git_ps1() by reorganizing code or replacing them with bash builtins. This speeds up the prompt immensely: now I get the same prompt as above in about 10ms(!). Timing results are shown in the log message of patch 19.

Unfortunately, to achive this users have to change their configuration, in particular change their $PS1 and $PROMPT_COMMAND (see patch 19) and should enable the discovery of git repositories across filesystem boundaries (see patch 10).

There are two RFC patches in there (9 and 18), please have a look.
Here's an outline of the series:

First, a couple of tests for the bash prompt, to lessen the chance that I break something ;) These are basically the same patches that I sent out a while ago in [1], the only noteworthy change is that I renamed the helper library to lib-bash.sh (from lib-completion.sh), because all it does is to run tests under bash and there is nothing completion-specific in there.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/195587/focus=195711
  [PATCH 01/19] tests: move code to run tests under bash into a helper library
  [PATCH 02/19] tests: add tests for the bash prompt functions in the completion script
The next four patches contain a couple of fixes and cleanups:
  [PATCH 03/19] completion: use __gitdir() in _git_log()
  [PATCH 04/19] completion: respect $GIT_DIR
  [PATCH 05/19] bash prompt: don't show the prompt when .git/HEAD is unreadable
  [PATCH 06/19] bash prompt: return early from __git_ps1() when not in a git repository

These four make __gitdir() faster, so they'll benefit not only the bash prompt but completing refs, aliases, config variables, etc., too.

  [PATCH 07/19] completion: make __gitdir() store repository path in $__git_dir
  [PATCH 08/19] completion: use $__git_dir instead of $(__gitdir)
  [RFC PATCH 09/19] completion: platform-specific helper function to get physical path
  [PATCH 10/19] completion: use bash builtins to search for repository

These three make the main codepath in __git_ps1() faster by eliminating a couple of command substitutions and git commands.

  [PATCH 11/19] bash prompt: use bash builtins to find out current branch
  [PATCH 12/19] bash prompt: use bash builtins to check whether inside git dir
  [PATCH 13/19] bash prompt: check whether inside the worktree only when necessary

Eliminate some more command substitutions and git commands that are not necessarily run for every prompt:

  [PATCH 14/19] bash prompt: use bash builtins to find out current branch during rebase
  [PATCH 15/19] bash prompt: use bash builtins to get detached HEAD abbrev. object name
  [PATCH 16/19] bash prompt: display stash and upstream state even inside the repository
  [PATCH 17/19] bash prompt: use bash builtins to check stash state
  [RFC PATCH 18/19] bash prompt: avoid command substitution when checking for untracked files

And finally get rid of the command substitution used to include the git prompt in $PS1:

  [PATCH 19/19] bash prompt: alternative git prompt without command substitution
Enjoy.

Best, Gábor

SZEDER Gábor (19):
  tests: move code to run tests under bash into a helper library
  tests: add tests for the bash prompt functions in the completion
    script
  completion: use __gitdir() in _git_log()
  completion: respect $GIT_DIR
  bash prompt: don't show the prompt when .git/HEAD is unreadable
  bash prompt: return early from __git_ps1() when not in a git
    repository
  completion: make __gitdir() store repository path in $__git_dir
  completion: use $__git_dir instead of $(__gitdir)
  completion: platform-specific helper function to get physical path
  completion: use bash builtins to search for repository
  bash prompt: use bash builtins to find out current branch
  bash prompt: use bash builtins to check whether inside git dir
  bash prompt: check whether inside the worktree only when necessary
  bash prompt: use bash builtins to find out current branch during
    rebase
  bash prompt: use bash builtins to get detached HEAD abbrev. object
    name
  bash prompt: display stash and upstream state even inside the
    repository
  bash prompt: use bash builtins to check stash state
  bash prompt: avoid command substitution when checking for untracked
    files
  bash prompt: alternative git prompt without command substitution
 contrib/completion/git-completion.bash | 315 ++++++++++-------
 t/lib-bash.sh                          |  18 +
 t/t9902-completion.sh                  |  14 +-
 t/t9903-bash-prompt.sh                 | 593 +++++++++++++++++++++++++++++++++
 4 files changed, 815 insertions(+), 125 deletions(-)
 create mode 100644 t/lib-bash.sh
 create mode 100755 t/t9903-bash-prompt.sh
-- 
1.7.10.1.541.gb1be298
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 01/19] tests: move code to run tests under bash into a helper library

The following patch will add tests for the bash prompt functions as a new test script, which also has to be run under bash.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 t/lib-bash.sh         | 18 ++++++++++++++++++
 t/t9902-completion.sh | 14 +-------------
 2 files changed, 19 insertions(+), 13 deletions(-)
 create mode 100755 t/lib-bash.sh
Show changes to 2 files +19 −13

t/lib-bash.sh, t/t9902-completion.sh

diff --git a/t/lib-bash.sh b/t/lib-bash.sh
new file mode 100644
index 00000000..11397f74
--- /dev/null
+++ b/t/lib-bash.sh
@@ -0,0 +1,18 @@
+#!/bin/sh
+#
+# Ensures that tests are run under Bash; primarily intended for running tests
+# of the completion script.
+
+if test -n "$BASH" && test -z "$POSIXLY_CORRECT"; then
+	# we are in full-on bash mode
+	true
+elif type bash >/dev/null 2>&1; then
+	# execute in full-on bash mode
+	unset POSIXLY_CORRECT
+	exec bash "$0" "$@"
+else
+	echo '1..0 #SKIP skipping bash completion tests; bash not available'
+	exit 0
+fi
+
+. ./test-lib.sh
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 5bda6b6e..a0ea9463 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -3,21 +3,9 @@
 # Copyright (c) 2012 Felipe Contreras
 #
 
-if test -n "$BASH" && test -z "$POSIXLY_CORRECT"; then
-	# we are in full-on bash mode
-	true
-elif type bash >/dev/null 2>&1; then
-	# execute in full-on bash mode
-	unset POSIXLY_CORRECT
-	exec bash "$0" "$@"
-else
-	echo '1..0 #SKIP skipping bash completion tests; bash not available'
-	exit 0
-fi
-
 test_description='test bash completion'
 
-. ./test-lib.sh
+. ./lib-bash.sh
 
 complete ()
 {
-- 
1.7.10.1.541.gb1be298
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 02/19] tests: add tests for the bash prompt functions in the completion script

The tests cover the discovery of the '.git' directory in the __gitdir() function in different scenarios, and the prompt itself, i.e. branch name, detached heads, operations (rebase, merge, cherry-pick, bisect), and status indicators (dirty, stash, untracked files; but not the upstream status).

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 t/t9903-bash-prompt.sh | 448 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 448 insertions(+)
 create mode 100755 t/t9903-bash-prompt.sh
Show changes to t/t9903-bash-prompt.sh +448 −0
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
new file mode 100755
index 00000000..a6c9ce94
--- /dev/null
+++ b/t/t9903-bash-prompt.sh
@@ -0,0 +1,448 @@
+#!/bin/sh
+#
+# Copyright (c) 2012 SZEDER Gábor
+#
+
+test_description='test git-specific bash prompt functions'
+
+. ./lib-bash.sh
+
+. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
+
+actual="$TRASH_DIRECTORY/actual"
+
+test_expect_success 'setup for prompt tests' '
+	mkdir -p subdir/subsubdir &&
+	git init otherrepo &&
+	echo 1 > file &&
+	git add file &&
+	test_tick &&
+	git commit -m initial &&
+	git tag -a -m msg1 t1 &&
+	git checkout -b b1 &&
+	echo 2 > file &&
+	git commit -m "second b1" file &&
+	echo 3 > file &&
+	git commit -m "third b1" file &&
+	git tag -a -m msg2 t2 &&
+	git checkout -b b2 master &&
+	echo 0 > file &&
+	git commit -m "second b2" file &&
+	git checkout master
+'
+
+test_expect_success 'gitdir - from command line (through $__git_dir)' '
+	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
+	(
+		__git_dir="$TRASH_DIRECTORY/otherrepo/.git" &&
+		__gitdir > "$actual"
+	) &&
+	test_cmp expected "$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 'gitdir - .git directory in cwd' '
+	echo ".git" > expected &&
+	__gitdir > "$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'gitdir - .git directory in parent' '
+	echo "$TRASH_DIRECTORY/.git" > expected &&
+	(
+		cd subdir/subsubdir &&
+		__gitdir > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'gitdir - cwd is a .git directory' '
+	echo "." > expected &&
+	(
+		cd .git &&
+		__gitdir > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'gitdir - parent is a .git directory' '
+	echo "$TRASH_DIRECTORY/.git" > expected &&
+	(
+		cd .git/refs/heads &&
+		__gitdir > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_failure 'gitdir - $GIT_DIR set while .git directory in cwd' '
+	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
+	(
+		GIT_DIR="$TRASH_DIRECTORY/otherrepo/.git" &&
+		export GIT_DIR &&
+		__gitdir > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'gitdir - $GIT_DIR set while .git directory in parent' '
+	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
+	(
+		GIT_DIR="$TRASH_DIRECTORY/otherrepo/.git" &&
+		export GIT_DIR &&
+		cd subdir &&
+		__gitdir > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'gitdir - gitfile in cwd' '
+	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
+	echo "gitdir: $TRASH_DIRECTORY/otherrepo/.git" > subdir/.git &&
+	test_when_finished "rm -f subdir/.git" &&
+	(
+		cd subdir &&
+		__gitdir > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'gitdir - gitfile in parent' '
+	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
+	echo "gitdir: $TRASH_DIRECTORY/otherrepo/.git" > subdir/.git &&
+	test_when_finished "rm -f subdir/.git" &&
+	(
+		cd subdir/subsubdir &&
+		__gitdir > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success SYMLINKS 'gitdir - resulting path avoids symlinks' '
+	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
+	mkdir otherrepo/dir &&
+	test_when_finished "rm -rf otherrepo/dir" &&
+	ln -s otherrepo/dir link &&
+	test_when_finished "rm -f link" &&
+	(
+		cd link &&
+		__gitdir > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'gitdir - not a git repository' '
+	(
+		cd subdir/subsubdir &&
+		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY" &&
+		export GIT_CEILING_DIRECTORIES &&
+		test_must_fail __gitdir
+	)
+'
+
+test_expect_success 'prompt - branch name' '
+	printf " (master)" > expected &&
+	__git_ps1 > "$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - detached head' '
+	printf " ((%s...))" $(git log -1 --format="%h" b1^) > expected &&
+	git checkout b1^ &&
+	test_when_finished "git checkout master" &&
+	__git_ps1 > "$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - describe detached head - contains' '
+	printf " ((t2~1))" > expected &&
+	git checkout b1^ &&
+	test_when_finished "git checkout master" &&
+	(
+		GIT_PS1_DESCRIBE_STYLE=contains &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - describe detached head - branch' '
+	printf " ((b1~1))" > expected &&
+	git checkout b1^ &&
+	test_when_finished "git checkout master" &&
+	(
+		GIT_PS1_DESCRIBE_STYLE=branch &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - describe detached head - describe' '
+	printf " ((t1-1-g%s))" $(git log -1 --format="%h" b1^) > expected &&
+	git checkout b1^ &&
+	test_when_finished "git checkout master" &&
+	(
+		GIT_PS1_DESCRIBE_STYLE=describe &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - describe detached head - default' '
+	printf " ((t2))" > expected &&
+	git checkout --detach b1 &&
+	test_when_finished "git checkout master" &&
+	__git_ps1 > "$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - inside .git directory' '
+	printf " (GIT_DIR!)" > expected &&
+	(
+		cd .git &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - deep inside .git directory' '
+	printf " (GIT_DIR!)" > expected &&
+	(
+		cd .git/refs/heads &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - inside bare repository' '
+	printf " (BARE:master)" > expected &&
+	git init --bare bare.git &&
+	test_when_finished "rm -rf bare.git" &&
+	(
+		cd bare.git &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - interactive rebase' '
+	printf " (b1|REBASE-i)" > expected
+	echo "#!$SHELL_PATH" >fake_editor.sh &&
+	cat >>fake_editor.sh <<\EOF &&
+echo "edit $(git log -1 --format="%h")" > "$1"
+EOF
+	test_when_finished "rm -f fake_editor.sh" &&
+	chmod a+x fake_editor.sh &&
+	test_set_editor "$TRASH_DIRECTORY/fake_editor.sh" &&
+	git checkout b1 &&
+	test_when_finished "git checkout master" &&
+	git rebase -i HEAD^ &&
+	test_when_finished "git rebase --abort"
+	__git_ps1 > "$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - rebase merge' '
+	printf " (b2|REBASE-m)" > expected &&
+	git checkout b2 &&
+	test_when_finished "git checkout master" &&
+	test_must_fail git rebase --merge b1 b2 &&
+	test_when_finished "git rebase --abort" &&
+	__git_ps1 > "$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - rebase' '
+	printf " ((t2)|REBASE)" > expected &&
+	git checkout b2 &&
+	test_when_finished "git checkout master" &&
+	test_must_fail git rebase b1 b2 &&
+	test_when_finished "git rebase --abort" &&
+	__git_ps1 > "$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - merge' '
+	printf " (b1|MERGING)" > expected &&
+	git checkout b1 &&
+	test_when_finished "git checkout master" &&
+	test_must_fail git merge b2 &&
+	test_when_finished "git reset --hard" &&
+	__git_ps1 > "$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - cherry-pick' '
+	printf " (master|CHERRY-PICKING)" > expected &&
+	test_must_fail git cherry-pick b1 &&
+	test_when_finished "git reset --hard" &&
+	__git_ps1 > "$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - bisect' '
+	printf " (master|BISECTING)" > expected &&
+	git bisect start &&
+	test_when_finished "git bisect reset" &&
+	__git_ps1 > "$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - dirty status indicator - clean' '
+	printf " (master)" > expected &&
+	(
+		GIT_PS1_SHOWDIRTYSTATE=y &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - dirty status indicator - dirty worktree' '
+	printf " (master *)" > expected &&
+	echo "dirty" > file &&
+	test_when_finished "git reset --hard" &&
+	(
+		GIT_PS1_SHOWDIRTYSTATE=y &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - dirty status indicator - dirty index' '
+	printf " (master +)" > expected &&
+	echo "dirty" > file &&
+	test_when_finished "git reset --hard" &&
+	git add -u &&
+	(
+		GIT_PS1_SHOWDIRTYSTATE=y &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - dirty status indicator - dirty index and worktree' '
+	printf " (master *+)" > expected &&
+	echo "dirty index" > file &&
+	test_when_finished "git reset --hard" &&
+	git add -u &&
+	echo "dirty worktree" > file &&
+	(
+		GIT_PS1_SHOWDIRTYSTATE=y &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - dirty status indicator - before root commit' '
+	printf " (master #)" > expected &&
+	(
+		GIT_PS1_SHOWDIRTYSTATE=y &&
+		cd otherrepo &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - dirty status indicator - disabled by config' '
+	printf " (master)" > expected &&
+	echo "dirty" > file &&
+	test_when_finished "git reset --hard" &&
+	test_config bash.showDirtyState false &&
+	(
+		GIT_PS1_SHOWDIRTYSTATE=y &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - dirty status indicator - not shown inside .git directory' '
+	printf " (GIT_DIR!)" > expected &&
+	echo "dirty" > file &&
+	test_when_finished "git reset --hard" &&
+	(
+		GIT_PS1_SHOWDIRTYSTATE=y &&
+		cd .git &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - stash status indicator - no stash' '
+	printf " (master)" > expected &&
+	(
+		GIT_PS1_SHOWSTASHSTATE=y &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - stash status indicator - stash' '
+	printf " (master $)" > expected &&
+	echo 2 >file &&
+	git stash &&
+	test_when_finished "git stash drop" &&
+	(
+		GIT_PS1_SHOWSTASHSTATE=y &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - stash status indicator - not shown inside .git directory' '
+	printf " (GIT_DIR!)" > expected &&
+	echo 2 >file &&
+	git stash &&
+	test_when_finished "git stash drop" &&
+	(
+		GIT_PS1_SHOWSTASHSTATE=y &&
+		cd .git &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - untracked files status indicator - no untracked files' '
+	printf " (master)" > expected &&
+	(
+		GIT_PS1_SHOWUNTRACKEDFILES=y &&
+		cd otherrepo &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - untracked files status indicator - untracked files' '
+	printf " (master %%)" > expected &&
+	(
+		GIT_PS1_SHOWUNTRACKEDFILES=y &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - untracked files status indicator - not shown inside .git directory' '
+	printf " (GIT_DIR!)" > expected &&
+	(
+		GIT_PS1_SHOWUNTRACKEDFILES=y &&
+		cd .git &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - format string starting with dash' '
+	printf -- "-master" > expected &&
+	__git_ps1 "-%s" > "$actual" &&
+	test_cmp expected "$actual"
+'
+
+test_done
-- 
1.7.10.1.541.gb1be298
Johannes Sixt· May 9, 2012, 08:07 UTC · re: SZEDER Gábor · lore

Re: [PATCH 02/19] tests: add tests for the bash prompt functions in the completion script

Am 5/9/2012 2:44, schrieb SZEDER Gábor:
Show 5 quoted lines
> The tests cover the discovery of the '.git' directory in the
> __gitdir() function in different scenarios, and the prompt itself,
> i.e. branch name, detached heads, operations (rebase, merge,
> cherry-pick, bisect), and status indicators (dirty, stash, untracked
> files; but not the upstream status).

The following patch contains fixups are needed to pass the tests at this point plus a few more changes.

--- >8 ---
From: Johannes Sixt <j6t@kdbg.org>
Subject: [PATCH] fixup! tests: add tests for the bash prompt functions in the
 completion script
- $TRASH_DIRECTORY is /c/dir style path, but git produces c:/dir style
  paths. Use $(pwd), which is short for $(pwd -W) that produces the
  latter, to assemble expected test data and paths read by git.
- As long as GIT_DIR is only exported for use by git and not used by
  the completion functions, the move to $(pwd) is only cosmetic and
  for consistency.
- Insert a #!/bin/sh in the shell script to ensure the test will pass
  should git-rebase ever be ported to C.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t9903-bash-prompt.sh |   29 +++++++++++++++--------------
 1 file changed, 15 insertions(+), 14 deletions(-)
Show changes to t/t9903-bash-prompt.sh +15 −14
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index a6c9ce9..3880c56 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -32,9 +32,9 @@ test_expect_success 'setup for prompt tests' '
 '
 
 test_expect_success 'gitdir - from command line (through $__git_dir)' '
-	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
+	echo "$(pwd)/otherrepo/.git" > expected &&
 	(
-		__git_dir="$TRASH_DIRECTORY/otherrepo/.git" &&
+		__git_dir="$(pwd)/otherrepo/.git" &&
 		__gitdir > "$actual"
 	) &&
 	test_cmp expected "$actual"
@@ -59,7 +59,7 @@ test_expect_success 'gitdir - .git directory in cwd' '
 '
 
 test_expect_success 'gitdir - .git directory in parent' '
-	echo "$TRASH_DIRECTORY/.git" > expected &&
+	echo "$(pwd)/.git" > expected &&
 	(
 		cd subdir/subsubdir &&
 		__gitdir > "$actual"
@@ -77,7 +77,7 @@ test_expect_success 'gitdir - cwd is a .git directory' '
 '
 
 test_expect_success 'gitdir - parent is a .git directory' '
-	echo "$TRASH_DIRECTORY/.git" > expected &&
+	echo "$(pwd)/.git" > expected &&
 	(
 		cd .git/refs/heads &&
 		__gitdir > "$actual"
@@ -86,9 +86,9 @@ test_expect_success 'gitdir - parent is a .git directory' '
 '
 
 test_expect_failure 'gitdir - $GIT_DIR set while .git directory in cwd' '
-	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
+	echo "$(pwd)/otherrepo/.git" > expected &&
 	(
-		GIT_DIR="$TRASH_DIRECTORY/otherrepo/.git" &&
+		GIT_DIR="$(pwd)/otherrepo/.git" &&
 		export GIT_DIR &&
 		__gitdir > "$actual"
 	) &&
@@ -96,9 +96,9 @@ test_expect_failure '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 "$(pwd)/otherrepo/.git" > expected &&
 	(
-		GIT_DIR="$TRASH_DIRECTORY/otherrepo/.git" &&
+		GIT_DIR="$(pwd)/otherrepo/.git" &&
 		export GIT_DIR &&
 		cd subdir &&
 		__gitdir > "$actual"
@@ -107,8 +107,8 @@ test_expect_success 'gitdir - $GIT_DIR set while .git directory in parent' '
 '
 
 test_expect_success 'gitdir - gitfile in cwd' '
-	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
-	echo "gitdir: $TRASH_DIRECTORY/otherrepo/.git" > subdir/.git &&
+	echo "$(pwd)/otherrepo/.git" > expected &&
+	echo "gitdir: $(pwd)/otherrepo/.git" > subdir/.git &&
 	test_when_finished "rm -f subdir/.git" &&
 	(
 		cd subdir &&
@@ -118,8 +118,8 @@ test_expect_success 'gitdir - gitfile in cwd' '
 '
 
 test_expect_success 'gitdir - gitfile in parent' '
-	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
-	echo "gitdir: $TRASH_DIRECTORY/otherrepo/.git" > subdir/.git &&
+	echo "$(pwd)/otherrepo/.git" > expected &&
+	echo "gitdir: $(pwd)/otherrepo/.git" > subdir/.git &&
 	test_when_finished "rm -f subdir/.git" &&
 	(
 		cd subdir/subsubdir &&
@@ -129,7 +129,7 @@ test_expect_success 'gitdir - gitfile in parent' '
 '
 
 test_expect_success SYMLINKS 'gitdir - resulting path avoids symlinks' '
-	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
+	echo "$(pwd)/otherrepo/.git" > expected &&
 	mkdir otherrepo/dir &&
 	test_when_finished "rm -rf otherrepo/dir" &&
 	ln -s otherrepo/dir link &&
@@ -238,11 +238,12 @@ test_expect_success 'prompt - interactive rebase' '
 	printf " (b1|REBASE-i)" > expected
 	echo "#!$SHELL_PATH" >fake_editor.sh &&
 	cat >>fake_editor.sh <<\EOF &&
+#!/bin/sh
 echo "edit $(git log -1 --format="%h")" > "$1"
 EOF
 	test_when_finished "rm -f fake_editor.sh" &&
 	chmod a+x fake_editor.sh &&
-	test_set_editor "$TRASH_DIRECTORY/fake_editor.sh" &&
+	test_set_editor "$(pwd)/fake_editor.sh" &&
 	git checkout b1 &&
 	test_when_finished "git checkout master" &&
 	git rebase -i HEAD^ &&
-- 
1.7.10.1.1689.gacdfbde
Junio C Hamano· May 9, 2012, 18:08 UTC · re: Johannes Sixt · lore

Re: [PATCH 02/19] tests: add tests for the bash prompt functions in the completion script

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 7 quoted lines
> From: Johannes Sixt <j6t@kdbg.org>
> Subject: [PATCH] fixup! tests: add tests for the bash prompt functions in the
>  completion script
>
> - $TRASH_DIRECTORY is /c/dir style path, but git produces c:/dir style
>   paths. Use $(pwd), which is short for $(pwd -W) that produces the
>   latter, to assemble expected test data and paths read by git.

This comes up very often whenever somebody (including me) touches test scripts. We do have a write-up in t/README in "Dos and Don'ts" section, but apparently that is not sufficient to avoid fix-ups.

Would it be possible to arrange so that $TRASH_DIRECTORY, $TEST_DIRECTORY and $PWD are set to c:/dir style paths in Windows environment? What would we break if we did so?

The other direction of changing the Windows port of git to produce /c/dir style paths would probably not work, as it would involve ripping out the path mangling feature of bash in MSYS, which is done for some reason, I presume.

Johannes Sixt· May 10, 2012, 06:09 UTC · re: Junio C Hamano · lore

Re: [PATCH 02/19] tests: add tests for the bash prompt functions in the completion script

Am 5/9/2012 20:08, schrieb Junio C Hamano:
> Would it be possible to arrange so that $TRASH_DIRECTORY, $TEST_DIRECTORY
> and $PWD are set to c:/dir style paths in Windows environment?  What would
> we break if we did so?

I don't think that this is possible. The POSIX emulation in MSYS (or bash, dunno) always converts to POSIX style:

> bash -c "cd c:/temp; PWD=$(pwd -W); echo $PWD; cd $PWD; pwd; echo $PWD"

c:/temp /c/temp /c/temp

That is, even if I use Windows style path to chdir around, the result is POSIX style.

> The other direction of changing the Windows port of git to produce /c/dir
> style paths would probably not work, as it would involve ripping out the
> path mangling feature of bash in MSYS, which is done for some reason, I
> presume.

Doing that is certainly not something that I would be prepared to do just to make the bash prompt work right ;-) And the added maintainance burden incurred by the status quo is certainly much cheaper than yet another fork of MSYS and/or bash.

-- Hannes
Junio C Hamano· May 9, 2012, 18:36 UTC · re: SZEDER Gábor · lore

Re: [PATCH 02/19] tests: add tests for the bash prompt functions in the completion script

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 7 quoted lines
> The tests cover the discovery of the '.git' directory in the __gitdir()
> function in different scenarios, and the prompt itself, i.e. branch
> name, detached heads, operations (rebase, merge, cherry-pick, bisect),
> and status indicators (dirty, stash, untracked files; but not the
> upstream status).
>
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>

Looks like a quite comprehensive tests around GIT_PS1_$MANY_DIFFERENT_STYLES (except that GIT_PS1_SHOWUPSTREAM seems to be missing); very nice.

> +	echo 1 > file &&

When you are going to re-roll to add the missing SHOWUPSTREAM test, in addition to J6t's $PWD vs $(pwd) vs $TRASH_DIRECTORY fix, please fix these redirections to match the coding styles (i.e. "cmd >file" and "cmd <file", with SP before and without SP after redirection operators).

SZEDER Gábor· May 9, 2012, 20:33 UTC · re: Junio C Hamano · lore

Re: [PATCH 02/19] tests: add tests for the bash prompt functions in the completion script

On Wed, May 09, 2012 at 11:36:25AM -0700, Junio C Hamano wrote:
Show 12 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > The tests cover the discovery of the '.git' directory in the __gitdir()
> > function in different scenarios, and the prompt itself, i.e. branch
> > name, detached heads, operations (rebase, merge, cherry-pick, bisect),
> > and status indicators (dirty, stash, untracked files; but not the
> > upstream status).
> >
> > Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
> 
> Looks like a quite comprehensive tests around GIT_PS1_$MANY_DIFFERENT_STYLES
> (except that GIT_PS1_SHOWUPSTREAM seems to be missing); very nice.

I've mentioned in the commit message that "... status indicators ... but not the upstream status". I didn't wrote tests for that because this series doesn't changes anything in the function producing the upstream status indicator.

However, thinking about it now, this series, in particular patch 16 (bash prompt: display stash and upstream state even inside the repository), does change the context in which that function might be invoked, i.e. not only from the work tree but even from within the repository. I don't think that would break anything (famous last words ;), because that function runs git config, rev-list, and log, and AFAICT these commands should work in a repository just as well, and the rest of the function is just preparing their arguments and processing their output.

Anyway, it would be definitely better to have a test to show that it indeed works from within a repository, but I didn't want to fiddle with svn upstreams. Perhaps patch 16 should leave the upstream status indicator as it is until someone ;) writes tests for it; since that function doesn't affect the main codepath and it involves several subshells and git processes anyway, there is not that much to be gained anyway.

Show 6 quoted lines
> > +	echo 1 > file &&
> 
> When you are going to re-roll to add the missing SHOWUPSTREAM test, in
> addition to J6t's $PWD vs $(pwd) vs $TRASH_DIRECTORY fix, please fix these
> redirections to match the coding styles (i.e. "cmd >file" and "cmd <file",
> with SP before and without SP after redirection operators).
OK.

I just followed suit of the recently added t9902-completion.sh, which uses SP on both sides of redirection operators.

SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 03/19] completion: use __gitdir() in _git_log()

The standard way to find out the path to the repository in the completion script is the __gitdir() helper function, because that handles the repository path given on the command line (i.e. git --git-dir=/path/to/repo log --<TAB>). However, there is one exception: the completion function for 'git log' still uses 'git rev-parse --git-dir' directly, and could offer (or not) the '--merge' option erroneously when the repository is specified on the command line.

Use __gitdir() there, too.
Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/completion/git-completion.bash +1 −1
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 9f56ec7a..f17abccb 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1551,7 +1551,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"
-- 
1.7.10.1.541.gb1be298
Junio C Hamano· May 9, 2012, 18:41 UTC · re: SZEDER Gábor · lore

Re: [PATCH 03/19] completion: use __gitdir() in _git_log()

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 8 quoted lines
> The standard way to find out the path to the repository in the
> completion script is the __gitdir() helper function, because that
> handles the repository path given on the command line (i.e. git
> --git-dir=/path/to/repo log --<TAB>).  However, there is one
> exception: the completion function for 'git log' still uses 'git
> rev-parse --git-dir' directly, and could offer (or not) the '--merge'
> option erroneously when the repository is specified on the command
> line.

Here `--merge` is the visible symptom, and the real issue you fixed is that it used to be looking into a repository that is different from the user is working with, right [*1*]?

Well spotted, and the fix sounds correct.
Thanks.
[Footnote]

*1* I am just making sure I am reading the above right; I am not suggesting to omit description of visible symptom at all---quite the opposite, I do want to see these visible symptom descriptions in the log messages.

SZEDER Gábor· May 9, 2012, 19:01 UTC · re: Junio C Hamano · lore

Re: [PATCH 03/19] completion: use __gitdir() in _git_log()

On Wed, May 09, 2012 at 11:41:13AM -0700, Junio C Hamano wrote:
Show 14 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > The standard way to find out the path to the repository in the
> > completion script is the __gitdir() helper function, because that
> > handles the repository path given on the command line (i.e. git
> > --git-dir=/path/to/repo log --<TAB>).  However, there is one
> > exception: the completion function for 'git log' still uses 'git
> > rev-parse --git-dir' directly, and could offer (or not) the '--merge'
> > option erroneously when the repository is specified on the command
> > line.
> 
> Here `--merge` is the visible symptom, and the real issue you fixed is
> that it used to be looking into a repository that is different from the
> user is working with, right [*1*]?

Exactly; will add a sentence about it to be more explicit in the reroll.

Note, however, that this doesn't influence refs completion, because __git_refs() does use __gitdir(), so it will look into the right repository.

Gábor
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 04/19] completion: respect $GIT_DIR

The __gitdir() helper function finds out the path of the git repository by running 'git rev-parse --git-dir'. However, it has a shortcut first to avoid the overhead of running a git command in a subshell when the current directory is at the top of the work tree, i.e. when it contains a '.git' subdirectory.

If the 'GIT_DIR' environment variable is set then it specifies the path to the git repository, and the autodetection of the '.git' directory is not necessary. However, $GIT_DIR is only taken into acocunt by 'git rev-parse --git-dir', and the check for the '.git' subdirectory is performed first, so it wins over the path given in $GIT_DIR.

There are several completion (helper) functions that depend on __gitdir(), and when the above case triggers the completion script will do weird things, like offering refs, aliases, or stashes from a different repository, or displaying wrong or broken prompt, etc.

So check first whether $GIT_DIR is set, and only proceed with checking the '.git' directory in the current directory if it isn't. 'git rev-parse' would also check whether the path in $GIT_DIR is a proper '.git' directory, i.e. 'HEAD', 'refs/', and 'objects/' are present and accessible, but we don't have to be that thorough for the bash prompt. And we've lived with an equally permissive check for '.git' in the current working directory for years anyway.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash |  3 +++
 t/t9903-bash-prompt.sh                 | 10 +++++++++-
 2 files changed, 12 insertions(+), 1 deletion(-)
Show changes to 2 files +12 −1

contrib/completion/git-completion.bash, t/t9903-bash-prompt.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index f17abccb..ab26bdc8 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -77,6 +77,9 @@ __gitdir ()
 	if [ -z "${1-}" ]; then
 		if [ -n "${__git_dir-}" ]; then
 			echo "$__git_dir"
+		elif [ -n "${GIT_DIR-}" ]; then
+			test -d "${GIT_DIR-}" || return 1
+			echo "$GIT_DIR"
 		elif [ -d .git ]; then
 			echo .git
 		else
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index a6c9ce94..96468ceb 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -85,7 +85,7 @@ test_expect_success 'gitdir - parent is a .git directory' '
 	test_cmp expected "$actual"
 '
 
-test_expect_failure 'gitdir - $GIT_DIR set while .git directory in cwd' '
+test_expect_success 'gitdir - $GIT_DIR set while .git directory in cwd' '
 	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
 	(
 		GIT_DIR="$TRASH_DIRECTORY/otherrepo/.git" &&
@@ -106,6 +106,14 @@ test_expect_success 'gitdir - $GIT_DIR set while .git directory in parent' '
 	test_cmp expected "$actual"
 '
 
+test_expect_success 'gitdir - non-existing $GIT_DIR' '
+	(
+		GIT_DIR="$TRASH_DIRECTORY/non-existing" &&
+		export GIT_DIR &&
+		test_must_fail __gitdir
+	)
+'
+
 test_expect_success 'gitdir - gitfile in cwd' '
 	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
 	echo "gitdir: $TRASH_DIRECTORY/otherrepo/.git" > subdir/.git &&
-- 
1.7.10.1.541.gb1be298
Johannes Sixt· May 9, 2012, 08:09 UTC · re: SZEDER Gábor · lore

Re: [PATCH 04/19] completion: respect $GIT_DIR

Am 5/9/2012 2:44, schrieb SZEDER Gábor:
Show 7 quoted lines
> +test_expect_success 'gitdir - non-existing $GIT_DIR' '
> +	(
> +		GIT_DIR="$TRASH_DIRECTORY/non-existing" &&
> +		export GIT_DIR &&
> +		test_must_fail __gitdir
> +	)
> +'
Another fixup, but it is only for consistency.
--- >8 ---
From: Johannes Sixt <j6t@kdbg.org>
Subject: [PATCH] fixup! completion: respect $GIT_DIR
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 t/t9903-bash-prompt.sh |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to t/t9903-bash-prompt.sh +1 −1
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index cf8e0ca..0318288 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -108,7 +108,7 @@ 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="$(pwd)/non-existing" &&
 		export GIT_DIR &&
 		test_must_fail __gitdir
 	)
-- 
1.7.10.1.1689.gacdfbde
Junio C Hamano· May 9, 2012, 18:54 UTC · re: SZEDER Gábor · lore

Re: [PATCH 04/19] completion: respect $GIT_DIR

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 12 quoted lines
> The __gitdir() helper function finds out the path of the git
> repository by running 'git rev-parse --git-dir'.  However, it has a
> shortcut first to avoid the overhead of running a git command in a
> subshell when the current directory is at the top of the work tree,
> i.e. when it contains a '.git' subdirectory.
>
> If the 'GIT_DIR' environment variable is set then it specifies the
> path to the git repository, and the autodetection of the '.git'
> directory is not necessary.  However, $GIT_DIR is only taken into
> acocunt by 'git rev-parse --git-dir', and the check for the '.git'
> subdirectory is performed first, so it wins over the path given in
> $GIT_DIR.

Strictly speaking, you have to be a bit careful here, though. If GIT_DIR is set as a shell variable without being exported, it will not affect where the "git" process you will spawn from your interactive shell session will find the repository. Only when it is exported it does.

It is a different matter if the distinction matters in the real life, in other words, GIT_DIR set but not exported is a use case that is worth worrying about. But note that our own git-sh-setup script is one such use case, so I wouldn't be surprised if somebody uses it for a strange workflow (I suspect that might involve a working tree that has its .git dir in a totally unrelated place, and the user runs "GIT_DIR=$GIT_DIR git subcmd", using the set but not exported GIT_DIR as a typesaver).

Show 13 quoted lines
> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index a6c9ce94..96468ceb 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -85,7 +85,7 @@ test_expect_success 'gitdir - parent is a .git directory' '
>  	test_cmp expected "$actual"
>  '
>  
> -test_expect_failure 'gitdir - $GIT_DIR set while .git directory in cwd' '
> +test_expect_success 'gitdir - $GIT_DIR set while .git directory in cwd' '
>  	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
>  	(
>  		GIT_DIR="$TRASH_DIRECTORY/otherrepo/.git" &&

And it is a good thing that the next line that comes after the above context is "export GIT_DIR". If we were to declare "set but not exported" an uninteresting use case whose outcome is undefined (and that might be fine), this still tests the defined behaviour of having the GIT_DIR in the environment.

SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 05/19] bash prompt: don't show the prompt when .git/HEAD is unreadable

__git_ps1() has a chain of commands to figure out what to display in the prompt as current branch, i.e. the branch name, or a described detached head, or the 7 hexdigits abbreviated object name, or "(unknown)" when all of the above fails.

Now, when the 7 hexdigits case fails, then '/path/to/.git/HEAD' can't be read. This can happen when the file became unreadable after __gitdir() found it: unlikely, but a parallel process can racily delete it or change it's permissions in that short timeframe. Alternatively, it is possible that either the subshell or the 'cut' command fail. Either way, when HEAD is not readable, then the path is not considered to be a git repository, therefore the bash prompt shouldn't be displayed at all. And there is no point in continuing the execution of __git_ps1(), because its subsequent git commands will error out anyway.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to contrib/completion/git-completion.bash +1 −1
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ab26bdc8..cd6a5f12 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -258,7 +258,7 @@ __git_ps1 ()
 				esac 2>/dev/null)" ||
 
 				b="$(cut -c1-7 "$g/HEAD" 2>/dev/null)..." ||
-				b="unknown"
+				return
 				b="($b)"
 			}
 		fi
-- 
1.7.10.1.541.gb1be298
Junio C Hamano· May 9, 2012, 19:32 UTC · re: SZEDER Gábor · lore

Re: [PATCH 05/19] bash prompt: don't show the prompt when .git/HEAD is unreadable

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 10 quoted lines
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index ab26bdc8..cd6a5f12 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -258,7 +258,7 @@ __git_ps1 ()
>  				esac 2>/dev/null)" ||
>  
>  				b="$(cut -c1-7 "$g/HEAD" 2>/dev/null)..." ||
> -				b="unknown"
> +				return

While you are touching the vicinity of the code, could we lose that "cut" and replace it with "rev-parse --short HEAD", without the hardcoded 1-7?

I wondered if we can use a single "git describe" output for all the describe/default and failure cases but didn't come up with a good way to do so only by using bash built-ins.

SZEDER Gábor· May 9, 2012, 19:45 UTC · re: Junio C Hamano · lore

Re: [PATCH 05/19] bash prompt: don't show the prompt when .git/HEAD is unreadable

On Wed, May 09, 2012 at 12:32:36PM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> > index ab26bdc8..cd6a5f12 100755
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -258,7 +258,7 @@ __git_ps1 ()
> >  				esac 2>/dev/null)" ||
> >  
> >  				b="$(cut -c1-7 "$g/HEAD" 2>/dev/null)..." ||
> > -				b="unknown"
> > +				return
> 
> While you are touching the vicinity of the code, could we lose that "cut"
> and replace it with "rev-parse --short HEAD", without the hardcoded 1-7?

Patch 15 (bash prompt: use bash builtins to get detached HEAD abbrev. object name) eliminates that 'cut' with some parameter expansions. While it doesn't respect the 'core.abbrev' config variable, it's much much faster, so IMHO it's worth it.

Gábor
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 06/19] bash prompt: return early from __git_ps1() when not in a git repository

... to gain one level of indentation for the bulk of the function.

(The patch looks quite unreadable, you'd better check it with 'git diff -w'.)

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 154 +++++++++++++++++----------------
 1 file changed, 78 insertions(+), 76 deletions(-)
Show changes to contrib/completion/git-completion.bash +78 −76
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index cd6a5f12..eaa3df9d 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -217,94 +217,96 @@ __git_ps1_show_upstream ()
 __git_ps1 ()
 {
 	local g="$(__gitdir)"
-	if [ -n "$g" ]; then
-		local r=""
-		local b=""
-		if [ -f "$g/rebase-merge/interactive" ]; then
-			r="|REBASE-i"
-			b="$(cat "$g/rebase-merge/head-name")"
-		elif [ -d "$g/rebase-merge" ]; then
-			r="|REBASE-m"
-			b="$(cat "$g/rebase-merge/head-name")"
-		else
-			if [ -d "$g/rebase-apply" ]; then
-				if [ -f "$g/rebase-apply/rebasing" ]; then
-					r="|REBASE"
-				elif [ -f "$g/rebase-apply/applying" ]; then
-					r="|AM"
-				else
-					r="|AM/REBASE"
-				fi
-			elif [ -f "$g/MERGE_HEAD" ]; then
-				r="|MERGING"
-			elif [ -f "$g/CHERRY_PICK_HEAD" ]; then
-				r="|CHERRY-PICKING"
-			elif [ -f "$g/BISECT_LOG" ]; then
-				r="|BISECTING"
-			fi
+	if [ -z "$g" ]; then
+		return
+	fi
 
-			b="$(git symbolic-ref HEAD 2>/dev/null)" || {
-
-				b="$(
-				case "${GIT_PS1_DESCRIBE_STYLE-}" in
-				(contains)
-					git describe --contains HEAD ;;
-				(branch)
-					git describe --contains --all HEAD ;;
-				(describe)
-					git describe HEAD ;;
-				(* | default)
-					git describe --tags --exact-match HEAD ;;
-				esac 2>/dev/null)" ||
-
-				b="$(cut -c1-7 "$g/HEAD" 2>/dev/null)..." ||
-				return
-				b="($b)"
-			}
+	local r=""
+	local b=""
+	if [ -f "$g/rebase-merge/interactive" ]; then
+		r="|REBASE-i"
+		b="$(cat "$g/rebase-merge/head-name")"
+	elif [ -d "$g/rebase-merge" ]; then
+		r="|REBASE-m"
+		b="$(cat "$g/rebase-merge/head-name")"
+	else
+		if [ -d "$g/rebase-apply" ]; then
+			if [ -f "$g/rebase-apply/rebasing" ]; then
+				r="|REBASE"
+			elif [ -f "$g/rebase-apply/applying" ]; then
+				r="|AM"
+			else
+				r="|AM/REBASE"
+			fi
+		elif [ -f "$g/MERGE_HEAD" ]; then
+			r="|MERGING"
+		elif [ -f "$g/CHERRY_PICK_HEAD" ]; then
+			r="|CHERRY-PICKING"
+		elif [ -f "$g/BISECT_LOG" ]; then
+			r="|BISECTING"
 		fi
 
-		local w=""
-		local i=""
-		local s=""
-		local u=""
-		local c=""
-		local p=""
+		b="$(git symbolic-ref HEAD 2>/dev/null)" || {
+
+			b="$(
+			case "${GIT_PS1_DESCRIBE_STYLE-}" in
+			(contains)
+				git describe --contains HEAD ;;
+			(branch)
+				git describe --contains --all HEAD ;;
+			(describe)
+				git describe HEAD ;;
+			(* | default)
+				git describe --tags --exact-match HEAD ;;
+			esac 2>/dev/null)" ||
+
+			b="$(cut -c1-7 "$g/HEAD" 2>/dev/null)..." ||
+			return
+			b="($b)"
+		}
+	fi
 
-		if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then
-			if [ "true" = "$(git rev-parse --is-bare-repository 2>/dev/null)" ]; then
-				c="BARE:"
-			else
-				b="GIT_DIR!"
-			fi
-		elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
-			if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
-				if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
-					git diff --no-ext-diff --quiet --exit-code || w="*"
-					if git rev-parse --quiet --verify HEAD >/dev/null; then
-						git diff-index --cached --quiet HEAD -- || i="+"
-					else
-						i="#"
-					fi
-				fi
-			fi
-			if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
-				git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
-			fi
+	local w=""
+	local i=""
+	local s=""
+	local u=""
+	local c=""
+	local p=""
 
-			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
-				if [ -n "$(git ls-files --others --exclude-standard)" ]; then
-					u="%"
+	if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then
+		if [ "true" = "$(git rev-parse --is-bare-repository 2>/dev/null)" ]; then
+			c="BARE:"
+		else
+			b="GIT_DIR!"
+		fi
+	elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
+		if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
+			if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
+				git diff --no-ext-diff --quiet --exit-code || w="*"
+				if git rev-parse --quiet --verify HEAD >/dev/null; then
+					git diff-index --cached --quiet HEAD -- || i="+"
+				else
+					i="#"
 				fi
 			fi
+		fi
+		if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
+			git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
+		fi
 
-			if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
-				__git_ps1_show_upstream
+		if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
+			if [ -n "$(git ls-files --others --exclude-standard)" ]; then
+				u="%"
 			fi
 		fi
 
-		local f="$w$i$s$u"
-		printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
+		if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
+			__git_ps1_show_upstream
+		fi
 	fi
+
+	local f="$w$i$s$u"
+	printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
 }
 
 __gitcomp_1 ()
-- 
1.7.10.1.541.gb1be298
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 07/19] completion: make __gitdir() store repository path in $__git_dir

The __gitdir() helper function is responsible for finding out the path to the repository, and it's invoked from several completion (helper) functions and from __git_ps1(). The path of the repository is printed to stdout, therefore it's invoked in a command substitution. This has the following drawbacks:

  1. The command substitution involves the forking of a subshell,
     which has a considerable overhead.
  2. There are a few cases, where __gitdir() is called more than once
     during a single completion, which means multiple command
     substitutions and eventual multiple 'git rev-parse --git-dir'
     executions.  For example, __gitdir() is invoked twice during the
     completion of options for 'gitk', 'git log', 'git rebase', but
     for certain aliases it might be invoked three times.

Now, _git(), the top-level git completion function declares the local $__git_dir variable for storing the repository path given as parameter to the '--git-dir=' option. Since $__git_dir is declared at the top-level, it is already available in all completion functions, but it's only used in __gitdir() (if set then there's no need to autodetect the path to the repository). This means that we can use $__git_dir to always store the path to the repository found by __gitdir().

This way we can address both of the above drawbacks. We won't need the command substitution when invoking __gitdir(), because won't need __gitdir()'s output anymore: the path to the repository will be available in $__git_dir. Furthermore, onlyt the first __gitdir() invocation will perform the search for the repository, and all subsequent calls will see the path already stored in $__git_dir by the first call.

So change __gitdir() to store the path to the repository in $__git_dir. However, still print the found path, because there might be user-supplied completion functions relying on it.

Also declare $__git_dir as local in __git_ps1() and _gitk() to prevent the variable from leaking into the environment when they call __gitdir() (that would break completion and bash prompt when the user moves to a different git repository).

Finally, extend the __gitdir() tests to check the value of $__git_dir, too.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 20 ++++----
 t/t9903-bash-prompt.sh                 | 85 +++++++++++++++++++++++-----------
 2 files changed, 71 insertions(+), 34 deletions(-)
Show changes to 2 files +71 −34

contrib/completion/git-completion.bash, t/t9903-bash-prompt.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index eaa3df9d..85b933f2 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -71,25 +71,27 @@ case "$COMP_WORDBREAKS" in
 esac
 
 # __gitdir accepts 0 or 1 arguments (i.e., location)
-# returns location of .git repo
+# Prints the path to the .git directory, and stores it in $__git_dir as well.
 __gitdir ()
 {
 	if [ -z "${1-}" ]; then
 		if [ -n "${__git_dir-}" ]; then
-			echo "$__git_dir"
+			:
 		elif [ -n "${GIT_DIR-}" ]; then
 			test -d "${GIT_DIR-}" || return 1
-			echo "$GIT_DIR"
+			__git_dir="$GIT_DIR"
 		elif [ -d .git ]; then
-			echo .git
+			__git_dir=.git
 		else
-			git rev-parse --git-dir 2>/dev/null
+			__git_dir="$(git rev-parse --git-dir 2>/dev/null)" || return 1
 		fi
 	elif [ -d "$1/.git" ]; then
-		echo "$1/.git"
+		__git_dir="$1/.git"
 	else
-		echo "$1"
+		__git_dir="$1"
 	fi
+
+	echo "$__git_dir"
 }
 
 # stores the divergence from upstream in $p
@@ -216,6 +218,7 @@ __git_ps1_show_upstream ()
 # returns text to add to bash PS1 prompt (includes branch name)
 __git_ps1 ()
 {
+	local __git_dir=""
 	local g="$(__gitdir)"
 	if [ -z "$g" ]; then
 		return
@@ -2606,7 +2609,7 @@ _git_whatchanged ()
 
 _git ()
 {
-	local i c=1 command __git_dir
+	local i c=1 command __git_dir=""
 
 	if [[ -n ${ZSH_VERSION-} ]]; then
 		emulate -L bash
@@ -2690,6 +2693,7 @@ _gitk ()
 
 	__git_has_doubledash && return
 
+	local __git_dir=""
 	local g="$(__gitdir)"
 	local merge=""
 	if [ -f "$g/MERGE_HEAD" ]; then
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 96468ceb..496e04ad 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -10,6 +10,7 @@ test_description='test git-specific bash prompt functions'
 . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
 
 actual="$TRASH_DIRECTORY/actual"
+actual_var="$TRASH_DIRECTORY/actual_var"
 
 test_expect_success 'setup for prompt tests' '
 	mkdir -p subdir/subsubdir &&
@@ -35,54 +36,74 @@ test_expect_success 'gitdir - from command line (through $__git_dir)' '
 	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
 	(
 		__git_dir="$TRASH_DIRECTORY/otherrepo/.git" &&
-		__gitdir > "$actual"
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
 	) &&
-	test_cmp expected "$actual"
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success 'gitdir - repo as argument' '
 	echo "otherrepo/.git" > expected &&
-	__gitdir "otherrepo" > "$actual" &&
-	test_cmp expected "$actual"
+	(
+		__gitdir "otherrepo" > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
+	) &&
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success 'gitdir - remote as argument' '
 	echo "remote" > expected &&
-	__gitdir "remote" > "$actual" &&
-	test_cmp expected "$actual"
+	(
+		__gitdir "remote" > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
+	) &&
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success 'gitdir - .git directory in cwd' '
 	echo ".git" > expected &&
-	__gitdir > "$actual" &&
-	test_cmp expected "$actual"
+	(
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
+	) &&
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success 'gitdir - .git directory in parent' '
 	echo "$TRASH_DIRECTORY/.git" > expected &&
 	(
 		cd subdir/subsubdir &&
-		__gitdir > "$actual"
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
 	) &&
-	test_cmp expected "$actual"
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success 'gitdir - cwd is a .git directory' '
 	echo "." > expected &&
 	(
 		cd .git &&
-		__gitdir > "$actual"
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
 	) &&
-	test_cmp expected "$actual"
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success 'gitdir - parent is a .git directory' '
 	echo "$TRASH_DIRECTORY/.git" > expected &&
 	(
 		cd .git/refs/heads &&
-		__gitdir > "$actual"
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
 	) &&
-	test_cmp expected "$actual"
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success 'gitdir - $GIT_DIR set while .git directory in cwd' '
@@ -90,9 +111,11 @@ test_expect_success 'gitdir - $GIT_DIR set while .git directory in cwd' '
 	(
 		GIT_DIR="$TRASH_DIRECTORY/otherrepo/.git" &&
 		export GIT_DIR &&
-		__gitdir > "$actual"
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
 	) &&
-	test_cmp expected "$actual"
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success 'gitdir - $GIT_DIR set while .git directory in parent' '
@@ -101,16 +124,19 @@ test_expect_success 'gitdir - $GIT_DIR set while .git directory in parent' '
 		GIT_DIR="$TRASH_DIRECTORY/otherrepo/.git" &&
 		export GIT_DIR &&
 		cd subdir &&
-		__gitdir > "$actual"
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
 	) &&
-	test_cmp expected "$actual"
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success 'gitdir - non-existing $GIT_DIR' '
 	(
 		GIT_DIR="$TRASH_DIRECTORY/non-existing" &&
 		export GIT_DIR &&
-		test_must_fail __gitdir
+		test_must_fail __gitdir &&
+		test -z "$__git_dir"
 	)
 '
 
@@ -120,9 +146,11 @@ test_expect_success 'gitdir - gitfile in cwd' '
 	test_when_finished "rm -f subdir/.git" &&
 	(
 		cd subdir &&
-		__gitdir > "$actual"
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
 	) &&
-	test_cmp expected "$actual"
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success 'gitdir - gitfile in parent' '
@@ -131,9 +159,11 @@ test_expect_success 'gitdir - gitfile in parent' '
 	test_when_finished "rm -f subdir/.git" &&
 	(
 		cd subdir/subsubdir &&
-		__gitdir > "$actual"
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
 	) &&
-	test_cmp expected "$actual"
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success SYMLINKS 'gitdir - resulting path avoids symlinks' '
@@ -144,9 +174,11 @@ test_expect_success SYMLINKS 'gitdir - resulting path avoids symlinks' '
 	test_when_finished "rm -f link" &&
 	(
 		cd link &&
-		__gitdir > "$actual"
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
 	) &&
-	test_cmp expected "$actual"
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
 '
 
 test_expect_success 'gitdir - not a git repository' '
@@ -154,7 +186,8 @@ test_expect_success 'gitdir - not a git repository' '
 		cd subdir/subsubdir &&
 		GIT_CEILING_DIRECTORIES="$TRASH_DIRECTORY" &&
 		export GIT_CEILING_DIRECTORIES &&
-		test_must_fail __gitdir
+		test_must_fail __gitdir &&
+		test -z "$__git_dir"
 	)
 '
 
-- 
1.7.10.1.541.gb1be298
Junio C Hamano· May 9, 2012, 19:36 UTC · re: SZEDER Gábor · lore

Re: [PATCH 07/19] completion: make __gitdir() store repository path in $__git_dir

SZEDER Gábor <szeder@ira.uka.de> writes:
> Also declare $__git_dir as local in __git_ps1() and _gitk() to prevent
> the variable from leaking into the environment when they call
> __gitdir() (that would break completion and bash prompt when the user
> moves to a different git repository).

Good; this was actually the only major thing I worried about when I saw the tail part of the series. So $__git_dir is global across the call chain of a single invocation to show __git_ps1, but it always is reset once __git_ps1 is called again, so that it will always know where the then-current git repository is, right?

Looks quite a sane and valid optimization to me.
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 08/19] completion: use $__git_dir instead of $(__gitdir)

The previous commit changed __gitdir() to store the repository path in the $__git_dir variable. Now we change all call sites to just call __gitdir() directly and then use $__git_dir instead of doing 'dir="$(__gitdir)"' command substitution, thereby sparing the overhead of fork()ing a subshell.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 106 ++++++++++++++++++---------------
 1 file changed, 58 insertions(+), 48 deletions(-)
Show changes to contrib/completion/git-completion.bash +58 −48
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 85b933f2..5c8d4aea 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -219,33 +219,33 @@ __git_ps1_show_upstream ()
 __git_ps1 ()
 {
 	local __git_dir=""
-	local g="$(__gitdir)"
-	if [ -z "$g" ]; then
+	__gitdir >/dev/null
+	if [ -z "$__git_dir" ]; then
 		return
 	fi
 
 	local r=""
 	local b=""
-	if [ -f "$g/rebase-merge/interactive" ]; then
+	if [ -f "$__git_dir/rebase-merge/interactive" ]; then
 		r="|REBASE-i"
-		b="$(cat "$g/rebase-merge/head-name")"
-	elif [ -d "$g/rebase-merge" ]; then
+		b="$(cat "$__git_dir/rebase-merge/head-name")"
+	elif [ -d "$__git_dir/rebase-merge" ]; then
 		r="|REBASE-m"
-		b="$(cat "$g/rebase-merge/head-name")"
+		b="$(cat "$__git_dir/rebase-merge/head-name")"
 	else
-		if [ -d "$g/rebase-apply" ]; then
-			if [ -f "$g/rebase-apply/rebasing" ]; then
+		if [ -d "$__git_dir/rebase-apply" ]; then
+			if [ -f "$__git_dir/rebase-apply/rebasing" ]; then
 				r="|REBASE"
-			elif [ -f "$g/rebase-apply/applying" ]; then
+			elif [ -f "$__git_dir/rebase-apply/applying" ]; then
 				r="|AM"
 			else
 				r="|AM/REBASE"
 			fi
-		elif [ -f "$g/MERGE_HEAD" ]; then
+		elif [ -f "$__git_dir/MERGE_HEAD" ]; then
 			r="|MERGING"
-		elif [ -f "$g/CHERRY_PICK_HEAD" ]; then
+		elif [ -f "$__git_dir/CHERRY_PICK_HEAD" ]; then
 			r="|CHERRY-PICKING"
-		elif [ -f "$g/BISECT_LOG" ]; then
+		elif [ -f "$__git_dir/BISECT_LOG" ]; then
 			r="|BISECTING"
 		fi
 
@@ -263,7 +263,7 @@ __git_ps1 ()
 				git describe --tags --exact-match HEAD ;;
 			esac 2>/dev/null)" ||
 
-			b="$(cut -c1-7 "$g/HEAD" 2>/dev/null)..." ||
+			b="$(cut -c1-7 "$__git_dir/HEAD" 2>/dev/null)..." ||
 			return
 			b="($b)"
 		}
@@ -522,9 +522,9 @@ __gitcomp_nl ()
 
 __git_heads ()
 {
-	local dir="$(__gitdir)"
-	if [ -d "$dir" ]; then
-		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
+	__gitdir "${1-}" >/dev/null
+	if [ -d "$__git_dir" ]; then
+		git --git-dir="$__git_dir" for-each-ref --format='%(refname:short)' \
 			refs/heads
 		return
 	fi
@@ -532,9 +532,9 @@ __git_heads ()
 
 __git_tags ()
 {
-	local dir="$(__gitdir)"
-	if [ -d "$dir" ]; then
-		git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
+	__gitdir "${1-}" >/dev/null
+	if [ -d "$__git_dir" ]; then
+		git --git-dir="$__git_dir" for-each-ref --format='%(refname:short)' \
 			refs/tags
 		return
 	fi
@@ -545,9 +545,10 @@ __git_tags ()
 # by checkout for tracking branches
 __git_refs ()
 {
-	local i hash dir="$(__gitdir "${1-}")" track="${2-}"
+	local i hash track="${2-}"
 	local format refs
-	if [ -d "$dir" ]; then
+	__gitdir "${1-}" >/dev/null
+	if [ -d "$__git_dir" ]; then
 		case "$cur" in
 		refs|refs/*)
 			format="refname"
@@ -556,20 +557,20 @@ __git_refs ()
 			;;
 		*)
 			for i in HEAD FETCH_HEAD ORIG_HEAD MERGE_HEAD; do
-				if [ -e "$dir/$i" ]; then echo $i; fi
+				if [ -e "$__git_dir/$i" ]; then echo $i; fi
 			done
 			format="refname:short"
 			refs="refs/tags refs/heads refs/remotes"
 			;;
 		esac
-		git --git-dir="$dir" for-each-ref --format="%($format)" \
+		git --git-dir="$__git_dir" for-each-ref --format="%($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 --git-dir="$__git_dir" for-each-ref --shell --format="ref=%(refname:short)" \
 				"refs/remotes/" | \
 			while read -r entry; do
 				eval "$entry"
@@ -583,7 +584,7 @@ __git_refs ()
 	fi
 	case "$cur" in
 	refs|refs/*)
-		git ls-remote "$dir" "$cur*" 2>/dev/null | \
+		git ls-remote "$__git_dir" "$cur*" 2>/dev/null | \
 		while read -r hash i; do
 			case "$i" in
 			*^{}) ;;
@@ -592,7 +593,7 @@ __git_refs ()
 		done
 		;;
 	*)
-		git ls-remote "$dir" HEAD ORIG_HEAD 'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' 2>/dev/null | \
+		git ls-remote "$__git_dir" HEAD ORIG_HEAD 'refs/tags/*' 'refs/heads/*' 'refs/remotes/*' 2>/dev/null | \
 		while read -r hash i; do
 			case "$i" in
 			*^{}) ;;
@@ -625,9 +626,10 @@ __git_refs_remotes ()
 
 __git_remotes ()
 {
-	local i IFS=$'\n' d="$(__gitdir)"
-	test -d "$d/remotes" && ls -1 "$d/remotes"
-	for i in $(git --git-dir="$d" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
+	local i IFS=$'\n'
+	__gitdir >/dev/null
+	test -d "$__git_dir/remotes" && ls -1 "$__git_dir/remotes"
+	for i in $(git --git-dir="$__git_dir" config --get-regexp 'remote\..*\.url' 2>/dev/null); do
 		i="${i#remote.}"
 		echo "${i/.url*/}"
 	done
@@ -685,8 +687,9 @@ __git_complete_revlist_file ()
 		esac
 
 		local IFS=$'\n'
+		__gitdir >/dev/null
 		COMPREPLY=($(compgen -P "$pfx" \
-			-W "$(git --git-dir="$(__gitdir)" ls-tree "$ls" \
+			-W "$(git --git-dir="$__git_dir" ls-tree "$ls" \
 				| sed '/^100... blob /{
 				           s,^.*	,,
 				           s,$, ,
@@ -936,7 +939,8 @@ __git_compute_porcelain_commands ()
 __git_pretty_aliases ()
 {
 	local i IFS=$'\n'
-	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "pretty\..*" 2>/dev/null); do
+	__gitdir >/dev/null
+	for i in $(git --git-dir="$__git_dir" config --get-regexp "pretty\..*" 2>/dev/null); do
 		case "$i" in
 		pretty.*)
 			i="${i#pretty.}"
@@ -949,7 +953,8 @@ __git_pretty_aliases ()
 __git_aliases ()
 {
 	local i IFS=$'\n'
-	for i in $(git --git-dir="$(__gitdir)" config --get-regexp "alias\..*" 2>/dev/null); do
+	__gitdir >/dev/null
+	for i in $(git --git-dir="$__git_dir" config --get-regexp "alias\..*" 2>/dev/null); do
 		case "$i" in
 		alias.*)
 			i="${i#alias.}"
@@ -962,7 +967,8 @@ __git_aliases ()
 # __git_aliased_command requires 1 argument
 __git_aliased_command ()
 {
-	local word cmdline=$(git --git-dir="$(__gitdir)" \
+	__gitdir >/dev/null
+	local word cmdline=$(git --git-dir="$__git_dir" \
 		config --get "alias.$1")
 	for word in $cmdline; do
 		case "$word" in
@@ -1013,8 +1019,8 @@ __git_whitespacelist="nowarn warn error error-all fix"
 
 _git_am ()
 {
-	local dir="$(__gitdir)"
-	if [ -d "$dir"/rebase-apply ]; then
+	__gitdir >/dev/null
+	if [ -d "$__git_dir"/rebase-apply ]; then
 		__gitcomp "--skip --continue --resolved --abort"
 		return
 	fi
@@ -1099,7 +1105,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
+		__gitdir >/dev/null
+		if [ -f "$__git_dir"/BISECT_START ]; then
 			__gitcomp "$subcommands"
 		else
 			__gitcomp "replay start"
@@ -1559,9 +1566,9 @@ _git_log ()
 {
 	__git_has_doubledash && return
 
-	local g="$(__gitdir)"
+	__gitdir >/dev/null
 	local merge=""
-	if [ -f "$g/MERGE_HEAD" ]; then
+	if [ -f "$__git_dir/MERGE_HEAD" ]; then
 		merge="--merge"
 	fi
 	case "$cur" in
@@ -1745,8 +1752,8 @@ _git_push ()
 
 _git_rebase ()
 {
-	local dir="$(__gitdir)"
-	if [ -d "$dir"/rebase-apply ] || [ -d "$dir"/rebase-merge ]; then
+	__gitdir >/dev/null
+	if [ -d "$__git_dir"/rebase-apply ] || [ -d "$__git_dir"/rebase-merge ]; then
 		__gitcomp "--continue --skip --abort"
 		return
 	fi
@@ -1845,7 +1852,8 @@ __git_config_get_set_variables ()
 		c=$((--c))
 	done
 
-	git --git-dir="$(__gitdir)" config $config_file --list 2>/dev/null |
+	__gitdir >/dev/null
+	git --git-dir="$__git_dir" config $config_file --list 2>/dev/null |
 	while read -r line
 	do
 		case "$line" in
@@ -1880,7 +1888,8 @@ _git_config ()
 	remote.*.push)
 		local remote="${prev#remote.}"
 		remote="${remote%.push}"
-		__gitcomp_nl "$(git --git-dir="$(__gitdir)" \
+		__gitdir >/dev/null
+		__gitcomp_nl "$(git --git-dir="$__git_dir" \
 			for-each-ref --format='%(refname):%(refname)' \
 			refs/heads)"
 		return
@@ -2304,7 +2313,8 @@ _git_remote ()
 		;;
 	update)
 		local i c='' IFS=$'\n'
-		for i in $(git --git-dir="$(__gitdir)" config --get-regexp "remotes\..*" 2>/dev/null); do
+		__gitdir >/dev/null
+		for i in $(git --git-dir="$__git_dir" config --get-regexp "remotes\..*" 2>/dev/null); do
 			i="${i#remotes.}"
 			c="$c ${i/ */}"
 		done
@@ -2441,7 +2451,8 @@ _git_stash ()
 			COMPREPLY=()
 			;;
 		show,*|apply,*|drop,*|pop,*|branch,*)
-			__gitcomp_nl "$(git --git-dir="$(__gitdir)" stash list \
+			__gitdir >/dev/null
+			__gitcomp_nl "$(git --git-dir="$__git_dir" stash list \
 					| sed -n -e 's/:.*//p')"
 			;;
 		*)
@@ -2693,10 +2704,9 @@ _gitk ()
 
 	__git_has_doubledash && return
 
-	local __git_dir=""
-	local g="$(__gitdir)"
-	local merge=""
-	if [ -f "$g/MERGE_HEAD" ]; then
+	local __git_dir="" merge=""
+	__gitdir >/dev/null
+	if [ -f "$__git_dir/MERGE_HEAD" ]; then
 		merge="--merge"
 	fi
 	case "$cur" in
-- 
1.7.10.1.541.gb1be298
Junio C Hamano· May 9, 2012, 19:43 UTC · re: SZEDER Gábor · lore

Re: [PATCH 08/19] completion: use $__git_dir instead of $(__gitdir)

SZEDER Gábor <szeder@ira.uka.de> writes:
> +	__gitdir >/dev/null

If this becomes the only call site of __gitdir helper function (and that was the way I read the log message), it would be sane to rename it to a more descriptive __setup_dash_dash_git_dir function and lose the need to redirect its output, no?

Show 10 quoted lines
> @@ -962,7 +967,8 @@ __git_aliases ()
>  # __git_aliased_command requires 1 argument
>  __git_aliased_command ()
>  {
> -	local word cmdline=$(git --git-dir="$(__gitdir)" \
> +	__gitdir >/dev/null
> +	local word cmdline=$(git --git-dir="$__git_dir" \
>  		config --get "alias.$1")
>  	for word in $cmdline; do
>  		case "$word" in

Now this worries me. The way I read 07/19 was that the local __git_dir="" declarations in __git_ps1 and __git were what protected this whole machinery to protect us against surprises from user doing "cd" between interactive commands, but you have the same __gitdir call to set up the global $__git_dir variable there, without the initialization to "".

Having to have a call to __gitdir seems to indicate to me that you cannot assume that the other initialization sites may not have been called before we get to this point. Then why is 'local __git_dir=""' unneeded here?

SZEDER Gábor· May 9, 2012, 20:22 UTC · re: Junio C Hamano · lore

Re: [PATCH 08/19] completion: use $__git_dir instead of $(__gitdir)

On Wed, May 09, 2012 at 12:43:33PM -0700, Junio C Hamano wrote:
Show 8 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > +	__gitdir >/dev/null
> 
> If this becomes the only call site of __gitdir helper function (and that
> was the way I read the log message), it would be sane to rename it to
> a more descriptive __setup_dash_dash_git_dir function and lose the need to
> redirect its output, no?

There might be user-defined completion functions using __gitdir(), and I didn't want to break them by changing the function's name or altering its existing behavior, i.e. printing the path.

Show 20 quoted lines
> > @@ -962,7 +967,8 @@ __git_aliases ()
> >  # __git_aliased_command requires 1 argument
> >  __git_aliased_command ()
> >  {
> > -	local word cmdline=$(git --git-dir="$(__gitdir)" \
> > +	__gitdir >/dev/null
> > +	local word cmdline=$(git --git-dir="$__git_dir" \
> >  		config --get "alias.$1")
> >  	for word in $cmdline; do
> >  		case "$word" in
> 
> Now this worries me.  The way I read 07/19 was that the local __git_dir=""
> declarations in __git_ps1 and __git were what protected this whole
> machinery to protect us against surprises from user doing "cd" between
> interactive commands, but you have the same __gitdir call to set up the
> global $__git_dir variable there, without the initialization to "".
> 
> Having to have a call to __gitdir seems to indicate to me that you cannot
> assume that the other initialization sites may not have been called before
> we get to this point.  Then why is 'local __git_dir=""' unneeded here?
Your comments to the previous patch apply here.

All completion functions are called either from _git() or from _gitk(), where $__git_dir is declared as local. So no matter how deep is $__git_dir set in the callchain, it can't leak into the environment. It won't even survive between two subsequent completions on the same command line.

Now, it would definitely be simpler to just initialize $__git_dir in the two toplevel functions. But there are many codepatch that don't need $__git_dir at all, and would only be slowed down by an additional $(git rev-parse --git-dir).

Gábor
Junio C Hamano· May 9, 2012, 20:56 UTC · re: SZEDER Gábor · lore

Re: [PATCH 08/19] completion: use $__git_dir instead of $(__gitdir)

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 22 quoted lines
>> > @@ -962,7 +967,8 @@ __git_aliases ()
>> >  # __git_aliased_command requires 1 argument
>> >  __git_aliased_command ()
>> >  {
>> > -	local word cmdline=$(git --git-dir="$(__gitdir)" \
>> > +	__gitdir >/dev/null
>> > +	local word cmdline=$(git --git-dir="$__git_dir" \
>> >  		config --get "alias.$1")
>> >  	for word in $cmdline; do
>> >  		case "$word" in
>> 
>> Now this worries me.  The way I read 07/19 was that the local __git_dir=""
>> declarations in __git_ps1 and __git were what protected this whole
>> machinery to protect us against surprises from user doing "cd" between
>> interactive commands, but you have the same __gitdir call to set up the
>> global $__git_dir variable there, without the initialization to "".
>> 
>> Having to have a call to __gitdir seems to indicate to me that you cannot
>> assume that the other initialization sites may not have been called before
>> we get to this point.  Then why is 'local __git_dir=""' unneeded here?
>
> Your comments to the previous patch apply here.

Not really. __git_ps1 and __gitk seems to do __gitdir very early to make sure anybody that use $__git_dir can rely on it, but having to sprinkle "set up $__git_dir variable" everywhere means anybody who wants to update need to know if it is already called, which defeats the point of "we can use $__git_dir instead of calling $(__gitdir)" from maintainability's point of view.

SZEDER Gábor· May 9, 2012, 21:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 08/19] completion: use $__git_dir instead of $(__gitdir)

On Wed, May 09, 2012 at 01:56:02PM -0700, Junio C Hamano wrote:
Show 27 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> >> > @@ -962,7 +967,8 @@ __git_aliases ()
> >> >  # __git_aliased_command requires 1 argument
> >> >  __git_aliased_command ()
> >> >  {
> >> > -	local word cmdline=$(git --git-dir="$(__gitdir)" \
> >> > +	__gitdir >/dev/null
> >> > +	local word cmdline=$(git --git-dir="$__git_dir" \
> >> >  		config --get "alias.$1")
> >> >  	for word in $cmdline; do
> >> >  		case "$word" in
> >> 
> >> Now this worries me.  The way I read 07/19 was that the local __git_dir=""
> >> declarations in __git_ps1 and __git were what protected this whole
> >> machinery to protect us against surprises from user doing "cd" between
> >> interactive commands, but you have the same __gitdir call to set up the
> >> global $__git_dir variable there, without the initialization to "".
> >> 
> >> Having to have a call to __gitdir seems to indicate to me that you cannot
> >> assume that the other initialization sites may not have been called before
> >> we get to this point.  Then why is 'local __git_dir=""' unneeded here?
> >
> > Your comments to the previous patch apply here.
> 
> Not really.  __git_ps1 and __gitk seems to do __gitdir very early to make
> sure anybody that use $__git_dir can rely on it

No, __git_ps1() and __gitk() do __gitdir() early, because they need the path to the repository very early.

Show 5 quoted lines
> but having to sprinkle
> "set up $__git_dir variable" everywhere means anybody who wants to update
> need to know if it is already called, which defeats the point of "we can
> use $__git_dir instead of calling $(__gitdir)" from maintainability's
> point of view.
Well, the point is better explained in the body of the commit message:
  just call __gitdir() directly and then use $__git_dir instead of
  doing 'dir="$(__gitdir)"' command substitution

Unfortunately, I just couldn't manage to squeeze all this into a one-line short description. So it really is about performance, and not maintainability.

SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[RFC PATCH 09/19] completion: platform-specific helper function to get physical path

Two upcoming optimizations will replace invocations of git commands in command substitutions with bash builtins examining the path of the current working directory. Since git works with physical paths, we can't use logical path provided in $PWD, but have to resort to the '$(pwd -P)' command substitution to get the physical path of the current directory.

However, on platforms not supporting symbolic links, such as MinGW, the path in $PWD is bound to be the physical path. So on those platforms we could avoid the command substitution and use $PWD directly. Great for MinGW, because the overhead of forking a subshell is relatively large there.

So add a platform-specific helper function to get the physical path of the current directory: on MinGW it's defined such that it gets the physical path from $PWD, while on other platforms from '$(pwd -P)'. The path is stored in a variable whose name is passed as argument, so no command substitution is needed when invoking this function.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

I'm not sure how to check for MinGW; I just looked through the output from 'set', and saw OSTYPE=msys there.

 contrib/completion/git-completion.bash | 14 ++++++++++++++
 t/t9903-bash-prompt.sh                 | 21 +++++++++++++++++++++
 2 files changed, 35 insertions(+)
Show changes to 2 files +35 −0

contrib/completion/git-completion.bash, t/t9903-bash-prompt.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5c8d4aea..bd7d39e3 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -70,6 +70,20 @@ case "$COMP_WORDBREAKS" in
 *)   COMP_WORDBREAKS="$COMP_WORDBREAKS:"
 esac
 
+# __git_pwd_p() stores the physical path of the current working directory
+# in the variable whose name is given as argument
+if [ ${OSTYPE-} = "msys" ]; then
+__git_pwd_p ()
+{
+	eval $1="$PWD"
+}
+else
+__git_pwd_p ()
+{
+	eval $1=\"$(pwd -P)\"
+}
+fi
+
 # __gitdir accepts 0 or 1 arguments (i.e., location)
 # Prints the path to the .git directory, and stores it in $__git_dir as well.
 __gitdir ()
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 496e04ad..3d722b25 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -32,6 +32,27 @@ test_expect_success 'setup for prompt tests' '
 	git checkout master
 '
 
+test_expect_success 'getting pwd -P' '
+	echo "$TRASH_DIRECTORY" > expected &&
+	(
+		__git_pwd_p p &&
+		echo "$p" > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success SYMLINKS 'getting pwd -P - avoids symlinks' '
+	echo "$TRASH_DIRECTORY/otherrepo" > expected &&
+	ln -s otherrepo link &&
+	test_when_finished "rm -f link" &&
+	(
+		cd link &&
+		__git_pwd_p p &&
+		echo "$p" > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
 test_expect_success 'gitdir - from command line (through $__git_dir)' '
 	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
 	(
-- 
1.7.10.1.541.gb1be298
Johannes Sixt· May 9, 2012, 07:37 UTC · re: SZEDER Gábor · lore

Re: [RFC PATCH 09/19] completion: platform-specific helper function to get physical path

Am 5/9/2012 2:44, schrieb SZEDER Gábor:
> I'm not sure how to check for MinGW; I just looked through the output from
> 'set', and saw OSTYPE=msys there.

Ususally, we check uname -s for *MINGW* for "portability", but since a subshell is counter-productive and we are sure that we have a bash here, I think your check is OK.

Show 14 quoted lines
> +# __git_pwd_p() stores the physical path of the current working directory
> +# in the variable whose name is given as argument
> +if [ ${OSTYPE-} = "msys" ]; then
> +__git_pwd_p ()
> +{
> +	eval $1="$PWD"
> +}
> +else
> +__git_pwd_p ()
> +{
> +	eval $1=\"$(pwd -P)\"
> +}
> +fi
> +

The following fixup of the quoting is needed at any rate to make the eval'd commands resistent against directory names with blanks and double-quotes.

--- >8 ---
From: Johannes Sixt <j6t@kdbg.org>
Subject: [PATCH] fixup! completion: platform-specific helper function to get
 physical path
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
 contrib/completion/git-completion.bash |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to contrib/completion/git-completion.bash +2 −2
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index bd7d39e..2cab4a0 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -75,12 +75,12 @@ esac
 if [ ${OSTYPE-} = "msys" ]; then
 __git_pwd_p ()
 {
-	eval $1="$PWD"
+	eval "$1=\$PWD"
 }
 else
 __git_pwd_p ()
 {
-	eval $1=\"$(pwd -P)\"
+	eval "$1=\$(pwd -P)"
 }
 fi
 
-- 
1.7.10.1.1689.gacdfbde
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 10/19] completion: use bash builtins to search for repository

When not at the top of the work tree or when no path is specified by 'git --git-dir=...' or $GIT_DIR, the __gitdir() helper function finds out the physical path to the '.git' directory by running the '$(git rev-parse --git-dir)' command substitution. fork()ing a subshell and fork()+exec()ing a git command take some time; during the same amount of time we can stat a lot of files and directories using bash builtins, likely finding the '.git' directory faster.

This patch extends __gitdir() to search for a '.git' directory using only bash builtins. Like the existing check for '.git' in the current working directory or the previously added check for $GIT_DIR, this search is not that thorough either, as it doesn't check whether the found '.git' directory or the directory pointed to by a gitfile is a valid '.git' repository.

Since git doesn't search for a .git directory beyond filesystem boundaries and beyond paths specified in $GIT_CEILING_DIRECTORIES, the search in __gitdir() should not do that either. However, bash doesn't provide builtins to check that two paths are on the same filesystem, so we can't limit the search cheaply to one filesystem. Therefore, __gitdir() will only use bash builtins when neither of these limits are active, i.e. $GIT_DISCOVERY_ACROSS_FILESYSTEM is set and $GIT_CEILING_DIRECTORIES is empty, otherwise it will fall back on executing 'git rev-parse'. $GIT_CEILING_DIRECTORIES is empty by default, but users have to set $GIT_DISCOVERY_ACROSS_FILESYSTEM explicitly to enable this optimisation, when they find its consequences acceptable.

Of course, stat()ing in C is much faster than in bash, so there is a point when bash builtins will be slower than '$(git rev-parse --git-dir)' despite all the fork()s+exec() overhead. On MinGW this overhead is considerable, and the builtins version is faster even at a depth of 50 directories. On Linux the builtins version is only faster for paths less than 10 directories deeper than the toplevel; with my usage patterns that covers 99.9% of the prompts displayed.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 30 ++++++++++++
 t/t9903-bash-prompt.sh                 | 83 ++++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+)
Show changes to 2 files +113 −0

contrib/completion/git-completion.bash, t/t9903-bash-prompt.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index bd7d39e3..dd69e56e 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -89,13 +89,43 @@ fi
 __gitdir ()
 {
 	if [ -z "${1-}" ]; then
+		local gitfile
+
 		if [ -n "${__git_dir-}" ]; then
 			:
 		elif [ -n "${GIT_DIR-}" ]; then
 			test -d "${GIT_DIR-}" || return 1
 			__git_dir="$GIT_DIR"
+		elif [ -f .git ]; then
+			read gitfile <.git 2>/dev/null || return 1
+			__git_dir="${gitfile#gitdir: }"
 		elif [ -d .git ]; then
 			__git_dir=.git
+		elif [ -d refs ] && [ -r HEAD ] && \
+				[ -d "${GIT_OBJECT_DIRECTORY:-objects}" ]; then
+			__git_dir=.
+		elif [ -n "${GIT_DISCOVERY_ACROSS_FILESYSTEM-}" -a \
+				-z "${GIT_CEILING_DIRECTORIES-}" ]; then
+			local p
+			__git_pwd_p p
+			while true; do
+				p="${p%/*}"
+				if [ -f "$p/.git" ]; then
+					read gitfile <"$p/.git" 2>/dev/null || return 1
+					__git_dir="${gitfile#gitdir: }"
+					break
+				elif [ -d "$p/.git" ]; then
+					__git_dir="$p/.git"
+					break
+				elif [ -d "$p/refs" ] && [ -r "$p/HEAD" ] && \
+						[ -d "${GIT_OBJECT_DIRECTORY:-$p/objects}" ]; then
+					__git_dir="${p:-/}"
+					break
+				fi
+				if [ -z "$p" ]; then
+					return 1
+				fi
+			done
 		else
 			__git_dir="$(git rev-parse --git-dir 2>/dev/null)" || return 1
 		fi
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index 3d722b25..ffa22d39 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -105,6 +105,18 @@ test_expect_success 'gitdir - .git directory in parent' '
 	test_cmp expected "$actual_var"
 '
 
+test_expect_success 'gitdir - .git directory in parent - with builtins' '
+	echo "$TRASH_DIRECTORY/.git" > expected &&
+	(
+		GIT_DISCOVERY_ACROSS_FILESYSTEM=true &&
+		cd subdir/subsubdir &&
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
+	) &&
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
+'
+
 test_expect_success 'gitdir - cwd is a .git directory' '
 	echo "." > expected &&
 	(
@@ -116,6 +128,20 @@ test_expect_success 'gitdir - cwd is a .git directory' '
 	test_cmp expected "$actual_var"
 '
 
+test_expect_success 'gitdir - cwd is a .git directory - GIT_OBJECT_DIRECTORY' '
+	echo "." > expected &&
+	mv .git/objects _objects &&
+	test_when_finished "mv _objects .git/objects" &&
+	(
+		GIT_OBJECT_DIRECTORY="$TRASH_DIRECTORY/_objects" &&
+		cd .git &&
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
+	) &&
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
+'
+
 test_expect_success 'gitdir - parent is a .git directory' '
 	echo "$TRASH_DIRECTORY/.git" > expected &&
 	(
@@ -127,6 +153,33 @@ test_expect_success 'gitdir - parent is a .git directory' '
 	test_cmp expected "$actual_var"
 '
 
+test_expect_success 'gitdir - parent is a .git directory - with builtins' '
+	echo "$TRASH_DIRECTORY/.git" > expected &&
+	(
+		GIT_DISCOVERY_ACROSS_FILESYSTEM=true &&
+		cd .git/refs/heads &&
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
+	) &&
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
+'
+
+test_expect_success 'gitdir - parent is a .git directory - with builtins and GIT_OBJECT_DIRECTORY' '
+	echo "$TRASH_DIRECTORY/.git" > expected &&
+	mv .git/objects _objects &&
+	test_when_finished "mv _objects .git/objects" &&
+	(
+		GIT_OBJECT_DIRECTORY="$TRASH_DIRECTORY/_objects" &&
+		GIT_DISCOVERY_ACROSS_FILESYSTEM=true &&
+		cd .git/refs/heads &&
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
+	) &&
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
+'
+
 test_expect_success 'gitdir - $GIT_DIR set while .git directory in cwd' '
 	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
 	(
@@ -187,6 +240,20 @@ test_expect_success 'gitdir - gitfile in parent' '
 	test_cmp expected "$actual_var"
 '
 
+test_expect_success 'gitdir - gitfile in parent - with builtins' '
+	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
+	echo "gitdir: $TRASH_DIRECTORY/otherrepo/.git" > subdir/.git &&
+	test_when_finished "rm -f subdir/.git" &&
+	(
+		GIT_DISCOVERY_ACROSS_FILESYSTEM=true &&
+		cd subdir/subsubdir &&
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
+	) &&
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
+'
+
 test_expect_success SYMLINKS 'gitdir - resulting path avoids symlinks' '
 	echo "$TRASH_DIRECTORY/otherrepo/.git" > expected &&
 	mkdir otherrepo/dir &&
@@ -202,6 +269,22 @@ test_expect_success SYMLINKS 'gitdir - resulting path avoids symlinks' '
 	test_cmp expected "$actual_var"
 '
 
+test_expect_success SYMLINKS 'gitdir - resulting path avoids symlinks - with builtins' '
+	echo "$TRASH_DIRECTORY/otherrepo/.git" >expected &&
+	mkdir otherrepo/dir &&
+	test_when_finished "rm -rf otherrepo/dir" &&
+	ln -s otherrepo/dir link &&
+	test_when_finished "rm -f link" &&
+	(
+		GIT_DISCOVERY_ACROSS_FILESYSTEM=true &&
+		cd link &&
+		__gitdir > "$actual" &&
+		echo "$__git_dir" > "$actual_var"
+	) &&
+	test_cmp expected "$actual" &&
+	test_cmp expected "$actual_var"
+'
+
 test_expect_success 'gitdir - not a git repository' '
 	(
 		cd subdir/subsubdir &&
-- 
1.7.10.1.541.gb1be298
Junio C Hamano· May 9, 2012, 19:52 UTC · re: SZEDER Gábor · lore

Re: [PATCH 10/19] completion: use bash builtins to search for repository

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 8 quoted lines
> ..., this
> search is not that thorough either, as it doesn't check whether the
> found '.git' directory or the directory pointed to by a gitfile is a
> valid '.git' repository.
> ...
> Of course, stat()ing in C is much faster than in bash, so there is a
> point when bash builtins will be slower than '$(git rev-parse
> --git-dir)' despite all the fork()s+exec() overhead.

I'd feel safer if this new logic were an opt-in feature, at least in the beginning, with these pros-and-cons summarized near the beginning of the file to let the users choose if they want to use "exactly matches the command the prompt script is trying to help" version (i.e. rev-parse) vs "matches most of the time and faster under these conditions" version (i.e. the new logic).

Thanks.
SZEDER Gábor· May 9, 2012, 22:34 UTC · re: Junio C Hamano · lore

Re: [PATCH 10/19] completion: use bash builtins to search for repository

On Wed, May 09, 2012 at 12:52:49PM -0700, Junio C Hamano wrote:
Show 17 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > ..., this
> > search is not that thorough either, as it doesn't check whether the
> > found '.git' directory or the directory pointed to by a gitfile is a
> > valid '.git' repository.
> > ...
> > Of course, stat()ing in C is much faster than in bash, so there is a
> > point when bash builtins will be slower than '$(git rev-parse
> > --git-dir)' despite all the fork()s+exec() overhead.
> 
> I'd feel safer if this new logic were an opt-in feature, at least in the
> beginning, with these pros-and-cons summarized near the beginning of the
> file to let the users choose if they want to use "exactly matches the
> command the prompt script is trying to help" version (i.e. rev-parse) vs
> "matches most of the time and faster under these conditions" version
> (i.e. the new logic).

I'm not sure what you mean by opt-in. It's already opt-in in the sense that users have to set $GIT_DISCOVERY_ACROSS_FILESYSTEM to enable this logic. Or do you mean that $GIT_DISCOVERY_ACROSS_FILESYSTEM should not implicitly enable this logic, but it should be controlled by a new dedicated variable?

Junio C Hamano· May 9, 2012, 22:59 UTC · re: SZEDER Gábor · lore

Re: [PATCH 10/19] completion: use bash builtins to search for repository

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 5 quoted lines
> I'm not sure what you mean by opt-in.  It's already opt-in in the
> sense that users have to set $GIT_DISCOVERY_ACROSS_FILESYSTEM to
> enable this logic.  Or do you mean that
> $GIT_DISCOVERY_ACROSS_FILESYSTEM should not implicitly enable this
> logic, but it should be controlled by a new dedicated variable?

Exactly. In the three words in DISCOVERY_ACROSS_FILESYSTEM, I do not see anything that indicates that the user wishes to use a "may be faster in some situations but may be less correct" behaviour for one thing. Also, some day you or somebody else may find out how to take advantage of the new code in this patch series and still stop at the filesystem boundary and at that point, people may want to still forbid git to go up across filesystem boundary and want to use the new prompt code. Tying these two independent concepts (i.e. "do I want to use the new experimental prompt code?" and "do I want git to stop going up across fs boundaries?") together only because the initial implementation happens to rely on it is not such a good idea.

SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 11/19] bash prompt: use bash builtins to find out current branch

__git_ps1() runs the '$(git symbolic-ref HEAD)' command substitution to find out whether we are on a branch and to find out the name of that branch. This imposes the overhead of fork()ing a subshell and fork()+exec()ing a git process.

Since HEAD is a single-line file and the symbolic ref format is quite simple to recognize and parse, read and parse it using only bash builtins, thereby sparing all that fork()+exec() overhead.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)
Show changes to contrib/completion/git-completion.bash +6 −3
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index dd69e56e..ed372c41 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -293,8 +293,11 @@ __git_ps1 ()
 			r="|BISECTING"
 		fi
 
-		b="$(git symbolic-ref HEAD 2>/dev/null)" || {
-
+		local head=""
+		read head 2>/dev/null <"$__git_dir/HEAD" || return
+		# is it a symbolic ref?
+		b="${head#ref: }"
+		if [ "$head" = "$b" ]; then
 			b="$(
 			case "${GIT_PS1_DESCRIBE_STYLE-}" in
 			(contains)
@@ -310,7 +313,7 @@ __git_ps1 ()
 			b="$(cut -c1-7 "$__git_dir/HEAD" 2>/dev/null)..." ||
 			return
 			b="($b)"
-		}
+		fi
 	fi
 
 	local w=""
-- 
1.7.10.1.541.gb1be298
Junio C Hamano· May 9, 2012, 20:02 UTC · re: SZEDER Gábor · lore

Re: [PATCH 11/19] bash prompt: use bash builtins to find out current branch

SZEDER Gábor <szeder@ira.uka.de> writes:
> Since HEAD is a single-line file and the symbolic ref format is quite
> simple to recognize and parse,...

Strictly speaking, that is true only if you somehow know that HEAD is not a symlinked symref. You may end up reading [0-9a-f]{40} out of HEAD without learning where the symbolic link pointed at.

I personally do not _know_ of anybody who is still using a symlinked symref, but the reasoning behind 9f0bb90 (core.prefersymlinkrefs: use symlinks for .git/HEAD, 2006-05-02) cannot go away by definition until every project that benefited from the configuration the commit introduced goes extinct, so I wouldn't be surprised if you get complaints from the users if we adopt this patch.

SZEDER Gábor· May 9, 2012, 21:11 UTC · re: Junio C Hamano · lore

Re: [PATCH 11/19] bash prompt: use bash builtins to find out current branch

On Wed, May 09, 2012 at 01:02:41PM -0700, Junio C Hamano wrote:
Show 15 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > Since HEAD is a single-line file and the symbolic ref format is quite
> > simple to recognize and parse,...
> 
> Strictly speaking, that is true only if you somehow know that HEAD is not
> a symlinked symref.  You may end up reading [0-9a-f]{40} out of HEAD
> without learning where the symbolic link pointed at.
> 
> I personally do not _know_ of anybody who is still using a symlinked
> symref, but the reasoning behind 9f0bb90 (core.prefersymlinkrefs: use
> symlinks for .git/HEAD, 2006-05-02) cannot go away by definition until
> every project that benefited from the configuration the commit introduced
> goes extinct, so I wouldn't be surprised if you get complaints from the
> users if we adopt this patch.
Symlinked symref, wow.  That was long before my time ;)
So, let's see whether I understand it correctly:
- If HEAD is a symlink, then it's a symlinked symref, and points to
  a real ref file somewhere under refs/.
- If HEAD is a regular file, then it's either a symref containing
  'ref: refs/...', or it's a detached HEAD containing 40 hexdigits.

If the above is right, then we could check with bash builtins whether HEAD is a symbolic link, which is cheap, and stick to '$(git symbolic-ref HEAD)' if it is, or use bash builtins if it isn't, right? This way we could get most of the performance benefits for modern HEADs, while still supporting symlinked symrefs.

Gábor
Junio C Hamano· May 9, 2012, 21:25 UTC · re: SZEDER Gábor · lore

Re: [PATCH 11/19] bash prompt: use bash builtins to find out current branch

SZEDER Gábor <szeder@ira.uka.de> writes:
> If the above is right, then we could check with bash builtins whether
> HEAD is a symbolic link, which is cheap, and stick to '$(git
> symbolic-ref HEAD)' if it is, or use bash builtins if it isn't, right?  

Sure. Alternatively, you could run "readlink" on it if that is available built-in, and manipulate the result in string builtins, but that is a b/c slow path anyway, so I wouldn't bother.

Thanks.
SZEDER Gábor· May 9, 2012, 21:45 UTC · re: Junio C Hamano · lore

Re: [PATCH 11/19] bash prompt: use bash builtins to find out current branch

On Wed, May 09, 2012 at 02:25:46PM -0700, Junio C Hamano wrote:
Show 9 quoted lines
> SZEDER Gábor <szeder@ira.uka.de> writes:
> 
> > If the above is right, then we could check with bash builtins whether
> > HEAD is a symbolic link, which is cheap, and stick to '$(git
> > symbolic-ref HEAD)' if it is, or use bash builtins if it isn't, right?  
> 
> Sure.  Alternatively, you could run "readlink" on it if that is available
> built-in, and manipulate the result in string builtins, but that is a b/c
> slow path anyway, so I wouldn't bother.
OK, will do that then.

'readlink' is not a bash builtin, so it would need the same number of fork()s and exec() as 'symbolic-ref'. Of course, the 'readlink' binary is much smaller than git and has less to do, so it might be a tiny bit faster, but for this rare corner case it really doesn't matter.

Junio C Hamano· May 9, 2012, 21:50 UTC · re: SZEDER Gábor · lore

Re: [PATCH 11/19] bash prompt: use bash builtins to find out current branch

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 5 quoted lines
> 'readlink' is not a bash builtin, so it would need the same number of
> fork()s and exec() as 'symbolic-ref'.  Of course, the 'readlink'
> binary is much smaller than git and has less to do, so it might be a
> tiny bit faster, but for this rare corner case it really doesn't
> matter.

Correct, and by not using it you do not have to worry about systems that does not install the binary.

SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 12/19] bash prompt: use bash builtins to check whether inside git dir

__git_ps1() runs the '$(git rev-parse --is-inside-git-dir)' command substitution to check whether we are inside a .git directory and the bash prompt needs to be adjusted accordingly (i.e. display 'BARE!' or 'GIT_DIR!'). This imposes the overhead of fork()ing a subshell and fork()+exec()ing a git process.

Perform this check by comparing the path to the repository and the current directory using only bash builtins, thereby sparing all that fork()+exec() overhead.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
Show changes to contrib/completion/git-completion.bash +5 −1
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index ed372c41..72f7d0ed 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -323,7 +323,11 @@ __git_ps1 ()
 	local c=""
 	local p=""
 
-	if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then
+	local pwd_p
+	__git_pwd_p pwd_p
+	# inside .git dir?
+	if [ "$__git_dir" = "." -o \
+			"${pwd_p#$__git_dir}" != "$pwd_p" ]; then
 		if [ "true" = "$(git rev-parse --is-bare-repository 2>/dev/null)" ]; then
 			c="BARE:"
 		else
-- 
1.7.10.1.541.gb1be298
Johannes Sixt· May 9, 2012, 08:07 UTC · re: SZEDER Gábor · lore

Re: [PATCH 12/19] bash prompt: use bash builtins to check whether inside git dir

Am 5/9/2012 2:44, schrieb SZEDER Gábor:
Show 9 quoted lines
> __git_ps1() runs the '$(git rev-parse --is-inside-git-dir)' command
> substitution to check whether we are inside a .git directory and the
> bash prompt needs to be adjusted accordingly (i.e. display 'BARE!' or
> 'GIT_DIR!').  This imposes the overhead of fork()ing a subshell and
> fork()+exec()ing a git process.
> 
> Perform this check by comparing the path to the repository and the
> current directory using only bash builtins, thereby sparing all that
> fork()+exec() overhead.
Show 6 quoted lines
> -	if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then
> +	local pwd_p
> +	__git_pwd_p pwd_p
> +	# inside .git dir?
> +	if [ "$__git_dir" = "." -o \
> +			"${pwd_p#$__git_dir}" != "$pwd_p" ]; then

At this point, $__git_dir is c:/dir style, whereas $pwd_p is /c/dir style, and the intended prefix check does not trigger.

As long as $__git_dir is only used to access files, it does not matter whether it is Windows style or POSIX style. But if $__git_dir is used in a comparison, then you must make 100% sure that the involved paths are of the same vintage.

What would be lost if this patch were dropped?
-- Hannes
Junio C Hamano· May 9, 2012, 20:06 UTC · re: Johannes Sixt · lore

Re: [PATCH 12/19] bash prompt: use bash builtins to check whether inside git dir

Johannes Sixt <j.sixt@viscovery.net> writes:
Show 26 quoted lines
>> __git_ps1() runs the '$(git rev-parse --is-inside-git-dir)' command
>> substitution to check whether we are inside a .git directory and the
>> bash prompt needs to be adjusted accordingly (i.e. display 'BARE!' or
>> 'GIT_DIR!').  This imposes the overhead of fork()ing a subshell and
>> fork()+exec()ing a git process.
>> 
>> Perform this check by comparing the path to the repository and the
>> current directory using only bash builtins, thereby sparing all that
>> fork()+exec() overhead.
>
>> -	if [ "true" = "$(git rev-parse --is-inside-git-dir 2>/dev/null)" ]; then
>> +	local pwd_p
>> +	__git_pwd_p pwd_p
>> +	# inside .git dir?
>> +	if [ "$__git_dir" = "." -o \
>> +			"${pwd_p#$__git_dir}" != "$pwd_p" ]; then
>
> At this point, $__git_dir is c:/dir style, whereas $pwd_p is /c/dir style,
> and the intended prefix check does not trigger.
>
> As long as $__git_dir is only used to access files, it does not matter
> whether it is Windows style or POSIX style. But if $__git_dir is used in a
> comparison, then you must make 100% sure that the involved paths are of
> the same vintage.
>
> What would be lost if this patch were dropped?

One loss of fork/exec is what would be lost, I would guess, and that seems to be the primary point of this entire series, so...

At the conceptual level, I think the optimization in this patch makes sense, but if the assumed primary beneficiary (i.e. Windows) cannot benefit from this particular optimization due to two different path representations, it wouldn't help being conceptually sound X-<.

SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 13/19] bash prompt: check whether inside the worktree only when necessary

Besides the current branch name or detached head info __git_ps1() can also display status indicators in the prompt for changes in the work tree, stashes, untracked files, and divergence from upstream. These are only displayed when enabled by specific environment variables and when we are in a work tree. The latter condition is checked by the '$(git rev-parse --is-inside-work-tree)' command substitution, impossing the overhead of fork()ing a subshell and fork()+exec()ing a git process.

However, the check for the work tree preceeds the check of the environment variables enabling status indicators, so it's evaluated even when all these indicators are disabled.

Check upfront whether any of these indicators are enabled, i.e. a corresponding environment variable is set, to spare the unnecessary fork()+exec() overhead when all of them are disabled.

(Ideally we could check whether we are in a work tree using only bash builtins, like we did in the previous commit for .git directory, but the path of the work tree can be specified by the 'core.worktree' config variable, and running 'git config' to get its value would be just as expensive.)

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 42 +++++++++++++++++++---------------
 1 file changed, 24 insertions(+), 18 deletions(-)
Show changes to contrib/completion/git-completion.bash +24 −18
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 72f7d0ed..64b96f13 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -333,29 +333,35 @@ __git_ps1 ()
 		else
 			b="GIT_DIR!"
 		fi
-	elif [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
-		if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
-			if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
-				git diff --no-ext-diff --quiet --exit-code || w="*"
-				if git rev-parse --quiet --verify HEAD >/dev/null; then
-					git diff-index --cached --quiet HEAD -- || i="+"
-				else
-					i="#"
+	elif [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" \
+			-o -n "${GIT_PS1_SHOWSTASHSTATE-}" \
+			-o -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" \
+			-n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
+		if [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
+			if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
+				if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
+					git diff --no-ext-diff --quiet --exit-code || w="*"
+					if git rev-parse --quiet --verify HEAD >/dev/null; then
+						git diff-index --cached --quiet HEAD -- || i="+"
+					else
+						i="#"
+					fi
 				fi
 			fi
-		fi
-		if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
-			git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
-		fi
 
-		if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
-			if [ -n "$(git ls-files --others --exclude-standard)" ]; then
-				u="%"
+			if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
+			        git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
 			fi
-		fi
 
-		if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
-			__git_ps1_show_upstream
+			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
+				if [ -n "$(git ls-files --others --exclude-standard)" ]; then
+					u="%"
+				fi
+			fi
+
+			if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
+				__git_ps1_show_upstream
+			fi
 		fi
 	fi
 
-- 
1.7.10.1.541.gb1be298
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 14/19] bash prompt: use bash builtins to find out current branch during rebase

During an ongoing interactive rebase __git_ps1() finds out the name of the rebased branch by executing the '$(cat .git/rebase-merge/head-name)' command substitution. That is not quite the most efficient way to read a single line single word file, because it imposes the overhead of fork()ing a subshell and fork()+exec()ing 'cat'.

Use the 'read' bash builtin instead to avoid that overhead.
Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to contrib/completion/git-completion.bash +2 −2
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 64b96f13..671032bf 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -272,10 +272,10 @@ __git_ps1 ()
 	local b=""
 	if [ -f "$__git_dir/rebase-merge/interactive" ]; then
 		r="|REBASE-i"
-		b="$(cat "$__git_dir/rebase-merge/head-name")"
+		read b <"$__git_dir/rebase-merge/head-name"
 	elif [ -d "$__git_dir/rebase-merge" ]; then
 		r="|REBASE-m"
-		b="$(cat "$__git_dir/rebase-merge/head-name")"
+		read b <"$__git_dir/rebase-merge/head-name"
 	else
 		if [ -d "$__git_dir/rebase-apply" ]; then
 			if [ -f "$__git_dir/rebase-apply/rebasing" ]; then
-- 
1.7.10.1.541.gb1be298
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 15/19] bash prompt: use bash builtins to get detached HEAD abbrev. object name

When describing a detached HEAD according to the $GIT_PS1_DESCRIBE environment variable fails, __git_ps1() runs the '$(cut -c1-7 .git/HEAD)' command substitution to put the 7 hexdigits abbreviated commit object name in the prompt. This imposes the overhead of fork()ing a subshell and fork(+exec()ing 'cut'.

Thanks to an earlier commit in this series the contents of HEAD is already read into a local variable, so we can get the 7 hexdigits using only parameter expansions, sparing the fork()+exec() overhead.

Since zsh doesn't implement substring expansion we can't just use ${head:0:7}, hence the "remove everything except the first 7 chars" parameter expansion combination.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to contrib/completion/git-completion.bash +2 −2
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 671032bf..2346962d 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -310,8 +310,8 @@ __git_ps1 ()
 				git describe --tags --exact-match HEAD ;;
 			esac 2>/dev/null)" ||
 
-			b="$(cut -c1-7 "$__git_dir/HEAD" 2>/dev/null)..." ||
-			return
+			# detached head abbreviated object name
+			b="${head%${head#???????}}..."
 			b="($b)"
 		fi
 	fi
-- 
1.7.10.1.541.gb1be298
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 16/19] bash prompt: display stash and upstream state even inside the repository

Besides the current branch name or detached head info __git_ps1() can also display some status indicators in the prompt. The first such indicator was for changes in the work tree in 738a94a9 (bash: offer to show (un)staged changes, 2009-02-03), and was only checked/displayed when inside the work tree. Later other indicators were added in 2414b45c (Show presence of stashed changes in bash prompt., 2009-06-02), 397f7c63 (Show the presence of untracked files in the bash prompt., 2009-07-22), and 6d158cba (bash completion: Support "divergence from upstream" messages in __git_ps1, 2010-06-17). All of these just followed suit and were checked only when inside the work tree, i.e. after checking the results of the '$(git rev-parse --is-inside-work-tree)' command substitution, imposing the overhead of fork()ing a subshell and fork()+exec()ing a git process.

However, the presence of stashes and the divergence from upstream is not a property of the work tree but a property of the repository, and the implementation of their indicators doesn't actually require a work tree. Therefore, we can display these two indicators even inside the repository. Not that it's very useful to see the stash status while poking around deep inside the .git directory, but this way users enabling only the stash indicator won't pay the additional performance penalty of the check for the work tree.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 20 +++++++++-----------
 t/t9903-bash-prompt.sh                 |  4 ++--
 2 files changed, 11 insertions(+), 13 deletions(-)
Show changes to 2 files +11 −13

contrib/completion/git-completion.bash, t/t9903-bash-prompt.sh

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 2346962d..64207e3c 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -334,9 +334,7 @@ __git_ps1 ()
 			b="GIT_DIR!"
 		fi
 	elif [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" \
-			-o -n "${GIT_PS1_SHOWSTASHSTATE-}" \
-			-o -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" \
-			-n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
+			-o -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
 		if [ "true" = "$(git rev-parse --is-inside-work-tree 2>/dev/null)" ]; then
 			if [ -n "${GIT_PS1_SHOWDIRTYSTATE-}" ]; then
 				if [ "$(git config --bool bash.showDirtyState)" != "false" ]; then
@@ -349,22 +347,22 @@ __git_ps1 ()
 				fi
 			fi
 
-			if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
-			        git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
-			fi
-
 			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
 				if [ -n "$(git ls-files --others --exclude-standard)" ]; then
 					u="%"
 				fi
 			fi
-
-			if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
-				__git_ps1_show_upstream
-			fi
 		fi
 	fi
 
+	if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
+	        git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
+	fi
+
+	if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
+		__git_ps1_show_upstream
+	fi
+
 	local f="$w$i$s$u"
 	printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
 }
diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index ffa22d39..a43d402a 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -542,8 +542,8 @@ test_expect_success 'prompt - stash status indicator - stash' '
 	test_cmp expected "$actual"
 '
 
-test_expect_success 'prompt - stash status indicator - not shown inside .git directory' '
-	printf " (GIT_DIR!)" > expected &&
+test_expect_success 'prompt - stash status indicator - stash while inside .git directory' '
+	printf " (GIT_DIR! $)" > expected &&
 	echo 2 >file &&
 	git stash &&
 	test_when_finished "git stash drop" &&
-- 
1.7.10.1.541.gb1be298
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 17/19] bash prompt: use bash builtins to check stash state

When the environment variable $GIT_PS1_SHOWSTASHSTATE is set __git_ps1() checks the presence of stashes by running 'git rev-parse --verify refs/stash'. This command not only checks that the 'refs/stash' ref exists but also, well, verifies that it's a valid ref.

However, we don't need to be that thorough for the bash prompt. We can omit that verification and only check whether 'refs/stash' exists or not. Since 'git pack-refs' never packs 'refs/stash', it's a matter of checking the existence of a ref file. Perform this check using only bash builtins to spare the overhead of fork()+exec()ing a git process.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
 contrib/completion/git-completion.bash | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
Show changes to contrib/completion/git-completion.bash +3 −1
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 64207e3c..c4feab68 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -356,7 +356,9 @@ __git_ps1 ()
 	fi
 
 	if [ -n "${GIT_PS1_SHOWSTASHSTATE-}" ]; then
-	        git rev-parse --verify refs/stash >/dev/null 2>&1 && s="$"
+		if [ -r "$__git_dir/refs/stash" ]; then
+			s="$"
+		fi
 	fi
 
 	if [ -n "${GIT_PS1_SHOWUPSTREAM-}" ]; then
-- 
1.7.10.1.541.gb1be298
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[RFC PATCH 18/19] bash prompt: avoid command substitution when checking for untracked files

When enabled, the bash prompt can indicate the presence of untracked files with a '%' sign. __git_ps1() checks for untracked files by running the '$(git ls-files --others --exclude-standard)' command substitution, and displays the indicator when there is no output.

Avoid this command substitution by additionally passing '--error-unmatch *', and checking the command's return value.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

This seems to do the right thing, but I'm not quite sure, so I would appreciate a pair of expert eyeballs on it.

 contrib/completion/git-completion.bash | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)
Show changes to contrib/completion/git-completion.bash +2 −3
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index c4feab68..5ea19018 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -348,9 +348,8 @@ __git_ps1 ()
 			fi
 
 			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
-				if [ -n "$(git ls-files --others --exclude-standard)" ]; then
-					u="%"
-				fi
+				git ls-files --others --exclude-standard --error-unmatch -- '*' >/dev/null 2>/dev/null &&
+				u="%"
 			fi
 		fi
 	fi
-- 
1.7.10.1.541.gb1be298
Junio C Hamano· May 9, 2012, 20:32 UTC · re: SZEDER Gábor · lore

Re: [RFC PATCH 18/19] bash prompt: avoid command substitution when checking for untracked files

SZEDER Gábor <szeder@ira.uka.de> writes:
Show 7 quoted lines
> When enabled, the bash prompt can indicate the presence of untracked
> files with a '%' sign.  __git_ps1() checks for untracked files by running the
> '$(git ls-files --others --exclude-standard)' command substitution,
> and displays the indicator when there is no output.
>
> Avoid this command substitution by additionally passing
> '--error-unmatch *', and checking the command's return value.

This is too subtle and needs to be explained in a in-code comment. For example, it is unclear to me how this '*' pathspec and an untracked file that does not fnmatch(3) with the pattern (e.g. ".trash").

        $ rm -fr /var/tmp/x && git init /var/tmp/x && cd /var/tmp/x
	$ args='ls-files --others --exclude-standard'
	$ git $args | wc -l
        0
        $ git $args --error-unmatch -- '*' >/dev/null 2>&1 ; echo $?
        1
        $ >a
        $ git $args | wc -l
	1
        $ git $args --error-unmatch -- '*' >/dev/null 2>&1 ; echo $?
	0
	$ mv a .a
        $ git $args | wc -l
	1
        $ git $args --error-unmatch -- '*' >/dev/null 2>&1 ; echo $?
	0

The first two cases seem to be fine, but isn't the last one showing that your update is incorrect?

Show 26 quoted lines
>
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
> ---
>
> This seems to do the right thing, but I'm not quite sure, so I would
> appreciate a pair of expert eyeballs on it.
>
>  contrib/completion/git-completion.bash | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index c4feab68..5ea19018 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -348,9 +348,8 @@ __git_ps1 ()
>  			fi
>  
>  			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
> -				if [ -n "$(git ls-files --others --exclude-standard)" ]; then
> -					u="%"
> -				fi
> +				git ls-files --others --exclude-standard --error-unmatch -- '*' >/dev/null 2>/dev/null &&
> +				u="%"
>  			fi
>  		fi
>  	fi
SZEDER Gábor· May 9, 2012, 00:44 UTC · re: SZEDER Gábor · lore

[PATCH 19/19] bash prompt: alternative git prompt without command substitution

__git_ps1() prints the branch name, status indicators, etc. to stdout, therefore it has to be included in $PS1 through a command substitution to display that information in the prompt. The configuration is straightforward, but it imposes the overhead of fork()ing a subshell for the command substitution.

However, bash has the $PROMPT_COMMAND shell variable, which "if set, the value is executed as a command prior to issuing each primary prompt" (quoted from bash man page). Its value isn't executed in a subshell but in the context of the "main" shell, hence (non-local) variables set in invoked shell functions are available when expanding $PS1. We can use this facility to avoid that command substitution for __git_ps1().

So split out the meat of __git_ps1() into the new __git_prompt_command() function, which stores the branch name & co. in the $__git_ps1_string variable. This function, as its name suggests, should be included in $PROMPT_COMMAND, and $__git_ps1_string should in turn be included in $PS1 with a bit of a twist to put the parentheses around it:

   PROMPT_COMMAND=__git_prompt_command
   PS1='[\u@\h \W${__git_ps1_string:+ ($__git_ps1_string)}]\$ '

Turn __git_ps1() into a wrapper around __git_prompt_command() such that it's functionality remains unaltered, so already configured prompts won't break.

The whole series speeds up the bash prompt on Windows/MinGW immensely, in many cases brings it down to around 10ms on my machine while in powersave mode. Here are some timing results in three common scenarios (repeated 10 times, because the after cases were too fast to measure a single execution accurately with 'time'):

In my home directory, i.e. not in a git repository, before:
    /c/Users/szeder
    $ time for i in {0..9} ; do prompt=$(__git_ps1) ; done
    real    0m0.952s
    user    0m0.214s
    sys     0m0.444s
  After:
    /c/Users/szeder
    $ time for i in {0..9} ; do __git_prompt_command ;
           prompt=${__git_ps1_string:+ ($__git_ps1_string)} ; done
    real    0m0.718s
    user    0m0.136s
    sys     0m0.354s
  After, with discovery across filesystems enabled:
    /c/Users/szeder
    $ time for i in {0..9} ; do __git_prompt_command ;
           prompt=${__git_ps1_string:+ ($__git_ps1_string)} ; done
    real    0m0.078s
    user    0m0.016s
    sys     0m0.062s
At the top of a work tree, before:
    /c/Users/szeder/repo (master)
    $ time for i in {0..9} ; do prompt=$(__git_ps1) ; done
    real    0m2.901s
    user    0m0.391s
    sys     0m1.468s
  After:
    /c/Users/szeder/repo (master)
    $ time for i in {0..9} ; do __git_prompt_command ;
           prompt=${__git_ps1_string:+ ($__git_ps1_string)} ; done
    real    0m0.094s
    user    0m0.047s
    sys     0m0.047s
In a subdirectory, stash indicator enabled, before:
    /c/Users/szeder/repo/subdir (master $)
    $ time for i in {0..9} ; do prompt=$(__git_ps1) ; done
    real    0m4.118s
    user    0m0.468s
    sys     0m2.056s
  After:
    /c/Users/szeder/repo/subdir (master $)
    $ time for i in {0..9} ; do __git_prompt_command ;
           prompt=${__git_ps1_string:+ ($__git_ps1_string)} ; done
    real    0m0.858s
    user    0m0.152s
    sys     0m0.322s
  After, discovery across filesystems enabled:
    /c/Users/szeder/repo/subdir (master $)
    $ time for i in {0..9} ; do __git_prompt_command ;
           prompt=${__git_ps1_string:+ ($__git_ps1_string)} ; done
    real    0m0.109s
    user    0m0.047s
    sys     0m0.063s
Well, that's about 97% improvement.

The performance gain on Linux is smaller, the latter case goes down from 0.264s to 0.047, but since it was fast enough to begin with I won't lengthen this commit message with further timing results on Linux.

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---

We had some discussions recently about putting user-facing functions into a separate "namespace". This patch doesn't take that into account, but once a consensus is reached __git_prompt_command() should be put in that namespace.

 contrib/completion/git-completion.bash | 25 +++++++++++++++++++++----
 1 file changed, 21 insertions(+), 4 deletions(-)
Show changes to contrib/completion/git-completion.bash +21 −4
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 5ea19018..1c29f3d0 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -29,6 +29,11 @@
 #       are currently in a git repository.  The %s token will be
 #       the name of the current branch.
 #
+#       Alternatively, to make the above Bash prompt a bit faster:
+#               PROMPT_COMMAND=__git_prompt_command
+#               PS1='[\u@\h \W${__git_ps1_string:+ ($__git_ps1_string)}]\$ '
+#               GIT_DISCOVERY_ACROSS_FILESYSTEM=true
+#
 #       In addition, if you set GIT_PS1_SHOWDIRTYSTATE to a nonempty
 #       value, unstaged (*) and staged (+) changes will be shown next
 #       to the branch name.  You can configure this per-repository
@@ -258,11 +263,12 @@ __git_ps1_show_upstream ()
 }
 
 
-# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
-# returns text to add to bash PS1 prompt (includes branch name)
-__git_ps1 ()
+# Stores the text to be added to the bash prompt (branch name, status
+# indicators, etc.) in the $__git_ps1_string variable.
+__git_prompt_command ()
 {
 	local __git_dir=""
+	__git_ps1_string=""
 	__gitdir >/dev/null
 	if [ -z "$__git_dir" ]; then
 		return
@@ -365,7 +371,18 @@ __git_ps1 ()
 	fi
 
 	local f="$w$i$s$u"
-	printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
+	__git_ps1_string="$c${b##refs/heads/}${f:+ $f}$r$p"
+}
+
+# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
+# returns text to add to bash PS1 prompt (includes branch name)
+__git_ps1 ()
+{
+	local __git_ps1_string
+	__git_prompt_command
+	if [ -n "$__git_ps1_string" ]; then
+		printf -- "${1:- (%s)}" "$__git_ps1_string"
+	fi
 }
 
 __gitcomp_1 ()
-- 
1.7.10.1.541.gb1be298
Andrew Sayers· May 9, 2012, 19:38 UTC · re: SZEDER Gábor · lore

Re: [PATCH 19/19] bash prompt: alternative git prompt without command substitution

On 09/05/12 01:44, SZEDER Gábor wrote:
Show 22 quoted lines
> __git_ps1() prints the branch name, status indicators, etc. to stdout,
> therefore it has to be included in $PS1 through a command substitution
> to display that information in the prompt.  The configuration is
> straightforward, but it imposes the overhead of fork()ing a subshell
> for the command substitution.
> 
> However, bash has the $PROMPT_COMMAND shell variable, which "if set,
> the value is executed as a command prior to issuing each primary
> prompt" (quoted from bash man page).  Its value isn't executed in a
> subshell but in the context of the "main" shell, hence (non-local)
> variables set in invoked shell functions are available when expanding
> $PS1.  We can use this facility to avoid that command substitution for
> __git_ps1().
> 
> So split out the meat of __git_ps1() into the new
> __git_prompt_command() function, which stores the branch name & co.
> in the $__git_ps1_string variable.  This function, as its name
> suggests, should be included in $PROMPT_COMMAND, and $__git_ps1_string
> should in turn be included in $PS1 with a bit of a twist to put the
> parentheses around it:
> 
>    PROMPT_COMMAND=__git_prompt_command

Rather than overwrite any existing PROMPT_COMMAND, it would be better to do something like:

PROMPT_COMMAND="__git_prompt_command; $PROMPT_COMMAND"
Show 117 quoted lines
>    PS1='[\u@\h \W${__git_ps1_string:+ ($__git_ps1_string)}]\$ '
> 
> Turn __git_ps1() into a wrapper around __git_prompt_command() such
> that it's functionality remains unaltered, so already configured
> prompts won't break.
> 
> The whole series speeds up the bash prompt on Windows/MinGW
> immensely, in many cases brings it down to around 10ms on my
> machine while in powersave mode.  Here are some timing results in
> three common scenarios (repeated 10 times, because the after cases
> were too fast to measure a single execution accurately with 'time'):
> 
> In my home directory, i.e. not in a git repository, before:
> 
>     /c/Users/szeder
>     $ time for i in {0..9} ; do prompt=$(__git_ps1) ; done
> 
>     real    0m0.952s
>     user    0m0.214s
>     sys     0m0.444s
> 
>   After:
> 
>     /c/Users/szeder
>     $ time for i in {0..9} ; do __git_prompt_command ;
>            prompt=${__git_ps1_string:+ ($__git_ps1_string)} ; done
> 
>     real    0m0.718s
>     user    0m0.136s
>     sys     0m0.354s
> 
>   After, with discovery across filesystems enabled:
> 
>     /c/Users/szeder
>     $ time for i in {0..9} ; do __git_prompt_command ;
>            prompt=${__git_ps1_string:+ ($__git_ps1_string)} ; done
> 
>     real    0m0.078s
>     user    0m0.016s
>     sys     0m0.062s
> 
> At the top of a work tree, before:
> 
>     /c/Users/szeder/repo (master)
>     $ time for i in {0..9} ; do prompt=$(__git_ps1) ; done
> 
>     real    0m2.901s
>     user    0m0.391s
>     sys     0m1.468s
> 
>   After:
> 
>     /c/Users/szeder/repo (master)
>     $ time for i in {0..9} ; do __git_prompt_command ;
>            prompt=${__git_ps1_string:+ ($__git_ps1_string)} ; done
> 
>     real    0m0.094s
>     user    0m0.047s
>     sys     0m0.047s
> 
> In a subdirectory, stash indicator enabled, before:
> 
>     /c/Users/szeder/repo/subdir (master $)
>     $ time for i in {0..9} ; do prompt=$(__git_ps1) ; done
> 
>     real    0m4.118s
>     user    0m0.468s
>     sys     0m2.056s
> 
>   After:
> 
>     /c/Users/szeder/repo/subdir (master $)
>     $ time for i in {0..9} ; do __git_prompt_command ;
>            prompt=${__git_ps1_string:+ ($__git_ps1_string)} ; done
> 
>     real    0m0.858s
>     user    0m0.152s
>     sys     0m0.322s
> 
>   After, discovery across filesystems enabled:
> 
>     /c/Users/szeder/repo/subdir (master $)
>     $ time for i in {0..9} ; do __git_prompt_command ;
>            prompt=${__git_ps1_string:+ ($__git_ps1_string)} ; done
> 
>     real    0m0.109s
>     user    0m0.047s
>     sys     0m0.063s
> 
> Well, that's about 97% improvement.
> 
> The performance gain on Linux is smaller, the latter case goes down
> from 0.264s to 0.047, but since it was fast enough to begin with I
> won't lengthen this commit message with further timing results on
> Linux.
> 
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
> ---
> 
> We had some discussions recently about putting user-facing functions into
> a separate "namespace".  This patch doesn't take that into account, but
> once a consensus is reached __git_prompt_command() should be put in that
> namespace.
> 
>  contrib/completion/git-completion.bash | 25 +++++++++++++++++++++----
>  1 file changed, 21 insertions(+), 4 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 5ea19018..1c29f3d0 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -29,6 +29,11 @@
>  #       are currently in a git repository.  The %s token will be
>  #       the name of the current branch.
>  #
> +#       Alternatively, to make the above Bash prompt a bit faster:
> +#               PROMPT_COMMAND=__git_prompt_command

As above, I'd recommend a simple documentation change: PROMPT_COMMAND="__git_prompt_command; $PROMPT_COMMAND" (to show people how to chain any other prompt commands they have)

Show 39 quoted lines
> +#               PS1='[\u@\h \W${__git_ps1_string:+ ($__git_ps1_string)}]\$ '
> +#               GIT_DISCOVERY_ACROSS_FILESYSTEM=true
> +#
>  #       In addition, if you set GIT_PS1_SHOWDIRTYSTATE to a nonempty
>  #       value, unstaged (*) and staged (+) changes will be shown next
>  #       to the branch name.  You can configure this per-repository
> @@ -258,11 +263,12 @@ __git_ps1_show_upstream ()
>  }
>  
>  
> -# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
> -# returns text to add to bash PS1 prompt (includes branch name)
> -__git_ps1 ()
> +# Stores the text to be added to the bash prompt (branch name, status
> +# indicators, etc.) in the $__git_ps1_string variable.
> +__git_prompt_command ()
>  {
>  	local __git_dir=""
> +	__git_ps1_string=""
>  	__gitdir >/dev/null
>  	if [ -z "$__git_dir" ]; then
>  		return
> @@ -365,7 +371,18 @@ __git_ps1 ()
>  	fi
>  
>  	local f="$w$i$s$u"
> -	printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
> +	__git_ps1_string="$c${b##refs/heads/}${f:+ $f}$r$p"
> +}
> +
> +# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
> +# returns text to add to bash PS1 prompt (includes branch name)
> +__git_ps1 ()
> +{
> +	local __git_ps1_string
> +	__git_prompt_command
> +	if [ -n "$__git_ps1_string" ]; then
> +		printf -- "${1:- (%s)}" "$__git_ps1_string"
> +	fi

How hard/appropriate would it be to export individual parts of the prompt here? Something like:

__git_ps1_string_dirtystate="$i" __git_ps1_string_untrackedfiles="$u"

There have been requests in the past to let people individually colourise different bits of the prompt, which this would make practical.

>  }
>  
>  __gitcomp_1 ()
SZEDER Gábor· May 9, 2012, 22:08 UTC · re: Andrew Sayers · lore

Re: [PATCH 19/19] bash prompt: alternative git prompt without command substitution

Hi,
On Wed, May 09, 2012 at 08:38:22PM +0100, Andrew Sayers wrote:
Show 7 quoted lines
> On 09/05/12 01:44, SZEDER Gábor wrote:
> >    PROMPT_COMMAND=__git_prompt_command
> 
> Rather than overwrite any existing PROMPT_COMMAND, it would be better to
> do something like:
> 
> PROMPT_COMMAND="__git_prompt_command; $PROMPT_COMMAND"
Show 6 quoted lines
> > +#       Alternatively, to make the above Bash prompt a bit faster:
> > +#               PROMPT_COMMAND=__git_prompt_command
> 
> As above, I'd recommend a simple documentation change:
> PROMPT_COMMAND="__git_prompt_command; $PROMPT_COMMAND"
> (to show people how to chain any other prompt commands they have)

That's a good idea. In fact I did manage to overwrite my $PROMPT_COMMAND and was wondering why did the title of my terminal windows disappear so suddenly...

Show 26 quoted lines
> > @@ -365,7 +371,18 @@ __git_ps1 ()
> >  	fi
> >  
> >  	local f="$w$i$s$u"
> > -	printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
> > +	__git_ps1_string="$c${b##refs/heads/}${f:+ $f}$r$p"
> > +}
> > +
> > +# __git_ps1 accepts 0 or 1 arguments (i.e., format string)
> > +# returns text to add to bash PS1 prompt (includes branch name)
> > +__git_ps1 ()
> > +{
> > +	local __git_ps1_string
> > +	__git_prompt_command
> > +	if [ -n "$__git_ps1_string" ]; then
> > +		printf -- "${1:- (%s)}" "$__git_ps1_string"
> > +	fi
> 
> How hard/appropriate would it be to export individual parts of the
> prompt here?  Something like:
> 
> __git_ps1_string_dirtystate="$i"
> __git_ps1_string_untrackedfiles="$u"
> 
> There have been requests in the past to let people individually
> colourise different bits of the prompt, which this would make practical.

We can't do that from __git_ps1(), because, as I mentioned in the commit message, it must be invoked in a command substitution from $PS1, and what's exported in a subshell that stays in that subshell.

Doing so from __git_prompt_command() would be quite simple: just rename the appropriate variables and don't declare them as local. It would even be more pleasing to the eyes than the current one-letter variable names.

Gábor

← back to recent threads