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

Jun Wu quark at fb.com
Mon Jul 4 15:44:25 EDT 2016


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.

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.

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

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.

I like the idea of having a disposable ui object for uisetup, dropping all
effects extensions doing on it. But that sounds like a big breaking change.


More information about the Mercurial-devel mailing list