Re: [PATCH v6 4/6] stash: teach 'push' (and 'create_stash') to honor pathspec

From: Thomas Gummerer <t.gummerer@gmail.com>
Date: Feb 25, 2017, 20:27 UTC
Message-ID: <20170225202733.GB6243@hank>
In-Reply-To: <xmqqmvdfh4az.fsf@gitster.mtv.corp.google.com>

On 02/21, Junio C Hamano wrote:

Show 17 quoted lines

> Thomas Gummerer <t.gummerer@gmail.com> writes:
> 
> > -		git reset --hard ${GIT_QUIET:+-q}
> 
> This hunk is probably the most important one to review in the whole
> series, in the sense that these are entirely new code that didn't
> exist in the original.
> 
> > +		if test $# != 0
> > +		then
> > +			saved_untracked=
> > +			if test -n "$(git ls-files --others -- "$@")"
> 
> I notice that "ls-files -o" used in the code before this series are
> almost always used with --exclude-standard but we do not set up the
> standard exclude pattern here.  Is there a good reason to use (or
> not to use) it here as well?

We probably should use it, not adding it was an oversight.

Show 28 quoted lines

> > +			then
> > +				saved_untracked=$(
> > +					git ls-files -z --others -- "$@" |
> > +					    xargs -0 git stash create -u all --)
> > +			fi
> 
> Running the same ls-files twice look somewhat wasteful.
> 
> I suspect that we avoid "xargs -0" in our code from portability
> concern (isn't it a GNU invention?)
> 
> > +			git ls-files -z -- "$@" | xargs -0 git reset ${GIT_QUIET:+-q} --
> 
> Hmm, am I being naive to suspect that the above is a roundabout way
> to say:
> 
> 	git reset ${GIT_QUIET:+-q} -- "$@"
> 
> or is an effect quite different from that intended here?
> 
> > +			git ls-files -z --modified -- "$@" | xargs -0 git checkout ${GIT_QUIET:+-q} HEAD --
> 
> Likewise.  Wouldn't the above be equivalent to:
> 
> 	git checkout ${GIT_QUIET:+-q} HEAD -- "$@"
> 
> Or is this trying to preserve paths modified in the working tree and
> fully added to the index?.

No, it's not trying to do that (all the paths we're touching here would have been "reset" earlier, so it wouldn't change anything. However what this code tried to do is to allow "stash push -- path pathspec-not-in-repo", where "pathspec-not-in-repo" would end up tripping up "checkout" which does not accept pathspecs that are not in the index.

I think we should disallow such pathspecs in stash as well, except in the --include-untracked case, where it still makes sense.

This means we can't get rid of the "ls-files --modified" here, but we can in all other places, and get rid of the "stash_create" "stash_apply" dance to store the untracked files, simplifying this part quite a bit.

Will re-roll.

Show 26 quoted lines

> 
> > +			if test -n "$(git ls-files -z --others -- "$@")"
> > +			then
> > +				git ls-files -z --others -- "$@" | xargs -0 git clean --force -d ${GIT_QUIET:+-q} --
> 
> Likewise.  "ls-files --others" being the major part of what "clean"
> is about, I suspect the "ls-files piped to clean" is redundant.  Do
> you even need a test?  IOW, doesn't "git clean" with a pathspec that
> does not match anything silently succeed without doing anything
> harmful?
>
> > +			fi
> > +			if test -n "$saved_untracked"
> > +			then
> > +				git stash pop -q $saved_untracked
> 
> I see this thing was "created", and the whole point of "create" is
> to be different from "save/push" that automatically adds the result
> to the stash reflog.  Should we be "pop"ing it, or did you mean to
> just call apply_stash on it?
>
> > +			fi
> > +		else
> > +			git reset --hard ${GIT_QUIET:+-q}
> > +		fi
>