| Message ID | pull.1466.v5.git.1674619434.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | rebase: fix several code/testing/documentation issues around flag incompatibilities | expand |
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 <newren@gmail.com> > > - ## 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 >
"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > 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. Hmph, does this replace the previous round that has already been in 'next' for two days? I do not mind reverting the merge and requeuing it, but just wanted ot make sure before doing anything. Thanks.
On Wed, Jan 25, 2023 at 8:39 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > 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. > > Hmph, does this replace the previous round that has already been in > 'next' for two days? I do not mind reverting the merge and requeuing > it, but just wanted ot make sure before doing anything. Sorry, I didn't realize it had merged to next. With Phillip's ongoing reviews and requests for changes, which you had even commented on (https://lore.kernel.org/git/xmqqpmb4j8e9.fsf@gitster.g/ and the thread that was in the middle of), I assumed it was still only in seen. But yes, if you could revert from next and replace, that would be great. Now that v5 has Phillip's blessing (and multiple of his corrections), I think it's good to merge down.
Hi Elijah, On Wed, 25 Jan 2023, 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. Thank you for working on this! FWIW this report (and your patch series) made me wistful about that Google Summer of Code project that I hoped would bring about the trick to combine `--whitespace=fix` with interactive rebases. But in that GSoC project, this goal was treated as a "bonus feature" and pushed so far back that we did not even get to analyzing the complexity of the task, let alone any details. So I sat down and started that analysis. The result is https://github.com/gitgitgadget/git/issues/1472 where you can see that addressing the incompatibility is definitely outside of trivial. It does seem doable, if Outreachy/GSoC project-sized. Ciao, Johannes
On Thu, Feb 2, 2023 at 2:29 AM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Hi Elijah, > > On Wed, 25 Jan 2023, 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. > > Thank you for working on this! > > FWIW this report (and your patch series) made me wistful about that Google > Summer of Code project that I hoped would bring about the trick to combine > `--whitespace=fix` with interactive rebases. But in that GSoC project, > this goal was treated as a "bonus feature" and pushed so far back that we > did not even get to analyzing the complexity of the task, let alone any > details. > > So I sat down and started that analysis. The result is > https://github.com/gitgitgadget/git/issues/1472 where you can see that > addressing the incompatibility is definitely outside of trivial. It does > seem doable, if Outreachy/GSoC project-sized. Nice! Thanks for digging in and documenting what is needed!