Re: [RFC WIP PATCH v8 13/13] WIP: can_fast_forward() support for --preserve-merges and --rebase-merges

Hi Ævar,

On Wed, 8 May 2019, Ævar Arnfjörð Bjarmason wrote:

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