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