Re: [PATCH v4 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:

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

Two points.

 * Do not use strcasecmp() on the value.

   While "fetch.submoduleerrors" may be case-insenstive, the value
   does not have to be.  We do not want to encourage users to write
   "[fetch] submoduleErrors = Fail", as some people may want to
   write third-party add-on scripts that parse "git config --get
   fetch.submoduleerrors" output.  For example:

	error_handling=$(git config --get fetch.submoduleErrors)
	case "$error_handling" in
	fail)
		... do something ... ;;
	warn)
		... do something else ... ;;
	esac

   We should not force them to write extra code to handle the value
   case-insensitively.

 * Since you need to convert between the enum and the string here,
   in option_parse_submodule_errors(), and in add_options_to_argv(),
   defining a pair of parse/format functions would be cleaner.

	/* really private - use accessors to parse and format */
	static const char *submodule_errors_[] = {
        	[SUBMODULE_ERRORS_FAIL] = "fail",
        	[SUBMODULE_ERRORS_WARN] = "warn",
	};

	static const char *submodule_error(int num)
	{
		assert(0 <= num && num < ARRAY_SIZE(submodule_errors_));
		return submodule_errors[num];
	}

	static int parse_submodule_error(const char *name)
	{
		for (int num = 0; num <	ARRAY_SIZE(submodule_errors_); num++)
			if (!strcmp(submodule_errors_[num], name))
				return num;
		return -1;
	}

The configuration parsing block would then become:

	if (!strcmp(k, "fetch.submoduleerrors")) {
		int num;

		if (!v)
			return config_error_nonbool(k);
		num = parse_submodule_error(v);
		if (num < 0)
			die(_("invalid value..."), ...);
		fetch_config->submodule_errors = num;
		return 0;
	}

This approach is much more maintainable.  You only need to keep the
submodule_errors_[] array up to date with respect to the error-handling
preprocessor macros.  Some reviewers might suggest converting these macros
into a proper enum.  I would not object to that change, but I would not
bother doing it myself as I do not personally care much about the
distinction between an enum and a preprocessor macro in this context.

> @@ -2205,6 +2219,10 @@ 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_FAIL)
> +		strvec_push(argv, "--submodule-errors=fail");
> +	else if (config->submodule_errors == SUBMODULE_ERRORS_WARN)
> +		strvec_push(argv, "--submodule-errors=warn");

This part then becomes:

	if (config->submodule_errors < 0)
		; /* nothing */
	else {
		const char *name = submodule_error(config->submodule_errors);
		strvec_push(argv, "--submodule-errors=%s", name);
	}

This is, again, much more miantainable.

> +static int option_parse_submodule_errors(const struct option *opt,
> +					  const char *arg, int unset)
> +{
> +	int *v = opt->value;
> +	if (unset || !strcasecmp(arg, "fail"))
> +		*v = SUBMODULE_ERRORS_FAIL;
> +	else if (!strcasecmp(arg, "warn"))
> +		*v = SUBMODULE_ERRORS_WARN;
> +	else
> +		die(_("invalid value for '%s': '%s'"), "--submodule-errors", arg);
> +	return 0;
> +}

Updating this function is left as an exercise ;-)