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

Junio C Hamano [off-list ref] writes:

quoted

Setting 'stash.index' config option changes 'git-stash pop|apply' to behave
as if '--index' switch is always supplied.

One thing I forgot to say.  "stash.index" invites "index _what_?"
Naming it to "stash.useIndex" may avoid such reaction.

Also, the current code has this comment:

    #   INDEX_OPTION is set to --index if --index is specified.

but it probably makes sense to change it (in the first patch in the series
that adds --no-index support) to a boolean whose value can be either true
or empty.

The reason why the very original code used INDEX_OPTION=--index may be
because it did something like "git some-cmd $INDEX_OPTION", but that is
not what the current code does, and using "either '--index' or ''" as a
form of boolean is confusing.

In other words, something like....



 git-stash.sh |   37 ++++++++++++++++++++-----------------
 1 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/git-stash.sh b/git-stash.sh
index 0a94036..eed2d1e 100755
--- a/git-stash.sh
+++ b/git-stash.sh

@@ -239,7 +239,7 @@ show_stash () {
 #   i_tree is set to the index tree
 #
 #   GIT_QUIET is set to t if -q is specified
-#   INDEX_OPTION is set to --index if --index is specified.
+#   INDEX_OPTION is set to 'true' if applying/popping also to the index.
 #   FLAGS is set to the remaining flags
 #
 # dies if:

@@ -271,14 +271,17 @@ parse_flags_and_rev()
 	for opt
 	do
 		case "$opt" in
-			-q|--quiet)
-				GIT_QUIET=-t
+		-q|--quiet)
+			GIT_QUIET=-t
+			;;
+		--index)
+			INDEX_OPTION=true
 			;;
-			--index)
-				INDEX_OPTION=--index
+		--no-index)
+			INDEX_OPTION=
 			;;
-			-*)
-				FLAGS="${FLAGS}${FLAGS:+ }$opt"
+		-*)
+			FLAGS="${FLAGS}${FLAGS:+ }$opt"
 			;;
 		esac
 	done

@@ -286,15 +289,15 @@ parse_flags_and_rev()
 	set -- $REV
 
 	case $# in
-		0)
-			have_stash || die "No stash found."
-			set -- ${ref_stash}@{0}
+	0)
+		have_stash || die "No stash found."
+		set -- ${ref_stash}@{0}
 		;;
-		1)
-			:
+	1)
+		:
 		;;
-		*)
-			die "Too many revisions specified: $REV"
+	*)
+		die "Too many revisions specified: $REV"
 		;;
 	esac
 

@@ -342,8 +345,8 @@ apply_stash () {
 		die 'Cannot apply a stash in the middle of a merge'
 
 	unstashed_index_tree=
-	if test -n "$INDEX_OPTION" && test "$b_tree" != "$i_tree" &&
-			test "$c_tree" != "$i_tree"
+	if test true = "$INDEX_OPTION" &&
+		test "$b_tree" != "$i_tree" && test "$c_tree" != "$i_tree"
 	then
 		git diff-tree --binary $s^2^..$s^2 | git apply --cached
 		test $? -ne 0 &&

@@ -387,7 +390,7 @@ apply_stash () {
 	else
 		# Merge conflict; keep the exit status from merge-recursive
 		status=$?
-		if test -n "$INDEX_OPTION"
+		if test true = "$INDEX_OPTION"
 		then
 			echo >&2 'Index was not unstashed.'
 		fi

On May 11, 2011, at 17.26 , Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

quoted

Setting 'stash.index' config option changes 'git-stash pop|apply'  
to behave
as if '--index' switch is always supplied.

One thing I forgot to say.  "stash.index" invites "index _what_?"
Naming it to "stash.useIndex" may avoid such reaction.

No objection.  I want the feature (scratching my own itch here), but I  
don't really care what it's called. :)

Also, the current code has this comment:

   #   INDEX_OPTION is set to --index if --index is specified.

but it probably makes sense to change it (in the first patch in the  
series
that adds --no-index support) to a boolean whose value can be either  
true
or empty.

It seemed a little wonky to me also, but this is my first foray into  
hacking on git and was concerned someone was depending on this,  
strange though it may be. git-blame fingers ef763129d for this oddity,  
dating August 2010.

The reason why the very original code used INDEX_OPTION=--index may be
because it did something like "git some-cmd $INDEX_OPTION", but that  
is
not what the current code does, and using "either '--index' or ''"  
as a
form of boolean is confusing.

I agree.  I like your change, also.
Does this feature make sense to you overall?

<SNIP>

Thanks,
David