{"thread":{"id":"59277","subject":"[PATCH v2 2/4] rebase: add tests for --no-rebase-merges","startedAt":"2023-02-21T06:01:41Z","lastAt":"2023-02-23T05:35:32Z","messageCount":17,"participants":["Alex Henrie","Phillip Wood","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"472363","messageId":"20230221055805.210951-2-alexhenrie24@gmail.com","threadId":"59277","inReplyTo":"20230221055805.210951-1-alexhenrie24@gmail.com","subject":"[PATCH v2 2/4] rebase: add tests for --no-rebase-merges","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-02-21T05:58:03Z","receivedAt":"2023-02-21T06:01:41Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Signed-off-by: Alex Henrie \n---\n t/t3430-rebase-merges.sh | 25 +++++++++++++++++++++++++\n 1 file changed, 25 insertions(+)\n\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex fa2a06c19f..e0d910c229 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -250,6 +250,31 @@ test_expect_success 'with a branch tip that was cherry-picked already' '\n \tEOF\n '\n \n+test_expect_success 'do not rebase merges unless asked to' '\n+\tgit checkout -b rebase-merges-default E &&\n+\tbefore=\"$(git rev-parse --verify HEAD)\" &&\n+\ttest_tick &&\n+\tgit rebase --rebase-merges C &&\n+\ttest_cmp_rev HEAD $before &&\n+\ttest_tick &&\n+\tgit rebase C &&\n+\ttest_cmp_graph C.. <<-\\EOF\n+\t* B\n+\t* D\n+\to C\n+\tEOF\n+'\n+\n+test_expect_success '--no-rebase-merges countermands --rebase-merges' '\n+\tgit checkout -b no-rebase-merges E &&\n+\tgit rebase --rebase-merges --no-rebase-merges C &&\n+\ttest_cmp_graph C.. <<-\\EOF\n+\t* B\n+\t* D\n+\to C\n+\tEOF\n+'\n+\n test_expect_success 'do not rebase cousins unless asked for' '\n \tgit checkout -b cousins main &&\n \tbefore=\"$(git rev-parse --verify HEAD)\" &&\n-- \n2.39.2\n\n"},{"id":"472364","messageId":"20230221055805.210951-1-alexhenrie24@gmail.com","threadId":"59277","inReplyTo":null,"subject":"[PATCH v2 1/4] rebase: document the --no-rebase-merges option","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-02-21T05:58:02Z","receivedAt":"2023-02-21T06:01:41Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"Signed-off-by: Alex Henrie \n---\n Documentation/git-rebase.txt | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 9a295bcee4..c98784a0d2 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -529,13 +529,15 @@ See also INCOMPATIBLE OPTIONS below.\n \n -r::\n --rebase-merges[=(rebase-cousins|no-rebase-cousins)]::\n+--no-rebase-merges::\n \tBy default, a rebase will simply drop merge commits from the todo\n \tlist, and put the rebased commits into a single, linear branch.\n \tWith `--rebase-merges`, the rebase will instead try to preserve\n \tthe branching structure within the commits that are to be rebased,\n \tby recreating the merge commits. Any resolved merge conflicts or\n \tmanual amendments in these merge commits will have to be\n-\tresolved/re-applied manually.\n+\tresolved/re-applied manually. `--no-rebase-merges` can be used to\n+\tcountermand a previous `--rebase-merges`.\n +\n By default, or when `no-rebase-cousins` was specified, commits which do not\n have `` as direct ancestor will keep their original branch point,\n-- \n2.39.2\n\n"},{"id":"472365","messageId":"20230221055805.210951-3-alexhenrie24@gmail.com","threadId":"59277","inReplyTo":"20230221055805.210951-1-alexhenrie24@gmail.com","subject":"[PATCH v2 3/4] rebase: stop accepting --rebase-merges=\"\"","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-02-21T05:58:04Z","receivedAt":"2023-02-21T06:01:52Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"The unusual syntax --rebase-merges=\"\" (that is, --rebase-merges with an\nempty string argument) has been an undocumented synonym of\n--rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid\nconfusion when a rebase.merges config option is introduced, where\nrebase.merges=\"\" will be equivalent to not passing --rebase-merges.\n\nSigned-off-by: Alex Henrie \n---\n builtin/rebase.c | 42 +++++++++++++++++++++++++++-------------\n t/t3430-rebase-merges.sh | 6 ++++++\n 2 files changed, 35 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 6635f10d52..0a8366f30f 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -771,6 +771,20 @@ static int run_specific_rebase(struct rebase_options *opts)\n \treturn status ? -1 : 0;\n }\n \n+static void parse_merges_value(struct rebase_options *options, const char *value)\n+{\n+\tif (value) {\n+\t\tif (!strcmp(\"no-rebase-cousins\", value))\n+\t\t\toptions->rebase_cousins = 0;\n+\t\telse if (!strcmp(\"rebase-cousins\", value))\n+\t\t\toptions->rebase_cousins = 1;\n+\t\telse\n+\t\t\tdie(_(\"Unknown mode: %s\"), value);\n+\t}\n+\n+\toptions->rebase_merges = 1;\n+}\n+\n static int rebase_config(const char *var, const char *value, void *data)\n {\n \tstruct rebase_options *opts = data;\n@@ -980,6 +994,18 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)\n \treturn 0;\n }\n \n+static int parse_opt_merges(const struct option *opt, const char *arg, int unset)\n+{\n+\tstruct rebase_options *options = opt->value;\n+\n+\tif (unset)\n+\t\toptions->rebase_merges = 0;\n+\telse\n+\t\tparse_merges_value(options, arg);\n+\n+\treturn 0;\n+}\n+\n static void NORETURN error_on_missing_default_upstream(void)\n {\n \tstruct branch *current_branch = branch_get(NULL);\n@@ -1035,7 +1061,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tstruct object_id branch_base;\n \tint ignore_whitespace = 0;\n \tconst char *gpg_sign = NULL;\n-\tconst char *rebase_merges = NULL;\n \tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n \tstruct object_id squash_onto;\n \tchar *squash_onto_name = NULL;\n@@ -1137,10 +1162,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t &options.allow_empty_message,\n \t\t\t N_(\"allow rebasing commits with empty messages\"),\n \t\t\t PARSE_OPT_HIDDEN),\n-\t\t{OPTION_STRING, 'r', \"rebase-merges\", &rebase_merges,\n-\t\t\tN_(\"mode\"),\n+\t\tOPT_CALLBACK_F('r', \"rebase-merges\", &options, N_(\"mode\"),\n \t\t\tN_(\"try to rebase merges instead of skipping them\"),\n-\t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)\"\"},\n+\t\t\tPARSE_OPT_OPTARG, parse_opt_merges),\n \t\tOPT_BOOL(0, \"fork-point\", &options.fork_point,\n \t\t\t N_(\"use 'merge-base --fork-point' to refine upstream\")),\n \t\tOPT_STRING('s', \"strategy\", &options.strategy,\n@@ -1436,16 +1460,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.exec.nr)\n \t\timply_merge(&options, \"--exec\");\n \n-\tif (rebase_merges) {\n-\t\tif (!*rebase_merges)\n-\t\t\t; /* default mode; do nothing */\n-\t\telse if (!strcmp(\"rebase-cousins\", rebase_merges))\n-\t\t\toptions.rebase_cousins = 1;\n-\t\telse if (strcmp(\"no-rebase-cousins\", rebase_merges))\n-\t\t\tdie(_(\"Unknown mode: %s\"), rebase_merges);\n-\t\toptions.rebase_merges = 1;\n+\tif (options.rebase_merges)\n \t\timply_merge(&options, \"--rebase-merges\");\n-\t}\n \n \tif (options.type == REBASE_APPLY) {\n \t\tif (ignore_whitespace)\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex e0d910c229..b8ba323dbc 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -293,6 +293,12 @@ test_expect_success 'do not rebase cousins unless asked for' '\n \tEOF\n '\n \n+test_expect_success '--rebase-merges=\"\" is invalid syntax' '\n+\techo \"fatal: Unknown mode: \" >expect &&\n+\t! git rebase --rebase-merges=\"\" HEAD^ 2>actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'refs/rewritten/* is worktree-local' '\n \tgit worktree add wt &&\n \tcat >wt/script-from-scratch <<-\\EOF &&\n-- \n2.39.2\n\n"},{"id":"472366","messageId":"20230221055805.210951-4-alexhenrie24@gmail.com","threadId":"59277","inReplyTo":"20230221055805.210951-1-alexhenrie24@gmail.com","subject":"[PATCH v2 4/4] rebase: add a config option for --rebase-merges","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-02-21T05:58:05Z","receivedAt":"2023-02-21T06:01:55Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"The purpose of the new option is to accommodate users who would like\n--rebase-merges to be on by default and to facilitate possibly turning\non --rebase-merges by default without configuration in a future version\nof Git.\n\nSigned-off-by: Alex Henrie \n---\n Documentation/config/rebase.txt | 3 ++\n Documentation/git-rebase.txt | 3 +-\n builtin/rebase.c | 11 +++++\n t/t3430-rebase-merges.sh | 81 +++++++++++++++++++++++++++++++++\n 4 files changed, 97 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt\nindex f19bd0e040..d956ec4441 100644\n--- a/Documentation/config/rebase.txt\n+++ b/Documentation/config/rebase.txt\n@@ -67,3 +67,6 @@ rebase.rescheduleFailedExec::\n \n rebase.forkPoint::\n \tIf set to false set `--no-fork-point` option by default.\n+\n+rebase.merges::\n+\tDefault value of `--rebase-merges` option.\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex c98784a0d2..b02f9cbb8c 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -537,7 +537,8 @@ See also INCOMPATIBLE OPTIONS below.\n \tby recreating the merge commits. Any resolved merge conflicts or\n \tmanual amendments in these merge commits will have to be\n \tresolved/re-applied manually. `--no-rebase-merges` can be used to\n-\tcountermand a previous `--rebase-merges`.\n+\tcountermand both the `rebase.merges` config option and a previous\n+\t`--rebase-merges`.\n +\n By default, or when `no-rebase-cousins` was specified, commits which do not\n have `` as direct ancestor will keep their original branch point,\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0a8366f30f..35f3837f43 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -829,6 +829,17 @@ static int rebase_config(const char *var, const char *value, void *data)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(var, \"rebase.merges\")) {\n+\t\tconst char *rebase_merges;\n+\t\tif (!git_config_string(&rebase_merges, var, value) &&\n+\t\t rebase_merges && *rebase_merges) {\n+\t\t\topts->rebase_merges = git_parse_maybe_bool(rebase_merges);\n+\t\t\tif (opts->rebase_merges < 0)\n+\t\t\t\tparse_merges_value(opts, rebase_merges);\n+\t\t}\n+\t\treturn 0;\n+\t}\n+\n \tif (!strcmp(var, \"rebase.backend\")) {\n \t\treturn git_config_string(&opts->default_backend, var, value);\n \t}\ndiff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\nindex b8ba323dbc..4a7193d501 100755\n--- a/t/t3430-rebase-merges.sh\n+++ b/t/t3430-rebase-merges.sh\n@@ -299,6 +299,86 @@ test_expect_success '--rebase-merges=\"\" is invalid syntax' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'rebase.merges=\"\" is equivalent to not passing --rebase-merges' '\n+\tgit config rebase.merges \"\" &&\n+\tgit checkout -b config-merges-blank E &&\n+\tgit rebase C &&\n+\ttest_cmp_graph C.. <<-\\EOF\n+\t* B\n+\t* D\n+\to C\n+\tEOF\n+'\n+\n+test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '\n+\tgit config rebase.merges rebase-cousins &&\n+\tgit checkout -b config-rebase-cousins main &&\n+\tgit rebase HEAD^ &&\n+\ttest_cmp_graph HEAD^.. <<-\\EOF\n+\t* Merge the topic branch '\\''onebranch'\\''\n+\t|\\\n+\t| * D\n+\t| * G\n+\t|/\n+\to H\n+\tEOF\n+'\n+\n+test_expect_success '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' '\n+\tgit config rebase.merges no-rebase-cousins &&\n+\tgit checkout -b override-config-no-rebase-cousins E &&\n+\tgit rebase --no-rebase-merges C &&\n+\ttest_cmp_graph C.. <<-\\EOF\n+\t* B\n+\t* D\n+\to C\n+\tEOF\n+'\n+\n+test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' '\n+\tgit config rebase.merges rebase-cousins &&\n+\tgit checkout -b override-config-rebase-cousins main &&\n+\tgit rebase --rebase-merges=no-rebase-cousins HEAD^ &&\n+\ttest_cmp_graph HEAD^.. <<-\\EOF\n+\t* Merge the topic branch '\\''onebranch'\\''\n+\t|\\\n+\t| * D\n+\t| * G\n+\to | H\n+\t|/\n+\to A\n+\tEOF\n+'\n+\n+test_expect_success '--rebase-merges overrides rebase.merges=false' '\n+\tgit config rebase.merges false &&\n+\tgit checkout -b override-config-merges-false main &&\n+\tgit rebase --rebase-merges HEAD^ &&\n+\ttest_cmp_graph HEAD^.. <<-\\EOF\n+\t* Merge the topic branch '\\''onebranch'\\''\n+\t|\\\n+\t| * D\n+\t| * G\n+\to | H\n+\t|/\n+\to A\n+\tEOF\n+'\n+\n+test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' '\n+\tgit config rebase.merges rebase-cousins &&\n+\tgit checkout -b no-override-config-rebase-cousins main &&\n+\tgit rebase --rebase-merges HEAD^ &&\n+\ttest_cmp_graph HEAD^.. <<-\\EOF\n+\t* Merge the topic branch '\\''onebranch'\\''\n+\t|\\\n+\t| * D\n+\t| * G\n+\t|/\n+\to H\n+\tEOF\n+'\n+\n test_expect_success 'refs/rewritten/* is worktree-local' '\n \tgit worktree add wt &&\n \tcat >wt/script-from-scratch <<-\\EOF &&\n@@ -409,6 +489,7 @@ test_expect_success 'a \"merge\" into a root commit is a fast-forward' '\n '\n \n test_expect_success 'A root commit can be a cousin, treat it that way' '\n+\tgit config --unset rebase.merges &&\n \tgit checkout --orphan khnum &&\n \ttest_commit yama &&\n \tgit checkout -b asherah main &&\n-- \n2.39.2\n\n"},{"id":"472369","messageId":"d1f10c52-9cfb-8e6a-cbd6-cd81b778b8cc@dunelm.org.uk","threadId":"59277","inReplyTo":"20230221055805.210951-4-alexhenrie24@gmail.com","subject":"Re: [PATCH v2 4/4] rebase: add a config option for --rebase-merges","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-21T10:46:55Z","receivedAt":"2023-02-21T10:47:03Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 21/02/2023 05:58, Alex Henrie wrote:\n> The purpose of the new option is to accommodate users who would like\n> --rebase-merges to be on by default and to facilitate possibly turning\n> on --rebase-merges by default without configuration in a future version\n> of Git.\n\nThanks for improving the commit message, this seems like a reasonable \nchange.\n\n> Signed-off-by: Alex Henrie \n> ---\n> Documentation/config/rebase.txt | 3 ++\n> Documentation/git-rebase.txt | 3 +-\n> builtin/rebase.c | 11 +++++\n> t/t3430-rebase-merges.sh | 81 +++++++++++++++++++++++++++++++++\n> 4 files changed, 97 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/config/rebase.txt b/Documentation/config/rebase.txt\n> index f19bd0e040..d956ec4441 100644\n> --- a/Documentation/config/rebase.txt\n> +++ b/Documentation/config/rebase.txt\n> @@ -67,3 +67,6 @@ rebase.rescheduleFailedExec::\n> \n> rebase.forkPoint::\n> \tIf set to false set `--no-fork-point` option by default.\n> +\n> +rebase.merges::\n> +\tDefault value of `--rebase-merges` option.\n\nBelow I see this takes a boolean in addition to [no-]rebase-cousins, it \nwould be nice to document that, especially what \"true\" means.\n\n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index c98784a0d2..b02f9cbb8c 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -537,7 +537,8 @@ See also INCOMPATIBLE OPTIONS below.\n> \tby recreating the merge commits. Any resolved merge conflicts or\n> \tmanual amendments in these merge commits will have to be\n> \tresolved/re-applied manually. `--no-rebase-merges` can be used to\n> -\tcountermand a previous `--rebase-merges`.\n> +\tcountermand both the `rebase.merges` config option and a previous\n> +\t`--rebase-merges`.\n> +\n> By default, or when `no-rebase-cousins` was specified, commits which do not\n> have `` as direct ancestor will keep their original branch point,\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 0a8366f30f..35f3837f43 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -829,6 +829,17 @@ static int rebase_config(const char *var, const char *value, void *data)\n> \t\treturn 0;\n> \t}\n> \n> +\tif (!strcmp(var, \"rebase.merges\")) {\n> +\t\tconst char *rebase_merges;\n> +\t\tif (!git_config_string(&rebase_merges, var, value) &&\n> +\t\t rebase_merges && *rebase_merges) {\n\nrebase_merges is guaranteed to be non-NULL if git_config_string returns 0.\n\n> +\t\t\topts->rebase_merges = git_parse_maybe_bool(rebase_merges);\n> +\t\t\tif (opts->rebase_merges < 0)\n> +\t\t\t\tparse_merges_value(opts, rebase_merges);\n> +\t\t}\n> +\t\treturn 0;\n> +\t}\n\nI think this leaks rebase_merges as git_config_string() returns a copy \ndespite taking a \"const char*\".\n\nIf we're adding a new config option that is incompatible with the apply \nbackend we should add some logic to error out if the config is set and \nany of the apply options are given - see eddfcd8ece (rebase: provide \nbetter error message for apply options vs. merge config, 2023-01-25)\n\n> \tif (!strcmp(var, \"rebase.backend\")) {\n> \t\treturn git_config_string(&opts->default_backend, var, value);\n> \t}\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index b8ba323dbc..4a7193d501 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -299,6 +299,86 @@ test_expect_success '--rebase-merges=\"\" is invalid syntax' '\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'rebase.merges=\"\" is equivalent to not passing --rebase-merges' '\n> +\tgit config rebase.merges \"\" &&\n\ntest_config is generally preferred as it clears the value at the end of \nthe test. Then you don't need the final hunk of this patch.\n\n> +\tgit checkout -b config-merges-blank E &&\n> +\tgit rebase C &&\n> +\ttest_cmp_graph C.. <<-\\EOF\n> +\t* B\n> +\t* D\n> +\to C\n> +\tEOF\n> +'\n> +\n> +test_expect_success 'rebase.merges=rebase-cousins is equivalent to --rebase-merges=rebase-cousins' '\n> +\tgit config rebase.merges rebase-cousins &&\n> +\tgit checkout -b config-rebase-cousins main &&\n> +\tgit rebase HEAD^ &&\n> +\ttest_cmp_graph HEAD^.. <<-\\EOF\n> +\t* Merge the topic branch '\\''onebranch'\\''\n> +\t|\\\n> +\t| * D\n> +\t| * G\n> +\t|/\n> +\to H\n> +\tEOF\n> +'\n> +\n> +test_expect_success '--no-rebase-merges overrides rebase.merges=no-rebase-cousins' '\n> +\tgit config rebase.merges no-rebase-cousins &&\n> +\tgit checkout -b override-config-no-rebase-cousins E &&\n> +\tgit rebase --no-rebase-merges C &&\n> +\ttest_cmp_graph C.. <<-\\EOF\n> +\t* B\n> +\t* D\n> +\to C\n> +\tEOF\n> +'\n> +\n> +test_expect_success '--rebase-merges=no-rebase-cousins overrides rebase.merges=rebase-cousins' '\n> +\tgit config rebase.merges rebase-cousins &&\n> +\tgit checkout -b override-config-rebase-cousins main &&\n> +\tgit rebase --rebase-merges=no-rebase-cousins HEAD^ &&\n> +\ttest_cmp_graph HEAD^.. <<-\\EOF\n> +\t* Merge the topic branch '\\''onebranch'\\''\n> +\t|\\\n> +\t| * D\n> +\t| * G\n> +\to | H\n> +\t|/\n> +\to A\n> +\tEOF\n> +'\n> +\n> +test_expect_success '--rebase-merges overrides rebase.merges=false' '\n> +\tgit config rebase.merges false &&\n> +\tgit checkout -b override-config-merges-false main &&\n> +\tgit rebase --rebase-merges HEAD^ &&\n> +\ttest_cmp_graph HEAD^.. <<-\\EOF\n> +\t* Merge the topic branch '\\''onebranch'\\''\n> +\t|\\\n> +\t| * D\n> +\t| * G\n> +\to | H\n> +\t|/\n> +\to A\n> +\tEOF\n> +'\n> +\n> +test_expect_success '--rebase-merges does not override rebase.merges=rebase-cousins' '\n> +\tgit config rebase.merges rebase-cousins &&\n> +\tgit checkout -b no-override-config-rebase-cousins main &&\n> +\tgit rebase --rebase-merges HEAD^ &&\n> +\ttest_cmp_graph HEAD^.. <<-\\EOF\n> +\t* Merge the topic branch '\\''onebranch'\\''\n> +\t|\\\n> +\t| * D\n> +\t| * G\n> +\t|/\n> +\to H\n> +\tEOF\n> +'\n\nThanks for the tests, I think they provide good coverage of the various \ncombinations of config and commandline settings\n\nBest Wishes\n\nPhillip\n\n> test_expect_success 'refs/rewritten/* is worktree-local' '\n> \tgit worktree add wt &&\n> \tcat >wt/script-from-scratch <<-\\EOF &&\n> @@ -409,6 +489,7 @@ test_expect_success 'a \"merge\" into a root commit is a fast-forward' '\n> '\n> \n> test_expect_success 'A root commit can be a cousin, treat it that way' '\n> +\tgit config --unset rebase.merges &&\n> \tgit checkout --orphan khnum &&\n> \ttest_commit yama &&\n> \tgit checkout -b asherah main &&\n"},{"id":"472370","messageId":"852c6efd-49a7-f6f0-dd6a-b28cb0909784@dunelm.org.uk","threadId":"59277","inReplyTo":"20230221055805.210951-3-alexhenrie24@gmail.com","subject":"Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=\"\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-21T10:55:35Z","receivedAt":"2023-02-21T10:55:40Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 21/02/2023 05:58, Alex Henrie wrote:\n> The unusual syntax --rebase-merges=\"\" (that is, --rebase-merges with an\n> empty string argument) has been an undocumented synonym of\n> --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid\n> confusion when a rebase.merges config option is introduced, where\n> rebase.merges=\"\" will be equivalent to not passing --rebase-merges.\n\nI think this is sensible in the context of adding a config option. It is \na backwards incompatible change though, lets hope no one was relying on \nit. Is there a particular reason you decided to redo the option parsing \nrather than just calling parse_merges_value() from the existing \"if \n(rebase_merges)\" block? I don't think it really matters, I'm just curious.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Alex Henrie \n> ---\n> builtin/rebase.c | 42 +++++++++++++++++++++++++++-------------\n> t/t3430-rebase-merges.sh | 6 ++++++\n> 2 files changed, 35 insertions(+), 13 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 6635f10d52..0a8366f30f 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -771,6 +771,20 @@ static int run_specific_rebase(struct rebase_options *opts)\n> \treturn status ? -1 : 0;\n> }\n> \n> +static void parse_merges_value(struct rebase_options *options, const char *value)\n> +{\n> +\tif (value) {\n> +\t\tif (!strcmp(\"no-rebase-cousins\", value))\n> +\t\t\toptions->rebase_cousins = 0;\n> +\t\telse if (!strcmp(\"rebase-cousins\", value))\n> +\t\t\toptions->rebase_cousins = 1;\n> +\t\telse\n> +\t\t\tdie(_(\"Unknown mode: %s\"), value);\n> +\t}\n> +\n> +\toptions->rebase_merges = 1;\n> +}\n> +\n> static int rebase_config(const char *var, const char *value, void *data)\n> {\n> \tstruct rebase_options *opts = data;\n> @@ -980,6 +994,18 @@ static int parse_opt_empty(const struct option *opt, const char *arg, int unset)\n> \treturn 0;\n> }\n> \n> +static int parse_opt_merges(const struct option *opt, const char *arg, int unset)\n> +{\n> +\tstruct rebase_options *options = opt->value;\n> +\n> +\tif (unset)\n> +\t\toptions->rebase_merges = 0;\n> +\telse\n> +\t\tparse_merges_value(options, arg);\n> +\n> +\treturn 0;\n> +}\n> +\n> static void NORETURN error_on_missing_default_upstream(void)\n> {\n> \tstruct branch *current_branch = branch_get(NULL);\n> @@ -1035,7 +1061,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tstruct object_id branch_base;\n> \tint ignore_whitespace = 0;\n> \tconst char *gpg_sign = NULL;\n> -\tconst char *rebase_merges = NULL;\n> \tstruct string_list strategy_options = STRING_LIST_INIT_NODUP;\n> \tstruct object_id squash_onto;\n> \tchar *squash_onto_name = NULL;\n> @@ -1137,10 +1162,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t\t &options.allow_empty_message,\n> \t\t\t N_(\"allow rebasing commits with empty messages\"),\n> \t\t\t PARSE_OPT_HIDDEN),\n> -\t\t{OPTION_STRING, 'r', \"rebase-merges\", &rebase_merges,\n> -\t\t\tN_(\"mode\"),\n> +\t\tOPT_CALLBACK_F('r', \"rebase-merges\", &options, N_(\"mode\"),\n> \t\t\tN_(\"try to rebase merges instead of skipping them\"),\n> -\t\t\tPARSE_OPT_OPTARG, NULL, (intptr_t)\"\"},\n> +\t\t\tPARSE_OPT_OPTARG, parse_opt_merges),\n> \t\tOPT_BOOL(0, \"fork-point\", &options.fork_point,\n> \t\t\t N_(\"use 'merge-base --fork-point' to refine upstream\")),\n> \t\tOPT_STRING('s', \"strategy\", &options.strategy,\n> @@ -1436,16 +1460,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tif (options.exec.nr)\n> \t\timply_merge(&options, \"--exec\");\n> \n> -\tif (rebase_merges) {\n> -\t\tif (!*rebase_merges)\n> -\t\t\t; /* default mode; do nothing */\n> -\t\telse if (!strcmp(\"rebase-cousins\", rebase_merges))\n> -\t\t\toptions.rebase_cousins = 1;\n> -\t\telse if (strcmp(\"no-rebase-cousins\", rebase_merges))\n> -\t\t\tdie(_(\"Unknown mode: %s\"), rebase_merges);\n> -\t\toptions.rebase_merges = 1;\n> +\tif (options.rebase_merges)\n> \t\timply_merge(&options, \"--rebase-merges\");\n> -\t}\n> \n> \tif (options.type == REBASE_APPLY) {\n> \t\tif (ignore_whitespace)\n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index e0d910c229..b8ba323dbc 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -293,6 +293,12 @@ test_expect_success 'do not rebase cousins unless asked for' '\n> \tEOF\n> '\n> \n> +test_expect_success '--rebase-merges=\"\" is invalid syntax' '\n> +\techo \"fatal: Unknown mode: \" >expect &&\n> +\t! git rebase --rebase-merges=\"\" HEAD^ 2>actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n> test_expect_success 'refs/rewritten/* is worktree-local' '\n> \tgit worktree add wt &&\n> \tcat >wt/script-from-scratch <<-\\EOF &&\n"},{"id":"472371","messageId":"24be0c50-9dfa-e9d9-9d78-1b993832884b@dunelm.org.uk","threadId":"59277","inReplyTo":"20230221055805.210951-2-alexhenrie24@gmail.com","subject":"Re: [PATCH v2 2/4] rebase: add tests for --no-rebase-merges","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-21T11:00:00Z","receivedAt":"2023-02-21T11:00:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nThanks for adding some tests,\n\nOn 21/02/2023 05:58, Alex Henrie wrote:\n> Signed-off-by: Alex Henrie \n> ---\n> t/t3430-rebase-merges.sh | 25 +++++++++++++++++++++++++\n> 1 file changed, 25 insertions(+)\n> \n> diff --git a/t/t3430-rebase-merges.sh b/t/t3430-rebase-merges.sh\n> index fa2a06c19f..e0d910c229 100755\n> --- a/t/t3430-rebase-merges.sh\n> +++ b/t/t3430-rebase-merges.sh\n> @@ -250,6 +250,31 @@ test_expect_success 'with a branch tip that was cherry-picked already' '\n> \tEOF\n> '\n> \n> +test_expect_success 'do not rebase merges unless asked to' '\n> +\tgit checkout -b rebase-merges-default E &&\n> +\tbefore=\"$(git rev-parse --verify HEAD)\" &&\n> +\ttest_tick &&\n> +\tgit rebase --rebase-merges C &&\n\nI don't quite follow what this part of the test is for\n\n> +\ttest_cmp_rev HEAD $before &&\n> +\ttest_tick &&\n> +\tgit rebase C &&\n> +\ttest_cmp_graph C.. <<-\\EOF\n> +\t* B\n> +\t* D\n> +\to C\n> +\tEOF\n> +'\n> +\n> +test_expect_success '--no-rebase-merges countermands --rebase-merges' '\n> +\tgit checkout -b no-rebase-merges E &&\n> +\tgit rebase --rebase-merges --no-rebase-merges C &&\n> +\ttest_cmp_graph C.. <<-\\EOF\n> +\t* B\n> +\t* D\n> +\to C\n> +\tEOF\n> +'\n\nThis test looks good. I think we could probably have just this second \ntest and squash this into the previous patch - improving the \ndocumentation and test coverage for --no-rebase-merges would make a nice \nself-contained commit.\n\nBest Wishes\n\nPhillip\n\n> test_expect_success 'do not rebase cousins unless asked for' '\n> \tgit checkout -b cousins main &&\n> \tbefore=\"$(git rev-parse --verify HEAD)\" &&\n"},{"id":"472372","messageId":"f67126c0-82d3-bbec-89e8-fbe3ec84e04b@dunelm.org.uk","threadId":"59277","inReplyTo":"20230221055805.210951-1-alexhenrie24@gmail.com","subject":"Re: [PATCH v2 1/4] rebase: document the --no-rebase-merges option","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-21T11:01:38Z","receivedAt":"2023-02-21T11:01:42Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nThanks for working on this series, I think it is going in a sensible \ndirection. I think the final patch needs a little bit of work but \nthey're all looking good.\n\nBest Wishes\n\nPhillip\n\nOn 21/02/2023 05:58, Alex Henrie wrote:\n> Signed-off-by: Alex Henrie \n> ---\n> Documentation/git-rebase.txt | 4 +++-\n> 1 file changed, 3 insertions(+), 1 deletion(-)\n> \n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 9a295bcee4..c98784a0d2 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -529,13 +529,15 @@ See also INCOMPATIBLE OPTIONS below.\n> \n> -r::\n> --rebase-merges[=(rebase-cousins|no-rebase-cousins)]::\n> +--no-rebase-merges::\n> \tBy default, a rebase will simply drop merge commits from the todo\n> \tlist, and put the rebased commits into a single, linear branch.\n> \tWith `--rebase-merges`, the rebase will instead try to preserve\n> \tthe branching structure within the commits that are to be rebased,\n> \tby recreating the merge commits. Any resolved merge conflicts or\n> \tmanual amendments in these merge commits will have to be\n> -\tresolved/re-applied manually.\n> +\tresolved/re-applied manually. `--no-rebase-merges` can be used to\n> +\tcountermand a previous `--rebase-merges`.\n> +\n> By default, or when `no-rebase-cousins` was specified, commits which do not\n> have `` as direct ancestor will keep their original branch point,\n"},{"id":"472422","messageId":"CAMMLpeSMO38acvnxK2QX18CdoaEiUojwLsV+AQZYOTe5h4Nq=Q@mail.gmail.com","threadId":"59277","inReplyTo":"24be0c50-9dfa-e9d9-9d78-1b993832884b@dunelm.org.uk","subject":"Re: [PATCH v2 2/4] rebase: add tests for --no-rebase-merges","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-02-22T01:37:52Z","receivedAt":"2023-02-22T01:38:08Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Tue, Feb 21, 2023 at 4:00 AM Phillip Wood wrote:\n>\n> On 21/02/2023 05:58, Alex Henrie wrote:\n> > +test_expect_success 'do not rebase merges unless asked to' '\n> > + git checkout -b rebase-merges-default E &&\n> > + before=\"$(git rev-parse --verify HEAD)\" &&\n> > + test_tick &&\n> > + git rebase --rebase-merges C &&\n>\n> I don't quite follow what this part of the test is for\n\nThe test is modeled after the existing test \"do not rebase cousins\nunless asked for\". First, it verifies that --rebase-merges rebases the\nmerges, which in this case results in no changes to the branch. Then,\nit verifies that `git rebase` without arguments flattens the history.\n\n> > +test_expect_success '--no-rebase-merges countermands --rebase-merges' '\n> > + git checkout -b no-rebase-merges E &&\n> > + git rebase --rebase-merges --no-rebase-merges C &&\n> > + test_cmp_graph C.. <<-\\EOF\n> > + * B\n> > + * D\n> > + o C\n> > + EOF\n> > +'\n>\n> This test looks good. I think we could probably have just this second\n> test\n\nI like having a test for the default behavior. Nevertheless, I am\nhappy to remove that test as requested. Does anyone else have an\nopinion?\n\n> and squash this into the previous patch - improving the\n> documentation and test coverage for --no-rebase-merges would make a nice\n> self-contained commit.\n\nSure, squashing the first two patches is no problem.\n\nThanks for the feedback,\n\n-Alex\n"},{"id":"472423","messageId":"CAMMLpeQ9gEKNX5VtGCgLL_Qzk59ZYji57u-SPy-XeSRPuF2NwA@mail.gmail.com","threadId":"59277","inReplyTo":"852c6efd-49a7-f6f0-dd6a-b28cb0909784@dunelm.org.uk","subject":"Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=\"\"","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-02-22T01:38:20Z","receivedAt":"2023-02-22T01:38:40Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood wrote:\n>\n> On 21/02/2023 05:58, Alex Henrie wrote:\n> > The unusual syntax --rebase-merges=\"\" (that is, --rebase-merges with an\n> > empty string argument) has been an undocumented synonym of\n> > --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid\n> > confusion when a rebase.merges config option is introduced, where\n> > rebase.merges=\"\" will be equivalent to not passing --rebase-merges.\n>\n> I think this is sensible in the context of adding a config option. It is\n> a backwards incompatible change though, lets hope no one was relying on\n> it.\n\nSince the syntax is bizarre and undocumented, I doubt anyone is\nrelying on it for anything serious, if anyone uses it at all.\n\n> Is there a particular reason you decided to redo the option parsing\n> rather than just calling parse_merges_value() from the existing \"if\n> (rebase_merges)\" block? I don't think it really matters, I'm just curious.\n\nWithout a parse_opt_merges callback, how could we know whether the\nuser passed --no-rebase-merges as opposed to passing nothing at all?\nconst char *rebase_merges would be NULL in either case. It's an\nimportant distinction to make because --no-rebase-merges overrides\nrebase.merges but the absence of a command-line argument does not.\n\nAll the same, your comment made me realize that it would probably make\nmore sense to simply change the default value of --rebase-cousins from\n\"\" to \"no-rebase-cousins\" in this patch and then add the\nparse_opt_merges callback in the next patch when it is actually\nneeded.\n\n-Alex\n"},{"id":"472424","messageId":"CAMMLpeQ2QFrNm5Eh02P1tobQwr-L8YYSE5Cfrt4fpfo1Z9Z=EQ@mail.gmail.com","threadId":"59277","inReplyTo":"CAMMLpeQ9gEKNX5VtGCgLL_Qzk59ZYji57u-SPy-XeSRPuF2NwA@mail.gmail.com","subject":"Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=\"\"","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-02-22T01:41:13Z","receivedAt":"2023-02-22T01:41:28Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Tue, Feb 21, 2023 at 6:38 PM Alex Henrie wrote:\n>\n> All the same, your comment made me realize that it would probably make\n> more sense to simply change the default value of --rebase-cousins from\n> \"\" to \"no-rebase-cousins\" in this patch\n\nI meant --rebase-merges, sorry. --rebase-cousins obviously isn't a thing.\n\n-Alex\n"},{"id":"472425","messageId":"CAMMLpeQpSMVm0ZNMNz20+9bKAGhn4pFwTX62nR+B-V-KzvqT+A@mail.gmail.com","threadId":"59277","inReplyTo":"d1f10c52-9cfb-8e6a-cbd6-cd81b778b8cc@dunelm.org.uk","subject":"Re: [PATCH v2 4/4] rebase: add a config option for --rebase-merges","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-02-22T01:41:31Z","receivedAt":"2023-02-22T01:41:46Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Tue, Feb 21, 2023 at 3:46 AM Phillip Wood wrote:\n>\n> On 21/02/2023 05:58, Alex Henrie wrote:\n> > +rebase.merges::\n> > + Default value of `--rebase-merges` option.\n>\n> Below I see this takes a boolean in addition to [no-]rebase-cousins, it\n> would be nice to document that, especially what \"true\" means.\n\nGood point. I'll add another sentence explaining that \"true\" is\nequivalent to --rebase-merges without arguments and \"false\" is\nequivalent to --no-rebase-merges.\n\n> > + if (!strcmp(var, \"rebase.merges\")) {\n> > + const char *rebase_merges;\n> > + if (!git_config_string(&rebase_merges, var, value) &&\n> > + rebase_merges && *rebase_merges) {\n>\n> rebase_merges is guaranteed to be non-NULL if git_config_string returns 0.\n>\n> > + opts->rebase_merges = git_parse_maybe_bool(rebase_merges);\n> > + if (opts->rebase_merges < 0)\n> > + parse_merges_value(opts, rebase_merges);\n> > + }\n> > + return 0;\n> > + }\n>\n> I think this leaks rebase_merges as git_config_string() returns a copy\n> despite taking a \"const char*\".\n\nCome to think of it, I don't think we need git_config_string at all\nhere. As far as I can tell, it should be fine to just check (value &&\n*value) and not duplicate the string in memory.\n\n> > +test_expect_success 'rebase.merges=\"\" is equivalent to not passing --rebase-merges' '\n> > + git config rebase.merges \"\" &&\n>\n> test_config is generally preferred as it clears the value at the end of\n> the test. Then you don't need the final hunk of this patch.\n\nI've used test_config before but I forgot about it. Thanks for the reminder.\n\n-Alex\n"},{"id":"472439","messageId":"36090abb-72a0-f9b7-5a41-ddd8ccde92bd@dunelm.org.uk","threadId":"59277","inReplyTo":"CAMMLpeSMO38acvnxK2QX18CdoaEiUojwLsV+AQZYOTe5h4Nq=Q@mail.gmail.com","subject":"Re: [PATCH v2 2/4] rebase: add tests for --no-rebase-merges","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-22T10:16:05Z","receivedAt":"2023-02-22T10:16:14Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 22/02/2023 01:37, Alex Henrie wrote:\n> On Tue, Feb 21, 2023 at 4:00 AM Phillip Wood wrote:\n>>\n>> On 21/02/2023 05:58, Alex Henrie wrote:\n>>> +test_expect_success 'do not rebase merges unless asked to' '\n>>> + git checkout -b rebase-merges-default E &&\n>>> + before=\"$(git rev-parse --verify HEAD)\" &&\n>>> + test_tick &&\n>>> + git rebase --rebase-merges C &&\n>>\n>> I don't quite follow what this part of the test is for\n> \n> The test is modeled after the existing test \"do not rebase cousins\n> unless asked for\". First, it verifies that --rebase-merges rebases the\n> merges, which in this case results in no changes to the branch. Then,\n> it verifies that `git rebase` without arguments flattens the history.\n\nI think \"do not rebase cousins unless asked for\" is a bit different \nbecause it is checking the default for --rebase-merges which seems \nreasonable. I cannot see the point of checking that --rebase-merges \nworks in this test as we have a whole file of tests that already do that.\n\nBest Wishes\n\nPhillip\n\n\n>>> +test_expect_success '--no-rebase-merges countermands --rebase-merges' '\n>>> + git checkout -b no-rebase-merges E &&\n>>> + git rebase --rebase-merges --no-rebase-merges C &&\n>>> + test_cmp_graph C.. <<-\\EOF\n>>> + * B\n>>> + * D\n>>> + o C\n>>> + EOF\n>>> +'\n>>\n>> This test looks good. I think we could probably have just this second\n>> test\n> \n> I like having a test for the default behavior. Nevertheless, I am\n> happy to remove that test as requested. Does anyone else have an\n> opinion?\n> \n>> and squash this into the previous patch - improving the\n>> documentation and test coverage for --no-rebase-merges would make a nice\n>> self-contained commit.\n> \n> Sure, squashing the first two patches is no problem.\n> \n> Thanks for the feedback,\n> \n> -Alex\n"},{"id":"472440","messageId":"f0d0a3fb-d7eb-84d6-7852-0d30eb38af63@dunelm.org.uk","threadId":"59277","inReplyTo":"CAMMLpeQ9gEKNX5VtGCgLL_Qzk59ZYji57u-SPy-XeSRPuF2NwA@mail.gmail.com","subject":"Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=\"\"","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-02-22T10:18:28Z","receivedAt":"2023-02-22T10:18:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nOn 22/02/2023 01:38, Alex Henrie wrote:\n> On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood wrote:\n>>\n>> On 21/02/2023 05:58, Alex Henrie wrote:\n>>> The unusual syntax --rebase-merges=\"\" (that is, --rebase-merges with an\n>>> empty string argument) has been an undocumented synonym of\n>>> --rebase-merges=no-rebase-cousins. Stop accepting that syntax to avoid\n>>> confusion when a rebase.merges config option is introduced, where\n>>> rebase.merges=\"\" will be equivalent to not passing --rebase-merges.\n>>\n>> I think this is sensible in the context of adding a config option. It is\n>> a backwards incompatible change though, lets hope no one was relying on\n>> it.\n> \n> Since the syntax is bizarre and undocumented, I doubt anyone is\n> relying on it for anything serious, if anyone uses it at all.\n> \n>> Is there a particular reason you decided to redo the option parsing\n>> rather than just calling parse_merges_value() from the existing \"if\n>> (rebase_merges)\" block? I don't think it really matters, I'm just curious.\n> \n> Without a parse_opt_merges callback, how could we know whether the\n> user passed --no-rebase-merges as opposed to passing nothing at all?\n> const char *rebase_merges would be NULL in either case. It's an\n> important distinction to make because --no-rebase-merges overrides\n> rebase.merges but the absence of a command-line argument does not.\n\nThe usual way we handle that is to set the value of rebase_merges from \nthe config before calling parse_options(). However your solution is fine.\n\n> All the same, your comment made me realize that it would probably make\n> more sense to simply change the default value of --rebase-cousins from\n> \"\" to \"no-rebase-cousins\" in this patch and then add the\n> parse_opt_merges callback in the next patch when it is actually\n> needed.\n\nSounds good\n\nPhillip\n\n> -Alex\n"},{"id":"472462","messageId":"xmqq8rgpcn8l.fsf@gitster.g","threadId":"59277","inReplyTo":"f0d0a3fb-d7eb-84d6-7852-0d30eb38af63@dunelm.org.uk","subject":"Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=\"\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-02-22T23:08:10Z","receivedAt":"2023-02-22T23:08:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> The usual way we handle that is to set the value of rebase_merges from\n> the config before calling parse_options(). However your solution is\n> fine.\n\nIs it? If it is not too much to ask, I'd prefer to have code that\ndoes not surprise people, and \"the usual way\" you mentioned is what\nreaders around here expect to see. I didn't check and think about\nthe patch in quetion, and especially the existing code that the\npatch needs to touch, too deeply, so if it is too convoluted already\nthat it would be a lot of work to make it work in \"the usual way\",\nit may be OK, but otherwise, the solution may not be fine. \n\nThanks.\n"},{"id":"472466","messageId":"CAMMLpeRXYm-Qtr6556fZxmND+stku9cK+aBGr7h7ezw+Mrg=-g@mail.gmail.com","threadId":"59277","inReplyTo":"xmqq8rgpcn8l.fsf@gitster.g","subject":"Re: [PATCH v2 3/4] rebase: stop accepting --rebase-merges=\"\"","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-02-22T23:33:21Z","receivedAt":"2023-02-22T23:33:43Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Wed, Feb 22, 2023 at 3:18 AM Phillip Wood wrote:\n>\n> On 22/02/2023 01:38, Alex Henrie wrote:\n> > On Tue, Feb 21, 2023 at 3:55 AM Phillip Wood wrote:\n> >>\n> >> Is there a particular reason you decided to redo the option parsing\n> >> rather than just calling parse_merges_value() from the existing \"if\n> >> (rebase_merges)\" block? I don't think it really matters, I'm just curious.\n> >\n> > Without a parse_opt_merges callback, how could we know whether the\n> > user passed --no-rebase-merges as opposed to passing nothing at all?\n> > const char *rebase_merges would be NULL in either case. It's an\n> > important distinction to make because --no-rebase-merges overrides\n> > rebase.merges but the absence of a command-line argument does not.\n>\n> The usual way we handle that is to set the value of rebase_merges from\n> the config before calling parse_options(). However your solution is fine.\n\nOn Wed, Feb 22, 2023 at 4:08 PM Junio C Hamano wrote:\n>\n> Is it? If it is not too much to ask, I'd prefer to have code that\n> does not surprise people, and \"the usual way\" you mentioned is what\n> readers around here expect to see. I didn't check and think about\n> the patch in quetion, and especially the existing code that the\n> patch needs to touch, too deeply, so if it is too convoluted already\n> that it would be a lot of work to make it work in \"the usual way\",\n> it may be OK, but otherwise, the solution may not be fine.\n\nThere was a const char *rebase_merges in cmd_rebase and an int\nrebase_merges in struct rebase_options. I deleted const char\n*rebase_merges, leaving only int rebase_merges. int rebase_merges is\nset from rebase_config before it is set from parse_options.\n\nDo you want me to add back const char *rebase_merges? If so, where\nshould it be declared so that it can be set from both rebase_config\nand parse_options?\n\n-Alex\n"},{"id":"472476","messageId":"CAMMLpeSAOjo9rghuGKLKNWnz8Wm5SgFamzyPsMvZBB6AMONJTw@mail.gmail.com","threadId":"59277","inReplyTo":"36090abb-72a0-f9b7-5a41-ddd8ccde92bd@dunelm.org.uk","subject":"Re: [PATCH v2 2/4] rebase: add tests for --no-rebase-merges","fromName":"Alex Henrie","fromEmail":"alexhenrie24@gmail.com","sentAt":"2023-02-23T05:35:15Z","receivedAt":"2023-02-23T05:35:32Z","isPatch":true,"sender":{"key":"alexhenrie24@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5951993?v=4"},"body":"On Wed, Feb 22, 2023 at 3:16 AM Phillip Wood wrote:\n>\n> On 22/02/2023 01:37, Alex Henrie wrote:\n> > On Tue, Feb 21, 2023 at 4:00 AM Phillip Wood wrote:\n> >>\n> >> On 21/02/2023 05:58, Alex Henrie wrote:\n> >>> +test_expect_success 'do not rebase merges unless asked to' '\n> >>> + git checkout -b rebase-merges-default E &&\n> >>> + before=\"$(git rev-parse --verify HEAD)\" &&\n> >>> + test_tick &&\n> >>> + git rebase --rebase-merges C &&\n> >>\n> >> I don't quite follow what this part of the test is for\n> >\n> > The test is modeled after the existing test \"do not rebase cousins\n> > unless asked for\". First, it verifies that --rebase-merges rebases the\n> > merges, which in this case results in no changes to the branch. Then,\n> > it verifies that `git rebase` without arguments flattens the history.\n>\n> I think \"do not rebase cousins unless asked for\" is a bit different\n> because it is checking the default for --rebase-merges which seems\n> reasonable. I cannot see the point of checking that --rebase-merges\n> works in this test as we have a whole file of tests that already do that.\n\nI've removed the test in v4. Thanks again for the feedback.\n\n-Alex\n"}]}