patch, 2 partsrebase: add a --rebase-merges=drop option

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   Documentation/config/rebase.txt |  3 ++
>   builtin/rebase.c                | 50 ++++++++++++++-----
>   t/t3430-rebase-merges.sh        | 87 +++++++++++++++++++++++++++++++++
>   3 files changed, 127 insertions(+), 13 deletions(-)
> 
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index f19bd0e040..d956ec4441 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -67,3 +67,6 @@ rebase.rescheduleFailedExec::
>   
>   rebase.forkPoint::
>   	If set to false set `--no-fork-point` option by default.
> +
> +rebase.merges::
> +	Default value of `--rebase-merges` option.
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 96c0474379..ab4c3b2870 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -771,6 +771,25 @@ static int run_specific_rebase(struct rebase_options *opts)
>   	return status ? -1 : 0;
>   }
>   
> +static void parse_merges_value(struct rebase_options *options, const char *value)
> +{
> +	if (value) {
> +		if (!strcmp("drop", value)) {
> +			options->rebase_merges = 0;
> +			return;
> +		}
> +
> +		if (!strcmp("no-rebase-cousins", value))
> +			options->rebase_cousins = 0;
> +		else if (!strcmp("rebase-cousins", value))
> +			options->rebase_cousins = 1;
> +		else
> +			die(_("Unknown mode: %s"), value);
> +	}
> +
> +	options->rebase_merges = 1;
> +}
> +
>   static int rebase_config(const char *var, const char *value, void *data)
>   {
>   	struct rebase_options *opts = data;
> @@ -815,6 +834,14 @@ static int rebase_config(const char *var, const char *value, void *data)
>   		return 0;
>   	}
>   
> +	if (!strcmp(var, "rebase.merges")) {
> +		const char *rebase_merges;
> +		if (!git_config_string(&rebase_merges, var, value) &&
> +		    rebase_merges && *rebase_merges)
> +			parse_merges_value(opts, rebase_merges);
> +		return 0;
> +	}
> +
>   	if (!strcmp(var, "rebase.backend")) {
>   		return git_config_string(&opts->default_backend, var, value);
>   	}
> @@ -980,6 +1007,13 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
>   	return 0;
>   }
>   
> +static int parse_opt_merges(const struct option *opt, const char *arg, int unset)
> +{
> +	BUG_ON_OPT_NEG(unset);
> +	parse_merges_value(opt->value, arg);
> +	return 0;
> +}
> +
>   static void NORETURN error_on_missing_default_upstream(void)
>   {
>   	struct branch *current_branch = branch_get(NULL);
> @@ -1035,7 +1069,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	struct object_id branch_base;
>   	int ignore_whitespace = 0;
>   	const char *gpg_sign = NULL;
> -	const char *rebase_merges = NULL;
>   	struct string_list strategy_options = STRING_LIST_INIT_NODUP;
>   	struct object_id squash_onto;
>   	char *squash_onto_name = NULL;
> @@ -1137,10 +1170,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   			   &options.allow_empty_message,
>   			   N_("allow rebasing commits with empty messages"),
>   			   PARSE_OPT_HIDDEN),
> -		{OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
> -			N_("mode"),
> +		OPT_CALLBACK_F('r', "rebase-merges", &options, N_("mode"),
>   			N_("try to rebase merges instead of skipping them"),
> -			PARSE_OPT_OPTARG, NULL, (intptr_t)""},
> +			PARSE_OPT_OPTARG, parse_opt_merges),
>   		OPT_BOOL(0, "fork-point", &options.fork_point,
>   			 N_("use 'merge-base --fork-point' to refine upstream")),
>   		OPT_STRING('s', "strategy", &options.strategy,
> @@ -1436,16 +1468,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	if (options.exec.nr)
>   		imply_merge(&options, "--exec");
>   
> -	if (rebase_merges && strcmp("drop", rebase_merges)) {
> -		if (!*rebase_merges)
> -			; /* default mode; do nothing */
> -		else if (!strcmp("rebase-cousins", rebase_merges))
> -			options.rebase_cousins = 1;
> -		else if (strcmp("no-rebase-cousins", rebase_merges))
> -			die(_("Unknown mode: %s"), rebase_merges);
> -		options.rebase_merges = 1;
> +	if (options.rebase_merges)
>   		imply_merge(&options, "--rebase-merges");
> -	}
>   
>   	if (options.type == REBASE_APPLY) {
>   		if (ignore_whitespace)
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index 861c8405f2..9be07249cc 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -298,6 +298,92 @@ test_expect_success 'do not rebase cousins unless asked for' '
>   	EOF
>   '
>   
> +test_expect_success '--rebase-merges="" is invalid syntax' '
> +	echo "fatal: Unknown mode: " >expect &&
> +	! git rebase --rebase-merges="" HEAD^ 2>actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'rebase.merges="" is equivalent to not passing --rebase-merges' '
> +	git config rebase.merges "" &&
> +	git checkout -b config-merges-blank E &&
> +	git rebase C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
> +test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
> +	git config rebase.merges rebase-cousins &&
> +	git checkout -b config-rebase-cousins main &&
> +	git rebase HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	|/
> +	o H
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges=drop overrides rebase.merges=no-rebase-cousins' '
> +	git config rebase.merges no-rebase-cousins &&
> +	git checkout -b override-config-no-rebase-cousins E &&
> +	git rebase --rebase-merges=drop C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' '
> +	git config rebase.merges rebase-cousins &&
> +	git checkout -b override-config-rebase-cousins main &&
> +	git rebase --rebase-merges=no-rebase-cousins HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	o | H
> +	|/
> +	o A
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges overrides rebase.merges=drop' '
> +	git config rebase.merges drop &&
> +	git checkout -b override-config-merges-drop main &&
> +	git rebase --rebase-merges HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	o | H
> +	|/
> +	o A
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' '
> +	git config rebase.merges rebase-cousins &&
> +	git checkout -b no-override-config-rebase-cousins main &&
> +	git rebase --rebase-merges HEAD^ &&
> +	test_cmp_graph HEAD^.. <<-\EOF
> +	*   Merge the topic branch '\''onebranch'\''
> +	|\
> +	| * D
> +	| * G
> +	|/
> +	o H
> +	EOF
> +'
> +
>   test_expect_success 'refs/rewritten/* is worktree-local' '
>   	git worktree add wt &&
>   	cat >wt/script-from-scratch <<-\EOF &&
> @@ -408,6 +494,7 @@ test_expect_success 'a "merge" into a root commit is a fast-forward' '
>   '
>   
>   test_expect_success 'A root commit can be a cousin, treat it that way' '
> +	git config --unset rebase.merges &&
>   	git checkout --orphan khnum &&
>   	test_commit yama &&
>   	git checkout -b asherah main &&

>
> On 20/02/2023 03:32, Alex Henrie wrote:
>
> I think the commit message could benefit from some justification for why
> this config option is useful. I don't object to it being added but you
> need to make the case for why it is a good idea.

> > At the same time, stop accepting --rebase-merges="" as a synonym of
> > --rebase-merges=no-rebase-cousins.
>
> Please try to avoid combining unrelated changes in the same patch. I
> agree that accepting an empty argument to mean "no-rebase-cousins" is
> slightly odd but as that is the default I'm not sure it is doing any harm.

> Name the new option "drop" intead of "no" or "false" to avoid confusion > in the future if --rebase-merges grows the ability to truly "rebase"
> merge commits by reusing the conflict resolution information from the
> original merge commit, and we want to add an option to ignore the
> conflict resolution information.
> 
> This option can be used to countermand a previous --rebase-merges
> option.

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   Documentation/git-rebase.txt |  2 +-
>   builtin/rebase.c             |  2 +-
>   t/t3430-rebase-merges.sh     | 30 ++++++++++++++++++++++++++++++
>   3 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 9a295bcee4..92e90f96aa 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -528,7 +528,7 @@ have the long commit hash prepended to the format.
>   See also INCOMPATIBLE OPTIONS below.
>   
>   -r::
> ---rebase-merges[=(rebase-cousins|no-rebase-cousins)]::
> +--rebase-merges[=(rebase-cousins|no-rebase-cousins|drop)]::
>   	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
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6635f10d52..96c0474379 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1436,7 +1436,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	if (options.exec.nr)
>   		imply_merge(&options, "--exec");
>   
> -	if (rebase_merges) {
> +	if (rebase_merges && strcmp("drop", rebase_merges)) {
>   		if (!*rebase_merges)
>   			; /* default mode; do nothing */
>   		else if (!strcmp("rebase-cousins", rebase_merges))
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index fa2a06c19f..861c8405f2 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -250,6 +250,36 @@ test_expect_success 'with a branch tip that was cherry-picked already' '
>   	EOF
>   '
>   
> +test_expect_success 'do not rebase merges unless asked' '
> +	git checkout -b rebase-merges-default E &&
> +	before="$(git rev-parse --verify HEAD)" &&
> +	test_tick &&
> +	git rebase --rebase-merges C &&
> +	test_cmp_rev HEAD $before &&
> +	test_tick &&
> +	git rebase C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
> +test_expect_success 'do not rebase merges when asked to drop them' '
> +	git checkout -b rebase-merges-drop E &&
> +	before="$(git rev-parse --verify HEAD)" &&
> +	test_tick &&
> +	git rebase --rebase-merges C &&
> +	test_cmp_rev HEAD $before &&
> +	test_tick &&
> +	git rebase --rebase-merges=drop C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
>   test_expect_success 'do not rebase cousins unless asked for' '
>   	git checkout -b cousins main &&
>   	before="$(git rev-parse --verify HEAD)" &&

>
> On 20/02/2023 03:32, Alex Henrie wrote:
> > Name the new option "drop" intead of "no" or "false" to avoid confusion > in the future if --rebase-merges grows the ability to truly "rebase"
> > merge commits by reusing the conflict resolution information from the
> > original merge commit, and we want to add an option to ignore the
> > conflict resolution information.
> >
> > This option can be used to countermand a previous --rebase-merges
> > option.
>
> I'm a bit confused as to the reason for this change - what's the
> advantage over just saying --no-rebase-merges which already exists?

> Alex Henrie <alexhenrie24@gmail.com> writes:
>
>> Name the new option "drop" intead of "no" or "false" to avoid confusion
> This is traditionally called "flattening the history".  Don't we
> confuse uesrs by introducing a new phrase?

>
> rebase-merges is about transplanting the history without flattening,
> i.e. keeping the mergy commit graph topology.  If there are only two
> kinds of rebase (i.e. keeping the topology which is rebase-merges
> and the other "flattening" kind) operation, shouldn't the option be
> called "--no-rebase-merges" instead?  --rebase-merges=no is also
> understandable.
>
>> in the future if --rebase-merges grows the ability to truly "rebase"
>> merge commits by reusing the conflict resolution information from the
>> original merge commit, and we want to add an option to ignore the
>> conflict resolution information.
> I am not sure why such a change "in the future" is not merely a
> bugfix of the current "--rebase-merges", though.  Once it is fixed,
> is there a reason to make the fixed behaviour only available behind
> an option?

> +[[def_flatten]]flatten::
> +	Flattening is a common term for the 'linearizing' of a
> +	selected portion of the <<def_commit_graph_general,commit graph>>.
> +	Flattening may include excluding commits, or rearranging commits,
> +	for the linearized sequence.

> Philip Oakley <philipoakley@iee.email> writes:
>
>> +[[def_flatten]]flatten::
>> +	Flattening is a common term for the 'linearizing' of a
>> +	selected portion of the <<def_commit_graph_general,commit graph>>.
>> +	Flattening may include excluding commits, or rearranging commits,
>> +	for the linearized sequence.
> Thanks for writing.  I agree that it is a good idea to define the
> verb "flatten".  The above I agree with 100%.
>
> I think I was one of the first ones who used the verb in the context
> of Git; what I wanted to convey with the verb was what it happens
> when you use "am" to rebuild some history made into a series of
> patches using the "format-patch" command on a part of the history.

>> +	In particular, linkgit:git-log[1] and linkgit:git-show[1] have a
>> +	range of "History Simplification" techniques that affect which
>> +	commits are included, and how they are linearized.
> I didn't think (and I do not yet agree, but I may change my mind
> after thinking about it further) that the history simplification had
> much to do with flattening.

>
> Even after a history is simplified (in the sense how rev-list family
> of commands do so), there will still be merge commits left if both
> branches contribute something to the end result.  So unless a merge
> is to cauterize the side branch (i.e. in order to record the fact
> that we already have everything we may want possibly merge to the
> integration branch from the side branch, we create a merge commit
> that merges the branch but does not change the tree from the parent
> commit on the integration branch), history simplification may not
> contribute to "excluding" commits.

>
>> +	The default linkgit:git-rebase[1] will drop merge commits when it
>> +	flattens history, which also may be unexpected.
> I am tempted to suggest dropping ", which also may be unexpected"
> here.  When learning a new system, there may be things a learner may
> not expect (that is why we have documents),

>
> And in this case, the reason may be obvious and it is OK to be left
> unsaid---"git rebase" (without an option to keep merge commits) was
> designed to be a way to flatten history, and a flattened history by
> definition cannot have any merge commits in it.

> +[[def_flatten]]flatten::
> +	Flattening is a common term for the 'linearizing' of a
> +	selected portion of the <<def_commit_graph_general,commit graph>>.
> +	Flattening may include excluding commits, or rearranging commits,
> +	for the linearized sequence.
> +	In particular, linkgit:git-log[1] and linkgit:git-show[1] have a
> +	range of "History Simplification" techniques that affect which
> +	commits are included, and how they are linearized.
> +	The default linkgit:git-rebase[1] will drop merge commits when it
> +	flattens history, which also may be unexpected.
> +	The two common linearization types are chronological (date-time), and
> +	topological (shape) based orderings. Generation numbering is topological.

> Hi
> 
> On Sat, May 13, 2023, at 18:56, Philip Oakley wrote:
>> Clarify the term 'flatten' and the unexpected effects that the user
>> may come across, such as discussed in [1] and [2].
> 
> Nice to see this effort. I would like more “labels” such as this one to
> conceptualize things because sometimes it feels that Git concepts are
> just handled bottom-up.

>  Specifically in the case of rebase it seems that
> (judging by things like StackOverflow) the pedagogy amounts to
> explaining how rebase *works* (without factoring in `--rebase-merges`)
> and then explaining how that in turn means that a linearization kind of
> “falls out” of that process. And then it seems that you are expected to
> remember that bottom-up explanation without putting any kind of label on
> it; it’s just what it is.

> 
>> +[[def_flatten]]flatten::
>> +	Flattening is a common term for the 'linearizing' of a
>> +	selected portion of the <<def_commit_graph_general,commit graph>>.
>> +	Flattening may include excluding commits, or rearranging commits,
>> +	for the linearized sequence.
>> +	In particular, linkgit:git-log[1] and linkgit:git-show[1] have a
>> +	range of "History Simplification" techniques that affect which
>> +	commits are included, and how they are linearized.
>> +	The default linkgit:git-rebase[1] will drop merge commits when it
>> +	flattens history, which also may be unexpected.
>> +	The two common linearization types are chronological (date-time), and
>> +	topological (shape) based orderings. Generation numbering is topological.
> 
> When I first read this I thought, ah, so this is an explanation of how
> linearized rebases are born. But this part also mentions history
> viewing. Then I thought: does my history viewing (git-log(1)) work the
> same as shuffling around changes into new (and linearized) commits? And
> can git-rebase-(1) move between chronological and topological and
> ordering? 

>  But these two things feel different to me (just a feeling,
> UX-wise). So after reading this I am left wondering if different parts
> of this paragraph apply *only* to history viewing and to rebase
> (“rewriting”).
> 
> Again, this is just how I immediately read this paragraph as a user.