This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4284

copies: correctly skip directories that have already been considered
ClosedPublic

Authored by spectral on Aug 15 2018, 5:47 PM.

Details

Reviewers
martinvonz
Group Reviewers
hg-reviewers
Commits
rHGeebd591803ab: copies: correctly skip directories that have already been considered
Summary

Previously, if dsrc in invalid would never be true, since we added
dsrc +"/" to invalid, not dsrc itself. Since it's much more common for
individual files (not whole directories) to be moved, it seemed cleaner to
delay appending the "/" until we know we have some directory moves to
actually consider.

I haven't benchmarked this, but I imagine this is a mild performance win.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

spectral created this revision.Aug 15 2018, 5:47 PM
spectral edited the summary of this revision. (Show Details)Aug 23 2018, 5:08 PM
jpsugar added a subscriber: jpsugar.Aug 24 2018, 3:49 PM
jpsugar added inline comments.
mercurial/copies.py
615

Maybe do this after the debug loop to avoid an un-pretty output?

spectral added inline comments.Aug 24 2018, 4:35 PM
mercurial/copies.py
615

I personally have no preference, but had been trying to make this an unobservable difference, whereas that is technically observable (though I doubt anything is relying on this particular one, we have had reports of tooling like one of the IDEs depending on the format of some of our debug messages in the past)

Asking martinvonz in person, they prefer the slashes on the end to more strongly indicate that this is a directory, especially in the debug messages (which most people will never see).

martinvonz accepted this revision.Aug 24 2018, 4:52 PM
This revision is now accepted and ready to land.Aug 24 2018, 4:52 PM
Closed by commit rHGeebd591803ab: copies: correctly skip directories that have already been considered (authored by spectral). · Explain WhyAug 24 2018, 4:54 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/copies.py (12 lines)

Diff 10561

mercurial/copies.py

# when all the files in a directory are moved to a new directory # when all the files in a directory are moved to a new directory
for dst, src in fullcopy.iteritems(): for dst, src in fullcopy.iteritems():
dsrc, ddst = pathutil.dirname(src), pathutil.dirname(dst) dsrc, ddst = pathutil.dirname(src), pathutil.dirname(dst)
if dsrc in invalid: if dsrc in invalid:
# already seen to be uninteresting # already seen to be uninteresting
continue continue
elif dsrc in d1 and ddst in d1: elif dsrc in d1 and ddst in d1:
# directory wasn't entirely moved locally # directory wasn't entirely moved locally
invalid.add(dsrc + "/") invalid.add(dsrc)
elif dsrc in d2 and ddst in d2: elif dsrc in d2 and ddst in d2:
# directory wasn't entirely moved remotely # directory wasn't entirely moved remotely
invalid.add(dsrc + "/") invalid.add(dsrc)
elif dsrc + "/" in dirmove and dirmove[dsrc + "/"] != ddst + "/": elif dsrc in dirmove and dirmove[dsrc] != ddst:
# files from the same directory moved to two different places # files from the same directory moved to two different places
invalid.add(dsrc + "/") invalid.add(dsrc)
else: else:
# looks good so far # looks good so far
dirmove[dsrc + "/"] = ddst + "/" dirmove[dsrc] = ddst
for i in invalid: for i in invalid:
if i in dirmove: if i in dirmove:
del dirmove[i] del dirmove[i]
del d1, d2, invalid del d1, d2, invalid
if not dirmove: if not dirmove:
return copy, {}, diverge, renamedelete, {} return copy, {}, diverge, renamedelete, {}
dirmove = {k + "/": v + "/" for k, v in dirmove.iteritems()}
jpsugarUnsubmitted
Not Done

Maybe do this after the debug loop to avoid an un-pretty output?

jpsugar: Maybe do this after the debug loop to avoid an un-pretty output?
spectralAuthorUnsubmitted
Not Done

I personally have no preference, but had been trying to make this an unobservable difference, whereas that is technically observable (though I doubt anything is relying on this particular one, we have had reports of tooling like one of the IDEs depending on the format of some of our debug messages in the past)

Asking martinvonz in person, they prefer the slashes on the end to more strongly indicate that this is a directory, especially in the debug messages (which most people will never see).

spectral: I personally have no preference, but had been trying to make this an unobservable difference…
for d in dirmove: for d in dirmove:
repo.ui.debug(" discovered dir src: '%s' -> dst: '%s'\n" % repo.ui.debug(" discovered dir src: '%s' -> dst: '%s'\n" %
(d, dirmove[d])) (d, dirmove[d]))
movewithdir = {} movewithdir = {}
# check unaccounted nonoverlapping files against directory moves # check unaccounted nonoverlapping files against directory moves
for f in u1r + u2r: for f in u1r + u2r:
if f not in fullcopy: if f not in fullcopy: