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
<mailto: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
<mailto:boris.f...@octobus.net>> wrote:
# HG changeset patch
# User Boris Feld <boris.f...@octobus.net
<mailto: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?
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
<mailto:Mercurial-devel@mercurial-scm.org>
https://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
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
