Git Mailing List Archive mirror
 help / color / mirror / Atom feed
* [PATCH v3 1/3] rebase: add documentation and tests for --no-rebase-merges
@ 2023-02-22  5:17 Alex Henrie
  2023-02-22  5:17 ` [PATCH v3 2/3] rebase: stop accepting --rebase-merges="" Alex Henrie
  2023-02-22  5:17 ` [PATCH v3 3/3] rebase: add a config option for --rebase-merges Alex Henrie
  0 siblings, 2 replies; 6+ messages in thread
From: Alex Henrie @ 2023-02-22  5:17 UTC (permalink / raw)
  To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin
  Cc: Alex Henrie

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/git-rebase.txt |  4 +++-
 t/t3430-rebase-merges.sh     | 25 +++++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 9a295bcee4..c98784a0d2 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -529,13 +529,15 @@ See also INCOMPATIBLE OPTIONS below.
 
 -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
index fa2a06c19f..e0d910c229 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -250,6 +250,31 @@ test_expect_success 'with a branch tip that was cherry-picked already' '
 	EOF
 '
 
+test_expect_success 'do not rebase merges unless asked to' '
+	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 '--no-rebase-merges countermands --rebase-merges' '
+	git checkout -b no-rebase-merges E &&
+	git rebase --rebase-merges --no-rebase-merges 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)" &&
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 2/3] rebase: stop accepting --rebase-merges=""
  2023-02-22  5:17 [PATCH v3 1/3] rebase: add documentation and tests for --no-rebase-merges Alex Henrie
@ 2023-02-22  5:17 ` Alex Henrie
  2023-02-22 23:56   ` Junio C Hamano
  2023-02-22  5:17 ` [PATCH v3 3/3] rebase: add a config option for --rebase-merges Alex Henrie
  1 sibling, 1 reply; 6+ messages in thread
From: Alex Henrie @ 2023-02-22  5:17 UTC (permalink / raw)
  To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin
  Cc: Alex Henrie

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.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 builtin/rebase.c         | 6 ++----
 t/t3430-rebase-merges.sh | 6 ++++++
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6635f10d52..b68fc2fbb7 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1140,7 +1140,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		{OPTION_STRING, 'r', "rebase-merges", &rebase_merges,
 			N_("mode"),
 			N_("try to rebase merges instead of skipping them"),
-			PARSE_OPT_OPTARG, NULL, (intptr_t)""},
+			PARSE_OPT_OPTARG, NULL, (intptr_t)"no-rebase-cousins"},
 		OPT_BOOL(0, "fork-point", &options.fork_point,
 			 N_("use 'merge-base --fork-point' to refine upstream")),
 		OPT_STRING('s', "strategy", &options.strategy,
@@ -1437,9 +1437,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		imply_merge(&options, "--exec");
 
 	if (rebase_merges) {
-		if (!*rebase_merges)
-			; /* default mode; do nothing */
-		else if (!strcmp("rebase-cousins", rebase_merges))
+		if (!strcmp("rebase-cousins", rebase_merges))
 			options.rebase_cousins = 1;
 		else if (strcmp("no-rebase-cousins", rebase_merges))
 			die(_("Unknown mode: %s"), rebase_merges);
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index e0d910c229..b8ba323dbc 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -293,6 +293,12 @@ 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 'refs/rewritten/* is worktree-local' '
 	git worktree add wt &&
 	cat >wt/script-from-scratch <<-\EOF &&
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* [PATCH v3 3/3] rebase: add a config option for --rebase-merges
  2023-02-22  5:17 [PATCH v3 1/3] rebase: add documentation and tests for --no-rebase-merges Alex Henrie
  2023-02-22  5:17 ` [PATCH v3 2/3] rebase: stop accepting --rebase-merges="" Alex Henrie
@ 2023-02-22  5:17 ` Alex Henrie
  2023-02-23  0:12   ` Junio C Hamano
  1 sibling, 1 reply; 6+ messages in thread
From: Alex Henrie @ 2023-02-22  5:17 UTC (permalink / raw)
  To: git, tao, gitster, newren, phillip.wood123, Johannes.Schindelin
  Cc: Alex Henrie

The purpose of the new option is to accommodate users who would like
--rebase-merges to be on by default and to facilitate possibly turning
on --rebase-merges by default without configuration in a future version
of Git.

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/config/rebase.txt | 10 ++++
 Documentation/git-rebase.txt    |  3 +-
 builtin/rebase.c                | 47 ++++++++++++----
 t/t3430-rebase-merges.sh        | 96 +++++++++++++++++++++++++++++++++
 4 files changed, 144 insertions(+), 12 deletions(-)

diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt
index f19bd0e040..308baa9dbb 100644
--- a/Documentation/config/rebase.txt
+++ b/Documentation/config/rebase.txt
@@ -67,3 +67,13 @@ rebase.rescheduleFailedExec::
 
 rebase.forkPoint::
 	If set to false set `--no-fork-point` option by default.
+
+rebase.merges::
+	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`.
diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index c98784a0d2..b02f9cbb8c 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -537,7 +537,8 @@ 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.merges` config option and 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/builtin/rebase.c b/builtin/rebase.c
index b68fc2fbb7..45cf445d42 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -771,6 +771,20 @@ 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("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 +829,13 @@ static int rebase_config(const char *var, const char *value, void *data)
 		return 0;
 	}
 
+	if (!strcmp(var, "rebase.merges") && value && *value) {
+		opts->rebase_merges = git_parse_maybe_bool(value);
+		if (opts->rebase_merges < 0)
+			parse_merges_value(opts, value);
+		return 0;
+	}
+
 	if (!strcmp(var, "rebase.backend")) {
 		return git_config_string(&opts->default_backend, var, value);
 	}
@@ -980,6 +1001,18 @@ 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)
+{
+	struct rebase_options *options = opt->value;
+
+	if (unset)
+		options->rebase_merges = 0;
+	else
+		parse_merges_value(options, arg);
+
+	return 0;
+}
+
 static void NORETURN error_on_missing_default_upstream(void)
 {
 	struct branch *current_branch = branch_get(NULL);
@@ -1035,7 +1068,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 +1169,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_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,14 +1467,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 	if (options.exec.nr)
 		imply_merge(&options, "--exec");
 
-	if (rebase_merges) {
-		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 b8ba323dbc..88d347cea5 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -299,6 +299,102 @@ test_expect_success '--rebase-merges="" is invalid syntax' '
 	test_cmp expect actual
 '
 
+test_expect_success 'rebase.merges="" is equivalent to not passing --rebase-merges' '
+	test_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' '
+	test_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 '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' '
+	test_config rebase.merges 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.merges=rebase-cousins' '
+	test_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=false' '
+	test_config rebase.merges 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.merges=rebase-cousins' '
+	test_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 'local rebase.merges=false overrides global rebase.merges=true' '
+	test_config_global rebase.merges true &&
+	test_config rebase.merges false &&
+	git checkout -b override-global-config E &&
+	git rebase C &&
+	test_cmp_graph C.. <<-\EOF
+	* B
+	* D
+	o C
+	EOF
+'
+
+test_expect_success 'local rebase.merges="" does not override global rebase.merges=true' '
+	test_config_global rebase.merges no-rebase-cousins &&
+	test_config rebase.merges "" &&
+	git checkout -b no-override-global-config E &&
+	before="$(git rev-parse --verify HEAD)" &&
+	test_tick &&
+	git rebase C &&
+	test_cmp_rev HEAD $before
+'
+
 test_expect_success 'refs/rewritten/* is worktree-local' '
 	git worktree add wt &&
 	cat >wt/script-from-scratch <<-\EOF &&
-- 
2.39.2


^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/3] rebase: stop accepting --rebase-merges=""
  2023-02-22  5:17 ` [PATCH v3 2/3] rebase: stop accepting --rebase-merges="" Alex Henrie
@ 2023-02-22 23:56   ` Junio C Hamano
  2023-02-23  5:35     ` Alex Henrie
  0 siblings, 1 reply; 6+ messages in thread
From: Junio C Hamano @ 2023-02-22 23:56 UTC (permalink / raw)
  To: Alex Henrie; +Cc: git, tao, newren, phillip.wood123, Johannes.Schindelin

Alex Henrie <alexhenrie24@gmail.com> writes:

> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index e0d910c229..b8ba323dbc 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -293,6 +293,12 @@ 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 &&

"!" takes any non-zero exit, even a segfaulting "git".  Let's use
test_must_fail to make sure it dies in a controlled way, i.e.

	test_must_fail git rebase --rebase-merges="" HEAD^ 2>actual &&

> +	test_cmp expect actual
> +'
> +
>  test_expect_success 'refs/rewritten/* is worktree-local' '
>  	git worktree add wt &&
>  	cat >wt/script-from-scratch <<-\EOF &&

Other than that, looking good.

Thanks.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 3/3] rebase: add a config option for --rebase-merges
  2023-02-22  5:17 ` [PATCH v3 3/3] rebase: add a config option for --rebase-merges Alex Henrie
@ 2023-02-23  0:12   ` Junio C Hamano
  0 siblings, 0 replies; 6+ messages in thread
From: Junio C Hamano @ 2023-02-23  0:12 UTC (permalink / raw)
  To: Alex Henrie; +Cc: git, tao, newren, phillip.wood123, Johannes.Schindelin

Alex Henrie <alexhenrie24@gmail.com> writes:

> +rebase.merges::
> +	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`.

OK.  And ...

> +static void parse_merges_value(struct rebase_options *options, const char *value)
> +{
> +	if (value) {
> +		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;
> +}

... this fallback parsing that is called after parse_maybe_bool()
finds the value a non-Boolean ...

> @@ -815,6 +829,13 @@ static int rebase_config(const char *var, const char *value, void *data)
>  		return 0;
>  	}
>  
> +	if (!strcmp(var, "rebase.merges") && value && *value) {
> +		opts->rebase_merges = git_parse_maybe_bool(value);
> +		if (opts->rebase_merges < 0)
> +			parse_merges_value(opts, value);
> +		return 0;
> +	}

... looks pretty much standard way to handle such an "extended
Boolean" value.  Nicely done.

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v3 2/3] rebase: stop accepting --rebase-merges=""
  2023-02-22 23:56   ` Junio C Hamano
@ 2023-02-23  5:35     ` Alex Henrie
  0 siblings, 0 replies; 6+ messages in thread
From: Alex Henrie @ 2023-02-23  5:35 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, tao, newren, phillip.wood123, Johannes.Schindelin

On Wed, Feb 22, 2023 at 4:56 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Henrie <alexhenrie24@gmail.com> writes:
>
> > +test_expect_success '--rebase-merges="" is invalid syntax' '
> > +     echo "fatal: Unknown mode: " >expect &&
> > +     ! git rebase --rebase-merges="" HEAD^ 2>actual &&
>
> "!" takes any non-zero exit, even a segfaulting "git".  Let's use
> test_must_fail to make sure it dies in a controlled way, i.e.
>
>         test_must_fail git rebase --rebase-merges="" HEAD^ 2>actual &&

Done in v4. Thanks for the feedback.

-Alex

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2023-02-23  5:35 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-22  5:17 [PATCH v3 1/3] rebase: add documentation and tests for --no-rebase-merges Alex Henrie
2023-02-22  5:17 ` [PATCH v3 2/3] rebase: stop accepting --rebase-merges="" Alex Henrie
2023-02-22 23:56   ` Junio C Hamano
2023-02-23  5:35     ` Alex Henrie
2023-02-22  5:17 ` [PATCH v3 3/3] rebase: add a config option for --rebase-merges Alex Henrie
2023-02-23  0:12   ` Junio C Hamano

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).