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