Re: [PATCH 7/9] submodule: migrate get_next_submodule to use repository structs

From: Jonathan Tan <jonathantanmy@google.com>
Date: Dec 5, 2018, 00:17 UTC
Message-ID: <20181205001736.72764-1-jonathantanmy@google.com>
In-Reply-To: <20181129002756.167615-8-sbeller@google.com>

Show 7 quoted lines

> 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 [1], and I see that my comments have been addressed. Also, Stefan says [2] (and implements in this patch):

Show 12 quoted lines

> > > 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.

[1] https://public-inbox.org/git/20181017225811.66554-1-jonathantanmy@google.com/ [2] https://public-inbox.org/git/CAGZ79kbNXD35ZwevjLZcrGsT=2hNcUPmVUWvP1RjsKSH0Gd3ww@mail.gmail.com/