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

[Thomas Gummerer <t.gummerer@gmail.com>]

On 01/30, Junio C Hamano wrote:
> 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.

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