git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=""

From
PWPhillip 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
Previous: Alex HenrieNext: Junio C Hamano
Message 11 of 17 in “rebase: document the --no-rebase-merges option”
  1. 1/4 rebase: document the --no-rebase-merges optionAlex Henrie, Feb 21, 2023
  2. 2/4 rebase: add tests for --no-rebase-mergesAlex Henrie, Feb 21, 2023
  3. Phillip WoodFeb 21, 2023
  4. Alex HenrieFeb 22, 2023
  5. Phillip WoodFeb 22, 2023
  6. Alex HenrieFeb 23, 2023
  7. 3/4 rebase: stop accepting --rebase-merges=""Alex Henrie, Feb 21, 2023
  8. Phillip WoodFeb 21, 2023
  9. Alex HenrieFeb 22, 2023
  10. Alex HenrieFeb 22, 2023
  11. Phillip WoodFeb 22, 2023
  12. Junio C HamanoFeb 22, 2023
  13. Alex HenrieFeb 22, 2023
  14. 4/4 rebase: add a config option for --rebase-mergesAlex Henrie, Feb 21, 2023
  15. Phillip WoodFeb 21, 2023
  16. Alex HenrieFeb 22, 2023
  17. Phillip WoodFeb 21, 2023

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.