[PATCH v2 1/4] rebase: document the --no-rebase-merges option

[PATCH v2 1/4] rebase: document the --no-rebase-merges option

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 Documentation/git-rebase.txt | 4 +++-
 1 file changed, 3 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,
-- 
2.39.2

[PATCH v2 2/4] rebase: add tests for --no-rebase-merges

[Alex Henrie]

Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
---
 t/t3430-rebase-merges.sh | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

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

Re: [PATCH v2 2/4] rebase: add tests for --no-rebase-merges

[Phillip Wood]

Hi Alex

Thanks for adding some tests,

On 21/02/2023 05:58, Alex Henrie wrote:
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   t/t3430-rebase-merges.sh | 25 +++++++++++++++++++++++++
>   1 file changed, 25 insertions(+)
> 
> 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 &&

I don't quite follow what this part of the test is for

> +	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
> +'

This test looks good. I think we could probably have just this second 
test and squash this into the previous patch - improving the 
documentation and test coverage for --no-rebase-merges would make a nice 
self-contained commit.

Best Wishes

Phillip

>   test_expect_success 'do not rebase cousins unless asked for' '
>   	git checkout -b cousins main &&
>   	before="$(git rev-parse --verify HEAD)" &&

Re: [PATCH v2 2/4] rebase: add tests for --no-rebase-merges

[Alex Henrie]

On Tue, Feb 21, 2023 at 4:00 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 21/02/2023 05:58, Alex Henrie wrote:
> > +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 &&
>
> I don't quite follow what this part of the test is for

The test is modeled after the existing test "do not rebase cousins
unless asked for". First, it verifies that --rebase-merges rebases the
merges, which in this case results in no changes to the branch. Then,
it verifies that `git rebase` without arguments flattens the history.

> > +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
> > +'
>
> This test looks good. I think we could probably have just this second
> test

I like having a test for the default behavior. Nevertheless, I am
happy to remove that test as requested. Does anyone else have an
opinion?

> and squash this into the previous patch - improving the
> documentation and test coverage for --no-rebase-merges would make a nice
> self-contained commit.

Sure, squashing the first two patches is no problem.

Thanks for the feedback,

-Alex

Re: [PATCH v2 2/4] rebase: add tests for --no-rebase-merges

[Phillip Wood]

Hi Alex

On 22/02/2023 01:37, Alex Henrie wrote:
> On Tue, Feb 21, 2023 at 4:00 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 21/02/2023 05:58, Alex Henrie wrote:
>>> +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 &&
>>
>> I don't quite follow what this part of the test is for
> 
> The test is modeled after the existing test "do not rebase cousins
> unless asked for". First, it verifies that --rebase-merges rebases the
> merges, which in this case results in no changes to the branch. Then,
> it verifies that `git rebase` without arguments flattens the history.

I think "do not rebase cousins unless asked for" is a bit different 
because it is checking the default for --rebase-merges which seems 
reasonable. I cannot see the point of checking that --rebase-merges 
works in this test as we have a whole file of tests that already do that.

Best Wishes

Phillip


>>> +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
>>> +'
>>
>> This test looks good. I think we could probably have just this second
>> test
> 
> I like having a test for the default behavior. Nevertheless, I am
> happy to remove that test as requested. Does anyone else have an
> opinion?
> 
>> and squash this into the previous patch - improving the
>> documentation and test coverage for --no-rebase-merges would make a nice
>> self-contained commit.
> 
> Sure, squashing the first two patches is no problem.
> 
> Thanks for the feedback,
> 
> -Alex

Re: [PATCH v2 2/4] rebase: add tests for --no-rebase-merges

[Alex Henrie]

On Wed, Feb 22, 2023 at 3:16 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 22/02/2023 01:37, Alex Henrie wrote:
> > On Tue, Feb 21, 2023 at 4:00 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >>
> >> On 21/02/2023 05:58, Alex Henrie wrote:
> >>> +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 &&
> >>
> >> I don't quite follow what this part of the test is for
> >
> > The test is modeled after the existing test "do not rebase cousins
> > unless asked for". First, it verifies that --rebase-merges rebases the
> > merges, which in this case results in no changes to the branch. Then,
> > it verifies that `git rebase` without arguments flattens the history.
>
> I think "do not rebase cousins unless asked for" is a bit different
> because it is checking the default for --rebase-merges which seems
> reasonable. I cannot see the point of checking that --rebase-merges
> works in this test as we have a whole file of tests that already do that.

I've removed the test in v4. Thanks again for the feedback.

-Alex

[PATCH v2 3/4] rebase: stop accepting --rebase-merges=""

[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         | 42 +++++++++++++++++++++++++++-------------
 t/t3430-rebase-merges.sh |  6 ++++++
 2 files changed, 35 insertions(+), 13 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 6635f10d52..0a8366f30f 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;
@@ -980,6 +994,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 +1061,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 +1162,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 +1460,8 @@ 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)
-			; /* 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 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

Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=""

[Phillip Wood]

Hi Alex

On 21/02/2023 05:58, Alex Henrie wrote:
> 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.

I think this is sensible in the context of adding a config option. It is 
a backwards incompatible change though, lets hope no one was relying on 
it. Is there a particular reason you decided to redo the option parsing 
rather than just calling parse_merges_value() from the existing "if 
(rebase_merges)" block? I don't think it really matters, I'm just curious.

Best Wishes

Phillip

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   builtin/rebase.c         | 42 +++++++++++++++++++++++++++-------------
>   t/t3430-rebase-merges.sh |  6 ++++++
>   2 files changed, 35 insertions(+), 13 deletions(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 6635f10d52..0a8366f30f 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;
> @@ -980,6 +994,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 +1061,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 +1162,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 +1460,8 @@ 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)
> -			; /* 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 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 &&

Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=""

[Alex Henrie]

On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 21/02/2023 05:58, Alex Henrie wrote:
> > 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.
>
> I think this is sensible in the context of adding a config option. It is
> a backwards incompatible change though, lets hope no one was relying on
> it.

Since the syntax is bizarre and undocumented, I doubt anyone is
relying on it for anything serious, if anyone uses it at all.

> Is there a particular reason you decided to redo the option parsing
> rather than just calling parse_merges_value() from the existing "if
> (rebase_merges)" block? I don't think it really matters, I'm just curious.

Without a parse_opt_merges callback, how could we know whether the
user passed --no-rebase-merges as opposed to passing nothing at all?
const char *rebase_merges would be NULL in either case. It's an
important distinction to make because --no-rebase-merges overrides
rebase.merges but the absence of a command-line argument does not.

All the same, your comment made me realize that it would probably make
more sense to simply change the default value of --rebase-cousins from
"" to "no-rebase-cousins" in this patch and then add the
parse_opt_merges callback in the next patch when it is actually
needed.

-Alex

Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=""

[Alex Henrie]

On Tue, Feb 21, 2023 at 6:38 PM Alex Henrie <alexhenrie24@gmail.com> wrote:
>
> All the same, your comment made me realize that it would probably make
> more sense to simply change the default value of --rebase-cousins from
> "" to "no-rebase-cousins" in this patch

I meant --rebase-merges, sorry. --rebase-cousins obviously isn't a thing.

-Alex

Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=""

[Phillip Wood]

Hi Alex

On 22/02/2023 01:38, Alex Henrie wrote:
> On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> On 21/02/2023 05:58, Alex Henrie wrote:
>>> 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.
>>
>> I think this is sensible in the context of adding a config option. It is
>> a backwards incompatible change though, lets hope no one was relying on
>> it.
> 
> Since the syntax is bizarre and undocumented, I doubt anyone is
> relying on it for anything serious, if anyone uses it at all.
> 
>> Is there a particular reason you decided to redo the option parsing
>> rather than just calling parse_merges_value() from the existing "if
>> (rebase_merges)" block? I don't think it really matters, I'm just curious.
> 
> Without a parse_opt_merges callback, how could we know whether the
> user passed --no-rebase-merges as opposed to passing nothing at all?
> const char *rebase_merges would be NULL in either case. It's an
> important distinction to make because --no-rebase-merges overrides
> rebase.merges but the absence of a command-line argument does not.

The usual way we handle that is to set the value of rebase_merges from 
the config before calling parse_options(). However your solution is fine.

> All the same, your comment made me realize that it would probably make
> more sense to simply change the default value of --rebase-cousins from
> "" to "no-rebase-cousins" in this patch and then add the
> parse_opt_merges callback in the next patch when it is actually
> needed.

Sounds good

Phillip

> -Alex

Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=""

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

> The usual way we handle that is to set the value of rebase_merges from
> the config before calling parse_options(). However your solution is
> fine.

Is it?  If it is not too much to ask, I'd prefer to have code that
does not surprise people, and "the usual way" you mentioned is what
readers around here expect to see.  I didn't check and think about
the patch in quetion, and especially the existing code that the
patch needs to touch, too deeply, so if it is too convoluted already
that it would be a lot of work to make it work in "the usual way",
it may be OK, but otherwise, the solution may not be fine. 

Thanks.

Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=""

[Alex Henrie]

On Wed, Feb 22, 2023 at 3:18 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 22/02/2023 01:38, Alex Henrie wrote:
> > On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> >>
> >> Is there a particular reason you decided to redo the option parsing
> >> rather than just calling parse_merges_value() from the existing "if
> >> (rebase_merges)" block? I don't think it really matters, I'm just curious.
> >
> > Without a parse_opt_merges callback, how could we know whether the
> > user passed --no-rebase-merges as opposed to passing nothing at all?
> > const char *rebase_merges would be NULL in either case. It's an
> > important distinction to make because --no-rebase-merges overrides
> > rebase.merges but the absence of a command-line argument does not.
>
> The usual way we handle that is to set the value of rebase_merges from
> the config before calling parse_options(). However your solution is fine.

On Wed, Feb 22, 2023 at 4:08 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Is it?  If it is not too much to ask, I'd prefer to have code that
> does not surprise people, and "the usual way" you mentioned is what
> readers around here expect to see.  I didn't check and think about
> the patch in quetion, and especially the existing code that the
> patch needs to touch, too deeply, so if it is too convoluted already
> that it would be a lot of work to make it work in "the usual way",
> it may be OK, but otherwise, the solution may not be fine.

There was a const char *rebase_merges in cmd_rebase and an int
rebase_merges in struct rebase_options. I deleted const char
*rebase_merges, leaving only int rebase_merges. int rebase_merges is
set from rebase_config before it is set from parse_options.

Do you want me to add back const char *rebase_merges? If so, where
should it be declared so that it can be set from both rebase_config
and parse_options?

-Alex

[PATCH v2 4/4] rebase: add a config option for --rebase-merges

[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 |  3 ++
 Documentation/git-rebase.txt    |  3 +-
 builtin/rebase.c                | 11 +++++
 t/t3430-rebase-merges.sh        | 81 +++++++++++++++++++++++++++++++++
 4 files changed, 97 insertions(+), 1 deletion(-)

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/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 0a8366f30f..35f3837f43 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -829,6 +829,17 @@ 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) {
+			opts->rebase_merges = git_parse_maybe_bool(rebase_merges);
+			if (opts->rebase_merges < 0)
+				parse_merges_value(opts, rebase_merges);
+		}
+		return 0;
+	}
+
 	if (!strcmp(var, "rebase.backend")) {
 		return git_config_string(&opts->default_backend, var, value);
 	}
diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
index b8ba323dbc..4a7193d501 100755
--- a/t/t3430-rebase-merges.sh
+++ b/t/t3430-rebase-merges.sh
@@ -299,6 +299,86 @@ test_expect_success '--rebase-merges="" is invalid syntax' '
 	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 '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' '
+	git 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' '
+	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=false' '
+	git config rebase.merges false &&
+	git checkout -b override-config-merges-false 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 &&
@@ -409,6 +489,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 &&
-- 
2.39.2

Re: [PATCH v2 4/4] rebase: add a config option for --rebase-merges

[Phillip Wood]

Hi Alex

On 21/02/2023 05:58, Alex Henrie wrote:
> 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.

Thanks for improving the commit message, this seems like a reasonable 
change.

> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   Documentation/config/rebase.txt |  3 ++
>   Documentation/git-rebase.txt    |  3 +-
>   builtin/rebase.c                | 11 +++++
>   t/t3430-rebase-merges.sh        | 81 +++++++++++++++++++++++++++++++++
>   4 files changed, 97 insertions(+), 1 deletion(-)
> 
> 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.

Below I see this takes a boolean in addition to [no-]rebase-cousins, it 
would be nice to document that, especially what "true" means.

> 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 0a8366f30f..35f3837f43 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -829,6 +829,17 @@ 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) {

rebase_merges is guaranteed to be non-NULL if git_config_string returns 0.

> +			opts->rebase_merges = git_parse_maybe_bool(rebase_merges);
> +			if (opts->rebase_merges < 0)
> +				parse_merges_value(opts, rebase_merges);
> +		}
> +		return 0;
> +	}

I think this leaks rebase_merges as git_config_string() returns a copy 
despite taking a "const char*".

If we're adding a new config option that is incompatible with the apply 
backend we should add some logic to error out if the config is set and 
any of the apply options are given - see eddfcd8ece (rebase: provide 
better error message for apply options vs. merge config, 2023-01-25)

>   	if (!strcmp(var, "rebase.backend")) {
>   		return git_config_string(&opts->default_backend, var, value);
>   	}
> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh
> index b8ba323dbc..4a7193d501 100755
> --- a/t/t3430-rebase-merges.sh
> +++ b/t/t3430-rebase-merges.sh
> @@ -299,6 +299,86 @@ test_expect_success '--rebase-merges="" is invalid syntax' '
>   	test_cmp expect actual
>   '
>   
> +test_expect_success 'rebase.merges="" is equivalent to not passing --rebase-merges' '
> +	git config rebase.merges "" &&

test_config is generally preferred as it clears the value at the end of 
the test. Then you don't need the final hunk of this patch.

> +	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 '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' '
> +	git 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' '
> +	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=false' '
> +	git config rebase.merges false &&
> +	git checkout -b override-config-merges-false 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
> +'

Thanks for the tests, I think they provide good coverage of the various 
combinations of config and commandline settings

Best Wishes

Phillip

>   test_expect_success 'refs/rewritten/* is worktree-local' '
>   	git worktree add wt &&
>   	cat >wt/script-from-scratch <<-\EOF &&
> @@ -409,6 +489,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 &&

Re: [PATCH v2 4/4] rebase: add a config option for --rebase-merges

[Alex Henrie]

On Tue, Feb 21, 2023 at 3:46 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>
> On 21/02/2023 05:58, Alex Henrie wrote:
> > +rebase.merges::
> > +     Default value of `--rebase-merges` option.
>
> Below I see this takes a boolean in addition to [no-]rebase-cousins, it
> would be nice to document that, especially what "true" means.

Good point. I'll add another sentence explaining that "true" is
equivalent to --rebase-merges without arguments and "false" is
equivalent to --no-rebase-merges.

> > +     if (!strcmp(var, "rebase.merges")) {
> > +             const char *rebase_merges;
> > +             if (!git_config_string(&rebase_merges, var, value) &&
> > +                 rebase_merges && *rebase_merges) {
>
> rebase_merges is guaranteed to be non-NULL if git_config_string returns 0.
>
> > +                     opts->rebase_merges = git_parse_maybe_bool(rebase_merges);
> > +                     if (opts->rebase_merges < 0)
> > +                             parse_merges_value(opts, rebase_merges);
> > +             }
> > +             return 0;
> > +     }
>
> I think this leaks rebase_merges as git_config_string() returns a copy
> despite taking a "const char*".

Come to think of it, I don't think we need git_config_string at all
here. As far as I can tell, it should be fine to just check (value &&
*value) and not duplicate the string in memory.

> > +test_expect_success 'rebase.merges="" is equivalent to not passing --rebase-merges' '
> > +     git config rebase.merges "" &&
>
> test_config is generally preferred as it clears the value at the end of
> the test. Then you don't need the final hunk of this patch.

I've used test_config before but I forgot about it. Thanks for the reminder.

-Alex

Re: [PATCH v2 1/4] rebase: document the --no-rebase-merges option

[Phillip Wood]

Hi Alex

Thanks for working on this series, I think it is going in a sensible 
direction. I think the final patch needs a little bit of work but 
they're all looking good.

Best Wishes

Phillip

On 21/02/2023 05:58, Alex Henrie wrote:
> Signed-off-by: Alex Henrie <alexhenrie24@gmail.com>
> ---
>   Documentation/git-rebase.txt | 4 +++-
>   1 file changed, 3 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,