This is an archive of the discontinued Mercurial Phabricator instance.

Differential D154

configwarn: new extension to warn about unsupported configs
ClosedPublic

Authored by quark on Jul 20 2017, 7:52 AM.

Details

Reviewers
simonfar
durham
Group Reviewers
Restricted Project
Commits
rFBHGX1859b098b99e: configwarn: new extension to warn about unsupported configs
Summary

We have seen issues caused by user config file (~/.hgrc) having
unsupported extensions enabled. This new extension would allow us to warn
users about those problematic configs.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

quark created this revision.Jul 20 2017, 7:52 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptJul 20 2017, 7:52 AM
quark updated this revision to Diff 344.Jul 20 2017, 7:55 AM
quark added a reviewer: simonfar.Jul 21 2017, 10:41 AM
quark added a subscriber: simonfar.

cc @simonfar: I think this can help clean up legacy configs and unblocks us from simplifying the build process (aka comments on T20019093)

stash added a subscriber: stash.Jul 24 2017, 5:25 AM

It looks like quite scary change. Maybe we can just prevent bad extensions from loading?

simonfar requested changes to this revision.Jul 24 2017, 10:46 AM

Back to you for a reply

hgext3rd/configrewrite.py
40 ↗(On Diff #344)

Please fail hard if the support contact is not set, with a suitable error - if we're going to do this, we *need* to make sure that people don't enable this without a decent support contact in place.

59–72 ↗(On Diff #344)

I really don't like this - it feels like a route to trouble.

I would prefer this module to:

  1. Change in-memory config so that the "bad" config does not affect user experience when they're in a hurry - they can still work.
  2. Tell the user which bits of their config they need to edit to avoid the warning from this module.

Then we avoid all the fun with multi-line configs, commenting etc, and leave the fix in the user's hands, with an annoying warning every time they run hg until they fix their config.

This revision now requires changes to proceed.Jul 24 2017, 10:46 AM
quark added inline comments.Jul 24 2017, 12:28 PM
hgext3rd/configrewrite.py
59–72 ↗(On Diff #344)

We can do 2 (which is what this is doing if it cannot write a file). But 1 could be challenging since by the time our extension gets loaded, extensions.py has decided which extensions to load already. I'll revisit the code to see if there is a way (even hacky).

quark edited edge metadata.Aug 4 2017, 1:17 PM
quark updated this revision to Diff 548.
quark edited the summary of this revision. (Show Details)Aug 4 2017, 1:23 PM
quark retitled this revision from configrewrite: new extension to rewrite user config files to configwarn: new extension to warn about unsupported configs.
quark updated this revision to Diff 549.
durham added a subscriber: durham.Aug 4 2017, 5:41 PM
durham added inline comments.
hgext3rd/configwarn.py
52

What about projectrc settings? If I understand the code here, a projectrc setting will not print the warning below. So maybe "systemconfigs" isn't quite the right name?

quark added inline comments.Aug 4 2017, 5:58 PM
hgext3rd/configwarn.py
52

You're right. I thought it was rare that people change .hg/hgrc. Will add code to handle it.

quark updated this revision to Diff 573.Aug 4 2017, 6:20 PM
durham accepted this revision.Aug 7 2017, 4:07 PM
Closed by commit rFBHGX1859b098b99e: configwarn: new extension to warn about unsupported configs (authored by quark). · Explain WhyAug 7 2017, 4:27 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
AMhgext3rd/configwarn.py (53 lines)
AMtests/test-configwarn.t (54 lines)

Diff 613

hgext3rd/configwarn.py

  • This file was added.
# configwarn.py - warn unsupported user configs
#
# Copyright 2017 Facebook, Inc.
#
# This software may be used and distributed according to the terms of the
# GNU General Public License version 2 or any later version.
"""warn unsupported user configs
Config::
[configwarn]
# Config names that are supposed to be set by system config and not
# overrided by user config.
systemconfigs = diff.git, extensions.hggit
"""
from __future__ import absolute_import
from mercurial.i18n import _
from mercurial import (
rcutil,
registrar,
)
configtable = {}
configitem = registrar.configitem(configtable)
configitem('configwarn', 'systemconfigs', default=[])
def reposetup(ui, repo):
# use reposetup, not uisetup to work better with chg and it checks reporc.
if not repo.local():
return
nonsystempaths = set(rcutil.userrcpath() + [repo.vfs.join('hgrc')])
systemconfigs = ui.configlist('configwarn', 'systemconfigs')
for configname in systemconfigs:
if '.' not in configname:
continue
section, name = configname.split('.', 1)
source = ui.configsource(section, name)
if ':' not in source:
continue
path, lineno = source.split(':', 1)
if path in nonsystempaths and lineno.isdigit():
ui.warn(_('warning: overriding config %s is unsupported (hint: '
'remove line %s from %s to resolve this issue)\n')
durhamUnsubmitted
Not Done

What about projectrc settings? If I understand the code here, a projectrc setting will not print the warning below. So maybe "systemconfigs" isn't quite the right name?

durham: What about projectrc settings? If I understand the code here, a projectrc setting will not…
quarkAuthorUnsubmitted
Not Done

You're right. I thought it was rare that people change .hg/hgrc. Will add code to handle it.

quark: You're right. I thought it was rare that people change .hg/hgrc. Will add code to handle it.
% (configname, lineno, path))

tests/test-configwarn.t

  • This file was added.
$ cat >> $HGRCPATH <<EOF
> [extensions]
> configwarn=$TESTDIR/../hgext3rd/configwarn.py
> [configwarn]
> systemconfigs=diff.git, phases.publish
> [alias]
> noop=log -r null -T '{files}'
> EOF
Need to override rcutil.userrcpath to test properly without side-effects
$ cat >> $TESTTMP/rcpath.py <<EOF
> from __future__ import absolute_import
> from mercurial import encoding, rcutil
> def userrcpath():
> return [encoding.environ[b'USERHGRC']]
> rcutil.userrcpath = userrcpath
> EOF
$ cat >> $HGRCPATH <<EOF
> [extensions]
> rcpath=$TESTTMP/rcpath.py
> EOF
$ USERHGRC="$TESTTMP/userhgrc"
$ HGRCPATH="$HGRCPATH:$USERHGRC"
$ export USERHGRC
Config set by system config files or command line flags are fine
$ hg init
$ hg noop
$ hg noop --config diff.git=1
Config set by user config will generate warnings
$ cat >> $USERHGRC <<EOF
> [diff]
> git=0
> EOF
$ hg noop
warning: overriding config diff.git is unsupported (hint: remove line 2 from $TESTTMP/userhgrc to resolve this issue)
Config set by repo will generate warnings
$ cat >> .hg/hgrc << EOF
> [phases]
> publish=1
> EOF
$ hg noop
warning: overriding config diff.git is unsupported (hint: remove line 2 from $TESTTMP/userhgrc to resolve this issue)
warning: overriding config phases.publish is unsupported (hint: remove line 2 from $TESTTMP/.hg/hgrc to resolve this issue)