This is an archive of the discontinued Mercurial Phabricator instance.

Differential D899

annotate: track whether a particular annotation was the result of a skip
ClosedPublic

Authored by sid0 on Oct 2 2017, 5:35 AM.

Details

Reviewers
indygreg
Group Reviewers
hg-reviewers
Commits
rHG2f5a135b2b04: annotate: track whether a particular annotation was the result of a skip
Summary

We're going to expose this information in the UI in an upcoming patch.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

sid0 created this revision.Oct 2 2017, 5:35 AM
indygreg added a subscriber: indygreg.Oct 2 2017, 6:32 AM

Before I stamp this I'd like an answer to the mutability concerns.

mercurial/context.py
1164–1165

I see that we're copying a ref to the object instead of making an object copy. When we had tuples, that was fine because tuples are immutable. But with attr, instances can be modified.

Will this pose any problems?

1177

Ditto.

sid0 added inline comments.Oct 2 2017, 10:56 AM
mercurial/context.py
1164–1165

Good question! Not in this case, because a particular annotation can never go from skip=True to skip=False. If we decide to overwrite the annotation afterwards, the whole object is replaced, not just fctx and lineno.

sid0 added a comment.Oct 2 2017, 10:57 AM

I'll add a comment explaining that.

sid0 added inline comments.Oct 2 2017, 10:59 AM
mercurial/context.py
1164–1165

Actually -- hmm, you're right, this is a bit risky to code changes in the future -- especially if the same object gets shared between skipped and not-skipped lines. I'll create a new one to be safe.

sid0 updated this revision to Diff 2329.Oct 2 2017, 11:05 AM
sid0 updated this revision to Diff 2331.Oct 2 2017, 11:08 AM
sid0 added a comment.Oct 2 2017, 11:09 AM

Turns out it actually was buggy. Thanks for catching it!

I switched to using attrs.evolve (and in the earlier diff setting frozen=True to make the attr immutable)

indygreg accepted this revision.Oct 2 2017, 1:14 PM

Immutability is a magical property, isn't it ;)

This revision is now accepted and ready to land.Oct 2 2017, 1:14 PM
Closed by commit rHG2f5a135b2b04: annotate: track whether a particular annotation was the result of a skip (authored by sid0). · Explain WhyOct 2 2017, 1:23 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/context.py (4 lines)
Mtests/test-annotate.py (12 lines)

Diff 2320

mercurial/context.py

This is often equivalent to how the data would be expressed on disk. This is often equivalent to how the data would be expressed on disk.
""" """
return self._repo.wwritedata(self.path(), self.data()) return self._repo.wwritedata(self.path(), self.data())
@attr.s(slots=True) @attr.s(slots=True)
class annotateline(object): class annotateline(object):
fctx = attr.ib() fctx = attr.ib()
lineno = attr.ib(default=False) lineno = attr.ib(default=False)
# Whether this annotation was the result of a skip-annotate.
skip = attr.ib(default=False)
def _annotatepair(parents, childfctx, child, skipchild, diffopts): def _annotatepair(parents, childfctx, child, skipchild, diffopts):
r''' r'''
Given parent and child fctxes and annotate data for parents, for all lines Given parent and child fctxes and annotate data for parents, for all lines
in either parent that match the child, annotate the child with the parent's in either parent that match the child, annotate the child with the parent's
data. data.
Additionally, if `skipchild` is True, replace all other lines with parent Additionally, if `skipchild` is True, replace all other lines with parent
# First, replace as much as possible without repeating the last line. # First, replace as much as possible without repeating the last line.
remaining = [(parent, []) for parent, _blocks in pblocks] remaining = [(parent, []) for parent, _blocks in pblocks]
for idx, (parent, blocks) in enumerate(pblocks): for idx, (parent, blocks) in enumerate(pblocks):
for (a1, a2, b1, b2), _t in blocks: for (a1, a2, b1, b2), _t in blocks:
if a2 - a1 >= b2 - b1: if a2 - a1 >= b2 - b1:
for bk in xrange(b1, b2): for bk in xrange(b1, b2):
if child[0][bk].fctx == childfctx: if child[0][bk].fctx == childfctx:
ak = min(a1 + (bk - b1), a2 - 1) ak = min(a1 + (bk - b1), a2 - 1)
child[0][bk] = parent[0][ak] child[0][bk] = parent[0][ak]
child[0][bk].skip = True
indygregUnsubmitted
Not Done

I see that we're copying a ref to the object instead of making an object copy. When we had tuples, that was fine because tuples are immutable. But with attr, instances can be modified.

Will this pose any problems?

indygreg: I see that we're copying a ref to the object instead of making an object copy. When we had…
sid0AuthorUnsubmitted
Not Done

Good question! Not in this case, because a particular annotation can never go from skip=True to skip=False. If we decide to overwrite the annotation afterwards, the whole object is replaced, not just fctx and lineno.

sid0: Good question! Not in this case, because a particular annotation can never go from skip=True to…
sid0AuthorUnsubmitted
Not Done

Actually -- hmm, you're right, this is a bit risky to code changes in the future -- especially if the same object gets shared between skipped and not-skipped lines. I'll create a new one to be safe.

sid0: Actually -- hmm, you're right, this is a bit risky to code changes in the future -- especially…
else: else:
remaining[idx][1].append((a1, a2, b1, b2)) remaining[idx][1].append((a1, a2, b1, b2))
# Then, look at anything left, which might involve repeating the last # Then, look at anything left, which might involve repeating the last
# line. # line.
for parent, blocks in remaining: for parent, blocks in remaining:
for a1, a2, b1, b2 in blocks: for a1, a2, b1, b2 in blocks:
for bk in xrange(b1, b2): for bk in xrange(b1, b2):
if child[0][bk].fctx == childfctx: if child[0][bk].fctx == childfctx:
ak = min(a1 + (bk - b1), a2 - 1) ak = min(a1 + (bk - b1), a2 - 1)
child[0][bk] = parent[0][ak] child[0][bk] = parent[0][ak]
child[0][bk].skip = True
indygregUnsubmitted
Not Done

Ditto.

indygreg: Ditto.
return child return child
class filectx(basefilectx): class filectx(basefilectx):
"""A filecontext object makes access to data related to a particular """A filecontext object makes access to data related to a particular
filerevision convenient.""" filerevision convenient."""
def __init__(self, repo, path, changeid=None, fileid=None, def __init__(self, repo, path, changeid=None, fileid=None,
filelog=None, changectx=None): filelog=None, changectx=None):
"""changeid can be a changeset revision, node, or tag. """changeid can be a changeset revision, node, or tag.

tests/test-annotate.py

# Test with skipchild (note the difference caused by ordering) # Test with skipchild (note the difference caused by ordering)
childann = decorate(childdata, childfctx) childann = decorate(childdata, childfctx)
childann = _annotatepair([p1ann, p2ann], childfctx, childann, True, childann = _annotatepair([p1ann, p2ann], childfctx, childann, True,
diffopts) diffopts)
self.assertEqual(childann[0], [ self.assertEqual(childann[0], [
annotateline('old', 1), annotateline('old', 1),
annotateline('old', 2), annotateline('old', 2, True),
annotateline('p2', 2), annotateline('p2', 2, True),
annotateline('p2', 2), annotateline('p2', 2, True),
annotateline('p2', 3), annotateline('p2', 3),
]) ])
childann = decorate(childdata, childfctx) childann = decorate(childdata, childfctx)
childann = _annotatepair([p2ann, p1ann], childfctx, childann, True, childann = _annotatepair([p2ann, p1ann], childfctx, childann, True,
diffopts) diffopts)
self.assertEqual(childann[0], [ self.assertEqual(childann[0], [
annotateline('old', 1), annotateline('old', 1),
annotateline('old', 2), annotateline('old', 2, True),
annotateline('p1', 3), annotateline('p1', 3, True),
annotateline('p1', 3), annotateline('p1', 3, True),
annotateline('p2', 3), annotateline('p2', 3),
]) ])
if __name__ == '__main__': if __name__ == '__main__':
import silenttestrunner import silenttestrunner
silenttestrunner.main(__name__) silenttestrunner.main(__name__)