On Tue, Feb 21, 2023 at 3:46 AM Phillip Wood <phillip.wood123@xxxxxxxxx> wrote:
>
> On 21/02/2023 05:58, Alex Henrie wrote:
> > +rebase.merges::
> > + Default value of `--rebase-merges` option.
>
> Below I see this takes a boolean in addition to [no-]rebase-cousins, it
> would be nice to document that, especially what "true" means.
Good point. I'll add another sentence explaining that "true" is
equivalent to --rebase-merges without arguments and "false" is
equivalent to --no-rebase-merges.
> > + if (!strcmp(var, "rebase.merges")) {
> > + const char *rebase_merges;
> > + if (!git_config_string(&rebase_merges, var, value) &&
> > + rebase_merges && *rebase_merges) {
>
> rebase_merges is guaranteed to be non-NULL if git_config_string returns 0.
>
> > + opts->rebase_merges = git_parse_maybe_bool(rebase_merges);
> > + if (opts->rebase_merges < 0)
> > + parse_merges_value(opts, rebase_merges);
> > + }
> > + return 0;
> > + }
>
> I think this leaks rebase_merges as git_config_string() returns a copy
> despite taking a "const char*".
Come to think of it, I don't think we need git_config_string at all
here. As far as I can tell, it should be fine to just check (value &&
*value) and not duplicate the string in memory.
> > +test_expect_success 'rebase.merges="" is equivalent to not passing --rebase-merges' '
> > + git config rebase.merges "" &&
>
> test_config is generally preferred as it clears the value at the end of
> the test. Then you don't need the final hunk of this patch.
I've used test_config before but I forgot about it. Thanks for the reminder.
-Alex
