[0/5] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point

Message

Derrick Stolee via GitGitGadget Aug. 15, 2022, 3:11 p.m. UTC

A while a go Philippe reported [1] that he was surprised 'git rebase
--keep-base' removed commits that had been cherry-picked upstream even
though to branch was not being rebased. I think it is also surprising if
'--keep-base' changes the base of the branch without '--fork-point' being
explicitly given on the command line. This series therefore changes the
default behavior of '--keep-base' to imply '--reapply-cherry-picks' and
'--no-fork-point' so that the base of the branch is unchanged and no commits
are removed.

The first 3 patches are preparatory cleanups and refactorings, the last 2
patches then implement the new defaults for --keep-base.

[1]
https://lore.kernel.org/git/[e-mail]/

Phillip Wood (5):
  t3416: set $EDITOR in subshell
  rebase: store orig_head as a commit
  rebase: factor out merge_base calculation
  rebase --keep-base: imply --reapply-cherry-picks
  rebase --keep-base: imply --no-fork-point

 Documentation/git-rebase.txt     |   2 +-
 builtin/rebase.c                 | 116 +++++++++++++++++++------------
 t/t3416-rebase-onto-threedots.sh |  52 +++++++++++---
 t/t3431-rebase-fork-point.sh     |   2 +-
 4 files changed, 114 insertions(+), 58 deletions(-)


base-commit: afa70145a25e81faa685dc0b465e52b45d2444bd
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1323%2Fphillipwood%2Fwip%2Frebase--keep-base-tweaks-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1323/phillipwood/wip/rebase--keep-base-tweaks-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1323

Comments

Johannes Schindelin Aug. 16, 2022, 9:23 a.m. UTC | #1

Hi Phillip,

On Mon, 15 Aug 2022, Phillip Wood via GitGitGadget wrote:

> A while a go Philippe reported [1] that he was surprised 'git rebase
> --keep-base' removed commits that had been cherry-picked upstream even
> though to branch was not being rebased.

This has bitten me, too (I did just not get around to reply to Philippe's
mail).

> I think it is also surprising if '--keep-base' changes the base of the
> branch without '--fork-point' being explicitly given on the command
> line. This series therefore changes the default behavior of
> '--keep-base' to imply '--reapply-cherry-picks' and '--no-fork-point' so
> that the base of the branch is unchanged and no commits are removed.

In my mind, `--keep-base` should always have adjusted the `<upstream>` to
point to the base commit, which is what patch 4/5 does.

So I am very much in favor of this patch series.

Junio seems to be on top of the code review, so I'll avoid stepping on his
toes by adding mine ;-)

Ciao,
Dscho

Jonathan Tan Aug. 24, 2022, 10:27 p.m. UTC | #2

"Phillip Wood via GitGitGadget" <[e-mail]> writes:
> A while a go Philippe reported [1] that he was surprised 'git rebase
> --keep-base' removed commits that had been cherry-picked upstream even
> though to branch was not being rebased.
[...]
> [1]
> https://lore.kernel.org/git/[e-mail]/

In the thread at [1] I said that (in [2]) I expected that
--reapply-cherry-picks would be the default, but this implicit
--no-reapply-cherry-picks (in order to automatically exclude certain
commits) is a long-available and long-advertised feature. So I
symphatize with making certain modes of rebase automatically imply
--reapply-cherry-picks, but I think that either all modes (as much as
possible) should imply it, or no mode should imply it.

Having said that, I already looked at the code so I have included my
review comments anyway (in case we do decide to have implied
--reapply-cherry-picks for --keep-base but not for other cases).

As for the --fork-point part, I made a comment in my reply to patch 5.

[2] https://lore.kernel.org/git/[e-mail]/