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

[Elijah Newren <newren@gmail.com>]

On Sat, Mar 30, 2019 at 2:12 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> 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.

Thanks for taking a look.  I'll make the fix, and wait for other
feedback before resending.