Re: [PATCHv4 1/3] submodule: implement `list` as a builtin helper
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Sep 1, 2015, 19:55 UTC
- Message-ID
- <xmqqtwrdrk1k.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <1441131869-29474-2-git-send-email-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 42 quoted lines
> +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; } --------------------------------------------------