Re: [PATCH v5 1/3] rebase: add documentation and test for --no-rebase-merges

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

Alex Henrie <alexhenrie24@gmail.com> writes:

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>

I inferred from the 'subject line' that this "always" worked and was undocumented, and that we'd want to document this not for its own sake, but because we'll introduce a config option. I would personally prefer to get that explicitly in writing, but it's definitely not worth a reroll.

Show 16 quoted lines

>  -r::
>  --rebase-merges[=(rebase-cousins|no-rebase-cousins)]::
> +--no-rebase-merges::
>  	By default, a rebase will simply drop merge commits from the todo
>  	list, and put the rebased commits into a single, linear branch.
>  	With `--rebase-merges`, the rebase will instead try to preserve
>  	the branching structure within the commits that are to be rebased,
>  	by recreating the merge commits. Any resolved merge conflicts or
>  	manual amendments in these merge commits will have to be
> -	resolved/re-applied manually.
> +	resolved/re-applied manually. `--no-rebase-merges` can be used to
> +	countermand a previous `--rebase-merges`.
>  +
>  By default, or when `no-rebase-cousins` was specified, commits which do not
>  have `<upstream>` as direct ancestor will keep their original branch point,
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh

This isn't the fault of this patch, but I find this description a little confusing. First we say

  By default (*when no option is given*), a rebase will simply drop
  merge commits from the todo list...

and then later we also say

  By default (*when --rebase-merges is provided without a value), or
  when `no-rebase-cousins`...

So although you didn't create this problem, given the changes we're making in the later patches, I think we have the chance to make this description more sensible; I'll elaborate in my response to those patches.