[PATCH 05 of 10] chgserver: add a context manager to learn what to include for confighash

[Yuya Nishihara]

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
> confident.
>  
> > > 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.