This is an archive of the discontinued Mercurial Phabricator instance.

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

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.

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

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