| marmoute | |
| indygreg |
| hg-reviewers |
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()
| No Linters Available |
| No Unit Test Coverage |
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...
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 | ||
|---|---|---|
| 136 | Can you also add test for the trusting logic ? making sure the other hgrc is untrusted as expected when relevant ? | |
| 160–177 | 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? | |
I would be more confident if we get some explicit test about trust. However unshare is now working.
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 | |
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.
| Path | Packages | |||
|---|---|---|---|---|
| M | hgext/phabricator.py (6 lines) | |||
| M | mercurial/hg.py (27 lines) | |||
| M | mercurial/localrepo.py (15 lines) | |||
| M | tests/test-share-safe.t (108 lines) |
| Commit | Parents | Author | Summary | Date |
|---|---|---|---|---|
| ecb3be61c59a | f2457285dd31 | Pulkit Goyal | Jul 2 2020, 6:53 AM |
| b', otherwise will mock all http requests using the specified vcr file.' | b', otherwise will mock all http requests using the specified vcr file.' | ||||
| b' (ADVANCED)' | b' (ADVANCED)' | ||||
| ), | ), | ||||
| ), | ), | ||||
| ] | ] | ||||
| @eh.wrapfunction(localrepo, "loadhgrc") | @eh.wrapfunction(localrepo, "loadhgrc") | ||||
| def _loadhgrc(orig, ui, wdirvfs, hgvfs, requirements): | def _loadhgrc(orig, ui, wdirvfs, hgvfs, requirements, **opts): | ||||
| """Load ``.arcconfig`` content into a ui instance on repository open. | """Load ``.arcconfig`` content into a ui instance on repository open. | ||||
| """ | """ | ||||
| result = False | result = False | ||||
| arcconfig = {} | arcconfig = {} | ||||
| try: | try: | ||||
| # json.loads only accepts bytes from 3.6+ | # json.loads only accepts bytes from 3.6+ | ||||
| rawparams = encoding.unifromlocal(wdirvfs.read(b".arcconfig")) | rawparams = encoding.unifromlocal(wdirvfs.read(b".arcconfig")) | ||||
| cfg[(b"phabricator", b"callsign")] = arcconfig[b"repository.callsign"] | cfg[(b"phabricator", b"callsign")] = arcconfig[b"repository.callsign"] | ||||
| if b"phabricator.uri" in arcconfig: | if b"phabricator.uri" in arcconfig: | ||||
| cfg[(b"phabricator", b"url")] = arcconfig[b"phabricator.uri"] | cfg[(b"phabricator", b"url")] = arcconfig[b"phabricator.uri"] | ||||
| if cfg: | if cfg: | ||||
| ui.applyconfig(cfg, source=wdirvfs.join(b".arcconfig")) | ui.applyconfig(cfg, source=wdirvfs.join(b".arcconfig")) | ||||
| return orig(ui, wdirvfs, hgvfs, requirements) or result # Load .hg/hgrc | return ( | ||||
| orig(ui, wdirvfs, hgvfs, requirements, **opts) or result | |||||
| ) # Load .hg/hgrc | |||||
| def vcrcommand(name, flags, spec, helpcategory=None, optionalrepo=False): | def vcrcommand(name, flags, spec, helpcategory=None, optionalrepo=False): | ||||
| fullflags = flags + _VCR_FLAGS | fullflags = flags + _VCR_FLAGS | ||||
| def hgmatcher(r1, r2): | def hgmatcher(r1, r2): | ||||
| if r1.uri != r2.uri or r1.method != r2.method: | if r1.uri != r2.uri or r1.method != r2.method: | ||||
| return False | return False | ||||
| ) | ) | ||||
| postshare(srcrepo, r, defaultpath=defaultpath) | postshare(srcrepo, r, defaultpath=defaultpath) | ||||
| r = repository(ui, dest) | r = repository(ui, dest) | ||||
| _postshareupdate(r, update, checkout=checkout) | _postshareupdate(r, update, checkout=checkout) | ||||
| return r | return r | ||||
| def _prependsourcehgrc(repo): | |||||
| """ copies the source repo config and prepend it in current repo .hg/hgrc | |||||
| on unshare. This is only done if the share was perfomed using share safe | |||||
| method where we share config of source in shares""" | |||||
| srcvfs = vfsmod.vfs(repo.sharedpath) | |||||
| dstvfs = vfsmod.vfs(repo.path) | |||||
| if not srcvfs.exists(b'hgrc'): | |||||
| return | |||||
| currentconfig = b'' | |||||
| if dstvfs.exists(b'hgrc'): | |||||
| currentconfig = dstvfs.read(b'hgrc') | |||||
| with dstvfs(b'hgrc', b'wb') as fp: | |||||
| sourceconfig = srcvfs.read(b'hgrc') | |||||
| fp.write(b"# Config copied from shared source\n") | |||||
| fp.write(sourceconfig) | |||||
| fp.write(b'\n') | |||||
| fp.write(currentconfig) | |||||
| def unshare(ui, repo): | def unshare(ui, repo): | ||||
| """convert a shared repository to a normal one | """convert a shared repository to a normal one | ||||
| Copy the store data to the repo and remove the sharedpath data. | Copy the store data to the repo and remove the sharedpath data. | ||||
| Returns a new repository object representing the unshared repository. | Returns a new repository object representing the unshared repository. | ||||
| The passed repository object is not usable after this function is | The passed repository object is not usable after this function is | ||||
| called. | called. | ||||
| """ | """ | ||||
| with repo.lock(): | with repo.lock(): | ||||
| # we use locks here because if we race with commit, we | # we use locks here because if we race with commit, we | ||||
| # can end up with extra data in the cloned revlogs that's | # can end up with extra data in the cloned revlogs that's | ||||
| # not pointed to by changesets, thus causing verify to | # not pointed to by changesets, thus causing verify to | ||||
| # fail | # fail | ||||
| destlock = copystore(ui, repo, repo.path) | destlock = copystore(ui, repo, repo.path) | ||||
| with destlock or util.nullcontextmanager(): | with destlock or util.nullcontextmanager(): | ||||
| if localrepo.SHARESAFE_REQUIREMENT in repo.requirements: | |||||
| # we were sharing .hg/hgrc of the share source with the current | |||||
| # repo. We need to copy that while unsharing otherwise it can | |||||
| # disable hooks and other checks | |||||
| _prependsourcehgrc(repo) | |||||
| sharefile = repo.vfs.join(b'sharedpath') | sharefile = repo.vfs.join(b'sharedpath') | ||||
| util.rename(sharefile, sharefile + b'.old') | util.rename(sharefile, sharefile + b'.old') | ||||
| repo.requirements.discard(b'shared') | repo.requirements.discard(b'shared') | ||||
| repo.requirements.discard(b'relshared') | repo.requirements.discard(b'relshared') | ||||
| scmutil.writereporequirements(repo) | scmutil.writereporequirements(repo) | ||||
| else: | else: | ||||
| storevfs = vfsmod.vfs(hgvfs.join(b'store'), cacheaudited=True) | storevfs = vfsmod.vfs(hgvfs.join(b'store'), cacheaudited=True) | ||||
| requirements |= _readrequires(storevfs, False) | requirements |= _readrequires(storevfs, False) | ||||
| # 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): | ||||
| revlogv1 | revlogv1 | ||||
| shared | shared | ||||
| 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 | |||||
| Testing that hooks set in source repository also runs in shared repo | |||||
| $ cd ../source | |||||
| $ cat <<EOF >> .hg/hgrc | |||||
| > [extensions] | |||||
| > hooklib= | |||||
| > [hooks] | |||||
| > pretxnchangegroup.reject_merge_commits = \ | |||||
| > python:hgext.hooklib.reject_merge_commits.hook | |||||
| > EOF | |||||
| $ cd .. | |||||
| $ hg clone source cloned | |||||
| updating to branch default | |||||
| 3 files updated, 0 files merged, 0 files removed, 0 files unresolved | |||||
| $ cd cloned | |||||
| $ hg up 0 | |||||
| 0 files updated, 0 files merged, 2 files removed, 0 files unresolved | |||||
| $ echo bar > bar | |||||
| $ hg ci -Aqm "added bar" | |||||
| $ hg merge | |||||
| 2 files updated, 0 files merged, 0 files removed, 0 files unresolved | |||||
| (branch merge, don't forget to commit) | |||||
| $ hg ci -m "merge commit" | |||||
| $ hg push ../source | |||||
| pushing to ../source | |||||
| searching for changes | |||||
| adding changesets | |||||
| adding manifests | |||||
| adding file changes | |||||
| error: pretxnchangegroup.reject_merge_commits hook failed: bcde3522682d rejected as merge on the same branch. Please consider rebase. | |||||
| transaction abort! | |||||
| rollback completed | |||||
| abort: bcde3522682d rejected as merge on the same branch. Please consider rebase. | |||||
| [255] | |||||
| $ hg push ../shared1 | |||||
| pushing to ../shared1 | |||||
| searching for changes | |||||
| adding changesets | |||||
| adding manifests | |||||
| adding file changes | |||||
| error: pretxnchangegroup.reject_merge_commits hook failed: bcde3522682d rejected as merge on the same branch. Please consider rebase. | |||||
| transaction abort! | |||||
| rollback completed | |||||
| abort: bcde3522682d rejected as merge on the same branch. Please consider rebase. | |||||
| [255] | |||||
marmoute: Can you also add test for the trusting logic ? making sure the other hgrc is untrusted as… | |||||
| Test that if share source config is untrusted, we dont read it | |||||
| $ cd ../shared1 | |||||
| $ cat << EOF > $TESTTMP/untrusted.py | |||||
| > from mercurial import scmutil, util | |||||
| > def uisetup(ui): | |||||
| > class untrustedui(ui.__class__): | |||||
| > def _trusted(self, fp, f): | |||||
| > if util.normpath(fp.name).endswith(b'source/.hg/hgrc'): | |||||
| > return False | |||||
| > return super(untrustedui, self)._trusted(fp, f) | |||||
| > ui.__class__ = untrustedui | |||||
| > EOF | |||||
| $ hg showconfig hooks | |||||
| hooks.pretxnchangegroup.reject_merge_commits=python:hgext.hooklib.reject_merge_commits.hook | |||||
| $ hg showconfig hooks --config extensions.untrusted=$TESTTMP/untrusted.py | |||||
@marmoute test about trust pulkit: @marmoute test about trust | |||||
marmoute: Ha greate, I missed this whil read it :-) | |||||
| [1] | |||||
| Unsharing works | |||||
| $ hg unshare | $ hg unshare | ||||
| Test that source config is added to the shared one after unshare, and the config | |||||
| of current repo is still respected over the config which came from source config | |||||
| $ cd ../cloned | |||||
| $ hg push ../shared1 | |||||
| pushing to ../shared1 | |||||
| searching for changes | |||||
| adding changesets | |||||
| adding manifests | |||||
| adding file changes | |||||
| error: pretxnchangegroup.reject_merge_commits hook failed: bcde3522682d rejected as merge on the same branch. Please consider rebase. | |||||
| transaction abort! | |||||
| rollback completed | |||||
| abort: bcde3522682d rejected as merge on the same branch. Please consider rebase. | |||||
| [255] | |||||
| $ hg showconfig ui.curses -R ../shared1 | |||||
| false | |||||
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… | |||||
Can you also add test for the trusting logic ? making sure the other hgrc is untrusted as expected when relevant ?