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