Patchwork [V2] rebase: fix --collapse when a file was added then removed

login
register
mail settings
Submitter Durham Goode
Date March 15, 2013, 8:34 p.m.
Message ID <2afcd45177c91a315d94.1363379664@dev350.prn1.facebook.com>
Download mbox | patch
Permalink /patch/1131/
State Accepted
Commit 1ef89df2c2487d4957e6d6f8f0d96505eb58ccef
Headers show

Comments

Durham Goode - March 15, 2013, 8:34 p.m.
# HG changeset patch
# User Durham Goode <durham@fb.com>
# Date 1363371809 25200
#      Fri Mar 15 11:23:29 2013 -0700
# Node ID 2afcd45177c91a315d94e80af86e5c281e986da9
# Parent  a039d7868d8546d0f83d46f28e720890a6e9594a
rebase: fix --collapse when a file was added then removed

When a series of commits first adds a file and then removes it,
hg rebase --collapse prompts whether to keep the file or delete it. This is
due to it reusing the branch merge code. In a noninteractive terminal it
defaults to keeping the file, which results in a collapsed commit that is
has a file that should be deleted. This bug resulted in developers accidentally
commiting unintentional changes to our repo twice today, so it's fairly
important to get fixed.

This change allows rebase --collapse to tell the merge code to accept the
latest version every time without prompting.

Adds a test as well.
Durham Goode - March 15, 2013, 8:35 p.m.
This is identical to V1 except I use 'hg manifest' in the test instead of
'ls' for cross platform compatibility.
Bryan O'Sullivan - March 18, 2013, 8:46 p.m.
On Fri, Mar 15, 2013 at 1:34 PM, Durham Goode <durham@fb.com> wrote:

> rebase: fix --collapse when a file was added then removed
>

I've applied this.

But if this is a temporary fix, what will a non-temporary fix look like?
Durham Goode - March 18, 2013, 8:50 p.m.
On 3/18/13 1:46 PM, "Bryan O'Sullivan" <bos@serpentine.com> wrote:

>
>On Fri, Mar 15, 2013 at 1:34 PM, Durham Goode <durham@fb.com> wrote:
>
>But if this is a temporary fix, what will a non-temporary fix look like?
>

The 'temporary fix' comment was there already from whoever added that
parameter originally.  I don't consider my fix temporary.  I just adjusted
the comment to mention my new use case as well.

That said, the merge code could use a bit of refactoring so we don't have
to keep adding parameters whenever we need the behavior to change in
particular cases.

Patch

diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -361,9 +361,10 @@ 
 # writing the files into the working copy and lfcommands.updatelfiles
 # will update the largefiles.
 def overridemanifestmerge(origfn, repo, p1, p2, pa, branchmerge, force,
-                          partial):
+                          partial, acceptremote=False):
     overwrite = force and not branchmerge
-    actions = origfn(repo, p1, p2, pa, branchmerge, force, partial)
+    actions = origfn(repo, p1, p2, pa, branchmerge, force, partial,
+                     acceptremote)
     processed = []
 
     for action in actions:
diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -185,12 +185,14 @@ 
 
     return actions
 
-def manifestmerge(repo, wctx, p2, pa, branchmerge, force, partial):
+def manifestmerge(repo, wctx, p2, pa, branchmerge, force, partial,
+                  acceptremote=False):
     """
     Merge p1 and p2 with ancestor pa and generate merge action list
 
     branchmerge and force are as passed in to update
     partial = function to filter file lists
+    acceptremote = accept the incoming changes without prompting
     """
 
     overwrite = force and not branchmerge
@@ -331,7 +333,9 @@ 
 
     for f, m in sorted(prompts):
         if m == "cd":
-            if repo.ui.promptchoice(
+            if acceptremote:
+                actions.append((f, "r", None, "remote delete"))
+            elif repo.ui.promptchoice(
                 _("local changed %s which remote deleted\n"
                   "use (c)hanged version or (d)elete?") % f,
                 (_("&Changed"), _("&Delete")), 0):
@@ -339,7 +343,9 @@ 
             else:
                 actions.append((f, "a", None, "prompt keep"))
         elif m == "dc":
-            if repo.ui.promptchoice(
+            if acceptremote:
+                actions.append((f, "g", (m2.flags(f),), "remote recreating"))
+            elif repo.ui.promptchoice(
                 _("remote changed %s which local deleted\n"
                   "use (c)hanged version or leave (d)eleted?") % f,
                 (_("&Changed"), _("&Deleted")), 0) == 0:
@@ -512,7 +518,8 @@ 
 
     return updated, merged, removed, unresolved
 
-def calculateupdates(repo, tctx, mctx, ancestor, branchmerge, force, partial):
+def calculateupdates(repo, tctx, mctx, ancestor, branchmerge, force, partial,
+                     acceptremote=False):
     "Calculate the actions needed to merge mctx into tctx"
     actions = []
     folding = not util.checkcase(repo.path)
@@ -526,7 +533,7 @@ 
     actions += manifestmerge(repo, tctx, mctx,
                              ancestor,
                              branchmerge, force,
-                             partial)
+                             partial, acceptremote)
     if tctx.rev() is None:
         actions += _forgetremoved(tctx, mctx, branchmerge)
     return actions
@@ -602,10 +609,11 @@ 
     branchmerge = whether to merge between branches
     force = whether to force branch merging or file overwriting
     partial = a function to filter file lists (dirstate not updated)
-    mergeancestor = if false, merging with an ancestor (fast-forward)
-      is only allowed between different named branches. This flag
-      is used by rebase extension as a temporary fix and should be
-      avoided in general.
+    mergeancestor = whether it is merging with an ancestor. If true,
+      we should accept the incoming changes for any prompts that occur.
+      If false, merging with an ancestor (fast-forward) is only allowed
+      between different named branches. This flag is used by rebase extension
+      as a temporary fix and should be avoided in general.
 
     The table below shows all the behaviors of the update command
     given the -c and -C or no options, whether the working directory
@@ -693,7 +701,7 @@ 
 
         ### calculate phase
         actions = calculateupdates(repo, wc, p2, pa,
-                                   branchmerge, force, partial)
+                                   branchmerge, force, partial, mergeancestor)
 
         ### apply phase
         if not branchmerge: # just jump to the new rev
diff --git a/tests/test-rebase-collapse.t b/tests/test-rebase-collapse.t
--- a/tests/test-rebase-collapse.t
+++ b/tests/test-rebase-collapse.t
@@ -719,6 +719,30 @@ 
 
   $ cd ..
 
+Test collapsing changes that add then remove a file
 
+  $ hg init collapseaddremove
+  $ cd collapseaddremove
 
+  $ touch base
+  $ hg commit -Am base
+  adding base
+  $ touch a
+  $ hg commit -Am a
+  adding a
+  $ hg rm a
+  $ touch b
+  $ hg commit -Am b
+  adding b
+  $ hg rebase -d 0 -r "1::2" --collapse -m collapsed
+  saved backup bundle to $TESTTMP/collapseaddremove/.hg/strip-backup/*-backup.hg (glob)
+  $ hg tglog
+  @  1: 'collapsed'
+  |
+  o  0: 'base'
+  
+  $ hg manifest
+  b
+  base
 
+  $ cd ..
diff --git a/tests/test-rebase-detach.t b/tests/test-rebase-detach.t
--- a/tests/test-rebase-detach.t
+++ b/tests/test-rebase-detach.t
@@ -326,8 +326,6 @@ 
   $ hg ci -m "J"
 
   $ hg rebase -s 8 -d 7 --collapse --config ui.merge=internal:other
-  remote changed E which local deleted
-  use (c)hanged version or leave (d)eleted? c
   saved backup bundle to $TESTTMP/a6/.hg/strip-backup/*-backup.hg (glob)
 
   $ hg tglog