Re: [PATCH 3/3] Fixes bug: GIT_PS1_SHOWDIRTYSTATE is no not respect diff.ignoreSubmodules config variable

From: Jens Lehmann <jens.lehmann@web.de>
Date: Dec 25, 2010, 12:33 UTC
Message-ID: <4D15E48A.9050805@web.de>
In-Reply-To: <1293240049-7744-3-git-send-email-zapped@mail.ru>

Am 25.12.2010 02:20, schrieb Zapped:

Show 19 quoted lines

> Signed-off-by: Zapped <zapped@mail.ru>
> ---
>  contrib/completion/git-completion.bash |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index d3037fc..50fc385 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -280,7 +280,8 @@ __git_ps1 ()
>  		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="*"
> +					is=$(git config diff.ignoreSubmodules)
> +					git diff --no-ext-diff --quiet --exit-code --ignore-submodules=$is || w="*"
>  					if git rev-parse --quiet --verify HEAD >/dev/null; then
>  						git diff-index --cached --quiet HEAD -- || i="+"
>  					else

Thanks for resubmitting this as an inline patch for review (although it would have been easier for me if the commit message would have described the problem you tried to fix a bit more in detail ;-).

After testing this patch it looks like it has a few issues:

1) it will break any per-submodule configuration done via
   the 'submodule.<name>.ignore' setting in .git/config or
   .gitmodules, as using the --ignore-submodules option
   overrides those while only setting 'diff.ignoreSubmodules'
   should not do that.

2) If diff.ignoreSubmodules is unset it leads to an error
   every time the prompt is displayed:
   'fatal: bad --ignore-submodules argument:'

3) And for me it didn't change the behavior at all:

   - The '*' in the prompt vanishes as I set diff.ignoreSubmodules
     as expected with or without your patch.
     Am I missing something here?

   - The real problem here is that the '+' never goes away even
     when 'diff.ignoreSubmodules' is set to 'all'. This is due
     to the fact that 'diff.ignoreSubmodules' is only honored by
     "git diff", but not by "git diff-index".

So the real issue here seems to be the "git diff-index" call, which doesn't honor the 'diff.ignoreSubmodules' setting. In commit 37aea37 Dscho (CCed) introduced this configuration setting while explicitly stating that it only affects porcelain. As the other config options always influence porcelain and plumbing, it looks like we would want to have this option honored by plumbing too, no?

So are there any reasons for the plumbing diff commands not to honor the diff.ignoreSubmodules setting?