On Fri, Jul 31, 2015 at 6:01 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> 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,
I agree there. The way you word it here, it sounds as if the mixture
of dashes and underscores are a problem.
> 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
Why would you use an underscore in here as opposed to a dash?
$ git submodule--helper module-list
I went with --module-list for now as I see no reason real to make it
a command word for now as it is not user facing but just a helper.
I have a patch from my previous attempt to rewrite "git submodule"
as a whole to accept both command words as well as double dashed
selected modes.
>
>> +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.
That's true, I thought about splitting it up later when I actually need it.
[That seems easier to write, but not easier to review :( ]
I did split up the function just now.
>
> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html