Re: [PATCH 3/3] stash: support filename argument

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 23, 2017, 18:50 UTC
Message-ID: <xmqqinp5sj98.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170121200804.19009-4-t.gummerer@gmail.com>

Thomas Gummerer <t.gummerer@gmail.com> writes:

Show 10 quoted lines

> diff --git a/git-stash.sh b/git-stash.sh
> index d6b4ae3290..7dcce629bd 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

Does $1 need to be quoted to prevent it from split at $IFS?

Show 17 quoted lines

> @@ -56,6 +56,23 @@ clear_stash () {
>  }
>  
>  create_stash () {
> +	files=
> +	while test $# != 0
> +	do
> +		case "$1" in
> +		--)
> +			shift
> +			break
> +			;;
> +		--files)
> +			;;
> +		*)
> +			files="$1 $files"
> +			;;

Hmph. What is this "no-op" option about? Did you mean to say something like this instead?

	case "$1" in
	...
	--file)
		case $# in
		1)
			die "--file needs a pathspec" ;;
		*)
			shift
			files="$files$1 " ;;
		esac
		;;

Another thing I noticed. We won't support filenames with embedded $IFS characters at all?

I somehow had an impression that the script was carefully done (e.g. by using -z option where appropriate) to add such a limitation.

Perhaps we have broken it over time and it no longer matters (i.e. there already may be existing breakages), but this troubles me somehow.

By the way, in addition to "push" thing that corrects the argument convention by requiring "-m" before the message, we need to correct create_stash that is used internally from "stash push" somehow?