Re: [PATCH v2 4/4] stash: support filename argument

[Junio C Hamano <gitster@pobox.com>]

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.

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