Re: [PATCH v3 2/5] stash: introduce push verb

[Thomas Gummerer <t.gummerer@gmail.com>]

[sorry for the late responses, life is keeping me busy]

On 02/06, Jeff King wrote:
> 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.

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

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