Patchwork merge: remove files with extra actions from merge action list

login
register
mail settings
Submitter Siddharth Agarwal
Date Aug. 24, 2016, 1:01 a.m.
Message ID <6cae957b9be33ddd8e9d.1472000506@devvm14349.prn1.facebook.com>
Download mbox | patch
Permalink /patch/16398/
State Accepted
Headers show

Comments

Siddharth Agarwal - Aug. 24, 2016, 1:01 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1472000333 25200
#      Tue Aug 23 17:58:53 2016 -0700
# Node ID 6cae957b9be33ddd8e9d1246626a8908456b1aef
# Parent  b1809f5d7630a3fff0fa715bbd30dba0f07672a8
merge: remove files with extra actions from merge action list

See the comment for a detailed explanation why.

Even though this is a bug, I've sent it to 'default' rather than 'stable'
because it isn't triggered in any code paths in stock Mercurial, just with the
merge driver included. For the same reason I haven't included any tests here --
the merge driver is getting a new test.
via Mercurial-devel - Aug. 24, 2016, 5:09 a.m.
Not pretty, but I don't have a better solution. Queued, thanks.

On Tue, Aug 23, 2016 at 6:01 PM, Siddharth Agarwal <sid0@fb.com> wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1472000333 25200
> #      Tue Aug 23 17:58:53 2016 -0700
> # Node ID 6cae957b9be33ddd8e9d1246626a8908456b1aef
> # Parent  b1809f5d7630a3fff0fa715bbd30dba0f07672a8
> merge: remove files with extra actions from merge action list
>
> See the comment for a detailed explanation why.
>
> Even though this is a bug, I've sent it to 'default' rather than 'stable'
> because it isn't triggered in any code paths in stock Mercurial, just with the
> merge driver included. For the same reason I haven't included any tests here --
> the merge driver is getting a new test.
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -1286,8 +1286,29 @@ def applyupdates(repo, actions, wctx, mc
>      removed += msremoved
>
>      extraactions = ms.actions()
> -    for k, acts in extraactions.iteritems():
> -        actions[k].extend(acts)
> +    if extraactions:
> +        mfiles = set(a[0] for a in actions['m'])
> +        for k, acts in extraactions.iteritems():
> +            actions[k].extend(acts)
> +            # Remove these files from actions['m'] as well. This is important
> +            # because in recordupdates, files in actions['m'] are processed
> +            # after files in other actions, and the merge driver might add
> +            # files to those actions via extraactions above. This can lead to a
> +            # file being recorded twice, with poor results. This is especially
> +            # problematic for actions['r'] (currently only possible with the
> +            # merge driver in the initial merge process; interrupted merges
> +            # don't go through this flow).
> +            #
> +            # The real fix here is to have indexes by both file and action so
> +            # that when the action for a file is changed it is automatically
> +            # reflected in the other action lists. But that involves a more
> +            # complex data structure, so this will do for now.
> +            #
> +            # We don't need to do the same operation for 'dc' and 'cd' because
> +            # those lists aren't consulted again.
> +            mfiles.difference_update(a[0] for a in acts)
> +
> +        actions['m'] = [a for a in actions['m'] if a[0] in mfiles]
>
>      progress(_updating, None, total=numupdates, unit=_files)
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1286,8 +1286,29 @@  def applyupdates(repo, actions, wctx, mc
     removed += msremoved
 
     extraactions = ms.actions()
-    for k, acts in extraactions.iteritems():
-        actions[k].extend(acts)
+    if extraactions:
+        mfiles = set(a[0] for a in actions['m'])
+        for k, acts in extraactions.iteritems():
+            actions[k].extend(acts)
+            # Remove these files from actions['m'] as well. This is important
+            # because in recordupdates, files in actions['m'] are processed
+            # after files in other actions, and the merge driver might add
+            # files to those actions via extraactions above. This can lead to a
+            # file being recorded twice, with poor results. This is especially
+            # problematic for actions['r'] (currently only possible with the
+            # merge driver in the initial merge process; interrupted merges
+            # don't go through this flow).
+            #
+            # The real fix here is to have indexes by both file and action so
+            # that when the action for a file is changed it is automatically
+            # reflected in the other action lists. But that involves a more
+            # complex data structure, so this will do for now.
+            #
+            # We don't need to do the same operation for 'dc' and 'cd' because
+            # those lists aren't consulted again.
+            mfiles.difference_update(a[0] for a in acts)
+
+        actions['m'] = [a for a in actions['m'] if a[0] in mfiles]
 
     progress(_updating, None, total=numupdates, unit=_files)