Btw, just because I found this patch hard to follow doesn't necessarily mean that others do. I won't mind if someone else understands what it does and queues it (and a third person adds a second accept stamp).
On Tue, Oct 2, 2018 at 5:46 AM Martin von Zweigbergk <martinv...@google.com> wrote: > > > On Tue, Oct 2, 2018, 01:58 Boris FELD <boris.f...@octobus.net> wrote: > >> >> On 01/10/2018 18:43, Martin von Zweigbergk via Mercurial-devel wrote: >> >> >> >> On Mon, Oct 1, 2018 at 9:11 AM Boris FELD <boris.f...@octobus.net> wrote: >> >>> On 10/09/2018 18:21, Martin von Zweigbergk via Mercurial-devel wrote: >>> >>> >>> >>> On Fri, Sep 7, 2018 at 8:09 AM Boris Feld <boris.f...@octobus.net> >>> wrote: >>> >>>> # HG changeset patch >>>> # User Boris Feld <boris.f...@octobus.net> >>>> # Date 1536254177 14400 >>>> # Thu Sep 06 13:16:17 2018 -0400 >>>> # Node ID a4c3eb6c1a36cbbf64fa8930b173154b2e77ef2b >>>> # Parent 9a18509c522deeb62a7b244dcf4c7b79a8dc1132 >>>> # EXP-Topic copy-perf >>>> # Available At https://bitbucket.org/octobus/mercurial-devel/ >>>> # hg pull https://bitbucket.org/octobus/mercurial-devel/ >>>> -r a4c3eb6c1a36 >>>> context: fix introrev to avoid computation as initially intended >>>> >>>> diff --git a/mercurial/context.py b/mercurial/context.py >>>> --- a/mercurial/context.py >>>> +++ b/mercurial/context.py >>>> @@ -829,6 +829,23 @@ class basefilectx(object): >>>> # result is crash somewhere else at to some point. >>>> return lkr >>>> >>>> + def _lazyrev(self): >>>> >>> >>> We usually try to separate refactoring (like extracting a method) from >>> functional (or performance-related) changes. Could you do that with this >>> patch or does it not make sense for some reason? >>> >>> In this case, the two changes are a bit too interleaved to be easily >>> split in two. We can't do the special casing until we have the new method >>> and the new method can't have any caller without changing the conditionals >>> to include the special casing. >>> >> >> Maybe I missed something, but it looks like _lazyrev() returns either >> "None" or "self.rev()", even at the end of the series. Did I get that >> right? Will that change later? If not, it seems like you could instead >> extract a method that calculates what's currently called "noctx". Even if >> that's going to change, it might make it easier to understand this patch if >> you split out a patch that made the structure here more similar to your >> goal, something like: >> >> >> @@ -837,9 +837,13 @@ class basefilectx(object): >> lkr = self.linkrev() >> attrs = vars(self) >> noctx = not (r'_changeid' in attrs or r'_changectx' in attrs) >> - if noctx or self.rev() == lkr: >> + if noctx: >> return self.linkrev() >> - return self._adjustlinkrev(self.rev(), inclusive=True) >> + else: >> + if self.rev() == lkr: >> + return self.linkrev() >> + else: >> + return self._adjustlinkrev(self.rev(), inclusive=True) >> >> Yes, you are right, we can split the changeset in two. >> >> I don't feel that it would help the readability of the series but I'm not >> the reviewer. Do you think it would help you review the patch? >> >> > Yes, I think it would. I did it myself in order to understand this patch. > I think it would also help to make that return just a boolean value, > assuming that will still work with later patches. Thanks. > > > >> >>> >>> _______________________________________________ >>> Mercurial-devel mailing >>> listMercurial-devel@mercurial-scm.orghttps://www.mercurial-scm.org/mailman/listinfo/mercurial-devel >>> >>> >> _______________________________________________ >> Mercurial-devel mailing >> listMercurial-devel@mercurial-scm.orghttps://www.mercurial-scm.org/mailman/listinfo/mercurial-devel >> >>
_______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel