{"thread":{"id":"59117","subject":"[PATCH] rebase: mark --update-refs as requiring the merge backend","startedAt":"2023-01-19T05:36:33Z","lastAt":"2023-02-02T23:49:09Z","messageCount":70,"participants":["Elijah Newren via GitGitGadget","Derrick Stolee","Elijah Newren","Eric Sunshine","Martin Ågren","Junio C Hamano","Phillip Wood","Johannes Schindelin"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"470699","messageId":"pull.1466.git.1674106587550.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":null,"subject":"[PATCH] rebase: mark --update-refs as requiring the merge backend","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-19T05:36:27Z","receivedAt":"2023-01-19T05:36:33Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--update-refs is built in terms of the sequencer, which requires the\nmerge backend. It was already marked as incompatible with the apply\nbackend in the git-rebase manual, but the code didn't check for this\nincompatibility and warn the user. Check and warn now.\n\nWhile at it, fix a typo in t3422...and fix some misleading wording (all\nuseful options other than --whitespace=fix have long since been\nimplemented in the merge backend).\n\nSigned-off-by: Elijah Newren \n---\n rebase: mark --update-refs as requiring the merge backend\n \n --update-refs is built in terms of the sequencer, which requires the\n merge backend. It was already marked as incompatible with the apply\n backend in the git-rebase manual, but the code didn't check for this\n incompatibility and warn the user. Check and warn now.\n \n While at it, fix a typo in t3422...and fix some misleading wording (all\n useful options other than --whitespace=fix have long since been\n implemented in the merge backend).\n \n Signed-off-by: Elijah Newren newren@gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1466\n\n builtin/rebase.c | 3 +++\n t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----\n 2 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 1481c5b6a5b..accd62fce48 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1514,6 +1514,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (options.update_refs)\n+\t\timply_merge(&options, \"--update-refs\");\n+\n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\n \t\t\timply_merge(&options, \"--merge\");\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 6dabb05a2ad..5a00618d265 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -25,11 +25,11 @@ test_expect_success 'setup' '\n '\n \n #\n-# Rebase has lots of useful options like --whitepsace=fix, which are\n-# actually all built in terms of flags to git-am. Since neither\n-# --merge nor --interactive (nor any options that imply those two) use\n-# git-am, using them together will result in flags like --whitespace=fix\n-# being ignored. Make sure rebase warns the user and aborts instead.\n+# Rebase has a useful option, --whitespace=fix, which is actually\n+# built in terms of flags to git-am. Since neither --merge nor\n+# --interactive (nor any options that imply those two) use git-am,\n+# using them together will result in --whitespace=fix being ignored.\n+# Make sure rebase warns the user and aborts instead.\n #\n \n test_rebase_am_only () {\n@@ -60,6 +60,11 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --exec 'true' A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --update-refs\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --update-refs A\n+\t\"\n+\n }\n \n test_rebase_am_only --whitespace=fix\n\nbase-commit: 2b4f5a4e4bb102ac8d967cea653ed753b608193c\n-- \ngitgitgadget\n"},{"id":"470738","messageId":"f480813c-7583-179f-0149-d970d3f2519f@github.com","threadId":"59117","inReplyTo":"pull.1466.git.1674106587550.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase: mark --update-refs as requiring the merge backend","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-19T21:47:53Z","receivedAt":"2023-01-19T22:11:00Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/19/2023 12:36 AM, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> --update-refs is built in terms of the sequencer, which requires the\n> merge backend. It was already marked as incompatible with the apply\n> backend in the git-rebase manual, but the code didn't check for this\n> incompatibility and warn the user. Check and warn now.\n\nThank you for submitting this version.\n\n> @@ -1514,6 +1514,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \t}\n> \n> +\tif (options.update_refs)\n> +\t\timply_merge(&options, \"--update-refs\");\n> +\n\nThis solution is very elegant. The only downside is the lack of warning\nif --update-refs was implied by rebase.updateRefs=true, but I'm happy to\ndelay implementing that warning in favor of your complete solution here.\n\nThinking ahead to that option, are there other options that are implied\nby config that are required in imply_merge()? Is --autosquash in that\ncamp? If so, then maybe it would make sense to expand imply_merge() to\ninclude a boolean config key and supply that warning within its\nimplementation. (This consideration should not block this patch, as it\nis complete as-is.)\n\n> While at it, fix a typo in t3422...and fix some misleading wording (all\n> useful options other than --whitespace=fix have long since been\n> implemented in the merge backend).\n\n> #\n> -# Rebase has lots of useful options like --whitepsace=fix, which are\n> -# actually all built in terms of flags to git-am. Since neither\n> -# --merge nor --interactive (nor any options that imply those two) use\n> -# git-am, using them together will result in flags like --whitespace=fix\n> -# being ignored. Make sure rebase warns the user and aborts instead.\n> +# Rebase has a useful option, --whitespace=fix, which is actually\n> +# built in terms of flags to git-am. Since neither --merge nor\n> +# --interactive (nor any options that imply those two) use git-am,\n> +# using them together will result in --whitespace=fix being ignored.\n> +# Make sure rebase warns the user and aborts instead.\n> #\n\nThanks for the update here. The -C option is also used in this test,\nso --whitespace=fix isn't the only option, right? At least, -C doesn't\nmake sense to port over to the merge backend, so maybe that's what\nyou mean by --whitespace=fix being the last one?\n\nThe user could also explicitly request the apply backend with --apply,\nbut this test script doesn't check it, strangely. That seems like an\noversight, but not a drawback to your patch.\n\n> test_rebase_am_only () {\n> @@ -60,6 +60,11 @@ test_rebase_am_only () {\n> \t\ttest_must_fail git rebase $opt --exec 'true' A\n> \t\"\n> \n> +\ttest_expect_success \"$opt incompatible with --update-refs\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --update-refs A\n> +\t\"\n> +\n\nThanks for adding this test. I would delay the rebase.updateRefs\nversion until there is extra behavior to check, such as the\nwarning message.\n\nThanks,\n-Stolee\n"},{"id":"470761","messageId":"CABPp-BFMJZx8BfWK5NPSt_4Mjz+MGA7d6_jwBivm-5A9fovbWw@mail.gmail.com","threadId":"59117","inReplyTo":"f480813c-7583-179f-0149-d970d3f2519f@github.com","subject":"Re: [PATCH] rebase: mark --update-refs as requiring the merge backend","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-20T01:54:08Z","receivedAt":"2023-01-20T01:54:26Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jan 19, 2023 at 1:47 PM Derrick Stolee wrote:\n>\n> On 1/19/2023 12:36 AM, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > --update-refs is built in terms of the sequencer, which requires the\n> > merge backend. It was already marked as incompatible with the apply\n> > backend in the git-rebase manual, but the code didn't check for this\n> > incompatibility and warn the user. Check and warn now.\n>\n> Thank you for submitting this version.\n>\n> > @@ -1514,6 +1514,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > }\n> > }\n> >\n> > + if (options.update_refs)\n> > + imply_merge(&options, \"--update-refs\");\n> > +\n>\n> This solution is very elegant. The only downside is the lack of warning\n> if --update-refs was implied by rebase.updateRefs=true, but I'm happy to\n> delay implementing that warning in favor of your complete solution here.\n>\n> Thinking ahead to that option, are there other options that are implied\n> by config that are required in imply_merge()? Is --autosquash in that\n> camp? If so, then maybe it would make sense to expand imply_merge() to\n> include a boolean config key and supply that warning within its\n> implementation. (This consideration should not block this patch, as it\n> is complete as-is.)\n\nOoh, that does sound like a useful future idea to improve things further.\n\n> > While at it, fix a typo in t3422...and fix some misleading wording (all\n> > useful options other than --whitespace=fix have long since been\n> > implemented in the merge backend).\n>\n> > #\n> > -# Rebase has lots of useful options like --whitepsace=fix, which are\n> > -# actually all built in terms of flags to git-am. Since neither\n> > -# --merge nor --interactive (nor any options that imply those two) use\n> > -# git-am, using them together will result in flags like --whitespace=fix\n> > -# being ignored. Make sure rebase warns the user and aborts instead.\n> > +# Rebase has a useful option, --whitespace=fix, which is actually\n> > +# built in terms of flags to git-am. Since neither --merge nor\n> > +# --interactive (nor any options that imply those two) use git-am,\n> > +# using them together will result in --whitespace=fix being ignored.\n> > +# Make sure rebase warns the user and aborts instead.\n> > #\n>\n> Thanks for the update here. The -C option is also used in this test,\n> so --whitespace=fix isn't the only option, right? At least, -C doesn't\n> make sense to port over to the merge backend, so maybe that's what\n> you mean by --whitespace=fix being the last one?\n\nSorry if it was confusing. I was trying to figure out how to word it\nto remove the old confusion, and actually spent a while thinking about\nthis point you brought up...but after a while gave up and just\nsubmitted my patch because it was taking too long and I didn't think\nthe -C option was worth as much brain power as has been spent on it.\nBut since you noted the incongruity, let me explain my thought process\na little...\n\n* I stated that --whitespace=... was the last *useful* option, not\nthat it was the last option. Yes, that was an implicit assertion that\n-C is not useful, though I avoided stating it directly for fear I'd\nhave to dig up proof and/or spend a bunch of time trying to reproduce\nan old bug and debug it.\n\n* The correct way to port the '-C' option to the merge backend would\nprobably be to just accept the flag and ignore it. The merge backend\nalready functions with entire files or essentially infinite context.\nI can't think of any case where ignoring this option would result in\nnegative surprises, other than possibly confusing people about how the\nalgorithm behaves and making them decide to tweak it to no avail. And\neven if users were to waste time twiddling that flag in combination\nwith the merge backend, even that might be a correct \"port\" of the\nexisting -C option because...\n\n* I once tried to use the '-C' option with the apply backend to see if\nit could workaround a bug inherent to the apply backend (where people\nhave a source file with several very similar code blocks, and rebase\nor am applying changes to the wrong one due to fuzz factor and large\ndeletions/insertions having happened upstream elsewhere in the file).\nIt appeared to have absolutely no effect. I suspected it was buggy,\nbut didn't feel the option was worth debugging (I thought switching\nthe default rebase backend was a much better use of time).\n\n* We have *zero* tests of the functionality of rebase's -C option in\nthe testsuite. The only testing we do for rebase's -C option is\nsolely around option parsing.\n\n....\n\nAnd you inspired me to do some digging. rebase -C was introduced with\n67dad687ad (\"add -C[NUM] to git-am\", 2007-02-08). Based on feedback\non the patch, the author added the -C option not just to git-am but\nalso to git-rebase. However, the way it was added to rebase was to\njust pass it through to git-am, with no corresponding option to\nformat-patch. Therefore, format-patch is going to continue generating\npatches with 3 lines of context, while we tell git-am/git-apply to pay\nattention to more than 3 lines of context. The -C option is\ndocumented as using all available context if exceeds the number\nof lines of context provided in the input patches. So, the -C option\nto rebase has never done anything useful, and the port from the legacy\nrebase script to the builtin C did not accidentally introduce any\nmodicum of utility to this option; rather, it faithfully ported this\npointless option precisely as-is.\n\nSo, I'll adjust this series to include a preliminary patch that rips\nthe rebase -C option out.\n\n> The user could also explicitly request the apply backend with --apply,\n> but this test script doesn't check it, strangely. That seems like an\n> oversight, but not a drawback to your patch.\n>\n> > test_rebase_am_only () {\n> > @@ -60,6 +60,11 @@ test_rebase_am_only () {\n> > test_must_fail git rebase $opt --exec 'true' A\n> > \"\n> >\n> > + test_expect_success \"$opt incompatible with --update-refs\" \"\n> > + git checkout B^0 &&\n> > + test_must_fail git rebase $opt --update-refs A\n> > + \"\n> > +\n>\n> Thanks for adding this test. I would delay the rebase.updateRefs\n> version until there is extra behavior to check, such as the\n> warning message.\n>\n> Thanks,\n> -Stolee\n"},{"id":"470767","messageId":"pull.1466.v2.git.1674190573.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.git.1674106587550.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] rebase: mark --update-refs as requiring the merge backend","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-20T04:56:11Z","receivedAt":"2023-01-20T05:10:12Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"--update-refs is built in terms of the sequencer, which requires the merge\nbackend. It was already marked as incompatible with the apply backend in the\ngit-rebase manual, but the code didn't check for this incompatibility and\nwarn the user. Check and warn now.\n\nWhile at it, fix a typo in t3422...and fix some misleading wording (all\nuseful options other than --whitespace=fix have long since been implemented\nin the merge backend).\n\nChanges since v1:\n\n * Add a patch nuking the -C option to rebase (fixes confusion around the\n comment in t3422 and acknowledges the fact that the option is totally and\n utterly useless and always has been. It literally never affects the\n results of a rebase.)\n\nSigned-off-by: Elijah Newren newren@gmail.com\n\nElijah Newren (2):\n rebase: remove completely useless -C option\n rebase: mark --update-refs as requiring the merge backend\n\n Documentation/git-rebase.txt | 9 ---------\n builtin/rebase.c | 12 ++++--------\n t/t3406-rebase-message.sh | 7 -------\n t/t3422-rebase-incompatible-options.sh | 16 ++++++++++------\n 4 files changed, 14 insertions(+), 30 deletions(-)\n\n\nbase-commit: 2b4f5a4e4bb102ac8d967cea653ed753b608193c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1466\n\nRange-diff vs v1:\n\n -: ----------- > 1: a0f8f5fac1c rebase: remove completely useless -C option\n 1: f7459f0996b = 2: 2e44d0b7e57 rebase: mark --update-refs as requiring the merge backend\n\n-- \ngitgitgadget\n"},{"id":"470768","messageId":"a0f8f5fac1c3f79cd46b943e95636728677dffef.1674190573.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v2.git.1674190573.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] rebase: remove completely useless -C option","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-20T04:56:12Z","receivedAt":"2023-01-20T05:10:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe `-C` option to rebase was introduced with 67dad687ad (\"add -C[NUM]\nto git-am\", 2007-02-08).  Based on feedback on the patch, the author\nadded the -C option not just to git-am but also to git-rebase.  But did\nit such that the option was just passed through to git-am (which passes\nit along to git-apply), with no corresponding option to format-patch.\n\nAs per the git-apply documentation for the `-C` option:\n Ensure at least lines of surrounding context match...When fewer\n lines of surrounding context exist they all must match.\n\nThe fact that format-patch was not passed a -U option to increase the\nnumber of context lines meant that there would still only be 3 lines of\ncontext to match on. So, anyone attempting to use this option in\ngit-rebase would just be spinning their wheels and wasting time. I was\none such user a number of years ago.\n\nSince this option can at best waste users time and is nothing more than\na confusing no-op, and has never been anything other than a confusing\nno-op, and no one has ever bothered to create a testcase for it that\ngoes beyond option parsing, simply excise the option.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 9 ---------\n builtin/rebase.c | 9 +--------\n t/t3406-rebase-message.sh | 7 -------\n t/t3422-rebase-incompatible-options.sh | 1 -\n 4 files changed, 1 insertion(+), 25 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex f9675bd24e6..412887deda7 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -416,14 +416,6 @@ include::rerere-options.txt[]\n \tAllows the pre-rebase hook to run, which is the default. This option can\n \tbe used to override `--no-verify`. See also linkgit:githooks[5].\n \n--C::\n-\tEnsure at least `` lines of surrounding context match before\n-\tand after each change. When fewer lines of surrounding\n-\tcontext exist they all must match. By default no context is\n-\tever ignored. Implies `--apply`.\n-+\n-See also INCOMPATIBLE OPTIONS below.\n-\n --no-ff::\n --force-rebase::\n -f::\n@@ -631,7 +623,6 @@ The following options:\n \n * --apply\n * --whitespace\n- * -C\n \n are incompatible with the following options:\n \ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 1481c5b6a5b..ace8bd4a41c 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1067,8 +1067,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\t N_(\"ignore author date and use current date\")),\n \t\tOPT_HIDDEN_BOOL(0, \"ignore-date\", &options.ignore_date,\n \t\t\t\tN_(\"synonym of --reset-author-date\")),\n-\t\tOPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_(\"n\"),\n-\t\t\t\t N_(\"passed to 'git apply'\"), 0),\n \t\tOPT_BOOL(0, \"ignore-whitespace\", &ignore_whitespace,\n \t\t\t N_(\"ignore changes in whitespace\")),\n \t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n@@ -1390,12 +1388,7 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tif (!strcmp(option, \"--whitespace=fix\") ||\n \t\t !strcmp(option, \"--whitespace=strip\"))\n \t\t\tallow_preemptive_ff = 0;\n-\t\telse if (skip_prefix(option, \"-C\", &p)) {\n-\t\t\twhile (*p)\n-\t\t\t\tif (!isdigit(*(p++)))\n-\t\t\t\t\tdie(_(\"switch `C' expects a \"\n-\t\t\t\t\t \"numerical value\"));\n-\t\t} else if (skip_prefix(option, \"--whitespace=\", &p)) {\n+\t\telse if (skip_prefix(option, \"--whitespace=\", &p)) {\n \t\t\tif (*p && strcmp(p, \"warn\") && strcmp(p, \"nowarn\") &&\n \t\t\t strcmp(p, \"error\") && strcmp(p, \"error-all\"))\n \t\t\t\tdie(\"Invalid whitespace option: '%s'\", p);\ndiff --git a/t/t3406-rebase-message.sh b/t/t3406-rebase-message.sh\nindex ceca1600053..c8dca845dd7 100755\n--- a/t/t3406-rebase-message.sh\n+++ b/t/t3406-rebase-message.sh\n@@ -78,13 +78,6 @@ test_expect_success 'rebase --onto outputs the invalid ref' '\n \ttest_i18ngrep \"invalid-ref\" err\n '\n \n-test_expect_success 'error out early upon -C or --whitespace=' '\n-\ttest_must_fail git rebase -Cnot-a-number HEAD 2>err &&\n-\ttest_i18ngrep \"numerical value\" err &&\n-\ttest_must_fail git rebase --whitespace=bad HEAD 2>err &&\n-\ttest_i18ngrep \"Invalid whitespace option\" err\n-'\n-\n write_reflog_expect () {\n \tif test $mode = --apply\n \tthen\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 6dabb05a2ad..ebcbd79ab8a 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -63,6 +63,5 @@ test_rebase_am_only () {\n }\n \n test_rebase_am_only --whitespace=fix\n-test_rebase_am_only -C4\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"470769","messageId":"2e44d0b7e571cfac2a25d00f3fe3d143c895793b.1674190573.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v2.git.1674190573.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] rebase: mark --update-refs as requiring the merge backend","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-20T04:56:13Z","receivedAt":"2023-01-20T05:10:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--update-refs is built in terms of the sequencer, which requires the\nmerge backend. It was already marked as incompatible with the apply\nbackend in the git-rebase manual, but the code didn't check for this\nincompatibility and warn the user. Check and warn now.\n\nWhile at it, fix a typo in t3422...and fix some misleading wording (all\nuseful options other than --whitespace=fix have long since been\nimplemented in the merge backend).\n\nSigned-off-by: Elijah Newren \n---\n builtin/rebase.c | 3 +++\n t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----\n 2 files changed, 13 insertions(+), 5 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex ace8bd4a41c..e8bcdbf9fcd 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1507,6 +1507,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (options.update_refs)\n+\t\timply_merge(&options, \"--update-refs\");\n+\n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\n \t\t\timply_merge(&options, \"--merge\");\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex ebcbd79ab8a..d72c863b21b 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -25,11 +25,11 @@ test_expect_success 'setup' '\n '\n \n #\n-# Rebase has lots of useful options like --whitepsace=fix, which are\n-# actually all built in terms of flags to git-am. Since neither\n-# --merge nor --interactive (nor any options that imply those two) use\n-# git-am, using them together will result in flags like --whitespace=fix\n-# being ignored. Make sure rebase warns the user and aborts instead.\n+# Rebase has a useful option, --whitespace=fix, which is actually\n+# built in terms of flags to git-am. Since neither --merge nor\n+# --interactive (nor any options that imply those two) use git-am,\n+# using them together will result in --whitespace=fix being ignored.\n+# Make sure rebase warns the user and aborts instead.\n #\n \n test_rebase_am_only () {\n@@ -60,6 +60,11 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --exec 'true' A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --update-refs\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --update-refs A\n+\t\"\n+\n }\n \n test_rebase_am_only --whitespace=fix\n-- \ngitgitgadget\n"},{"id":"470772","messageId":"CAPig+cQj4Gi+ikkPz3wffqGZVz5uALGzYV25nio3k4+cthP9Uw@mail.gmail.com","threadId":"59117","inReplyTo":"a0f8f5fac1c3f79cd46b943e95636728677dffef.1674190573.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] rebase: remove completely useless -C option","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2023-01-20T05:40:19Z","receivedAt":"2023-01-20T05:42:09Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Jan 20, 2023 at 12:14 AM Elijah Newren via GitGitGadget\n wrote:\n> The `-C` option to rebase was introduced with 67dad687ad (\"add -C[NUM]\n> to git-am\", 2007-02-08). Based on feedback on the patch, the author\n> added the -C option not just to git-am but also to git-rebase. But did\n> it such that the option was just passed through to git-am (which passes\n> it along to git-apply), with no corresponding option to format-patch.\n>\n> As per the git-apply documentation for the `-C` option:\n> Ensure at least lines of surrounding context match...When fewer\n> lines of surrounding context exist they all must match.\n>\n> The fact that format-patch was not passed a -U option to increase the\n> number of context lines meant that there would still only be 3 lines of\n> context to match on. So, anyone attempting to use this option in\n> git-rebase would just be spinning their wheels and wasting time. I was\n> one such user a number of years ago.\n>\n> Since this option can at best waste users time and is nothing more than\n> a confusing no-op, and has never been anything other than a confusing\n> no-op, and no one has ever bothered to create a testcase for it that\n> goes beyond option parsing, simply excise the option.\n>\n> Signed-off-by: Elijah Newren \n\nIs there a chance that this patch could break some existing tooling or\nsomeone's workflow or alias? If so, perhaps it would be better to\ncontinue recognizing it, but as a proper no-op? That is:\n\n(1) continue accepting the option but mark it PARSE_OPT_HIDDEN or some such\n\n(2) completely ignore the option\n\n(3) in case someone runs across it in some existing script or example\nand wants to know what it does, leave one mention of it in the\ndocumentation, such as:\n\n -C\n Does nothing. This option is accepted for backward compatibility\n only. Do not use.\n"},{"id":"470773","messageId":"CABPp-BHCKweQR=i8sZx2Nrh3YTXCThys7=EeN+jsxvLb=L_KVg@mail.gmail.com","threadId":"59117","inReplyTo":"CAPig+cQj4Gi+ikkPz3wffqGZVz5uALGzYV25nio3k4+cthP9Uw@mail.gmail.com","subject":"Re: [PATCH v2 1/2] rebase: remove completely useless -C option","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-20T06:42:32Z","receivedAt":"2023-01-20T06:42:52Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Jan 19, 2023 at 9:40 PM Eric Sunshine wrote:\n>\n> On Fri, Jan 20, 2023 at 12:14 AM Elijah Newren via GitGitGadget\n> wrote:\n> > The `-C` option to rebase was introduced with 67dad687ad (\"add -C[NUM]\n> > to git-am\", 2007-02-08). Based on feedback on the patch, the author\n> > added the -C option not just to git-am but also to git-rebase. But did\n> > it such that the option was just passed through to git-am (which passes\n> > it along to git-apply), with no corresponding option to format-patch.\n> >\n> > As per the git-apply documentation for the `-C` option:\n> > Ensure at least lines of surrounding context match...When fewer\n> > lines of surrounding context exist they all must match.\n> >\n> > The fact that format-patch was not passed a -U option to increase the\n> > number of context lines meant that there would still only be 3 lines of\n> > context to match on. So, anyone attempting to use this option in\n> > git-rebase would just be spinning their wheels and wasting time. I was\n> > one such user a number of years ago.\n> >\n> > Since this option can at best waste users time and is nothing more than\n> > a confusing no-op, and has never been anything other than a confusing\n> > no-op, and no one has ever bothered to create a testcase for it that\n> > goes beyond option parsing, simply excise the option.\n> >\n> > Signed-off-by: Elijah Newren \n>\n> Is there a chance that this patch could break some existing tooling or\n> someone's workflow or alias? If so, perhaps it would be better to\n> continue recognizing it, but as a proper no-op? That is:\n>\n> (1) continue accepting the option but mark it PARSE_OPT_HIDDEN or some such\n>\n> (2) completely ignore the option\n>\n> (3) in case someone runs across it in some existing script or example\n> and wants to know what it does, leave one mention of it in the\n> documentation, such as:\n>\n> -C\n> Does nothing. This option is accepted for backward compatibility\n> only. Do not use.\n\nIf an option becomes a no-op over time due to other changes, this\nwould likely be the route I'd lean towards. I'm just not sure it's\nmerited in this case.\n\nWe already turned numerous no-op choices in rebase into errors with\ncommit 8295ed690b (\"rebase: make the backend configurable via config\nsetting\", 2020-02-15), where we started pointing out that apply-based\nand merge-based options were incompatible (as opposed to silently\naccepting competing options and ignoring ones not supported by\nwhichever backend happened to trump at the time based on the options.)\n So, there's recent precedent to jump directly to errors with no-ops\nin rebase. Those cases are slightly different in that options were\nonly conditionally no-ops, whereas in this case we have an option that\nis unconditionally a no-op, suggesting we might be able to do\nsomething stronger.\n\nOn top of that, I just can't find any evidence of anyone ever using\nthis option: (a) it was only put in as an afterthought by the original\nauthor (who was only really interested in git-am -C), (b) there are\nabsolutely no testcases in the testsuite covering its behavior, (c) my\nsearches of the mailing list and google don't turn up any hits though\nadmittedly it's hard to figure out what to search on, and (d) while I\ntried to use the option at times, it was only because I was doing work\nto make backends consistent and switch the default one and trying to\nunderstand all the inner workings, and even then I gave up on it and\npunted it down the road.\n\nAnd, of course, the option never worked as advertised anyway.\n\nThat combination makes me think nuking it would go completely\nunnoticed outside this mailing list. If others would rather see the\nmore cautious route despite all the above, I can implement it, but\nlikely alter your step (2) from ignoring into throwing an error\nmessage (or at the very least a warning).\n"},{"id":"470777","messageId":"CAN0heSo8cqNqhx9+AJZJS2rwJxBG_HZjhgxQZf8nw5t82NDXBg@mail.gmail.com","threadId":"59117","inReplyTo":"a0f8f5fac1c3f79cd46b943e95636728677dffef.1674190573.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] rebase: remove completely useless -C option","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2023-01-20T09:55:25Z","receivedAt":"2023-01-20T09:56:22Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Fri, 20 Jan 2023 at 06:27, Elijah Newren via GitGitGadget\n wrote:\n>\n> As per the git-apply documentation for the `-C` option:\n> Ensure at least lines of surrounding context match...When fewer\n> lines of surrounding context exist they all must match.\n>\n> The fact that format-patch was not passed a -U option to increase the\n> number of context lines meant that there would still only be 3 lines of\n> context to match on. So, anyone attempting to use this option in\n> git-rebase would just be spinning their wheels and wasting time. I was\n> one such user a number of years ago.\n\nI suppose someone might have something like GIT_DIFF_OPTS=\"--unified=20\"\nmeaning they would actually have more context for their `-C` to act on.\nSo I guess there is a chance that someone somewhere has actually been\nable to make use of `git rebase -C` after all? I'm not really arguing\neither way -- just noting the possibility, as remote as it may be.\n\nMartin\n"},{"id":"470779","messageId":"xmqq5yd1za0t.fsf@gitster.g","threadId":"59117","inReplyTo":"a0f8f5fac1c3f79cd46b943e95636728677dffef.1674190573.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] rebase: remove completely useless -C option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-20T12:05:22Z","receivedAt":"2023-01-20T12:05:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> From: Elijah Newren \n>\n> The `-C` option to rebase was introduced with 67dad687ad (\"add -C[NUM]\n> to git-am\", 2007-02-08).\n> ...\n> As per the git-apply documentation for the `-C` option:\n> Ensure at least lines of surrounding context match...When fewer\n> lines of surrounding context exist they all must match.\n>\n> The fact that format-patch was not passed a -U option to increase the\n> number of context lines meant that there would still only be 3 lines of\n> context to match on.\n\nI am afraid that this is only less than half true. Isn't the\nintended use of -C similar to how \"patch --fuzz\" is used?\n\nThat is, even when a patch does not cleanly apply with full context\nin the incoming diff, by requiring *smaller* number of lines to\nmatch, the diff *could* be forced to apply with reduced precision?\n\nMy read of \"even if context has changed a bit\" in the log of that\ncommit is exactly that. And for such a use case (which I think is\nthe primary use case for the feature), you do not need to futz with\nthe patch generation side at all.\n\ncommit 67dad687ad15d26d8e26f4d27874af0bc0965ce2\nAuthor: Michael S. Tsirkin \nDate: Thu Feb 8 15:57:08 2007 +0200\n\n add -C[NUM] to git-am\n \n Add -C[NUM] to git-am and git-rebase so that patches can be applied even\n if context has changed a bit.\n \n Signed-off-by: Michael S. Tsirkin \n Signed-off-by: Junio C Hamano \n"},{"id":"470785","messageId":"xmqqr0vpxm3d.fsf@gitster.g","threadId":"59117","inReplyTo":"f480813c-7583-179f-0149-d970d3f2519f@github.com","subject":"Re: [PATCH] rebase: mark --update-refs as requiring the merge backend","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-20T15:27:34Z","receivedAt":"2023-01-20T15:27:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>> +\tif (options.update_refs)\n>> +\t\timply_merge(&options, \"--update-refs\");\n>> +\n>\n> This solution is very elegant. The only downside is the lack of warning\n> if --update-refs was implied by rebase.updateRefs=true, but I'm happy to\n> delay implementing that warning in favor of your complete solution here.\n\nIf features A and B are incompatible and both can be specified from\nboth command line and configuration, ideally I would expect the\nsystem to operate in one of two ways. I haven't thought it through\nto decide which one I prefer between the two.\n\n * Take \"command line trumps configuration\" one step further, so\n that A that is configured but not asked for from the command\n line is defeated by B that is asked for from the command line.\n\n This way, only when A and B are both requested via the\n configuration, of via the command line, we'd fail the operation\n by saying A and B are incompatible. Otherwise, the one that is\n configured but overridden is turned off (either silently or with\n a warning).\n\n * Declare \"command line trumps configuration\" is only among the\n same feature. Regardless of how features A and B that are\n incompatible are requested, the command will error out, citing\n incompatibility. It would be very nice if the warning mentioned\n where the requests for features A and B came from (e.g. \"You\n asked for -B from the command line, but you have A configured,\n and both cannot be active at the same time---disable A from the\n command line, or do not ask for B\")\n\n When A is configured and B is requested from the command line,\n the command will error out, and the user must defeat A from the\n command line before the user can use B, e.g. \"git cmd --no-A -B\".\n\nA knee-jerk reaction to the situation is that the latter feels\nsomewhat safer than the former, but when I imagine the actual end\nuser who saw the error message, especially the suggested solution\n\"disable A from the command line or do not ask for B from the\ncommand line\", may say \"well, I asked for B for this invocation\nexplicitly with -B from the command line, and you(Git) should be\nable to make it imply --no-A\", which amounts to the same thing as\nthe former choice.\n\n"},{"id":"470786","messageId":"CABPp-BFakaEqnpW4Xn1rzcOC6oVmcEz+OxBV4dKA_TJZ-xbTvg@mail.gmail.com","threadId":"59117","inReplyTo":"xmqq5yd1za0t.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] rebase: remove completely useless -C option","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-20T15:31:15Z","receivedAt":"2023-01-20T15:31:32Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > From: Elijah Newren \n> >\n> > The `-C` option to rebase was introduced with 67dad687ad (\"add -C[NUM]\n> > to git-am\", 2007-02-08).\n> > ...\n> > As per the git-apply documentation for the `-C` option:\n> > Ensure at least lines of surrounding context match...When fewer\n> > lines of surrounding context exist they all must match.\n> >\n> > The fact that format-patch was not passed a -U option to increase the\n> > number of context lines meant that there would still only be 3 lines of\n> > context to match on.\n>\n> I am afraid that this is only less than half true. Isn't the\n> intended use of -C similar to how \"patch --fuzz\" is used?\n>\n> That is, even when a patch does not cleanly apply with full context\n> in the incoming diff, by requiring *smaller* number of lines to\n> match, the diff *could* be forced to apply with reduced precision?\n\nOh! Reducing the number of lines of context to pay attention to never\neven occurred to me for whatever reason. I'll drop the patch.\n"},{"id":"470787","messageId":"CABPp-BHjcD=sa5ACorrzSmcGNrksQ-Jr0Z2MVptOJjWcN=h4Rw@mail.gmail.com","threadId":"59117","inReplyTo":"CAN0heSo8cqNqhx9+AJZJS2rwJxBG_HZjhgxQZf8nw5t82NDXBg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] rebase: remove completely useless -C option","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-20T15:32:04Z","receivedAt":"2023-01-20T15:32:20Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 20, 2023 at 1:55 AM Martin Ågren wrote:\n>\n> On Fri, 20 Jan 2023 at 06:27, Elijah Newren via GitGitGadget\n> wrote:\n> >\n> > As per the git-apply documentation for the `-C` option:\n> > Ensure at least lines of surrounding context match...When fewer\n> > lines of surrounding context exist they all must match.\n> >\n> > The fact that format-patch was not passed a -U option to increase the\n> > number of context lines meant that there would still only be 3 lines of\n> > context to match on. So, anyone attempting to use this option in\n> > git-rebase would just be spinning their wheels and wasting time. I was\n> > one such user a number of years ago.\n>\n> I suppose someone might have something like GIT_DIFF_OPTS=\"--unified=20\"\n> meaning they would actually have more context for their `-C` to act on.\n> So I guess there is a chance that someone somewhere has actually been\n> able to make use of `git rebase -C` after all? I'm not really arguing\n> either way -- just noting the possibility, as remote as it may be.\n>\n> Martin\n\nAh, good point. And combined with Junio's point that -C is apparently\nabout reducing rather than increasing context, I should just drop the\npatch.\n"},{"id":"470793","messageId":"xmqqh6wlxjuo.fsf@gitster.g","threadId":"59117","inReplyTo":"CABPp-BFakaEqnpW4Xn1rzcOC6oVmcEz+OxBV4dKA_TJZ-xbTvg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] rebase: remove completely useless -C option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-20T16:15:59Z","receivedAt":"2023-01-20T16:16:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano wrote:\n>> ...\n>> That is, even when a patch does not cleanly apply with full context\n>> in the incoming diff, by requiring *smaller* number of lines to\n>> match, the diff *could* be forced to apply with reduced precision?\n>\n> Oh! Reducing the number of lines of context to pay attention to never\n> even occurred to me for whatever reason. I'll drop the patch.\n\nYup. \"completely useless\" on the title is less than half correct,\nbut still correct for a minor use case where -C is used to use\n*more* context lines than the default.\n\nWe could update \"rebase --apply\" codepath to increase the context\nlines generated by format-patch. That would make the \"completely\nuseless\" totally incorrect ;-)\n"},{"id":"470796","messageId":"bb75c8e1-05d3-1359-e06a-ee013ae677da@dunelm.org.uk","threadId":"59117","inReplyTo":"2e44d0b7e571cfac2a25d00f3fe3d143c895793b.1674190573.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] rebase: mark --update-refs as requiring the merge backend","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-20T16:46:04Z","receivedAt":"2023-01-20T16:46:11Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nThanks for working on this\n\nOn 20/01/2023 04:56, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> --update-refs is built in terms of the sequencer, which requires the\n> merge backend. It was already marked as incompatible with the apply\n> backend in the git-rebase manual, but the code didn't check for this\n> incompatibility and warn the user. Check and warn now.\n\nStrictly speaking we die rather than warn but I don't think that \nwarrants a re-roll. I just had a quick look to see how easy it would be \nto add the advice Stolee's patch had if the user has set \nrebase.updaterefs but does not pass \"--no-update-refs\" when using the \napply backend but it looks a bit fiddly unfortunately as we could die in \nimply_merge() or later on.\n\nThinking more generally, imply_merge() does a good job of telling the \nuser which option is incompatible with \"--apply\" but if the user passes \na merge option with \"--whitespace=fix\" and omits \"--apply\" then we just \nprint a generic message saying \"apply options and merge options cannot \nbe used together\" which isn't terribly helpful to the user (doubly so \nwhen the merge option come from a config setting).\n\nI've also noticed that \"--autosquash\" is ignored if we end up using the \napply backend. That's a separate issue but shares the \"this may have \ncome from a config setting rather than a command line argument\" problem.\n\nAll in all I'm not sure if it is friendlier to die when the user has \nrebsase.updaterefs set and they try to rebase with \"--whitespace=fix\" or \nif it is better just to ignore the config in that case. If we can find a \nway to print some help when we die in that case it would be nicer for \nthe user.\n\nBest Wishes\n\nPhillip\n\n> While at it, fix a typo in t3422...and fix some misleading wording (all\n> useful options other than --whitespace=fix have long since been\n> implemented in the merge backend).\n> \n> Signed-off-by: Elijah Newren \n> ---\n> builtin/rebase.c | 3 +++\n> t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----\n> 2 files changed, 13 insertions(+), 5 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index ace8bd4a41c..e8bcdbf9fcd 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1507,6 +1507,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\t}\n> \t}\n> \n> +\tif (options.update_refs)\n> +\t\timply_merge(&options, \"--update-refs\");\n> +\n> \tif (options.type == REBASE_UNSPECIFIED) {\n> \t\tif (!strcmp(options.default_backend, \"merge\"))\n> \t\t\timply_merge(&options, \"--merge\");\n> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\n> index ebcbd79ab8a..d72c863b21b 100755\n> --- a/t/t3422-rebase-incompatible-options.sh\n> +++ b/t/t3422-rebase-incompatible-options.sh\n> @@ -25,11 +25,11 @@ test_expect_success 'setup' '\n> '\n> \n> #\n> -# Rebase has lots of useful options like --whitepsace=fix, which are\n> -# actually all built in terms of flags to git-am. Since neither\n> -# --merge nor --interactive (nor any options that imply those two) use\n> -# git-am, using them together will result in flags like --whitespace=fix\n> -# being ignored. Make sure rebase warns the user and aborts instead.\n> +# Rebase has a useful option, --whitespace=fix, which is actually\n> +# built in terms of flags to git-am. Since neither --merge nor\n> +# --interactive (nor any options that imply those two) use git-am,\n> +# using them together will result in --whitespace=fix being ignored.\n> +# Make sure rebase warns the user and aborts instead.\n> #\n> \n> test_rebase_am_only () {\n> @@ -60,6 +60,11 @@ test_rebase_am_only () {\n> \t\ttest_must_fail git rebase $opt --exec 'true' A\n> \t\"\n> \n> +\ttest_expect_success \"$opt incompatible with --update-refs\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --update-refs A\n> +\t\"\n> +\n> }\n> \n> test_rebase_am_only --whitespace=fix\n"},{"id":"470797","messageId":"CABPp-BHDhpSVpuaubTP=smWaf7FBmpzB-_Frh0Dn5oN+vx0xzw@mail.gmail.com","threadId":"59117","inReplyTo":"xmqqr0vpxm3d.fsf@gitster.g","subject":"Re: [PATCH] rebase: mark --update-refs as requiring the merge backend","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-20T16:47:30Z","receivedAt":"2023-01-20T16:47:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 20, 2023 at 7:27 AM Junio C Hamano wrote:\n>\n> Derrick Stolee writes:\n>\n> >> + if (options.update_refs)\n> >> + imply_merge(&options, \"--update-refs\");\n> >> +\n> >\n> > This solution is very elegant. The only downside is the lack of warning\n> > if --update-refs was implied by rebase.updateRefs=true, but I'm happy to\n> > delay implementing that warning in favor of your complete solution here.\n>\n> If features A and B are incompatible and both can be specified from\n> both command line and configuration, ideally I would expect the\n> system to operate in one of two ways.\n\nI agree that one of the two ways you highlight below would be ideal.\nShould this series be held up on extending the checks to implement\nthis ideal, or are you just commenting for whoever later circles back\nto implement this?\n\n> I haven't thought it through\n> to decide which one I prefer between the two.\n>\n> * Take \"command line trumps configuration\" one step further, so\n> that A that is configured but not asked for from the command\n> line is defeated by B that is asked for from the command line.\n>\n> This way, only when A and B are both requested via the\n> configuration, of via the command line, we'd fail the operation\n> by saying A and B are incompatible. Otherwise, the one that is\n> configured but overridden is turned off (either silently or with\n> a warning).\n>\n> * Declare \"command line trumps configuration\" is only among the\n> same feature. Regardless of how features A and B that are\n> incompatible are requested, the command will error out, citing\n> incompatibility. It would be very nice if the warning mentioned\n> where the requests for features A and B came from (e.g. \"You\n> asked for -B from the command line, but you have A configured,\n> and both cannot be active at the same time---disable A from the\n> command line, or do not ask for B\")\n>\n> When A is configured and B is requested from the command line,\n> the command will error out, and the user must defeat A from the\n> command line before the user can use B, e.g. \"git cmd --no-A -B\".\n>\n> A knee-jerk reaction to the situation is that the latter feels\n> somewhat safer than the former, but when I imagine the actual end\n> user who saw the error message, especially the suggested solution\n> \"disable A from the command line or do not ask for B from the\n> command line\", may say \"well, I asked for B for this invocation\n> explicitly with -B from the command line, and you(Git) should be\n> able to make it imply --no-A\", which amounts to the same thing as\n> the former choice.\n\nIf it is clear to the user that A and B preclude each other, then I\nagree with this sentiment that the former choice (silently ignoring\nthe config) would avoid a minor frustration for some users and thus\nwould be better. But I don't think that's applicable here. There is\nno reason that --whitespace=fix shouldn't be available from the merge\nbackend other than that we haven't implemented it yet, and it's likely\nfeasible to implement --update-refs for the apply backend with enough\neffort if we thought it was worth it. So, if a user sets\nrebase.updateRefs=true in their config because they always want\nincluded branches updated, but one time they run `git rebase\n--whitespace=fix`, they will likely have a negative experience like\nthe one that inspired this patch. Perhaps we're forced to choose\nbetween possible frustration by different end users, but if so, I\nthink trying to debug and figure out \"Wait, I switched to this branch\nand started tweaking it but it appears to not have some relevant\nchanges I'm sure I made to it yesterday. What happened?\" is a much\nworse frustration than \"I have to manually specify --no-A in this\nspecial case\". So, when it's not at all obvious that A and B preclude\neach other, I think we're better off giving the error.\n"},{"id":"470824","messageId":"CABPp-BG8JmXApAsTVxpi2_8DpWi6ix-PhuOG6w=NwR0f=eA7xw@mail.gmail.com","threadId":"59117","inReplyTo":"bb75c8e1-05d3-1359-e06a-ee013ae677da@dunelm.org.uk","subject":"Re: [PATCH v2 2/2] rebase: mark --update-refs as requiring the merge backend","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-21T01:34:36Z","receivedAt":"2023-01-21T01:34:55Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 20, 2023 at 8:46 AM Phillip Wood wrote:\n>\n> Hi Elijah\n>\n> Thanks for working on this\n>\n> On 20/01/2023 04:56, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > --update-refs is built in terms of the sequencer, which requires the\n> > merge backend. It was already marked as incompatible with the apply\n> > backend in the git-rebase manual, but the code didn't check for this\n> > incompatibility and warn the user. Check and warn now.\n>\n> Strictly speaking we die rather than warn but I don't think that\n> warrants a re-roll.\n\nOh, good catch. I'm re-rolling anyway, so I might as well fix this.\n\n> I just had a quick look to see how easy it would be\n> to add the advice Stolee's patch had if the user has set\n> rebase.updaterefs but does not pass \"--no-update-refs\" when using the\n> apply backend but it looks a bit fiddly unfortunately as we could die in\n> imply_merge() or later on.\n\nYeah, and it gets even more finicky than that. If the user specifies\n_any_ merge-specific options on the command line together with an\napply-specific option, then there's no point bringing up\nrebase.updaterefs (or rebase.autosquash). We only want to bring up\nthose config options if they are the only reasons for getting a\nbackends-are-incompatible error message.\n\n> Thinking more generally, imply_merge() does a good job of telling the\n> user which option is incompatible with \"--apply\" but if the user passes\n> a merge option with \"--whitespace=fix\" and omits \"--apply\" then we just\n> print a generic message saying \"apply options and merge options cannot\n> be used together\" which isn't terribly helpful to the user (doubly so\n> when the merge option come from a config setting).\n\nThat's not specific to --whitespace=fix (it also happens with -C, and\nin the past happened with other options that used to only work with\nthe apply backend). In particular, it's whenever both backends are\nimplied -- in those cases, we don't try to keep track of which options\nimplied it and thus only provide a very generic error message.\n\n> I've also noticed that \"--autosquash\" is ignored if we end up using the\n> apply backend. That's a separate issue but shares the \"this may have\n> come from a config setting rather than a command line argument\" problem.\n\nYeah, Stolee also pointed this one out...and --autosquash was missing\nthe same incompatible-with-apply-options warnings too.\n\n> All in all I'm not sure if it is friendlier to die when the user has\n> rebsase.updaterefs set and they try to rebase with \"--whitespace=fix\" or\n> if it is better just to ignore the config in that case. If we can find a\n> way to print some help when we die in that case it would be nicer for\n> the user.\n\nI think ignoring it would be worse, as I argued over at [1]. But\nanother thing to keep in mind is that we can eventually make the\nquestion obsolete by deprecating and eventually removing the apply\nbackend, as suggested by Junio[2]. That would allow us to remove all\nthe incompatibility checking and simplify the manual.\n\n\n[1] https://lore.kernel.org/git/CABPp-BHDhpSVpuaubTP=smWaf7FBmpzB-_Frh0Dn5oN+vx0xzw@mail.gmail.com/\n[2] See \"longer term goal\" of\nhttps://lore.kernel.org/git/xmqqa78d2qmk.fsf@gitster-ct.c.googlers.com/\n"},{"id":"470825","messageId":"pull.1466.v3.git.1674266126.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v2.git.1674190573.gitgitgadget@gmail.com","subject":"[PATCH v3 0/7] rebase: fix several code/testing/documentation issues around flag incompatibilities","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-21T01:55:19Z","receivedAt":"2023-01-21T01:55:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We had a report about --update-refs being ignored when --whitespace=fix was\npassed, confusing an end user. These were already marked as incompatible in\nthe manual, but the code didn't check for the incompatibility and provide an\nerror to the user.\n\nFolks brought up other flags tangentially when reviewing an earlier round of\nthis series, and I found we had more that were missing checks, and that we\nwere missing some testcases, and that the documentation was wrong on some of\nthe relevant options. So, I ended up getting lots of little fixes to\nstraighten these all out.\n\nLeft out of this series:\n\n * If an option like rebase.autosquash or rebase.updateRefs are selected and\n the user specifies e.g. --whitespace=fix, we should either (a) tailor the\n error message better to point out that it's a config option that is\n incompatible with their command line flag, or (b) implement\n --whitespace=fix for the merge backend so we can just deprecate and\n eventually remove the apply backend[1].\n\n[1] See \"longer term goal\" at\nhttps://lore.kernel.org/git/xmqqa78d2qmk.fsf@gitster-ct.c.googlers.com/\n\nChanges since v2:\n\n * Remove the extra patch and reword the comment in t3422 more thoroughly.\n * Add tests for other flag incompatibilities that were missing\n * Add code that was missing for checking various flag incompatibilities\n * Fix incorrect claims in the documentation around incompatible options\n * A few other miscellaneous fixups noticed while doing all the above\n\nChanges since v1:\n\n * Add a patch nuking the -C option to rebase (fixes confusion around the\n comment in t3422 and acknowledges the fact that the option is totally and\n utterly useless and always has been. It literally never affects the\n results of a rebase.)\n\nSigned-off-by: Elijah Newren newren@gmail.com\n\nElijah Newren (7):\n rebase: mark --update-refs as requiring the merge backend\n rebase: flag --apply and --merge as incompatible\n rebase: remove --allow-empty-message from incompatible opts\n rebase: fix docs about incompatibilities with --root\n rebase: add coverage of other incompatible options\n rebase: clarify the OPT_CMDMODE incompatibilities\n rebase: fix formatting of rebase --reapply-cherry-picks option in docs\n\n Documentation/git-rebase.txt | 73 +++++++++++++-------------\n builtin/rebase.c | 36 +++++++++----\n t/t3422-rebase-incompatible-options.sh | 42 +++++++++++++--\n 3 files changed, 101 insertions(+), 50 deletions(-)\n\n\nbase-commit: 221222b278e713054e65cbbbcb2b1ac85483ea89\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/1466\n\nRange-diff vs v2:\n\n 2: 2e44d0b7e57 ! 1: 9089834adea rebase: mark --update-refs as requiring the merge backend\n @@ Commit message\n --update-refs is built in terms of the sequencer, which requires the\n merge backend. It was already marked as incompatible with the apply\n backend in the git-rebase manual, but the code didn't check for this\n - incompatibility and warn the user. Check and warn now.\n + incompatibility and warn the user. Check and error now.\n \n - While at it, fix a typo in t3422...and fix some misleading wording (all\n - useful options other than --whitespace=fix have long since been\n - implemented in the merge backend).\n + While at it, fix a typo in t3422...and fix some misleading wording\n + (most options which used to be am-specific have since been implemented\n + in the merge backend as well).\n \n Signed-off-by: Elijah Newren \n \n + ## Documentation/git-rebase.txt ##\n +@@ Documentation/git-rebase.txt: start would be overridden by the presence of\n + +\n + If the configuration variable `rebase.updateRefs` is set, then this option\n + can be used to override and disable this setting.\n +++\n ++See also INCOMPATIBLE OPTIONS below.\n + \n + INCOMPATIBLE OPTIONS\n + --------------------\n +\n ## builtin/rebase.c ##\n @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n @@ t/t3422-rebase-incompatible-options.sh: test_expect_success 'setup' '\n -# --merge nor --interactive (nor any options that imply those two) use\n -# git-am, using them together will result in flags like --whitespace=fix\n -# being ignored. Make sure rebase warns the user and aborts instead.\n -+# Rebase has a useful option, --whitespace=fix, which is actually\n -+# built in terms of flags to git-am. Since neither --merge nor\n -+# --interactive (nor any options that imply those two) use git-am,\n -+# using them together will result in --whitespace=fix being ignored.\n -+# Make sure rebase warns the user and aborts instead.\n ++# Rebase has a couple options which are specific to the apply backend,\n ++# and several options which are specific to the merge backend. Flags\n ++# from the different sets cannot work together, and we do not want to\n ++# just ignore one of the sets of flags. Make sure rebase warns the\n ++# user and aborts instead.\n #\n \n test_rebase_am_only () {\n 1: a0f8f5fac1c ! 2: a8b5a0e4fb0 rebase: remove completely useless -C option\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - rebase: remove completely useless -C option\n + rebase: flag --apply and --merge as incompatible\n \n - The `-C` option to rebase was introduced with 67dad687ad (\"add -C[NUM]\n - to git-am\", 2007-02-08).  Based on feedback on the patch, the author\n - added the -C option not just to git-am but also to git-rebase.  But did\n - it such that the option was just passed through to git-am (which passes\n - it along to git-apply), with no corresponding option to format-patch.\n -\n - As per the git-apply documentation for the `-C` option:\n - Ensure at least lines of surrounding context match...When fewer\n - lines of surrounding context exist they all must match.\n -\n - The fact that format-patch was not passed a -U option to increase the\n - number of context lines meant that there would still only be 3 lines of\n - context to match on. So, anyone attempting to use this option in\n - git-rebase would just be spinning their wheels and wasting time. I was\n - one such user a number of years ago.\n -\n - Since this option can at best waste users time and is nothing more than\n - a confusing no-op, and has never been anything other than a confusing\n - no-op, and no one has ever bothered to create a testcase for it that\n - goes beyond option parsing, simply excise the option.\n + Previously, we flagged options which implied --apply as being\n + incompatible with options which implied --merge. But if both options\n + were given explicitly, then we didn't flag the incompatibility. The\n + same is true with --apply and --interactive. Add the check, and add\n + some testcases to verify these are also caught.\n \n Signed-off-by: Elijah Newren \n \n - ## Documentation/git-rebase.txt ##\n -@@ Documentation/git-rebase.txt: include::rerere-options.txt[]\n - \tAllows the pre-rebase hook to run, which is the default. This option can\n - \tbe used to override `--no-verify`. See also linkgit:githooks[5].\n + ## builtin/rebase.c ##\n +@@ builtin/rebase.c: static int parse_opt_am(const struct option *opt, const char *arg, int unset)\n + \tBUG_ON_OPT_NEG(unset);\n + \tBUG_ON_OPT_ARG(arg);\n \n ---C::\n --\tEnsure at least `` lines of surrounding context match before\n --\tand after each change. When fewer lines of surrounding\n --\tcontext exist they all must match. By default no context is\n --\tever ignored. Implies `--apply`.\n --+\n --See also INCOMPATIBLE OPTIONS below.\n --\n - --no-ff::\n - --force-rebase::\n - -f::\n -@@ Documentation/git-rebase.txt: The following options:\n ++\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY)\n ++\t die(_(\"apply options and merge options cannot be used together\"));\n ++\n + \topts->type = REBASE_APPLY;\n \n - * --apply\n - * --whitespace\n -- * -C\n + \treturn 0;\n +@@ builtin/rebase.c: static int parse_opt_merge(const struct option *opt, const char *arg, int unset)\n + \tBUG_ON_OPT_NEG(unset);\n + \tBUG_ON_OPT_ARG(arg);\n \n - are incompatible with the following options:\n +-\tif (!is_merge(opts))\n +-\t\topts->type = REBASE_MERGE;\n ++\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)\n ++\t die(_(\"apply options and merge options cannot be used together\"));\n ++\n ++\topts->type = REBASE_MERGE;\n \n -\n - ## builtin/rebase.c ##\n -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n - \t\t\t N_(\"ignore author date and use current date\")),\n - \t\tOPT_HIDDEN_BOOL(0, \"ignore-date\", &options.ignore_date,\n - \t\t\t\tN_(\"synonym of --reset-author-date\")),\n --\t\tOPT_PASSTHRU_ARGV('C', NULL, &options.git_am_opts, N_(\"n\"),\n --\t\t\t\t N_(\"passed to 'git apply'\"), 0),\n - \t\tOPT_BOOL(0, \"ignore-whitespace\", &ignore_whitespace,\n - \t\t\t N_(\"ignore changes in whitespace\")),\n - \t\tOPT_PASSTHRU_ARGV(0, \"whitespace\", &options.git_am_opts,\n -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n - \t\tif (!strcmp(option, \"--whitespace=fix\") ||\n - \t\t !strcmp(option, \"--whitespace=strip\"))\n - \t\t\tallow_preemptive_ff = 0;\n --\t\telse if (skip_prefix(option, \"-C\", &p)) {\n --\t\t\twhile (*p)\n --\t\t\t\tif (!isdigit(*(p++)))\n --\t\t\t\t\tdie(_(\"switch `C' expects a \"\n --\t\t\t\t\t \"numerical value\"));\n --\t\t} else if (skip_prefix(option, \"--whitespace=\", &p)) {\n -+\t\telse if (skip_prefix(option, \"--whitespace=\", &p)) {\n - \t\t\tif (*p && strcmp(p, \"warn\") && strcmp(p, \"nowarn\") &&\n - \t\t\t strcmp(p, \"error\") && strcmp(p, \"error-all\"))\n - \t\t\t\tdie(\"Invalid whitespace option: '%s'\", p);\n -\n - ## t/t3406-rebase-message.sh ##\n -@@ t/t3406-rebase-message.sh: test_expect_success 'rebase --onto outputs the invalid ref' '\n - \ttest_i18ngrep \"invalid-ref\" err\n - '\n + \treturn 0;\n + }\n +@@ builtin/rebase.c: static int parse_opt_interactive(const struct option *opt, const char *arg,\n + \tBUG_ON_OPT_NEG(unset);\n + \tBUG_ON_OPT_ARG(arg);\n + \n ++\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)\n ++\t die(_(\"apply options and merge options cannot be used together\"));\n ++\n + \topts->type = REBASE_MERGE;\n + \topts->flags |= REBASE_INTERACTIVE_EXPLICIT;\n \n --test_expect_success 'error out early upon -C or --whitespace=' '\n --\ttest_must_fail git rebase -Cnot-a-number HEAD 2>err &&\n --\ttest_i18ngrep \"numerical value\" err &&\n --\ttest_must_fail git rebase --whitespace=bad HEAD 2>err &&\n --\ttest_i18ngrep \"Invalid whitespace option\" err\n --'\n --\n - write_reflog_expect () {\n - \tif test $mode = --apply\n - \tthen\n \n ## t/t3422-rebase-incompatible-options.sh ##\n @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {\n + \n }\n \n ++# Check options which imply --apply\n test_rebase_am_only --whitespace=fix\n --test_rebase_am_only -C4\n + test_rebase_am_only -C4\n ++# Also check an explicit --apply\n ++test_rebase_am_only --apply\n \n test_done\n -: ----------- > 3: f4fbfd40d45 rebase: remove --allow-empty-message from incompatible opts\n -: ----------- > 4: a1e61ac8f21 rebase: fix docs about incompatibilities with --root\n -: ----------- > 5: 48c40c0dda0 rebase: add coverage of other incompatible options\n -: ----------- > 6: 8664cce6cf7 rebase: clarify the OPT_CMDMODE incompatibilities\n -: ----------- > 7: 0e8b06163f2 rebase: fix formatting of rebase --reapply-cherry-picks option in docs\n\n-- \ngitgitgadget\n"},{"id":"470826","messageId":"9089834adeacb2233cb924f36a04b027dac76594.1674266126.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v3.git.1674266126.gitgitgadget@gmail.com","subject":"[PATCH v3 1/7] rebase: mark --update-refs as requiring the merge backend","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-21T01:55:20Z","receivedAt":"2023-01-21T01:55:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--update-refs is built in terms of the sequencer, which requires the\nmerge backend. It was already marked as incompatible with the apply\nbackend in the git-rebase manual, but the code didn't check for this\nincompatibility and warn the user. Check and error now.\n\nWhile at it, fix a typo in t3422...and fix some misleading wording\n(most options which used to be am-specific have since been implemented\nin the merge backend as well).\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 2 ++\n builtin/rebase.c | 3 +++\n t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----\n 3 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex f9675bd24e6..00d21d7287d 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -623,6 +623,8 @@ start would be overridden by the presence of\n +\n If the configuration variable `rebase.updateRefs` is set, then this option\n can be used to override and disable this setting.\n++\n+See also INCOMPATIBLE OPTIONS below.\n \n INCOMPATIBLE OPTIONS\n --------------------\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 1481c5b6a5b..accd62fce48 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1514,6 +1514,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (options.update_refs)\n+\t\timply_merge(&options, \"--update-refs\");\n+\n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\n \t\t\timply_merge(&options, \"--merge\");\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 6dabb05a2ad..9da39cd91c2 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -25,11 +25,11 @@ test_expect_success 'setup' '\n '\n \n #\n-# Rebase has lots of useful options like --whitepsace=fix, which are\n-# actually all built in terms of flags to git-am. Since neither\n-# --merge nor --interactive (nor any options that imply those two) use\n-# git-am, using them together will result in flags like --whitespace=fix\n-# being ignored. Make sure rebase warns the user and aborts instead.\n+# Rebase has a couple options which are specific to the apply backend,\n+# and several options which are specific to the merge backend. Flags\n+# from the different sets cannot work together, and we do not want to\n+# just ignore one of the sets of flags. Make sure rebase warns the\n+# user and aborts instead.\n #\n \n test_rebase_am_only () {\n@@ -60,6 +60,11 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --exec 'true' A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --update-refs\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --update-refs A\n+\t\"\n+\n }\n \n test_rebase_am_only --whitespace=fix\n-- \ngitgitgadget\n\n"},{"id":"470827","messageId":"a8b5a0e4fb01bb3f4332b94f7e82e10d946b7828.1674266126.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v3.git.1674266126.gitgitgadget@gmail.com","subject":"[PATCH v3 2/7] rebase: flag --apply and --merge as incompatible","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-21T01:55:21Z","receivedAt":"2023-01-21T01:55:39Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nPreviously, we flagged options which implied --apply as being\nincompatible with options which implied --merge. But if both options\nwere given explicitly, then we didn't flag the incompatibility. The\nsame is true with --apply and --interactive. Add the check, and add\nsome testcases to verify these are also caught.\n\nSigned-off-by: Elijah Newren \n---\n builtin/rebase.c | 12 ++++++++++--\n t/t3422-rebase-incompatible-options.sh | 3 +++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex accd62fce48..2a5e0e8a7a0 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -920,6 +920,9 @@ static int parse_opt_am(const struct option *opt, const char *arg, int unset)\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n \n+\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY)\n+\t die(_(\"apply options and merge options cannot be used together\"));\n+\n \topts->type = REBASE_APPLY;\n \n \treturn 0;\n@@ -933,8 +936,10 @@ static int parse_opt_merge(const struct option *opt, const char *arg, int unset)\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n \n-\tif (!is_merge(opts))\n-\t\topts->type = REBASE_MERGE;\n+\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)\n+\t die(_(\"apply options and merge options cannot be used together\"));\n+\n+\topts->type = REBASE_MERGE;\n \n \treturn 0;\n }\n@@ -948,6 +953,9 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n \n+\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)\n+\t die(_(\"apply options and merge options cannot be used together\"));\n+\n \topts->type = REBASE_MERGE;\n \topts->flags |= REBASE_INTERACTIVE_EXPLICIT;\n \ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 9da39cd91c2..9b9e78479f6 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -67,7 +67,10 @@ test_rebase_am_only () {\n \n }\n \n+# Check options which imply --apply\n test_rebase_am_only --whitespace=fix\n test_rebase_am_only -C4\n+# Also check an explicit --apply\n+test_rebase_am_only --apply\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"470828","messageId":"f4fbfd40d4599542b041081880e89075f4ff792b.1674266126.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v3.git.1674266126.gitgitgadget@gmail.com","subject":"[PATCH v3 3/7] rebase: remove --allow-empty-message from incompatible opts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-21T01:55:22Z","receivedAt":"2023-01-21T01:55:44Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--allow-empty-message was turned into a no-op and even documented\nas such; the flag is simply ignored. Since the flag is ignored, it\nshouldn't be documented as being incompatible with other flags.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 00d21d7287d..3929535c0cd 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -640,7 +640,6 @@ are incompatible with the following options:\n * --merge\n * --strategy\n * --strategy-option\n- * --allow-empty-message\n * --[no-]autosquash\n * --rebase-merges\n * --interactive\n-- \ngitgitgadget\n\n"},{"id":"470829","messageId":"a1e61ac8f2184b5d3f71ae279633342c997d9319.1674266126.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v3.git.1674266126.gitgitgadget@gmail.com","subject":"[PATCH v3 4/7] rebase: fix docs about incompatibilities with --root","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-21T01:55:23Z","receivedAt":"2023-01-21T01:55:46Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit 5dacd4abdd (\"git-rebase.txt: document incompatible options\",\n2018-06-25), I added notes about incompatibilities between options for\nthe apply and merge backends. Unfortunately, I inverted the condition\nwhen --root was incompatible with the apply backend. Fix the\ndocumentation, and add a testcase that verifies the documentation\nmatches the code.\n\nWhile at it, the documentation for --root also tried to cover some of\nthe backend differences between the apply and merge backends in relation\nto reapplying cherry picks. The information:\n * assumed that the apply backend was the default (it isn't anymore)\n * was written before --reapply-cherry-picks became an option\n * was written before the detailed information on backend differences\nAll of these factors make the sentence under --root about reapplying\ncherry picks contradict information that is now available elsewhere in\nthe manual, and the other references are correct. So just strike this\nsentence.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 7 ++-----\n t/t3422-rebase-incompatible-options.sh | 4 ++++\n 2 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 3929535c0cd..3f12d242d83 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -567,10 +567,7 @@ See also INCOMPATIBLE OPTIONS below.\n --root::\n \tRebase all commits reachable from ``, instead of\n \tlimiting them with an ``. This allows you to rebase\n-\tthe root commit(s) on a branch. When used with `--onto`, it\n-\twill skip changes already contained in `` (instead of\n-\t``) whereas without `--onto` it will operate on every\n-\tchange.\n+\tthe root commit(s) on a branch.\n +\n See also INCOMPATIBLE OPTIONS below.\n \n@@ -649,7 +646,7 @@ are incompatible with the following options:\n * --reapply-cherry-picks\n * --edit-todo\n * --update-refs\n- * --root when used in combination with --onto\n+ * --root when used without --onto\n \n In addition, the following pairs of options are incompatible:\n \ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 9b9e78479f6..f86274990b0 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -65,6 +65,10 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --update-refs A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --root without --onto\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --root A\n+\t\"\n }\n \n # Check options which imply --apply\n-- \ngitgitgadget\n\n"},{"id":"470830","messageId":"48c40c0dda00eeb8b9bdc5ba9372b46964eea14a.1674266126.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v3.git.1674266126.gitgitgadget@gmail.com","subject":"[PATCH v3 5/7] rebase: add coverage of other incompatible options","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-21T01:55:24Z","receivedAt":"2023-01-21T01:55:47Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe git-rebase manual noted several sets of incompatible options, but\nwe were missing tests for a few of these. Further, we were missing\ncode checks for some of these, which could result in command line\noptions being silently ignored.\n\nSigned-off-by: Elijah Newren \n---\n builtin/rebase.c | 21 ++++++++++++++-------\n t/t3422-rebase-incompatible-options.sh | 20 ++++++++++++++++++++\n 2 files changed, 34 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 2a5e0e8a7a0..6dcdb59bb02 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1238,6 +1238,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tif (options.fork_point < 0)\n \t\t\toptions.fork_point = 0;\n \t}\n+\t/*\n+\t * reapply_cherry_picks is slightly weird. It starts out with a\n+\t * value of -1. It will be assigned a value of keep_base below and\n+\t * then handled specially. The apply backend is hardcoded to\n+\t * behave like reapply_cherry_picks==1, so if it has that value, we\n+\t * can just ignore the flag with the apply backend. Thus, we only\n+\t * really need to throw an error and require the merge backend if\n+\t * reapply_cherry_picks==0.\n+\t */\n+\tif (options.reapply_cherry_picks == 0)\n+\t\timply_merge(&options, \"--no-reapply-cherry-picks\");\n \t/*\n \t * --keep-base defaults to --reapply-cherry-picks to avoid losing\n \t * commits when using this option.\n@@ -1420,13 +1431,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.empty != EMPTY_UNSPECIFIED)\n \t\timply_merge(&options, \"--empty\");\n \n-\t/*\n-\t * --keep-base implements --reapply-cherry-picks by altering upstream so\n-\t * it works with both backends.\n-\t */\n-\tif (options.reapply_cherry_picks && !keep_base)\n-\t\timply_merge(&options, \"--reapply-cherry-picks\");\n-\n \tif (gpg_sign)\n \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n \n@@ -1525,6 +1529,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.update_refs)\n \t\timply_merge(&options, \"--update-refs\");\n \n+\tif (options.autosquash)\n+\t\timply_merge(&options, \"--autosquash\");\n+\n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\n \t\t\timply_merge(&options, \"--merge\");\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex f86274990b0..c830025470f 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -50,6 +50,11 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --strategy-option=ours A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --autosquash\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --autosquash A\n+\t\"\n+\n \ttest_expect_success \"$opt incompatible with --interactive\" \"\n \t\tgit checkout B^0 &&\n \t\ttest_must_fail git rebase $opt --interactive A\n@@ -60,6 +65,21 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --exec 'true' A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --keep-empty\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --keep-empty A\n+\t\"\n+\n+\ttest_expect_success \"$opt incompatible with --empty=...\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --empty=ask A\n+\t\"\n+\n+\ttest_expect_success \"$opt incompatible with --no-reapply-cherry-picks\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n+\t\"\n+\n \ttest_expect_success \"$opt incompatible with --update-refs\" \"\n \t\tgit checkout B^0 &&\n \t\ttest_must_fail git rebase $opt --update-refs A\n-- \ngitgitgadget\n\n"},{"id":"470831","messageId":"8664cce6cf7f42850176d852b5f866bf70e6ce07.1674266126.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v3.git.1674266126.gitgitgadget@gmail.com","subject":"[PATCH v3 6/7] rebase: clarify the OPT_CMDMODE incompatibilities","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-21T01:55:25Z","receivedAt":"2023-01-21T01:55:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--edit-todo was documented as being incompatible with any of the options\nfor the apply backend. However, it is also incompatible with any of the\noptions for the merge backend, and is incompatible with any options that\nare not backend specific as well. The same can be said for --continue,\n--skip, --abort, --quit, etc.\n\nThis is already somewhat implicitly covered by the synopsis, but since\n\"[]\" in the first two variants are vague it might be easy to\nmiss this. That might not be a big deal, but since the rebase manpage\nhas to spend so much verbiage about incompatibility of options, making\na separate section for these options that are incompatible with\neverything else seems clearer. Do that, and remove the needless\ninclusion of --edit-todo in the explicit incompatibility list.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 61 +++++++++++++++++++-----------------\n 1 file changed, 33 insertions(+), 28 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 3f12d242d83..00a9e22bc32 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -201,6 +201,39 @@ Alternatively, you can undo the 'git rebase' with\n \n git rebase --abort\n \n+MODE OPTIONS\n+------------\n+\n+The options in this section cannot be used with any other option,\n+including not with each other:\n+\n+--continue::\n+\tRestart the rebasing process after having resolved a merge conflict.\n+\n+--skip::\n+\tRestart the rebasing process by skipping the current patch.\n+\n+--abort::\n+\tAbort the rebase operation and reset HEAD to the original\n+\tbranch. If `` was provided when the rebase operation was\n+\tstarted, then `HEAD` will be reset to ``. Otherwise `HEAD`\n+\twill be reset to where it was when the rebase operation was\n+\tstarted.\n+\n+--quit::\n+\tAbort the rebase operation but `HEAD` is not reset back to the\n+\toriginal branch. The index and working tree are also left\n+\tunchanged as a result. If a temporary stash entry was created\n+\tusing `--autostash`, it will be saved to the stash list.\n+\n+--edit-todo::\n+\tEdit the todo list during an interactive rebase.\n+\n+--show-current-patch::\n+\tShow the current patch in an interactive rebase or when rebase\n+\tis stopped because of conflicts. This is the equivalent of\n+\t`git show REBASE_HEAD`.\n+\n OPTIONS\n -------\n --onto ::\n@@ -242,22 +275,6 @@ See also INCOMPATIBLE OPTIONS below.\n ::\n \tWorking branch; defaults to `HEAD`.\n \n---continue::\n-\tRestart the rebasing process after having resolved a merge conflict.\n-\n---abort::\n-\tAbort the rebase operation and reset HEAD to the original\n-\tbranch. If `` was provided when the rebase operation was\n-\tstarted, then `HEAD` will be reset to ``. Otherwise `HEAD`\n-\twill be reset to where it was when the rebase operation was\n-\tstarted.\n-\n---quit::\n-\tAbort the rebase operation but `HEAD` is not reset back to the\n-\toriginal branch. The index and working tree are also left\n-\tunchanged as a result. If a temporary stash entry was created\n-\tusing `--autostash`, it will be saved to the stash list.\n-\n --apply::\n \tUse applying strategies to rebase (calling `git-am`\n \tinternally). This option may become a no-op in the future\n@@ -338,17 +355,6 @@ See also INCOMPATIBLE OPTIONS below.\n +\n See also INCOMPATIBLE OPTIONS below.\n \n---skip::\n-\tRestart the rebasing process by skipping the current patch.\n-\n---edit-todo::\n-\tEdit the todo list during an interactive rebase.\n-\n---show-current-patch::\n-\tShow the current patch in an interactive rebase or when rebase\n-\tis stopped because of conflicts. This is the equivalent of\n-\t`git show REBASE_HEAD`.\n-\n -m::\n --merge::\n \tUsing merging strategies to rebase (default).\n@@ -644,7 +650,6 @@ are incompatible with the following options:\n * --no-keep-empty\n * --empty=\n * --reapply-cherry-picks\n- * --edit-todo\n * --update-refs\n * --root when used without --onto\n \n-- \ngitgitgadget\n\n"},{"id":"470832","messageId":"0e8b06163f2b21748a19f32d515aecb16cd4574b.1674266126.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v3.git.1674266126.gitgitgadget@gmail.com","subject":"[PATCH v3 7/7] rebase: fix formatting of rebase --reapply-cherry-picks option in docs","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-21T01:55:26Z","receivedAt":"2023-01-21T01:55:49Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCommit ce5238a690 (\"rebase --keep-base: imply --reapply-cherry-picks\",\n2022-10-17) accidentally added some blank lines that cause extra\nparagraphs about --reapply-cherry-picks to be considered not part of\nthe documentation of that option. Remove the blank lines to make it\nclear we are still discussing --reapply-cherry-picks.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 00a9e22bc32..140c984d0ea 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -331,7 +331,6 @@ See also INCOMPATIBLE OPTIONS below.\n \tupstream changes, the behavior towards them is controlled by\n \tthe `--empty` flag.)\n +\n-\n In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is\n given), these commits will be automatically dropped. Because this\n necessitates reading all upstream commits, this can be expensive in\n@@ -340,7 +339,6 @@ read. When using the 'merge' backend, warnings will be issued for each\n dropped commit (unless `--quiet` is given). Advice will also be issued\n unless `advice.skippedCherryPicks` is set to false (see\n linkgit:git-config[1]).\n-\n +\n `--reapply-cherry-picks` allows rebase to forgo reading all upstream\n commits, potentially improving performance.\n-- \ngitgitgadget\n"},{"id":"470834","messageId":"CABPp-BFLUf7htEMXOLGOTNhUP=K11TfKrXQuC-FO4W6mHsjWQg@mail.gmail.com","threadId":"59117","inReplyTo":"xmqqh6wlxjuo.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] rebase: remove completely useless -C option","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-21T04:52:05Z","receivedAt":"2023-01-21T04:52:24Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jan 20, 2023 at 8:16 AM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > On Fri, Jan 20, 2023 at 4:05 AM Junio C Hamano wrote:\n> >> ...\n> >> That is, even when a patch does not cleanly apply with full context\n> >> in the incoming diff, by requiring *smaller* number of lines to\n> >> match, the diff *could* be forced to apply with reduced precision?\n> >\n> > Oh! Reducing the number of lines of context to pay attention to never\n> > even occurred to me for whatever reason. I'll drop the patch.\n>\n> Yup. \"completely useless\" on the title is less than half correct,\n> but still correct for a minor use case where -C is used to use\n> *more* context lines than the default.\n>\n> We could update \"rebase --apply\" codepath to increase the context\n> lines generated by format-patch. That would make the \"completely\n> useless\" totally incorrect ;-)\n\nhaha :-)\n\nI'd probably go for that, but since in my mind the plan is still to\ndeprecate and remove the apply backend as you suggested at [1], I'm\nnot particularly motivated to improve/extend options specific to the\napply backend of rebase.\n\n[1] https://lore.kernel.org/git/xmqqa78d2qmk.fsf@gitster-ct.c.googlers.com/\n"},{"id":"470840","messageId":"8d6c741c-3271-db61-4e69-3b93049f9f8f@dunelm.org.uk","threadId":"59117","inReplyTo":"f4fbfd40d4599542b041081880e89075f4ff792b.1674266126.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 3/7] rebase: remove --allow-empty-message from incompatible opts","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-21T15:09:33Z","receivedAt":"2023-01-21T15:09:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 21/01/2023 01:55, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> --allow-empty-message was turned into a no-op and even documented\n> as such; the flag is simply ignored. Since the flag is ignored, it\n> shouldn't be documented as being incompatible with other flags.\n\nThe patch looks fine. Just to note that\n\n#leftoverbits - I notice there is some code in builtin/rebase.c, \nbuiltin/revert.c and sequencer.[ch] related to this option that could be \nremoved. The setting seems to be completely ignored by the sequencer and \nso could be removed from struct replay_opts.\n\nBest Wishes\n\nPhillip\n\n> Signed-off-by: Elijah Newren \n> ---\n> Documentation/git-rebase.txt | 1 -\n> 1 file changed, 1 deletion(-)\n> \n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 00d21d7287d..3929535c0cd 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -640,7 +640,6 @@ are incompatible with the following options:\n> * --merge\n> * --strategy\n> * --strategy-option\n> - * --allow-empty-message\n> * --[no-]autosquash\n> * --rebase-merges\n> * --interactive\n"},{"id":"470841","messageId":"d5ef5870-baac-14d4-65a5-deb94a848011@dunelm.org.uk","threadId":"59117","inReplyTo":"48c40c0dda00eeb8b9bdc5ba9372b46964eea14a.1674266126.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 5/7] rebase: add coverage of other incompatible options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-21T15:20:32Z","receivedAt":"2023-01-21T15:20:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 21/01/2023 01:55, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> The git-rebase manual noted several sets of incompatible options, but\n> we were missing tests for a few of these. Further, we were missing\n> code checks for some of these, which could result in command line\n> options being silently ignored.\n> \n> Signed-off-by: Elijah Newren \n> ---\n> builtin/rebase.c | 21 ++++++++++++++-------\n> t/t3422-rebase-incompatible-options.sh | 20 ++++++++++++++++++++\n> 2 files changed, 34 insertions(+), 7 deletions(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 2a5e0e8a7a0..6dcdb59bb02 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1238,6 +1238,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tif (options.fork_point < 0)\n> \t\t\toptions.fork_point = 0;\n> \t}\n> +\t/*\n> +\t * reapply_cherry_picks is slightly weird. It starts out with a\n> +\t * value of -1. It will be assigned a value of keep_base below and\n> +\t * then handled specially. The apply backend is hardcoded to\n> +\t * behave like reapply_cherry_picks==1,\n\nI think it is hard coded to 0 not 1. We generate the patches with\n\n\tgit format-patch --right-only $upstream...$head\n\nso cherry-picks will not be reapplied. I'm hardly an impartial observer \nbut I think the current check is correct. We support\n\n\t--whitespace=fix --no-reapply-cherry-picks\n\t--whitespace=fix --keep-base --reapply-cherry-picks\n\nbut not\n\n\t--whitespace=fix --reapply-cherry-picks\n\n> @@ -1525,6 +1529,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tif (options.update_refs)\n> \t\timply_merge(&options, \"--update-refs\");\n> \n> +\tif (options.autosquash)\n> +\t\timply_merge(&options, \"--autosquash\");\n\nThanks for adding this, it maybe merits a mention in the commit message \nthough as it is a change in behavior for users who have \nrebase.autosquash set and try to use the apply backend.\n\nBest Wishes\n\nPhillip\n\n> \tif (options.type == REBASE_UNSPECIFIED) {\n> \t\tif (!strcmp(options.default_backend, \"merge\"))\n> \t\t\timply_merge(&options, \"--merge\");\n> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\n> index f86274990b0..c830025470f 100755\n> --- a/t/t3422-rebase-incompatible-options.sh\n> +++ b/t/t3422-rebase-incompatible-options.sh\n> @@ -50,6 +50,11 @@ test_rebase_am_only () {\n> \t\ttest_must_fail git rebase $opt --strategy-option=ours A\n> \t\"\n> \n> +\ttest_expect_success \"$opt incompatible with --autosquash\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --autosquash A\n> +\t\"\n> +\n> \ttest_expect_success \"$opt incompatible with --interactive\" \"\n> \t\tgit checkout B^0 &&\n> \t\ttest_must_fail git rebase $opt --interactive A\n> @@ -60,6 +65,21 @@ test_rebase_am_only () {\n> \t\ttest_must_fail git rebase $opt --exec 'true' A\n> \t\"\n> \n> +\ttest_expect_success \"$opt incompatible with --keep-empty\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --keep-empty A\n> +\t\"\n> +\n> +\ttest_expect_success \"$opt incompatible with --empty=...\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --empty=ask A\n> +\t\"\n> +\n> +\ttest_expect_success \"$opt incompatible with --no-reapply-cherry-picks\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n> +\t\"\n> +\n> \ttest_expect_success \"$opt incompatible with --update-refs\" \"\n> \t\tgit checkout B^0 &&\n> \t\ttest_must_fail git rebase $opt --update-refs A\n"},{"id":"470842","messageId":"878097e3-7a74-6b7d-40d6-90f31558f8ad@dunelm.org.uk","threadId":"59117","inReplyTo":"0e8b06163f2b21748a19f32d515aecb16cd4574b.1674266126.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 7/7] rebase: fix formatting of rebase --reapply-cherry-picks option in docs","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-21T15:21:52Z","receivedAt":"2023-01-21T15:21:59Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 21/01/2023 01:55, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> Commit ce5238a690 (\"rebase --keep-base: imply --reapply-cherry-picks\",\n> 2022-10-17) accidentally added some blank lines that cause extra\n> paragraphs about --reapply-cherry-picks to be considered not part of\n> the documentation of that option. Remove the blank lines to make it\n> clear we are still discussing --reapply-cherry-picks.\n\nThanks for clearing up my mess!\n\nPhillip\n\n> Signed-off-by: Elijah Newren \n> ---\n> Documentation/git-rebase.txt | 2 --\n> 1 file changed, 2 deletions(-)\n> \n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 00a9e22bc32..140c984d0ea 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -331,7 +331,6 @@ See also INCOMPATIBLE OPTIONS below.\n> \tupstream changes, the behavior towards them is controlled by\n> \tthe `--empty` flag.)\n> +\n> -\n> In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is\n> given), these commits will be automatically dropped. Because this\n> necessitates reading all upstream commits, this can be expensive in\n> @@ -340,7 +339,6 @@ read. When using the 'merge' backend, warnings will be issued for each\n> dropped commit (unless `--quiet` is given). Advice will also be issued\n> unless `advice.skippedCherryPicks` is set to false (see\n> linkgit:git-config[1]).\n> -\n> +\n> `--reapply-cherry-picks` allows rebase to forgo reading all upstream\n> commits, potentially improving performance.\n"},{"id":"470844","messageId":"20e759bd-7d13-b7ba-c0df-69b4f827c5bd@dunelm.org.uk","threadId":"59117","inReplyTo":"d5ef5870-baac-14d4-65a5-deb94a848011@dunelm.org.uk","subject":"Re: [PATCH v3 5/7] rebase: add coverage of other incompatible options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-21T19:25:50Z","receivedAt":"2023-01-21T19:26:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 21/01/2023 15:20, Phillip Wood wrote:\n>> @@ -1238,6 +1238,17 @@ int cmd_rebase(int argc, const char **argv, \n>> const char *prefix)\n>>           if (options.fork_point < 0)\n>>               options.fork_point = 0;\n>>       }\n>> +    /*\n>> +     * reapply_cherry_picks is slightly weird.  It starts out with a\n>> +     * value of -1.  It will be assigned a value of keep_base below and\n>> +     * then handled specially.  The apply backend is hardcoded to\n>> +     * behave like reapply_cherry_picks==1,\n> \n> I think it is hard coded to 0 not 1. We generate the patches with\n> \n>     git format-patch --right-only $upstream...$head\n\nSorry I somhow managed to omit --cherry-pick from that, it should have been\n\n\tgit format-patch --right-only --cherry-pick $upstream...$head\n\nPhillip\n\n\n> so cherry-picks will not be reapplied. I'm hardly an impartial observer \n> but I think the current check is correct. We support\n> \n>     --whitespace=fix --no-reapply-cherry-picks\n>     --whitespace=fix --keep-base --reapply-cherry-picks\n> \n> but not\n> \n>     --whitespace=fix --reapply-cherry-picks\n> \n>> @@ -1525,6 +1529,9 @@ int cmd_rebase(int argc, const char **argv, \n>> const char *prefix)\n>>       if (options.update_refs)\n>>           imply_merge(&options, \"--update-refs\");\n>> +    if (options.autosquash)\n>> +        imply_merge(&options, \"--autosquash\");\n> \n> Thanks for adding this, it maybe merits a mention in the commit message \n> though as it is a change in behavior for users who have \n> rebase.autosquash set and try to use the apply backend.\n> \n> Best Wishes\n> \n> Phillip\n> \n>>       if (options.type == REBASE_UNSPECIFIED) {\n>>           if (!strcmp(options.default_backend, \"merge\"))\n>>               imply_merge(&options, \"--merge\");\n>> diff --git a/t/t3422-rebase-incompatible-options.sh \n>> b/t/t3422-rebase-incompatible-options.sh\n>> index f86274990b0..c830025470f 100755\n>> --- a/t/t3422-rebase-incompatible-options.sh\n>> +++ b/t/t3422-rebase-incompatible-options.sh\n>> @@ -50,6 +50,11 @@ test_rebase_am_only () {\n>>           test_must_fail git rebase $opt --strategy-option=ours A\n>>       \"\n>> +    test_expect_success \"$opt incompatible with --autosquash\" \"\n>> +        git checkout B^0 &&\n>> +        test_must_fail git rebase $opt --autosquash A\n>> +    \"\n>> +\n>>       test_expect_success \"$opt incompatible with --interactive\" \"\n>>           git checkout B^0 &&\n>>           test_must_fail git rebase $opt --interactive A\n>> @@ -60,6 +65,21 @@ test_rebase_am_only () {\n>>           test_must_fail git rebase $opt --exec 'true' A\n>>       \"\n>> +    test_expect_success \"$opt incompatible with --keep-empty\" \"\n>> +        git checkout B^0 &&\n>> +        test_must_fail git rebase $opt --keep-empty A\n>> +    \"\n>> +\n>> +    test_expect_success \"$opt incompatible with --empty=...\" \"\n>> +        git checkout B^0 &&\n>> +        test_must_fail git rebase $opt --empty=ask A\n>> +    \"\n>> +\n>> +    test_expect_success \"$opt incompatible with \n>> --no-reapply-cherry-picks\" \"\n>> +        git checkout B^0 &&\n>> +        test_must_fail git rebase $opt --no-reapply-cherry-picks A\n>> +    \"\n>> +\n>>       test_expect_success \"$opt incompatible with --update-refs\" \"\n>>           git checkout B^0 &&\n>>           test_must_fail git rebase $opt --update-refs A\n"},{"id":"470849","messageId":"xmqqilgzwi61.fsf@gitster.g","threadId":"59117","inReplyTo":"CABPp-BFLUf7htEMXOLGOTNhUP=K11TfKrXQuC-FO4W6mHsjWQg@mail.gmail.com","subject":"Re: [PATCH v2 1/2] rebase: remove completely useless -C option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-22T00:02:14Z","receivedAt":"2023-01-22T00:02:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> I'd probably go for that, but since in my mind the plan is still to\n> deprecate and remove the apply backend as you suggested at [1], I'm\n> not particularly motivated to improve/extend options specific to the\n> apply backend of rebase.\n\nI still consider that deprecating and removing the \"am|format-patch\npipeline\" mode is a good longer term goal, but it seems we still\nhave some features that are only available with the mode and not the\nother \"sequence of cherry-pick\" mode? We'd need to port that over\nto the other backend before we can wean ourselves off of the apply\nbackend. Until then, ...\n\n\n"},{"id":"470859","messageId":"CABPp-BFCHvc2rDjYLYmx9ji2QcnGFJHptJR97exHNqifcz4vDg@mail.gmail.com","threadId":"59117","inReplyTo":"20e759bd-7d13-b7ba-c0df-69b4f827c5bd@dunelm.org.uk","subject":"Re: [PATCH v3 5/7] rebase: add coverage of other incompatible options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-22T05:11:39Z","receivedAt":"2023-01-22T05:11:59Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Sat, Jan 21, 2023 at 11:25 AM Phillip Wood wrote:\n>\n> On 21/01/2023 15:20, Phillip Wood wrote:\n> >> @@ -1238,6 +1238,17 @@ int cmd_rebase(int argc, const char **argv,\n> >> const char *prefix)\n> >> if (options.fork_point < 0)\n> >> options.fork_point = 0;\n> >> }\n> >> + /*\n> >> + * reapply_cherry_picks is slightly weird. It starts out with a\n> >> + * value of -1. It will be assigned a value of keep_base below and\n> >> + * then handled specially. The apply backend is hardcoded to\n> >> + * behave like reapply_cherry_picks==1,\n> >\n> > I think it is hard coded to 0 not 1. We generate the patches with\n> >\n> > git format-patch --right-only $upstream...$head\n>\n> Sorry I somhow managed to omit --cherry-pick from that, it should have been\n>\n> git format-patch --right-only --cherry-pick $upstream...$head\n>\n> Phillip\n\nOh, indeed, I was reading wrong. Thanks for the correction. I still\nthink there's something to fix up here, which I'll include in my\nre-roll.\n\n> > so cherry-picks will not be reapplied. I'm hardly an impartial observer\n> > but I think the current check is correct. We support\n> >\n> > --whitespace=fix --no-reapply-cherry-picks\n> > --whitespace=fix --keep-base --reapply-cherry-picks\n> >\n> > but not\n> >\n> > --whitespace=fix --reapply-cherry-picks\n\nRight, nor do we support\n --whitespace=fix --keep-base --no-reapply-cherry-picks\n(If you try it, you'll notice that the code accepts those flags and\nstarts the rebase pretending everything is fine, but it silently\nignores the --no-reapply-cherry-picks flag.)\n\nStepping back a bit, though, the apply backend doesn't support\ntoggling --[no-]reapply-cherry-picks at all. It hard codes the\nbehavior (in a way dependent upon --keep-base). So, if the user\npasses --[no-]reapply-cherry-picks and we don't error out, we are just\ngoing to ignore whatever they passed and do whatever hardcoded thing\nis baked into the algorithm.\n\nWhile the user can pass the flag in a way that happens to match what\nis baked into the apply backend, I'd still say it's wrong for them to\nspecify it since we will just ignore it. Doing so is likely to result\nin the user later toggling the flag and being surprised that they get\nan error when it used to be accepted, or seeing that it only sometimes\ngets accepted.\n\nAnyway, I'll update this patch to document this a bit more clearly in\na code comment and add an improved check.\n"},{"id":"470860","messageId":"cc129b8718594a98557d21b5c79369af8935d404.1674367961.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","subject":"[PATCH v4 2/9] rebase: flag --apply and --merge as incompatible","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-22T06:12:33Z","receivedAt":"2023-01-22T06:13:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nPreviously, we flagged options which implied --apply as being\nincompatible with options which implied --merge. But if both options\nwere given explicitly, then we didn't flag the incompatibility. The\nsame is true with --apply and --interactive. Add the check, and add\nsome testcases to verify these are also caught.\n\nSigned-off-by: Elijah Newren \n---\n builtin/rebase.c | 12 ++++++++++--\n t/t3422-rebase-incompatible-options.sh | 3 +++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex c111b89e137..b742cc8fb5c 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -907,6 +907,9 @@ static int parse_opt_am(const struct option *opt, const char *arg, int unset)\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n \n+\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY)\n+\t die(_(\"apply options and merge options cannot be used together\"));\n+\n \topts->type = REBASE_APPLY;\n \n \treturn 0;\n@@ -920,8 +923,10 @@ static int parse_opt_merge(const struct option *opt, const char *arg, int unset)\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n \n-\tif (!is_merge(opts))\n-\t\topts->type = REBASE_MERGE;\n+\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)\n+\t die(_(\"apply options and merge options cannot be used together\"));\n+\n+\topts->type = REBASE_MERGE;\n \n \treturn 0;\n }\n@@ -935,6 +940,9 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n \n+\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)\n+\t die(_(\"apply options and merge options cannot be used together\"));\n+\n \topts->type = REBASE_MERGE;\n \topts->flags |= REBASE_INTERACTIVE_EXPLICIT;\n \ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 9da39cd91c2..9b9e78479f6 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -67,7 +67,10 @@ test_rebase_am_only () {\n \n }\n \n+# Check options which imply --apply\n test_rebase_am_only --whitespace=fix\n test_rebase_am_only -C4\n+# Also check an explicit --apply\n+test_rebase_am_only --apply\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"470861","messageId":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v3.git.1674266126.gitgitgadget@gmail.com","subject":"[PATCH v4 0/9] rebase: fix several code/testing/documentation issues around flag incompatibilities","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-22T06:12:31Z","receivedAt":"2023-01-22T06:13:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We had a report about --update-refs being ignored when --whitespace=fix was\npassed, confusing an end user. These were already marked as incompatible in\nthe manual, but the code didn't check for the incompatibility and provide an\nerror to the user.\n\nFolks brought up other flags tangentially when reviewing an earlier round of\nthis series, and I found we had more that were missing checks, and that we\nwere missing some testcases, and that the documentation was wrong on some of\nthe relevant options. So, I ended up getting lots of little fixes to\nstraighten these all out.\n\nChanges since v3:\n\n * Corrected the code surrounding --[no-]reapply-cherry-picks, and extended\n the testcases (Thanks to Phillip for pointing out my error)\n * I went ahead and implemented the better error message when the merge\n backend is triggered solely via config options.\n\nChanges since v2:\n\n * Remove the extra patch and reword the comment in t3422 more thoroughly.\n * Add tests for other flag incompatibilities that were missing\n * Add code that was missing for checking various flag incompatibilities\n * Fix incorrect claims in the documentation around incompatible options\n * A few other miscellaneous fixups noticed while doing all the above\n\nChanges since v1:\n\n * Add a patch nuking the -C option to rebase (fixes confusion around the\n comment in t3422 and acknowledges the fact that the option is totally and\n utterly useless and always has been. It literally never affects the\n results of a rebase.)\n\nSigned-off-by: Elijah Newren newren@gmail.com\n\nElijah Newren (9):\n rebase: mark --update-refs as requiring the merge backend\n rebase: flag --apply and --merge as incompatible\n rebase: remove --allow-empty-message from incompatible opts\n rebase: fix docs about incompatibilities with --root\n rebase: add coverage of other incompatible options\n rebase: clarify the OPT_CMDMODE incompatibilities\n rebase: fix formatting of rebase --reapply-cherry-picks option in docs\n rebase: put rebase_options initialization in single place\n rebase: provide better error message for apply options vs. merge\n config\n\n Documentation/git-rebase.txt | 77 +++++++++++++-------------\n builtin/rebase.c | 72 +++++++++++++++++++-----\n t/t3422-rebase-incompatible-options.sh | 71 ++++++++++++++++++++++--\n 3 files changed, 162 insertions(+), 58 deletions(-)\n\n\nbase-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/1466\n\nRange-diff vs v3:\n\n 1: 9089834adea = 1: 8a676e6ec1a rebase: mark --update-refs as requiring the merge backend\n 2: a8b5a0e4fb0 = 2: cc129b87185 rebase: flag --apply and --merge as incompatible\n 3: f4fbfd40d45 = 3: 9464bbbe9ba rebase: remove --allow-empty-message from incompatible opts\n 4: a1e61ac8f21 = 4: b702f15ed7c rebase: fix docs about incompatibilities with --root\n 5: 48c40c0dda0 ! 5: 5e4851e611e rebase: add coverage of other incompatible options\n @@ Commit message\n code checks for some of these, which could result in command line\n options being silently ignored.\n \n + Also, note that adding a check for autosquash means that using\n + --whitespace=fix together with the config setting rebase.autosquash=true\n + will trigger an error. A subsequent commit will improve the error\n + message.\n +\n Signed-off-by: Elijah Newren \n \n + ## Documentation/git-rebase.txt ##\n +@@ Documentation/git-rebase.txt: are incompatible with the following options:\n + * --exec\n + * --no-keep-empty\n + * --empty=\n +- * --reapply-cherry-picks\n ++ * --[no-]reapply-cherry-picks\n + * --edit-todo\n + * --update-refs\n + * --root when used without --onto\n +\n ## builtin/rebase.c ##\n @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tif (options.fork_point < 0)\n \t\t\toptions.fork_point = 0;\n \t}\n +\t/*\n -+\t * reapply_cherry_picks is slightly weird. It starts out with a\n -+\t * value of -1. It will be assigned a value of keep_base below and\n -+\t * then handled specially. The apply backend is hardcoded to\n -+\t * behave like reapply_cherry_picks==1, so if it has that value, we\n -+\t * can just ignore the flag with the apply backend. Thus, we only\n -+\t * really need to throw an error and require the merge backend if\n -+\t * reapply_cherry_picks==0.\n ++\t * The apply backend does not support --[no-]reapply-cherry-picks.\n ++\t * The behavior it implements by default is equivalent to\n ++\t * --no-reapply-cherry-picks (due to passing --cherry-picks to\n ++\t * format-patch), but --keep-base alters the upstream such that no\n ++\t * cherry-picks can be found (effectively making it act like\n ++\t * --reapply-cherry-picks).\n ++\t *\n ++\t * Now, if the user does specify --[no-]reapply-cherry-picks, but\n ++\t * does so in such a way that options.reapply_cherry_picks ==\n ++\t * keep_base, then the behavior they get will match what they\n ++\t * expect despite options.reapply_cherry_picks being ignored. We\n ++\t * could just allow the flag in that case, but it seems better to\n ++\t * just alert the user that they've specified a flag that the\n ++\t * backend ignores.\n +\t */\n -+\tif (options.reapply_cherry_picks == 0)\n -+\t\timply_merge(&options, \"--no-reapply-cherry-picks\");\n ++\tif (options.reapply_cherry_picks >= 0)\n ++\t\timply_merge(&options, options.reapply_cherry_picks ? \"--reapply-cherry-picks\" :\n ++\t\t\t\t\t\t\t\t \"--no-reapply-cherry-picks\");\n ++\n \t/*\n \t * --keep-base defaults to --reapply-cherry-picks to avoid losing\n \t * commits when using this option.\n @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {\n +\t\tgit checkout B^0 &&\n +\t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n +\t\"\n ++\n ++\ttest_expect_success \"$opt incompatible with --reapply-cherry-picks\" \"\n ++\t\tgit checkout B^0 &&\n ++\t\ttest_must_fail git rebase $opt --reapply-cherry-picks A\n ++\t\"\n +\n \ttest_expect_success \"$opt incompatible with --update-refs\" \"\n \t\tgit checkout B^0 &&\n 6: 8664cce6cf7 ! 6: 21ae9e05313 rebase: clarify the OPT_CMDMODE incompatibilities\n @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.\n @@ Documentation/git-rebase.txt: are incompatible with the following options:\n * --no-keep-empty\n * --empty=\n - * --reapply-cherry-picks\n + * --[no-]reapply-cherry-picks\n - * --edit-todo\n * --update-refs\n * --root when used without --onto\n 7: 0e8b06163f2 = 7: 74a216bf0c0 rebase: fix formatting of rebase --reapply-cherry-picks option in docs\n -: ----------- > 8: a8adf7fda61 rebase: put rebase_options initialization in single place\n -: ----------- > 9: 5cb00e5103b rebase: provide better error message for apply options vs. merge config\n\n-- \ngitgitgadget\n"},{"id":"470863","messageId":"8a676e6ec1a30a0220ca0d222211a4cdbf0646a2.1674367961.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","subject":"[PATCH v4 1/9] rebase: mark --update-refs as requiring the merge backend","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-22T06:12:32Z","receivedAt":"2023-01-22T06:13:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--update-refs is built in terms of the sequencer, which requires the\nmerge backend. It was already marked as incompatible with the apply\nbackend in the git-rebase manual, but the code didn't check for this\nincompatibility and warn the user. Check and error now.\n\nWhile at it, fix a typo in t3422...and fix some misleading wording\n(most options which used to be am-specific have since been implemented\nin the merge backend as well).\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 2 ++\n builtin/rebase.c | 3 +++\n t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----\n 3 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex d811c1cf443..6490bc96a15 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -630,6 +630,8 @@ start would be overridden by the presence of\n +\n If the configuration variable `rebase.updateRefs` is set, then this option\n can be used to override and disable this setting.\n++\n+See also INCOMPATIBLE OPTIONS below.\n \n INCOMPATIBLE OPTIONS\n --------------------\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex a26cc0cfdb5..c111b89e137 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1492,6 +1492,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (options.update_refs)\n+\t\timply_merge(&options, \"--update-refs\");\n+\n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\n \t\t\timply_merge(&options, \"--merge\");\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 6dabb05a2ad..9da39cd91c2 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -25,11 +25,11 @@ test_expect_success 'setup' '\n '\n \n #\n-# Rebase has lots of useful options like --whitepsace=fix, which are\n-# actually all built in terms of flags to git-am. Since neither\n-# --merge nor --interactive (nor any options that imply those two) use\n-# git-am, using them together will result in flags like --whitespace=fix\n-# being ignored. Make sure rebase warns the user and aborts instead.\n+# Rebase has a couple options which are specific to the apply backend,\n+# and several options which are specific to the merge backend. Flags\n+# from the different sets cannot work together, and we do not want to\n+# just ignore one of the sets of flags. Make sure rebase warns the\n+# user and aborts instead.\n #\n \n test_rebase_am_only () {\n@@ -60,6 +60,11 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --exec 'true' A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --update-refs\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --update-refs A\n+\t\"\n+\n }\n \n test_rebase_am_only --whitespace=fix\n-- \ngitgitgadget\n\n"},{"id":"470862","messageId":"9464bbbe9ba60927a3c36ac9685545447744819c.1674367961.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","subject":"[PATCH v4 3/9] rebase: remove --allow-empty-message from incompatible opts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-22T06:12:34Z","receivedAt":"2023-01-22T06:13:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--allow-empty-message was turned into a no-op and even documented\nas such; the flag is simply ignored. Since the flag is ignored, it\nshouldn't be documented as being incompatible with other flags.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 6490bc96a15..7d01d1412d1 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -647,7 +647,6 @@ are incompatible with the following options:\n * --merge\n * --strategy\n * --strategy-option\n- * --allow-empty-message\n * --[no-]autosquash\n * --rebase-merges\n * --interactive\n-- \ngitgitgadget\n\n"},{"id":"470864","messageId":"a8adf7fda61f6651d739c2209c38775b59923eec.1674367961.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","subject":"[PATCH v4 8/9] rebase: put rebase_options initialization in single place","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-22T06:12:39Z","receivedAt":"2023-01-22T06:13:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n builtin/rebase.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex ed7475804cb..2358605b254 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -134,6 +134,8 @@ struct rebase_options {\n \t\t.exec = STRING_LIST_INIT_NODUP,\t\t\\\n \t\t.git_format_patch_opt = STRBUF_INIT,\t\\\n \t\t.fork_point = -1,\t\t\t\\\n+\t\t.reapply_cherry_picks = -1, \\\n+\t\t.allow_empty_message = 1, \\\n \t}\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n@@ -1158,8 +1160,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\n \n-\toptions.reapply_cherry_picks = -1;\n-\toptions.allow_empty_message = 1;\n \tgit_config(rebase_config, &options);\n \t/* options.gpg_sign_opt will be either \"-S\" or NULL */\n \tgpg_sign = options.gpg_sign_opt ? \"\" : NULL;\n-- \ngitgitgadget\n\n"},{"id":"470865","messageId":"5e4851e611ee18112bd71939ee900e02a8d590c5.1674367961.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","subject":"[PATCH v4 5/9] rebase: add coverage of other incompatible options","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-22T06:12:36Z","receivedAt":"2023-01-22T06:13:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe git-rebase manual noted several sets of incompatible options, but\nwe were missing tests for a few of these. Further, we were missing\ncode checks for some of these, which could result in command line\noptions being silently ignored.\n\nAlso, note that adding a check for autosquash means that using\n--whitespace=fix together with the config setting rebase.autosquash=true\nwill trigger an error. A subsequent commit will improve the error\nmessage.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 2 +-\n builtin/rebase.c | 30 ++++++++++++++++++++------\n t/t3422-rebase-incompatible-options.sh | 25 +++++++++++++++++++++\n 3 files changed, 49 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 846aeed1b69..8cb075b2bdb 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -650,7 +650,7 @@ are incompatible with the following options:\n * --exec\n * --no-keep-empty\n * --empty=\n- * --reapply-cherry-picks\n+ * --[no-]reapply-cherry-picks\n * --edit-todo\n * --update-refs\n * --root when used without --onto\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b742cc8fb5c..ed7475804cb 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tif (options.fork_point < 0)\n \t\t\toptions.fork_point = 0;\n \t}\n+\t/*\n+\t * The apply backend does not support --[no-]reapply-cherry-picks.\n+\t * The behavior it implements by default is equivalent to\n+\t * --no-reapply-cherry-picks (due to passing --cherry-picks to\n+\t * format-patch), but --keep-base alters the upstream such that no\n+\t * cherry-picks can be found (effectively making it act like\n+\t * --reapply-cherry-picks).\n+\t *\n+\t * Now, if the user does specify --[no-]reapply-cherry-picks, but\n+\t * does so in such a way that options.reapply_cherry_picks ==\n+\t * keep_base, then the behavior they get will match what they\n+\t * expect despite options.reapply_cherry_picks being ignored. We\n+\t * could just allow the flag in that case, but it seems better to\n+\t * just alert the user that they've specified a flag that the\n+\t * backend ignores.\n+\t */\n+\tif (options.reapply_cherry_picks >= 0)\n+\t\timply_merge(&options, options.reapply_cherry_picks ? \"--reapply-cherry-picks\" :\n+\t\t\t\t\t\t\t\t \"--no-reapply-cherry-picks\");\n+\n \t/*\n \t * --keep-base defaults to --reapply-cherry-picks to avoid losing\n \t * commits when using this option.\n@@ -1406,13 +1426,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.empty != EMPTY_UNSPECIFIED)\n \t\timply_merge(&options, \"--empty\");\n \n-\t/*\n-\t * --keep-base implements --reapply-cherry-picks by altering upstream so\n-\t * it works with both backends.\n-\t */\n-\tif (options.reapply_cherry_picks && !keep_base)\n-\t\timply_merge(&options, \"--reapply-cherry-picks\");\n-\n \tif (gpg_sign)\n \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n \n@@ -1503,6 +1516,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.update_refs)\n \t\timply_merge(&options, \"--update-refs\");\n \n+\tif (options.autosquash)\n+\t\timply_merge(&options, \"--autosquash\");\n+\n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\n \t\t\timply_merge(&options, \"--merge\");\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex f86274990b0..6a17b571ec7 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -50,6 +50,11 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --strategy-option=ours A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --autosquash\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --autosquash A\n+\t\"\n+\n \ttest_expect_success \"$opt incompatible with --interactive\" \"\n \t\tgit checkout B^0 &&\n \t\ttest_must_fail git rebase $opt --interactive A\n@@ -60,6 +65,26 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --exec 'true' A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --keep-empty\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --keep-empty A\n+\t\"\n+\n+\ttest_expect_success \"$opt incompatible with --empty=...\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --empty=ask A\n+\t\"\n+\n+\ttest_expect_success \"$opt incompatible with --no-reapply-cherry-picks\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n+\t\"\n+\n+\ttest_expect_success \"$opt incompatible with --reapply-cherry-picks\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --reapply-cherry-picks A\n+\t\"\n+\n \ttest_expect_success \"$opt incompatible with --update-refs\" \"\n \t\tgit checkout B^0 &&\n \t\ttest_must_fail git rebase $opt --update-refs A\n-- \ngitgitgadget\n\n"},{"id":"470866","messageId":"5cb00e5103bd701a90ea3f5b3532fa184b6e6ae9.1674367961.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","subject":"[PATCH v4 9/9] rebase: provide better error message for apply options vs. merge config","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-22T06:12:40Z","receivedAt":"2023-01-22T06:13:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen config which selects the merge backend (currently,\nrebase.autosquash=true or rebase.updateRefs=true) conflicts with other\noptions on the command line (such as --whitespace=fix), make the error\nmessage specifically call out the config option and specify how to\noverride that config option on the command line.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 2 +-\n builtin/rebase.c | 27 ++++++++++++++++++++------\n t/t3422-rebase-incompatible-options.sh | 24 +++++++++++++++++++++++\n 3 files changed, 46 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 9f794f5bdcc..c357f6c3d5c 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -648,7 +648,7 @@ are incompatible with the following options:\n * --merge\n * --strategy\n * --strategy-option\n- * --[no-]autosquash\n+ * --autosquash\n * --rebase-merges\n * --interactive\n * --exec\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 2358605b254..dd7e2c788f5 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -122,6 +122,8 @@ struct rebase_options {\n \tint reapply_cherry_picks;\n \tint fork_point;\n \tint update_refs;\n+\tint config_autosquash;\n+\tint config_update_refs;\n };\n \n #define REBASE_OPTIONS_INIT {\t\t\t \t\\\n@@ -136,6 +138,10 @@ struct rebase_options {\n \t\t.fork_point = -1,\t\t\t\\\n \t\t.reapply_cherry_picks = -1, \\\n \t\t.allow_empty_message = 1, \\\n+\t\t.autosquash = -1, \\\n+\t\t.config_autosquash = -1, \\\n+\t\t.update_refs = -1, \\\n+\t\t.config_update_refs = -1, \\\n \t}\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n@@ -778,7 +784,7 @@ static int rebase_config(const char *var, const char *value, void *data)\n \t}\n \n \tif (!strcmp(var, \"rebase.autosquash\")) {\n-\t\topts->autosquash = git_config_bool(var, value);\n+\t\topts->config_autosquash = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -795,7 +801,7 @@ static int rebase_config(const char *var, const char *value, void *data)\n \t}\n \n \tif (!strcmp(var, \"rebase.updaterefs\")) {\n-\t\topts->update_refs = git_config_bool(var, value);\n+\t\topts->config_update_refs = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -1393,7 +1399,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||\n \t (options.action != ACTION_NONE) ||\n \t (options.exec.nr > 0) ||\n-\t options.autosquash) {\n+\t (options.autosquash == -1 && options.config_autosquash == 1) ||\n+\t options.autosquash == 1) {\n \t\tallow_preemptive_ff = 0;\n \t}\n \tif (options.committer_date_is_author_date || options.ignore_date)\n@@ -1504,20 +1511,28 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tif (strcmp(options.git_am_opts.v[i], \"-q\"))\n \t\t\t\tbreak;\n \n-\t\tif (i >= 0) {\n+\t\tif (i >= 0 || options.type == REBASE_APPLY) {\n \t\t\tif (is_merge(&options))\n \t\t\t\tdie(_(\"apply options and merge options \"\n \t\t\t\t\t \"cannot be used together\"));\n+\t\t\telse if (options.autosquash == -1 && options.config_autosquash == 1)\n+\t\t\t\tdie(_(\"apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash\"));\n+\t\t\telse if (options.update_refs == -1 && options.config_update_refs == 1)\n+\t\t\t\tdie(_(\"apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs\"));\n \t\t\telse\n \t\t\t\toptions.type = REBASE_APPLY;\n \t\t}\n \t}\n \n-\tif (options.update_refs)\n+\tif (options.update_refs == 1)\n \t\timply_merge(&options, \"--update-refs\");\n+\toptions.update_refs = (options.update_refs >= 0) ? options.update_refs :\n+\t\t\t ((options.config_update_refs >= 0) ? options.config_update_refs : 0);\n \n-\tif (options.autosquash)\n+\tif (options.autosquash == 1)\n \t\timply_merge(&options, \"--autosquash\");\n+\toptions.autosquash = (options.autosquash >= 0) ? options.autosquash :\n+\t\t\t ((options.config_autosquash >= 0) ? options.config_autosquash : 0);\n \n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 6a17b571ec7..4711b37a288 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -94,6 +94,30 @@ test_rebase_am_only () {\n \t\tgit checkout B^0 &&\n \t\ttest_must_fail git rebase $opt --root A\n \t\"\n+\n+\ttest_expect_success \"$opt incompatible with rebase.autosquash\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git -c rebase.autosquash=true rebase $opt A 2>err &&\n+\t\tgrep -e --no-autosquash err\n+\t\"\n+\n+\ttest_expect_success \"$opt incompatible with rebase.updateRefs\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&\n+\t\tgrep -e --no-update-refs err\n+\t\"\n+\n+\ttest_expect_success \"$opt okay with overridden rebase.autosquash\" \"\n+\t\ttest_when_finished \\\"git reset --hard B^0\\\" &&\n+\t\tgit checkout B^0 &&\n+\t\tgit -c rebase.autosquash=true rebase --no-autosquash $opt A\n+\t\"\n+\n+\ttest_expect_success \"$opt okay with overridden rebase.updateRefs\" \"\n+\t\ttest_when_finished \\\"git reset --hard B^0\\\" &&\n+\t\tgit checkout B^0 &&\n+\t\tgit -c rebase.updateRefs=true rebase --no-update-refs $opt A\n+\t\"\n }\n \n # Check options which imply --apply\n-- \ngitgitgadget\n"},{"id":"470867","messageId":"b702f15ed7c2c48dc0f502ed0d7d6491c2056408.1674367961.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","subject":"[PATCH v4 4/9] rebase: fix docs about incompatibilities with --root","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-22T06:12:35Z","receivedAt":"2023-01-22T06:13:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit 5dacd4abdd (\"git-rebase.txt: document incompatible options\",\n2018-06-25), I added notes about incompatibilities between options for\nthe apply and merge backends. Unfortunately, I inverted the condition\nwhen --root was incompatible with the apply backend. Fix the\ndocumentation, and add a testcase that verifies the documentation\nmatches the code.\n\nWhile at it, the documentation for --root also tried to cover some of\nthe backend differences between the apply and merge backends in relation\nto reapplying cherry picks. The information:\n * assumed that the apply backend was the default (it isn't anymore)\n * was written before --reapply-cherry-picks became an option\n * was written before the detailed information on backend differences\nAll of these factors make the sentence under --root about reapplying\ncherry picks contradict information that is now available elsewhere in\nthe manual, and the other references are correct. So just strike this\nsentence.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 7 ++-----\n t/t3422-rebase-incompatible-options.sh | 4 ++++\n 2 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 7d01d1412d1..846aeed1b69 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -574,10 +574,7 @@ See also INCOMPATIBLE OPTIONS below.\n --root::\n \tRebase all commits reachable from ``, instead of\n \tlimiting them with an ``. This allows you to rebase\n-\tthe root commit(s) on a branch. When used with `--onto`, it\n-\twill skip changes already contained in `` (instead of\n-\t``) whereas without `--onto` it will operate on every\n-\tchange.\n+\tthe root commit(s) on a branch.\n +\n See also INCOMPATIBLE OPTIONS below.\n \n@@ -656,7 +653,7 @@ are incompatible with the following options:\n * --reapply-cherry-picks\n * --edit-todo\n * --update-refs\n- * --root when used in combination with --onto\n+ * --root when used without --onto\n \n In addition, the following pairs of options are incompatible:\n \ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 9b9e78479f6..f86274990b0 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -65,6 +65,10 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --update-refs A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --root without --onto\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --root A\n+\t\"\n }\n \n # Check options which imply --apply\n-- \ngitgitgadget\n\n"},{"id":"470868","messageId":"74a216bf0c02d4cb3e91b0de9e5fcd617f611672.1674367961.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","subject":"[PATCH v4 7/9] rebase: fix formatting of rebase --reapply-cherry-picks option in docs","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-22T06:12:38Z","receivedAt":"2023-01-22T06:13:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCommit ce5238a690 (\"rebase --keep-base: imply --reapply-cherry-picks\",\n2022-10-17) accidentally added some blank lines that cause extra\nparagraphs about --reapply-cherry-picks to be considered not part of\nthe documentation of that option. Remove the blank lines to make it\nclear we are still discussing --reapply-cherry-picks.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 1ba691e4c5f..9f794f5bdcc 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -338,7 +338,6 @@ See also INCOMPATIBLE OPTIONS below.\n \tupstream changes, the behavior towards them is controlled by\n \tthe `--empty` flag.)\n +\n-\n In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is\n given), these commits will be automatically dropped. Because this\n necessitates reading all upstream commits, this can be expensive in\n@@ -347,7 +346,6 @@ read. When using the 'merge' backend, warnings will be issued for each\n dropped commit (unless `--quiet` is given). Advice will also be issued\n unless `advice.skippedCherryPicks` is set to false (see\n linkgit:git-config[1]).\n-\n +\n `--reapply-cherry-picks` allows rebase to forgo reading all upstream\n commits, potentially improving performance.\n-- \ngitgitgadget\n\n"},{"id":"470869","messageId":"21ae9e053131a9a5f0ddb13fba4bf6502a1d19a6.1674367961.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","subject":"[PATCH v4 6/9] rebase: clarify the OPT_CMDMODE incompatibilities","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-22T06:12:37Z","receivedAt":"2023-01-22T06:13:05Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--edit-todo was documented as being incompatible with any of the options\nfor the apply backend. However, it is also incompatible with any of the\noptions for the merge backend, and is incompatible with any options that\nare not backend specific as well. The same can be said for --continue,\n--skip, --abort, --quit, etc.\n\nThis is already somewhat implicitly covered by the synopsis, but since\n\"[]\" in the first two variants are vague it might be easy to\nmiss this. That might not be a big deal, but since the rebase manpage\nhas to spend so much verbiage about incompatibility of options, making\na separate section for these options that are incompatible with\neverything else seems clearer. Do that, and remove the needless\ninclusion of --edit-todo in the explicit incompatibility list.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 61 +++++++++++++++++++-----------------\n 1 file changed, 33 insertions(+), 28 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 8cb075b2bdb..1ba691e4c5f 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -208,6 +208,39 @@ Alternatively, you can undo the 'git rebase' with\n \n git rebase --abort\n \n+MODE OPTIONS\n+------------\n+\n+The options in this section cannot be used with any other option,\n+including not with each other:\n+\n+--continue::\n+\tRestart the rebasing process after having resolved a merge conflict.\n+\n+--skip::\n+\tRestart the rebasing process by skipping the current patch.\n+\n+--abort::\n+\tAbort the rebase operation and reset HEAD to the original\n+\tbranch. If `` was provided when the rebase operation was\n+\tstarted, then `HEAD` will be reset to ``. Otherwise `HEAD`\n+\twill be reset to where it was when the rebase operation was\n+\tstarted.\n+\n+--quit::\n+\tAbort the rebase operation but `HEAD` is not reset back to the\n+\toriginal branch. The index and working tree are also left\n+\tunchanged as a result. If a temporary stash entry was created\n+\tusing `--autostash`, it will be saved to the stash list.\n+\n+--edit-todo::\n+\tEdit the todo list during an interactive rebase.\n+\n+--show-current-patch::\n+\tShow the current patch in an interactive rebase or when rebase\n+\tis stopped because of conflicts. This is the equivalent of\n+\t`git show REBASE_HEAD`.\n+\n OPTIONS\n -------\n --onto ::\n@@ -249,22 +282,6 @@ See also INCOMPATIBLE OPTIONS below.\n ::\n \tWorking branch; defaults to `HEAD`.\n \n---continue::\n-\tRestart the rebasing process after having resolved a merge conflict.\n-\n---abort::\n-\tAbort the rebase operation and reset HEAD to the original\n-\tbranch. If `` was provided when the rebase operation was\n-\tstarted, then `HEAD` will be reset to ``. Otherwise `HEAD`\n-\twill be reset to where it was when the rebase operation was\n-\tstarted.\n-\n---quit::\n-\tAbort the rebase operation but `HEAD` is not reset back to the\n-\toriginal branch. The index and working tree are also left\n-\tunchanged as a result. If a temporary stash entry was created\n-\tusing `--autostash`, it will be saved to the stash list.\n-\n --apply::\n \tUse applying strategies to rebase (calling `git-am`\n \tinternally). This option may become a no-op in the future\n@@ -345,17 +362,6 @@ See also INCOMPATIBLE OPTIONS below.\n +\n See also INCOMPATIBLE OPTIONS below.\n \n---skip::\n-\tRestart the rebasing process by skipping the current patch.\n-\n---edit-todo::\n-\tEdit the todo list during an interactive rebase.\n-\n---show-current-patch::\n-\tShow the current patch in an interactive rebase or when rebase\n-\tis stopped because of conflicts. This is the equivalent of\n-\t`git show REBASE_HEAD`.\n-\n -m::\n --merge::\n \tUsing merging strategies to rebase (default).\n@@ -651,7 +657,6 @@ are incompatible with the following options:\n * --no-keep-empty\n * --empty=\n * --[no-]reapply-cherry-picks\n- * --edit-todo\n * --update-refs\n * --root when used without --onto\n \n-- \ngitgitgadget\n\n"},{"id":"470926","messageId":"1d9748df-4f54-908d-75cf-49ff1d154fcb@github.com","threadId":"59117","inReplyTo":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 0/9] rebase: fix several code/testing/documentation issues around flag incompatibilities","fromName":"Derrick Stolee","fromEmail":"derrickstolee@github.com","sentAt":"2023-01-23T15:56:05Z","receivedAt":"2023-01-23T15:56:18Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/22/2023 1:12 AM, Elijah Newren via GitGitGadget wrote:\n> We had a report about --update-refs being ignored when --whitespace=fix was\n> passed, confusing an end user. These were already marked as incompatible in\n> the manual, but the code didn't check for the incompatibility and provide an\n> error to the user.\n> \n> Folks brought up other flags tangentially when reviewing an earlier round of\n> this series, and I found we had more that were missing checks, and that we\n> were missing some testcases, and that the documentation was wrong on some of\n> the relevant options. So, I ended up getting lots of little fixes to\n> straighten these all out.\n\nWow, this really expanded since I last looked at it. Thanks for taking on all\nof that extra work! (That was not my intention when recommending that you take\nover the fix.)\n \n> Changes since v3:\n> \n> * Corrected the code surrounding --[no-]reapply-cherry-picks, and extended\n> the testcases (Thanks to Phillip for pointing out my error)\n> * I went ahead and implemented the better error message when the merge\n> backend is triggered solely via config options.\n\nI really appreciate this extra attention to detail. I'm also really happy with\nhow you implemented it, using different variables to signal how the option was\nspecified (and using -1 for \"unset\" in both cases).\n\nWhile I had not been following version 2 or 3, I read this version in its\nentirety and everything looked good to me. These improvements to our docs,\ntests, and implementation will be felt by users.\n\nThanks!\n-Stolee\n"},{"id":"470947","messageId":"759fb313-ce88-4eb7-96c0-4adeb75ca9f9@dunelm.org.uk","threadId":"59117","inReplyTo":"5e4851e611ee18112bd71939ee900e02a8d590c5.1674367961.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-23T20:08:50Z","receivedAt":"2023-01-23T20:09:00Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> The git-rebase manual noted several sets of incompatible options, but\n> we were missing tests for a few of these. Further, we were missing\n> code checks for some of these, which could result in command line\n> options being silently ignored.\n> \n> Also, note that adding a check for autosquash means that using\n> --whitespace=fix together with the config setting rebase.autosquash=true\n> will trigger an error. A subsequent commit will improve the error\n> message.\n\nThanks for updating the commit message and for the new commits at the \nend of the series.\n\n> Signed-off-by: Elijah Newren \n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tif (options.fork_point < 0)\n> \t\t\toptions.fork_point = 0;\n> \t}\n> +\t/*\n> +\t * The apply backend does not support --[no-]reapply-cherry-picks.\n> +\t * The behavior it implements by default is equivalent to\n> +\t * --no-reapply-cherry-picks (due to passing --cherry-picks to\n> +\t * format-patch), but --keep-base alters the upstream such that no\n> +\t * cherry-picks can be found (effectively making it act like\n> +\t * --reapply-cherry-picks).\n> +\t *\n> +\t * Now, if the user does specify --[no-]reapply-cherry-picks, but\n> +\t * does so in such a way that options.reapply_cherry_picks ==\n> +\t * keep_base, then the behavior they get will match what they\n> +\t * expect despite options.reapply_cherry_picks being ignored. We\n> +\t * could just allow the flag in that case, but it seems better to\n> +\t * just alert the user that they've specified a flag that the\n> +\t * backend ignores.\n> +\t */\n\nI'm a bit confused by this. --keep-base works with either \n--reapply-cherry-picks (which is the default if --keep-base is given) or \n--no-reapply-cherry-picks. Just below this hunk we have\n\n\tif (options.reapply_cherry_picks < 0)\n\t\toptions.reapply_cherry_picks = keep_base;\n\nSo we only set options.reapply_cherry_picks to match keep_base if the \nuser did not specify -[-no]-reapply-cherry-picks on the commandline.\n\nBest Wishes\n\nPhillip\n\n> +\tif (options.reapply_cherry_picks >= 0)\n> +\t\timply_merge(&options, options.reapply_cherry_picks ? \"--reapply-cherry-picks\" :\n> +\t\t\t\t\t\t\t\t \"--no-reapply-cherry-picks\");\n> +\n> \t/*\n> \t * --keep-base defaults to --reapply-cherry-picks to avoid losing\n> \t * commits when using this option.\n> @@ -1406,13 +1426,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tif (options.empty != EMPTY_UNSPECIFIED)\n> \t\timply_merge(&options, \"--empty\");\n> \n> -\t/*\n> -\t * --keep-base implements --reapply-cherry-picks by altering upstream so\n> -\t * it works with both backends.\n> -\t */\n> -\tif (options.reapply_cherry_picks && !keep_base)\n> -\t\timply_merge(&options, \"--reapply-cherry-picks\");\n> -\n> \tif (gpg_sign)\n> \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n> \n> @@ -1503,6 +1516,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tif (options.update_refs)\n> \t\timply_merge(&options, \"--update-refs\");\n> \n> +\tif (options.autosquash)\n> +\t\timply_merge(&options, \"--autosquash\");\n> +\n> \tif (options.type == REBASE_UNSPECIFIED) {\n> \t\tif (!strcmp(options.default_backend, \"merge\"))\n> \t\t\timply_merge(&options, \"--merge\");\n> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\n> index f86274990b0..6a17b571ec7 100755\n> --- a/t/t3422-rebase-incompatible-options.sh\n> +++ b/t/t3422-rebase-incompatible-options.sh\n> @@ -50,6 +50,11 @@ test_rebase_am_only () {\n> \t\ttest_must_fail git rebase $opt --strategy-option=ours A\n> \t\"\n> \n> +\ttest_expect_success \"$opt incompatible with --autosquash\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --autosquash A\n> +\t\"\n> +\n> \ttest_expect_success \"$opt incompatible with --interactive\" \"\n> \t\tgit checkout B^0 &&\n> \t\ttest_must_fail git rebase $opt --interactive A\n> @@ -60,6 +65,26 @@ test_rebase_am_only () {\n> \t\ttest_must_fail git rebase $opt --exec 'true' A\n> \t\"\n> \n> +\ttest_expect_success \"$opt incompatible with --keep-empty\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --keep-empty A\n> +\t\"\n> +\n> +\ttest_expect_success \"$opt incompatible with --empty=...\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --empty=ask A\n> +\t\"\n> +\n> +\ttest_expect_success \"$opt incompatible with --no-reapply-cherry-picks\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n> +\t\"\n> +\n> +\ttest_expect_success \"$opt incompatible with --reapply-cherry-picks\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --reapply-cherry-picks A\n> +\t\"\n> +\n> \ttest_expect_success \"$opt incompatible with --update-refs\" \"\n> \t\tgit checkout B^0 &&\n> \t\ttest_must_fail git rebase $opt --update-refs A\n"},{"id":"470960","messageId":"CABPp-BG4mUv0W1Duo3Jm6iOALY4Z2sT3o3nqDhz2sboe3AH1aQ@mail.gmail.com","threadId":"59117","inReplyTo":"1d9748df-4f54-908d-75cf-49ff1d154fcb@github.com","subject":"Re: [PATCH v4 0/9] rebase: fix several code/testing/documentation issues around flag incompatibilities","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-24T02:05:45Z","receivedAt":"2023-01-24T02:06:02Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Jan 23, 2023 at 7:56 AM Derrick Stolee wrote:\n>\n> On 1/22/2023 1:12 AM, Elijah Newren via GitGitGadget wrote:\n> > We had a report about --update-refs being ignored when --whitespace=fix was\n> > passed, confusing an end user. These were already marked as incompatible in\n> > the manual, but the code didn't check for the incompatibility and provide an\n> > error to the user.\n> >\n> > Folks brought up other flags tangentially when reviewing an earlier round of\n> > this series, and I found we had more that were missing checks, and that we\n> > were missing some testcases, and that the documentation was wrong on some of\n> > the relevant options. So, I ended up getting lots of little fixes to\n> > straighten these all out.\n>\n> Wow, this really expanded since I last looked at it. Thanks for taking on all\n> of that extra work! (That was not my intention when recommending that you take\n> over the fix.)\n\nYeah, I know you were willing to let some of the work wait for some\nfuture series, and I intended to take that route. But...\n * both you and Phillip brought up --autosquash, and it turns out we\ndidn't check it\n * the above made me check the whole list of incompatibilities and\ndiscover other missing checks\n * adding tests made me discover that the documented\nincompatibilities had a few issues\n * you mentioned that an explicit --apply wasn't checked either, and\nsince I was already diving in I figured I might as well handle that\ntoo\n * even though you mentioned the config fix wasn't needed, both Junio\nand Phillip brought it up as well,\n and based on the various feedback gotten so far, I started think\nthat just addressing it might require\n less work than going through more back and forth on review of the\nfeature without that implementation.\n\n> > Changes since v3:\n> >\n> > * Corrected the code surrounding --[no-]reapply-cherry-picks, and extended\n> > the testcases (Thanks to Phillip for pointing out my error)\n> > * I went ahead and implemented the better error message when the merge\n> > backend is triggered solely via config options.\n>\n> I really appreciate this extra attention to detail. I'm also really happy with\n> how you implemented it, using different variables to signal how the option was\n> specified (and using -1 for \"unset\" in both cases).\n>\n> While I had not been following version 2 or 3, I read this version in its\n> entirety and everything looked good to me. These improvements to our docs,\n> tests, and implementation will be felt by users.\n\nThanks for reading!\n"},{"id":"470961","messageId":"CABPp-BE+wRgjmWknARQpNsdUFjNOz0ND9wgx_-_RTyK+EwJjXA@mail.gmail.com","threadId":"59117","inReplyTo":"759fb313-ce88-4eb7-96c0-4adeb75ca9f9@dunelm.org.uk","subject":"Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-24T02:36:00Z","receivedAt":"2023-01-24T02:36:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Phillip,\n\nOn Mon, Jan 23, 2023 at 12:08 PM Phillip Wood wrote:\n>\n> Hi Elijah\n>\n> On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:\n> > From: Elijah Newren \n> >\n> > The git-rebase manual noted several sets of incompatible options, but\n> > we were missing tests for a few of these. Further, we were missing\n> > code checks for some of these, which could result in command line\n> > options being silently ignored.\n> >\n> > Also, note that adding a check for autosquash means that using\n> > --whitespace=fix together with the config setting rebase.autosquash=true\n> > will trigger an error. A subsequent commit will improve the error\n> > message.\n>\n> Thanks for updating the commit message and for the new commits at the\n> end of the series.\n>\n> > Signed-off-by: Elijah Newren \n> > --- a/builtin/rebase.c\n> > +++ b/builtin/rebase.c\n> > @@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> > if (options.fork_point < 0)\n> > options.fork_point = 0;\n> > }\n> > + /*\n> > + * The apply backend does not support --[no-]reapply-cherry-picks.\n> > + * The behavior it implements by default is equivalent to\n> > + * --no-reapply-cherry-picks (due to passing --cherry-picks to\n> > + * format-patch), but --keep-base alters the upstream such that no\n> > + * cherry-picks can be found (effectively making it act like\n> > + * --reapply-cherry-picks).\n> > + *\n> > + * Now, if the user does specify --[no-]reapply-cherry-picks, but\n> > + * does so in such a way that options.reapply_cherry_picks ==\n> > + * keep_base, then the behavior they get will match what they\n> > + * expect despite options.reapply_cherry_picks being ignored. We\n> > + * could just allow the flag in that case, but it seems better to\n> > + * just alert the user that they've specified a flag that the\n> > + * backend ignores.\n> > + */\n>\n> I'm a bit confused by this. --keep-base works with either\n> --reapply-cherry-picks (which is the default if --keep-base is given) or\n> --no-reapply-cherry-picks. Just below this hunk we have\n>\n> if (options.reapply_cherry_picks < 0)\n> options.reapply_cherry_picks = keep_base;\n>\n> So we only set options.reapply_cherry_picks to match keep_base if the\n> user did not specify -[-no]-reapply-cherry-picks on the commandline.\n\noptions.reapply_cherry_picks is totally ignored by the apply backend,\nregardless of whether it's set by the user or the setup code in\nbuiltin/rebase.c. And if we have an option which is ignored, isn't it\nnicer to provide an error message to the user if they tried to set it?\n\nSaid another way, while users could start with these command lines:\n\n (Y) git rebase --whitespace=fix\n (Z) git rebase --whitespace=fix --keep-base\n\nand modify them to include flags that would be ignored, we could allow:\n\n (A) git rebase --whitespace=fix --no-reapply-cherry-picks\n (B) git rebase --whitespace=fix --keep-base --reapply-cherry-picks\n\nBut we could not allow commands like\n\n (C) git rebase --whitespace=fix --reapply-cherry-picks\n (D) git rebase --whitespace=fix --keep-base --no-reapply-cherry-picks\n\nFor all four cases (A)-(D), the apply backend will ignore whatever\n--[no-]reapply-cherry-picks flag is provided. For (A) and (B), the\nbehavior the apply backend provides happens to match what the user\nis requesting, while for (C) and (D) the behavior does not match.\nSo we should at least reject (C) and (D). But, although we could\ntechnically allow (A) and (B), what advantage would it provide? I\nthink the results of allowing those two commands would be:\n\n 1) Confusion by end users -- why should (C) & (D) throw errors if\n (A) and (B) are accepted? That's not an easy rule to understand.\n\n 2) More confusion by end users -- the documentation for years has\n stated that --reapply-cherry-picks is incompatible with the apply\n backend, suggesting users would be surprised if at least (B) and\n probably (A) didn't throw error messages.\n\n 3) Confusing documentation -- If we don't want to throw errors for\n (A) and (B), how do we modify the \"INCOMPATIBLE OPTIONS\" section\n of Documentation/git-rebase.txt to explain the relevant details\n of when these flags are (or are not) incompatible with the apply\n backend? I think it'd end up with a very verbose explanation\n that likely confuses more than it helps.\n\n 4) Excessively complicated code -- The previous attempts to\n implement this got it wrong. Prior to ce5238a690 (\"rebase\n --keep-base: imply --reapply-cherry-picks\", 2022-10-17), the code\n would error out on (B) and (C). After that commit, it would only\n error out on (C). Both solutions are incorrect since they miss\n (D), and I think the code just becomes hard to hard to follow in\n order to only error out on both (C) and (D) without (A) and (B).\n\n(#2 and #3 might just be a repeat of the same issue, documentation,\nbut it seemed easier to write separately.)\n\nI think it's simpler for the code, for the documentation, and for end\nusers to just error out on all of (A), (B), (C), and (D).\n --[no-]reapply-cherry-picks is not supported by the apply backend.\n\nBut, given this lengthy email, perhaps I should split out the handling\nof --[no-]reapply-cherry-picks into its own commit and copy some or\nall of the description above into the commit message?\n"},{"id":"470964","messageId":"83d27162-59d4-d8c0-fde3-f522630d024d@dunelm.org.uk","threadId":"59117","inReplyTo":"CABPp-BE+wRgjmWknARQpNsdUFjNOz0ND9wgx_-_RTyK+EwJjXA@mail.gmail.com","subject":"Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-24T10:27:53Z","receivedAt":"2023-01-24T10:28:06Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 24/01/2023 02:36, Elijah Newren wrote:\n> Hi Phillip,\n> \n> On Mon, Jan 23, 2023 at 12:08 PM Phillip Wood wrote:\n>>\n>> Hi Elijah\n>>\n>> On 22/01/2023 06:12, Elijah Newren via GitGitGadget wrote:\n>>> From: Elijah Newren \n>>>\n>>> The git-rebase manual noted several sets of incompatible options, but\n>>> we were missing tests for a few of these. Further, we were missing\n>>> code checks for some of these, which could result in command line\n>>> options being silently ignored.\n>>>\n>>> Also, note that adding a check for autosquash means that using\n>>> --whitespace=fix together with the config setting rebase.autosquash=true\n>>> will trigger an error. A subsequent commit will improve the error\n>>> message.\n>>\n>> Thanks for updating the commit message and for the new commits at the\n>> end of the series.\n>>\n>>> Signed-off-by: Elijah Newren \n>>> --- a/builtin/rebase.c\n>>> +++ b/builtin/rebase.c\n>>> @@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>>> if (options.fork_point < 0)\n>>> options.fork_point = 0;\n>>> }\n>>> + /*\n>>> + * The apply backend does not support --[no-]reapply-cherry-picks.\n>>> + * The behavior it implements by default is equivalent to\n>>> + * --no-reapply-cherry-picks (due to passing --cherry-picks to\n>>> + * format-patch), but --keep-base alters the upstream such that no\n>>> + * cherry-picks can be found (effectively making it act like\n>>> + * --reapply-cherry-picks).\n>>> + *\n>>> + * Now, if the user does specify --[no-]reapply-cherry-picks, but\n>>> + * does so in such a way that options.reapply_cherry_picks ==\n>>> + * keep_base, then the behavior they get will match what they\n>>> + * expect despite options.reapply_cherry_picks being ignored. We\n>>> + * could just allow the flag in that case, but it seems better to\n>>> + * just alert the user that they've specified a flag that the\n>>> + * backend ignores.\n>>> + */\n>>\n>> I'm a bit confused by this. --keep-base works with either\n>> --reapply-cherry-picks (which is the default if --keep-base is given) or\n>> --no-reapply-cherry-picks. Just below this hunk we have\n>>\n>> if (options.reapply_cherry_picks < 0)\n>> options.reapply_cherry_picks = keep_base;\n>>\n>> So we only set options.reapply_cherry_picks to match keep_base if the\n>> user did not specify -[-no]-reapply-cherry-picks on the commandline.\n> \n> options.reapply_cherry_picks is totally ignored by the apply backend,\n> regardless of whether it's set by the user or the setup code in\n> builtin/rebase.c. And if we have an option which is ignored, isn't it\n> nicer to provide an error message to the user if they tried to set it?\n> \n> Said another way, while users could start with these command lines:\n> \n> (Y) git rebase --whitespace=fix\n> (Z) git rebase --whitespace=fix --keep-base\n> \n> and modify them to include flags that would be ignored, we could allow:\n> \n> (A) git rebase --whitespace=fix --no-reapply-cherry-picks\n> (B) git rebase --whitespace=fix --keep-base --reapply-cherry-picks\n> \n> But we could not allow commands like\n> \n> (C) git rebase --whitespace=fix --reapply-cherry-picks\n> (D) git rebase --whitespace=fix --keep-base --no-reapply-cherry-picks\n\n(C) is already an error\n(D) is currently allowed and I think works as expected (--keep-base only \nimplies --reapply-cherry-picks, the user is free to override that with \n--no-reapply-cherry-picks) so I don't see why we'd want to make it an error.\n\n> For all four cases (A)-(D), the apply backend will ignore whatever\n> --[no-]reapply-cherry-picks flag is provided.\n\nFor (D) the flag is respected, (C) errors out, the other cases \ncorrespond to the default so it's like saying\n\n\tgit rebase --merge --no-reapply-cherry-picks\n\nignores the flag. Arguably it is confusing that the apply backend only \nsupports -[-no]-reapply-cherry-picks if --keep-base is given but I'm not \nsure that is a good reason to reject a combination that currently works \nas expected.\n\nBest Wishes\n\nPhillip\n\n> For (A) and (B), the > behavior the apply backend provides happens to match what the user\n> is requesting, while for (C) and (D) the behavior does not match.\n> So we should at least reject (C) and (D). But, although we could\n> technically allow (A) and (B), what advantage would it provide? I\n> think the results of allowing those two commands would be:\n> \n> 1) Confusion by end users -- why should (C) & (D) throw errors if\n> (A) and (B) are accepted? That's not an easy rule to understand.\n> \n> 2) More confusion by end users -- the documentation for years has\n> stated that --reapply-cherry-picks is incompatible with the apply\n> backend, suggesting users would be surprised if at least (B) and\n> probably (A) didn't throw error messages.\n> \n> 3) Confusing documentation -- If we don't want to throw errors for\n> (A) and (B), how do we modify the \"INCOMPATIBLE OPTIONS\" section\n> of Documentation/git-rebase.txt to explain the relevant details\n> of when these flags are (or are not) incompatible with the apply\n> backend? I think it'd end up with a very verbose explanation\n> that likely confuses more than it helps.\n> \n> 4) Excessively complicated code -- The previous attempts to\n> implement this got it wrong. Prior to ce5238a690 (\"rebase\n> --keep-base: imply --reapply-cherry-picks\", 2022-10-17), the code\n> would error out on (B) and (C). After that commit, it would only\n> error out on (C). Both solutions are incorrect since they miss\n> (D), and I think the code just becomes hard to hard to follow in\n> order to only error out on both (C) and (D) without (A) and (B).\n> \n> (#2 and #3 might just be a repeat of the same issue, documentation,\n> but it seemed easier to write separately.)\n> \n> I think it's simpler for the code, for the documentation, and for end\n> users to just error out on all of (A), (B), (C), and (D).\n> --[no-]reapply-cherry-picks is not supported by the apply backend.\n> \n> But, given this lengthy email, perhaps I should split out the handling\n> of --[no-]reapply-cherry-picks into its own commit and copy some or\n> all of the description above into the commit message?\n"},{"id":"470970","messageId":"7b9ee972-2680-2e1b-bef3-201d8a1e4bdd@dunelm.org.uk","threadId":"59117","inReplyTo":"83d27162-59d4-d8c0-fde3-f522630d024d@dunelm.org.uk","subject":"Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-24T13:16:34Z","receivedAt":"2023-01-24T13:16:44Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":">>>> Signed-off-by: Elijah Newren \n>>>> --- a/builtin/rebase.c\n>>>> +++ b/builtin/rebase.c\n>>>> @@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv, \n>>>> const char *prefix)\n>>>>                if (options.fork_point < 0)\n>>>>                        options.fork_point = 0;\n>>>>        }\n>>>> +     /*\n>>>> +      * The apply backend does not support \n>>>> --[no-]reapply-cherry-picks.\n>>>> +      * The behavior it implements by default is equivalent to\n>>>> +      * --no-reapply-cherry-picks (due to passing --cherry-picks to\n>>>> +      * format-patch), but --keep-base alters the upstream such \n>>>> that no\n>>>> +      * cherry-picks can be found (effectively making it act like\n>>>> +      * --reapply-cherry-picks).\n>>>> +      *\n>>>> +      * Now, if the user does specify --[no-]reapply-cherry-picks, but\n>>>> +      * does so in such a way that options.reapply_cherry_picks ==\n>>>> +      * keep_base, then the behavior they get will match what they\n>>>> +      * expect despite options.reapply_cherry_picks being ignored.  We\n>>>> +      * could just allow the flag in that case, but it seems better to\n>>>> +      * just alert the user that they've specified a flag that the\n>>>> +      * backend ignores.\n>>>> +      */\n>>>\n>>> I'm a bit confused by this. --keep-base works with either\n>>> --reapply-cherry-picks (which is the default if --keep-base is given) or\n>>> --no-reapply-cherry-picks. Just below this hunk we have\n>>>\n>>>          if (options.reapply_cherry_picks < 0)\n>>>                  options.reapply_cherry_picks = keep_base;\n>>>\n>>> So we only set options.reapply_cherry_picks to match keep_base if the\n>>> user did not specify -[-no]-reapply-cherry-picks on the commandline.\n>>\n>> options.reapply_cherry_picks is totally ignored by the apply backend,\n>> regardless of whether it's set by the user or the setup code in\n>> builtin/rebase.c.  And if we have an option which is ignored, isn't it\n>> nicer to provide an error message to the user if they tried to set it?\n>>\n>> Said another way, while users could start with these command lines:\n>>\n>>      (Y) git rebase --whitespace=fix\n>>      (Z) git rebase --whitespace=fix --keep-base\n>>\n>> and modify them to include flags that would be ignored, we could allow:\n>>\n>>      (A) git rebase --whitespace=fix --no-reapply-cherry-picks\n>>      (B) git rebase --whitespace=fix --keep-base --reapply-cherry-picks\n>>\n>> But we could not allow commands like\n>>\n>>      (C) git rebase --whitespace=fix --reapply-cherry-picks\n>>      (D) git rebase --whitespace=fix --keep-base \n>> --no-reapply-cherry-picks\n> \n> (C) is already an error\n> (D) is currently allowed and I think works as expected (--keep-base only \n> implies --reapply-cherry-picks, the user is free to override that with \n> --no-reapply-cherry-picks) so I don't see why we'd want to make it an \n> error.\n> \n>> For all four cases (A)-(D), the apply backend will ignore whatever\n>> --[no-]reapply-cherry-picks flag is provided.\n> \n> For (D) the flag is respected, (C) errors out, the other cases \n> correspond to the default so it's like saying\n> \n>     git rebase --merge --no-reapply-cherry-picks\n> \n> ignores the flag.\n\nOn reflection that is only true for (B). I agree that we should error \nout on (A) which we don't at the moment.\n\nI'd support a change that errors out on (A) and (C) but continues to \nallow (B) and (D). I think we can do that with the diff below\n\nBest Wishes\n\nPhillip\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 1481c5b6a5..66aef356b8 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1230,12 +1230,6 @@ int cmd_rebase(int argc, const char **argv, const \nchar *prefix)\n if (options.fork_point < 0)\n options.fork_point = 0;\n }\n- /*\n- * --keep-base defaults to --reapply-cherry-picks to avoid losing\n- * commits when using this option.\n- */\n- if (options.reapply_cherry_picks < 0)\n- options.reapply_cherry_picks = keep_base;\n\n if (options.root && options.fork_point > 0)\n die(_(\"options '%s' and '%s' cannot be used \ntogether\"), \"--root\", \"--fork-point\");\n@@ -1412,11 +1406,17 @@ int cmd_rebase(int argc, const char **argv, \nconst char *prefix)\n if (options.empty != EMPTY_UNSPECIFIED)\n imply_merge(&options, \"--empty\");\n\n- /*\n- * --keep-base implements --reapply-cherry-picks by altering \nupstream so\n- * it works with both backends.\n- */\n- if (options.reapply_cherry_picks && !keep_base)\n+ if (options.reapply_cherry_picks < 0)\n+ /*\n+ * --keep-base defaults to --reapply-cherry-picks to\n+ * avoid losing commits when using this option.\n+ */\n+ options.reapply_cherry_picks = keep_base;\n+ else if (!keep_base)\n+ /*\n+ * --keep-base implements --reapply-cherry-picks by\n+ * altering upstream so it works with both backends.\n+ */\n imply_merge(&options, \"--reapply-cherry-picks\");\n\n if (gpg_sign)\n"},{"id":"470977","messageId":"xmqqpmb4j8e9.fsf@gitster.g","threadId":"59117","inReplyTo":"7b9ee972-2680-2e1b-bef3-201d8a1e4bdd@dunelm.org.uk","subject":"Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-24T14:48:30Z","receivedAt":"2023-01-24T14:48:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On reflection that is only true for (B). I agree that we should error\n> out on (A) which we don't at the moment.\n>\n> I'd support a change that errors out on (A) and (C) but continues to\n> allow (B) and (D). I think we can do that with the diff below\n\nThanks, both of you, for well reasoned design work and respectful\ncommunication.\n"},{"id":"470985","messageId":"CABPp-BHVUc7EdY9z_TPcHspCak6Yc3mfDXUkivj4zq_fJem3SQ@mail.gmail.com","threadId":"59117","inReplyTo":"7b9ee972-2680-2e1b-bef3-201d8a1e4bdd@dunelm.org.uk","subject":"Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-24T15:41:48Z","receivedAt":"2023-01-24T15:42:34Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Phillip,\n\nOn Tue, Jan 24, 2023 at 5:16 AM Phillip Wood wrote:\n>\n> >>>> Signed-off-by: Elijah Newren \n> >>>> --- a/builtin/rebase.c\n> >>>> +++ b/builtin/rebase.c\n> >>>> @@ -1224,6 +1224,26 @@ int cmd_rebase(int argc, const char **argv,\n> >>>> const char *prefix)\n> >>>> if (options.fork_point < 0)\n> >>>> options.fork_point = 0;\n> >>>> }\n> >>>> + /*\n> >>>> + * The apply backend does not support\n> >>>> --[no-]reapply-cherry-picks.\n> >>>> + * The behavior it implements by default is equivalent to\n> >>>> + * --no-reapply-cherry-picks (due to passing --cherry-picks to\n> >>>> + * format-patch), but --keep-base alters the upstream such\n> >>>> that no\n> >>>> + * cherry-picks can be found (effectively making it act like\n> >>>> + * --reapply-cherry-picks).\n> >>>> + *\n> >>>> + * Now, if the user does specify --[no-]reapply-cherry-picks, but\n> >>>> + * does so in such a way that options.reapply_cherry_picks ==\n> >>>> + * keep_base, then the behavior they get will match what they\n> >>>> + * expect despite options.reapply_cherry_picks being ignored. We\n> >>>> + * could just allow the flag in that case, but it seems better to\n> >>>> + * just alert the user that they've specified a flag that the\n> >>>> + * backend ignores.\n> >>>> + */\n> >>>\n> >>> I'm a bit confused by this. --keep-base works with either\n> >>> --reapply-cherry-picks (which is the default if --keep-base is given) or\n> >>> --no-reapply-cherry-picks. Just below this hunk we have\n> >>>\n> >>> if (options.reapply_cherry_picks < 0)\n> >>> options.reapply_cherry_picks = keep_base;\n> >>>\n> >>> So we only set options.reapply_cherry_picks to match keep_base if the\n> >>> user did not specify -[-no]-reapply-cherry-picks on the commandline.\n> >>\n> >> options.reapply_cherry_picks is totally ignored by the apply backend,\n> >> regardless of whether it's set by the user or the setup code in\n> >> builtin/rebase.c. And if we have an option which is ignored, isn't it\n> >> nicer to provide an error message to the user if they tried to set it?\n> >>\n> >> Said another way, while users could start with these command lines:\n> >>\n> >> (Y) git rebase --whitespace=fix\n> >> (Z) git rebase --whitespace=fix --keep-base\n> >>\n> >> and modify them to include flags that would be ignored, we could allow:\n> >>\n> >> (A) git rebase --whitespace=fix --no-reapply-cherry-picks\n> >> (B) git rebase --whitespace=fix --keep-base --reapply-cherry-picks\n> >>\n> >> But we could not allow commands like\n> >>\n> >> (C) git rebase --whitespace=fix --reapply-cherry-picks\n> >> (D) git rebase --whitespace=fix --keep-base\n> >> --no-reapply-cherry-picks\n> >\n> > (C) is already an error\n> > (D) is currently allowed and I think works as expected (--keep-base only\n> > implies --reapply-cherry-picks, the user is free to override that with\n> > --no-reapply-cherry-picks) so I don't see why we'd want to make it an\n> > error.\n\nAh, despite looking over the code multiple times to check my\nstatements, I somehow kept missing this:\n\n if (keep_base && options.reapply_cherry_picks)\n options.upstream = options.onto;\n\nwhich is how --[no-]reapply-cherry-picks is supported in conjunction\nwith --keep-base. Thanks.\n\n> >> For all four cases (A)-(D), the apply backend will ignore whatever\n> >> --[no-]reapply-cherry-picks flag is provided.\n> >\n> > For (D) the flag is respected, (C) errors out, the other cases\n> > correspond to the default so it's like saying\n> >\n> > git rebase --merge --no-reapply-cherry-picks\n> >\n> > ignores the flag.\n>\n> On reflection that is only true for (B). I agree that we should error\n> out on (A) which we don't at the moment.\n>\n> I'd support a change that errors out on (A) and (C) but continues to\n> allow (B) and (D). I think we can do that with the diff below\n>\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index 1481c5b6a5..66aef356b8 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1230,12 +1230,6 @@ int cmd_rebase(int argc, const char **argv, const\n> char *prefix)\n> if (options.fork_point < 0)\n> options.fork_point = 0;\n> }\n> - /*\n> - * --keep-base defaults to --reapply-cherry-picks to avoid losing\n> - * commits when using this option.\n> - */\n> - if (options.reapply_cherry_picks < 0)\n> - options.reapply_cherry_picks = keep_base;\n>\n> if (options.root && options.fork_point > 0)\n> die(_(\"options '%s' and '%s' cannot be used\n> together\"), \"--root\", \"--fork-point\");\n> @@ -1412,11 +1406,17 @@ int cmd_rebase(int argc, const char **argv,\n> const char *prefix)\n> if (options.empty != EMPTY_UNSPECIFIED)\n> imply_merge(&options, \"--empty\");\n>\n> - /*\n> - * --keep-base implements --reapply-cherry-picks by altering\n> upstream so\n> - * it works with both backends.\n> - */\n> - if (options.reapply_cherry_picks && !keep_base)\n> + if (options.reapply_cherry_picks < 0)\n> + /*\n> + * --keep-base defaults to --reapply-cherry-picks to\n> + * avoid losing commits when using this option.\n> + */\n\nI know you were copying the previous comment, but this comment is\nreally confusing to me. Shouldn't it read \"--reapply-cherry-picks\ndefaults to --keep-base\" instead of vice-versa?\n\n> + options.reapply_cherry_picks = keep_base;\n> + else if (!keep_base)\n> + /*\n> + * --keep-base implements --reapply-cherry-picks by\n\nShould this be --[no-]reapply-cherry-picks, to clarify that it handles\nboth cases? Especially given how many times I missed it?\n\n> + * altering upstream so it works with both backends.\n> + */\n> imply_merge(&options, \"--reapply-cherry-picks\");\n\nAnd perhaps this should be\n\n imply_merge(&options, options.reapply_cherry_picks ?\n\"--reapply-cherry-picks\" :\n \"--no-reapply-cherry-picks\");\n\nAlso, the comment in git-rebase.txt about incompatibilities would become\n\n * --[no-]reapply-cherry-picks, when --keep-base is not provided\n"},{"id":"470988","messageId":"0418e5b6-8cbd-9dc9-085e-31380beda89b@dunelm.org.uk","threadId":"59117","inReplyTo":"CABPp-BHVUc7EdY9z_TPcHspCak6Yc3mfDXUkivj4zq_fJem3SQ@mail.gmail.com","subject":"Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-24T16:48:18Z","receivedAt":"2023-01-24T16:48:35Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 24/01/2023 15:41, Elijah Newren wrote:\n> Ah, despite looking over the code multiple times to check my\n> statements, I somehow kept missing this:\n> \n> if (keep_base && options.reapply_cherry_picks)\n> options.upstream = options.onto;\n> \n> which is how --[no-]reapply-cherry-picks is supported in conjunction\n> with --keep-base. Thanks.\n> \n>>>> For all four cases (A)-(D), the apply backend will ignore whatever\n>>>> --[no-]reapply-cherry-picks flag is provided.\n>>>\n>>> For (D) the flag is respected, (C) errors out, the other cases\n>>> correspond to the default so it's like saying\n>>>\n>>> git rebase --merge --no-reapply-cherry-picks\n>>>\n>>> ignores the flag.\n>>\n>> On reflection that is only true for (B). I agree that we should error\n>> out on (A) which we don't at the moment.\n>>\n>> I'd support a change that errors out on (A) and (C) but continues to\n>> allow (B) and (D). I think we can do that with the diff below\n>>\n>>\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index 1481c5b6a5..66aef356b8 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -1230,12 +1230,6 @@ int cmd_rebase(int argc, const char **argv, const\n>> char *prefix)\n>> if (options.fork_point < 0)\n>> options.fork_point = 0;\n>> }\n>> - /*\n>> - * --keep-base defaults to --reapply-cherry-picks to avoid losing\n>> - * commits when using this option.\n>> - */\n>> - if (options.reapply_cherry_picks < 0)\n>> - options.reapply_cherry_picks = keep_base;\n>>\n>> if (options.root && options.fork_point > 0)\n>> die(_(\"options '%s' and '%s' cannot be used\n>> together\"), \"--root\", \"--fork-point\");\n>> @@ -1412,11 +1406,17 @@ int cmd_rebase(int argc, const char **argv,\n>> const char *prefix)\n>> if (options.empty != EMPTY_UNSPECIFIED)\n>> imply_merge(&options, \"--empty\");\n>>\n>> - /*\n>> - * --keep-base implements --reapply-cherry-picks by altering\n>> upstream so\n>> - * it works with both backends.\n>> - */\n>> - if (options.reapply_cherry_picks && !keep_base)\n>> + if (options.reapply_cherry_picks < 0)\n>> + /*\n>> + * --keep-base defaults to --reapply-cherry-picks to\n>> + * avoid losing commits when using this option.\n>> + */\n> \n> I know you were copying the previous comment, but this comment is\n> really confusing to me. Shouldn't it read \"--reapply-cherry-picks\n> defaults to --keep-base\" instead of vice-versa?\n\nClearly this comment has not been as helpful as I indented it to be. I \nthink maybe we should spell out the defaults with and without \n--keep-base. Perhaps something like\n\ndefault to --no-reapply-cherry-picks unless --keep-base is given in \nwhich case default to --reapply-cherry-picks\n\n\n>> + options.reapply_cherry_picks = keep_base;\n>> + else if (!keep_base)\n>> + /*\n>> + * --keep-base implements --reapply-cherry-picks by\n> \n> Should this be --[no-]reapply-cherry-picks, to clarify that it handles\n> both cases? Especially given how many times I missed it?\n\nThis has obviously proved to be confusing. The aim was to explain that \nin order to work with the apply backend \"[--reapply-cherry-picks] \n--keep-base\" was doing something unusual with `upstream` to reapply \ncherry picks. \"--no-reapply-cherry-picks --keep-base\" does not do \nanything unusual with `upstream`. I don't think changing it to \n--[no-]reapply-cherry-picks quite captures that. I came up with\n\nTo support --reapply-cherry-picks (which is not supported by the apply \nbackend) --keep-base alters upstream to prevent cherry picked commits \nfrom being dropped.\n\nbut it really needs to mention that --keep-base also supports \n--no-reapply-cherry-picks in the usual way\n\n>> + * altering upstream so it works with both backends.\n>> + */\n>> imply_merge(&options, \"--reapply-cherry-picks\");\n> \n> And perhaps this should be\n> \n> imply_merge(&options, options.reapply_cherry_picks ?\n> \"--reapply-cherry-picks\" :\n> \"--no-reapply-cherry-picks\");\n\nThat's a good idea\n\n> Also, the comment in git-rebase.txt about incompatibilities would become\n> \n> * --[no-]reapply-cherry-picks, when --keep-base is not provided\n\nYes, that's good\n\nThanks again for working on this\n\nPhillip\n"},{"id":"470989","messageId":"CABPp-BEwv+cRMOR_-kz_UhfQt1+SGRdhictLmwmq=122LYZaDw@mail.gmail.com","threadId":"59117","inReplyTo":"0418e5b6-8cbd-9dc9-085e-31380beda89b@dunelm.org.uk","subject":"Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-24T17:12:07Z","receivedAt":"2023-01-24T17:12:25Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Jan 24, 2023 at 8:48 AM Phillip Wood wrote:\n>\n> Hi Elijah\n>\n> On 24/01/2023 15:41, Elijah Newren wrote:\n[...]\n> >> - /*\n> >> - * --keep-base implements --reapply-cherry-picks by altering\n> >> upstream so\n> >> - * it works with both backends.\n> >> - */\n> >> - if (options.reapply_cherry_picks && !keep_base)\n> >> + if (options.reapply_cherry_picks < 0)\n> >> + /*\n> >> + * --keep-base defaults to --reapply-cherry-picks to\n> >> + * avoid losing commits when using this option.\n> >> + */\n> >\n> > I know you were copying the previous comment, but this comment is\n> > really confusing to me. Shouldn't it read \"--reapply-cherry-picks\n> > defaults to --keep-base\" instead of vice-versa?\n>\n> Clearly this comment has not been as helpful as I indented it to be. I\n> think maybe we should spell out the defaults with and without\n> --keep-base. Perhaps something like\n>\n> default to --no-reapply-cherry-picks unless --keep-base is given in\n> which case default to --reapply-cherry-picks\n\nI like that; sounds good to me.\n\n> >> + options.reapply_cherry_picks = keep_base;\n> >> + else if (!keep_base)\n> >> + /*\n> >> + * --keep-base implements --reapply-cherry-picks by\n> >\n> > Should this be --[no-]reapply-cherry-picks, to clarify that it handles\n> > both cases? Especially given how many times I missed it?\n>\n> This has obviously proved to be confusing. The aim was to explain that\n> in order to work with the apply backend \"[--reapply-cherry-picks]\n> --keep-base\" was doing something unusual with `upstream` to reapply\n> cherry picks. \"--no-reapply-cherry-picks --keep-base\" does not do\n> anything unusual with `upstream`. I don't think changing it to\n> --[no-]reapply-cherry-picks quite captures that. I came up with\n>\n> To support --reapply-cherry-picks (which is not supported by the apply\n> backend) --keep-base alters upstream to prevent cherry picked commits\n> from being dropped.\n>\n> but it really needs to mention that --keep-base also supports\n> --no-reapply-cherry-picks in the usual way\n\nSomewhat wordy, but perhaps:\n\n /*\n * The apply backend always searches for and drops cherry\n * picks. This is often not wanted with --keep-base, so\n * --keep-base allows --reapply-cherry-picks to be\n * simulated by altering the upstream such that\n * cherry-picks cannot be detected and thus all commits are\n * reapplied. Thus, --[no-]reapply-cherry-picks is\n * supported when --keep-base is specified, but not when\n * --keep-base is left out.\n */\n\n?\n"},{"id":"470998","messageId":"83c489c6-7927-ee3a-03bb-7896a928c280@dunelm.org.uk","threadId":"59117","inReplyTo":"CABPp-BEwv+cRMOR_-kz_UhfQt1+SGRdhictLmwmq=122LYZaDw@mail.gmail.com","subject":"Re: [PATCH v4 5/9] rebase: add coverage of other incompatible options","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-24T19:21:18Z","receivedAt":"2023-01-24T19:21:43Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 24/01/2023 17:12, Elijah Newren wrote:\n> On Tue, Jan 24, 2023 at 8:48 AM Phillip Wood wrote:\n>>>> + options.reapply_cherry_picks = keep_base;\n>>>> + else if (!keep_base)\n>>>> + /*\n>>>> + * --keep-base implements --reapply-cherry-picks by\n>>>\n>>> Should this be --[no-]reapply-cherry-picks, to clarify that it handles\n>>> both cases? Especially given how many times I missed it?\n>>\n>> This has obviously proved to be confusing. The aim was to explain that\n>> in order to work with the apply backend \"[--reapply-cherry-picks]\n>> --keep-base\" was doing something unusual with `upstream` to reapply\n>> cherry picks. \"--no-reapply-cherry-picks --keep-base\" does not do\n>> anything unusual with `upstream`. I don't think changing it to\n>> --[no-]reapply-cherry-picks quite captures that. I came up with\n>>\n>> To support --reapply-cherry-picks (which is not supported by the apply\n>> backend) --keep-base alters upstream to prevent cherry picked commits\n>> from being dropped.\n>>\n>> but it really needs to mention that --keep-base also supports\n>> --no-reapply-cherry-picks in the usual way\n> \n> Somewhat wordy, but perhaps:\n> \n> /*\n> * The apply backend always searches for and drops cherry\n> * picks. This is often not wanted with --keep-base, so\n> * --keep-base allows --reapply-cherry-picks to be\n> * simulated by altering the upstream such that\n> * cherry-picks cannot be detected and thus all commits are\n> * reapplied. Thus, --[no-]reapply-cherry-picks is\n> * supported when --keep-base is specified, but not when\n> * --keep-base is left out.\n> */\n\nThat sounds good to me, it is definitely an improvement on the current \ncomment which I think is too terse.\n\nBest Wishes\n\nPhillip\n"},{"id":"471006","messageId":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v4.git.1674367961.gitgitgadget@gmail.com","subject":"[PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T04:03:44Z","receivedAt":"2023-01-25T04:04:03Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"We had a report about --update-refs being ignored when --whitespace=fix was\npassed, confusing an end user. These were already marked as incompatible in\nthe manual, but the code didn't check for the incompatibility and provide an\nerror to the user.\n\nFolks brought up other flags tangentially when reviewing an earlier round of\nthis series, and I found we had more that were missing checks, and that we\nwere missing some testcases, and that the documentation was wrong on some of\nthe relevant options. So, I ended up getting lots of little fixes to\nstraighten these all out.\n\nChanges since v4:\n\n * Pulled out the changes regarding incompatibility detection for\n --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot\n with understanding the behavior, suggesting changes, and getting the\n wording right, and I think it deserves its own patch with its own\n explanation.\n\nChanges since v3:\n\n * Corrected the code surrounding --[no-]reapply-cherry-picks, and extended\n the testcases (Thanks to Phillip for pointing out my error)\n * I went ahead and implemented the better error message when the merge\n backend is triggered solely via config options.\n\nChanges since v2:\n\n * Remove the extra patch and reword the comment in t3422 more thoroughly.\n * Add tests for other flag incompatibilities that were missing\n * Add code that was missing for checking various flag incompatibilities\n * Fix incorrect claims in the documentation around incompatible options\n * A few other miscellaneous fixups noticed while doing all the above\n\nChanges since v1:\n\n * Add a patch nuking the -C option to rebase (fixes confusion around the\n comment in t3422 and acknowledges the fact that the option is totally and\n utterly useless and always has been. It literally never affects the\n results of a rebase.)\n\nSigned-off-by: Elijah Newren newren@gmail.com\n\nElijah Newren (10):\n rebase: mark --update-refs as requiring the merge backend\n rebase: flag --apply and --merge as incompatible\n rebase: remove --allow-empty-message from incompatible opts\n rebase: fix docs about incompatibilities with --root\n rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks\n rebase: add coverage of other incompatible options\n rebase: clarify the OPT_CMDMODE incompatibilities\n rebase: fix formatting of rebase --reapply-cherry-picks option in docs\n rebase: put rebase_options initialization in single place\n rebase: provide better error message for apply options vs. merge\n config\n\n Documentation/git-rebase.txt | 77 ++++++++++++-------------\n builtin/rebase.c | 79 +++++++++++++++++++-------\n t/t3422-rebase-incompatible-options.sh | 71 +++++++++++++++++++++--\n 3 files changed, 163 insertions(+), 64 deletions(-)\n\n\nbase-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/1466\n\nRange-diff vs v4:\n\n 1: 8a676e6ec1a = 1: 8a676e6ec1a rebase: mark --update-refs as requiring the merge backend\n 2: cc129b87185 = 2: cc129b87185 rebase: flag --apply and --merge as incompatible\n 3: 9464bbbe9ba = 3: 9464bbbe9ba rebase: remove --allow-empty-message from incompatible opts\n 4: b702f15ed7c = 4: b702f15ed7c rebase: fix docs about incompatibilities with --root\n -: ----------- > 5: 3a8429f3d2b rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks\n 5: 5e4851e611e ! 6: 2777dd2788a rebase: add coverage of other incompatible options\n @@ Commit message\n \n The git-rebase manual noted several sets of incompatible options, but\n we were missing tests for a few of these. Further, we were missing\n - code checks for some of these, which could result in command line\n + code checks for one of these, which could result in command line\n options being silently ignored.\n \n Also, note that adding a check for autosquash means that using\n @@ Commit message\n \n Signed-off-by: Elijah Newren \n \n - ## Documentation/git-rebase.txt ##\n -@@ Documentation/git-rebase.txt: are incompatible with the following options:\n - * --exec\n - * --no-keep-empty\n - * --empty=\n -- * --reapply-cherry-picks\n -+ * --[no-]reapply-cherry-picks\n - * --edit-todo\n - * --update-refs\n - * --root when used without --onto\n -\n ## builtin/rebase.c ##\n -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n - \t\tif (options.fork_point < 0)\n - \t\t\toptions.fork_point = 0;\n - \t}\n -+\t/*\n -+\t * The apply backend does not support --[no-]reapply-cherry-picks.\n -+\t * The behavior it implements by default is equivalent to\n -+\t * --no-reapply-cherry-picks (due to passing --cherry-picks to\n -+\t * format-patch), but --keep-base alters the upstream such that no\n -+\t * cherry-picks can be found (effectively making it act like\n -+\t * --reapply-cherry-picks).\n -+\t *\n -+\t * Now, if the user does specify --[no-]reapply-cherry-picks, but\n -+\t * does so in such a way that options.reapply_cherry_picks ==\n -+\t * keep_base, then the behavior they get will match what they\n -+\t * expect despite options.reapply_cherry_picks being ignored. We\n -+\t * could just allow the flag in that case, but it seems better to\n -+\t * just alert the user that they've specified a flag that the\n -+\t * backend ignores.\n -+\t */\n -+\tif (options.reapply_cherry_picks >= 0)\n -+\t\timply_merge(&options, options.reapply_cherry_picks ? \"--reapply-cherry-picks\" :\n -+\t\t\t\t\t\t\t\t \"--no-reapply-cherry-picks\");\n -+\n - \t/*\n - \t * --keep-base defaults to --reapply-cherry-picks to avoid losing\n - \t * commits when using this option.\n -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n - \tif (options.empty != EMPTY_UNSPECIFIED)\n - \t\timply_merge(&options, \"--empty\");\n - \n --\t/*\n --\t * --keep-base implements --reapply-cherry-picks by altering upstream so\n --\t * it works with both backends.\n --\t */\n --\tif (options.reapply_cherry_picks && !keep_base)\n --\t\timply_merge(&options, \"--reapply-cherry-picks\");\n --\n - \tif (gpg_sign)\n - \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n - \n @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.update_refs)\n \t\timply_merge(&options, \"--update-refs\");\n @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {\n +\t\ttest_must_fail git rebase $opt --empty=ask A\n +\t\"\n +\n -+\ttest_expect_success \"$opt incompatible with --no-reapply-cherry-picks\" \"\n -+\t\tgit checkout B^0 &&\n -+\t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n -+\t\"\n -+\n -+\ttest_expect_success \"$opt incompatible with --reapply-cherry-picks\" \"\n -+\t\tgit checkout B^0 &&\n -+\t\ttest_must_fail git rebase $opt --reapply-cherry-picks A\n -+\t\"\n -+\n - \ttest_expect_success \"$opt incompatible with --update-refs\" \"\n + \ttest_expect_success \"$opt incompatible with --no-reapply-cherry-picks\" \"\n \t\tgit checkout B^0 &&\n - \t\ttest_must_fail git rebase $opt --update-refs A\n + \t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n 6: 21ae9e05313 ! 7: 0d0541ea243 rebase: clarify the OPT_CMDMODE incompatibilities\n @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.\n @@ Documentation/git-rebase.txt: are incompatible with the following options:\n * --no-keep-empty\n * --empty=\n - * --[no-]reapply-cherry-picks\n + * --[no-]reapply-cherry-picks when used without --keep-base\n - * --edit-todo\n * --update-refs\n * --root when used without --onto\n 7: 74a216bf0c0 = 8: 01808cf84a8 rebase: fix formatting of rebase --reapply-cherry-picks option in docs\n 8: a8adf7fda61 = 9: f646abee524 rebase: put rebase_options initialization in single place\n 9: 5cb00e5103b = 10: 328f5a1d534 rebase: provide better error message for apply options vs. merge config\n\n-- \ngitgitgadget\n"},{"id":"471007","messageId":"8a676e6ec1a30a0220ca0d222211a4cdbf0646a2.1674619434.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"[PATCH v5 01/10] rebase: mark --update-refs as requiring the merge backend","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T04:03:45Z","receivedAt":"2023-01-25T04:04:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--update-refs is built in terms of the sequencer, which requires the\nmerge backend. It was already marked as incompatible with the apply\nbackend in the git-rebase manual, but the code didn't check for this\nincompatibility and warn the user. Check and error now.\n\nWhile at it, fix a typo in t3422...and fix some misleading wording\n(most options which used to be am-specific have since been implemented\nin the merge backend as well).\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 2 ++\n builtin/rebase.c | 3 +++\n t/t3422-rebase-incompatible-options.sh | 15 ++++++++++-----\n 3 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex d811c1cf443..6490bc96a15 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -630,6 +630,8 @@ start would be overridden by the presence of\n +\n If the configuration variable `rebase.updateRefs` is set, then this option\n can be used to override and disable this setting.\n++\n+See also INCOMPATIBLE OPTIONS below.\n \n INCOMPATIBLE OPTIONS\n --------------------\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex a26cc0cfdb5..c111b89e137 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1492,6 +1492,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n+\tif (options.update_refs)\n+\t\timply_merge(&options, \"--update-refs\");\n+\n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\n \t\t\timply_merge(&options, \"--merge\");\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 6dabb05a2ad..9da39cd91c2 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -25,11 +25,11 @@ test_expect_success 'setup' '\n '\n \n #\n-# Rebase has lots of useful options like --whitepsace=fix, which are\n-# actually all built in terms of flags to git-am. Since neither\n-# --merge nor --interactive (nor any options that imply those two) use\n-# git-am, using them together will result in flags like --whitespace=fix\n-# being ignored. Make sure rebase warns the user and aborts instead.\n+# Rebase has a couple options which are specific to the apply backend,\n+# and several options which are specific to the merge backend. Flags\n+# from the different sets cannot work together, and we do not want to\n+# just ignore one of the sets of flags. Make sure rebase warns the\n+# user and aborts instead.\n #\n \n test_rebase_am_only () {\n@@ -60,6 +60,11 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --exec 'true' A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --update-refs\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --update-refs A\n+\t\"\n+\n }\n \n test_rebase_am_only --whitespace=fix\n-- \ngitgitgadget\n\n"},{"id":"471008","messageId":"cc129b8718594a98557d21b5c79369af8935d404.1674619434.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"[PATCH v5 02/10] rebase: flag --apply and --merge as incompatible","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T04:03:46Z","receivedAt":"2023-01-25T04:04:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nPreviously, we flagged options which implied --apply as being\nincompatible with options which implied --merge. But if both options\nwere given explicitly, then we didn't flag the incompatibility. The\nsame is true with --apply and --interactive. Add the check, and add\nsome testcases to verify these are also caught.\n\nSigned-off-by: Elijah Newren \n---\n builtin/rebase.c | 12 ++++++++++--\n t/t3422-rebase-incompatible-options.sh | 3 +++\n 2 files changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex c111b89e137..b742cc8fb5c 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -907,6 +907,9 @@ static int parse_opt_am(const struct option *opt, const char *arg, int unset)\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n \n+\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_APPLY)\n+\t die(_(\"apply options and merge options cannot be used together\"));\n+\n \topts->type = REBASE_APPLY;\n \n \treturn 0;\n@@ -920,8 +923,10 @@ static int parse_opt_merge(const struct option *opt, const char *arg, int unset)\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n \n-\tif (!is_merge(opts))\n-\t\topts->type = REBASE_MERGE;\n+\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)\n+\t die(_(\"apply options and merge options cannot be used together\"));\n+\n+\topts->type = REBASE_MERGE;\n \n \treturn 0;\n }\n@@ -935,6 +940,9 @@ static int parse_opt_interactive(const struct option *opt, const char *arg,\n \tBUG_ON_OPT_NEG(unset);\n \tBUG_ON_OPT_ARG(arg);\n \n+\tif (opts->type != REBASE_UNSPECIFIED && opts->type != REBASE_MERGE)\n+\t die(_(\"apply options and merge options cannot be used together\"));\n+\n \topts->type = REBASE_MERGE;\n \topts->flags |= REBASE_INTERACTIVE_EXPLICIT;\n \ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 9da39cd91c2..9b9e78479f6 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -67,7 +67,10 @@ test_rebase_am_only () {\n \n }\n \n+# Check options which imply --apply\n test_rebase_am_only --whitespace=fix\n test_rebase_am_only -C4\n+# Also check an explicit --apply\n+test_rebase_am_only --apply\n \n test_done\n-- \ngitgitgadget\n\n"},{"id":"471009","messageId":"9464bbbe9ba60927a3c36ac9685545447744819c.1674619434.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"[PATCH v5 03/10] rebase: remove --allow-empty-message from incompatible opts","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T04:03:47Z","receivedAt":"2023-01-25T04:04:15Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--allow-empty-message was turned into a no-op and even documented\nas such; the flag is simply ignored. Since the flag is ignored, it\nshouldn't be documented as being incompatible with other flags.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 6490bc96a15..7d01d1412d1 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -647,7 +647,6 @@ are incompatible with the following options:\n * --merge\n * --strategy\n * --strategy-option\n- * --allow-empty-message\n * --[no-]autosquash\n * --rebase-merges\n * --interactive\n-- \ngitgitgadget\n\n"},{"id":"471010","messageId":"b702f15ed7c2c48dc0f502ed0d7d6491c2056408.1674619434.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"[PATCH v5 04/10] rebase: fix docs about incompatibilities with --root","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T04:03:48Z","receivedAt":"2023-01-25T04:04:17Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nIn commit 5dacd4abdd (\"git-rebase.txt: document incompatible options\",\n2018-06-25), I added notes about incompatibilities between options for\nthe apply and merge backends. Unfortunately, I inverted the condition\nwhen --root was incompatible with the apply backend. Fix the\ndocumentation, and add a testcase that verifies the documentation\nmatches the code.\n\nWhile at it, the documentation for --root also tried to cover some of\nthe backend differences between the apply and merge backends in relation\nto reapplying cherry picks. The information:\n * assumed that the apply backend was the default (it isn't anymore)\n * was written before --reapply-cherry-picks became an option\n * was written before the detailed information on backend differences\nAll of these factors make the sentence under --root about reapplying\ncherry picks contradict information that is now available elsewhere in\nthe manual, and the other references are correct. So just strike this\nsentence.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 7 ++-----\n t/t3422-rebase-incompatible-options.sh | 4 ++++\n 2 files changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 7d01d1412d1..846aeed1b69 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -574,10 +574,7 @@ See also INCOMPATIBLE OPTIONS below.\n --root::\n \tRebase all commits reachable from ``, instead of\n \tlimiting them with an ``. This allows you to rebase\n-\tthe root commit(s) on a branch. When used with `--onto`, it\n-\twill skip changes already contained in `` (instead of\n-\t``) whereas without `--onto` it will operate on every\n-\tchange.\n+\tthe root commit(s) on a branch.\n +\n See also INCOMPATIBLE OPTIONS below.\n \n@@ -656,7 +653,7 @@ are incompatible with the following options:\n * --reapply-cherry-picks\n * --edit-todo\n * --update-refs\n- * --root when used in combination with --onto\n+ * --root when used without --onto\n \n In addition, the following pairs of options are incompatible:\n \ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 9b9e78479f6..f86274990b0 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -65,6 +65,10 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --update-refs A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --root without --onto\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --root A\n+\t\"\n }\n \n # Check options which imply --apply\n-- \ngitgitgadget\n\n"},{"id":"471011","messageId":"3a8429f3d2b72d59d0a071d83f6fd9effe6824ed.1674619434.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"[PATCH v5 05/10] rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T04:03:49Z","receivedAt":"2023-01-25T04:04:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--[no-]reapply-cherry-picks was traditionally only supported by the\nsequencer. Support was added for the apply backend, when --keep-base is\nalso specified, in commit ce5238a690 (\"rebase --keep-base: imply\n--reapply-cherry-picks\", 2022-10-17). Make the code error out when\n--[no-]reapply-cherry-picks is specified AND the apply backend is used\nAND --keep-base is not specified. Also, clarify a number of comments\nsurrounding the interaction of these flags.\n\nHelped-by: Phillip Wood \nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 2 +-\n builtin/rebase.c | 34 ++++++++++++++++----------\n t/t3422-rebase-incompatible-options.sh | 10 ++++++++\n 3 files changed, 32 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 846aeed1b69..8a09f12d897 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -650,7 +650,7 @@ are incompatible with the following options:\n * --exec\n * --no-keep-empty\n * --empty=\n- * --reapply-cherry-picks\n+ * --[no-]reapply-cherry-picks when used without --keep-base\n * --edit-todo\n * --update-refs\n * --root when used without --onto\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b742cc8fb5c..05b130bfae5 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1224,13 +1224,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tif (options.fork_point < 0)\n \t\t\toptions.fork_point = 0;\n \t}\n-\t/*\n-\t * --keep-base defaults to --reapply-cherry-picks to avoid losing\n-\t * commits when using this option.\n-\t */\n-\tif (options.reapply_cherry_picks < 0)\n-\t\toptions.reapply_cherry_picks = keep_base;\n-\n \tif (options.root && options.fork_point > 0)\n \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--root\", \"--fork-point\");\n \n@@ -1406,12 +1399,27 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.empty != EMPTY_UNSPECIFIED)\n \t\timply_merge(&options, \"--empty\");\n \n-\t/*\n-\t * --keep-base implements --reapply-cherry-picks by altering upstream so\n-\t * it works with both backends.\n-\t */\n-\tif (options.reapply_cherry_picks && !keep_base)\n-\t\timply_merge(&options, \"--reapply-cherry-picks\");\n+\tif (options.reapply_cherry_picks < 0)\n+\t\t/*\n+\t\t * We default to --no-reapply-cherry-picks unless\n+\t\t * --keep-base is given; when --keep-base is given, we want\n+\t\t * to default to --reapply-cherry-picks.\n+\t\t */\n+\t\toptions.reapply_cherry_picks = keep_base;\n+\telse if (!keep_base)\n+\t\t/*\n+\t\t * The apply backend always searches for and drops cherry\n+\t\t * picks. This is often not wanted with --keep-base, so\n+\t\t * --keep-base allows --reapply-cherry-picks to be\n+\t\t * simulated by altering the upstream such that\n+\t\t * cherry-picks cannot be detected and thus all commits are\n+\t\t * reapplied. Thus, --[no-]reapply-cherry-picks is\n+\t\t * supported when --keep-base is specified, but not when\n+\t\t * --keep-base is left out.\n+\t\t */\n+\t\timply_merge(&options, options.reapply_cherry_picks ?\n+\t\t\t\t\t \"--reapply-cherry-picks\" :\n+\t\t\t\t\t \"--no-reapply-cherry-picks\");\n \n \tif (gpg_sign)\n \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex f86274990b0..826f3b94ae6 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -60,6 +60,16 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --exec 'true' A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --no-reapply-cherry-picks\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n+\t\"\n+\n+\ttest_expect_success \"$opt incompatible with --reapply-cherry-picks\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --reapply-cherry-picks A\n+\t\"\n+\n \ttest_expect_success \"$opt incompatible with --update-refs\" \"\n \t\tgit checkout B^0 &&\n \t\ttest_must_fail git rebase $opt --update-refs A\n-- \ngitgitgadget\n\n"},{"id":"471012","messageId":"01808cf84a887b8ec3cb7e3fd01e7ea363a1d0c9.1674619434.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"[PATCH v5 08/10] rebase: fix formatting of rebase --reapply-cherry-picks option in docs","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T04:03:52Z","receivedAt":"2023-01-25T04:04:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nCommit ce5238a690 (\"rebase --keep-base: imply --reapply-cherry-picks\",\n2022-10-17) accidentally added some blank lines that cause extra\nparagraphs about --reapply-cherry-picks to be considered not part of\nthe documentation of that option. Remove the blank lines to make it\nclear we are still discussing --reapply-cherry-picks.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex d2b731bd605..99aadac28ef 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -338,7 +338,6 @@ See also INCOMPATIBLE OPTIONS below.\n \tupstream changes, the behavior towards them is controlled by\n \tthe `--empty` flag.)\n +\n-\n In the absence of `--keep-base` (or if `--no-reapply-cherry-picks` is\n given), these commits will be automatically dropped. Because this\n necessitates reading all upstream commits, this can be expensive in\n@@ -347,7 +346,6 @@ read. When using the 'merge' backend, warnings will be issued for each\n dropped commit (unless `--quiet` is given). Advice will also be issued\n unless `advice.skippedCherryPicks` is set to false (see\n linkgit:git-config[1]).\n-\n +\n `--reapply-cherry-picks` allows rebase to forgo reading all upstream\n commits, potentially improving performance.\n-- \ngitgitgadget\n\n"},{"id":"471013","messageId":"2777dd2788a9bbb4687cb236642c37a2f538858f.1674619434.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"[PATCH v5 06/10] rebase: add coverage of other incompatible options","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T04:03:50Z","receivedAt":"2023-01-25T04:04:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThe git-rebase manual noted several sets of incompatible options, but\nwe were missing tests for a few of these. Further, we were missing\ncode checks for one of these, which could result in command line\noptions being silently ignored.\n\nAlso, note that adding a check for autosquash means that using\n--whitespace=fix together with the config setting rebase.autosquash=true\nwill trigger an error. A subsequent commit will improve the error\nmessage.\n\nSigned-off-by: Elijah Newren \n---\n builtin/rebase.c | 3 +++\n t/t3422-rebase-incompatible-options.sh | 15 +++++++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 05b130bfae5..d6b20a6a536 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1511,6 +1511,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif (options.update_refs)\n \t\timply_merge(&options, \"--update-refs\");\n \n+\tif (options.autosquash)\n+\t\timply_merge(&options, \"--autosquash\");\n+\n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\n \t\t\timply_merge(&options, \"--merge\");\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 826f3b94ae6..6a17b571ec7 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -50,6 +50,11 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --strategy-option=ours A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --autosquash\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --autosquash A\n+\t\"\n+\n \ttest_expect_success \"$opt incompatible with --interactive\" \"\n \t\tgit checkout B^0 &&\n \t\ttest_must_fail git rebase $opt --interactive A\n@@ -60,6 +65,16 @@ test_rebase_am_only () {\n \t\ttest_must_fail git rebase $opt --exec 'true' A\n \t\"\n \n+\ttest_expect_success \"$opt incompatible with --keep-empty\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --keep-empty A\n+\t\"\n+\n+\ttest_expect_success \"$opt incompatible with --empty=...\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git rebase $opt --empty=ask A\n+\t\"\n+\n \ttest_expect_success \"$opt incompatible with --no-reapply-cherry-picks\" \"\n \t\tgit checkout B^0 &&\n \t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n-- \ngitgitgadget\n\n"},{"id":"471014","messageId":"0d0541ea243faaf8ae9df80f7caf64a8e7f80292.1674619434.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"[PATCH v5 07/10] rebase: clarify the OPT_CMDMODE incompatibilities","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T04:03:51Z","receivedAt":"2023-01-25T04:04:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\n--edit-todo was documented as being incompatible with any of the options\nfor the apply backend. However, it is also incompatible with any of the\noptions for the merge backend, and is incompatible with any options that\nare not backend specific as well. The same can be said for --continue,\n--skip, --abort, --quit, etc.\n\nThis is already somewhat implicitly covered by the synopsis, but since\n\"[]\" in the first two variants are vague it might be easy to\nmiss this. That might not be a big deal, but since the rebase manpage\nhas to spend so much verbiage about incompatibility of options, making\na separate section for these options that are incompatible with\neverything else seems clearer. Do that, and remove the needless\ninclusion of --edit-todo in the explicit incompatibility list.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 61 +++++++++++++++++++-----------------\n 1 file changed, 33 insertions(+), 28 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 8a09f12d897..d2b731bd605 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -208,6 +208,39 @@ Alternatively, you can undo the 'git rebase' with\n \n git rebase --abort\n \n+MODE OPTIONS\n+------------\n+\n+The options in this section cannot be used with any other option,\n+including not with each other:\n+\n+--continue::\n+\tRestart the rebasing process after having resolved a merge conflict.\n+\n+--skip::\n+\tRestart the rebasing process by skipping the current patch.\n+\n+--abort::\n+\tAbort the rebase operation and reset HEAD to the original\n+\tbranch. If `` was provided when the rebase operation was\n+\tstarted, then `HEAD` will be reset to ``. Otherwise `HEAD`\n+\twill be reset to where it was when the rebase operation was\n+\tstarted.\n+\n+--quit::\n+\tAbort the rebase operation but `HEAD` is not reset back to the\n+\toriginal branch. The index and working tree are also left\n+\tunchanged as a result. If a temporary stash entry was created\n+\tusing `--autostash`, it will be saved to the stash list.\n+\n+--edit-todo::\n+\tEdit the todo list during an interactive rebase.\n+\n+--show-current-patch::\n+\tShow the current patch in an interactive rebase or when rebase\n+\tis stopped because of conflicts. This is the equivalent of\n+\t`git show REBASE_HEAD`.\n+\n OPTIONS\n -------\n --onto ::\n@@ -249,22 +282,6 @@ See also INCOMPATIBLE OPTIONS below.\n ::\n \tWorking branch; defaults to `HEAD`.\n \n---continue::\n-\tRestart the rebasing process after having resolved a merge conflict.\n-\n---abort::\n-\tAbort the rebase operation and reset HEAD to the original\n-\tbranch. If `` was provided when the rebase operation was\n-\tstarted, then `HEAD` will be reset to ``. Otherwise `HEAD`\n-\twill be reset to where it was when the rebase operation was\n-\tstarted.\n-\n---quit::\n-\tAbort the rebase operation but `HEAD` is not reset back to the\n-\toriginal branch. The index and working tree are also left\n-\tunchanged as a result. If a temporary stash entry was created\n-\tusing `--autostash`, it will be saved to the stash list.\n-\n --apply::\n \tUse applying strategies to rebase (calling `git-am`\n \tinternally). This option may become a no-op in the future\n@@ -345,17 +362,6 @@ See also INCOMPATIBLE OPTIONS below.\n +\n See also INCOMPATIBLE OPTIONS below.\n \n---skip::\n-\tRestart the rebasing process by skipping the current patch.\n-\n---edit-todo::\n-\tEdit the todo list during an interactive rebase.\n-\n---show-current-patch::\n-\tShow the current patch in an interactive rebase or when rebase\n-\tis stopped because of conflicts. This is the equivalent of\n-\t`git show REBASE_HEAD`.\n-\n -m::\n --merge::\n \tUsing merging strategies to rebase (default).\n@@ -651,7 +657,6 @@ are incompatible with the following options:\n * --no-keep-empty\n * --empty=\n * --[no-]reapply-cherry-picks when used without --keep-base\n- * --edit-todo\n * --update-refs\n * --root when used without --onto\n \n-- \ngitgitgadget\n\n"},{"id":"471015","messageId":"328f5a1d534dca2998785d1fad555e3645bc2c81.1674619434.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"[PATCH v5 10/10] rebase: provide better error message for apply options vs. merge config","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T04:03:54Z","receivedAt":"2023-01-25T04:04:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nWhen config which selects the merge backend (currently,\nrebase.autosquash=true or rebase.updateRefs=true) conflicts with other\noptions on the command line (such as --whitespace=fix), make the error\nmessage specifically call out the config option and specify how to\noverride that config option on the command line.\n\nSigned-off-by: Elijah Newren \n---\n Documentation/git-rebase.txt | 2 +-\n builtin/rebase.c | 27 ++++++++++++++++++++------\n t/t3422-rebase-incompatible-options.sh | 24 +++++++++++++++++++++++\n 3 files changed, 46 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\nindex 99aadac28ef..9a295bcee45 100644\n--- a/Documentation/git-rebase.txt\n+++ b/Documentation/git-rebase.txt\n@@ -648,7 +648,7 @@ are incompatible with the following options:\n * --merge\n * --strategy\n * --strategy-option\n- * --[no-]autosquash\n+ * --autosquash\n * --rebase-merges\n * --interactive\n * --exec\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex ad5ebecbbdb..7171be40eeb 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -122,6 +122,8 @@ struct rebase_options {\n \tint reapply_cherry_picks;\n \tint fork_point;\n \tint update_refs;\n+\tint config_autosquash;\n+\tint config_update_refs;\n };\n \n #define REBASE_OPTIONS_INIT {\t\t\t \t\\\n@@ -136,6 +138,10 @@ struct rebase_options {\n \t\t.fork_point = -1,\t\t\t\\\n \t\t.reapply_cherry_picks = -1, \\\n \t\t.allow_empty_message = 1, \\\n+\t\t.autosquash = -1, \\\n+\t\t.config_autosquash = -1, \\\n+\t\t.update_refs = -1, \\\n+\t\t.config_update_refs = -1, \\\n \t}\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n@@ -778,7 +784,7 @@ static int rebase_config(const char *var, const char *value, void *data)\n \t}\n \n \tif (!strcmp(var, \"rebase.autosquash\")) {\n-\t\topts->autosquash = git_config_bool(var, value);\n+\t\topts->config_autosquash = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -795,7 +801,7 @@ static int rebase_config(const char *var, const char *value, void *data)\n \t}\n \n \tif (!strcmp(var, \"rebase.updaterefs\")) {\n-\t\topts->update_refs = git_config_bool(var, value);\n+\t\topts->config_update_refs = git_config_bool(var, value);\n \t\treturn 0;\n \t}\n \n@@ -1366,7 +1372,8 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tif ((options.flags & REBASE_INTERACTIVE_EXPLICIT) ||\n \t (options.action != ACTION_NONE) ||\n \t (options.exec.nr > 0) ||\n-\t options.autosquash) {\n+\t (options.autosquash == -1 && options.config_autosquash == 1) ||\n+\t options.autosquash == 1) {\n \t\tallow_preemptive_ff = 0;\n \t}\n \tif (options.committer_date_is_author_date || options.ignore_date)\n@@ -1499,20 +1506,28 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\t\tif (strcmp(options.git_am_opts.v[i], \"-q\"))\n \t\t\t\tbreak;\n \n-\t\tif (i >= 0) {\n+\t\tif (i >= 0 || options.type == REBASE_APPLY) {\n \t\t\tif (is_merge(&options))\n \t\t\t\tdie(_(\"apply options and merge options \"\n \t\t\t\t\t \"cannot be used together\"));\n+\t\t\telse if (options.autosquash == -1 && options.config_autosquash == 1)\n+\t\t\t\tdie(_(\"apply options are incompatible with rebase.autosquash. Consider adding --no-autosquash\"));\n+\t\t\telse if (options.update_refs == -1 && options.config_update_refs == 1)\n+\t\t\t\tdie(_(\"apply options are incompatible with rebase.updateRefs. Consider adding --no-update-refs\"));\n \t\t\telse\n \t\t\t\toptions.type = REBASE_APPLY;\n \t\t}\n \t}\n \n-\tif (options.update_refs)\n+\tif (options.update_refs == 1)\n \t\timply_merge(&options, \"--update-refs\");\n+\toptions.update_refs = (options.update_refs >= 0) ? options.update_refs :\n+\t\t\t ((options.config_update_refs >= 0) ? options.config_update_refs : 0);\n \n-\tif (options.autosquash)\n+\tif (options.autosquash == 1)\n \t\timply_merge(&options, \"--autosquash\");\n+\toptions.autosquash = (options.autosquash >= 0) ? options.autosquash :\n+\t\t\t ((options.config_autosquash >= 0) ? options.config_autosquash : 0);\n \n \tif (options.type == REBASE_UNSPECIFIED) {\n \t\tif (!strcmp(options.default_backend, \"merge\"))\ndiff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\nindex 6a17b571ec7..4711b37a288 100755\n--- a/t/t3422-rebase-incompatible-options.sh\n+++ b/t/t3422-rebase-incompatible-options.sh\n@@ -94,6 +94,30 @@ test_rebase_am_only () {\n \t\tgit checkout B^0 &&\n \t\ttest_must_fail git rebase $opt --root A\n \t\"\n+\n+\ttest_expect_success \"$opt incompatible with rebase.autosquash\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git -c rebase.autosquash=true rebase $opt A 2>err &&\n+\t\tgrep -e --no-autosquash err\n+\t\"\n+\n+\ttest_expect_success \"$opt incompatible with rebase.updateRefs\" \"\n+\t\tgit checkout B^0 &&\n+\t\ttest_must_fail git -c rebase.updateRefs=true rebase $opt A 2>err &&\n+\t\tgrep -e --no-update-refs err\n+\t\"\n+\n+\ttest_expect_success \"$opt okay with overridden rebase.autosquash\" \"\n+\t\ttest_when_finished \\\"git reset --hard B^0\\\" &&\n+\t\tgit checkout B^0 &&\n+\t\tgit -c rebase.autosquash=true rebase --no-autosquash $opt A\n+\t\"\n+\n+\ttest_expect_success \"$opt okay with overridden rebase.updateRefs\" \"\n+\t\ttest_when_finished \\\"git reset --hard B^0\\\" &&\n+\t\tgit checkout B^0 &&\n+\t\tgit -c rebase.updateRefs=true rebase --no-update-refs $opt A\n+\t\"\n }\n \n # Check options which imply --apply\n-- \ngitgitgadget\n"},{"id":"471016","messageId":"f646abee52400c3bb6ca0b2c32f712547b3c9df3.1674619434.git.gitgitgadget@gmail.com","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"[PATCH v5 09/10] rebase: put rebase_options initialization in single place","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-01-25T04:03:53Z","receivedAt":"2023-01-25T04:04:38Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nSigned-off-by: Elijah Newren \n---\n builtin/rebase.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex d6b20a6a536..ad5ebecbbdb 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -134,6 +134,8 @@ struct rebase_options {\n \t\t.exec = STRING_LIST_INIT_NODUP,\t\t\\\n \t\t.git_format_patch_opt = STRBUF_INIT,\t\\\n \t\t.fork_point = -1,\t\t\t\\\n+\t\t.reapply_cherry_picks = -1, \\\n+\t\t.allow_empty_message = 1, \\\n \t}\n \n static struct replay_opts get_replay_opts(const struct rebase_options *opts)\n@@ -1158,8 +1160,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\n \n-\toptions.reapply_cherry_picks = -1;\n-\toptions.allow_empty_message = 1;\n \tgit_config(rebase_config, &options);\n \t/* options.gpg_sign_opt will be either \"-S\" or NULL */\n \tgpg_sign = options.gpg_sign_opt ? \"\" : NULL;\n-- \ngitgitgadget\n\n"},{"id":"471021","messageId":"94deff87-a4a0-e937-7549-3e5348361a12@dunelm.org.uk","threadId":"59117","inReplyTo":"3a8429f3d2b72d59d0a071d83f6fd9effe6824ed.1674619434.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 05/10] rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-25T14:14:54Z","receivedAt":"2023-01-25T14:16:13Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 25/01/2023 04:03, Elijah Newren via GitGitGadget wrote:\n> From: Elijah Newren \n> \n> --[no-]reapply-cherry-picks was traditionally only supported by the\n> sequencer. Support was added for the apply backend, when --keep-base is\n> also specified, in commit ce5238a690 (\"rebase --keep-base: imply\n> --reapply-cherry-picks\", 2022-10-17). Make the code error out when\n> --[no-]reapply-cherry-picks is specified AND the apply backend is used\n> AND --keep-base is not specified. Also, clarify a number of comments\n> surrounding the interaction of these flags.\n\nThis looks great to me. Sorry it took me so long to realize that the \napply backend should be erroring out on --no-reapply-cherry-picks \nwithout --keep-base. Thanks for taking the time to update the comments, \nhopefully they'll be more helpful to future readers that the current \nones have proved to be.\n\nBest Wishes\n\nPhillip\n\n> Helped-by: Phillip Wood \n> Signed-off-by: Elijah Newren \n> ---\n> Documentation/git-rebase.txt | 2 +-\n> builtin/rebase.c | 34 ++++++++++++++++----------\n> t/t3422-rebase-incompatible-options.sh | 10 ++++++++\n> 3 files changed, 32 insertions(+), 14 deletions(-)\n> \n> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt\n> index 846aeed1b69..8a09f12d897 100644\n> --- a/Documentation/git-rebase.txt\n> +++ b/Documentation/git-rebase.txt\n> @@ -650,7 +650,7 @@ are incompatible with the following options:\n> * --exec\n> * --no-keep-empty\n> * --empty=\n> - * --reapply-cherry-picks\n> + * --[no-]reapply-cherry-picks when used without --keep-base\n> * --edit-todo\n> * --update-refs\n> * --root when used without --onto\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index b742cc8fb5c..05b130bfae5 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1224,13 +1224,6 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \t\tif (options.fork_point < 0)\n> \t\t\toptions.fork_point = 0;\n> \t}\n> -\t/*\n> -\t * --keep-base defaults to --reapply-cherry-picks to avoid losing\n> -\t * commits when using this option.\n> -\t */\n> -\tif (options.reapply_cherry_picks < 0)\n> -\t\toptions.reapply_cherry_picks = keep_base;\n> -\n> \tif (options.root && options.fork_point > 0)\n> \t\tdie(_(\"options '%s' and '%s' cannot be used together\"), \"--root\", \"--fork-point\");\n> \n> @@ -1406,12 +1399,27 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tif (options.empty != EMPTY_UNSPECIFIED)\n> \t\timply_merge(&options, \"--empty\");\n> \n> -\t/*\n> -\t * --keep-base implements --reapply-cherry-picks by altering upstream so\n> -\t * it works with both backends.\n> -\t */\n> -\tif (options.reapply_cherry_picks && !keep_base)\n> -\t\timply_merge(&options, \"--reapply-cherry-picks\");\n> +\tif (options.reapply_cherry_picks < 0)\n> +\t\t/*\n> +\t\t * We default to --no-reapply-cherry-picks unless\n> +\t\t * --keep-base is given; when --keep-base is given, we want\n> +\t\t * to default to --reapply-cherry-picks.\n> +\t\t */\n> +\t\toptions.reapply_cherry_picks = keep_base;\n> +\telse if (!keep_base)\n> +\t\t/*\n> +\t\t * The apply backend always searches for and drops cherry\n> +\t\t * picks. This is often not wanted with --keep-base, so\n> +\t\t * --keep-base allows --reapply-cherry-picks to be\n> +\t\t * simulated by altering the upstream such that\n> +\t\t * cherry-picks cannot be detected and thus all commits are\n> +\t\t * reapplied. Thus, --[no-]reapply-cherry-picks is\n> +\t\t * supported when --keep-base is specified, but not when\n> +\t\t * --keep-base is left out.\n> +\t\t */\n> +\t\timply_merge(&options, options.reapply_cherry_picks ?\n> +\t\t\t\t\t \"--reapply-cherry-picks\" :\n> +\t\t\t\t\t \"--no-reapply-cherry-picks\");\n> \n> \tif (gpg_sign)\n> \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n> diff --git a/t/t3422-rebase-incompatible-options.sh b/t/t3422-rebase-incompatible-options.sh\n> index f86274990b0..826f3b94ae6 100755\n> --- a/t/t3422-rebase-incompatible-options.sh\n> +++ b/t/t3422-rebase-incompatible-options.sh\n> @@ -60,6 +60,16 @@ test_rebase_am_only () {\n> \t\ttest_must_fail git rebase $opt --exec 'true' A\n> \t\"\n> \n> +\ttest_expect_success \"$opt incompatible with --no-reapply-cherry-picks\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n> +\t\"\n> +\n> +\ttest_expect_success \"$opt incompatible with --reapply-cherry-picks\" \"\n> +\t\tgit checkout B^0 &&\n> +\t\ttest_must_fail git rebase $opt --reapply-cherry-picks A\n> +\t\"\n> +\n> \ttest_expect_success \"$opt incompatible with --update-refs\" \"\n> \t\tgit checkout B^0 &&\n> \t\ttest_must_fail git rebase $opt --update-refs A\n"},{"id":"471022","messageId":"d82b463e-1d94-64fa-a90f-0a4d919ca27c@dunelm.org.uk","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2023-01-25T14:17:12Z","receivedAt":"2023-01-25T14:17:31Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Elijah\n\nOn 25/01/2023 04:03, Elijah Newren via GitGitGadget wrote:\n> We had a report about --update-refs being ignored when --whitespace=fix was\n> passed, confusing an end user. These were already marked as incompatible in\n> the manual, but the code didn't check for the incompatibility and provide an\n> error to the user.\n> \n> Folks brought up other flags tangentially when reviewing an earlier round of\n> this series, and I found we had more that were missing checks, and that we\n> were missing some testcases, and that the documentation was wrong on some of\n> the relevant options. So, I ended up getting lots of little fixes to\n> straighten these all out.\n\nThis version looks good to me. Thanks for working on it - it turned out \nto be quite a bit of work in the end but it is nice improvement, \nespecially the last patch.\n\nBest Wishes\n\nPhillip\n\n\n> Changes since v4:\n> \n> * Pulled out the changes regarding incompatibility detection for\n> --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot\n> with understanding the behavior, suggesting changes, and getting the\n> wording right, and I think it deserves its own patch with its own\n> explanation.\n> \n> Changes since v3:\n> \n> * Corrected the code surrounding --[no-]reapply-cherry-picks, and extended\n> the testcases (Thanks to Phillip for pointing out my error)\n> * I went ahead and implemented the better error message when the merge\n> backend is triggered solely via config options.\n> \n> Changes since v2:\n> \n> * Remove the extra patch and reword the comment in t3422 more thoroughly.\n> * Add tests for other flag incompatibilities that were missing\n> * Add code that was missing for checking various flag incompatibilities\n> * Fix incorrect claims in the documentation around incompatible options\n> * A few other miscellaneous fixups noticed while doing all the above\n> \n> Changes since v1:\n> \n> * Add a patch nuking the -C option to rebase (fixes confusion around the\n> comment in t3422 and acknowledges the fact that the option is totally and\n> utterly useless and always has been. It literally never affects the\n> results of a rebase.)\n> \n> Signed-off-by: Elijah Newren newren@gmail.com\n> \n> Elijah Newren (10):\n> rebase: mark --update-refs as requiring the merge backend\n> rebase: flag --apply and --merge as incompatible\n> rebase: remove --allow-empty-message from incompatible opts\n> rebase: fix docs about incompatibilities with --root\n> rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks\n> rebase: add coverage of other incompatible options\n> rebase: clarify the OPT_CMDMODE incompatibilities\n> rebase: fix formatting of rebase --reapply-cherry-picks option in docs\n> rebase: put rebase_options initialization in single place\n> rebase: provide better error message for apply options vs. merge\n> config\n> \n> Documentation/git-rebase.txt | 77 ++++++++++++-------------\n> builtin/rebase.c | 79 +++++++++++++++++++-------\n> t/t3422-rebase-incompatible-options.sh | 71 +++++++++++++++++++++--\n> 3 files changed, 163 insertions(+), 64 deletions(-)\n> \n> \n> base-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2\n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v5\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v5\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1466\n> \n> Range-diff vs v4:\n> \n> 1: 8a676e6ec1a = 1: 8a676e6ec1a rebase: mark --update-refs as requiring the merge backend\n> 2: cc129b87185 = 2: cc129b87185 rebase: flag --apply and --merge as incompatible\n> 3: 9464bbbe9ba = 3: 9464bbbe9ba rebase: remove --allow-empty-message from incompatible opts\n> 4: b702f15ed7c = 4: b702f15ed7c rebase: fix docs about incompatibilities with --root\n> -: ----------- > 5: 3a8429f3d2b rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks\n> 5: 5e4851e611e ! 6: 2777dd2788a rebase: add coverage of other incompatible options\n> @@ Commit message\n> \n> The git-rebase manual noted several sets of incompatible options, but\n> we were missing tests for a few of these. Further, we were missing\n> - code checks for some of these, which could result in command line\n> + code checks for one of these, which could result in command line\n> options being silently ignored.\n> \n> Also, note that adding a check for autosquash means that using\n> @@ Commit message\n> \n> Signed-off-by: Elijah Newren \n> \n> - ## Documentation/git-rebase.txt ##\n> -@@ Documentation/git-rebase.txt: are incompatible with the following options:\n> - * --exec\n> - * --no-keep-empty\n> - * --empty=\n> -- * --reapply-cherry-picks\n> -+ * --[no-]reapply-cherry-picks\n> - * --edit-todo\n> - * --update-refs\n> - * --root when used without --onto\n> -\n> ## builtin/rebase.c ##\n> -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n> - \t\tif (options.fork_point < 0)\n> - \t\t\toptions.fork_point = 0;\n> - \t}\n> -+\t/*\n> -+\t * The apply backend does not support --[no-]reapply-cherry-picks.\n> -+\t * The behavior it implements by default is equivalent to\n> -+\t * --no-reapply-cherry-picks (due to passing --cherry-picks to\n> -+\t * format-patch), but --keep-base alters the upstream such that no\n> -+\t * cherry-picks can be found (effectively making it act like\n> -+\t * --reapply-cherry-picks).\n> -+\t *\n> -+\t * Now, if the user does specify --[no-]reapply-cherry-picks, but\n> -+\t * does so in such a way that options.reapply_cherry_picks ==\n> -+\t * keep_base, then the behavior they get will match what they\n> -+\t * expect despite options.reapply_cherry_picks being ignored. We\n> -+\t * could just allow the flag in that case, but it seems better to\n> -+\t * just alert the user that they've specified a flag that the\n> -+\t * backend ignores.\n> -+\t */\n> -+\tif (options.reapply_cherry_picks >= 0)\n> -+\t\timply_merge(&options, options.reapply_cherry_picks ? \"--reapply-cherry-picks\" :\n> -+\t\t\t\t\t\t\t\t \"--no-reapply-cherry-picks\");\n> -+\n> - \t/*\n> - \t * --keep-base defaults to --reapply-cherry-picks to avoid losing\n> - \t * commits when using this option.\n> -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n> - \tif (options.empty != EMPTY_UNSPECIFIED)\n> - \t\timply_merge(&options, \"--empty\");\n> -\n> --\t/*\n> --\t * --keep-base implements --reapply-cherry-picks by altering upstream so\n> --\t * it works with both backends.\n> --\t */\n> --\tif (options.reapply_cherry_picks && !keep_base)\n> --\t\timply_merge(&options, \"--reapply-cherry-picks\");\n> --\n> - \tif (gpg_sign)\n> - \t\toptions.gpg_sign_opt = xstrfmt(\"-S%s\", gpg_sign);\n> -\n> @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix)\n> \tif (options.update_refs)\n> \t\timply_merge(&options, \"--update-refs\");\n> @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () {\n> +\t\ttest_must_fail git rebase $opt --empty=ask A\n> +\t\"\n> +\n> -+\ttest_expect_success \"$opt incompatible with --no-reapply-cherry-picks\" \"\n> -+\t\tgit checkout B^0 &&\n> -+\t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n> -+\t\"\n> -+\n> -+\ttest_expect_success \"$opt incompatible with --reapply-cherry-picks\" \"\n> -+\t\tgit checkout B^0 &&\n> -+\t\ttest_must_fail git rebase $opt --reapply-cherry-picks A\n> -+\t\"\n> -+\n> - \ttest_expect_success \"$opt incompatible with --update-refs\" \"\n> + \ttest_expect_success \"$opt incompatible with --no-reapply-cherry-picks\" \"\n> \t\tgit checkout B^0 &&\n> - \t\ttest_must_fail git rebase $opt --update-refs A\n> + \t\ttest_must_fail git rebase $opt --no-reapply-cherry-picks A\n> 6: 21ae9e05313 ! 7: 0d0541ea243 rebase: clarify the OPT_CMDMODE incompatibilities\n> @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below.\n> @@ Documentation/git-rebase.txt: are incompatible with the following options:\n> * --no-keep-empty\n> * --empty=\n> - * --[no-]reapply-cherry-picks\n> + * --[no-]reapply-cherry-picks when used without --keep-base\n> - * --edit-todo\n> * --update-refs\n> * --root when used without --onto\n> 7: 74a216bf0c0 = 8: 01808cf84a8 rebase: fix formatting of rebase --reapply-cherry-picks option in docs\n> 8: a8adf7fda61 = 9: f646abee524 rebase: put rebase_options initialization in single place\n> 9: 5cb00e5103b = 10: 328f5a1d534 rebase: provide better error message for apply options vs. merge config\n> \n"},{"id":"471025","messageId":"xmqqa626fu0x.fsf@gitster.g","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-01-25T16:39:26Z","receivedAt":"2023-01-25T16:39:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> Changes since v4:\n>\n> * Pulled out the changes regarding incompatibility detection for\n> --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot\n> with understanding the behavior, suggesting changes, and getting the\n> wording right, and I think it deserves its own patch with its own\n> explanation.\n\nHmph, does this replace the previous round that has already been in\n'next' for two days? I do not mind reverting the merge and requeuing\nit, but just wanted ot make sure before doing anything.\n\nThanks.\n"},{"id":"471026","messageId":"CABPp-BEWC_GQXLKvokLVgd4-Hq16MoZggyp6r-sj29SYyLippQ@mail.gmail.com","threadId":"59117","inReplyTo":"xmqqa626fu0x.fsf@gitster.g","subject":"Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-01-25T16:48:39Z","receivedAt":"2023-01-25T16:49:37Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Wed, Jan 25, 2023 at 8:39 AM Junio C Hamano wrote:\n>\n> \"Elijah Newren via GitGitGadget\" writes:\n>\n> > Changes since v4:\n> >\n> > * Pulled out the changes regarding incompatibility detection for\n> > --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot\n> > with understanding the behavior, suggesting changes, and getting the\n> > wording right, and I think it deserves its own patch with its own\n> > explanation.\n>\n> Hmph, does this replace the previous round that has already been in\n> 'next' for two days? I do not mind reverting the merge and requeuing\n> it, but just wanted ot make sure before doing anything.\n\nSorry, I didn't realize it had merged to next. With Phillip's ongoing\nreviews and requests for changes, which you had even commented on\n(https://lore.kernel.org/git/xmqqpmb4j8e9.fsf@gitster.g/ and the\nthread that was in the middle of), I assumed it was still only in\nseen.\n\nBut yes, if you could revert from next and replace, that would be\ngreat. Now that v5 has Phillip's blessing (and multiple of his\ncorrections), I think it's good to merge down.\n"},{"id":"471347","messageId":"49581695-e072-ee3e-1748-f0b8112545ff@gmx.de","threadId":"59117","inReplyTo":"pull.1466.v5.git.1674619434.gitgitgadget@gmail.com","subject":"rebase --merge vs --whitespace=fix, was Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-02-02T10:29:43Z","receivedAt":"2023-02-02T10:29:52Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Elijah,\n\nOn Wed, 25 Jan 2023, Elijah Newren via GitGitGadget wrote:\n\n> We had a report about --update-refs being ignored when --whitespace=fix\n> was passed, confusing an end user. These were already marked as\n> incompatible in the manual, but the code didn't check for the\n> incompatibility and provide an error to the user.\n\nThank you for working on this!\n\nFWIW this report (and your patch series) made me wistful about that Google\nSummer of Code project that I hoped would bring about the trick to combine\n`--whitespace=fix` with interactive rebases. But in that GSoC project,\nthis goal was treated as a \"bonus feature\" and pushed so far back that we\ndid not even get to analyzing the complexity of the task, let alone any\ndetails.\n\nSo I sat down and started that analysis. The result is\nhttps://github.com/gitgitgadget/git/issues/1472 where you can see that\naddressing the incompatibility is definitely outside of trivial. It does\nseem doable, if Outreachy/GSoC project-sized.\n\nCiao,\nJohannes\n"},{"id":"471408","messageId":"CABPp-BFtxH_pNLsP1mR2ciwPt-GcpzWyvv4OJiVg+uns7FyaqA@mail.gmail.com","threadId":"59117","inReplyTo":"49581695-e072-ee3e-1748-f0b8112545ff@gmx.de","subject":"Re: rebase --merge vs --whitespace=fix, was Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2023-02-02T23:48:52Z","receivedAt":"2023-02-02T23:49:09Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Thu, Feb 2, 2023 at 2:29 AM Johannes Schindelin\n wrote:\n>\n> Hi Elijah,\n>\n> On Wed, 25 Jan 2023, Elijah Newren via GitGitGadget wrote:\n>\n> > We had a report about --update-refs being ignored when --whitespace=fix\n> > was passed, confusing an end user. These were already marked as\n> > incompatible in the manual, but the code didn't check for the\n> > incompatibility and provide an error to the user.\n>\n> Thank you for working on this!\n>\n> FWIW this report (and your patch series) made me wistful about that Google\n> Summer of Code project that I hoped would bring about the trick to combine\n> `--whitespace=fix` with interactive rebases. But in that GSoC project,\n> this goal was treated as a \"bonus feature\" and pushed so far back that we\n> did not even get to analyzing the complexity of the task, let alone any\n> details.\n>\n> So I sat down and started that analysis. The result is\n> https://github.com/gitgitgadget/git/issues/1472 where you can see that\n> addressing the incompatibility is definitely outside of trivial. It does\n> seem doable, if Outreachy/GSoC project-sized.\n\nNice! Thanks for digging in and documenting what is needed!\n"}]}