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

Martin von Zweigbergk martinvonz at google.com
Wed Jun 21 13:37:17 EDT 2017


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.


More information about the Mercurial-devel mailing list