This is an archive of the discontinued Mercurial Phabricator instance.

Differential D4416

copies: improve logic of deciding copytracing on based of config options
ClosedPublic

Authored by pulkit on Aug 29 2018, 12:31 PM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
Commits
rHGa41497b5117c: copies: improve logic of deciding copytracing on based of config options
Summary

Few months ago or maybe a year ago, I imported Fb's heuristics based copytracing
algorithms. While importing that, I renamed experimental.disablecopytrace with
experimental.copytrace and the behavior of the new config option was like
this:

  • "heuristics" : Fb's heuristic copytracing algorithm
  • "off" : copytracing is turned off
  • something else: copytracing is on

This is the behavior right now also and this is bad because it hardcodes the
string 'off' to turn off the copytracing. On big repositories, copytracing is
very slow and people wants to turn copytracing off. However if the user sets it
to 'False', 'Off', '0', none of them is going to disbale copytracing while they
should.
I lacked the understanding of why this can be bad when I coded it.

After this patch, the new behavior of the config option will be:

  • "heuristics": Fb's heuristic copytracing algorithm
  • '0', 'false', 'off', 'never', 'no', 'NO', all the values which repo.ui.configbool() evaluates to False: copytracing in turned off
  • something else: copytracing is on

Since 'off' still evaluates to copytracing being turned off, this is not BC.
Also the config option is experimental.

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

pulkit created this revision.Aug 29 2018, 12:31 PM
yuja added a subscriber: yuja.Aug 29 2018, 6:55 PM

check-config complains about bool vs str. parsebool() might help.

pulkit updated this revision to Diff 10677.Aug 30 2018, 8:57 AM
yuja added a comment.Aug 30 2018, 9:33 AM

Queued, thanks.

+ elif boolctrace is False:
+ # stringutil.parsebool() returns None when it is unable to parse the
+ # value, so we should rely on making sure copytracing is on such cases

Yep. Maybe we can add a helper function that returns truthy value if
copytracing is somewhat on.

v = ui.config('experimental', 'copytrace')
if parsebool(v) is False:
    return None
return v
Closed by commit rHGa41497b5117c: copies: improve logic of deciding copytracing on based of config options (authored by pulkit). · Explain WhyAug 30 2018, 9:34 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/copies.py (16 lines)

Diff 10678

mercurial/copies.py

from . import ( from . import (
match as matchmod, match as matchmod,
node, node,
pathutil, pathutil,
scmutil, scmutil,
util, util,
) )
from .utils import (
stringutil,
)
def _findlimit(repo, a, b): def _findlimit(repo, a, b):
""" """
Find the last revision that needs to be checked to ensure that a full Find the last revision that needs to be checked to ensure that a full
transitive closure for file copies can be properly calculated. transitive closure for file copies can be properly calculated.
Generally, this means finding the earliest revision number that's an Generally, this means finding the earliest revision number that's an
ancestor of a or b but not both, except when a or b is a direct descendent ancestor of a or b but not both, except when a or b is a direct descendent
of the other, in which case we can return the minimum revnum of a and b. of the other, in which case we can return the minimum revnum of a and b.
if not c1 or not c2 or c1 == c2: if not c1 or not c2 or c1 == c2:
return {}, {}, {}, {}, {} return {}, {}, {}, {}, {}
# avoid silly behavior for parent -> working dir # avoid silly behavior for parent -> working dir
if c2.node() is None and c1.node() == repo.dirstate.p1(): if c2.node() is None and c1.node() == repo.dirstate.p1():
return repo.dirstate.copies(), {}, {}, {}, {} return repo.dirstate.copies(), {}, {}, {}, {}
copytracing = repo.ui.config('experimental', 'copytrace') copytracing = repo.ui.config('experimental', 'copytrace')
boolctrace = stringutil.parsebool(copytracing)
# Copy trace disabling is explicitly below the node == p1 logic above # Copy trace disabling is explicitly below the node == p1 logic above
# because the logic above is required for a simple copy to be kept across a # because the logic above is required for a simple copy to be kept across a
# rebase. # rebase.
if copytracing == 'off': if copytracing == 'heuristics':
return {}, {}, {}, {}, {}
elif copytracing == 'heuristics':
# Do full copytracing if only non-public revisions are involved as # Do full copytracing if only non-public revisions are involved as
# that will be fast enough and will also cover the copies which could # that will be fast enough and will also cover the copies which could
# be missed by heuristics # be missed by heuristics
if _isfullcopytraceable(repo, c1, base): if _isfullcopytraceable(repo, c1, base):
return _fullcopytracing(repo, c1, c2, base) return _fullcopytracing(repo, c1, c2, base)
return _heuristicscopytracing(repo, c1, c2, base) return _heuristicscopytracing(repo, c1, c2, base)
elif boolctrace is False:
# stringutil.parsebool() returns None when it is unable to parse the
# value, so we should rely on making sure copytracing is on such cases
return {}, {}, {}, {}, {}
else: else:
return _fullcopytracing(repo, c1, c2, base) return _fullcopytracing(repo, c1, c2, base)
def _isfullcopytraceable(repo, c1, base): def _isfullcopytraceable(repo, c1, base):
""" Checks that if base, source and destination are all no-public branches, """ Checks that if base, source and destination are all no-public branches,
if yes let's use the full copytrace algorithm for increased capabilities if yes let's use the full copytrace algorithm for increased capabilities
since it will be fast enough. since it will be fast enough.
"""reproduce copies from fromrev to rev in the dirstate """reproduce copies from fromrev to rev in the dirstate
If skiprev is specified, it's a revision that should be used to If skiprev is specified, it's a revision that should be used to
filter copy records. Any copies that occur between fromrev and filter copy records. Any copies that occur between fromrev and
skiprev will not be duplicated, even if they appear in the set of skiprev will not be duplicated, even if they appear in the set of
copies between fromrev and rev. copies between fromrev and rev.
""" """
exclude = {} exclude = {}
ctraceconfig = repo.ui.config('experimental', 'copytrace')
bctrace = stringutil.parsebool(ctraceconfig)
if (skiprev is not None and if (skiprev is not None and
repo.ui.config('experimental', 'copytrace') != 'off'): (ctraceconfig == 'heuristics' or bctrace or bctrace is None)):
# copytrace='off' skips this line, but not the entire function because # copytrace='off' skips this line, but not the entire function because
# the line below is O(size of the repo) during a rebase, while the rest # the line below is O(size of the repo) during a rebase, while the rest
# of the function is much faster (and is required for carrying copy # of the function is much faster (and is required for carrying copy
# metadata across the rebase anyway). # metadata across the rebase anyway).
exclude = pathcopies(repo[fromrev], repo[skiprev]) exclude = pathcopies(repo[fromrev], repo[skiprev])
for dst, src in pathcopies(repo[fromrev], repo[rev]).iteritems(): for dst, src in pathcopies(repo[fromrev], repo[rev]).iteritems():
# copies.pathcopies returns backward renames, so dst might not # copies.pathcopies returns backward renames, so dst might not
# actually be in the dirstate # actually be in the dirstate
if dst in exclude: if dst in exclude:
continue continue
wctx[dst].markcopied(src) wctx[dst].markcopied(src)