On 01/30, Junio C Hamano wrote:
> Thomas Gummerer <t.gummerer@xxxxxxxxx> writes:
>
> > Add an optional filename argument to git stash push, which allows for
> > stashing a single (or multiple) files.
>
> You can give pathspec with one or more elements, so "an optional
> argument" sounds too limiting.
>
> Allow 'git stash push' to take pathspec to specify which paths
> to stash.
>
> Also retitle
>
> stash: teach 'push' (and 'create') to honor pathspec
>
> or something.
>
> > @@ -56,6 +61,10 @@ save [-p|--patch] [-k|--[no-]keep-index] [-u|--include-untracked] [-a|--all] [-q
> > only <message> does not trigger this action to prevent a misspelled
> > subcommand from making an unwanted stash.
> > +
> > +If the paths argument is given in 'git stash push', only these files
> > +are put in the new 'stash'. In addition only the indicated files are
> > +changed in the working tree to match the index.
>
> Actually the stash contains "all paths". You could say that you are
> placing _modifications_ to these paths in stash, even though that is
> not how Git's world model works (i.e. everything is a snapshot, and
> modifications are merely difference between two successive
> snapshots). A technically correct version may be:
>
> When pathspec is given to 'git stash push', the new stash
> records the modified states only for the files that match
> the pathspec. The index entries and working tree files are
> then rolled back to the state in HEAD only for these files,
> too, leaving files that do not match the pathspec intact.
>
> > diff --git a/git-stash.sh b/git-stash.sh
> > index 5f08b43967..0072a38b4c 100755
> > --- a/git-stash.sh
> > +++ b/git-stash.sh
> > @@ -41,7 +41,7 @@ no_changes () {
> > untracked_files () {
> > excl_opt=--exclude-standard
> > test "$untracked" = "all" && excl_opt=
> > - git ls-files -o -z $excl_opt
> > + git ls-files -o -z $excl_opt -- $1
>
> Hmph, why "$1" is spelled without dq, implying that it is split at
> $IFS boundary? This line alone makes me suspect that this is not
> prepared to deal correctly with $IFS. Let's read on...
>
> > @@ -59,6 +59,7 @@ create_stash () {
> > stash_msg=
> > untracked=
> > new_style=
> > + files=
> > while test $# != 0
> > do
> > case "$1" in
> > @@ -72,6 +73,12 @@ create_stash () {
> > untracked="$1"
> > new_style=t
> > ;;
> > + --)
> > + shift
> > + files="$@"
>
> Isn't this the same as writing files="$*", i.e. concatenate the
> multiple arguments with the first whitespace in $IFS in between?
>
> > @@ -134,7 +141,7 @@ create_stash () {
> > # Untracked files are stored by themselves in a parentless commit, for
> > # ease of unpacking later.
> > u_commit=$(
> > - untracked_files | (
> > + untracked_files $files | (
>
> ... and this lets it split at $IFS again when passing it down to the
> helper. But the helper looks only at $1 so the second and subsequent
> ones will be ignored altogether.
>
> This cannot be correct, and any hunk in the remainder of the patch
> that mentions $files will be incorrect for the same reason.
>
> Is it possible to carry what the caller (and the end user) gave you
> in "$@" without molesting it at all? That would mean you do not
> need to introduce $files variable at all, and then the places that
> do things like this:
>
> > - create_stash -m "$stash_msg" -u "$untracked"
> > + create_stash -m "$stash_msg" -u "$untracked" -- $files
>
> can instead do
>
> create_stash -m "$stash_msg" -u "$untracked" -- "$@"
>
> That would allow you to work correctly with pathspec with $IFS
> whitespaces in them.
Thanks for taking the time for this explanation! It cleared quite a
few things up in my head. I'll make these fixes in the re-roll.
--
Thomas