Re: [PATCHv3 2/3] submodule: implement `module-name` as a builtin helper
- From
Stefan Beller <sbeller@google.com>
- Date
- Sep 1, 2015, 16:18 UTC
- Message-ID
- <CAGZ79kYAEic2kNmK10xB5g2JArpyqcqxNWjTBUkx6DfG3a_zpg@mail.gmail.com>
- In-Reply-To
- <CAPig+cTTiW_tZBNoz-kj6CkViR+MFegv1Tz5xmVvbGxZ0pbqwQ@mail.gmail.com>
On Mon, Aug 31, 2015 at 7:31 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 59 quoted lines
> On Mon, Aug 31, 2015 at 8:40 PM, Stefan Beller <sbeller@google.com> wrote:
>> This implements the helper `module_name` in C instead of shell,
>> yielding a nice performance boost.
>>
>> Before this patch, I measured a time (best out of three):
>>
>> $ time ./t7400-submodule-basic.sh >/dev/null
>> real 0m11.066s
>> user 0m3.348s
>> sys 0m8.534s
>>
>> With this patch applied I measured (also best out of three)
>>
>> $ time ./t7400-submodule-basic.sh >/dev/null
>> real 0m10.063s
>> user 0m3.044s
>> sys 0m7.487s
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index 44310f5..91bd420 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -102,16 +105,38 @@ static int module_list(int argc, const char **argv, const char *prefix)
>> +static int module_name(int argc, const char **argv, const char *prefix)
>> +{
>> + const struct submodule *sub;
>> +
>> + if (argc != 1)
>> + usage("git submodule--helper module_name <path>\n");
>
> usage(_("..."));
>
> s/module_name/module-name/
>
> I think you also need to drop the newline from the usage() argument.
>
>> + gitmodules_config();
>> + sub = submodule_from_path(null_sha1, argv[0]);
>> +
>> + if (!sub)
>> + die(N_("No submodule mapping found in .gitmodules for path '%s'"),
>> + argv[0]);
>> +
>> + printf("%s\n", sub->name);
>> +
>> + return 0;
>> +}
>> +
>> int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
>> {
>> if (argc < 2)
>> die(N_("fatal: submodule--helper subcommand must be called with "
>> - "a subcommand, which is module-list\n"));
>> + "a subcommand, which are module-list, module-name\n"));
>
> Manually maintaining this list is likely to become a maintenance issue
> pretty quickly. Isn't there an OPT_CMDMODE for this sort of thing?If we were using OPT_CMDMODE, we'd need to have all the options in one array, passing it to the option parser. I don't think we could have 2 layers of option parsing here. If the first layer only contains the check for argv[1] (list, name, clone), then it would fail not knowing the arguments for each of these sub commands.
If we parse all the options together, then we would need to have lots of
if (cmd == name)
die ("it makes no sense to give --reference or --depth");also the variables where the options are passed into would not be function specific, but file global.
Show 18 quoted lines
>
> Also, it would like be more readable if the possible commands were
> printed one per line rather than all squashed together.
>
>> if (!strcmp(argv[1], "module-list"))
>> return module_list(argc - 1, argv + 1, prefix);
>>
>> + if (!strcmp(argv[1], "module-name"))
>> + return module_name(argc - 2, argv + 2, prefix);
>> +
>> die(N_("fatal: '%s' is not a valid submodule--helper subcommand, "
>> - "which is module-list\n"),
>> + "which are module-list, module-name\n"),
>
> And, it's duplicated here, making it even more of a maintenance problem.
>
>> argv[1]);
>> }Maybe we can do it similar to git.c to have our own array with name and function pointers, which can be used both in the help text as well as the actual function call.