Re: [PATCH v2 4/4] stash: support filename argument
Thomas Gummerer <t.gummerer@gmail.com> 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.
Show 7 quoted lines
> @@ -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.
Show 10 quoted lines
> 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 -- $1Hmph, 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...
Show 15 quoted lines
> @@ -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?
Show 6 quoted lines
> @@ -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.
Thomas Gummerer
