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

[Jonathan Nieder]

Hi,

Stefan Beller wrote:

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

Sounds sensible.

[...]
> --- a/submodule.c
> +++ b/submodule.c
[...]
> @@ -1319,10 +1338,23 @@ static int get_next_submodule(struct child_process 
> *cp,
>   argv_array_push(&cp->args, default_argv);
>   argv_array_push(&cp->args, "--submodule-prefix");
>   argv_array_push(&cp->args, submodule_prefix.buf);
> +
> + repo_clear(repo);
> + free(repo);
>   ret = 1;
> + } else {
> + /*
> +  * An empty directory is normal,
> +  * the submodule is not initialized
> +  */
> + if (S_ISGITLINK(ce->ce_mode) &&
> + !is_empty_dir(ce->name)) {

What if the directory is nonempty (e.g. contains build artifacts)?

> + spf->result = 1;
> + strbuf_addf(err,
> + _("Could not access submodule 
> '%s'"),
> + ce->name);
> + }

Should this exit the loop?  Otherwise, multiple "Could not access"
messages can go in the same err string a big concatenated line.

Thanks,
Jonathan

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

[Jonathan Tan]

> 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):

> > > 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-jonathanta...@google.com/
[2] 
https://public-inbox.org/git/CAGZ79kbNXD35ZwevjLZcrGsT=2hncupmvuwvp1rjsksh0gd...@mail.gmail.com/