Patchwork D8982: merge: disable `m2-vs-ma` diff optimization in case of flat manifests

login
register
mail settings
Submitter phabricator
Date Sept. 5, 2020, 7:09 a.m.
Message ID <differential-rev-PHID-DREV-z2rjxpbe7exfeqkqqgqo-req@mercurial-scm.org>
Download mbox | patch
Permalink /patch/47091/
State New
Headers show

Comments

phabricator - Sept. 5, 2020, 7:09 a.m.
pulkit created this revision.
Herald added a reviewer: hg-reviewers.
Herald added a subscriber: mercurial-patches.

REVISION SUMMARY
  4d504e541d3d1a139f8ebbf8c405291140fd1853 <https://phab.mercurial-scm.org/rHG4d504e541d3d1a139f8ebbf8c405291140fd1853> introduced an optimization where in
  some cases we first diff ancestor and second parent, get the list of files
  changed between them and then only diff for these files between first parent and
  second parent.
  
  However as the commit message mentions, this optimization is only helpful when
  rebasing very old commits and using treemanifest. On flat manifests, this
  optimization rather adds a bit of overhead.
  
  This makes optimization not much useful on flat manifests and can be disabled.
  
  Now, while working on solving criss-cross merge issues where we want to create
  new filenode for some files in initial merges, this optimization was causing
  issues. Since it's filters file for which diff needs to be performed, the files
  we are concerned with gets filters out and we don't get to store anything
  related to those files.
  
  Hence, want to disabling the optimization for flat manifests.
  
  The tests changes are result of fact that we diff more files now in m1-vs-m2
  comparison.
  
  Small note: all these criss cross and new filenode thing I am talking about will
  be behind a config flag. So nothing invasive yet :)

REPOSITORY
  rHG Mercurial

BRANCH
  default

REVISION DETAIL
  https://phab.mercurial-scm.org/D8982

AFFECTED FILES
  mercurial/merge.py
  tests/test-graft.t
  tests/test-issue1802.t
  tests/test-issue522.t
  tests/test-issue672.t
  tests/test-rebase-conflicts.t
  tests/test-rename-dir-merge.t
  tests/test-rename-merge1.t
  tests/test-rename-merge2.t

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-patches, mercurial-devel

Patch

diff --git a/tests/test-rename-merge2.t b/tests/test-rename-merge2.t
--- a/tests/test-rename-merge2.t
+++ b/tests/test-rename-merge2.t
@@ -88,6 +88,7 @@ 
    preserving a for resolve of b
    preserving rev for resolve of rev
   starting 4 threads for background file closing (?)
+   a: remote unchanged -> k
    b: remote copied from a -> m (premerge)
   picked tool '* ../merge' for b (binary False symlink False changedelete False) (glob)
   merging a and b to b
@@ -276,6 +277,7 @@ 
    ancestor: 924404dff337, local: 86a2aa42fc76+, remote: 97c705ade336
    preserving rev for resolve of rev
   starting 4 threads for background file closing (?)
+   b: ancestor missing, remote missing -> k
    rev: versions differ -> m (premerge)
   picked tool '* ../merge' for rev (binary False symlink False changedelete False) (glob)
   merging rev
@@ -342,6 +344,8 @@ 
    ancestor: 924404dff337, local: 02963e448370+, remote: 97c705ade336
    preserving rev for resolve of rev
   starting 4 threads for background file closing (?)
+   b: ancestor missing, remote missing -> k
+   a: local not present, remote unchanged -> ka
    rev: versions differ -> m (premerge)
   picked tool '* ../merge' for rev (binary False symlink False changedelete False) (glob)
   merging rev
@@ -425,6 +429,7 @@ 
    preserving rev for resolve of rev
    c: remote created -> g
   getting c
+   b: ancestor missing, remote missing -> k
    rev: versions differ -> m (premerge)
   picked tool '* ../merge' for rev (binary False symlink False changedelete False) (glob)
   merging rev
@@ -652,6 +657,7 @@ 
    preserving b for resolve of b
    preserving rev for resolve of rev
   starting 4 threads for background file closing (?)
+   a: remote unchanged -> k
    b: both renamed from a -> m (premerge)
   picked tool '* ../merge' for b (binary False symlink False changedelete False) (glob)
   merging b
diff --git a/tests/test-rename-merge1.t b/tests/test-rename-merge1.t
--- a/tests/test-rename-merge1.t
+++ b/tests/test-rename-merge1.t
@@ -44,6 +44,7 @@ 
   removing a
    b2: remote created -> g
   getting b2
+   c2: ancestor missing, remote missing -> k
    b: remote moved from a -> m (premerge)
   picked tool ':merge' for b (binary False symlink False changedelete False)
   merging a and b to b
diff --git a/tests/test-rename-dir-merge.t b/tests/test-rename-dir-merge.t
--- a/tests/test-rename-dir-merge.t
+++ b/tests/test-rename-dir-merge.t
@@ -86,6 +86,10 @@ 
    branchmerge: True, force: False, partial: False
    ancestor: f9b20c0d4c51, local: 397f8b00a740+, remote: ce36d17b18fb
   starting 4 threads for background file closing (?)
+   b/a: file is rename dest and rename source did not change -> k
+   b/b: file is rename dest and rename source did not change -> k
+   a/a: local not present, remote unchanged -> ka
+   a/b: local not present, remote unchanged -> ka
    b/c: local directory rename - get from a/c -> dg
   getting a/c to b/c
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
diff --git a/tests/test-rebase-conflicts.t b/tests/test-rebase-conflicts.t
--- a/tests/test-rebase-conflicts.t
+++ b/tests/test-rebase-conflicts.t
@@ -254,6 +254,7 @@ 
    ancestor: 8e4e2c1a07ae, local: 4bc80088dc6b+, remote: e31216eec445
    f1.txt: remote is newer -> g
   getting f1.txt
+   f2.txt: local not present, remote unchanged -> ka
   committing files:
   f1.txt
   committing manifest
@@ -271,6 +272,7 @@ 
    ancestor: e31216eec445, local: 19c888675e13+, remote: 2f2496ddf49d
    f1.txt: remote is newer -> g
   getting f1.txt
+   f2.txt: local not present, remote unchanged -> ka
   committing files:
   f1.txt
   committing manifest
diff --git a/tests/test-issue672.t b/tests/test-issue672.t
--- a/tests/test-issue672.t
+++ b/tests/test-issue672.t
@@ -38,6 +38,7 @@ 
   removing 1
    1a: remote created -> g
   getting 1a
+   2: remote unchanged -> k
   1 files updated, 0 files merged, 1 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
 
diff --git a/tests/test-issue522.t b/tests/test-issue522.t
--- a/tests/test-issue522.t
+++ b/tests/test-issue522.t
@@ -30,6 +30,7 @@ 
    ancestor: bbd179dfa0a7, local: 71766447bdbb+, remote: 4d9e78aaceee
    foo: remote is newer -> g
   getting foo
+   bar: ancestor missing, remote missing -> k
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
 
diff --git a/tests/test-issue1802.t b/tests/test-issue1802.t
--- a/tests/test-issue1802.t
+++ b/tests/test-issue1802.t
@@ -55,6 +55,7 @@ 
   resolving manifests
    branchmerge: True, force: False, partial: False
    ancestor: a03b0deabf2b, local: d6fa54f68ae1+, remote: 2d8bcf2dda39
+   b: ancestor missing, remote missing -> k
    a: update permissions -> e
   1 files updated, 0 files merged, 0 files removed, 0 files unresolved
   (branch merge, don't forget to commit)
diff --git a/tests/test-graft.t b/tests/test-graft.t
--- a/tests/test-graft.t
+++ b/tests/test-graft.t
@@ -232,6 +232,8 @@ 
    ancestor: 4c60f11aa304, local: 6b9e5368ca4e+, remote: 97f8bfe72746
    e: remote is newer -> g
   getting e
+   b: remote unchanged -> k
+   c: local not present, remote unchanged -> ka
   committing files:
   e
   committing manifest
@@ -250,6 +252,8 @@ 
    preserving e for resolve of e
    d: remote is newer -> g
   getting d
+   b: remote unchanged -> k
+   c: local not present, remote unchanged -> ka
    e: versions differ -> m (premerge)
   picked tool ':merge' for e (binary False symlink False changedelete False)
   merging e
@@ -758,6 +762,7 @@ 
    branchmerge: True, force: True, partial: False
    ancestor: b592ea63bb0c, local: 7e61b508e709+, remote: 7a4785234d87
   starting 4 threads for background file closing (?)
+   g: local not present, remote unchanged -> ka
   nothing to commit, clearing merge state
   note: graft of 13:7a4785234d87 created no changes to commit
   $ hg log -r 'destination(13)'
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -795,7 +795,23 @@ 
     # - ma is the same as m1 or m2, which we're just going to diff again later
     # - The caller specifically asks for a full diff, which is useful during bid
     #   merge.
-    if pa not in ([wctx, p2] + wctx.parents()) and not forcefulldiff:
+    # - flat manifests are use (reasoning below)
+    #
+    # The optimization is more effective when treemanifests are in play and
+    # this saves the time by avoiding loading almost the entire tree.
+    # Meanwhile this optimization also adds a bit of overhead for flat
+    # manifests especially when rebasing very recent commits because of
+    # additional diff call
+    # (refer commit message of 4d504e541d3d1a139f8ebbf8c405291140fd1853)
+    #
+    # Rebasing recent changesets is more common case than rebasing a cset
+    # which is 10k commits old
+    # Hence let's disable this optimization for flat manifests
+    if (
+        pa not in ([wctx, p2] + wctx.parents())
+        and not forcefulldiff
+        and scmutil.istreemanifest(repo)
+    ):
         # Identify which files are relevant to the merge, so we can limit the
         # total m1-vs-m2 diff to just those files. This has significant
         # performance benefits in large repositories.