Subject: Re: [PATCH v2 15/15] merge-recursive: switch directory rename detection default

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Sat, 30 Mar 2019 10:12:39 +0100

On Sat, Mar 30 2019, Elijah Newren wrote:

I may have more, just quickly skimming this for the first time...

>  merge.renames::
> -	Whether and how Git detects renames.  If set to "false",
> -	rename detection is disabled. If set to "true", basic rename
> -	detection is enabled.  Defaults to the value of diff.renames.
> +	Whether Git detects renames.  If set to "false", rename detection
> +	is disabled. If set to "true", basic rename detection is enabled.
> +	Defaults to the value of diff.renames.
> [...]
> +	if (!git_config_get_string("merge.directoryrenames", &value)) {
> +		if (!strcasecmp(value, "true"))
> +			opt->detect_directory_renames = 2;
> +		else if (!strcasecmp(value, "false"))
> +			opt->detect_directory_renames = 0;
> +		else if (!strcasecmp(value, "conflict"))
> +			opt->detect_directory_renames = 1;
> +		else {
> +			error(_("Invalid value for merge.directoryRenames: %s"),
> +			      value);
> +			opt->detect_directory_renames = 1;
> +		}
> +		free(value);
> +	}

Instead of making your own true/false parser you can use
git_parse_maybe_bool(). See what we do for merge.ff:

    builtin/merge.c-617-    else if (!strcmp(k, "merge.ff")) {
    builtin/merge.c:618:            int boolval = git_parse_maybe_bool(v);
    builtin/merge.c-619-            if (0 <= boolval) {
    builtin/merge.c-620-                    fast_forward = boolval ? FF_ALLOW : FF_NO;
    builtin/merge.c-621-            } else if (v && !strcmp(v, "only")) {
    builtin/merge.c-622-                    fast_forward = FF_ONLY;
    builtin/merge.c-623-            } /* do not barf on values from future versions of git */
    builtin/merge.c-624-            return 0;

Small nit, but allows us to document "this config takse bool or ..."
without having different verions of "bool" in various places.

Also, I don't care personally, but this also violates the "if one thing
requires braces put it on all the if/else arms" in CodingGuidelines.