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

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

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?

> +			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?


> +			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