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

[Jonathan Nieder <jrnieder@gmail.com>]

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