Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo
From: Junio C Hamano <hidden>
Date: 2020-12-07 19:44:03
Philippe Blain [off-list ref] writes:
quoted hunk
Maybe a personal preference, but I would have gone for something a little simpler, like the following:diff --git a/submodule.c b/submodule.c index b3bb59f066..4200865174 100644 --- a/submodule.c +++ b/submodule.c@@ -1486,7 +1486,7 @@ static int get_next_submodule(struct child_process *cp, * the submodule is not initialized */ if (S_ISGITLINK(ce->ce_mode) && - !is_empty_dir(ce->name)) { + !is_empty_dir(repo_worktree_path(spf->r, "%s", ce->name))) {
But then you leak the return value from repo_worktree_path(), no?
quoted
+test_expect_success 'recursive fetch with uninit submodule' ' + # depends on previous test for setup + + git -C main submodule deinit -f sub &&Here you are deiniting the submodule, such that the Git directory will stay in .git/modules/sub. This is not the same thing as a submodule that was never initialized ("uninitialized"), for which .git/modules/sub will not yet exist. So maybe we could harden the tests by also testing for that scenario ? I don't know... maybe the infinite loop only happens if .git/modules/sub actually already exists. If so, the test name should be "recursive fetch with deinitialized submodule", I think.
Even if the original breakage happens only for deinitialized case, it would be sensible to test uninitialized one as well, I would think.
Thanks for working on that, and sorry for not having the time to comment before you sent v2.
Thanks, all. It's not like we corral everybody to a single place on a single day to work on a single thing. Reviews ov v2 by reviewers who have not seen v1 is totally expected and very much appreciated.