| Message ID | 20190508001252.15752-14-avarab@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v7,1/6] t3431: add rebase --fork-point tests | expand |
Hi Ævar, On Wed, 8 May 2019, Ævar Arnfjörð Bjarmason wrote: > This seems to work, needs more tests etc... I can see how it works, but it is a bit limited, and at the same time overzealous. The reason why we do not enter the fast-forwarding block in the interactive case would appear to me to be that the interactive rebase *might* want to avoid fast-forwarding e.g. with --force-rebase. However, that is not the only instance where we must not simply fast-forward: Think `--exec`. There might be others, too. (I just saw that `--signoff` sets `FORCE_REBASE`, but `--exec` does not, so you cannot even use the `FORCE_REBASE` flag as indicator.) Since `git rebase -rx "make -j8"` is something I use myself, this here patch would therefore break *my* workflow. This is the "overzealous" part. Now for the "limited" part: Let's take a step back and ask how the interactive rebase handles these potentially fast-forwarding cases? Via `skip_unnecessary_picks()`. And that function is ill-prepared for rebasing merges (I specifically do *not* think about `--preserve-merges` at this point, for all I care, it is already deprecated *and* dropped). Even if this function *was* well-prepared for rebasing merges, I think that would miss the mark. Take this todo list, for example: label onto # Branch dscho reset onto pick a123 first label dscho # Branch avar reset onto pick b789 second label avar reset onto merge -C c124 dscho merge -C d314 avar Two branches, both one patch deep, both merged, one after the other. Now, if you insert `pick abc zeroth` before the first `pick`, obviously the first branch would no longer be skippable, but the second one totally would be! This is the "limited" part. To remedy this, I think what we would need is code in `pick_commits()`, right where `TODO_RESET` is handled (or more toward the beginning of that function), that would: - parse the argument (this is currently done in `do_reset()` and would have to be refactored out) and pretend that it is `HEAD`, - look at the following command: if it is - a `pick`, and if its parent agrees with `HEAD`, pretend that the `pick` was actually a `reset`, update the pretended `HEAD` and keep looking at the next command, - a `merge`, and if its option was `-C <orig-merge>` (not lower-case `-c`!), and if its parent agrees with `HEAD`, and if its merge head(s) agree with the original merge commit's (if any), pretend that it was actually a `reset <orig-merge>`, update the pretended `HEAD` and keep looking at the next command, - a `label`, perform it, but with the pretended `HEAD`, and keep looking for the next command, - a `reset`, update the `done` and `git-rebase-todo` files and start the entire spiel from the top, - otherwise perform the reset. - all while skipping, this code would need to take care of updating the `done` and `git-rebase-todo` files, - if a `reset` is necessary, and if it fails, the `done` and `git-rebase-todo` files should *not* be updated, but the original `reset` should be re-scheduled, and - since this adds quite a bit of code, it should probably be done in a separate function. Instead of marking this as a left-over bit (which I would either forget, or whose status would be hard to track), I decided to open a ticket: https://github.com/gitgitgadget/git/issues/209 (I opened GitGitGadget's issues for exactly this kind of use case, because I recently tried to find some useful left-over bits as easy project starters, and even *I* found it super hard to find those, let alone figure out whether they are being/have been addressed already, a mailing list is just not a good bug tracker, even if it is still better than trying to report a bug on Twitter, where I could not even have written this paragraph in a single Tweet.) Ciao, Dscho > Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> > --- > builtin/rebase.c | 6 ++++++ > t/t3432-rebase-fast-forward.sh | 7 +++++-- > 2 files changed, 11 insertions(+), 2 deletions(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index 167d4fcf67..de1c5cacb8 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -892,6 +892,12 @@ static void populate_merge_bases(struct commit *head, struct commit *onto, > > static int should_fast_forward(struct rebase_options *opts) > { > + if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) { > + if (opts->rebase_merges) > + return 1; > + if (opts->type == REBASE_PRESERVE_MERGES) > + return 1; > + } > return !is_interactive(opts); > } > > diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh > index e8a9bf42b6..d3e1815057 100755 > --- a/t/t3432-rebase-fast-forward.sh > +++ b/t/t3432-rebase-fast-forward.sh > @@ -44,12 +44,13 @@ test_rebase_same_head_ () { > test_expect_$status "git rebase$flag $* with $changes is $what with $cmp HEAD" " > oldhead=\$(git rev-parse HEAD) && > test_when_finished 'git reset --hard \$oldhead' && > - git rebase$flag $* >stdout && > + git rebase$flag $* >stdout 2>stderr && > if test $what = work > then > # Must check this case first, for 'is up to > # date, rebase forced[...]rewinding head' cases > - test_i18ngrep 'rewinding head' stdout > + test_i18ngrep 'rewinding head' stdout || > + test_i18ngrep 'is up to date, rebase forced' stdout > elif test $what = noop > then > test_i18ngrep 'is up to date' stdout && > @@ -79,6 +80,8 @@ test_rebase_same_head success noop same success noop-force same --keep-base mast > test_rebase_same_head success noop same success noop-force same --keep-base > test_rebase_same_head success noop same success noop-force same --no-fork-point > test_rebase_same_head success noop same success noop-force same --keep-base --no-fork-point > +test_rebase_same_head success noop same success noop-force same --preserve-merges > +test_rebase_same_head success noop same success noop-force same --rebase-merges > test_rebase_same_head success noop same success work same --fork-point master > test_rebase_same_head success noop same success work diff --fork-point --onto B B > test_rebase_same_head success noop same success work diff --fork-point --onto B... B > -- > 2.21.0.1020.gf2820cf01a > >
diff --git a/builtin/rebase.c b/builtin/rebase.c index 167d4fcf67..de1c5cacb8 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -892,6 +892,12 @@ static void populate_merge_bases(struct commit *head, struct commit *onto, static int should_fast_forward(struct rebase_options *opts) { + if (!(opts->flags & REBASE_INTERACTIVE_EXPLICIT)) { + if (opts->rebase_merges) + return 1; + if (opts->type == REBASE_PRESERVE_MERGES) + return 1; + } return !is_interactive(opts); } diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh index e8a9bf42b6..d3e1815057 100755 --- a/t/t3432-rebase-fast-forward.sh +++ b/t/t3432-rebase-fast-forward.sh @@ -44,12 +44,13 @@ test_rebase_same_head_ () { test_expect_$status "git rebase$flag $* with $changes is $what with $cmp HEAD" " oldhead=\$(git rev-parse HEAD) && test_when_finished 'git reset --hard \$oldhead' && - git rebase$flag $* >stdout && + git rebase$flag $* >stdout 2>stderr && if test $what = work then # Must check this case first, for 'is up to # date, rebase forced[...]rewinding head' cases - test_i18ngrep 'rewinding head' stdout + test_i18ngrep 'rewinding head' stdout || + test_i18ngrep 'is up to date, rebase forced' stdout elif test $what = noop then test_i18ngrep 'is up to date' stdout && @@ -79,6 +80,8 @@ test_rebase_same_head success noop same success noop-force same --keep-base mast test_rebase_same_head success noop same success noop-force same --keep-base test_rebase_same_head success noop same success noop-force same --no-fork-point test_rebase_same_head success noop same success noop-force same --keep-base --no-fork-point +test_rebase_same_head success noop same success noop-force same --preserve-merges +test_rebase_same_head success noop same success noop-force same --rebase-merges test_rebase_same_head success noop same success work same --fork-point master test_rebase_same_head success noop same success work diff --fork-point --onto B B test_rebase_same_head success noop same success work diff --fork-point --onto B... B
This seems to work, needs more tests etc... Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> --- builtin/rebase.c | 6 ++++++ t/t3432-rebase-fast-forward.sh | 7 +++++-- 2 files changed, 11 insertions(+), 2 deletions(-)