Re: [GSoC][RFC/PATCH v2] submodule: port subcommand foreach from shell to C
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 24, 2017, 02:24 UTC
- Message-ID
- <xmqqfugy35lv.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20170422195804.18477-1-pc44800@gmail.com>
> +static void foreach_submodule(int argc, const char **argv, const char *path,
> + const unsigned char *sha1, const char *prefix,
> + int quiet, int recursive)
> +{I think that a reader would expect that a function whose name is foreach_something() to iterate over some collection and do something on each of them, but this is not. It is "do something on one thing" part in a loop that exists elsewhere.
Do not call such a helper "foreach_<something>".
Would it make more sense to use the "struct object_id" (instead of uchar[40]) interface for new functions like this?
Show 16 quoted lines
> + const char *toplevel = xgetcwd();
> + const struct submodule *sub;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + struct strbuf sb = STRBUF_INIT;
> + struct strbuf sub_sha1 = STRBUF_INIT;
> + struct strbuf file = STRBUF_INIT;
> + char* displaypath = NULL;
> + int i;
> +
> + /* Only loads from .gitmodules, no overlay with .git/config */
> + gitmodules_config();
> +
> + if (prefix && get_super_prefix()) {
> + die("BUG: cannot have prefix and superprefix");
> + } else if (prefix) {
> + displaypath = xstrdup(relative_path(path, prefix, &sb));An extra SP after the last comma?
Show 13 quoted lines
> + } else if (get_super_prefix()) {
> + strbuf_addf(&sb, "%s/%s", get_super_prefix(), path);
> + displaypath = strbuf_detach(&sb, NULL);
> + } else {
> + displaypath = xstrdup(path);
> + }
> +
> + sub = submodule_from_path(null_sha1, path);
> +
> + if (!sub)
> + die(_("No url found for submodule path '%s' in .gitmodules"),
> + displaypath);
> + strbuf_add_unique_abbrev(&sub_sha1, sha1 , 40);Why add-unique-abbrev if we do not want to abbreviate at all (with hardcoded constant 40)? IOW, shouldn't this be
strbuf_addstr(&sub_sha1, sha1_to_hex(sha1));
> + argv_array_pushf(&cp.env_array, "name=%s", sub->name); > + argv_array_pushf(&cp.env_array, "path=%s", displaypath); > + argv_array_pushf(&cp.env_array, "sm_path=%s", displaypath);
Why are we sending a value that will always be the same in two variables? "git submodule --help" seems to list only name, path, sha1 and toplevel variables, and not sm_path. Is it a documentation bug, or are we clobbering a variable that the end user may be using for other purposes in her foreach script?
Show 19 quoted lines
> + argv_array_pushf(&cp.env_array, "sha1=%s", sub_sha1.buf);
> + argv_array_pushf(&cp.env_array, "toplevel=%s", toplevel);
> +
> + cp.use_shell = 1;
> + cp.dir = path;
> +
> + for (i = 0; i < argc; i++)
> + argv_array_push(&cp.args, argv[i]);
> +
> + strbuf_addstr(&file, path);
> + strbuf_addstr(&file, "/.git");
> +
> + if (!quiet && !access_or_warn(file.buf, R_OK, 0))
> + printf(_("Entering '%s'\n"), displaypath);
> +
> + if (!access_or_warn(file.buf, R_OK, 0))
> + run_command(&cp);
> +
> + if(recursive) {Missing SP after "if".
More importantly, if the earlier access-or-warn failed and we didn't do the run-command for the path, does it even make sense to recurse into it?
The original scripted version seems to refrain from recursing into it if the command fails in the submodule in the first place. This code discards the exit status from run_command(), which looks wrong.
Show 20 quoted lines
> + struct child_process cpr = CHILD_PROCESS_INIT; > + > + cpr.use_shell = 1; > + cpr.dir = path; > + > + argv_array_pushf(&cpr.args, "git"); > + argv_array_pushf(&cpr.args, "--super-prefix"); > + argv_array_push(&cpr.args, displaypath); > + argv_array_pushf(&cpr.args, "submodule--helper"); > + > + if (quiet) > + argv_array_pushf(&cpr.args, "--quiet"); > + > + argv_array_pushf(&cpr.args, "foreach"); > + argv_array_pushf(&cpr.args, "--recursive"); > + > + for (i = 0; i < argc; i++) > + argv_array_push(&cpr.args, argv[i]); > + > + run_command(&cpr);
Similarly, the exit status of this invocation is discarded, which probably is wrong. The original seems to pay attention to the failure from the command invoked via "foreach --recursive" and stops interation when it sees a failure.
Show 22 quoted lines
> +static int module_foreach(int argc, const char **argv, const char *prefix)
> +{
> + struct pathspec pathspec;
> + struct module_list list = MODULE_LIST_INIT;
> + int quiet = 0;
> + int recursive = 0;
> + int i;
> +
> + struct option module_foreach_options[] = {
> + OPT__QUIET(&quiet, N_("Suppress output of entering each submodule command")),
> + OPT_BOOL(0, "recursive", &recursive,
> + N_("Recurse into nested submodules")),
> + OPT_END()
> + };
> +
> + const char *const git_submodule_helper_usage[] = {
> + N_("git submodule--helper [--quiet] foreach [--recursive] <command>"),
> + NULL
> + };
> +
> + argc = parse_options(argc, argv, prefix, module_foreach_options,
> + git_submodule_helper_usage, PARSE_OPT_KEEP_UNKNOWN);> + if (module_list_compute(0, NULL, prefix, &pathspec, &list) < 0)
> + die("BUG: module_list_compute should not choke on empty pathspec");This line would fit within 80-col if it weren't overly indented, I would think.
One suggestion.
Start by updating "git submodule foreach [--recursive]" tests in t/ directory before writing any more C code. Arrange to have two submodules, have your custom command that is run with foreach fail in the first submodule, and write a test that documents the behaviour of the scripted version (e.g. your custom command should not be even run inside the second submodule). Add a similar test with two submodules, first of which has a nested submodule, and have your custom command fail in one of these three places, and document the behaviour of the scripted version (e.g. your custom command will run for the first submodule, and if it fails there, does not run in the nested submodule or in the second submodule; if your custom command succeeds in the first submodule, it goes to its nested submodule, and if your custom command fails there, the second submodule will not visited, etc.).
Also if it is not done in the existing tests, perhaps writing tests or two with submodules whose names are "submodule 1", "submodule 2", etc. and making sure that there is no funny splitting of the arguments would be a healthy thing to do.
After getting that working, then start writing the above C code. That would catch cases where your "rewrite in C" accidentally introduces regressions.
Thanks.