Philippe Blain <levraiphilippeblain@xxxxxxxxx> writes:
> 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?
>> +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.
