Subject: Re: [PATCH v2 2/4] stash: introduce push verb

From: Junio C Hamano <[email-masked]>
Date: Mon, 30 Jan 2017 13:12:02 -0800

Thomas Gummerer <[email-masked]> writes:

> Introduce a new git stash push verb in addition to git stash save.  The
> push verb is used to transition from the current command line arguments
> to a more conventional way, in which the message is specified after a -m
> parameter instead of being a positional argument.

I think the canonical way to express that is "... the message is
given as an argument to the -m option" (i.e. some options take an
argument, some others do not, and the "-m" takes one).

> This allows introducing a new filename argument to stash single files.

I do not want them to be "a filename argument", and I do not think
you meant them as such, either.  

    This allows us to have pathspecs at the end of the command line
    arguments like other Git commands do, so that the user can say
    which subset of paths to stash (and leave others behind).

> +save_stash () {
> +	push_options=
> +	while test $# != 0
> +	do
> +		case "$1" in
> +		-k|--keep-index)
> +...
> +		esac
> +		shift
> +	done

It is a bit unfortunate that we need to duplicate the above
case/esac here.  I do not know if doing it this way:

	case "$1" in
	--)
		shift
		break 
		;;
	--help)
		show_help
		;;
	-*)
		# pass all options through to push_stash
		push_options="$push_options $1"
		;;
	*)
		break
                ;;
	esac

and letting push_stash complain for an unknown option is easier to
maintain.

You are reversing the order of the options in the loop.  Don't.