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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 21, 2017, 16:55 UTC
Message-ID: <xmqqmvdfh4az.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170219110313.24070-5-t.gummerer@gmail.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?

Show 5 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?

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