Re: [PATCH v3 2/2] fetch: add fetch.submoduleErrors to make submodule fetch errors non-fatal

[Junio C Hamano <gitster@pobox.com>]

Paulius Zaleckas <paulius.zaleckas@gmail.com> writes:

> When fetching with --recurse-submodules, a submodule commit that is not
> yet reachable from any of the submodule's remote refs causes the entire
> fetch to fail.  This is overly strict when the missing commit belongs to
> an upstream branch that is still being prepared (e.g. an in-progress
> merge topic): the local branch does not need that commit, so there is no
> reason to treat its absence as fatal.
>
> Add a new config key fetch.submoduleErrors (values: fail/warn) and a
> corresponding --submodule-errors=(fail|warn) command-line option that
> control this behaviour.  The default remains fail (existing behaviour);
> setting the value to warn causes submodule fetch failures to be reported
> on stderr without affecting the overall exit status of git fetch / git
> pull.
>
> Forward the option to child fetches in add_options_to_argv() so that it
> also takes effect for `git fetch --all` / `--multiple` (where per-remote
> child processes handle the submodule recursion themselves) and for
> nested submodule recursion.
>
> Signed-off-by: Paulius Zaleckas <paulius.zaleckas@gmail.com>
> ---
>  Documentation/config/fetch.adoc  | 14 ++++++
>  Documentation/fetch-options.adoc |  8 ++++
>  builtin/fetch.c                  | 41 ++++++++++++++++-
>  submodule.c                      |  8 +++-
>  submodule.h                      |  7 ++-
>  t/t5526-fetch-submodules.sh      | 76 ++++++++++++++++++++++++++++++++
>  6 files changed, 150 insertions(+), 4 deletions(-)
>
> diff --git a/Documentation/config/fetch.adoc b/Documentation/config/fetch.adoc
> index 04ac90912d..5c9c942a70 100644
> --- a/Documentation/config/fetch.adoc
> +++ b/Documentation/config/fetch.adoc
> @@ -10,6 +10,20 @@
>  	reference.
>  	Defaults to `on-demand`, or to the value of `submodule.recurse` if set.
>  
> +`fetch.submoduleErrors`::
> +	Controls how errors from submodule fetches are handled when
> +	`--recurse-submodules` is in effect. When set to `fail` (the default),
> +	any submodule fetch error causes the overall `git fetch` or `git pull`
> +	to exit with a non-zero status. When set to `warn`, submodule fetch
> +	errors are reported to standard error but do not affect the exit
> +	status of the command. This is useful when working in repositories
> +	where some branches reference submodule commits that are not yet
> +	available on the submodule remote, but those commits are not needed
> +	for the currently checked-out branch.
> ++
> +The value of this option can be overridden by the `--submodule-errors`
> +option of linkgit:git-fetch[1].
> +
>  `fetch.fsckObjects`::
>  	If it is set to true, git-fetch-pack will check all fetched
>  	objects. See `transfer.fsckObjects` for what's
> diff --git a/Documentation/fetch-options.adoc b/Documentation/fetch-options.adoc
> index 035f780e58..78525f6848 100644
> --- a/Documentation/fetch-options.adoc
> +++ b/Documentation/fetch-options.adoc
> @@ -294,6 +294,14 @@ ifndef::git-pull[]
>  `--no-recurse-submodules`::
>  	Disable recursive fetching of submodules (this has the same effect as
>  	using the `--recurse-submodules=no` option).
> +
> +`--submodule-errors=(fail|warn)`::
> +	Control how errors from submodule fetches are handled when
> +	`--recurse-submodules` is in effect. When set to `fail` (the default),
> +	any submodule fetch error causes the overall `git fetch` to exit with a
> +	non-zero status. When set to `warn`, submodule fetch errors are reported
> +	to standard error but do not affect the exit status of the command. Can
> +	also be configured via `fetch.submoduleErrors`. See linkgit:git-config[1].
>  endif::git-pull[]
>  
>  `--set-upstream`::
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index c1d7c672f4..40daaf5cc7 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -110,6 +110,7 @@ struct fetch_config {
>  	int recurse_submodules;
>  	int parallel;
>  	int submodule_fetch_jobs;
> +	int submodule_errors;
>  };
>  
>  static int git_fetch_config(const char *k, const char *v,
> @@ -152,6 +153,19 @@ static int git_fetch_config(const char *k, const char *v,
>  		return 0;
>  	}
>  
> +	if (!strcmp(k, "fetch.submoduleerrors")) {
> +		if (!v)
> +			return config_error_nonbool(k);
> +		else if (!strcasecmp(v, "fail"))
> +			fetch_config->submodule_errors = SUBMODULE_ERRORS_FAIL;
> +		else if (!strcasecmp(v, "warn"))
> +			fetch_config->submodule_errors = SUBMODULE_ERRORS_WARN;
> +		else
> +			die(_("invalid value for '%s': '%s'"),
> +			    "fetch.submoduleErrors", v);
> +		return 0;
> +	}
> +
>  	if (!strcmp(k, "fetch.parallel")) {
>  		fetch_config->parallel = git_config_int(k, v, ctx->kvi);
>  		if (fetch_config->parallel < 0)


> @@ -2205,6 +2219,8 @@ static void add_options_to_argv(struct strvec *argv,
>  		strvec_push(argv, "--no-recurse-submodules");
>  	else if (config->recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND)
>  		strvec_push(argv, "--recurse-submodules=on-demand");
> +	if (config->submodule_errors == SUBMODULE_ERRORS_WARN)
> +		strvec_push(argv, "--submodule-errors=warn");
>  	if (tags == TAGS_SET)
>  		strvec_push(argv, "--tags");
>  	else if (tags == TAGS_UNSET)

If (config->submodule_errors != SUBMODULE_ERRORS_WARN), then the argv[]
would not see any --submodule-errors=<anything> to propagate down.
Specifically, this function is called when recurse-submodules is not
disabled, and prepares argv[] used to call fetch_submodules().

>  int cmd_fetch(int argc,
>  	      const char **argv,
>  	      const char *prefix,
> @@ -2477,6 +2506,7 @@ int cmd_fetch(int argc,
>  		.recurse_submodules = RECURSE_SUBMODULES_DEFAULT,
>  		.parallel = 1,
>  		.submodule_fetch_jobs = -1,
> +		.submodule_errors = SUBMODULE_ERRORS_FAIL,
>  	};

Here, .submodule_errors member is initialized to
SUBMODULE_ERRORS_FAIL (i.e. 0).

> @@ -2491,6 +2521,7 @@ int cmd_fetch(int argc,
>  	int max_jobs = -1;
>  	int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
>  	int recurse_submodules_default = RECURSE_SUBMODULES_ON_DEMAND;
> +	int submodule_errors_cli = -1; /* -1: not set on command line */
>  	int fetch_write_commit_graph = -1;
>  	int stdin_refspecs = 0;
>  	int negotiate_only = 0;
> @@ -2527,6 +2558,10 @@ int cmd_fetch(int argc,
>  		OPT_CALLBACK_F(0, "recurse-submodules", &recurse_submodules_cli, N_("on-demand"),
>  			    N_("control recursive fetching of submodules"),
>  			    PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
> +		OPT_CALLBACK_F(0, "submodule-errors", &submodule_errors_cli,
> +			    N_("(fail|warn)"),
> +			    N_("control how submodule fetch errors are handled"),
> +			    0, option_parse_submodule_errors),

And command line option "--submodule-errors={warn,fail}" may update
the local variable submodule_errors_cli (initialied to -1) to one of
SUBMODULE_ERRORS_{WARN,FAIL}.   These are different from -1, so we
can reliably tell if we saw a command line override, which is good.

>  		OPT_BOOL(0, "dry-run", &dry_run,
>  			 N_("dry run")),
>  		OPT_BOOL(0, "porcelain", &porcelain, N_("machine-readable output")),
> @@ -2616,6 +2651,9 @@ int cmd_fetch(int argc,
>  	if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
>  		config.recurse_submodules = recurse_submodules_cli;
>  
> +	if (submodule_errors_cli != -1)
> +		config.submodule_errors = submodule_errors_cli;

And we override what we read from the configuration if we got a
command line override.

And the value in config.submodule_errors is used much later, in a
call to add_options_to_argv() we saw earlier, but this patch does
not touch the caller so we do not see the calling site.

I do not do submodules, so my expectation here may be a bit skewed,
but what happens when we configure fetch.submoduleErrors to warn,
but override it from the command line to fail?  .submodule_errors is
set to SUBMODULE_ERRORS_FAIL here?  As we saw, add_options_to_argv()
stuff --submodule-error=<setting> only when config.submodule_errors
is set to SUBMODULE_ERRORS_WARN, so we do not pass command line
override.  Is this desirable?  Don't we want to pass down not just
--submodule-error=warn but --submodule-error=fail if that is what
was given from the command line?  Or does it not matter because fail
is the default?

Thanks.