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

[Junio C Hamano <gitster@pobox.com>]

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.