This is an archive of the discontinued Mercurial Phabricator instance.

Differential D9269

config: read system hgrc in lexicographical order
ClosedPublic

Authored by martinvonz on Nov 3 2020, 2:26 PM.

Details

Reviewers
Alphare
mharbison72
Group Reviewers
hg-reviewers
Commits
rHG9ac96b9fa76e: config: read system hgrc in lexicographical order
Summary

This is similar to edbcf5b239f9 (config: read configs from directories
in lexicographical order, 2019-04-03). Apparently I forgot to sort the
system hgrc files there. That's fixed by this patch.

Diff Detail

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

Event Timeline

martinvonz created this revision.Nov 3 2020, 2:26 PM
Alphare accepted this revision.Nov 5 2020, 5:30 AM
mharbison72 requested changes to this revision.Nov 8 2020, 11:51 PM
mharbison72 added a subscriber: mharbison72.

I like the idea of a stable order, but I think this is a BC. Specifically, hg help config.files says:

The names of these files depend on the system on which Mercurial is
installed. "*.rc" files from a single directory are read in alphabetical
order, later ones overriding earlier ones. Where multiple paths are given
below, settings from earlier paths override later ones.

 On Windows, the following files are consulted:

- "<repo>/.hg/hgrc" (per-repository)
- "%USERPROFILE%\.hgrc" (per-user)
- "%USERPROFILE%\Mercurial.ini" (per-user)
- "%HOME%\.hgrc" (per-user)
- "%HOME%\Mercurial.ini" (per-user)
- "HKEY_LOCAL_MACHINE\SOFTWARE\Mercurial" (per-system)
- "<install-dir>\hgrc.d\*.rc" (per-installation)
- "<install-dir>\Mercurial.ini" (per-installation)
- "%PROGRAMDATA%\Mercurial\hgrc" (per-system)
- "%PROGRAMDATA%\Mercurial\Mercurial.ini" (per-system)
- "%PROGRAMDATA%\Mercurial\hgrc.d\*.rc" (per-system)
- "<internal>/*.rc" (defaults)

I read that as %USERPROFILE%\.hgrc overrides %USERPROFILE%\Mercurial.ini, but .hgrc will sort first.

There's also a bit of ordering of the list in mercurial.scmwindows.systemrcpath(). So maybe the sorting needs to happen inside the platform functions, and only when scanning a directory?

This revision now requires changes to proceed.Nov 8 2020, 11:51 PM
martinvonz added a comment.Nov 9 2020, 11:37 AM
In D9269#139641, @mharbison72 wrote:

I like the idea of a stable order, but I think this is a BC. Specifically, hg help config.files says:

The names of these files depend on the system on which Mercurial is
installed. "*.rc" files from a single directory are read in alphabetical
order, later ones overriding earlier ones. Where multiple paths are given
below, settings from earlier paths override later ones.
 On Windows, the following files are consulted:
- "<repo>/.hg/hgrc" (per-repository)
- "%USERPROFILE%\.hgrc" (per-user)
- "%USERPROFILE%\Mercurial.ini" (per-user)
- "%HOME%\.hgrc" (per-user)
- "%HOME%\Mercurial.ini" (per-user)
- "HKEY_LOCAL_MACHINE\SOFTWARE\Mercurial" (per-system)
- "<install-dir>\hgrc.d\*.rc" (per-installation)
- "<install-dir>\Mercurial.ini" (per-installation)
- "%PROGRAMDATA%\Mercurial\hgrc" (per-system)
- "%PROGRAMDATA%\Mercurial\Mercurial.ini" (per-system)
- "%PROGRAMDATA%\Mercurial\hgrc.d\*.rc" (per-system)
- "<internal>/*.rc" (defaults)

I read that as %USERPROFILE%\.hgrc overrides %USERPROFILE%\Mercurial.ini, but .hgrc will sort first.
There's also a bit of ordering of the list in mercurial.scmwindows.systemrcpath(). So maybe the sorting needs to happen inside the platform functions, and only when scanning a directory?

Ohh, good catch! I'll move the sorting into the platform functions as you suggested. Thanks!

martinvonz edited the summary of this revision. (Show Details)Nov 9 2020, 11:41 AM
martinvonz updated this revision to Diff 23441.
martinvonz added a commit: rHG9ac96b9fa76e: config: read system hgrc in lexicographical order.Nov 9 2020, 2:10 PM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG9ac96b9fa76e: config: read system hgrc in lexicographical order (authored by martinvonz). · Explain Why
This revision was automatically updated to reflect the committed changes.
mharbison72 added a comment.Nov 9 2020, 2:11 PM

Queued, thanks.

mercurial/scmwindows.py
34

Not related to this patch, but I'm surprised that we don't care to ignore directories here (and in posix), and just key off of the name. IDK what happens when a directory instead of a file is tossed into the config list. rcutil._expandrcpath() similarly doesn't check.

martinvonz added inline comments.Nov 9 2020, 3:21 PM
mercurial/scmwindows.py
34

Based on some manual testing I just did, it simply ignores paths that are directories. I haven't checked where in the code that happens.

Revision Contents
Changeset List

PathPackages
Mmercurial/scmposix.py (2 lines)
Mmercurial/scmwindows.py (2 lines)

Diff 23443

mercurial/scmposix.py

def _rcfiles(path): def _rcfiles(path):
rcs = [os.path.join(path, b'hgrc')] rcs = [os.path.join(path, b'hgrc')]
rcdir = os.path.join(path, b'hgrc.d') rcdir = os.path.join(path, b'hgrc.d')
try: try:
rcs.extend( rcs.extend(
[ [
os.path.join(rcdir, f) os.path.join(rcdir, f)
for f, kind in util.listdir(rcdir) for f, kind in sorted(util.listdir(rcdir))
if f.endswith(b".rc") if f.endswith(b".rc")
] ]
) )
except OSError: except OSError:
pass pass
return rcs return rcs

mercurial/scmwindows.py

rcpath = [] rcpath = []
filename = win32.executablepath() filename = win32.executablepath()
# Use mercurial.ini found in directory with hg.exe # Use mercurial.ini found in directory with hg.exe
progrc = os.path.join(os.path.dirname(filename), b'mercurial.ini') progrc = os.path.join(os.path.dirname(filename), b'mercurial.ini')
rcpath.append(progrc) rcpath.append(progrc)
def _processdir(progrcd): def _processdir(progrcd):
if os.path.isdir(progrcd): if os.path.isdir(progrcd):
for f, kind in util.listdir(progrcd): for f, kind in sorted(util.listdir(progrcd)):
mharbison72Unsubmitted
Not Done

Not related to this patch, but I'm surprised that we don't care to ignore directories here (and in posix), and just key off of the name. IDK what happens when a directory instead of a file is tossed into the config list. rcutil._expandrcpath() similarly doesn't check.

mharbison72: Not related to this patch, but I'm surprised that we don't care to ignore directories here (and…
martinvonzAuthorUnsubmitted
Done

Based on some manual testing I just did, it simply ignores paths that are directories. I haven't checked where in the code that happens.

martinvonz: Based on some manual testing I just did, it simply ignores paths that are directories. I…
if f.endswith(b'.rc'): if f.endswith(b'.rc'):
rcpath.append(os.path.join(progrcd, f)) rcpath.append(os.path.join(progrcd, f))
# Use hgrc.d found in directory with hg.exe # Use hgrc.d found in directory with hg.exe
_processdir(os.path.join(os.path.dirname(filename), b'hgrc.d')) _processdir(os.path.join(os.path.dirname(filename), b'hgrc.d'))
# treat a PROGRAMDATA directory as equivalent to /etc/mercurial # treat a PROGRAMDATA directory as equivalent to /etc/mercurial
programdata = encoding.environ.get(b'PROGRAMDATA') programdata = encoding.environ.get(b'PROGRAMDATA')