Re: [PATCH 7/9] submodule: migrate get_next_submodule to use repository structs
- Date: Tue, 4 Dec 2018 16:17:36 -0800
- From: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
- Subject: Re: [PATCH 7/9] submodule: migrate get_next_submodule to use repository structs
> We used to recurse into submodules, even if they were broken having
> only an objects directory. The child process executed in the submodule
> would fail though if the submodule was broken. This is tested via
> "fetching submodule into a broken repository" in t5526.
> This patch tightens the check upfront, such that we do not need
> to spawn a child process to find out if the submodule is broken.
Thanks, patches 4-7 look good to me - I see that you have addressed all
my comments. Not sending one email each for patches 4, 5, and 6 -
although I have commented on all of them, my comments were minor.
My more in-depth review was done on a previous version , and I see
that my comments have been addressed. Also, Stefan says  (and implements
in this patch):
> > > If the working tree directory is empty for that submodule, it means
> > > it is likely not initialized. But why would we use that as a signal to
> > > skip the submodule?
> > What I meant was: if empty, skip it completely. Otherwise, do the
> > repo_submodule_init() and repo_init() thing, and if they both fail, set
> > spf->result to 1, preserving existing behavior.
> I did it the other way round:
> If repo_[submodule_]init fails, see if we have a gitlink in tree and
> an empty dir in the FS, to decide if we need to signal failure.
This works too.