[PATCH 3 of 3] patch: rewrite reversehunks (issue5337)

Sean Farley sean at farley.io
Wed Jun 21 18:19:56 UTC 2017


Martin von Zweigbergk <martinvonz at google.com> writes:

> On Wed, Jun 21, 2017 at 10:30 AM, Sean Farley <sean at farley.io> wrote:
>> Martin von Zweigbergk via Mercurial-devel
>> <mercurial-devel at mercurial-scm.org> writes:
>>
>>> On Tue, Jun 20, 2017 at 8:45 PM, Jun Wu <quark at fb.com> wrote:
>>>> # HG changeset patch
>>>> # User Jun Wu <quark at 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 :-)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.mercurial-scm.org/pipermail/mercurial-devel/attachments/20170621/adb5ace7/attachment.asc>


More information about the Mercurial-devel mailing list