threads / patch / 32890

patch, 2 partsAdd bash.showUntrackedFiles config option

Subject: [PATCH 0/2] Add bash.showUntrackedFiles config option

## tl;dr

20 messages between Feb 12, 2013 and Feb 13, 2013. Diffs are folded; open one to read it.

replies: 19people: 2as markdown or json

Martin Erik Werner· Feb 12, 2013, 20:12 UTC · lore

Hi,

Here is a patch adding a config option for showing untracked files in the shell prompt, I've noticed having it enabled tends to make the prompt act very sluggish in some cases (large repos / unfriendly filesystems). So it would be nice to have a more fine-grained control over it, similar to what exists for bash.showDirtyState.

Martin Erik Werner (2):
  bash completion: add bash.showUntrackedFiles option
  t9903: add test case for bash.showUntrackedFiles

 contrib/completion/git-prompt.sh |   11 ++++++++---
 t/t9903-bash-prompt.sh           |   11 +++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

-- 
1.7.10.4

Martin Erik Werner· Feb 12, 2013, 20:12 UTC · re: Martin Erik Werner · lore

[PATCH 1/2] bash completion: add bash.showUntrackedFiles option

Add a config option 'bash.showUntrackedFiles' which allows enabling the prompt showing untracked files on a per-repository basis. This is useful for some repositories where the 'git ls-files ...' command may take a long time.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 contrib/completion/git-prompt.sh |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Show changes to contrib/completion/git-prompt.sh +8 −3

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9bef053..9b2eec2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -43,7 +43,10 @@
 #
 # If you would like to see if there're untracked files, then you can set
 # GIT_PS1_SHOWUNTRACKEDFILES to a nonempty value. If there're untracked
-# files, then a '%' will be shown next to the branch name.
+# files, then a '%' will be shown next to the branch name.  You can
+# configure this per-repository with the bash.showUntrackedFiles
+# variable, which defaults to true once GIT_PS1_SHOWUNTRACKEDFILES is
+# enabled.
 #
 # If you would like to see the difference between HEAD and its upstream,
 # set GIT_PS1_SHOWUPSTREAM="auto".  A "<" indicates you are behind, ">"
@@ -332,8 +335,10 @@ __git_ps1 ()
 			fi
 
 			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
-				if [ -n "$(git ls-files --others --exclude-standard)" ]; then
-					u="%"
+				if [ "$(git config --bool bash.showUntrackedFiles)" != "false" ]; then
+					if [ -n "$(git ls-files --others --exclude-standard)" ]; then
+						u="%"
+					fi
 				fi
 			fi

-- 
1.7.10.4

Junio C Hamano· Feb 12, 2013, 22:17 UTC · re: Martin Erik Werner · lore

Re: [PATCH 1/2] bash completion: add bash.showUntrackedFiles option

Martin Erik Werner <martinerikwerner@gmail.com> writes:

Show 7 quoted lines

> Add a config option 'bash.showUntrackedFiles' which allows enabling
> the prompt showing untracked files on a per-repository basis. This is
> useful for some repositories where the 'git ls-files ...' command may
> take a long time.
>
> Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
> ---

OK.

The subject needs to be corrected, though ;-) No need to resend. I'll do s/completion/prompt/ while applying.

Thanks.

Show 31 quoted lines

>  contrib/completion/git-prompt.sh |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 9bef053..9b2eec2 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -43,7 +43,10 @@
>  #
>  # If you would like to see if there're untracked files, then you can set
>  # GIT_PS1_SHOWUNTRACKEDFILES to a nonempty value. If there're untracked
> -# files, then a '%' will be shown next to the branch name.
> +# files, then a '%' will be shown next to the branch name.  You can
> +# configure this per-repository with the bash.showUntrackedFiles
> +# variable, which defaults to true once GIT_PS1_SHOWUNTRACKEDFILES is
> +# enabled.
>  #
>  # If you would like to see the difference between HEAD and its upstream,
>  # set GIT_PS1_SHOWUPSTREAM="auto".  A "<" indicates you are behind, ">"
> @@ -332,8 +335,10 @@ __git_ps1 ()
>  			fi
>  
>  			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
> -				if [ -n "$(git ls-files --others --exclude-standard)" ]; then
> -					u="%"
> +				if [ "$(git config --bool bash.showUntrackedFiles)" != "false" ]; then
> +					if [ -n "$(git ls-files --others --exclude-standard)" ]; then
> +						u="%"
> +					fi
>  				fi
>  			fi

Martin Erik Werner· Feb 12, 2013, 20:12 UTC · re: Martin Erik Werner · lore

[PATCH 2/2] t9903: add test case for bash.showUntrackedFiles

Add a test case for the bash.showUntrackedFiles config option, which checks that the config option can disable the global effect of the GIT_PS1_SHOWUNTRACKEDFILES environmant variable.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t9903-bash-prompt.sh |   11 +++++++++++
 1 file changed, 11 insertions(+)

Show changes to t/t9903-bash-prompt.sh +11 −0

diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index f17c1f8..c9417b9 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -447,6 +447,17 @@ test_expect_success 'prompt - untracked files status indicator - not shown insid
 	test_cmp expected "$actual"
 '
 
+test_expect_success 'prompt - untracked files status indicator - disabled by config' '
+	printf " (master)" > expected &&
+	echo "untracked" > file_untracked &&
+	test_config bash.showUntrackedFiles false &&
+	(
+		GIT_PS1_SHOWUNTRACKEDFILES=y &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
 test_expect_success 'prompt - format string starting with dash' '
 	printf -- "-master" > expected &&
 	__git_ps1 "-%s" > "$actual" &&

-- 
1.7.10.4

Junio C Hamano· Feb 12, 2013, 22:29 UTC · re: Martin Erik Werner · lore

Re: [PATCH 2/2] t9903: add test case for bash.showUntrackedFiles

Martin Erik Werner <martinerikwerner@gmail.com> writes:

Show 27 quoted lines

> Add a test case for the bash.showUntrackedFiles config option, which
> checks that the config option can disable the global effect of the
> GIT_PS1_SHOWUNTRACKEDFILES environmant variable.
>
> Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
> ---
>  t/t9903-bash-prompt.sh |   11 +++++++++++
>  1 file changed, 11 insertions(+)
>
> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index f17c1f8..c9417b9 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -447,6 +447,17 @@ test_expect_success 'prompt - untracked files status indicator - not shown insid
>  	test_cmp expected "$actual"
>  '
>  
> +test_expect_success 'prompt - untracked files status indicator - disabled by config' '
> +	printf " (master)" > expected &&
> +	echo "untracked" > file_untracked &&
> +	test_config bash.showUntrackedFiles false &&
> +	(
> +		GIT_PS1_SHOWUNTRACKEDFILES=y &&
> +		__git_ps1 > "$actual"
> +	) &&
> +	test_cmp expected "$actual"
> +'

All six combinations need checking:

 * not having the configuration at all and not having the shell
   variable should not show the untracked indicator (already tested).

 * not having the configuration at all and having the shell variable
   should show the untracked indicator (already tested).

 * setting configuration to true without having the shell variable
   should not show the untracked indicator.

 * setting configuration to true and having the shell variable
   should show the unttracked indicator.

 * setting configuration to false and having the shell variable
   should not show the untracked indicator (the above test checks
   this).

 * setting configuration to false without having the shell variable
   should not show the untracked indicator.

to prevent others from breaking the code you wrote for [PATCH 1/2], so you need three more tests, I guess?

Martin Erik Werner· Feb 13, 2013, 10:58 UTC · re: Junio C Hamano · lore

[PATCH v2 0/3] Add bash.showUntrackedFiles config option

On Tue, 2013-02-12 at 14:29 -0800, Junio C Hamano wrote:

Show 53 quoted lines

> Martin Erik Werner <martinerikwerner@gmail.com> writes:
> 
> > Add a test case for the bash.showUntrackedFiles config option, which
> > checks that the config option can disable the global effect of the
> > GIT_PS1_SHOWUNTRACKEDFILES environmant variable.
> >
> > Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
> > ---
> >  t/t9903-bash-prompt.sh |   11 +++++++++++
> >  1 file changed, 11 insertions(+)
> >
> > diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> > index f17c1f8..c9417b9 100755
> > --- a/t/t9903-bash-prompt.sh
> > +++ b/t/t9903-bash-prompt.sh
> > @@ -447,6 +447,17 @@ test_expect_success 'prompt - untracked files status indicator - not shown insid
> >  	test_cmp expected "$actual"
> >  '
> >  
> > +test_expect_success 'prompt - untracked files status indicator - disabled by config' '
> > +	printf " (master)" > expected &&
> > +	echo "untracked" > file_untracked &&
> > +	test_config bash.showUntrackedFiles false &&
> > +	(
> > +		GIT_PS1_SHOWUNTRACKEDFILES=y &&
> > +		__git_ps1 > "$actual"
> > +	) &&
> > +	test_cmp expected "$actual"
> > +'
> 
> All six combinations need checking:
> 
>  * not having the configuration at all and not having the shell
>    variable should not show the untracked indicator (already tested).
> 
>  * not having the configuration at all and having the shell variable
>    should show the untracked indicator (already tested).
> 
>  * setting configuration to true without having the shell variable
>    should not show the untracked indicator.
> 
>  * setting configuration to true and having the shell variable
>    should show the unttracked indicator.
> 
>  * setting configuration to false and having the shell variable
>    should not show the untracked indicator (the above test checks
>    this).
> 
>  * setting configuration to false without having the shell variable
>    should not show the untracked indicator.
> 
> to prevent others from breaking the code you wrote for [PATCH 1/2],
> so you need three more tests, I guess?

Ah, yes, I was mimicing what the test did for bash.showDirtyState, I've now added the three extra tests for bash.showUntrackedFiles, which should cover all of the above cases, hopefully?

I've also added in the three extra tests for bash.showDirtyState, equivalently. These only cover the case of dirty files and not combinations with content in index, which I felt was a bit overkill, is that reasonable?

Thanks for the review :)

-- 
Martin Erik Werner <martinerikwerner@gmail.com>

Martin Erik Werner· Feb 13, 2013, 11:01 UTC · re: Martin Erik Werner · lore

[PATCH v2 1/3] shell prompt: add bash.showUntrackedFiles option

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 contrib/completion/git-prompt.sh |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

Show changes to contrib/completion/git-prompt.sh +8 −3

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 9bef053..9b2eec2 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -43,7 +43,10 @@
 #
 # If you would like to see if there're untracked files, then you can set
 # GIT_PS1_SHOWUNTRACKEDFILES to a nonempty value. If there're untracked
-# files, then a '%' will be shown next to the branch name.
+# files, then a '%' will be shown next to the branch name.  You can
+# configure this per-repository with the bash.showUntrackedFiles
+# variable, which defaults to true once GIT_PS1_SHOWUNTRACKEDFILES is
+# enabled.
 #
 # If you would like to see the difference between HEAD and its upstream,
 # set GIT_PS1_SHOWUPSTREAM="auto".  A "<" indicates you are behind, ">"
@@ -332,8 +335,10 @@ __git_ps1 ()
 			fi
 
 			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
-				if [ -n "$(git ls-files --others --exclude-standard)" ]; then
-					u="%"
+				if [ "$(git config --bool bash.showUntrackedFiles)" != "false" ]; then
+					if [ -n "$(git ls-files --others --exclude-standard)" ]; then
+						u="%"
+					fi
 				fi
 			fi

-- 
1.7.10.4

Martin Erik Werner· Feb 13, 2013, 11:01 UTC · re: Martin Erik Werner · lore

[PATCH v2 2/3] t9903: add tests for bash.showUntrackedFiles

Add 4 test for the bash.showUntrackedFiles config option, covering all combinations of the shell var being set/unset and the config option being enabled/disabled.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t9903-bash-prompt.sh |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Show changes to t/t9903-bash-prompt.sh +40 −0

diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index f17c1f8..cb008e2 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -437,6 +437,46 @@ test_expect_success 'prompt - untracked files status indicator - untracked files
 	test_cmp expected "$actual"
 '
 
+test_expect_success 'prompt - untracked files status indicator - shell variable unset with config disabled' '
+	printf " (master)" > expected &&
+	test_config bash.showUntrackedFiles false &&
+	(
+		unset -v GIT_PS1_SHOWUNTRACKEDFILES &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - untracked files status indicator - shell variable unset with config enabled' '
+	printf " (master)" > expected &&
+	test_config bash.showUntrackedFiles true &&
+	(
+		unset -v GIT_PS1_SHOWUNTRACKEDFILES &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - untracked files status indicator - shell variable set with config disabled' '
+	printf " (master)" > expected &&
+	test_config bash.showUntrackedFiles false &&
+	(
+		GIT_PS1_SHOWUNTRACKEDFILES=y &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - untracked files status indicator - shell variable set with config enabled' '
+	printf " (master %%)" > expected &&
+	test_config bash.showUntrackedFiles true &&
+	(
+		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 &&
 	(

-- 
1.7.10.4

Junio C Hamano· Feb 13, 2013, 16:23 UTC · re: Martin Erik Werner · lore

Re: [PATCH v2 2/3] t9903: add tests for bash.showUntrackedFiles

Martin Erik Werner <martinerikwerner@gmail.com> writes:

Show 22 quoted lines

> Add 4 test for the bash.showUntrackedFiles config option, covering all
> combinations of the shell var being set/unset and the config option
> being enabled/disabled.
>
> Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
> ---
>  t/t9903-bash-prompt.sh |   40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index f17c1f8..cb008e2 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -437,6 +437,46 @@ test_expect_success 'prompt - untracked files status indicator - untracked files
>  	test_cmp expected "$actual"
>  '
>  
> +test_expect_success 'prompt - untracked files status indicator - shell variable unset with config disabled' '
> +	printf " (master)" > expected &&
> +	test_config bash.showUntrackedFiles false &&
> +	(
> +		unset -v GIT_PS1_SHOWUNTRACKEDFILES &&

We do not use "unset -v" anywhere else in our system. Shells mimicking SysV may choke on it. A Portable POSIX script can omit "-v" when unsetting a variable.

Also "unset" can return false when the variable is not set to begin with with some shells.

Neither of these matters for this particular case because we know we are running this under bash in non-posix mode. I however wonder if we can do something to prevent careless coders to copy and paste this piece when updating other tests that are not limited to bash. Commenting each and every use of "unset -v" does not sound like a good solution and perhaps I am being unnecessarily worried too much.

Martin Erik Werner· Feb 13, 2013, 17:27 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 2/3] t9903: add tests for bash.showUntrackedFiles

On Wed, 2013-02-13 at 08:23 -0800, Junio C Hamano wrote:

Show 39 quoted lines

> Martin Erik Werner <martinerikwerner@gmail.com> writes:
> 
> > Add 4 test for the bash.showUntrackedFiles config option, covering all
> > combinations of the shell var being set/unset and the config option
> > being enabled/disabled.
> >
> > Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
> > ---
> >  t/t9903-bash-prompt.sh |   40 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 40 insertions(+)
> >
> > diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> > index f17c1f8..cb008e2 100755
> > --- a/t/t9903-bash-prompt.sh
> > +++ b/t/t9903-bash-prompt.sh
> > @@ -437,6 +437,46 @@ test_expect_success 'prompt - untracked files status indicator - untracked files
> >  	test_cmp expected "$actual"
> >  '
> >  
> > +test_expect_success 'prompt - untracked files status indicator - shell variable unset with config disabled' '
> > +	printf " (master)" > expected &&
> > +	test_config bash.showUntrackedFiles false &&
> > +	(
> > +		unset -v GIT_PS1_SHOWUNTRACKEDFILES &&
> 
> We do not use "unset -v" anywhere else in our system.  Shells
> mimicking SysV may choke on it.  A Portable POSIX script can omit
> "-v" when unsetting a variable.
> 
> Also "unset" can return false when the variable is not set to begin
> with with some shells.
> 
> Neither of these matters for this particular case because we know we
> are running this under bash in non-posix mode.  I however wonder if
> we can do something to prevent careless coders to copy and paste
> this piece when updating other tests that are not limited to bash.
> Commenting each and every use of "unset -v" does not sound like a
> good solution and perhaps I am being unnecessarily worried too much.
>

Yeah, my (ba)sh foo is a bit limited, I was just basing on http://wiki.bash-hackers.org/commands/builtin/unset#portability_considerations which seemed to recommend using -v.

So would it make sense to do:
	GIT_PS1_SHOWUNTRACKEDFILES="dummy" &&
	unset GIT_PS1_SHOWUNTRACKEDFILES &&
	(...)
instead then?

-- 
Martin Erik Werner <martinerikwerner@gmail.com>

Junio C Hamano· Feb 13, 2013, 19:51 UTC · re: Martin Erik Werner · lore

Re: [PATCH v2 2/3] t9903: add tests for bash.showUntrackedFiles

Martin Erik Werner <martinerikwerner@gmail.com> writes:

Show 5 quoted lines

> So would it make sense to do:
> 	GIT_PS1_SHOWUNTRACKEDFILES="dummy" &&
> 	unset GIT_PS1_SHOWUNTRACKEDFILES &&
> 	(...)
> instead then?

I think we have sane_unset exactly for this reason.

Martin Erik Werner· Feb 13, 2013, 11:02 UTC · re: Martin Erik Werner · lore

[PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState

Added 3 extra tests for the bash.showDirtyState config option, tests should now cover all combinations of the shell var being set/unset and the config option being enabled/disabled, given a dirty file.

* Renamed test 'disabled by config' to 'shell variable set with config
  disabled' for consistency
---
 t/t9903-bash-prompt.sh |   38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

Show changes to t/t9903-bash-prompt.sh +37 −1

diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index cb008e2..fa81b09 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -360,12 +360,48 @@ test_expect_success 'prompt - dirty status indicator - before root commit' '
 	test_cmp expected "$actual"
 '
 
-test_expect_success 'prompt - dirty status indicator - disabled by config' '
+test_expect_success 'prompt - dirty status indicator - shell variable unset with config disabled' '
 	printf " (master)" > expected &&
 	echo "dirty" > file &&
 	test_when_finished "git reset --hard" &&
 	test_config bash.showDirtyState false &&
 	(
+		unset -v GIT_PS1_SHOWDIRTYSTATE &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - dirty status indicator - shell variable unset with config enabled' '
+	printf " (master)" > expected &&
+	echo "dirty" > file &&
+	test_when_finished "git reset --hard" &&
+	test_config bash.showDirtyState true &&
+	(
+		unset -v GIT_PS1_SHOWDIRTYSTATE &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - dirty status indicator - shell variable set with config disabled' '
+	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 - shell variable set with config enabled' '
+	printf " (master *)" > expected &&
+	echo "dirty" > file &&
+	test_when_finished "git reset --hard" &&
+	test_config bash.showDirtyState true &&
+	(
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		__git_ps1 > "$actual"
 	) &&

-- 
1.7.10.4

Junio C Hamano· Feb 13, 2013, 16:28 UTC · re: Martin Erik Werner · lore

Re: [PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState

Martin Erik Werner <martinerikwerner@gmail.com> writes:

> Added 3 extra tests for the bash.showDirtyState config option, tests
> should now cover all combinations of the shell var being set/unset and
> the config option being enabled/disabled, given a dirty file.

Strictly speaking, you have 6 not 4 combinations (shell variable set/unset * config missing/set to false/set to true). I think these additional tests cover should all 6 because "config missing" case should already have had tests before bash.showDirtyState was added.

> * Renamed test 'disabled by config' to 'shell variable set with config
>   disabled' for consistency
> ---

Sign-off?

Show 57 quoted lines

>  t/t9903-bash-prompt.sh |   38 +++++++++++++++++++++++++++++++++++++-
>  1 file changed, 37 insertions(+), 1 deletion(-)
>
> diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> index cb008e2..fa81b09 100755
> --- a/t/t9903-bash-prompt.sh
> +++ b/t/t9903-bash-prompt.sh
> @@ -360,12 +360,48 @@ test_expect_success 'prompt - dirty status indicator - before root commit' '
>  	test_cmp expected "$actual"
>  '
>  
> -test_expect_success 'prompt - dirty status indicator - disabled by config' '
> +test_expect_success 'prompt - dirty status indicator - shell variable unset with config disabled' '
>  	printf " (master)" > expected &&
>  	echo "dirty" > file &&
>  	test_when_finished "git reset --hard" &&
>  	test_config bash.showDirtyState false &&
>  	(
> +		unset -v GIT_PS1_SHOWDIRTYSTATE &&
> +		__git_ps1 > "$actual"
> +	) &&
> +	test_cmp expected "$actual"
> +'
> +
> +test_expect_success 'prompt - dirty status indicator - shell variable unset with config enabled' '
> +	printf " (master)" > expected &&
> +	echo "dirty" > file &&
> +	test_when_finished "git reset --hard" &&
> +	test_config bash.showDirtyState true &&
> +	(
> +		unset -v GIT_PS1_SHOWDIRTYSTATE &&
> +		__git_ps1 > "$actual"
> +	) &&
> +	test_cmp expected "$actual"
> +'
> +
> +test_expect_success 'prompt - dirty status indicator - shell variable set with config disabled' '
> +	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 - shell variable set with config enabled' '
> +	printf " (master *)" > expected &&
> +	echo "dirty" > file &&
> +	test_when_finished "git reset --hard" &&
> +	test_config bash.showDirtyState true &&
> +	(
>  		GIT_PS1_SHOWDIRTYSTATE=y &&
>  		__git_ps1 > "$actual"
>  	) &&

Martin Erik Werner· Feb 13, 2013, 17:37 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState

On Wed, 2013-02-13 at 08:28 -0800, Junio C Hamano wrote:

Show 11 quoted lines

> Martin Erik Werner <martinerikwerner@gmail.com> writes:
> 
> > Added 3 extra tests for the bash.showDirtyState config option, tests
> > should now cover all combinations of the shell var being set/unset and
> > the config option being enabled/disabled, given a dirty file.
> 
> Strictly speaking, you have 6 not 4 combinations (shell variable
> set/unset * config missing/set to false/set to true).  I think these
> additional tests cover should all 6 because "config missing" case
> should already have had tests before bash.showDirtyState was added.
>

Indeed, I only mentioned 4 since the other ones existed already, and I didn't change them, but maybe it should be mentioned as "combined with previous tests (...) cover all 6 combinations (...)" then?

Show 5 quoted lines

> > * Renamed test 'disabled by config' to 'shell variable set with config
> >   disabled' for consistency
> > ---
> 
> Sign-off?

Ah, just forgot the -s flag on that commit, yes it should be Signed-off by me.

Show 58 quoted lines

> 
> >  t/t9903-bash-prompt.sh |   38 +++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 37 insertions(+), 1 deletion(-)
> >
> > diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
> > index cb008e2..fa81b09 100755
> > --- a/t/t9903-bash-prompt.sh
> > +++ b/t/t9903-bash-prompt.sh
> > @@ -360,12 +360,48 @@ test_expect_success 'prompt - dirty status indicator - before root commit' '
> >  	test_cmp expected "$actual"
> >  '
> >  
> > -test_expect_success 'prompt - dirty status indicator - disabled by config' '
> > +test_expect_success 'prompt - dirty status indicator - shell variable unset with config disabled' '
> >  	printf " (master)" > expected &&
> >  	echo "dirty" > file &&
> >  	test_when_finished "git reset --hard" &&
> >  	test_config bash.showDirtyState false &&
> >  	(
> > +		unset -v GIT_PS1_SHOWDIRTYSTATE &&
> > +		__git_ps1 > "$actual"
> > +	) &&
> > +	test_cmp expected "$actual"
> > +'
> > +
> > +test_expect_success 'prompt - dirty status indicator - shell variable unset with config enabled' '
> > +	printf " (master)" > expected &&
> > +	echo "dirty" > file &&
> > +	test_when_finished "git reset --hard" &&
> > +	test_config bash.showDirtyState true &&
> > +	(
> > +		unset -v GIT_PS1_SHOWDIRTYSTATE &&
> > +		__git_ps1 > "$actual"
> > +	) &&
> > +	test_cmp expected "$actual"
> > +'
> > +
> > +test_expect_success 'prompt - dirty status indicator - shell variable set with config disabled' '
> > +	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 - shell variable set with config enabled' '
> > +	printf " (master *)" > expected &&
> > +	echo "dirty" > file &&
> > +	test_when_finished "git reset --hard" &&
> > +	test_config bash.showDirtyState true &&
> > +	(
> >  		GIT_PS1_SHOWDIRTYSTATE=y &&
> >  		__git_ps1 > "$actual"
> >  	) &&

Junio C Hamano· Feb 13, 2013, 19:53 UTC · re: Martin Erik Werner · lore

Re: [PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState

Martin Erik Werner <martinerikwerner@gmail.com> writes:

Show 9 quoted lines

>> Strictly speaking, you have 6 not 4 combinations (shell variable
>> set/unset * config missing/set to false/set to true).  I think these
>> additional tests cover should all 6 because "config missing" case
>> should already have had tests before bash.showDirtyState was added.
>> 
>
> Indeed, I only mentioned 4 since the other ones existed already, and I
> didn't change them, but maybe it should be mentioned as "combined with
> previous tests (...) cover all 6 combinations (...)" then?

It should be sufficient to change the third line of your original to say "the config option being missing/enabled/disabled, given a dirty file." and nothing else, I think.

>> Sign-off?
>
> Ah, just forgot the -s flag on that commit, yes it should be Signed-off
> by me.

OK, I'll locally amend the patch.  Thanks.

Martin Erik Werner· Feb 13, 2013, 20:40 UTC · re: Junio C Hamano · lore

Re: [PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState

On Wed, 2013-02-13 at 11:53 -0800, Junio C Hamano wrote:

Show 22 quoted lines

> Martin Erik Werner <martinerikwerner@gmail.com> writes:
> 
> >> Strictly speaking, you have 6 not 4 combinations (shell variable
> >> set/unset * config missing/set to false/set to true).  I think these
> >> additional tests cover should all 6 because "config missing" case
> >> should already have had tests before bash.showDirtyState was added.
> >> 
> >
> > Indeed, I only mentioned 4 since the other ones existed already, and I
> > didn't change them, but maybe it should be mentioned as "combined with
> > previous tests (...) cover all 6 combinations (...)" then?
> 
> It should be sufficient to change the third line of your original to
> say "the config option being missing/enabled/disabled, given a dirty
> file." and nothing else, I think.
> 
> >> Sign-off?
> >
> > Ah, just forgot the -s flag on that commit, yes it should be Signed-off
> > by me.
> 
> OK, I'll locally amend the patch.  Thanks.

Ok, so I shouldn't reroll them with s/unset -v/sane_unset/ and reworded commits + sign-off then, I can if you prefer that?

-- 
Martin Erik Werner <martinerikwerner@gmail.com>

Junio C Hamano· Feb 13, 2013, 20:42 UTC · re: Martin Erik Werner · lore

Re: [PATCH v2 3/3] t9903: add extra tests for bash.showDirtyState

Martin Erik Werner <martinerikwerner@gmail.com> writes:

>> OK, I'll locally amend the patch.  Thanks.
>
> Ok, so I shouldn't reroll them with s/unset -v/sane_unset/ and reworded
> commits + sign-off then, I can if you prefer that?

You can if you wanted to.  That would be less work for me ;-).

Martin Erik Werner· Feb 13, 2013, 20:58 UTC · re: Junio C Hamano · lore

[PATCH v3 2/3] t9903: add tests for bash.showUntrackedFiles

Add 4 test for the bash.showUntrackedFiles config option, the tests now cover all combinations of the shell var being set/unset and the config option being missing/enabled/disabled.

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t9903-bash-prompt.sh |   40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

Show changes to t/t9903-bash-prompt.sh +40 −0

diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index f17c1f8..dd9ac6a 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -437,6 +437,46 @@ test_expect_success 'prompt - untracked files status indicator - untracked files
 	test_cmp expected "$actual"
 '
 
+test_expect_success 'prompt - untracked files status indicator - shell variable unset with config disabled' '
+	printf " (master)" > expected &&
+	test_config bash.showUntrackedFiles false &&
+	(
+		sane_unset GIT_PS1_SHOWUNTRACKEDFILES &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - untracked files status indicator - shell variable unset with config enabled' '
+	printf " (master)" > expected &&
+	test_config bash.showUntrackedFiles true &&
+	(
+		sane_unset GIT_PS1_SHOWUNTRACKEDFILES &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - untracked files status indicator - shell variable set with config disabled' '
+	printf " (master)" > expected &&
+	test_config bash.showUntrackedFiles false &&
+	(
+		GIT_PS1_SHOWUNTRACKEDFILES=y &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - untracked files status indicator - shell variable set with config enabled' '
+	printf " (master %%)" > expected &&
+	test_config bash.showUntrackedFiles true &&
+	(
+		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 &&
 	(

-- 
1.7.10.4

Martin Erik Werner· Feb 13, 2013, 20:58 UTC · re: Martin Erik Werner · lore

[PATCH v3 3/3] t9903: add extra tests for bash.showDirtyState

Add 3 extra tests for the bash.showDirtyState config option, the test now cover all combinations of the shell var being set/unset and the config option being missing/enabled/disabled, given a dirty file.

* Renamed test 'disabled by config' to 'shell variable set with config
  disabled' for consistency

Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
---
 t/t9903-bash-prompt.sh |   38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

Show changes to t/t9903-bash-prompt.sh +37 −1

diff --git a/t/t9903-bash-prompt.sh b/t/t9903-bash-prompt.sh
index dd9ac6a..2101d91 100755
--- a/t/t9903-bash-prompt.sh
+++ b/t/t9903-bash-prompt.sh
@@ -360,12 +360,48 @@ test_expect_success 'prompt - dirty status indicator - before root commit' '
 	test_cmp expected "$actual"
 '
 
-test_expect_success 'prompt - dirty status indicator - disabled by config' '
+test_expect_success 'prompt - dirty status indicator - shell variable unset with config disabled' '
 	printf " (master)" > expected &&
 	echo "dirty" > file &&
 	test_when_finished "git reset --hard" &&
 	test_config bash.showDirtyState false &&
 	(
+		sane_unset GIT_PS1_SHOWDIRTYSTATE &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - dirty status indicator - shell variable unset with config enabled' '
+	printf " (master)" > expected &&
+	echo "dirty" > file &&
+	test_when_finished "git reset --hard" &&
+	test_config bash.showDirtyState true &&
+	(
+		sane_unset GIT_PS1_SHOWDIRTYSTATE &&
+		__git_ps1 > "$actual"
+	) &&
+	test_cmp expected "$actual"
+'
+
+test_expect_success 'prompt - dirty status indicator - shell variable set with config disabled' '
+	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 - shell variable set with config enabled' '
+	printf " (master *)" > expected &&
+	echo "dirty" > file &&
+	test_when_finished "git reset --hard" &&
+	test_config bash.showDirtyState true &&
+	(
 		GIT_PS1_SHOWDIRTYSTATE=y &&
 		__git_ps1 > "$actual"
 	) &&

-- 
1.7.10.4

Junio C Hamano· Feb 13, 2013, 16:12 UTC · re: Martin Erik Werner · lore

Re: [PATCH v2 1/3] shell prompt: add bash.showUntrackedFiles option

Martin Erik Werner <martinerikwerner@gmail.com> writes:

Show 38 quoted lines

> Add a config option 'bash.showUntrackedFiles' which allows enabling
> the prompt showing untracked files on a per-repository basis. This is
> useful for some repositories where the 'git ls-files ...' command may
> take a long time.
>
> Signed-off-by: Martin Erik Werner <martinerikwerner@gmail.com>
> ---
>  contrib/completion/git-prompt.sh |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
> index 9bef053..9b2eec2 100644
> --- a/contrib/completion/git-prompt.sh
> +++ b/contrib/completion/git-prompt.sh
> @@ -43,7 +43,10 @@
>  #
>  # If you would like to see if there're untracked files, then you can set
>  # GIT_PS1_SHOWUNTRACKEDFILES to a nonempty value. If there're untracked
> -# files, then a '%' will be shown next to the branch name.
> +# files, then a '%' will be shown next to the branch name.  You can
> +# configure this per-repository with the bash.showUntrackedFiles
> +# variable, which defaults to true once GIT_PS1_SHOWUNTRACKEDFILES is
> +# enabled.
>  #
>  # If you would like to see the difference between HEAD and its upstream,
>  # set GIT_PS1_SHOWUPSTREAM="auto".  A "<" indicates you are behind, ">"
> @@ -332,8 +335,10 @@ __git_ps1 ()
>  			fi
>  
>  			if [ -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" ]; then
> -				if [ -n "$(git ls-files --others --exclude-standard)" ]; then
> -					u="%"
> +				if [ "$(git config --bool bash.showUntrackedFiles)" != "false" ]; then
> +					if [ -n "$(git ls-files --others --exclude-standard)" ]; then
> +						u="%"
> +					fi
>  				fi
>  			fi

Somebody should simplify this deeply nested "if/then/if/then/fi/fi" sequence to a single if/then/fi statement, i.e. something like:

	if test -n "${GIT_PS1_SHOWUNTRACKEDFILES-}" &&
           test "$(git config --bool bash.showUntrackedFiles)" != false
        then
		u='%'
	fi

And do the same for the other one this patch copies the above from.

No need to re-roll this patch, though. It is a separate clean-up to be done on top, once this series is settles.

Thanks.