[v2,7/7] rebase --keep-base: imply --no-fork-point

Commit Message

Phillip Wood Sept. 7, 2022, 2:37 p.m. UTC

From: Phillip Wood <[e-mail]>

Given the name of the option it is confusing if --keep-base actually
changes the base of the branch without --fork-point being explicitly
given on the command line.

The combination of --keep-base with an explicit --fork-point is still
supported even though --fork-point means we do not keep the same base
if the upstream branch has been rewound.  We do this in case anyone is
relying on this behavior which is tested in t3431[1]

[1] https://lore.kernel.org/git/20200715032014.GA10818@generichostname/

Signed-off-by: Phillip Wood <[e-mail]>
---
 Documentation/git-rebase.txt | 8 ++++----
 builtin/rebase.c             | 6 ++++++
 t/t3431-rebase-fork-point.sh | 2 +-
 3 files changed, 11 insertions(+), 5 deletions(-)

Comments

Denton Liu Sept. 8, 2022, 2:44 a.m. UTC | #1

Hi Phillip,

Sorry I haven't been active in Git development lately. I've been busy
with other stuff. I was skimming my emails and this patch caught my eye.

On Wed, Sep 07, 2022 at 02:37:50PM +0000, Phillip Wood via GitGitGadget wrote:
> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
> index 1d0b15380ed..70e81363569 100755
> --- a/t/t3431-rebase-fork-point.sh
> +++ b/t/t3431-rebase-fork-point.sh
> @@ -50,7 +50,7 @@ test_rebase () {
>  
>  test_rebase 'G F E D B A'
>  test_rebase 'G F D B A' --onto D
> -test_rebase 'G F B A' --keep-base
> +test_rebase 'G F C B A' --keep-base

We should add a test here for explicit --fork-point just to ensure that
the behaviour stays the same in case anyone relies on it here.

>  test_rebase 'G F C E D B A' --no-fork-point
>  test_rebase 'G F C D B A' --no-fork-point --onto D
>  test_rebase 'G F C B A' --no-fork-point --keep-base
> -- 
> gitgitgadget

Phillip Wood Sept. 8, 2022, 1:21 p.m. UTC | #2

Hi Denton

On 08/09/2022 03:44, Denton Liu wrote:
> Hi Phillip,
> 
> Sorry I haven't been active in Git development lately. I've been busy
> with other stuff. I was skimming my emails and this patch caught my eye.

Thanks for taking a look

> On Wed, Sep 07, 2022 at 02:37:50PM +0000, Phillip Wood via GitGitGadget wrote:
>> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
>> index 1d0b15380ed..70e81363569 100755
>> --- a/t/t3431-rebase-fork-point.sh
>> +++ b/t/t3431-rebase-fork-point.sh
>> @@ -50,7 +50,7 @@ test_rebase () {
>>   
>>   test_rebase 'G F E D B A'
>>   test_rebase 'G F D B A' --onto D
>> -test_rebase 'G F B A' --keep-base
>> +test_rebase 'G F C B A' --keep-base
> 
> We should add a test here for explicit --fork-point just to ensure that
> the behaviour stays the same in case anyone relies on it here.

There is an existing test, it does not show up in the patch because it 
is unchanged.

Best Wishes

Phillip

Patch

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index ee6cdd56949..1e2d543ced9 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -218,7 +218,7 @@  leave out at most one of A and B, in which case it defaults to HEAD.
 	merge base of `<upstream>` and `<branch>`. Running
 	`git rebase --keep-base <upstream> <branch>` is equivalent to
 	running
-	`git rebase --reapply-cherry-picks --onto <upstream>...<branch> <upstream> <branch>`.
+	`git rebase --reapply-cherry-picks --no-fork-point --onto <upstream>...<branch> <upstream> <branch>`.
 +
 This option is useful in the case where one is developing a feature on
 top of an upstream branch. While the feature is being worked on, the
@@ -452,9 +452,9 @@  When `--fork-point` is active, 'fork_point' will be used instead of
 <branch>` command (see linkgit:git-merge-base[1]).  If 'fork_point'
 ends up being empty, the `<upstream>` will be used as a fallback.
 +
-If `<upstream>` is given on the command line, then the default is
-`--no-fork-point`, otherwise the default is `--fork-point`. See also
-`rebase.forkpoint` in linkgit:git-config[1].
+If `<upstream>` or `--keep-base` is given on the command line, then
+the default is `--no-fork-point`, otherwise the default is
+`--fork-point`. See also `rebase.forkpoint` in linkgit:git-config[1].
 +
 If your branch was based on `<upstream>` but `<upstream>` was rewound and
 your branch contains commits which were dropped, this option can be used
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 204155bb25b..3065e6f082b 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1239,6 +1239,12 @@  int cmd_rebase(int argc, const char **argv, const char *prefix)
 			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--onto");
 		if (options.root)
 			die(_("options '%s' and '%s' cannot be used together"), "--keep-base", "--root");
+		/*
+		 * --keep-base defaults to --no-fork-point to keep the
+		 * base the same.
+		 */
+		if (options.fork_point < 0)
+			options.fork_point = 0;
 	}
 	/*
 	 * --keep-base defaults to --reapply-cherry-picks to avoid losing
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 1d0b15380ed..70e81363569 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -50,7 +50,7 @@  test_rebase () {
 
 test_rebase 'G F E D B A'
 test_rebase 'G F D B A' --onto D
-test_rebase 'G F B A' --keep-base
+test_rebase 'G F C B A' --keep-base
 test_rebase 'G F C E D B A' --no-fork-point
 test_rebase 'G F C D B A' --no-fork-point --onto D
 test_rebase 'G F C B A' --no-fork-point --keep-base