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

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

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.

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.

Updating this function is left as an exercise ;-)