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

Yuya Nishihara yuya at tcha.org
Sun Jul 3 04:44:32 UTC 2016


On Thu, 30 Jun 2016 17:59:00 +0100, Jun Wu wrote:
> # HG changeset patch
> # User Jun Wu <quark at fb.com>
> # Date 1467292128 -3600
> #      Thu Jun 30 14:08:48 2016 +0100
> # Node ID 50d862642eb658dc28ca878a4ded89ab7303e3a9
> # Parent  c02da7bd56e98cf4e905550859031079ed9beb18
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r 50d862642eb6
> chgserver: add a context manager to learn what to include for confighash
> 
> A pitfall about chg is developers need to be aware the ui object passed to
> ui/extsetup can have wrong config and should not be depended on. This confuses
> people, and causes inconvenience.
> 
> Instead of adding another set APIs letting extensions telling chg what need to
> be hashed and fixing all problematic extensions one by one, I think it is
> better just letting chg collecting these pieces of information automatically.

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

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


More information about the Mercurial-devel mailing list