Patchwork [V2] patch: rewrite reversehunks (issue5337)

login
register
mail settings
Submitter Jun Wu
Date June 21, 2017, 6:23 a.m.
Message ID <f54272510c4ec915bc13.1498026219@x1c>
Download mbox | patch
Permalink /patch/21580/
State Accepted
Headers show

Comments

Jun Wu - June 21, 2017, 6:23 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1498026158 25200
#      Tue Jun 20 23:22:38 2017 -0700
# Node ID f54272510c4ec915bc13b1b9732d148cd1decdd0
# Parent  0ce2cbebd74964ffe61e79de8941461bccc9371b
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r f54272510c4e
patch: rewrite reversehunks (issue5337)

The old reversehunks code accesses "crecord.uihunk._hunk", which is the raw
recordhunk without crecord selection information, therefore "revert -i"
cannot revert individual lines, aka. issue5337.

The patch rewrites related logic to return the right reverse hunk for
revert. Namely,

 1. "fromline" and "toline" are correctly swapped [1]
 2. crecord.uihunk generates a correct reverse hunk [2]

Besides, reversehunks(hunks) will no longer modify its input "hunks", which
is more expected.

[1]: To explain why "fromline" and "toline" need to be swapped, take the
     following example:

  $ cat > a <<EOF
  > 1
  > 2
  > 3
  > 4
  > EOF

  $ cat > b <<EOF
  > 2
  > 3
  > 5
  > EOF

  $ diff a b
  1d0   <---- "1" is "fromline" and "0" is "toline"
  < 1         and they are swapped if diff from the reversed direction
  4c3             |
  < 4             |
  ---             |
  > 5             |
                  |
  $ diff b a      |
  0a1   <---------+
  > 1
  3c4   <---- also "4c3" gets swapped to "3c4"
  < 5
  ---
  > 4

[2]: This is a bit tricky.

For example, given a file which is empty in working parent but has 3 lines
in working copy, and the user selection:

    select hunk to discard
    [x] +1
    [ ] +2
    [x] +3

The user intent is to drop "1" and "3" in working copy but keep "2", so the
reverse patch would be something like:

        -1
         2 (2 is a "context line")
        -3

We cannot just take all selected lines and swap "-" and "+", which will be:

        -1
        -3

That patch won't apply because of "2". So the correct way is to insert "2"
as a "context line" by inserting it first then deleting it:

        -2
        +2

Therefore, the correct revert patch is:

        -1
        -2
        +2
        -3

It could be reordered to look more like a common diff hunk:

        -1
        -2
        -3
        +2

Note: It's possible to return multiple hunks so there won't be lines like
"-2", "+2". But the current implementation is much simpler.

For deletions, like the working parent has "1\n2\n3\n" and it was changed to
empty in working copy:

    select hunk to discard
    [x] -1
    [ ] -2
    [x] -3

The user intent is to drop the deletion of 1 and 3 (in other words, keep
those lines), but still delete "2".

The reverse patch is meant to be applied to working copy which is empty.
So the patch would be:

        +1
        +3

That is to say, there is no need to special handle the unselected "2" like
the above insertion case.
via Mercurial-devel - June 21, 2017, 7:07 a.m.
On Tue, Jun 20, 2017 at 11:23 PM, Jun Wu <quark@fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1498026158 25200
> #      Tue Jun 20 23:22:38 2017 -0700
> # Node ID f54272510c4ec915bc13b1b9732d148cd1decdd0
> # Parent  0ce2cbebd74964ffe61e79de8941461bccc9371b
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r f54272510c4e
> patch: rewrite reversehunks (issue5337)

I've run into that issue many times (until I learned to work around
it), so big thanks for fixing it.

>
> The old reversehunks code accesses "crecord.uihunk._hunk", which is the raw
> recordhunk without crecord selection information, therefore "revert -i"
> cannot revert individual lines, aka. issue5337.

Ooh, that was what was going on... Thanks for explaining. The examples
in the footnotes were very helpful too.

It's getting late for me, so I'll have to finish the review tomorrow.
via Mercurial-devel - June 21, 2017, 8:10 p.m.
On Tue, Jun 20, 2017 at 11:23 PM, Jun Wu <quark@fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1498026158 25200
> #      Tue Jun 20 23:22:38 2017 -0700
> # Node ID f54272510c4ec915bc13b1b9732d148cd1decdd0
> # Parent  0ce2cbebd74964ffe61e79de8941461bccc9371b
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r f54272510c4e
> patch: rewrite reversehunks (issue5337)

Queued. Thanks again for fixing this!

Patch

diff --git a/mercurial/crecord.py b/mercurial/crecord.py
--- a/mercurial/crecord.py
+++ b/mercurial/crecord.py
@@ -428,4 +428,52 @@  class uihunk(patchnode):
         return x.getvalue()
 
+    def reversehunk(self):
+        """return a recordhunk which is the reverse of the hunk
+
+        Assuming the displayed patch is diff(A, B) result. The returned hunk is
+        intended to be applied to B, instead of A.
+
+        For example, when A is "0\n1\n2\n6\n" and B is "0\n3\n4\n5\n6\n", and
+        the user made the following selection:
+
+                 0
+            [x] -1           [x]: selected
+            [ ] -2           [ ]: not selected
+            [x] +3
+            [ ] +4
+            [x] +5
+                 6
+
+        This function returns a hunk like:
+
+                 0
+                -3
+                -4
+                -5
+                +1
+                +4
+                 6
+
+        Note "4" was first deleted then added. That's because "4" exists in B
+        side and "-4" must exist between "-3" and "-5" to make the patch
+        applicable to B.
+        """
+        dels = []
+        adds = []
+        for line in self.changedlines:
+            text = line.linetext
+            if line.applied:
+                if text[0] == '+':
+                    dels.append(text[1:])
+                elif text[0] == '-':
+                    adds.append(text[1:])
+            elif text[0] == '+':
+                dels.append(text[1:])
+                adds.append(text[1:])
+        hunk = ['-%s' % l for l in dels] + ['+%s' % l for l in adds]
+        h = self._hunk
+        return patchmod.recordhunk(h.header, h.toline, h.fromline, h.proc,
+                                   h.before, hunk, h.after)
+
     def __getattr__(self, name):
         return getattr(self._hunk, name)
diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -960,4 +960,16 @@  class recordhunk(object):
         return add, rem
 
+    def reversehunk(self):
+        """return another recordhunk which is the reverse of the hunk
+
+        If this hunk is diff(A, B), the returned hunk is diff(B, A). To do
+        that, swap fromline/toline and +/- signs while keep other things
+        unchanged.
+        """
+        m = {'+': '-', '-': '+'}
+        hunk = ['%s%s' % (m[l[0]], l[1:]) for l in self.hunk]
+        return recordhunk(self.header, self.toline, self.fromline, self.proc,
+                          self.before, hunk, self.after)
+
     def write(self, fp):
         delta = len(self.before) + len(self.after)
@@ -1494,5 +1506,5 @@  def reversehunks(hunks):
      1
      2
-    @@ -1,6 +2,6 @@
+    @@ -2,6 +1,6 @@
      c
      1
@@ -1502,5 +1514,5 @@  def reversehunks(hunks):
      5
      d
-    @@ -5,3 +6,2 @@
+    @@ -6,3 +5,2 @@
      5
      d
@@ -1509,17 +1521,8 @@  def reversehunks(hunks):
     '''
 
-    from . import crecord as crecordmod
     newhunks = []
     for c in hunks:
-        if isinstance(c, crecordmod.uihunk):
-            # curses hunks encapsulate the record hunk in _hunk
-            c = c._hunk
-        if isinstance(c, recordhunk):
-            for j, line in enumerate(c.hunk):
-                if line.startswith("-"):
-                    c.hunk[j] = "+" + c.hunk[j][1:]
-                elif line.startswith("+"):
-                    c.hunk[j] = "-" + c.hunk[j][1:]
-            c.added, c.removed = c.removed, c.added
+        if util.safehasattr(c, 'reversehunk'):
+            c = c.reversehunk()
         newhunks.append(c)
     return newhunks