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