Re: [RFC][PATCH] git-stash: convert git stash list to C builtin
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Mar 25, 2018, 07:08 UTC
- Message-ID
- <CAPig+cS9QwCOG7BA7O5Nu_zsh-xTbDFy2vTWpAXxBuKTY-uzUw@mail.gmail.com>
- In-Reply-To
- <20180324182313.13705-1-ungureanupaulsebastian@gmail.com>
On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com> wrote:
Show 14 quoted lines
> 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/
Show 18 quoted lines
> + 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},