Re: [PATCH v3 2/5] stash: introduce push verb
[sorry for the late responses, life is keeping me busy]
On 02/06, Jeff King wrote:
Show 16 quoted lines
> On Sun, Feb 05, 2017 at 08:26:39PM +0000, Thomas Gummerer wrote:
>
> > + -m|--message)
> > + shift
> > + stash_msg=${1?"-m needs an argument"}
> > + ;;
>
> I think this is our first use of the "?" parameter expansion magic. It
> _is_ in POSIX, so it may be fine. We may get complaints from people on
> weird shell variants, though. If that's the only reason to avoid it, I'd
> be inclined to try it and see, as it is much shorter.
>
> OTOH, most of the other usage errors call usage(), and this one doesn't.
> Nor is the error message translatable. Perhaps we should stick to the
> longer form (and add a helper function if necessary to reduce the
> boilerplate).Yeah I do agree that calling usage is the better option here.
Show 28 quoted lines
> > +save_stash () {
> > + push_options=
> > + while test $# != 0
> > + do
> > + case "$1" in
> > + --help)
> > + show_help
> > + ;;
> > + --)
> > + shift
> > + break
> > + ;;
> > + -*)
> > + # pass all options through to push_stash
> > + push_options="$push_options $1"
> > + ;;
> > + *)
> > + break
> > + ;;
> > + esac
> > + shift
> > + done
>
> I suspect you could just let "--help" get handled in the pass-through
> case (it generally takes precedence over errors found in other options,
> but I do not see any other parsing errors that could be found by this
> loop). It is not too bad to keep it, though (the important thing is that
> we're not duplicating all of the push_stash options here).Good point, would be good to get rid of that duplication as well.
Show 35 quoted lines
> > + if test -z "$stash_msg"
> > + then
> > + push_stash $push_options
> > + else
> > + push_stash $push_options -m "$stash_msg"
> > + fi
>
> Hmm. So $push_options is subject to word-splitting here. That's
> necessary to split the options back apart. It does the wrong thing if
> any of the options had spaces in them. But I don't think there are any
> valid options which do so, and "save" would presumably not grow any new
> options (they would go straight to "push").
>
> So there is a detectable behavior change:
>
> [before]
> $ git stash "--bogus option"
> error: unknown option for 'stash save': --bogus option
> To provide a message, use git stash save -- '--bogus option'
> [etc...]
>
> [after]
> $ git stash "--bogus option"
> error: unknown option for 'stash save': --bogus
> To provide a message, use git stash save -- '--bogus'
>
> but it's probably an acceptable casualty (the "right" way would be to
> shell-quote everything you stuff into $push_options and then eval the
> result when you invoke push_stash).
>
> Likewise, it's usually a mistake to just stick a new option (like "-m")
> after a list of unknown options. But it's OK here because we know we
> removed any "--" or non-option arguments.
>
> -Peff
--
Thomas
Thomas Gummerer <t.gummerer@gmail.com>