Re: [RFC/PATCH 1/2] submodule: implement `module_list` as a builtin helper

> Stefan Beller <sbeller@google.com> writes:
>
>> +static const char * const git_submodule_helper_usage[] = {
>> +     N_("git submodule--helper --module_list [<path>...]"),
>
> Yuck.  Please do not force --multi_word_opt upon us, which is simply
> too ugly to live around here.  --module-list is perhaps OK,

> but
> because submodule--helper would not have an default action, I'd
> prefer to make these just "command words", i.e.
>
>     $ git submodule--helper module_list

>
>> +int module_list(int argc, const char **argv, const char *prefix)
>> +{
>> +     int i;
>> +     static struct pathspec pathspec;
>> +     const struct cache_entry **ce_entries = NULL;
>> +     int alloc = 0, used = 0;
>> +     char *ps_matched = NULL;
>> +     char *max_prefix;
>> +     int max_prefix_len;
>> +     struct string_list already_printed = STRING_LIST_INIT_NODUP;
>> +
>> +     parse_pathspec(&pathspec, 0,
>> +                    PATHSPEC_PREFER_FULL,
>> +                    prefix, argv);
>> +
>> +     /* Find common prefix for all pathspec's */
>> +     max_prefix = common_prefix(&pathspec);
>> +     max_prefix_len = max_prefix ? strlen(max_prefix) : 0;
>> +
>> +     if (pathspec.nr)
>> +             ps_matched = xcalloc(1, pathspec.nr);
>
> Up to this point it interprets its input, and ...
>
>> +     if (read_cache() < 0)
>> +             die("index file corrupt");
>> +
>> +     for (i = 0; i < active_nr; i++) {
>> +             const struct cache_entry *ce = active_cache[i];
>> +
>> +             if (!match_pathspec(&pathspec, ce->name, ce_namelen(ce),
>> +                                 max_prefix_len, ps_matched,
>> +                                 S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode)))
>> +                     continue;
>> +
>> +             if (S_ISGITLINK(ce->ce_mode)) {
>> +                     ALLOC_GROW(ce_entries, used + 1, alloc);
>> +                     ce_entries[used++] = ce;
>> +             }
>> +     }
>> +
>> +     if (ps_matched && report_path_error(ps_matched, &pathspec, prefix)) {
>> +             printf("#unmatched\n");
>> +             return 1;
>> +     }
>
> ... does the computation, with diagnosis.
>
> And then it does the I/O with formatting.
>
>> +
>> +     for (i = 0; i < used; i++) {
>> +             const struct cache_entry *ce = ce_entries[i];
> ...
>> +     return 0;
>> +}
>
> When you have the implementation of "foreach-parallel" to move the
> most expensive part of "submodule update" of a tree with 500
> submodules, you would want to receive more or less the same "args"
> as this thing takes and pass the ce_entries[] list to the "spawn and
> run the user script in them in parallel" engine.

>
> So I think it makes more sense to split this function into two (or
> three).  One that reads from (argc, argv) and allocates and fills
> ce_entries[] can become a helper that you can reuse later.
>
> 'int module_list()' (shouldn't it be static?), can make a call to
> that helper at the begining of it, and the remainder of the function
> would do the textual I/O.