( )⚙ D6837 cleanup: fix leakage of dirstate._map to client code

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6837

cleanup: fix leakage of dirstate._map to client code
ClosedPublic

Authored by durin42 on Sep 10 2019, 10:01 AM.

Details

Reviewers
indygreg
pulkit
Group Reviewers
hg-reviewers
Commits
rHG34ed651ba7e4: cleanup: fix leakage of dirstate._map to client code
Summary

We already had proper accessors for most of the behavior of
dirstate._map that callers cared about exposed in the actual dirstate
class as public methods. Sigh.

There are two remaining privacy violations in the codebase after this change:

  1. In the perf extension, which I suspect has to stick around because it's really testing the dirstate implementation directly
  2. In largefiles, where we deal with standins and mutating status. Looking at this, I _strongly_ suspect a formal dirstate interface would allow this to work via composition instead of inheritance and monkeypatching. Fortunately, such wins are a part of my motivation for this work. I anticipate we'll come back to this in due time.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

durin42 created this revision.Sep 10 2019, 10:01 AM
mharbison72 added a subscriber: mharbison72.Sep 10 2019, 12:23 PM
  1. In lfs, where we deal with standins and mutating status.

s/lfs/largefiles/?

durin42 edited the summary of this revision. (Show Details)Sep 11 2019, 9:53 AM
durin42 added a comment.Sep 11 2019, 9:54 AM
In D6837#100331, @mharbison72 wrote:
  1. In lfs, where we deal with standins and mutating status.

s/lfs/largefiles/?

Good catch, fixed.

indygreg accepted this revision.Sep 11 2019, 10:47 PM
This revision is now accepted and ready to land.Sep 11 2019, 10:47 PM
pulkit accepted this revision.Sep 12 2019, 1:55 AM
durin42 added a commit: rHG34ed651ba7e4: cleanup: fix leakage of dirstate._map to client code.Sep 12 2019, 2:26 AM
Closed by commit rHG34ed651ba7e4: cleanup: fix leakage of dirstate._map to client code (authored by durin42). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 16505

hgext/largefiles/reposetup.py

# files are not really removed if they are still in # files are not really removed if they are still in
# lfdirstate. This happens in merges where files # lfdirstate. This happens in merges where files
# change type. # change type.
removed = [f for f in removed removed = [f for f in removed
if f not in self.dirstate] if f not in self.dirstate]
result[2] = [f for f in result[2] result[2] = [f for f in result[2]
if f not in lfdirstate] if f not in lfdirstate]
lfiles = set(lfdirstate._map) lfiles = set(lfdirstate)
# Unknown files # Unknown files
result[4] = set(result[4]).difference(lfiles) result[4] = set(result[4]).difference(lfiles)
# Ignored files # Ignored files
result[5] = set(result[5]).difference(lfiles) result[5] = set(result[5]).difference(lfiles)
# combine normal files and largefiles # combine normal files and largefiles
normals = [[fn for fn in filelist normals = [[fn for fn in filelist
if not lfutil.isstandin(fn)] if not lfutil.isstandin(fn)]
for filelist in result] for filelist in result]

mercurial/debugcommands.py

if opts.get(r'nodates') is not None: if opts.get(r'nodates') is not None:
nodates = True nodates = True
datesort = opts.get(r'datesort') datesort = opts.get(r'datesort')
if datesort: if datesort:
keyfunc = lambda x: (x[1][3], x[0]) # sort by mtime, then by filename keyfunc = lambda x: (x[1][3], x[0]) # sort by mtime, then by filename
else: else:
keyfunc = None # sort by filename keyfunc = None # sort by filename
for file_, ent in sorted(repo.dirstate._map.iteritems(), key=keyfunc): for file_, ent in sorted(repo.dirstate.iteritems(), key=keyfunc):
if ent[3] == -1: if ent[3] == -1:
timestr = 'unset ' timestr = 'unset '
elif nodates: elif nodates:
timestr = 'set ' timestr = 'set '
else: else:
timestr = time.strftime(r"%Y-%m-%d %H:%M:%S ", timestr = time.strftime(r"%Y-%m-%d %H:%M:%S ",
time.localtime(ent[3])) time.localtime(ent[3]))
timestr = encoding.strtolocal(timestr) timestr = encoding.strtolocal(timestr)

mercurial/scmutil.py

raise error.ConfigError( raise error.ConfigError(
_("ui.portablefilenames value is invalid ('%s')") % val) _("ui.portablefilenames value is invalid ('%s')") % val)
return abort, warn return abort, warn
class casecollisionauditor(object): class casecollisionauditor(object):
def __init__(self, ui, abort, dirstate): def __init__(self, ui, abort, dirstate):
self._ui = ui self._ui = ui
self._abort = abort self._abort = abort
allfiles = '\0'.join(dirstate._map) allfiles = '\0'.join(dirstate)
self._loweredfiles = set(encoding.lower(allfiles).split('\0')) self._loweredfiles = set(encoding.lower(allfiles).split('\0'))
self._dirstate = dirstate self._dirstate = dirstate
# The purpose of _newfiles is so that we don't complain about # The purpose of _newfiles is so that we don't complain about
# case collisions if someone were to call this object with the # case collisions if someone were to call this object with the
# same filename twice. # same filename twice.
self._newfiles = set() self._newfiles = set()
def __call__(self, f): def __call__(self, f):