This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8764

localrepo: deduplicate code around reading requires file
AbandonedPublic

Authored by pulkit on Jul 17 2020, 10:20 AM.

Details

Reviewers
marmoute
Group Reviewers
hg-reviewers

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

pulkit created this revision.Jul 17 2020, 10:20 AM
marmoute requested changes to this revision.Jul 20 2020, 1:11 PM
This revision now requires changes to proceed.Jul 20 2020, 1:11 PM
marmoute added inline comments.Jul 20 2020, 3:29 PM
mercurial/localrepo.py
525–539

This does not seems to need to be a closure. Can we make it a normal function?

marmoute resigned from this revision.Jul 20 2020, 3:44 PM

More serious question: Maybe we should stop passing around tuple of dictionnary and return an object holding all that data with a better API. What do you think ?

This revision now requires review to proceed.Jul 20 2020, 3:44 PM
marmoute requested changes to this revision.Jul 20 2020, 4:59 PM
marmoute added a subscriber: marmoute.

I resigned (and commented) on the wrong revision.

This revision now requires changes to proceed.Jul 20 2020, 4:59 PM
pulkit added inline comments.Jul 21 2020, 9:07 AM
mercurial/localrepo.py
525–539

This code ideally should not be used outside this function. Hence a closure.

marmoute added a comment.EditedJul 21 2020, 9:12 AM

That is a bad reason to use a closure. Make the function private and move it at the module level.
(Mercurial used to be full of this strange closure and this was a pain)

pulkit updated this revision to Diff 22000.Jul 21 2020, 9:42 AM
marmoute requested changes to this revision.Jul 21 2020, 9:46 AM
marmoute added inline comments.
mercurial/localrepo.py
525–539

Still a closure, please make it not a closure.

This revision now requires changes to proceed.Jul 21 2020, 9:46 AM
pulkit updated this revision to Diff 22069.Jul 23 2020, 10:52 AM
pulkit added inline comments.Jul 23 2020, 10:56 AM
mercurial/localrepo.py
525–539

Fixed.

pulkit updated this revision to Diff 22172.Aug 1 2020, 10:36 AM
pulkit abandoned this revision.Aug 7 2020, 2:51 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/localrepo.py (23 lines)

Diff 21953

mercurial/localrepo.py

except ValueError as e: except ValueError as e:
# Can be raised on Python 3.8 when path is invalid. # Can be raised on Python 3.8 when path is invalid.
raise error.Abort( raise error.Abort(
_(b'invalid path %s: %s') % (path, pycompat.bytestr(e)) _(b'invalid path %s: %s') % (path, pycompat.bytestr(e))
) )
raise error.RepoError(_(b'repository %s not found') % path) raise error.RepoError(_(b'repository %s not found') % path)
# .hg/requires file contains a newline-delimited list of def _readrequires(vfs):
# features/capabilities the opener (us) must have in order to use
# the repository. This file was introduced in Mercurial 0.9.2,
# which means very old repositories may not have one. We assume
# a missing file translates to no requirements.
try: try:
requirements = set(hgvfs.read(b'requires').splitlines()) requirements = set(vfs.read(b'requires').splitlines())
except IOError as e: except IOError as e:
if e.errno != errno.ENOENT: if e.errno != errno.ENOENT:
raise raise
requirements = set() requirements = set()
return requirements
# .hg/requires file contains a newline-delimited list of
# features/capabilities the opener (us) must have in order to use
# the repository. This file was introduced in Mercurial 0.9.2,
# which means very old repositories may not have one. We assume
# a missing file translates to no requirements.
requirements = _readrequires(hgvfs)
marmouteUnsubmitted
Not Done

This does not seems to need to be a closure. Can we make it a normal function?

marmoute: This does not seems to need to be a closure. Can we make it a normal function?
pulkitAuthorUnsubmitted
Done

This code ideally should not be used outside this function. Hence a closure.

pulkit: This code ideally should not be used outside this function. Hence a closure.
marmouteUnsubmitted
Not Done

Still a closure, please make it not a closure.

marmoute: Still a closure, please make it not a closure.
pulkitAuthorUnsubmitted
Done

Fixed.

pulkit: Fixed.
# if .hg/requires contains the sharesafe requirement, it means # if .hg/requires contains the sharesafe requirement, it means
# there exists a `.hg/store/requires` too and we should read it # there exists a `.hg/store/requires` too and we should read it
if SHARESAFE_REQUIREMENT in requirements: if SHARESAFE_REQUIREMENT in requirements:
if hgvfs.exists(b'sharedpath'): if hgvfs.exists(b'sharedpath'):
# This is a shared repo # This is a shared repo
sharedpath = hgvfs.read(b'sharedpath').rstrip(b'\n') sharedpath = hgvfs.read(b'sharedpath').rstrip(b'\n')
if b'relshared' in requirements: if b'relshared' in requirements:
sharedpath = hgvfs.join(sharedpath) sharedpath = hgvfs.join(sharedpath)
storevfs = vfsmod.vfs(vfsmod.vfs(sharedpath).join(b'store')) storevfs = vfsmod.vfs(vfsmod.vfs(sharedpath).join(b'store'))
else: else:
storevfs = vfsmod.vfs(hgvfs.join(b'store'), cacheaudited=True) storevfs = vfsmod.vfs(hgvfs.join(b'store'), cacheaudited=True)
try: requirements |= _readrequires(storevfs)
store_requirements = set(storevfs.read(b'requires').splitlines())
requirements |= store_requirements
except IOError as e:
if e.errno != errno.ENOENT:
raise
# The .hg/hgrc file may load extensions or contain config options # The .hg/hgrc file may load extensions or contain config options
# that influence repository construction. Attempt to load it and # that influence repository construction. Attempt to load it and
# process any new extensions that it may have pulled in. # process any new extensions that it may have pulled in.
if loadhgrc(ui, wdirvfs, hgvfs, requirements): if loadhgrc(ui, wdirvfs, hgvfs, requirements):
afterhgrcload(ui, wdirvfs, hgvfs, requirements) afterhgrcload(ui, wdirvfs, hgvfs, requirements)
extensions.loadall(ui) extensions.loadall(ui)
extensions.populateui(ui) extensions.populateui(ui)