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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 1, 2015, 01:01 UTC
Message-ID: <xmqqmvybakjl.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1438384147-3275-2-git-send-email-sbeller@google.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

Show 21 quoted lines

> +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 ...

Show 21 quoted lines

> +	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.