On 02/05, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@xxxxxxxxx> 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
>
> ;-)
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.
> > @@ -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).
Will change.
> > + >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.