On Tue, Oct 23, 2018 at 3:55 PM Jonathan Tan <jonathantanmy@xxxxxxxxxx> wrote:
> > When adding the comment here, we'd also want to have
> > the comment in prepare_submodule_repo_env, which
> > could be its own preparation commit.
>
> I agree with the protection. As for the preparation commit, I don't
> think it's always the code author's responsibility to tidy up the
> surrounding code, but since you're adding an identical comment here,
> it's probably worth it to add the comment there too.
Am I the only one who dislikes inconsistent files? ;-)
(ie. clean in one place, not cleaned up in another)
I can see your point. Will add a comment
> > Thinking of that, maybe we need to announce that in get_next_submodule
>
> The consequence of getting caught changes, though. Currently,
> spf->result is set to 1 whenever a child process fails. But in this
> patch, some of these repositories would be entirely skipped, meaning
> that no child process is run, and spf->result is never modified.
Right.
> > 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.
I can switch it around again, but it seemed easier to write as
that puts corner cases away into one else {} case.
