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

From: Thomas Gummerer <[email-masked]>
Date: Sun, 29 Jan 2017 13:29:26 +0000

On 01/23, Junio C Hamano wrote:
> Thomas Gummerer <[email-masked]> writes:
> 
> > 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?

Not sure, I'll check and add a test for the re-roll.

> > @@ -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
> 		;;
>

Hmm that would require multiple --file arguments to create_stash,
which I wanted to avoid.  But probably the correct solution is to
introduce a new format for create_stash, which allows a -m before the
message, and uses -- to disambiguate before the file name arguments.

This would be similar to what Johannes suggested in [1], deprecating
the old syntax in git stash create.  While this didn't work in git
stash save, it would work in git stash create, as "--" isn't used to
disambiguate anything there yet.

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

Good point, I didn't think about $IFS characters.  Fill fix in the
next round.

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

Yeah, I think that would make sense, see above.

[1]: http://public-inbox.org/git/alpine.DEB.2.20.1701241148300.3469@virtualbox/