submodule--helper: don't use global --super-prefix in "absorbgitdirs"

The "--super-prefix" facility was introduced in [1] has always been a transitory hack, which is why we've made it an error to supply it as an option to "git" to commands that don't know about it. That's been a good goal, as it has a global effect we haven't wanted calls to get_super_prefix() from built-ins we didn't expect. But it has meant that when we've had chains of different built-ins using it all of the processes in that "chain" have needed to support it, and worse processes that don't need it have needed to ask for "SUPPORT_SUPER_PREFIX" because their parent process needs it. That's how "fsmonitor--daemon" ended up with it, per [2] it's called from (among other things) "submodule--helper absorbgitdirs", but as we declared "submodule--helper" as "SUPPORT_SUPER_PREFIX" we needed to declare "fsmonitor--daemon" as accepting it too, even though it doesn't care about it. But in the case of "absorbgitdirs" it only needed "--super-prefix" to invoke itself recursively, and we'd never have another "in-between" process in the chain. So we didn't need the bigger hammer of "git --super-prefix", and the "setenv(GIT_SUPER_PREFIX_ENVIRONMENT, ...)" that it entails. Let's instead accept a hidden "--super-prefix" option to "submodule--helper absorbgitdirs" itself. Eventually (as with all other "--super-prefix" users) we'll want to clean this code up so that this all happens in-process. I.e. needing any variant of "--super-prefix" is itself a hack around our various global state, and implicit reliance on "the_repository". This stepping stone makes such an eventual change easier, as we'll need to deal with less global state at that point. The "fsmonitor--daemon" test adjusted here was added in [3]. To assert that it didn't run into the "--super-prefix" message it was asserting the output it didn't have. Let's instead assert the full output that we *do* have, using the same pattern as a preceding change to "t/t7412-submodule-absorbgitdirs.sh" used. We could also remove the test entirely (as [4] did), but even though the initial reason for having it is gone we're still getting some marginal benefit from testing the "fsmonitor" and "submodule absorbgitdirs" interaction, so let's keep it. The change here to have either a NULL or non-"" string as a "super_prefix" instead of the previous arrangement of "" or non-"" is somewhat arbitrary. We could also decide to never have to check for NULL. As we'll be changing the rest of the "git --super-prefix" users to the same pattern, leaving them all consistent makes sense. Why not pick "" over NULL? Because that's how the "prefix" works[5], and having "prefix" and "super_prefix" work the same way will be less confusing. That "prefix" picked NULL instead of "" is itself arbitrary, but as it's easy to make this small bit of our overall API consistent, let's go with that. 1. 74866d75793 (git: make super-prefix option, 2016-10-07) 2. 53fcfbc84f6 (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26) 3. 53fcfbc84f6 (fsmonitor--daemon: allow --super-prefix argument, 2022-05-26) 4. https://lore.kernel.org/git/20221109004708.97668-5-chooglen@google.com/ 5. 9725c8dda20 (built-ins: trust the "prefix" from run_builtin(), 2022-02-16) Signed-off-by: Glen Choo <chooglen@google.com> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar Arnfjörð Bjarmason committed Dec 20, 2022 at 13:39 UTC bb61a962d2e759754fca35e4b31f73122eed49fc
5 files changed +29 -32
builtin/submodule--helper.c
+3 -4
@@ -2834,8 +2834,9 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
2834 int i;
2835 struct pathspec pathspec = { 0 };
2836 struct module_list list = MODULE_LIST_INIT;
2837 - const char *super_prefix;
2837 + const char *super_prefix = NULL;
2838 struct option embed_gitdir_options[] = {
2839 + OPT__SUPER_PREFIX(&super_prefix),
2840 OPT_END()
2841 };
2842 const char *const git_submodule_helper_usage[] = {
@@ -2850,7 +2851,6 @@ static int absorb_git_dirs(int argc, const char **argv, const char *prefix)
2851 if (module_list_compute(argv, prefix, &pathspec, &list) < 0)
2852 goto cleanup;
2853
2853 - super_prefix = get_super_prefix();
2854 for (i = 0; i < list.nr; i++)
2855 absorb_git_dir_into_superproject(list.entries[i]->name,
2856 super_prefix);
@@ -3391,8 +3391,7 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)
3391
3392 if (strcmp(subcmd, "clone") && strcmp(subcmd, "update") &&
3393 strcmp(subcmd, "foreach") && strcmp(subcmd, "status") &&
3394 - strcmp(subcmd, "sync") && strcmp(subcmd, "absorbgitdirs") &&
3395 - get_super_prefix())
3394 + strcmp(subcmd, "sync") && get_super_prefix())
3395 /*
3396 * xstrfmt() rather than "%s %s" to keep the translated
3397 * string identical to git.c's.
git.c
+1 -1
@@ -539,7 +539,7 @@ static struct cmd_struct commands[] = {
539 { "format-patch", cmd_format_patch, RUN_SETUP },
540 { "fsck", cmd_fsck, RUN_SETUP },
541 { "fsck-objects", cmd_fsck, RUN_SETUP },
542 - { "fsmonitor--daemon", cmd_fsmonitor__daemon, SUPPORT_SUPER_PREFIX | RUN_SETUP },
542 + { "fsmonitor--daemon", cmd_fsmonitor__daemon, RUN_SETUP },
543 { "gc", cmd_gc, RUN_SETUP },
544 { "get-tar-commit-id", cmd_get_tar_commit_id, NO_PARSEOPT },
545 { "grep", cmd_grep, RUN_SETUP_GENTLY },
parse-options.h
+4
@@ -369,6 +369,10 @@ int parse_opt_tracking_mode(const struct option *, const char *, int);
369 { OPTION_CALLBACK, 0, "abbrev", (var), N_("n"), \
370 N_("use <n> digits to display object names"), \
371 PARSE_OPT_OPTARG, &parse_opt_abbrev_cb, 0 }
372 +#define OPT__SUPER_PREFIX(var) \
373 + OPT_STRING_F(0, "super-prefix", (var), N_("prefix"), \
374 + N_("prefixed path to initial superproject"), PARSE_OPT_HIDDEN)
375 +
376 #define OPT__COLOR(var, h) \
377 OPT_COLOR_FLAG(0, "color", (var), (h))
378 #define OPT_COLUMN(s, l, v, h) \
submodule.c
+7 -5
@@ -2275,7 +2275,8 @@ int validate_submodule_git_dir(char *git_dir, const char *submodule_name)
2275 * Embeds a single submodules git directory into the superprojects git dir,
2276 * non recursively.
2277 */
2278 -static void relocate_single_git_dir_into_superproject(const char *path)
2278 +static void relocate_single_git_dir_into_superproject(const char *path,
2279 + const char *super_prefix)
2280 {
2281 char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;
2282 struct strbuf new_gitdir = STRBUF_INIT;
@@ -2305,7 +2306,7 @@ static void relocate_single_git_dir_into_superproject(const char *path)
2306 real_new_git_dir = real_pathdup(new_gitdir.buf, 1);
2307
2308 fprintf(stderr, _("Migrating git directory of '%s%s' from\n'%s' to\n'%s'\n"),
2308 - get_super_prefix_or_empty(), path,
2309 + super_prefix ? super_prefix : "", path,
2310 real_old_git_dir, real_new_git_dir);
2311
2312 relocate_gitdir(path, real_old_git_dir, real_new_git_dir);
@@ -2325,10 +2326,11 @@ static void absorb_git_dir_into_superproject_recurse(const char *path,
2326 cp.dir = path;
2327 cp.git_cmd = 1;
2328 cp.no_stdin = 1;
2328 - strvec_pushf(&cp.args, "--super-prefix=%s%s/", super_prefix ?
2329 - super_prefix : "", path);
2329 strvec_pushl(&cp.args, "submodule--helper",
2330 "absorbgitdirs", NULL);
2331 + strvec_pushf(&cp.args, "--super-prefix=%s%s/", super_prefix ?
2332 + super_prefix : "", path);
2333 +
2334 prepare_submodule_repo_env(&cp.env);
2335 if (run_command(&cp))
2336 die(_("could not recurse into submodule '%s'"), path);
@@ -2382,7 +2384,7 @@ void absorb_git_dir_into_superproject(const char *path,
2384 char *real_common_git_dir = real_pathdup(get_git_common_dir(), 1);
2385
2386 if (!starts_with(real_sub_git_dir, real_common_git_dir))
2385 - relocate_single_git_dir_into_superproject(path);
2387 + relocate_single_git_dir_into_superproject(path, super_prefix);
2388
2389 free(real_sub_git_dir);
2390 free(real_common_git_dir);
t/t7527-builtin-fsmonitor.sh
+14 -22
@@ -866,27 +866,9 @@ test_expect_success 'submodule always visited' '
866 # the submodule, and someone does a `git submodule absorbgitdirs`
867 # in the super, Git will recursively invoke `git submodule--helper`
868 # to do the work and this may try to read the index. This will
869 -# try to start the daemon in the submodule *and* pass (either
870 -# directly or via inheritance) the `--super-prefix` arg to the
871 -# `git fsmonitor--daemon start` command inside the submodule.
872 -# This causes a warning because fsmonitor--daemon does take that
873 -# global arg (see the table in git.c)
874 -#
875 -# This causes a warning when trying to start the daemon that is
876 -# somewhat confusing. It does not seem to hurt anything because
877 -# the fsmonitor code maps the query failure into a trivial response
878 -# and does the work anyway.
879 -#
880 -# It would be nice to silence the warning, however.
881 -
882 -have_t2_error_event () {
883 - log=$1
884 - msg="fsmonitor--daemon doesnQt support --super-prefix" &&
885 -
886 - tr '\047' Q <$1 | grep -e "$msg"
887 -}
869 +# try to start the daemon in the submodule.
870
889 -test_expect_success "stray submodule super-prefix warning" '
871 +test_expect_success "submodule absorbgitdirs implicitly starts daemon" '
872 test_when_finished "rm -rf super; \
873 rm -rf sub; \
874 rm super-sub.trace" &&
@@ -904,10 +886,20 @@ test_expect_success "stray submodule super-prefix warning" '
886
887 test_path_is_dir super/dir_1/dir_2/sub/.git &&
888
889 + cwd="$(cd super && pwd)" &&
890 + cat >expect <<-EOF &&
891 + Migrating git directory of '\''dir_1/dir_2/sub'\'' from
892 + '\''$cwd/dir_1/dir_2/sub/.git'\'' to
893 + '\''$cwd/.git/modules/dir_1/dir_2/sub'\''
894 + EOF
895 GIT_TRACE2_EVENT="$PWD/super-sub.trace" \
908 - git -C super submodule absorbgitdirs &&
896 + git -C super submodule absorbgitdirs >out 2>actual &&
897 + test_cmp expect actual &&
898 + test_must_be_empty out &&
899
910 - ! have_t2_error_event super-sub.trace
900 + # Confirm that the trace2 log contains a record of the
901 + # daemon starting.
902 + test_subcommand git fsmonitor--daemon start <super-sub.trace
903 '
904
905 # On a case-insensitive file system, confirm that the daemon