On 25.03.2018 10:08, Eric Sunshine wrote:
On Sat, Mar 24, 2018 at 2:23 PM, Paul-Sebastian Ungureanu <ungureanupaulsebastian@xxxxxxxxx> 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@xxxxxxxxxxxxx/
It makes sense. In the end, when all stash is converted, it would just require an additional pointless effort to bring (back) from dashed form to bare word form.
+ 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},
For now, I do not think that it is necessary (for stash list), but I am pretty sure that it will be required in the future when porting other commands.
Thanks for advice!
