From: Phillip Wood Date: Wed, 25 Jan 2023 14:17:12 GMT Subject: Re: [PATCH v5 00/10] rebase: fix several code/testing/documentation issues around flag incompatibilities Message-ID: In-Reply-To: Hi Elijah On 25/01/2023 04:03, Elijah Newren via GitGitGadget wrote: > We had a report about --update-refs being ignored when --whitespace=fix was > passed, confusing an end user. These were already marked as incompatible in > the manual, but the code didn't check for the incompatibility and provide an > error to the user. > > Folks brought up other flags tangentially when reviewing an earlier round of > this series, and I found we had more that were missing checks, and that we > were missing some testcases, and that the documentation was wrong on some of > the relevant options. So, I ended up getting lots of little fixes to > straighten these all out. This version looks good to me. Thanks for working on it - it turned out to be quite a bit of work in the end but it is nice improvement, especially the last patch. Best Wishes Phillip > Changes since v4: > > * Pulled out the changes regarding incompatibility detection for > --[no-]reapply-cherry-picks into a separate patch. Phillip helped a lot > with understanding the behavior, suggesting changes, and getting the > wording right, and I think it deserves its own patch with its own > explanation. > > Changes since v3: > > * Corrected the code surrounding --[no-]reapply-cherry-picks, and extended > the testcases (Thanks to Phillip for pointing out my error) > * I went ahead and implemented the better error message when the merge > backend is triggered solely via config options. > > Changes since v2: > > * Remove the extra patch and reword the comment in t3422 more thoroughly. > * Add tests for other flag incompatibilities that were missing > * Add code that was missing for checking various flag incompatibilities > * Fix incorrect claims in the documentation around incompatible options > * A few other miscellaneous fixups noticed while doing all the above > > Changes since v1: > > * Add a patch nuking the -C option to rebase (fixes confusion around the > comment in t3422 and acknowledges the fact that the option is totally and > utterly useless and always has been. It literally never affects the > results of a rebase.) > > Signed-off-by: Elijah Newren newren@gmail.com > > Elijah Newren (10): > rebase: mark --update-refs as requiring the merge backend > rebase: flag --apply and --merge as incompatible > rebase: remove --allow-empty-message from incompatible opts > rebase: fix docs about incompatibilities with --root > rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks > rebase: add coverage of other incompatible options > rebase: clarify the OPT_CMDMODE incompatibilities > rebase: fix formatting of rebase --reapply-cherry-picks option in docs > rebase: put rebase_options initialization in single place > rebase: provide better error message for apply options vs. merge > config > > Documentation/git-rebase.txt | 77 ++++++++++++------------- > builtin/rebase.c | 79 +++++++++++++++++++------- > t/t3422-rebase-incompatible-options.sh | 71 +++++++++++++++++++++-- > 3 files changed, 163 insertions(+), 64 deletions(-) > > > base-commit: 56c8fb1e95377900ec9d53c07886022af0a5d3c2 > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1466%2Fnewren%2Frebase-update-refs-imply-merge-v5 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1466/newren/rebase-update-refs-imply-merge-v5 > Pull-Request: https://github.com/gitgitgadget/git/pull/1466 > > Range-diff vs v4: > > 1: 8a676e6ec1a = 1: 8a676e6ec1a rebase: mark --update-refs as requiring the merge backend > 2: cc129b87185 = 2: cc129b87185 rebase: flag --apply and --merge as incompatible > 3: 9464bbbe9ba = 3: 9464bbbe9ba rebase: remove --allow-empty-message from incompatible opts > 4: b702f15ed7c = 4: b702f15ed7c rebase: fix docs about incompatibilities with --root > -: ----------- > 5: 3a8429f3d2b rebase: fix incompatiblity checks for --[no-]reapply-cherry-picks > 5: 5e4851e611e ! 6: 2777dd2788a rebase: add coverage of other incompatible options > @@ Commit message > > The git-rebase manual noted several sets of incompatible options, but > we were missing tests for a few of these. Further, we were missing > - code checks for some of these, which could result in command line > + code checks for one of these, which could result in command line > options being silently ignored. > > Also, note that adding a check for autosquash means that using > @@ Commit message > > Signed-off-by: Elijah Newren > > - ## Documentation/git-rebase.txt ## > -@@ Documentation/git-rebase.txt: are incompatible with the following options: > - * --exec > - * --no-keep-empty > - * --empty= > -- * --reapply-cherry-picks > -+ * --[no-]reapply-cherry-picks > - * --edit-todo > - * --update-refs > - * --root when used without --onto > - > ## builtin/rebase.c ## > -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) > - if (options.fork_point < 0) > - options.fork_point = 0; > - } > -+ /* > -+ * The apply backend does not support --[no-]reapply-cherry-picks. > -+ * The behavior it implements by default is equivalent to > -+ * --no-reapply-cherry-picks (due to passing --cherry-picks to > -+ * format-patch), but --keep-base alters the upstream such that no > -+ * cherry-picks can be found (effectively making it act like > -+ * --reapply-cherry-picks). > -+ * > -+ * Now, if the user does specify --[no-]reapply-cherry-picks, but > -+ * does so in such a way that options.reapply_cherry_picks == > -+ * keep_base, then the behavior they get will match what they > -+ * expect despite options.reapply_cherry_picks being ignored. We > -+ * could just allow the flag in that case, but it seems better to > -+ * just alert the user that they've specified a flag that the > -+ * backend ignores. > -+ */ > -+ if (options.reapply_cherry_picks >= 0) > -+ imply_merge(&options, options.reapply_cherry_picks ? "--reapply-cherry-picks" : > -+ "--no-reapply-cherry-picks"); > -+ > - /* > - * --keep-base defaults to --reapply-cherry-picks to avoid losing > - * commits when using this option. > -@@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) > - if (options.empty != EMPTY_UNSPECIFIED) > - imply_merge(&options, "--empty"); > - > -- /* > -- * --keep-base implements --reapply-cherry-picks by altering upstream so > -- * it works with both backends. > -- */ > -- if (options.reapply_cherry_picks && !keep_base) > -- imply_merge(&options, "--reapply-cherry-picks"); > -- > - if (gpg_sign) > - options.gpg_sign_opt = xstrfmt("-S%s", gpg_sign); > - > @@ builtin/rebase.c: int cmd_rebase(int argc, const char **argv, const char *prefix) > if (options.update_refs) > imply_merge(&options, "--update-refs"); > @@ t/t3422-rebase-incompatible-options.sh: test_rebase_am_only () { > + test_must_fail git rebase $opt --empty=ask A > + " > + > -+ test_expect_success "$opt incompatible with --no-reapply-cherry-picks" " > -+ git checkout B^0 && > -+ test_must_fail git rebase $opt --no-reapply-cherry-picks A > -+ " > -+ > -+ test_expect_success "$opt incompatible with --reapply-cherry-picks" " > -+ git checkout B^0 && > -+ test_must_fail git rebase $opt --reapply-cherry-picks A > -+ " > -+ > - test_expect_success "$opt incompatible with --update-refs" " > + test_expect_success "$opt incompatible with --no-reapply-cherry-picks" " > git checkout B^0 && > - test_must_fail git rebase $opt --update-refs A > + test_must_fail git rebase $opt --no-reapply-cherry-picks A > 6: 21ae9e05313 ! 7: 0d0541ea243 rebase: clarify the OPT_CMDMODE incompatibilities > @@ Documentation/git-rebase.txt: See also INCOMPATIBLE OPTIONS below. > @@ Documentation/git-rebase.txt: are incompatible with the following options: > * --no-keep-empty > * --empty= > - * --[no-]reapply-cherry-picks > + * --[no-]reapply-cherry-picks when used without --keep-base > - * --edit-todo > * --update-refs > * --root when used without --onto > 7: 74a216bf0c0 = 8: 01808cf84a8 rebase: fix formatting of rebase --reapply-cherry-picks option in docs > 8: a8adf7fda61 = 9: f646abee524 rebase: put rebase_options initialization in single place > 9: 5cb00e5103b = 10: 328f5a1d534 rebase: provide better error message for apply options vs. merge config >