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

[Eric Sunshine <sunshine@sunshineco.com>]

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
("--apply", "--drop"). In that case, OPT_CMDMODE doesn't seem
appropriate. Instead, you should be consulting argv[] directly (as in
[1]) after parse_options().

[1]: https://public-inbox.org/git/20180324173707.17699-2-joel@teichroeb.net/

> +       argc = parse_options(argc, argv, prefix, options,
> +                            git_stash__helper_usage, PARSE_OPT_KEEP_UNKNOWN);
> +
> +       if (!cmdmode)
> +               usage_with_options(git_stash__helper_usage, options);
> +
> +       switch (cmdmode) {
> +               case LIST_STASH:
> +                       return list_stash(argc, argv, prefix);
> +       }
> +       return 0;
> +}
> diff --git a/git.c b/git.c
> @@ -466,6 +466,7 @@ static struct cmd_struct commands[] = {
>         { "show-branch", cmd_show_branch, RUN_SETUP },
>         { "show-ref", cmd_show_ref, RUN_SETUP },
>         { "stage", cmd_add, RUN_SETUP | NEED_WORK_TREE },
> +       { "stash--helper", cmd_stash__helper, RUN_SETUP },

You don't require a working tree? Seems odd for git-stash.

>         { "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
>         { "stripspace", cmd_stripspace },
>         { "submodule--helper", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},