This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7627

config: drop debug messages saying where config was read from
ClosedPublic

Authored by martinvonz on Dec 12 2019, 6:44 PM.

Download Raw Diff

Details

Reviewers

pulkit

Group Reviewers

hg-reviewers

Commits

rHG6b6872822b32: config: drop debug messages saying where config was read from

Summary

hg config --debug includes lines like this:

set config by: $EDITOR

but also lines like this:

$EDITOR: ui.editor=emacs -nw

The set config by messages don't seem to provide much additional
information over what we get from the $EDITOR:-type message. I could
imagine wanting to see which values got overriden by a later entry,
but that information is already not present. So let's just remove the
first type of output. My next patch would otherwise amplify the
redundant output (there would be one set config by for each line in
mergetools.rc).

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.Dec 12 2019, 6:44 PM

Herald added a reviewer: hg-reviewers. · View Herald TranscriptDec 12 2019, 6:44 PM

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

martinvonz added a child revision: D7628: config: move reading of defaultrc/ files from ui to rcutil.Dec 12 2019, 6:44 PM

On Windows, you can set a registry key to one or more *.ini files or directories that get read in, so it seems slightly handy to know the order in which files are processed. I know I've used that mechanism on various systems to find directories in which to stash things too.

That said, I don't think I feel strongly about keeping it if it makes the next patch messy. (I did a quick skim of it, but didn't see why that would be.)

In D7627#112220, @mharbison72 wrote:

On Windows, you can set a registry key to one or more *.ini files or directories that get read in, so it seems slightly handy to know the order in which files are processed. I know I've used that mechanism on various systems to find directories in which to stash things too.
That said, I don't think I feel strongly about keeping it if it makes the next patch messy. (I did a quick skim of it, but didn't see why that would be.)

It means that every line in merge-tools.rc would get two lines in this output (one line saying "set value from ..." or something like that).

In D7627#112223, @martinvonz wrote:

In D7627#112220, @mharbison72 wrote:

On Windows, you can set a registry key to one or more *.ini files or directories that get read in, so it seems slightly handy to know the order in which files are processed. I know I've used that mechanism on various systems to find directories in which to stash things too.
That said, I don't think I feel strongly about keeping it if it makes the next patch messy. (I did a quick skim of it, but didn't see why that would be.)

It means that every line in merge-tools.rc would get two lines in this output (one line saying "set value from ..." or something like that).

I'm also happy to drop just the "set item" lines. Better?

In D7627#112373, @martinvonz wrote:

In D7627#112223, @martinvonz wrote:

In D7627#112220, @mharbison72 wrote:

On Windows, you can set a registry key to one or more *.ini files or directories that get read in, so it seems slightly handy to know the order in which files are processed. I know I've used that mechanism on various systems to find directories in which to stash things too.
That said, I don't think I feel strongly about keeping it if it makes the next patch messy. (I did a quick skim of it, but didn't see why that would be.)

It means that every line in merge-tools.rc would get two lines in this output (one line saying "set value from ..." or something like that).

I'm also happy to drop just the "set item" lines. Better?

I think so, thanks.

One of the last hunks dropped here mentions "set by $EDITOR", but then never mentions EDITOR again. That seems confusing if you don't know what that setting applies to and the precedence rules, and not real helpful if you do.

In D7627#112577, @mharbison72 wrote:

In D7627#112373, @martinvonz wrote:

In D7627#112223, @martinvonz wrote:

In D7627#112220, @mharbison72 wrote:

On Windows, you can set a registry key to one or more *.ini files or directories that get read in, so it seems slightly handy to know the order in which files are processed. I know I've used that mechanism on various systems to find directories in which to stash things too.
That said, I don't think I feel strongly about keeping it if it makes the next patch messy. (I did a quick skim of it, but didn't see why that would be.)

It means that every line in merge-tools.rc would get two lines in this output (one line saying "set value from ..." or something like that).

I'm also happy to drop just the "set item" lines. Better?

I think so, thanks.
One of the last hunks dropped here mentions "set by $EDITOR", but then never mentions EDITOR again. That seems confusing if you don't know what that setting applies to and the precedence rules, and not real helpful if you do.

Okay, will upload an update that adds back the read config from: lines a few minutes.

martinvonz edited the summary of this revision. (Show Details)Dec 16 2019, 6:34 PM

martinvonz updated this revision to Diff 18779.

After recent iteration, commit message needs to be reworked. Maybe I am biased because I know what the old version was and is still parsing the commit message that way.

In D7627#112962, @pulkit wrote:

After recent iteration, commit message needs to be reworked. Maybe I am biased because I know what the old version was and is still parsing the commit message that way.

I did rework the message, but can see that mentioning the read config from: lines is now misleading. I'll remove that.

martinvonz edited parent revisions, added: D7624: rcutil: don't check if defaultrc/ is a directory -- we know it is; removed: D7626: config: close file even if we fail to read it.Dec 17 2019, 9:20 AM

martinvonz edited the summary of this revision. (Show Details)

martinvonz updated this revision to Diff 18809.

pulkit accepted this revision.Dec 17 2019, 11:06 AM

This revision is now accepted and ready to land.Dec 17 2019, 11:06 AM

martinvonz added a commit: rHG6b6872822b32: config: drop debug messages saying where config was read from.Dec 17 2019, 11:43 AM

Closed by commit rHG6b6872822b32: config: drop debug messages saying where config was read from (authored by martinvonz). · Explain Why

This revision was automatically updated to reflect the committed changes.

martinvonz removed a child revision: D7628: config: move reading of defaultrc/ files from ui to rcutil.Dec 19 2019, 12:47 AM

Revision Contents
Changeset List

			Path	Packages
M			mercurial/commands.py (4 lines)
M			tests/test-hgrc.t (3 lines)

Diff	ID	Description	Created	Lint	Unit
Base		Base
Diff 1	18657		Dec 12 2019, 6:44 PM	★	★
Diff 2	18779		Dec 16 2019, 6:34 PM	★	★
Diff 3	18809		Dec 17 2019, 9:20 AM	★	★
Diff 4	18816	rHG6b6872822b32a76c66b0e95f5e5880be785558ac	Dec 12 2019, 1:23 AM	★	★

Status	Author	Revision
Closed	martinvonz	D7626 config: close file even if we fail to read it
Closed	martinvonz	D7625 config: catch intended exception when failing to parse config
Closed	martinvonz	D7627 config: drop debug messages saying where config was read from
Closed	martinvonz	D7624 rcutil: don't check if defaultrc/ is a directory -- we know it is
Closed	martinvonz	D7623 tests: make test-config-env.py a little less hacky
Closed	martinvonz	D7622 help: get helptext/ data from `resources` module if available
Closed	martinvonz	D7621 pycompat: allow pycompat.sysbytes() even if input already is bytes

Diff 18816

mercurial/commands.py

	)			)
	return			return
	ui.pager(b'config')			ui.pager(b'config')
	fm = ui.formatter(b'config', opts)			fm = ui.formatter(b'config', opts)
	for t, f in rcutil.rccomponents():			for t, f in rcutil.rccomponents():
	if t == b'path':			if t == b'path':
	ui.debug(b'read config from: %s\n' % f)			ui.debug(b'read config from: %s\n' % f)
	elif t == b'items':			elif t == b'items':
	for section, name, value, source in f:			# Don't print anything for 'items'.
	ui.debug(b'set config by: %s\n' % source)			pass
	else:			else:
	raise error.ProgrammingError(b'unknown rctype: %s' % t)			raise error.ProgrammingError(b'unknown rctype: %s' % t)
	untrusted = bool(opts.get(b'untrusted'))			untrusted = bool(opts.get(b'untrusted'))

	selsections = selentries = []			selsections = selentries = []
	if values:			if values:
	selsections = [v for v in values if b'.' not in v]			selsections = [v for v in values if b'.' not in v]
	selentries = [v for v in values if b'.' in v]			selentries = [v for v in values if b'.' in v]

tests/test-hgrc.t

	--config: ui.traceback=True			--config: ui.traceback=True
	--verbose: ui.verbose=False			--verbose: ui.verbose=False
	--debug: ui.debug=True			--debug: ui.debug=True
	--quiet: ui.quiet=False			--quiet: ui.quiet=False

	with environment variables			with environment variables

	$ PAGER=p1 EDITOR=e1 VISUAL=e2 hg showconfig --debug			$ PAGER=p1 EDITOR=e1 VISUAL=e2 hg showconfig --debug
	set config by: $EDITOR
	set config by: $VISUAL
	set config by: $PAGER
	read config from: $TESTTMP/hgrc			read config from: $TESTTMP/hgrc
	repo: bundle.mainreporoot=$TESTTMP			repo: bundle.mainreporoot=$TESTTMP
	$PAGER: pager.pager=p1			$PAGER: pager.pager=p1
	$VISUAL: ui.editor=e2			$VISUAL: ui.editor=e2
	--verbose: ui.verbose=False			--verbose: ui.verbose=False
	--debug: ui.debug=True			--debug: ui.debug=True
	--quiet: ui.quiet=False			--quiet: ui.quiet=False