Subject: Re: [PATCH v4 01/10] userdiff: refactor away the parse_bool() function

From: Jeff King <[email-masked]>
Date: Wed, 24 Mar 2021 14:47:00 -0400

On Wed, Mar 24, 2021 at 02:48:43AM +0100, Ævar Arnfjörð Bjarmason wrote:

> Since 6680a0874f (drop odd return value semantics from
> userdiff_config, 2012-02-07) we have not cared about the return values
> of parse_tristate() or git_config_bool() v.s. falling through in
> userdiff_config(), so let's do so in those cases to make the code
> easier to read.
> 
> Having a wrapper function for git_config_bool() dates back to
> d9bae1a178 (diff: cache textconv output, 2010-04-01) and
> 122aa6f9c0 (diff: introduce diff.<driver>.binary, 2008-10-05), both of
> which predated the change in 6680a0874f which made their return values
> redundant.

I think this cleanup makes sense.

>  int userdiff_config(const char *k, const char *v)
> @@ -312,16 +305,17 @@ int userdiff_config(const char *k, const char *v)
>  		return parse_funcname(&drv->funcname, k, v, 0);
>  	if (!strcmp(type, "xfuncname"))
>  		return parse_funcname(&drv->funcname, k, v, REG_EXTENDED);
> -	if (!strcmp(type, "binary"))
> -		return parse_tristate(&drv->binary, k, v);
>  	if (!strcmp(type, "command"))
>  		return git_config_string(&drv->external, k, v);
>  	if (!strcmp(type, "textconv"))
>  		return git_config_string(&drv->textconv, k, v);
> -	if (!strcmp(type, "cachetextconv"))
> -		return parse_bool(&drv->textconv_want_cache, k, v);
>  	if (!strcmp(type, "wordregex"))
>  		return git_config_string(&drv->word_regex, k, v);
> +	/* Don't care about the parse errors for these, fallthrough */
> +	if (!strcmp(type, "cachetextconv"))
> +		drv->textconv_want_cache = git_config_bool(k, v);
> +	if (!strcmp(type, "binary"))
> +		parse_tristate(&drv->binary, k, v);

The original returned early, which short-circuited the rest of the
strcmp(). that probably doesn't matter much in practice (after all, an
unrecognized value is inherently O(n)). But perhaps:

  if (...)
     assign...;
  else if (...)
     assign...;

would make the comment unnecessary. I don't feel strongly either way,
though.

-Peff