Patchwork [3,of,3] patch: rewrite reversehunks (issue5337)

login
register
mail settings
Submitter Jun Wu
Date June 21, 2017, 3:45 a.m.
Message ID <214414ec005223245104.1498016734@x1c>
Download mbox | patch
Permalink /patch/21574/
State Changes Requested
Headers show

Comments

Jun Wu - June 21, 2017, 3:45 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1498014757 25200
#      Tue Jun 20 20:12:37 2017 -0700
# Node ID 214414ec00522324510466ff63f265db6b2b0358
# Parent  4fddabb1c843b8068063c98cabbb55b3b9276d64
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r 214414ec0052
patch: rewrite reversehunks (issue5337)
via Mercurial-devel - June 21, 2017, 5:21 a.m.
On Tue, Jun 20, 2017 at 8:45 PM, Jun Wu <quark@fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1498014757 25200
> #      Tue Jun 20 20:12:37 2017 -0700
> # Node ID 214414ec00522324510466ff63f265db6b2b0358
> # Parent  4fddabb1c843b8068063c98cabbb55b3b9276d64
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 214414ec0052
> patch: rewrite reversehunks (issue5337)

Please explain a bit how this solves the problem. I'm sure it's
obvious to you, but this series is a complete mystery to me. Sure, I
could read the code and puzzle it all together, but since you already
know what it's for, it would be more efficient for us if you
explained.

Also, it feels like these three patches belong in a single patch, but
maybe it will be clearer what the purpose of each patch is once you've
explained it.
Sean Farley - June 21, 2017, 5:30 p.m.
Martin von Zweigbergk via Mercurial-devel
<mercurial-devel@mercurial-scm.org> writes:

> On Tue, Jun 20, 2017 at 8:45 PM, Jun Wu <quark@fb.com> wrote:
>> # HG changeset patch
>> # User Jun Wu <quark@fb.com>
>> # Date 1498014757 25200
>> #      Tue Jun 20 20:12:37 2017 -0700
>> # Node ID 214414ec00522324510466ff63f265db6b2b0358
>> # Parent  4fddabb1c843b8068063c98cabbb55b3b9276d64
>> # Available At https://bitbucket.org/quark-zju/hg-draft
>> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 214414ec0052
>> patch: rewrite reversehunks (issue5337)
>
> Please explain a bit how this solves the problem. I'm sure it's
> obvious to you, but this series is a complete mystery to me. Sure, I
> could read the code and puzzle it all together, but since you already
> know what it's for, it would be more efficient for us if you
> explained.
>
> Also, it feels like these three patches belong in a single patch, but
> maybe it will be clearer what the purpose of each patch is once you've
> explained it.

For what it's worth, I think the patches are split just fine. It's our
usual add function X, add function Y, change function Z to use X+Y, no?
via Mercurial-devel - June 21, 2017, 5:37 p.m.
On Wed, Jun 21, 2017 at 10:30 AM, Sean Farley <sean@farley.io> wrote:
> Martin von Zweigbergk via Mercurial-devel
> <mercurial-devel@mercurial-scm.org> writes:
>
>> On Tue, Jun 20, 2017 at 8:45 PM, Jun Wu <quark@fb.com> wrote:
>>> # HG changeset patch
>>> # User Jun Wu <quark@fb.com>
>>> # Date 1498014757 25200
>>> #      Tue Jun 20 20:12:37 2017 -0700
>>> # Node ID 214414ec00522324510466ff63f265db6b2b0358
>>> # Parent  4fddabb1c843b8068063c98cabbb55b3b9276d64
>>> # Available At https://bitbucket.org/quark-zju/hg-draft
>>> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 214414ec0052
>>> patch: rewrite reversehunks (issue5337)
>>
>> Please explain a bit how this solves the problem. I'm sure it's
>> obvious to you, but this series is a complete mystery to me. Sure, I
>> could read the code and puzzle it all together, but since you already
>> know what it's for, it would be more efficient for us if you
>> explained.
>>
>> Also, it feels like these three patches belong in a single patch, but
>> maybe it will be clearer what the purpose of each patch is once you've
>> explained it.
>
> For what it's worth, I think the patches are split just fine. It's our
> usual add function X, add function Y, change function Z to use X+Y, no?

Yes, I think we'll just have to agree to disagree here. I find the
single patch that adds the functions and updates the callers to be
much easier to review (because I don't have to have three windows open
to see them at once), but I know many others like to look at smaller
pieces at once.

In this case, I might personally have split it up into two patches,
but in a different way: 1) replace isinstance() checks by class
methods and safehasattr(), and 2) fix bug in uihunk.reversehunk().
That would have made for two pretty simple patches without need to go
back and forth to get the whole picture.
Sean Farley - June 21, 2017, 6:19 p.m.
Martin von Zweigbergk <martinvonz@google.com> writes:

> On Wed, Jun 21, 2017 at 10:30 AM, Sean Farley <sean@farley.io> wrote:
>> Martin von Zweigbergk via Mercurial-devel
>> <mercurial-devel@mercurial-scm.org> writes:
>>
>>> On Tue, Jun 20, 2017 at 8:45 PM, Jun Wu <quark@fb.com> wrote:
>>>> # HG changeset patch
>>>> # User Jun Wu <quark@fb.com>
>>>> # Date 1498014757 25200
>>>> #      Tue Jun 20 20:12:37 2017 -0700
>>>> # Node ID 214414ec00522324510466ff63f265db6b2b0358
>>>> # Parent  4fddabb1c843b8068063c98cabbb55b3b9276d64
>>>> # Available At https://bitbucket.org/quark-zju/hg-draft
>>>> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 214414ec0052
>>>> patch: rewrite reversehunks (issue5337)
>>>
>>> Please explain a bit how this solves the problem. I'm sure it's
>>> obvious to you, but this series is a complete mystery to me. Sure, I
>>> could read the code and puzzle it all together, but since you already
>>> know what it's for, it would be more efficient for us if you
>>> explained.
>>>
>>> Also, it feels like these three patches belong in a single patch, but
>>> maybe it will be clearer what the purpose of each patch is once you've
>>> explained it.
>>
>> For what it's worth, I think the patches are split just fine. It's our
>> usual add function X, add function Y, change function Z to use X+Y, no?
>
> Yes, I think we'll just have to agree to disagree here. I find the
> single patch that adds the functions and updates the callers to be
> much easier to review (because I don't have to have three windows open
> to see them at once), but I know many others like to look at smaller
> pieces at once.
>
> In this case, I might personally have split it up into two patches,
> but in a different way: 1) replace isinstance() checks by class
> methods and safehasattr(), and 2) fix bug in uihunk.reversehunk().
> That would have made for two pretty simple patches without need to go
> back and forth to get the whole picture.

Yeah, splitting into two would be fine and perhaps something we can both
agree on :-)

Patch

diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -1500,5 +1500,5 @@  def reversehunks(hunks):
      1
      2
-    @@ -1,6 +2,6 @@
+    @@ -2,6 +1,6 @@
      c
      1
@@ -1508,5 +1508,5 @@  def reversehunks(hunks):
      5
      d
-    @@ -5,3 +6,2 @@
+    @@ -6,3 +5,2 @@
      5
      d
@@ -1515,17 +1515,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