Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo

[Junio C Hamano <gitster@pobox.com>]

Philippe Blain <levraiphilippeblain@gmail.com> 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.