From 4765017f8db889d9aec80751b4021dd0fc6be7bd Mon Sep 17 00:00:00 2001 From: Shourya Shukla Date: Mon, 13 Jul 2020 03:05:01 +0530 Subject: [PATCH] Use is_submodule_active() instead of spawning a process Use is_submodule_active() instead of spawning a new process for every submodule in prepare_submodule_summary(). Quote from Dscho: > + /* Also show added or modified modules which are checked out */ > + cp_rev_parse.dir = p->sm_path; > + cp_rev_parse.git_cmd = 1; > + cp_rev_parse.no_stderr = 1; > + cp_rev_parse.no_stdout = 1; > + > + argv_array_pushl(&cp_rev_parse.args, "rev-parse", > + "--git-dir", NULL); > + > + if (!run_command(&cp_rev_parse)) I wonder whether we really need to waste an entire spawned process on figuring out whether `p->sm_path` refers to an active repository. Wouldn't `is_submodule_active(r, p->sm_path)` fulfill the same purpose? ----------------------------------------------------------- The shell version just tests for a gitdir. --------------8<----------- GIT_DIR="$sm_path/.git" git rev-parse --git-dir >/dev/null 2>&1 && printf '%s\n' "$sm_path" -------------->8----------- Then we should also do just that instead of spawning a CP ever. The 'rev-parse' Documentation states ========================= --git-dir Show $GIT_DIR if defined. Otherwise show the path to the .git directory. The path shown, when relative, is relative to the current working directory. If $GIT_DIR is not defined and the current directory is not detected to lie in a Git repository or work tree print a message to stderr and exit with nonzero status. ========================= Another way to do this is via the helper function 'is_nonbare_repository_dir()' (not using is_git_directory() because of the failure it was causing before) https://github.com/periperidip/git/commit/823a287b51ccb488042412cab27bf2c431f81e09 Therefore, instead of spawning a CP ever, do a check with the aforementioned function. All tests pass. https://lore.kernel.org/git/nycvar.QRO.7.76.6.2007031712160.50@tvgsbejvaqbjf.bet/ --- builtin/submodule--helper.c | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index fc270732001500..bd9515059e828d 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1192,7 +1192,7 @@ static void prepare_submodule_summary(struct summary_cb *info, for (i = 0; i < list->nr; i++) { const struct submodule *sub; struct module_cb *p = list->entries[i]; - struct child_process cp_rev_parse = CHILD_PROCESS_INIT; + struct strbuf sm_gitdir = STRBUF_INIT; if (p->status == 'D' || p->status == 'T') { generate_submodule_summary(info, p); @@ -1219,16 +1219,10 @@ static void prepare_submodule_summary(struct summary_cb *info, } /* Also show added or modified modules which are checked out */ - cp_rev_parse.dir = p->sm_path; - cp_rev_parse.git_cmd = 1; - cp_rev_parse.no_stderr = 1; - cp_rev_parse.no_stdout = 1; - - argv_array_pushl(&cp_rev_parse.args, "rev-parse", - "--git-dir", NULL); - - if (!run_command(&cp_rev_parse)) + strbuf_addstr(&sm_gitdir, p->sm_path); + if (is_nonbare_repository_dir(&sm_gitdir)) generate_submodule_summary(info, p); + strbuf_release(&sm_gitdir); } }