Re: [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
Thomas Gummerer writes:
> if test -z "$patch_mode"
> then
> - git reset --hard ${GIT_QUIET:+-q}
> + if test $# != 0
> + then
> + git reset ${GIT_QUIET:+-q} -- "$@"
> + git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z
> --modified "$@")
"ls-files -z" on the command line?
Apparently new tests do not cover the correctness of this codepath.
I wonder if this
git ls-files -z --modified "$@" |
git checkout-index -z --force --stdin
is what the above "checkout" wanted to do. The "reset" in the
previous step presumably updated the index entries that match
specified pathspec to those of the HEAD, so checking out the paths
that match "$@" from the index would be the same as checking them
out from the HEAD (while updating the index with them).
Perhaps squash the following into an appropriate patch in the
series?
git-stash.sh | 3 ++-
t/t3903-stash.sh | 16
2 files changed, 18 insertions(+), 1 deletion(-)
diff --git a/git-stash.sh b/git-stash.sh
index 28d0624c75..9c70662cc8 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -300,7 +300,8 @@ push_stash () {
if test $# != 0
then
git reset ${GIT_QUIET:+-q} -- "$@"
- git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z
--modified "$@")
+ git ls-files -z --modified -- "$@" |
+ git checkout-index -z --force --stdin
git clean --force ${GIT_QUIET:+-q} -d -- "$@"
else
git reset --hard ${GIT_QUIET:+-q}
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index f7733b4dd4..e868aafab2 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -891,4 +891,20 @@ test_expect_success 'stash without verb with pathspec' '
test_path_is_file bar
'
+test_expect_success 'stash with pathspec matching multiple paths' '
+ echo original >file &&
+ echo original >other-file &&
+ git commit -m "two" file other-file &&
+ echo modified >file &&
+ echo modified >other-file &&
+ git stash -- "*file" &&
+ echo original >expect &&
+ test_cmp expect file &&
+ test_cmp expect other-file &&
+ git stash pop &&
+ echo modified >expect &&
+ test_cmp expect file &&
+ test_cmp expect other-file
+'
+
test_done
Re: [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
Thomas Gummerer writes: > On 02/27, Junio C Hamano wrote: >> Thomas Gummerer writes: >> >> > + test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null >> > || exit 1 >> >> This silent "exit 1" made me scratch my head, but --error-unmatch >> would have already given an error message, like >> >> error: pathspec 'no such' did not match any file(s) known to git. >> Did you forget to 'git add'? >> >> so that would be OK. > > Yeah exactly, the plan was to let --error-unmatch print the error > message, as it's better at giving a good error message than we could > easily do here I think. > > Maybe this deserves a comment so others won't have the same doubts > about this line? Nah, I do not think so. It probably is obvious for those who write (and read) "--error-unmatch". I was just being slow to realize that the sole point of that option was to complain ;-)
Re: [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
Thomas Gummerer writes:
> if test -z "$patch_mode"
> then
> - git reset --hard ${GIT_QUIET:+-q}
> + if test $# != 0
> + then
> + git reset ${GIT_QUIET:+-q} -- "$@"
> + git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z
> --modified "$@")
"ls-files -z" on the command line?
Apparently new tests do not cover the correctness of this codepath.
I wonder if this
git ls-files -z --modified "$@" |
git checkout-index -z --stdin
is what the above "checkout" wanted to do. The "reset" in the
previous step presumably updated the index entries that match
specified pathspec to those of the HEAD, so checking out the paths
that match "$@" from the index would be the same as checking them
out from the HEAD (while updating the index with them).
> + git clean --force ${GIT_QUIET:+-q} -d -- "$@"
> + else
> + git reset --hard ${GIT_QUIET:+-q}
> + fi
Thanks.
Re: [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
On 02/27, Junio C Hamano wrote: > Thomas Gummerer writes: > > > + test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null > > || exit 1 > > This silent "exit 1" made me scratch my head, but --error-unmatch > would have already given an error message, like > > error: pathspec 'no such' did not match any file(s) known to git. > Did you forget to 'git add'? > > so that would be OK. Yeah exactly, the plan was to let --error-unmatch print the error message, as it's better at giving a good error message than we could easily do here I think. Maybe this deserves a comment so others won't have the same doubts about this line?
Re: [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
On 02/27, Junio C Hamano wrote:
> Thomas Gummerer writes:
>
> > if test -z "$patch_mode"
> > then
> > - git reset --hard ${GIT_QUIET:+-q}
> > + if test $# != 0
> > + then
> > + git reset ${GIT_QUIET:+-q} -- "$@"
> > + git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z
> > --modified "$@")
>
> "ls-files -z" on the command line?
>
> Apparently new tests do not cover the correctness of this codepath.
>
> I wonder if this
>
> git ls-files -z --modified "$@" |
> git checkout-index -z --force --stdin
>
> is what the above "checkout" wanted to do. The "reset" in the
> previous step presumably updated the index entries that match
> specified pathspec to those of the HEAD, so checking out the paths
> that match "$@" from the index would be the same as checking them
> out from the HEAD (while updating the index with them).
Yes, you're completely right, this is exactly what it should have
done. Sorry for being slow on getting this right.
> Perhaps squash the following into an appropriate patch in the
> series?
Thanks, I'll squash this in and re-roll.
> git-stash.sh | 3 ++-
> t/t3903-stash.sh | 16
> 2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 28d0624c75..9c70662cc8 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -300,7 +300,8 @@ push_stash () {
> if test $# != 0
> then
> git reset ${GIT_QUIET:+-q} -- "$@"
> - git checkout ${GIT_QUIET:+-q} HEAD -- $(git ls-files -z
> --modified "$@")
> + git ls-files -z --modified -- "$@" |
> + git checkout-index -z --force --stdin
> git clean --force ${GIT_QUIET:+-q} -d -- "$@"
> else
> git reset --hard ${GIT_QUIET:+-q}
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index f7733b4dd4..e868aafab2 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -891,4 +891,20 @@ test_expect_success 'stash without verb with pathspec' '
> test_path_is_file bar
> '
>
> +test_expect_success 'stash with pathspec matching multiple paths' '
> + echo original >file &&
> + echo original >other-file &&
> + git commit -m "two" file other-file &&
> + echo modified >file &&
> + echo modified >other-file &&
> + git stash -- "*file" &&
> + echo original >expect &&
> + test_cmp expect file &&
> + test_cmp expect other-file &&
> + git stash pop &&
> + echo modified >expect &&
> + test_cmp expect file &&
> + test_cmp expect other-file
> +'
> +
> test_done
Re: [PATCH v7 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec
Thomas Gummerer writes: > + test -n "$untracked" || git ls-files --error-unmatch -- "$@" >/dev/null > || exit 1 This silent "exit 1" made me scratch my head, but --error-unmatch would have already given an error message, like error: pathspec 'no such' did not match any file(s) known to git. Did you forget to 'git add'? so that would be OK.
