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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 7, 2015, 19:53 UTC
Message-ID: <xmqqwpx6yixf.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1438733070-15805-1-git-send-email-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 12 quoted lines

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

Show 22 quoted lines

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

Show 18 quoted lines

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