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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 30, 2017, 21:12 UTC
Message-ID: <xmqq8tpsuual.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170129201604.30445-3-t.gummerer@gmail.com>

Thomas Gummerer <t.gummerer@gmail.com> 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).

Show 10 quoted lines

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