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

> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > @@ -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
> 
> ;-)

> > @@ -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.
> 
> > +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).

> > +	>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.