Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=""
- From
- Phillip Wood <phillip.wood123@gmail.com>
- Date
- Feb 22, 2023, 10:18 UTC
- Message-ID
- <f0d0a3fb-d7eb-84d6-7852-0d30eb38af63@dunelm.org.uk>
- In-Reply-To
- <CAMMLpeQ9gEKNX5VtGCgLL_Qzk59ZYji57u-SPy-XeSRPuF2NwA@mail.gmail.com>
Hi Alex
On 22/02/2023 01:38, Alex Henrie wrote:
Show 25 quoted lines
> On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote: >> >> On 21/02/2023 05:58, Alex Henrie wrote: >>> The unusual syntax --rebase-merges="" (that is, --rebase-merges with an >>> empty string argument) has been an undocumented synonym of >>> --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid >>> confusion when a rebase.merges config option is introduced, where >>> rebase.merges="" will be equivalent to not passing --rebase-merges. >> >> I think this is sensible in the context of adding a config option. It is >> a backwards incompatible change though, lets hope no one was relying on >> it. > > Since the syntax is bizarre and undocumented, I doubt anyone is > relying on it for anything serious, if anyone uses it at all. > >> Is there a particular reason you decided to redo the option parsing >> rather than just calling parse_merges_value() from the existing "if >> (rebase_merges)" block? I don't think it really matters, I'm just curious. > > Without a parse_opt_merges callback, how could we know whether the > user passed --no-rebase-merges as opposed to passing nothing at all? > const char *rebase_merges would be NULL in either case. It's an > important distinction to make because --no-rebase-merges overrides > rebase.merges but the absence of a command-line argument does not.
The usual way we handle that is to set the value of rebase_merges from the config before calling parse_options(). However your solution is fine.
Show 5 quoted lines
> All the same, your comment made me realize that it would probably make > more sense to simply change the default value of --rebase-cousins from > "" to "no-rebase-cousins" in this patch and then add the > parse_opt_merges callback in the next patch when it is actually > needed.
Sounds good
Phillip
> -Alex