This is an archive of the discontinued Mercurial Phabricator instance.

config: read system hgrc in lexicographical order
ClosedPublic

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

Download Raw Diff

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

Branch

default

Lint

No Linters Available

Unit

No Unit Test Coverage

Event Timeline

martinvonz created this revision.Nov 3 2020, 2:26 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptNov 3 2020, 2:26 PM

Herald added a subscriber: mercurial-patches. · View Herald Transcript

Alphare accepted this revision.Nov 5 2020, 5:30 AM

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

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.

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

			Path	Packages
M			mercurial/scmposix.py (2 lines)
M			mercurial/scmwindows.py (2 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	23410		Nov 3 2020, 2:26 PM	★	★
Diff 2	23441		Nov 9 2020, 11:41 AM	★	★
Diff 3	23443	rHG9ac96b9fa76eb6a92e9c70a6fb14b0bc3041580a	Nov 3 2020, 12:56 PM	★	★

Commit	Parents	Author	Summary	Date
e6494fae0c50	793976f9029c	Martin von Zweigbergk		Nov 3 2020, 12:56 PM

Diff 23441

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')