Re: [PATCH 3/3] stash: support filename argument
- From
Thomas Gummerer <t.gummerer@gmail.com>
- Date
- Jan 29, 2017, 13:29 UTC
- Message-ID
- <20170129132926.GD31189@hank>
- In-Reply-To
- <xmqqinp5sj98.fsf@gitster.mtv.corp.google.com>
On 01/23, Junio C Hamano wrote:
Show 14 quoted lines
> Thomas Gummerer <t.gummerer@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.
Show 33 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
> ;;
>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.
Show 10 quoted lines
> 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/