On 01/23, Junio C Hamano wrote:
> Thomas Gummerer <t.gumme...@gmail.com> 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/

Reply via email to