Re: [PATCHv4 1/3] submodule: implement `list` as a builtin helper

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

Stefan Beller <sbeller@google.com> writes:

> +static int module_list_compute(int argc, const char **argv,
> +				const char *prefix,
> +				struct pathspec *pathspec)
> +{
> +	int i, result = 0;
> +	char *max_prefix, *ps_matched = NULL;
> +	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(pathspec->nr, 1);
> +
> +	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, ce_used + 1, ce_alloc);
> +			ce_entries[ce_used++] = ce;
> +		}
> +
> +		while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name))
> +			/*
> +			 * Skip entries with the same name in different stages
> +			 * to make sure an entry is returned only once.
> +			 */
> +			i++;
> +	}

I hate myself not catching this earlier, but I suspect that this is
not quite a faithful rewrite of the original.  It changes behaviour
when a conflicted path records a non submodule in stage #1 and a
submodule in stage #2 (or stage #3), doesn't it?

The original scripted version will see the stage #1 entry and skips
it because it is not a submodule, then will see the stage #2 entry
and because the path is not yet in the %unmerged hash, and it will
push it to @out.

This loop instead sees a non-submodule entry at stage #1, skips it
(because it is not a submodule), then goes on to skip the entries
with the same name, including the stage #2 entry that _is_ a
submodule.

I think the clever "we are done with this entry, so let's skip all
others that have the same name" optimization should be done only
when we did handle an entry with the same name.  I.e. something like
this squashed in.
	
--------------------------------------------------

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 16d9abe..c4aff0c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -39,12 +39,13 @@ static int module_list_compute(int argc, const char **argv,
 				    S_ISGITLINK(ce->ce_mode) | S_ISDIR(ce->ce_mode)))
 			continue;
 
-		if (S_ISGITLINK(ce->ce_mode)) {
-			ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);
-			ce_entries[ce_used++] = ce;
-		}
+		if (!S_ISGITLINK(ce->ce_mode))
+			continue;
 
-		while (i + 1 < active_nr && !strcmp(ce->name, active_cache[i + 1]->name))
+		ALLOC_GROW(ce_entries, ce_used + 1, ce_alloc);
+		ce_entries[ce_used++] = ce;
+		while (i + 1 < active_nr &&
+		       !strcmp(ce->name, active_cache[i + 1]->name))
 			/*
 			 * Skip entries with the same name in different stages
 			 * to make sure an entry is returned only once.

--------------------------------------------------

> +static int module_list(int argc, const char **argv, const char *prefix)
> +{
> +	int i;
> +	struct pathspec pathspec;
> +	const char *alternative_path;
> +
> +	struct option module_list_options[] = {
> +		OPT_STRING(0, "prefix", &alternative_path,
> +			   N_("path"),
> +			   N_("alternative anchor for relative paths")),
> +		OPT_END()
> +	};

Do we really need this alternative_path variable?  The "--prefix"
option that overrides the passed-in variable prefix would be easier
to understand if it touched the same variable, i.e.

--------------------------------------------------

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 16d9abe..387539f 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -65,10 +66,9 @@ static int module_list(int argc, const char **argv, const char *prefix)
 {
 	int i;
 	struct pathspec pathspec;
-	const char *alternative_path;
 
 	struct option module_list_options[] = {
-		OPT_STRING(0, "prefix", &alternative_path,
+		OPT_STRING(0, "prefix", &prefix,
 			   N_("path"),
 			   N_("alternative anchor for relative paths")),
 		OPT_END()
@@ -82,9 +82,7 @@ static int module_list(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix, module_list_options,
 			     git_submodule_helper_usage, 0);
 
-	if (module_list_compute(argc, argv, alternative_path
-					    ? alternative_path
-					    : prefix, &pathspec) < 0) {
+	if (module_list_compute(argc, argv, prefix, &pathspec) < 0) {
 		printf("#unmatched\n");
 		return 1;
 	}

--------------------------------------------------