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

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

Stefan Beller <sbeller@google.com> writes:

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> new file mode 100644
> index 0000000..cb18ddf
> --- /dev/null
> +++ b/builtin/submodule--helper.c
> @@ -0,0 +1,111 @@
> + ...
> +static char *ps_matched;
> +static const struct cache_entry **ce_entries;
> +static int ce_alloc, ce_used;
> +static struct pathspec pathspec;
> +static const char *alternative_path;

These are OK for now to be global variables, but in the longer run,
I think you would need to introduce a struct or two that group the
relevant pieces and passed around the callchain.  For example, a
caller calling into module_list_compute() would want to pass a
pointer to a struct that has ce_entries[] and ps_matched to receive
the result.  pathspec and alternative_path would want to be a
function-scope auto variable in module_list, I would think.

> +static void module_list_compute(int argc, const char **argv,
> +				const char *prefix,
> +				struct pathspec *pathspec)
> +{
> +	int i;
> +	char *max_prefix;
> +	int max_prefix_len;
> +	parse_pathspec(pathspec, 0,
> +		       PATHSPEC_PREFER_FULL |
> +		       PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
> +		       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);
> +
> +
> +	if (read_cache() < 0)
> +		die("index file corrupt");

Again, this is OK for now, but I suspect you would eventually want
to return an error and have the caller react to it.

> +static int module_list(int argc, const char **argv, const char *prefix)
> +{
> +...
> +	for (i = 0; i < ce_used; i++) {
> +		const struct cache_entry *ce = ce_entries[i];
> +
> +		if (string_list_has_string(&already_printed, ce->name))
> +			continue;
> +
> +		if (ce_stage(ce)) {
> +			printf("%06o %s U\t", ce->ce_mode, sha1_to_hex(null_sha1));
> +		} else {
> +			printf("%06o %s %d\t", ce->ce_mode, sha1_to_hex(ce->sha1), ce_stage(ce));
> +		}
> +
> +		utf8_fprintf(stdout, "%s\n", ce->name);
> +
> +		string_list_insert(&already_printed, ce->name);

This looks a wasteful use of string-list.

When we iterate over the in-core index, or a subset obtained from
the in-core index without reordering, the standard technique to
handle entries for the same path only once is to handle one (while
remembering what it is) and then skip the ones that follow with the
same name, with a loop like this:

	i = 0;
        while (i < ce_used) {
        	ce = ce_entries[i++];
		use that ce;
                while (i < ce_used && !strcmp(ce->ce_name, ce_entries[i]->ce_name))
			i++; /* skip entries with the same name */
	}

Take advantage of the fact that the entries are still sorted.