From: Paul-Sebastian Ungureanu <ungureanupaulsebastian@gmail.com>
Date: Tue, 03 Apr 2018 21:38:35 GMT
Subject: Re: [RFC][PATCH] git-stash: convert git stash list to C builtin
Message-ID: <63772b7d-7b1e-2a3a-b12c-2dae9e254b68@gmail.com>
In-Reply-To: <CAPig+cS9QwCOG7BA7O5Nu_zsh-xTbDFy2vTWpAXxBuKTY-uzUw@mail.gmail.com>



On 25.03.2018 10:08, 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
> ("--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/

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!