| 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
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.
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?
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.
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