Re: [PATCH v3 5/5] stash: teach 'push' (and 'create') to honor pathspec

On 02/05, Junio C Hamano wrote:

Thomas Gummerer [off-list ref] writes:

quoted

@@ -72,6 +72,11 @@ create_stash () {
 			untracked="$1"
 			new_style=t
 			;;
+		--)
+			shift
+			new_style=t
+			break
+			;;
 		*)
 			if test -n "$new_style"
 			then

@@ -99,6 +104,10 @@ create_stash () {
 	if test -z "$new_style"
 	then
 		stash_msg="$*"
+		while test $# != 0
+		do
+			shift
+		done
 	fi

The intent is correct, but I would probaly empty the "$@" not with
an iteration of shifts but with a single

	set x && shift

;-)

Thanks, I knew there had to be a better way, but I was unable to find
it.  I think I like Andreas suggestion of "shift $#" the best here.

quoted

@@ -134,7 +143,7 @@ create_stash () {
 		# Untracked files are stored by themselves in a parentless commit, for
 		# ease of unpacking later.
 		u_commit=$(
-			untracked_files | (
+			untracked_files "$@" | (

The above (and all other uses of "$@" was exactly what I had in mind
when I gave you the "can we leave the '$@' the user gave us as-is?"
comment during the review of the previous round.  

Looks much nicer.

quoted

+test_expect_success 'stash push with $IFS character' '
+	>"foo	bar" &&

IIRC, a pathname with HT in it cannot be tested on MINGW.  For the
purpose of this test, I think it is sufficient to use SP instead of
HT here (and matching change below in the expectation, of course).

Will change.

quoted

+	>foo &&
+	>bar &&
+	git add foo* &&
+	git stash push --include-untracked -- foo* &&

While it is good that you are testing "foo*", you would also want
another test to cover a pathspec with $IFS in it.  That is the case
the code in the previous round had problems with.  Perhaps try
something like

	>foo && >bar && >"foo bar" && git add . &&
	echo modify foo >foo &&
	echo modify bar >bar &&
	echo modify "foo bar" >"foo bar" &&
	git stash push -- "foo b*"

which should result in the changes to "foo bar" in the resulting
stash, while changes to "foo" and "bar" are not (and left in the
working tree).  And make sure that is what happens?  I think with
the code from the previous round, the above would instead stash
changes to "foo" and "bar" without catching changes to "foo bar",
because the single pathspec "foo b*" would have been mistakenly
split into two, i.e. "foo" and "b*", and failed to match "foo bar"
while matching the other two.

Thanks, I'll add a test for that.