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

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

From
PWPhillip Wood <phillip.wood123@gmail.com>
Date
Mar 7, 2023, 14:56 UTC
Message-ID
<7cf19017-518b-245e-aea2-5dee55f88276@dunelm.org.uk>
In-Reply-To
<20230305050709.68736-4-alexhenrie24@gmail.com>
Hi Alex
On 05/03/2023 05:07, Alex Henrie wrote:
Show 40 quoted lines
> The purpose of the new option is to accommodate users who would like
> --rebase-merges to be on by default and to facilitate turning on
> --rebase-merges by default without configuration in a future version of
> Git.
> 
> Name the new option rebase.rebaseMerges, even though it is a little
> redundant, for consistency with the name of the command line option and
> to be clear when scrolling through values in the [rebase] section of
> .gitconfig.
> 
> In the future, the default rebase-merges mode may change from
> no-rebase-cousins to rebase-cousins. Support setting rebase.rebaseMerges
> to the nonspecific value "true" for users who do not need or want to
> care about the default changing in the future. Similarly, for users who
> have --rebase-merges in an alias and want to get the future behavior
> now, use the specific rebase-merges mode from the config if a specific
> mode is not given on the command line.
> 
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   Documentation/config/rebase.txt        | 11 ++++
>   Documentation/git-rebase.txt           | 17 +++---
>   builtin/rebase.c                       | 79 ++++++++++++++++++--------
>   t/t3422-rebase-incompatible-options.sh | 17 ++++++
>   t/t3430-rebase-merges.sh               | 68 ++++++++++++++++++++++
>   5 files changed, 161 insertions(+), 31 deletions(-)
> 
> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
> index f19bd0e040..f7d3218b1d 100644
> --- a/Documentation/config/rebase.txt
> +++ b/Documentation/config/rebase.txt
> @@ -67,3 +67,14 @@ rebase.rescheduleFailedExec::
>   
>   rebase.forkPoint::
>   	If set to false set `--no-fork-point` option by default.
> +
> +rebase.rebaseMerges::
> +	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,

This is a bit picky but how can rebase.rebaseMerges=true be equivalent to --rebase-merges without an argument when the behavior of --rebase-merges without an argument depends on the value of rebase.rebaseMerges?

Show 7 quoted lines
> 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.rebaseMerges=false`
> +	configuration, but the absence of a specific rebase-merges mode on the
> +	command line does not counteract a specific mode set in the configuration.

I may not agree the with the design choice but the documentation here and below is very clear about the behavior of --rebase-merges without an argument which is good.

Show 27 quoted lines
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 4e57a87624..6ec86c9c6e 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -537,16 +537,17 @@ See also INCOMPATIBLE OPTIONS below.
>   	by recreating the merge commits. Any resolved merge conflicts or
>   	manual amendments in these merge commits will have to be
>   	resolved/re-applied manually. `--no-rebase-merges` can be used to
> -	countermand a previous `--rebase-merges`.
> +	countermand both the `rebase.rebaseMerges` config option and a previous
> +	`--rebase-merges`.
>   +
>   When rebasing merges, there are two modes: `rebase-cousins` and
> -`no-rebase-cousins`. If the mode is not specified, it defaults to
> -`no-rebase-cousins`. In `no-rebase-cousins` mode, commits which do not have
> -`<upstream>` as direct ancestor will keep their original branch point, i.e.
> -commits that would be excluded by linkgit:git-log[1]'s `--ancestry-path`
> -option will keep their original ancestry by default. In `rebase-cousins` mode,
> -such commits are instead rebased onto `<upstream>` (or `<onto>`, if
> -specified).
> +`no-rebase-cousins`. If the mode is not specified on the command line or in
> +the `rebase.rebaseMerges` config option, it defaults to `no-rebase-cousins`.
> +In `no-rebase-cousins` mode, commits which do not have `<upstream>` as direct
> +ancestor will keep their original branch point, i.e. commits that would be
> +excluded by linkgit:git-log[1]'s `--ancestry-path` option will keep their
> +original ancestry by default. In `rebase-cousins` mode, such commits are
> +instead rebased onto `<upstream>` (or `<onto>`, if specified).
 >[...]
Show 13 quoted lines
>   static int rebase_config(const char *var, const char *value, void *data)
>   {
>   	struct rebase_options *opts = data;
> @@ -800,6 +813,15 @@ static int rebase_config(const char *var, const char *value, void *data)
>   		return 0;
>   	}
>   
> +	if (!strcmp(var, "rebase.rebasemerges")) {
> +		opts->config_rebase_merges = git_parse_maybe_bool(value);
> +		if (opts->config_rebase_merges < 0) {
> +			opts->config_rebase_merges = 1;
> +			parse_rebase_merges_value(opts, value);
> +		}
I think we need
	} else {
		opts->rebase_cousins = 0;
	}

here. Otherwise if rebase.rebaseMerges is set twice we wont follow the usual "last config wins" convention. For example

	[rebase]
		rebaseMerges=rebase-cousins
		rebaseMerges=true
will result in us unexpectedly rebasing cousins
The rest of the patch looks fine
Best Wishes
Phillip
Show 230 quoted lines
> +		return 0;
> +	}
> +
>   	if (!strcmp(var, "rebase.updaterefs")) {
>   		opts->config_update_refs = git_config_bool(var, value);
>   		return 0;
> @@ -980,6 +1002,27 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)
>   	return 0;
>   }
>   
> +static int parse_opt_rebase_merges(const struct option *opt, const char *arg, int unset)
> +{
> +	struct rebase_options *options = opt->value;
> +
> +	options->rebase_merges = !unset;
> +
> +	if (arg) {
> +		if (!*arg) {
> +			warning(_("--rebase-merges with an empty string "
> +				  "argument is deprecated and will stop "
> +				  "working in a future version of Git. Use "
> +				  "--rebase-merges without an argument "
> +				  "instead, which does the same thing."));
> +			return 0;
> +		}
> +		parse_rebase_merges_value(options, arg);
> +	}
> +
> +	return 0;
> +}
> +
>   static void NORETURN error_on_missing_default_upstream(void)
>   {
>   	struct branch *current_branch = branch_get(NULL);
> @@ -1035,7 +1078,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 +1179,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)"no-rebase-cousins"},
> +			PARSE_OPT_OPTARG, parse_opt_rebase_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,21 +1477,6 @@ 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)
> -			warning(_("--rebase-merges with an empty string "
> -				  "argument is deprecated and will stop "
> -				  "working in a future version of Git. Use "
> -				  "--rebase-merges without an argument "
> -				  "instead, which does the same thing."));
> -		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;
> -		imply_merge(&options, "--rebase-merges");
> -	}
> -
>   	if (options.type == REBASE_APPLY) {
>   		if (ignore_whitespace)
>   			strvec_push(&options.git_am_opts,
> @@ -1513,13 +1539,15 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   				break;
>   
>   		if (i >= 0 || options.type == REBASE_APPLY) {
> -			if (is_merge(&options))
> -				die(_("apply options and merge options "
> -					  "cannot be used together"));
> -			else if (options.autosquash == -1 && options.config_autosquash == 1)
> +			if (options.autosquash == -1 && options.config_autosquash == 1)
>   				die(_("apply options are incompatible with rebase.autosquash.  Consider adding --no-autosquash"));
> +			else if (options.rebase_merges == -1 && options.config_rebase_merges == 1)
> +				die(_("apply options are incompatible with rebase.rebaseMerges.  Consider adding --no-rebase-merges"));
>   			else if (options.update_refs == -1 && options.config_update_refs == 1)
>   				die(_("apply options are incompatible with rebase.updateRefs.  Consider adding --no-update-refs"));
> +			else if (is_merge(&options))
> +				die(_("apply options and merge options "
> +					  "cannot be used together"));
>   			else
>   				options.type = REBASE_APPLY;
>   		}
> @@ -1530,6 +1558,11 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>   	options.update_refs = (options.update_refs >= 0) ? options.update_refs :
>   			     ((options.config_update_refs >= 0) ? options.config_update_refs : 0);
>   
> +	if (options.rebase_merges == 1)
> +		imply_merge(&options, "--rebase-merges");
> +	options.rebase_merges = (options.rebase_merges >= 0) ? options.rebase_merges :
> +				((options.config_rebase_merges >= 0) ? options.config_rebase_merges : 0);
> +
>   	if (options.autosquash == 1)
>   		imply_merge(&options, "--autosquash");
>   	options.autosquash = (options.autosquash >= 0) ? options.autosquash :
> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh
> index 4711b37a28..2eba00bdf5 100755
> --- a/t/t3422-rebase-incompatible-options.sh
> +++ b/t/t3422-rebase-incompatible-options.sh
> @@ -85,6 +85,11 @@ test_rebase_am_only () {
>   		test_must_fail git rebase $opt --reapply-cherry-picks A
>   	"
>   
> +	test_expect_success "$opt incompatible with --rebase-merges" "
> +		git checkout B^0 &&
> +		test_must_fail git rebase $opt --rebase-merges A
> +	"
> +
>   	test_expect_success "$opt incompatible with --update-refs" "
>   		git checkout B^0 &&
>   		test_must_fail git rebase $opt --update-refs A
> @@ -101,6 +106,12 @@ test_rebase_am_only () {
>   		grep -e --no-autosquash err
>   	"
>   
> +	test_expect_success "$opt incompatible with rebase.rebaseMerges" "
> +		git checkout B^0 &&
> +		test_must_fail git -c rebase.rebaseMerges=true rebase $opt A 2>err &&
> +		grep -e --no-rebase-merges err
> +	"
> +
>   	test_expect_success "$opt incompatible with rebase.updateRefs" "
>   		git checkout B^0 &&
>   		test_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&
> @@ -113,6 +124,12 @@ test_rebase_am_only () {
>   		git -c rebase.autosquash=true rebase --no-autosquash $opt A
>   	"
>   
> +	test_expect_success "$opt okay with overridden rebase.rebaseMerges" "
> +		test_when_finished \"git reset --hard B^0\" &&
> +		git checkout B^0 &&
> +		git -c rebase.rebaseMerges=true rebase --no-rebase-merges $opt A
> +	"
> +
>   	test_expect_success "$opt okay with overridden rebase.updateRefs" "
>   		test_when_finished \"git reset --hard B^0\" &&
>   		git checkout B^0 &&
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index d46d9545f2..aa75e192d1 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -278,6 +278,74 @@ test_expect_success 'do not rebase cousins unless asked for' '
>   	EOF
>   '
>   
> +test_expect_success '--rebase-merges="" is deprecated' '
> +	git rebase --rebase-merges="" HEAD^ 2>actual &&
> +	grep deprecated actual
> +'
> +
> +test_expect_success 'rebase.rebaseMerges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '
> +	test_config rebase.rebaseMerges 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 '--no-rebase-merges overrides rebase.rebaseMerges=no-rebase-cousins' '
> +	test_config rebase.rebaseMerges no-rebase-cousins &&
> +	git checkout -b override-config-no-rebase-cousins E &&
> +	git rebase --no-rebase-merges C &&
> +	test_cmp_graph C.. <<-\EOF
> +	* B
> +	* D
> +	o C
> +	EOF
> +'
> +
> +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.rebaseMerges=rebase-cousins' '
> +	test_config rebase.rebaseMerges 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.rebaseMerges=false' '
> +	test_config rebase.rebaseMerges false &&
> +	git checkout -b override-config-merges-false E &&
> +	before="$(git rev-parse --verify HEAD)" &&
> +	test_tick &&
> +	git rebase --rebase-merges C &&
> +	test_cmp_rev HEAD $before
> +'
> +
> +test_expect_success '--rebase-merges does not override rebase.rebaseMerges=rebase-cousins' '
> +	test_config rebase.rebaseMerges 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 &&
Previous: Alex HenrieNext: Junio C Hamano
Message 47 of 96 in “rebase: add documentation and test for --no-rebase-merges”
  1. 1/3 rebase: add documentation and test for --no-rebase-mergesAlex Henrie, Feb 23, 2023
  2. 2/3 rebase: stop accepting --rebase-merges=""Alex Henrie, Feb 23, 2023
  3. Johannes SchindelinFeb 24, 2023
  4. Junio C HamanoFeb 24, 2023
  5. Alex HenrieFeb 24, 2023
  6. Junio C HamanoFeb 24, 2023
  7. Alex HenrieFeb 24, 2023
  8. Junio C HamanoFeb 24, 2023
  9. Alex HenrieFeb 24, 2023
  10. Junio C HamanoFeb 24, 2023
  11. Alex HenrieFeb 24, 2023
  12. Phillip WoodFeb 24, 2023
  13. Alex HenrieFeb 24, 2023
  14. 3/3 rebase: add a config option for --rebase-mergesAlex Henrie, Feb 23, 2023
  15. Johannes SchindelinFeb 24, 2023
  16. Alex HenrieFeb 24, 2023
  17. Phillip WoodFeb 24, 2023
  18. Alex HenrieFeb 24, 2023
  19. Junio C HamanoFeb 23, 2023
  20. Johannes SchindelinFeb 24, 2023
  21. Junio C HamanoFeb 24, 2023
  22. Alex HenrieFeb 25, 2023
  23. 0/3 rebase: add a config option for --rebase-mergesAlex Henrie, Feb 25, 2023
  24. 1/3 rebase: add documentation and test for --no-rebase-mergesAlex Henrie, Feb 25, 2023
  25. Glen ChooMar 1, 2023
  26. 2/3 rebase: deprecate --rebase-merges=""Alex Henrie, Feb 25, 2023
  27. Glen ChooMar 1, 2023
  28. Phillip WoodMar 2, 2023
  29. Calvin WanMar 2, 2023
  30. 3/3 rebase: add a config option for --rebase-mergesAlex Henrie, Feb 25, 2023
  31. Glen ChooMar 1, 2023
  32. Phillip WoodMar 2, 2023
  33. Alex HenrieMar 4, 2023
  34. Phillip WoodMar 7, 2023
  35. Alex HenrieMar 12, 2023
  36. Phillip WoodMar 13, 2023
  37. Felipe ContrerasMar 13, 2023
  38. Junio C HamanoMar 13, 2023
  39. About replaying "evil" merges... Re: [PATCH v5 3/3] rebase: add a config option for --rebase-mergesJohannes Schindelin, Mar 24, 2023
  40. Calvin WanMar 2, 2023
  41. Alex HenrieMar 4, 2023
  42. Glen ChooMar 1, 2023
  43. Alex HenrieMar 2, 2023
  44. Alex HenrieMar 2, 2023
  45. 0/3 rebase: document, clean up, and introduce a config option for --rebase-mergesAlex Henrie, Mar 5, 2023
  46. 3/3 rebase: add a config option for --rebase-mergesAlex Henrie, Mar 5, 2023
  47. Phillip WoodMar 7, 2023
  48. Junio C HamanoMar 7, 2023
  49. Alex HenrieMar 12, 2023
  50. Glen ChooMar 8, 2023
  51. Glen ChooMar 8, 2023
  52. Alex HenrieMar 12, 2023
  53. Alex HenrieMar 15, 2023
  54. Glen ChooMar 16, 2023
  55. Felipe ContrerasMar 16, 2023
  56. Glen ChooMar 16, 2023
  57. Felipe ContrerasMar 16, 2023
  58. Alex HenrieMar 16, 2023
  59. Glen ChooMar 16, 2023
  60. Alex HenrieMar 18, 2023
  61. Johannes SchindelinMar 24, 2023
  62. Sergey OrganovMar 25, 2023
  63. 1/3 rebase: add documentation and test for --no-rebase-mergesAlex Henrie, Mar 5, 2023
  64. Sergey OrganovMar 8, 2023
  65. 2/3 rebase: deprecate --rebase-merges=""Alex Henrie, Mar 5, 2023
  66. Phillip WoodMar 7, 2023
  67. Sergey OrganovMar 5, 2023
  68. Alex HenrieMar 5, 2023
  69. Sergey OrganovMar 5, 2023
  70. Alex HenrieMar 6, 2023
  71. Sergey OrganovMar 6, 2023
  72. Junio C HamanoMar 6, 2023
  73. Junio C HamanoMar 6, 2023
  74. Phillip WoodMar 6, 2023
  75. Alex HenrieMar 6, 2023
  76. Phillip WoodMar 7, 2023
  77. Glen ChooMar 8, 2023
  78. 0/3 rebase: document, clean up, and introduce a config option for --rebase-mergesAlex Henrie, Mar 12, 2023
  79. 1/3 rebase: add documentation and test for --no-rebase-mergesAlex Henrie, Mar 12, 2023
  80. 2/3 rebase: deprecate --rebase-merges=""Alex Henrie, Mar 12, 2023
  81. 3/3 rebase: add a config option for --rebase-mergesAlex Henrie, Mar 12, 2023
  82. 0/3 rebase: document, clean up, and introduce a config option for --rebase-mergesAlex Henrie, Mar 20, 2023
  83. 1/3 rebase: add documentation and test for --no-rebase-mergesAlex Henrie, Mar 20, 2023
  84. 2/3 rebase: deprecate --rebase-merges=""Alex Henrie, Mar 20, 2023
  85. 3/3 rebase: add a config option for --rebase-mergesAlex Henrie, Mar 20, 2023
  86. Phillip WoodMar 22, 2023
  87. Junio C HamanoMar 23, 2023
  88. Phillip WoodMar 24, 2023
  89. Alex HenrieMar 25, 2023
  90. Alex HenrieMar 25, 2023
  91. 0/3 rebase: document, clean up, and introduce a config option for --rebase-mergesAlex Henrie, Mar 26, 2023
  92. 1/3 rebase: add documentation and test for --no-rebase-mergesAlex Henrie, Mar 26, 2023
  93. 2/3 rebase: deprecate --rebase-merges=""Alex Henrie, Mar 26, 2023
  94. 3/3 rebase: add a config option for --rebase-mergesAlex Henrie, Mar 26, 2023
  95. Phillip WoodMar 26, 2023
  96. Junio C HamanoMar 27, 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.