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 <[email protected]>
> ---
> 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
>
>
