Re: [PATCH v5 3/3] rebase: add a config option for --rebase-merges

From: Glen Choo <chooglen@google.com>
Date: Mar 1, 2023, 23:43 UTC
Message-ID: <kl6lpm9s3unl.fsf@chooglen-macbookpro.roam.corp.google.com>
In-Reply-To: <20230225180325.796624-4-alexhenrie24@gmail.com>

Alex Henrie <alexhenrie24@gmail.com> writes:

> +rebase.merges::

As far as I can tell, when a config option is meant to be a stand in for a CLI option, we have typically named it <subcommand>.<camelcase option>, e.g. grep.fullName, log.diffMerges, push.followTags. This probably matters more if we aren't specifying the subcommand that this flag is more (like in this patch).

By this convention, the config option would be rebase.rebaseMerges, which _does_ feel redundant and it's unlikely for "git rebase" to learn a "--merges" flag, so I don't have a strong opinion either way. Though as you mentioned in review club, this consistency might make it easier for users to read custom configs that use the "rebase." section.

Show 8 quoted lines

> +	Whether and how to set the `--rebase-merges` option by default. Can
> +	be `rebase-cousins`, `no-rebase-cousins`, or a boolean. Setting to
> +	true is equivalent to `--rebase-merges` without an argument, setting to
> +	`rebase-cousins` or `no-rebase-cousins` is equivalent to
> +	`--rebase-merges` with that value as its argument, and setting to false
> +	is equivalent to `--no-rebase-merges`. Passing `--rebase-merges` on the
> +	command line without an argument overrides a `rebase.merges=false`
> +	configuration but does not override other values of `rebase.merge`.

With the explanation here, I think having "--rebase-merges" _not_ override "rebase.merge=rebase-cousins" is quite confusing.

I think there are two possibilities forward:

- Be very consistent that "--rebase-merges" is just a synonym of
  "--rebase-merges=no-rebase-cousins". Then, have any value of
  "--rebase-merges" override any config value.

  I think this the easiest UX to understand, but I don't think we should
  be commiting to a default, especially since we may add a more sensible
  default (like rebasing 'evil merges') in the future.

- Keep the existing behavior, but reword the docs to indicate that the
  "no value" form means "I want the default but don't care what it is",
  and an explicit value means "I want to use a particular mode and the
  rules of config vs CLI specificity should apply". Perhaps something
  like:

    When rebasing merges, there are two modes: `no-rebase-cousins` and
    `rebase-cousins`. If no mode is specified in either the config or
    CLI, it defaults to `no-rebase-cousins`.

   which I think is clearer that "rebase.merges=some-val" will win out
   over "--rebase-merges". What's nice is that we don't commit to a
   default value, so we can change it without having to update much of
   the wording here.

The code looks good, no real comments there.