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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 2, 2019, 01:58 UTC
Message-ID: <20190202015817.GA241226@google.com>
In-Reply-To: <20181129002756.167615-8-sbeller@google.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

[...]

Show 15 quoted lines

> @@ -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)?

Show 5 quoted lines

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