Patchwork [STABLE] merge: don't try to merge subrepos twice (issue4988)

login
register
mail settings
Submitter Siddharth Agarwal
Date Jan. 29, 2016, 10:22 p.m.
Message ID <4e74815fcb8b3197141c.1454106162@dev666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/12912/
State Accepted
Headers show

Comments

Siddharth Agarwal - Jan. 29, 2016, 10:22 p.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1454105969 28800
#      Fri Jan 29 14:19:29 2016 -0800
# Branch stable
# Node ID 4e74815fcb8b3197141c59fa5b26de1e59d1d0f1
# Parent  7cb7264cfd52d2672644db4bc16a0bd50aa093ca
# Available At http://42.netv6.net/sid0-wip/hg/
#              hg pull http://42.netv6.net/sid0-wip/hg/ -r 4e74815fcb8b
merge: don't try to merge subrepos twice (issue4988)

In my patch series ending with rev 25e4b2f000c5 I switched most change/delete
conflicts to be handled at the resolve layer. .hgsubstate was the one file that
we weren't able to handle, so we kept the old code path around for it.

The old code path added .hgsubstate to one of the other lists as the user
specifies, including possibly the 'g' list.

Now since we did this check after converting the actions from being keyed by
file to being keyed by action type, there was nothing that actually removed
.hgsubstate from the 'cd' or 'dc' lists. This meant that the file would
eventually make its way into the 'mergeactions' list, now freshly augmented
with 'cd' and 'dc' actions.

We call subrepo.submerge for both 'g' actions and merge actions.

This means that if the resolution to an .hgsubstate change/delete conflict was
to add it to the 'g' list, subrepo.submerge would be called twice. It turns out
that his doesn't cause any adverse effects on Linux due to caching, but
apparently breaks on other operating systems including Windows.

The fix here moves this to before we convert the actions over. This ensures
that it .hgsubstate doesn't make its way into multiple lists.

The real fix here is going to be:
(1) move .hgsubstate conflict resolution into the resolve layer, and
(2) use a real data structure for the actions rather than shuffling data around
    between lists and dictionaries: we need a hash (or prefix-based) index by
    file and a list index by action type.

There's a very tiny behavior change here: collision detection on case-sensitive
systems will happen after this is resolved, not before. I think this is the
right change -- .hgsubstate could theoretically collide with other files -- but
in any case it makes no practical difference.

Thanks to Yuya Nishihara for investigating this.
Siddharth Agarwal - Jan. 29, 2016, 10:26 p.m.
On 1/29/16 14:22, Siddharth Agarwal wrote:
> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1454105969 28800
> #      Fri Jan 29 14:19:29 2016 -0800
> # Branch stable
> # Node ID 4e74815fcb8b3197141c59fa5b26de1e59d1d0f1
> # Parent  7cb7264cfd52d2672644db4bc16a0bd50aa093ca
> # Available At http://42.netv6.net/sid0-wip/hg/
> #              hg pull http://42.netv6.net/sid0-wip/hg/ -r 4e74815fcb8b
> merge: don't try to merge subrepos twice (issue4988)
>
> In my patch series ending with rev 25e4b2f000c5 I switched most change/delete
> conflicts to be handled at the resolve layer. .hgsubstate was the one file that
> we weren't able to handle, so we kept the old code path around for it.
>
> The old code path added .hgsubstate to one of the other lists as the user
> specifies, including possibly the 'g' list.
>
> Now since we did this check after converting the actions from being keyed by
> file to being keyed by action type, there was nothing that actually removed
> .hgsubstate from the 'cd' or 'dc' lists. This meant that the file would
> eventually make its way into the 'mergeactions' list, now freshly augmented
> with 'cd' and 'dc' actions.
>
> We call subrepo.submerge for both 'g' actions and merge actions.
>
> This means that if the resolution to an .hgsubstate change/delete conflict was
> to add it to the 'g' list, subrepo.submerge would be called twice. It turns out
> that his doesn't cause any adverse effects on Linux due to caching, but
> apparently breaks on other operating systems including Windows.
>
> The fix here moves this to before we convert the actions over. This ensures
> that it .hgsubstate doesn't make its way into multiple lists.
>
> The real fix here is going to be:
> (1) move .hgsubstate conflict resolution into the resolve layer, and
> (2) use a real data structure for the actions rather than shuffling data around
>      between lists and dictionaries: we need a hash (or prefix-based) index by
>      file and a list index by action type.
>
> There's a very tiny behavior change here: collision detection on case-sensitive

er, case-INsensitive.

> systems will happen after this is resolved, not before. I think this is the
> right change -- .hgsubstate could theoretically collide with other files -- but
> in any case it makes no practical difference.
>
> Thanks to Yuya Nishihara for investigating this.
>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -1464,6 +1464,34 @@ def update(repo, node, branchmerge, forc
>           actionbyfile, diverge, renamedelete = calculateupdates(
>               repo, wc, p2, pas, branchmerge, force, mergeancestor,
>               followcopies, matcher=matcher)
> +
> +        # Prompt and create actions. Most of this is in the resolve phase
> +        # already, but we can't handle .hgsubstate in filemerge or
> +        # subrepo.submerge yet so we have to keep prompting for it.
> +        if '.hgsubstate' in actionbyfile:
> +            f = '.hgsubstate'
> +            m, args, msg = actionbyfile[f]
> +            if m == 'cd':
> +                if repo.ui.promptchoice(
> +                    _("local changed %s which remote deleted\n"
> +                      "use (c)hanged version or (d)elete?"
> +                      "$$ &Changed $$ &Delete") % f, 0):
> +                    actionbyfile[f] = ('r', None, "prompt delete")
> +                elif f in p1:
> +                    actionbyfile[f] = ('am', None, "prompt keep")
> +                else:
> +                    actionbyfile[f] = ('a', None, "prompt keep")
> +            elif m == 'dc':
> +                f1, f2, fa, move, anc = args
> +                flags = p2[f2].flags()
> +                if repo.ui.promptchoice(
> +                    _("remote changed %s which local deleted\n"
> +                      "use (c)hanged version or leave (d)eleted?"
> +                      "$$ &Changed $$ &Deleted") % f, 0) == 0:
> +                    actionbyfile[f] = ('g', (flags, False), "prompt recreating")
> +                else:
> +                    del actionbyfile[f]
> +
>           # Convert to dictionary-of-lists format
>           actions = dict((m, []) for m in 'a am f g cd dc r dm dg m e k'.split())
>           for f, (m, args, msg) in actionbyfile.iteritems():
> @@ -1479,33 +1507,6 @@ def update(repo, node, branchmerge, forc
>               else:
>                   _checkcollision(repo, wc.manifest(), actions)
>   
> -        # Prompt and create actions. Most of this is in the resolve phase
> -        # already, but we can't handle .hgsubstate in filemerge or
> -        # subrepo.submerge yet so we have to keep prompting for it.
> -        for f, args, msg in sorted(actions['cd']):
> -            if f != '.hgsubstate':
> -                continue
> -            if repo.ui.promptchoice(
> -                _("local changed %s which remote deleted\n"
> -                  "use (c)hanged version or (d)elete?"
> -                  "$$ &Changed $$ &Delete") % f, 0):
> -                actions['r'].append((f, None, "prompt delete"))
> -            elif f in p1:
> -                actions['am'].append((f, None, "prompt keep"))
> -            else:
> -                actions['a'].append((f, None, "prompt keep"))
> -
> -        for f, args, msg in sorted(actions['dc']):
> -            if f != '.hgsubstate':
> -                continue
> -            f1, f2, fa, move, anc = args
> -            flags = p2[f2].flags()
> -            if repo.ui.promptchoice(
> -                _("remote changed %s which local deleted\n"
> -                  "use (c)hanged version or leave (d)eleted?"
> -                  "$$ &Changed $$ &Deleted") % f, 0) == 0:
> -                actions['g'].append((f, (flags, False), "prompt recreating"))
> -
>           # divergent renames
>           for f, fl in sorted(diverge.iteritems()):
>               repo.ui.warn(_("note: possible conflict - %s was renamed "
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Pierre-Yves David - Jan. 30, 2016, 2:38 p.m.
On 01/29/2016 11:26 PM, Siddharth Agarwal wrote:
> On 1/29/16 14:22, Siddharth Agarwal wrote:
>> # HG changeset patch
>> # User Siddharth Agarwal <sid0@fb.com>
>> # Date 1454105969 28800
>> #      Fri Jan 29 14:19:29 2016 -0800
>> # Branch stable
>> # Node ID 4e74815fcb8b3197141c59fa5b26de1e59d1d0f1
>> # Parent  7cb7264cfd52d2672644db4bc16a0bd50aa093ca
>> # Available At http://42.netv6.net/sid0-wip/hg/
>> #              hg pull http://42.netv6.net/sid0-wip/hg/ -r 4e74815fcb8b
>> merge: don't try to merge subrepos twice (issue4988)

The insensitive version have been pushed to the clowncopter, thanks.

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -1464,6 +1464,34 @@  def update(repo, node, branchmerge, forc
         actionbyfile, diverge, renamedelete = calculateupdates(
             repo, wc, p2, pas, branchmerge, force, mergeancestor,
             followcopies, matcher=matcher)
+
+        # Prompt and create actions. Most of this is in the resolve phase
+        # already, but we can't handle .hgsubstate in filemerge or
+        # subrepo.submerge yet so we have to keep prompting for it.
+        if '.hgsubstate' in actionbyfile:
+            f = '.hgsubstate'
+            m, args, msg = actionbyfile[f]
+            if m == 'cd':
+                if repo.ui.promptchoice(
+                    _("local changed %s which remote deleted\n"
+                      "use (c)hanged version or (d)elete?"
+                      "$$ &Changed $$ &Delete") % f, 0):
+                    actionbyfile[f] = ('r', None, "prompt delete")
+                elif f in p1:
+                    actionbyfile[f] = ('am', None, "prompt keep")
+                else:
+                    actionbyfile[f] = ('a', None, "prompt keep")
+            elif m == 'dc':
+                f1, f2, fa, move, anc = args
+                flags = p2[f2].flags()
+                if repo.ui.promptchoice(
+                    _("remote changed %s which local deleted\n"
+                      "use (c)hanged version or leave (d)eleted?"
+                      "$$ &Changed $$ &Deleted") % f, 0) == 0:
+                    actionbyfile[f] = ('g', (flags, False), "prompt recreating")
+                else:
+                    del actionbyfile[f]
+
         # Convert to dictionary-of-lists format
         actions = dict((m, []) for m in 'a am f g cd dc r dm dg m e k'.split())
         for f, (m, args, msg) in actionbyfile.iteritems():
@@ -1479,33 +1507,6 @@  def update(repo, node, branchmerge, forc
             else:
                 _checkcollision(repo, wc.manifest(), actions)
 
-        # Prompt and create actions. Most of this is in the resolve phase
-        # already, but we can't handle .hgsubstate in filemerge or
-        # subrepo.submerge yet so we have to keep prompting for it.
-        for f, args, msg in sorted(actions['cd']):
-            if f != '.hgsubstate':
-                continue
-            if repo.ui.promptchoice(
-                _("local changed %s which remote deleted\n"
-                  "use (c)hanged version or (d)elete?"
-                  "$$ &Changed $$ &Delete") % f, 0):
-                actions['r'].append((f, None, "prompt delete"))
-            elif f in p1:
-                actions['am'].append((f, None, "prompt keep"))
-            else:
-                actions['a'].append((f, None, "prompt keep"))
-
-        for f, args, msg in sorted(actions['dc']):
-            if f != '.hgsubstate':
-                continue
-            f1, f2, fa, move, anc = args
-            flags = p2[f2].flags()
-            if repo.ui.promptchoice(
-                _("remote changed %s which local deleted\n"
-                  "use (c)hanged version or leave (d)eleted?"
-                  "$$ &Changed $$ &Deleted") % f, 0) == 0:
-                actions['g'].append((f, (flags, False), "prompt recreating"))
-
         # divergent renames
         for f, fl in sorted(diverge.iteritems()):
             repo.ui.warn(_("note: possible conflict - %s was renamed "