git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>
Subject: Re: [PATCH v3 3/4] rebase: fast-forward --onto in more cases
Date: Tue, 02 Apr 2019 10:48:08 +0900
Message-ID: <xmqqo95ptbxj.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <xmqqsgv1tcyi.fsf@gitster-ct.c.googlers.com> (Junio C. Hamano's message of "Tue, 02 Apr 2019 10:25:57 +0900")

Junio C Hamano <gitster@pobox.com> writes:

> Denton Liu <liu.denton@gmail.com> writes:
>
>> Before, when we had the following graph,
>>
>> 	A---B---C (master)
>> 	    \
>> 	     D (side)
>>
>> running 'git rebase --onto master... master side' would result in D
>> being always rebased, no matter what. However, the desired behavior is
>> that rebase should notice that this is fast-forwardable and do that
>> instead.
>>
>> Add detection to `can_fast_forward` so that this case can be detected
>> and a fast-forward will be performed.
>
> OK.  As long as the 'onto' commit is a strict ancestor of the side
> branch being rebased, the 'upstream' that is used only to determine
> which commits are on the side branch (essentially those that are not
> reachable from upstream but that are from the side branch) should
> not count in the equation to decide if we fast-forward or not.  That
> makes sense.
>
>> ---
>>  builtin/rebase.c               | 40 +++++++++++++++++++++++-----------
>>  t/t3400-rebase.sh              |  2 +-
>>  t/t3404-rebase-interactive.sh  |  2 +-
>>  t/t3432-rebase-fast-forward.sh |  2 +-
>>  4 files changed, 30 insertions(+), 16 deletions(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index 77deebc65c..7aa6a090d4 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -895,12 +895,12 @@ static int is_linear_history(struct commit *from, struct commit *to)
>>  	return 1;
>>  }
>>  
>> -static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
>> -			    struct object_id *merge_base)
>> +static int can_fast_forward(struct commit *onto, struct commit *upstream,
>> +			    struct object_id *head_oid, struct object_id *merge_base)
>>  {
>>  	struct commit *head = lookup_commit(the_repository, head_oid);
>> -	struct commit_list *merge_bases;
>> -	int res;
>> +	struct commit_list *merge_bases = NULL;
>> +	int res = 0;
>>  
>>  	if (!head)
>>  		return 0;
>> @@ -908,12 +908,29 @@ static int can_fast_forward(struct commit *onto, struct object_id *head_oid,
>>  	merge_bases = get_merge_bases(onto, head);
>>  	if (merge_bases && !merge_bases->next) {
>>  		oidcpy(merge_base, &merge_bases->item->object.oid);
>> -		res = oideq(merge_base, &onto->object.oid);
>> +		if (!oideq(merge_base, &onto->object.oid))
>> +			goto done;
>>  	} else {
>>  		oidcpy(merge_base, &null_oid);
>> -		res = 0;
>> +		goto done;
>>  	}
>
> The above does not change any existing logic, but purely simplifies
> the code.  In your picture in the log message
>
>     A---B---C (master)
>          \
>           D (side)
>
> where "rebase --onto master... master side" is run, "onto" in this
> function is B, and "head" is D.  There is a single merge base B
> (i.e. merge_bases->next == NULL), so we jump to the label "done:"
> with res==0.
>
> So why does the remainder of the function need to be changed?
>
>> +	if (!upstream)
>> +		goto done;
>> +
>>  	free_commit_list(merge_bases);
>> +	merge_bases = get_merge_bases(upstream, head);
>> +	if (merge_bases && !merge_bases->next) {
>> +		if (!oideq(&onto->object.oid, &merge_bases->item->object.oid))
>> +			goto done;
>> +	} else
>> +		goto done;
>
> This computes the same between C and D (instead of B and D).  Why is
> this needed?
>
> Stepping back a bit, I understand that your argument we saw in the
> log message was that we only need to know if the commit we are
> transplanting the history on (i.e. "onto") already is an ancestor of
> the history being transplanted (i.e. "onto..head"), and it does not
> matter what upstream is.  Am I mistaken?  Why does this function now
> need to know what 'upstream' is?
>
> Puzzled....

So I replaced the part for builtin/rebase.c from your patch with the
above suggestion (attached) and the result seems to pass the three
tests you touched.  I am still puzzled.

Thanks.

 builtin/rebase.c               | 7 ++-----
 t/t3400-rebase.sh              | 2 +-
 t/t3404-rebase-interactive.sh  | 2 +-
 t/t3432-rebase-fast-forward.sh | 2 +-
 4 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index 77deebc65c..fe61c2a899 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1682,13 +1682,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 
 	/*
 	 * Check if we are already based on onto with linear history,
-	 * but this should be done only when upstream and onto are the same
-	 * and if this is not an interactive rebase.
+	 * but this should be done if this is not an interactive rebase.
 	 */
 	if (can_fast_forward(options.onto, &options.orig_head, &merge_base) &&
-	    !is_interactive(&options) && !options.restrict_revision &&
-	    options.upstream &&
-	    !oidcmp(&options.upstream->object.oid, &options.onto->object.oid)) {
+	    !is_interactive(&options) && !options.restrict_revision) {
 		int flag;
 
 		if (!(options.flags & REBASE_FORCE)) {
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 460d0523be..604d624ff8 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -295,7 +295,7 @@ test_expect_success 'rebase--am.sh and --show-current-patch' '
 		echo two >>init.t &&
 		git commit -a -m two &&
 		git tag two &&
-		test_must_fail git rebase --onto init HEAD^ &&
+		test_must_fail git rebase -f --onto init HEAD^ &&
 		GIT_TRACE=1 git rebase --show-current-patch >/dev/null 2>stderr &&
 		grep "show.*$(git rev-parse two)" stderr
 	)
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index b60b11f9f2..f054186cc7 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1066,7 +1066,7 @@ test_expect_success C_LOCALE_OUTPUT 'rebase --edit-todo does not work on non-int
 	git reset --hard &&
 	git checkout conflict-branch &&
 	set_fake_editor &&
-	test_must_fail git rebase --onto HEAD~2 HEAD~ &&
+	test_must_fail git rebase -f --onto HEAD~2 HEAD~ &&
 	test_must_fail git rebase --edit-todo &&
 	git rebase --abort
 '
diff --git a/t/t3432-rebase-fast-forward.sh b/t/t3432-rebase-fast-forward.sh
index 3e6362dd9c..414b4216d6 100755
--- a/t/t3432-rebase-fast-forward.sh
+++ b/t/t3432-rebase-fast-forward.sh
@@ -54,6 +54,6 @@ test_expect_success 'add work to upstream' '
 changes='our and their changes'
 test_rebase_same_head success '--onto B B'
 test_rebase_same_head success '--onto B... B'
-test_rebase_same_head failure '--onto master... master'
+test_rebase_same_head success '--onto master... master'
 
 test_done

  reply index

Thread overview: 123+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-03-23 15:25 [PATCH 0/3] rebase: learn --keep-base Denton Liu
2019-03-23 15:25 ` [PATCH 1/3] rebase: teach rebase --keep-base Denton Liu
2019-03-24  3:46   ` Eric Sunshine
2019-03-24 13:20   ` Junio C Hamano
2019-03-25  0:06     ` Denton Liu
2019-03-25  5:41       ` Denton Liu
2019-04-01 10:45         ` Junio C Hamano
2019-03-24 13:37   ` Junio C Hamano
2019-03-25  5:47   ` Denton Liu
2019-03-25 18:50   ` Johannes Schindelin
2019-03-25 19:29     ` Denton Liu
2019-03-26 13:27       ` Johannes Schindelin
2019-03-23 15:25 ` [PATCH 2/3] t3416: test " Denton Liu
2019-03-23 15:25 ` [PATCH 3/3] git-rebase.txt: document --keep-base option Denton Liu
2019-03-24 13:15 ` [PATCH 0/3] rebase: learn --keep-base Junio C Hamano
2019-03-25  0:04   ` Denton Liu
2019-04-01 10:45     ` Junio C Hamano
2019-03-26 14:35 ` Ævar Arnfjörð Bjarmason
2019-03-26 17:50   ` Denton Liu
2019-03-26 20:35     ` Ævar Arnfjörð Bjarmason
2019-03-26 21:30       ` Denton Liu
2019-03-27 15:39         ` Ævar Arnfjörð Bjarmason
2019-03-28 22:17 ` [PATCH v2] rebase: teach rebase --keep-base Denton Liu
2019-03-28 23:13   ` Ævar Arnfjörð Bjarmason
2019-03-29 15:47   ` Johannes Schindelin
2019-03-29 17:52     ` Denton Liu
2019-04-01 10:46     ` Junio C Hamano
2019-04-01 20:51   ` [PATCH v3 0/4] " Denton Liu
2019-04-01 20:51     ` [PATCH v3 1/4] t3431: add rebase --fork-point tests Denton Liu
2019-04-04 20:28       ` Denton Liu
2019-04-05 11:15       ` SZEDER Gábor
2019-04-08  4:38         ` Junio C Hamano
2019-04-05 14:55       ` Johannes Schindelin
2019-04-05 17:25         ` Denton Liu
2019-04-05 17:51           ` Johannes Sixt
2019-04-05 18:51             ` Johannes Schindelin
2019-04-05 20:19               ` Johannes Schindelin
2019-04-05 21:10                 ` SZEDER Gábor
2019-04-01 20:51     ` [PATCH v3 2/4] t3432: test rebase fast-forward behavior Denton Liu
2019-04-01 20:52     ` [PATCH v3 3/4] rebase: fast-forward --onto in more cases Denton Liu
2019-04-02  1:25       ` Junio C Hamano
2019-04-02  1:48         ` Junio C Hamano [this message]
2019-04-02  4:44           ` Denton Liu
2019-04-01 20:52     ` [PATCH v3 4/4] rebase: teach rebase --keep-base Denton Liu
2019-04-05 21:39     ` [PATCH v4 0/4] " Denton Liu
2019-04-05 21:40       ` [PATCH v4 1/4] t3431: add rebase --fork-point tests Denton Liu
2019-04-05 21:40       ` [PATCH v4 2/4] t3432: test rebase fast-forward behavior Denton Liu
2019-04-05 21:40       ` [PATCH v4 3/4] rebase: fast-forward --onto in more cases Denton Liu
2019-04-05 21:40       ` [PATCH v4 4/4] rebase: teach rebase --keep-base Denton Liu
2019-04-15 22:29       ` [PATCH v5 0/5] " Denton Liu
2019-04-15 22:29         ` [PATCH v5 1/5] t3431: add rebase --fork-point tests Denton Liu
2019-04-15 22:29         ` [PATCH v5 2/5] t3432: test rebase fast-forward behavior Denton Liu
2019-04-15 22:29         ` [PATCH v5 3/5] rebase: fast-forward --onto in more cases Denton Liu
2019-04-16  6:26           ` Junio C Hamano
2019-04-16 13:59           ` Phillip Wood
2019-04-17  6:44             ` Denton Liu
2019-04-17 14:14               ` Phillip Wood
2019-04-19 17:08           ` Denton Liu
2019-04-15 22:29         ` [PATCH v5 4/5] rebase: fast-forward --fork-point " Denton Liu
2019-04-15 22:29         ` [PATCH v5 5/5] rebase: teach rebase --keep-base Denton Liu
2019-04-17 18:01       ` [PATCH v6 0/5] " Denton Liu
2019-04-17 18:01         ` [PATCH v6 1/6] t3431: add rebase --fork-point tests Denton Liu
2019-04-17 18:01         ` [PATCH v6 2/6] t3432: test rebase fast-forward behavior Denton Liu
2019-04-17 18:01         ` [PATCH v6 3/6] rebase: refactor can_fast_forward into goto tower Denton Liu
2019-04-17 18:01         ` [PATCH v6 4/6] rebase: fast-forward --onto in more cases Denton Liu
2019-04-17 19:59           ` Phillip Wood
2019-04-17 18:01         ` [PATCH v6 5/6] rebase: fast-forward --fork-point " Denton Liu
2019-04-17 18:01         ` [PATCH v6 6/6] rebase: teach rebase --keep-base Denton Liu
2019-04-21  8:11         ` [PATCH v7 0/6] rebase: learn --keep-base Denton Liu
2019-04-21  8:11           ` [PATCH v7 1/6] t3431: add rebase --fork-point tests Denton Liu
2019-04-23 23:12             ` Denton Liu
2019-04-21  8:11           ` [PATCH v7 2/6] t3432: test rebase fast-forward behavior Denton Liu
2019-04-21  8:11           ` [PATCH v7 3/6] rebase: refactor can_fast_forward into goto tower Denton Liu
2019-04-21  8:11           ` [PATCH v7 4/6] rebase: fast-forward --onto in more cases Denton Liu
2019-04-21  8:11           ` [PATCH v7 5/6] rebase: fast-forward --fork-point " Denton Liu
2019-04-21  8:11           ` [PATCH v7 6/6] rebase: teach rebase --keep-base Denton Liu
2019-05-08  0:12           ` [RFC WIP PATCH v8 00/13] learn --keep-base & more Ævar Arnfjörð Bjarmason
2019-05-08  3:57             ` Junio C Hamano
2019-07-19 19:14               ` Junio C Hamano
2019-07-19 21:01                 ` Denton Liu
2019-08-25  9:11             ` [PATCH v9 0/9] rebase: learn --keep-base and improvements on fast-forward behaviour Denton Liu
2019-08-25  9:11               ` [PATCH v9 1/9] t3431: add rebase --fork-point tests Denton Liu
2019-08-25  9:12               ` [PATCH v9 2/9] t3432: test rebase fast-forward behavior Denton Liu
2019-08-25  9:12               ` [PATCH v9 3/9] t3432: distinguish "noop-same" v.s. "work-same" in "same head" tests Denton Liu
2019-08-25  9:12               ` [PATCH v9 4/9] t3432: test for --no-ff's interaction with fast-forward Denton Liu
2019-08-25  9:12               ` [PATCH v9 5/9] rebase: refactor can_fast_forward into goto tower Denton Liu
2019-08-25 13:17                 ` Pratyush Yadav
2019-08-26 23:17                   ` Denton Liu
2019-08-25  9:12               ` [PATCH v9 6/9] rebase: fast-forward --onto in more cases Denton Liu
2019-08-25  9:12               ` [PATCH v9 7/9] rebase: fast-forward --fork-point " Denton Liu
2019-08-25  9:12               ` [PATCH v9 8/9] rebase tests: test linear branch topology Denton Liu
2019-08-25  9:12               ` [PATCH v9 9/9] rebase: teach rebase --keep-base Denton Liu
2019-08-25 22:59                 ` Philip Oakley
2019-08-26 19:37               ` [PATCH v9 0/9] rebase: learn --keep-base and improvements on fast-forward behaviour Junio C Hamano
2019-08-27  5:37               ` [PATCH v10 " Denton Liu
2019-08-27  5:37                 ` [PATCH v10 1/9] t3431: add rebase --fork-point tests Denton Liu
2019-08-27  5:37                 ` [PATCH v10 2/9] t3432: test rebase fast-forward behavior Denton Liu
2019-08-27  5:37                 ` [PATCH v10 3/9] t3432: distinguish "noop-same" v.s. "work-same" in "same head" tests Denton Liu
2019-08-27  5:37                 ` [PATCH v10 4/9] t3432: test for --no-ff's interaction with fast-forward Denton Liu
2019-08-27  8:11                   ` SZEDER Gábor
2019-08-27  5:37                 ` [PATCH v10 5/9] rebase: refactor can_fast_forward into goto tower Denton Liu
2019-08-27  5:37                 ` [PATCH v10 6/9] rebase: fast-forward --onto in more cases Denton Liu
2019-08-27  5:38                 ` [PATCH v10 7/9] rebase: fast-forward --fork-point " Denton Liu
2019-08-27  5:38                 ` [PATCH v10 8/9] rebase tests: test linear branch topology Denton Liu
2019-08-27  5:38                 ` [PATCH v10 9/9] rebase: teach rebase --keep-base Denton Liu
2019-05-08  0:12           ` [RFC WIP PATCH v8 01/13] t3431: add rebase --fork-point tests Ævar Arnfjörð Bjarmason
2019-05-08  0:12           ` [RFC WIP PATCH v8 02/13] t3432: test rebase fast-forward behavior Ævar Arnfjörð Bjarmason
2019-05-08  0:12           ` [RFC WIP PATCH v8 03/13] t3432: distinguish "noop-same" v.s. "work-same" in "same head" tests Ævar Arnfjörð Bjarmason
2019-05-08  0:12           ` [RFC WIP PATCH v8 04/13] t3432: test for --no-ff's interaction with fast-forward Ævar Arnfjörð Bjarmason
2019-05-08  0:12           ` [RFC WIP PATCH v8 05/13] rebase: refactor can_fast_forward into goto tower Ævar Arnfjörð Bjarmason
2019-05-08  0:12           ` [RFC WIP PATCH v8 06/13] rebase: fast-forward --onto in more cases Ævar Arnfjörð Bjarmason
2019-05-08  0:12           ` [RFC WIP PATCH v8 07/13] rebase: fast-forward --fork-point " Ævar Arnfjörð Bjarmason
2019-05-08  0:12           ` [RFC WIP PATCH v8 08/13] rebase: teach rebase --keep-base Ævar Arnfjörð Bjarmason
2019-05-08  0:12           ` [RFC WIP PATCH v8 09/13] rebase tests: test linear branch topology Ævar Arnfjörð Bjarmason
2019-05-08  0:12           ` [RFC WIP PATCH v8 10/13] rebase: don't rebase linear topology with --fork-point Ævar Arnfjörð Bjarmason
2019-05-08  0:12           ` [RFC WIP PATCH v8 11/13] rebase: eliminate side-effects from can_fast_forward() Ævar Arnfjörð Bjarmason
2019-05-17 21:16             ` Johannes Schindelin
2019-05-08  0:12           ` [RFC WIP PATCH v8 12/13] rebase: add a should_fast_forward() utility function Ævar Arnfjörð Bjarmason
2019-05-08  0:12           ` [RFC WIP PATCH v8 13/13] WIP: can_fast_forward() support for --preserve-merges and --rebase-merges Ævar Arnfjörð Bjarmason
2019-05-17 22:02             ` Johannes Schindelin
2019-04-06 19:44     ` [PATCH v3 0/4] rebase: teach rebase --keep-base Ævar Arnfjörð Bjarmason
2019-04-06 20:38       ` Denton Liu
2019-04-13 21:10         ` Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqo95ptbxj.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Example config snippet for mirrors

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git