Re: [RFC][PATCH] git-stash: convert git stash list to C builtin

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

On 03/26, Johannes Schindelin wrote:
> Hi Eric,
> 
> On Sun, 25 Mar 2018, Eric Sunshine wrote:
> 
> > On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu
> > <ungureanupaulsebastian@gmail.com> wrote:
> > > Currently, because git stash is not fully converted to C, I
> > > introduced a new helper that will hold the converted commands.
> > > ---
> > > diff --git a/builtin/stash--helper.c b/builtin/stash--helper.c
> > > @@ -0,0 +1,52 @@
> > > +int cmd_stash__helper(int argc, const char **argv, const char *prefix)
> > > +{
> > > +       int cmdmode = 0;
> > > +
> > > +       struct option options[] = {
> > > +               OPT_CMDMODE(0, "list", &cmdmode,
> > > +                        N_("list stash entries"), LIST_STASH),
> > > +               OPT_END()
> > > +       };
> > 
> > Is the intention that once git-stash--helper implements all 'stash'
> > functionality, you will simply rename git-stash--helper to git-stash?
> > If so, then I'd think that you'd want it to accept subcommand
> > arguments as bare words ("apply", "drop") in order to be consistent
> > with the existing git-stash command set, not in dashed form
> 
> Why not start with cmdmode, and then add a single patch that *also*
> accepts argv[1] as a bare-word cmdmode?

I don't think we should accept the dashed form of the commands for
'git stash'.  The main reason being that we also have 'git stash'
without any arguments, which acts as 'git stash push'.  So if we would
ever come up with an argument to 'git stash push', that matches one of
the current verbs, or if we come up with a new verb that matches one
of the options to 'git stash push', that would not work.

In that case we could obviously go for a different word, but I think
the rules when 'git stash' is going to be 'git stash push' and when it
is not are already complicated enough, and allowing the verbs as
dashed options would only make the interface more complicated.

> This could even potentially be a patch to parse-options.[ch] that
> introduces, say, PARSE_OPT_ALLOW_BARE_CMDMODE.

Now if we'd take that one step further and make it
PARSE_OPT_BARE_CMDMODE, which would only allow the non-dashed options,
I think we could use that in other places in git as well (for example
in 'git worktree').

> Ciao,
> Dscho