Re: [PATCH v2 4/4] rebase: add a config option for --rebase-merges
- From
Alex Henrie <alexhenrie24@gmail.com>
- Date
- Feb 22, 2023, 01:41 UTC
- Message-ID
- <CAMMLpeQpSMVm0ZNMNz20+9bKAGhn4pFwTX62nR+B-V-KzvqT+A@mail.gmail.com>
- In-Reply-To
- <d1f10c52-9cfb-8e6a-cbd6-cd81b778b8cc@dunelm.org.uk>
On Tue, Feb 21, 2023 at 3:46 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
Show 7 quoted lines
> > 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.
Show 16 quoted lines
> > + 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.
Show 5 quoted lines
> > +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