RFC: Modify rebase-if-necessary strategy for rebase of merge commits

[Jacek Centkowski]

Current implementation of rebase-if-necessary falls back to merge-if-necessary strategy when target branch advances while merge commit is being reviewed. This is definitely the only option for octa-merges but not for more common (IMHO) 'merge one branch' scenario.

[1] describes more details about issues with current approach and provides simple solution to them but I believe that it is more natural to discuss it here.

So feel free to provide your feedback ;)

[1] https://gerrit-review.googlesource.com/#/c/76670/

[Dave Borowitz]

I agree with the motivation and with the idea of basically attempting "git rebase -p". I'm sure the devil is in the details, e.g. what if there are multiple merge commits in a single series of changes that are all submitted in a batch.

I would like to see in the commit message and in the tests a diagram showing how it's intended to work.

--
--
To unsubscribe, email repo-discuss...@googlegroups.com
More info at http://groups.google.com/group/repo-discuss?hl=en

---
You received this message because you are subscribed to the Google Groups "Repo and Gerrit Discussion" group.
To unsubscribe from this group and stop receiving emails from it, send an email to repo-discuss...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Jacek Centkowski]

I have played with the following 3 scenarios and I believe that other cases would be more or less combination of these 3 but feel free to point out scenario that you think is valid.

1. Use case for changes submitted together that contains merge (M) and regular (C) commit:

C ec34f0f feature advances ;) 31

M   3b95718 Merge branch 'feature_3'

| \

|  * e063518 rin test commit pr_3_02

|  * bfdc99f rin test commit pr_3_01

H1 | 832e5a0 master advances ;) 01

| /

* 1c5a52a Initial empty repository

Master advances to commit H2 (eedce53) and result looks as expected. Both M and C were rebased over H2 (to correspondingly M' and C') and as a result PatchSet#2 was created for related review. Here is history after operation:

C' 8964409 feature advances ;) 31

M'   b81e1b2 Merge branch 'feature_3'

| \

|  * e063518 rin test commit pr_3_02

|  * bfdc99f rin test commit pr_3_01

H2 | eedce53 topic advances ;) 1

H1 | 832e5a0 master advances ;) 01

| /

* 1c5a52a Initial empty repository

2. Use case for submit whole topic scenario. Both merge commits have common parent H1 (eedce53):

a) merge commit for feature 4

MF4   3bb1de8 Merge branch 'feature_4' into topic

| \

|  * 54d0c9c rin test commit pr_4_02

|  * b3059b4 rin test commit pr_4_01

H1 | eedce53 topic advances ;) 1

| /

* 832e5a0 master advances ;) 01

* 1c5a52a Initial empty repository

b) merge commit for feature 5

MF5   fb45500 Merge branch 'feature_5' into topic

| \

|  * 0655528 rin test commit pr_5_02

|  * 42cce1e rin test commit pr_5_01

H1 | eedce53 topic advances ;) 1

| /

* 832e5a0 master advances ;) 01

* 1c5a52a Initial empty repository

Master advances to commit H2 (8964409) and submit button is pressed on MF5 commit review (probably the reason why it was merged first ;) but it doesn't matter). Both MF5 and MF4 were rebased over H2 (to correspondingly MF5' and MF4') and as a result PatchSet#2 was created for corresponding reviews. Here is history after operation:

MF4'   2db2604 Merge branch 'feature_4' into topic

|   \

|    * 54d0c9c rin test commit pr_4_02

|    * b3059b4 rin test commit pr_4_01

MF5' |   777c741 Merge branch 'feature_5' into topic

| \   \

|  *   | 0655528 rin test commit pr_5_02

|  *   | 42cce1e rin test commit pr_5_01

|  |  /

H2 | 8964409 feature advances ;) 31

H1 | eedce53 topic advances ;) 1

| /

* 832e5a0 master advances ;) 01

* 1c5a52a Initial empty repository

3. Use case for changes submitted together that contain regular (C) and merge (M1, M2) commits:

M2   97c80bc Merge branch 'feature_2' into integration

| \

|  * d48e6a0 rin test commit pr_2_02

|  * 61dacca rin test commit pr_2_01

M1 |   7188044 Merge branch 'feature_1' into integration

|\  \

| *  | 90d52ab rin test commit pr_1_02

| *  | 23e265c rin test commit pr_1_01

| | /

C | 5d204a9 st to merge to ;) 01

|/

H1 1c5a52a Initial empty repository

Current master points to H1 (1c5a52a) commit. All reviews were reviewed successfully, master advances to H2 commit (832e5a0).

History result is as expected in regards to C and M2 commits (Patchset #2 is created for reviews) but falls back to MERGE-IF-NECESSARY for M1 commit and MC1 commit is created:

M2'  14f5b88 Merge branch 'feature_2' into integration

| \

|   * d48e6a0 rin test commit pr_2_02

|   * 61dacca rin test commit pr_2_01

MC1 |   3ada8bb Merge changes If01000f3,I8954c8f3

|\   \

| M1  \   7188044 Merge branch 'feature_1' into integration

|  |\  \

|  | *  | 90d52ab rin test commit pr_1_02

|  | *  | 23e265c rin test commit pr_1_01

|  | | /

|  C | 5d204a9 st to merge to ;) 01

|  |/

C' | e3beeac st to merge to ;) 01

H2 | 832e5a0 master advances ;) 01

| /

H1 1c5a52a Initial empty repository

The problem is when M1 is about to be rebased and its parents are examined for being part of target branch. Due to the fact that C (first parent) was actually rebased to C' none of the parents can be found in target branch and logic falls back to original MERGE-IF-NECESSARY strategy that results in MC1 commit. This actually helps :) in the following rebase of M2 commit as its parent M1 can be found in target branch (as a parent of MC1 commit) therefore rebase is performed and looks as designed.

As I have mentioned in at the beginning this is pretty simple implementation that basically checks if any parent is part of target branch and depending on it performs rebase or merge. I would need some help in order to determine that instead of C actually C' should be searched in target branch.

Expected behaviour is that all commits were rebased over H2 and history looks like in the following diagram:

M2'   14f5b88 Merge branch 'feature_2' into integration

|  \

|   * d48e6a0 rin test commit pr_2_02

|   * 61dacca rin test commit pr_2_01

M1' |   3ada8bb Merge branch 'feature_1' into integration

| \  \

|  *  | 90d52ab rin test commit pr_1_02

|  *  | 23e265c rin test commit pr_1_01

|  | /

C' | e3beeac st to merge to ;) 01

H2 | 832e5a0 master advances ;) 01

| /

H1 1c5a52a Initial empty repository