( )⚙ D6561 copies: simplify merging of copy dicts on merge commits

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6561

copies: simplify merging of copy dicts on merge commits
ClosedPublic

Authored by martinvonz on Jun 20 2019, 5:09 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHG907cef396635: copies: simplify merging of copy dicts on merge commits
Summary

After we removed some filtering in 35d674a3d5db (copies: don't filter
out copy targets created on other side of merge commit, 2019-04-18),
we will always include all entries from "copies1", so we can simplify
the code based on that.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Jun 20 2019, 5:09 PM
martinvonz added a commit: rHG907cef396635: copies: simplify merging of copy dicts on merge commits.Jun 24 2019, 2:59 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG907cef396635: copies: simplify merging of copy dicts on merge commits (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Jun 25 2019, 7:28 PM
while work:
  • r, i1, copies1 = heapq.heappop(work)

+ r, i1, copies = heapq.heappop(work)

if work and work[0][0] == r:
    # We are tracing copies from both parents
    r, i2, copies2 = heapq.heappop(work)
  • copies = {}
  • allcopies = set(copies1) | set(copies2)
  • for dst in allcopies:

+ for dst, src in copies2.items():

    1. Unlike when copies are stored in the filelog, we consider
    2. it a copy even if the destination already existed on the
    3. other branch. It's simply too expensive to check if the
    4. file existed in the manifest.
  • if dst in copies1:
  • # If it was copied on the p1 side, mark it as copied from

+ if dst not in copies:
+ # If it was copied on the p1 side, leave it as copied from

    1. that side, even if it was also copied on the p2 side.
  • copies[dst] = copies1[dst]
  • else: copies[dst] = copies2[dst]

Are we sure there's no copies alias held by later work?

I'm just asking because we've optimized some copies.copy()s away at
5ceb91136ebe. I don't know if it's safe or not to mutate copies at this
point.

martinvonz added a comment.Jun 25 2019, 9:08 PM
In D6561#95834, @yuja wrote:
while work:
  • r, i1, copies1 = heapq.heappop(work)

+ r, i1, copies = heapq.heappop(work)

if work and work[0][0] == r:
    # We are tracing copies from both parents
    r, i2, copies2 = heapq.heappop(work)
  • copies = {}
  • allcopies = set(copies1) | set(copies2)
  • for dst in allcopies:

+ for dst, src in copies2.items():

    1. Unlike when copies are stored in the filelog, we consider
    2. it a copy even if the destination already existed on the
    3. other branch. It's simply too expensive to check if the
    4. file existed in the manifest.
  • if dst in copies1:
  • # If it was copied on the p1 side, mark it as copied from

+ if dst not in copies:
+ # If it was copied on the p1 side, leave it as copied from

    1. that side, even if it was also copied on the p2 side.
  • copies[dst] = copies1[dst]
  • else: copies[dst] = copies2[dst]

Are we sure there's no copies alias held by later work?
I'm just asking because we've optimized some copies.copy()s away at
5ceb91136ebe. I don't know if it's safe or not to mutate copies at this
point.

I *think* it should be safe. The policy is that each instance can only appear in one place in the queue. I should send a patch to document that policy. Let me know if you see a violation of that policy.

Revision Contents
Changeset List

PathPackages
Mmercurial/copies.py (14 lines)

Diff 15625

mercurial/copies.py

roots = set(children) - set(missingrevs) roots = set(children) - set(missingrevs)
# 'work' contains 3-tuples of a (revision number, parent number, copies). # 'work' contains 3-tuples of a (revision number, parent number, copies).
# The parent number is only used for knowing which parent the copies dict # The parent number is only used for knowing which parent the copies dict
# came from. # came from.
work = [(r, 1, {}) for r in roots] work = [(r, 1, {}) for r in roots]
heapq.heapify(work) heapq.heapify(work)
alwaysmatch = match.always() alwaysmatch = match.always()
while work: while work:
r, i1, copies1 = heapq.heappop(work) r, i1, copies = heapq.heappop(work)
if work and work[0][0] == r: if work and work[0][0] == r:
# We are tracing copies from both parents # We are tracing copies from both parents
r, i2, copies2 = heapq.heappop(work) r, i2, copies2 = heapq.heappop(work)
copies = {} for dst, src in copies2.items():
allcopies = set(copies1) | set(copies2)
for dst in allcopies:
# Unlike when copies are stored in the filelog, we consider # Unlike when copies are stored in the filelog, we consider
# it a copy even if the destination already existed on the # it a copy even if the destination already existed on the
# other branch. It's simply too expensive to check if the # other branch. It's simply too expensive to check if the
# file existed in the manifest. # file existed in the manifest.
if dst in copies1: if dst not in copies:
# If it was copied on the p1 side, mark it as copied from # If it was copied on the p1 side, leave it as copied from
# that side, even if it was also copied on the p2 side. # that side, even if it was also copied on the p2 side.
copies[dst] = copies1[dst]
else:
copies[dst] = copies2[dst] copies[dst] = copies2[dst]
else:
copies = copies1
if r == b.rev(): if r == b.rev():
_filter(a, b, copies) _filter(a, b, copies)
return copies return copies
for i, c in enumerate(children[r]): for i, c in enumerate(children[r]):
childctx = repo[c] childctx = repo[c]
if r == childctx.p1().rev(): if r == childctx.p1().rev():
parent = 1 parent = 1
childcopies = childctx.p1copies() childcopies = childctx.p1copies()