Re: [PATCH v2] submodules: fix of regression on fetching of non-init subsub-repo
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 7, 2020, 19:43 UTC
- Message-ID
- <xmqqpn3l9zce.fsf@gitster.c.googlers.com>
- In-Reply-To
- <613FAD04-0D5A-4DE0-8FE8-0C5C5619B7BC@gmail.com>
Philippe Blain <levraiphilippeblain@gmail.com> writes:
Show 13 quoted lines
> 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?
Show 12 quoted lines
>> +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.