This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8656

localrepo: load the share source .hg/hgrc also in share-safe mode (API)
ClosedPublic

Authored by pulkit on Jun 24 2020, 10:31 AM.

Details

Reviewers
marmoute
indygreg
Group Reviewers
hg-reviewers
Commits
rHGb71858b42963: localrepo: load the share source .hg/hgrc also in share-safe mode (API)
Summary

The second part of the Share Safe Plan is to share source repo config also.
This patch adds logic to load the source repo .hg/hgrc if we are in share safe
mode. On unshare, we copy and prepend source config to current repo so that
config which was shared is persisted.

A test is added to show that now if we enable a hook on the source repo, that
also runs on the shared repositories.

API change as a new optional argument sharedvfs added to localrepo.loadhgrc()

Diff Detail

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

Event Timeline

pulkit created this revision.Jun 24 2020, 10:31 AM
pulkit updated this revision to Diff 21742.Jul 1 2020, 6:00 AM
pulkit edited the summary of this revision. (Show Details)Jul 2 2020, 7:16 AM
pulkit updated this revision to Diff 21752.
durin42 added a subscriber: durin42.Jul 15 2020, 11:44 AM

Something in this stack (that I didn't push) breaks the phabricator extension. See phabricator.py line 170:

@eh.wrapfunction(localrepo, "loadhgrc")
def _loadhgrc(orig, ui, wdirvfs, hgvfs, requirements):

So you'll need to do....something there. Maybe look for other wrappers in extensions too while you're there...

marmoute requested changes to this revision.Jul 15 2020, 2:11 PM
marmoute added a subscriber: marmoute.

I am fine with taking this root (reading source hgrc and adding a nonsharedrc) as I think it is a better experience overall. However, we have to be prepared to drop a large backward compatibility breaking notice when we turn sharesafe on by default, because people relying on the config to not be shared might be up for a surprise.

tests/test-share-safe.t
88

There is apparently an unshare command. Sicne we read the source config, that source config need to be copied in the repository when unsharing. Can you add code and test for this?

136

Can you also add test for the trusting logic ? making sure the other hgrc is untrusted as expected when relevant ?

This revision now requires changes to proceed.Jul 15 2020, 2:11 PM
pulkit retitled this revision from localrepo: load the share source .hg/hgrc also in share-safe mode to localrepo: load the share source .hg/hgrc also in share-safe mode (API).Jul 20 2020, 8:15 AM
pulkit edited the summary of this revision. (Show Details)
pulkit updated this revision to Diff 21977.
marmoute accepted this revision.EditedJul 20 2020, 1:18 PM

I would be more confident if we get some explicit test about trust. However unshare is now working.

pulkit added a comment.EditedJul 20 2020, 1:44 PM
In D8656#131136, @marmoute wrote:

I would be more confident if we get some explicit test about trust. However unshare is not working.

There is an explicit test about trust, kindly check inline comments, mentioned you.

Didn't understand the second part, what's not working?

tests/test-share-safe.t
155

@marmoute test about trust

pulkit updated this revision to Diff 22002.Jul 21 2020, 9:42 AM
marmoute accepted this revision.Jul 21 2020, 9:47 AM
marmoute added inline comments.
tests/test-share-safe.t
155

Ha greate, I missed this whil read it :-)

pulkit updated this revision to Diff 22071.Jul 23 2020, 10:53 AM
pulkit updated this revision to Diff 22223.Aug 3 2020, 9:12 AM
pulkit updated this revision to Diff 22382.Aug 10 2020, 11:27 AM
pulkit updated this revision to Diff 22398.Aug 11 2020, 5:06 AM
pulkit updated this revision to Diff 22451.Aug 26 2020, 6:05 AM
pulkit updated this revision to Diff 22655.Sep 16 2020, 7:56 AM
indygreg accepted this revision.Sep 17 2020, 9:37 PM
indygreg added a subscriber: indygreg.

This seems like useful functionality. However, I expect someone to complain about both the reading the extra config file and prepending its content on unshare. I'm tempted to say we should preemptively make this behavior configurable. Although I hate complexity. So let's wait until someone complains.

This revision is now accepted and ready to land.Sep 17 2020, 9:37 PM

Revision Contents
Changeset List

PathPackages
Mmercurial/localrepo.py (15 lines)
Mtests/test-share-safe.t (19 lines)

Diff 21704

mercurial/localrepo.py

requirements |= store_requirements requirements |= store_requirements
except IOError as e: except IOError as e:
if e.errno != errno.ENOENT: if e.errno != errno.ENOENT:
raise 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, sharedvfs):
afterhgrcload(ui, wdirvfs, hgvfs, requirements) afterhgrcload(ui, wdirvfs, hgvfs, requirements)
extensions.loadall(ui) extensions.loadall(ui)
extensions.populateui(ui) extensions.populateui(ui)
# Set of module names of extensions loaded for this repository. # Set of module names of extensions loaded for this repository.
extensionmodulenames = {m.__name__ for n, m in extensions.extensions(ui)} extensionmodulenames = {m.__name__ for n, m in extensions.extensions(ui)}
supportedrequirements = gathersupportedrequirements(ui) supportedrequirements = gathersupportedrequirements(ui)
store=store, store=store,
cachevfs=cachevfs, cachevfs=cachevfs,
wcachevfs=wcachevfs, wcachevfs=wcachevfs,
features=features, features=features,
intents=intents, intents=intents,
) )
def loadhgrc(ui, wdirvfs, hgvfs, requirements): def loadhgrc(ui, wdirvfs, hgvfs, requirements, sharedvfs=None):
"""Load hgrc files/content into a ui instance. """Load hgrc files/content into a ui instance.
This is called during repository opening to load any additional This is called during repository opening to load any additional
config files or settings relevant to the current repository. config files or settings relevant to the current repository.
Returns a bool indicating whether any additional configs were loaded. Returns a bool indicating whether any additional configs were loaded.
Extensions should monkeypatch this function to modify how per-repo Extensions should monkeypatch this function to modify how per-repo
configs are loaded. For example, an extension may wish to pull in configs are loaded. For example, an extension may wish to pull in
configs from alternate files or sources. configs from alternate files or sources.
sharedvfs is vfs object pointing to source repo if the current one is a
shared one
""" """
if not rcutil.use_repo_hgrc(): if not rcutil.use_repo_hgrc():
return False return False
# first load config from shared source if we has to
if SHARESAFE_REQUIREMENT in requirements and sharedvfs:
try:
ui.readconfig(sharedvfs.join(b'hgrc'), root=sharedvfs.base)
except IOError:
pass
try: try:
ui.readconfig(hgvfs.join(b'hgrc'), root=wdirvfs.base) ui.readconfig(hgvfs.join(b'hgrc'), root=wdirvfs.base)
return True return True
except IOError: except IOError:
return False return False
def afterhgrcload(ui, wdirvfs, hgvfs, requirements): def afterhgrcload(ui, wdirvfs, hgvfs, requirements):

tests/test-share-safe.t

shared shared
sharesafe sharesafe
sparserevlog sparserevlog
store store
$ echo c > c $ echo c > c
$ hg ci -Aqm "added c" $ hg ci -Aqm "added c"
Check that config of the source repository is also loaded
$ hg showconfig ui.curses
[1]
$ echo "[ui]" >> ../source/.hg/hgrc
$ echo "curses=true" >> ../source/.hg/hgrc
$ hg showconfig ui.curses
true
However, local .hg/hgrc should override the config set by share source
$ echo "[ui]" >> .hg/hgrc
$ echo "curses=false" >> .hg/hgrc
$ hg showconfig ui.curses
false
$ hg unshare $ hg unshare
marmouteUnsubmitted
Not Done

Can you also add test for the trusting logic ? making sure the other hgrc is untrusted as expected when relevant ?

marmoute: Can you also add test for the trusting logic ? making sure the other hgrc is untrusted as…
marmouteUnsubmitted
Not Done

There is apparently an unshare command. Sicne we read the source config, that source config need to be copied in the repository when unsharing. Can you add code and test for this?

marmoute: There is apparently an unshare command. Sicne we read the source config, that source config…
pulkitAuthorUnsubmitted
Done

@marmoute test about trust

pulkit: @marmoute test about trust
marmouteUnsubmitted
Not Done

Ha greate, I missed this whil read it :-)

marmoute: Ha greate, I missed this whil read it :-)