[PATCH 05 of 10] chgserver: add a context manager to learn what to include for confighash
yuya at tcha.org
Tue Jul 5 09:46:04 EDT 2016
On Mon, 4 Jul 2016 20:44:25 +0100, Jun Wu wrote:
> Excerpts from Yuya Nishihara's message of 2016-07-03 13:44:32 +0900:
> > Can the problems you're facing are solved if _configsections are the whitelist?
> > We can easily build a set of known-good config keys from our codebase. Assuming
> > config values don't vary amongst repositories and don't change often, it would
> > be acceptable to spawn new server if unknown config changed. We can't do that
> > for environment variables, but I can't think of a good reason why they have to
> > use os.environ in uisetup().
> I don't quite get your question. So, I draft some questions:
> 1. Is a static whitelist enough?
> No. The issue is 3rd-party extensions.
> We can say this is a limitation of chg and document it somewhere but
> people just don't read docs and yell at you whenever they see hg works
> while chg does not.
As long as third-party extensions use their own config sections, e.g.
"remotefilelog.blah-blah", all of them are included in the hash if the list is
a whitelist of safe sections. If they read e.g. ui.username in uisetup() and
we have to work around that, a static list wouldn't help.
> 2. Other solutions?
> I did have other attempts, like a chg-compat checker, devel-warn or
> thinking about an API to let extensions telling us what they want to hash.
> A chg-compat checker is actually hard to implement correctly since
> extensions depending on other extensions are hard to check.
> devel-warn will pollute extensions.py.
> An API for extensions work but why do we let extensions telling us things
> while we can know ourselves with a little effort?
> 3. Is "_configitems" necessary since we have "_configsections"?
> I think so. Some extension accesses "ui.debugflag" and wraps core
> functions with a version with a logger. Hashing the whole ui section
> looks undesirable.
Huh? Do they want to trust only [ui]debug ignoring --debug option?
> Therefore I think the "dynamically learn" approach is the way to go.
> Developers don't need to know about chg and aren't forced to rewrite their
> code (sometimes it's hard) using other patterns. Developers with chg in mind
> can write code to reduce the count of chgservers.
I prefer the static approach if it can save 90% of the problems with a little
penalty of excessive server redirection. Dynamically updating the list would
make debugging harder and would need more tests.
> I will send V2 after the unwrapfunction feature so it would be more
> > > Note this is not a solution for all cases, namely the following cases still
> > > need extra work:
> > >
> > > 1. "if ui.config('x'): ui.setconfig('x', 'y')" in uisetup.
> > > This will overwrite config 'x' and chg will be unable to hash the value
> > > user provides. The pattern is mainly used in the evolve extension. It
> > > could be solved if we have clear layers about what are set by extensions
> > > and what are set by users.
> > >
> > > 2. Patterns like "self.ui = ui" in uisetup.
> > > This will keep a reference to a stale ui object. We probably want to
> > > add devel-warn for the case. It may also be reasonable to add a similar
> > > check to reposetup.
> > >
> > > 3. Interacting with filesystem or network in uisetup.
> > > In general, extensions doing these should be aware of race conditions.
> > (1) and (2) are generally wrong because the ui passed to uisetup() and
> > extsetup() are "lui", a temporary copy of ui.
> Why are they wrong?
> For (1), you can have an extension:
> # a.py
> def uisetup(ui):
> # extdiff is in chgserver._configsections
> ui.setconfig('extdiff', 'x', 'x')
> And run chg:
> chg --config extensions.s=$PWD/a.py --config extdiff.x=1 version
> chg --config extensions.s=$PWD/a.py --config extdiff.x=2 version
> The second command will not start a new server.
IIRC, config values set in uisetup() aren't copied back to the original ui
and repo.ui, which is why I said generally wrong.
> For (2), if you save ui from uisetup to somewhere (say, a global variable),
> and then use it in reposetup, it can have wrong configs for all things
> outside chg confighash. It's not the case for short-lived hg process.
The ui passed to uisetup() isn't kept updated, which could have a different
value from repo.ui. IMHO, keeping ui globally is a bad idea.
More information about the Mercurial-devel