Re: [PATCH v3 2/5] stash: introduce push verb
flat view
From: Jeff King <hidden>
Date: 2017-02-06 15:46:35
On Sun, Feb 05, 2017 at 08:26:39PM +0000, Thomas Gummerer wrote:
quoted hunk ↗ jump to hunk
+ -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).
quoted hunk ↗ jump to hunk
+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).
quoted hunk ↗ jump to hunk
+ 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