Re: [PATCH] Adds 'stash.index' configuration option

From: Junio C Hamano <gitster@pobox.com>
Date: May 11, 2011, 23:46 UTC
Message-ID: <7vfwoker7i.fsf@alter.siamese.dyndns.org>
In-Reply-To: <D80C1130-8DE6-457E-B203-FCF25B8ED72C@gmail.com>

David Pisoni <dpisoni@gmail.com> writes:

Show 7 quoted lines

> Setting 'stash.index' config option changes 'git-stash pop|apply' to
> behave
> as if '--index' switch is always supplied.
> 'git-stash pop|apply' provides a --no-index switch to circumvent
> config default.
>
> Signed-off-by: David Pisoni <dpisoni@gmail.com>

Your MUA utterly mangled your whitespaces and linebreaks. Please be careful when you send a re-rolled series of this patch. The section "MUA Specific hints" of

  http://www.kernel.org/pub/software/scm/git/docs/git-format-patch.html

may be of help.

Show 14 quoted lines

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 285c7f7..d794c40 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1746,6 +1746,11 @@ showbranch.default::
> 	The default set of branches for linkgit:git-show-branch[1].
> 	See linkgit:git-show-branch[1].
>
> +stash.index::
> +    A boolean to make linkgit:git-stash[1] default to the behavior of
> --index
> +	when applying a stash to the working copy.  Can be
> circumvented by using
> +	--no-index switch to linkgit:git-stash[1].  Defaults to false.

This says "boolean", so all of these should mean "I do not want the command to default to --index":

	[stash]
                index = false
                index = 0

and all of these mean "I do want the default --index":

	[stash]
		index
                index = yes
                index = 1

I however do not think your implementation actually handle these correctly.

See below for one possible correct implementation, also the comment on the necessity to test both sides of the coin.

Show 10 quoted lines

> diff --git a/Documentation/git-stash.txt b/Documentation/git-stash.txt
> index 15f051f..de086ee 100644
> --- a/Documentation/git-stash.txt
> +++ b/Documentation/git-stash.txt
> @@ -11,7 +11,7 @@ SYNOPSIS
> 'git stash' list [<options>]
> 'git stash' show [<stash>]
> 'git stash' drop [-q|--quiet] [<stash>]
> -'git stash' ( pop | apply ) [--index] [-q|--quiet] [<stash>]
> +'git stash' ( pop | apply ) [--[no-]index] [-q|--quiet] [<stash>]

The --no-index is a very welcome addition, even if we did not have this new configuration variable. An alias "git pop" defined thusly:

	[alias]
        	pop = stash pop --index

can countermand it on demand with "git pop --no-index".

Please make it a separate patch, that comes before the addition of the configuration variable. And then another patch to add the configuration variable on top of it.

Show 9 quoted lines

> diff --git a/git-stash.sh b/git-stash.sh
> index 0a94036..7711bf6 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -31,6 +31,8 @@ else
>        reset_color=
> fi
>
> +CONFIG_INDEX="$(git config stash.index)"

This needs to be something like

	CONFIG_INDEX=$(git config --bool stash.index)
	
Then you would check the value against "true" later like this:

Show 6 quoted lines

> @@ -256,7 +258,7 @@ parse_flags_and_rev()
>
> 	IS_STASH_LIKE=
> 	IS_STASH_REF=
> -	INDEX_OPTION=
> +	INDEX_OPTION=$CONFIG_INDEX

	if test "$CONFIG_INDEX" = true
	then
		INDEX_OPTION=--index
	else
        	INDEX_OPTION=
	fi

Show 7 quoted lines

> @@ -277,6 +279,9 @@ parse_flags_and_rev()
> 			--index)
> 				INDEX_OPTION=--index
> 			;;
> +			--no-index)
> +				unset INDEX_OPTION
> +			;;

I'd rather sees this done as

			--no-index)
				INDEX_OPTION=
				;;

as a later part of the existing code says

	if test -n "$INDEX_OPTION" && ...

IOW, the code switches on the value of the variable, not on whether if it is set or unset.

Show 14 quoted lines

> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 5c72540..4999682 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -84,6 +84,34 @@ test_expect_success 'apply stashed changes
> (including index)' '
> 	test 1 = $(git show HEAD:file)
> '
>
> +test_expect_success 'apply stashed changes (including index) with
> index config set' '
> +    git config stash.index yes &&
> +	git reset --hard HEAD^ &&
> +	echo 7 > other-file &&

We write redirect from/to without an extra space, like you did in the next test to echo 8 into the same file.

Show 9 quoted lines

> +	git add other-file &&
> +	test_tick &&
> +	git commit -m other-file &&
> +	git stash apply &&
> +	test 3 = $(cat file) &&
> +	test 2 = $(git show :file) &&
> +	test 1 = $(git show HEAD:file) &&
> +	git config --unset stash.index
> +'

Use test_when_finished early to arrange so that this unset will happen even when a step somewhere in the test fails.

It is a common mistake to write tests that only show off a shiny new toy, making sure the feature kicks in when it should, and totally forget to make sure the feature does not kick in when it should not. Always remeber to test both sides of the coin.

Check what should happen with at least these combinations:

        stash.index set to...           command line says...
 ----------------------------------------------------------------
        (not set at all)                (nothing)
        (not set at all)                --index
 *      (not set at all)                --no-index
 *      (not set at all)                --index --no-index
 *      (not set at all)                --no-index --index
 *      false                           --index
 *      false                           --no-index
 *      true                            --index
 *      true                            --no-index
 ----------------------------------------------------------------

The cases marked with "*" are the possibilities your patch opens and need to have tests so that they are not broken when other people later change the code (e.g. the command line parsing may break "later one wins" rule if done carelessly).

Thanks.