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

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

On 01/30, Junio C Hamano wrote:
> 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).

Yeah, this is much better, thanks.

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

I think this will work out nicely.  Will try to implement it this way.

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

-- 
Thomas