Page MenuHomePhabricator

config: add --registered flag to show all known configs
Needs ReviewPublic

Authored by navaneeth.suresh on Fri, Aug 2, 5:48 AM.

Details

Reviewers
av6
marmoute
durin42
Group Reviewers
hg-reviewers
Summary

This patch fixes one of the issues in issue6014. This adds a
--registered flag to hg showconfig to show all known config
options.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

av6 requested changes to this revision.Fri, Aug 2, 8:44 AM
av6 added a subscriber: av6.
av6 added inline comments.
mercurial/commands.py
1774–1775

This needs a better description to differentiate this flag and the default mode (see line 1786).

tests/test-config.t
240

Certainly a good way to find all config options without default values, but in the final implementation of showconfig -a it should not produce any warnings (especially not developer warnings).

It's probably possible to add default values to some of the items this test found, but I think some of them are missing defaults by design (an obvious example being alias..*). In that case it might be better to just skip them.

This revision now requires changes to proceed.Fri, Aug 2, 8:44 AM
navaneeth.suresh marked an inline comment as done.Sat, Aug 3, 2:00 AM
navaneeth.suresh added inline comments.
mercurial/commands.py
1774–1775

Could you please suggest something better?

tests/test-config.t
240

I have ignored the cases with dynamicdefault. But, there are three cases showing a devel-warn: accessing unregistered config item message. Couldn't figure out why it is coming.

Interesting feature for sure. Thanks for looking into it.

The --all flag might be a bit too generic/vague. Maybe --known,
--registered or --default ?

We should skip experimental and deprecated configuration unless
--verbose is specified. And we should flag them as deprecated/experimental.

For config that has multiple alias, it could be a good idea to display
this information too (probably with --verbose)

navaneeth.suresh marked an inline comment as done.Sat, Aug 3, 12:53 PM
navaneeth.suresh edited the summary of this revision. (Show Details)
navaneeth.suresh retitled this revision from config: add --all flag to show all known configs to config: add --registered flag to show all known configs.
navaneeth.suresh updated this revision to Diff 16119.
pulkit added a subscriber: pulkit.Wed, Aug 7, 10:16 AM
pulkit added inline comments.
mercurial/ui.py
821

Since, we renamed the flag which was good idea, let's rename the function too.

Also, this should better return section, name, value, default_value.

tests/test-config.t
230

In cases when the default value and the value of config set are different, let's do something like <current-output> (default=<default_value>)

240

Just for record, @av6 comment here is not done as there are still devel-warnings.

829

Can you look as why these devel-warning are coming up?

Since we are using --registered, having warnings related to unregistered config does not seems good.

marmoute requested changes to this revision.Wed, Aug 7, 10:23 AM
marmoute added inline comments.
tests/test-config.t
230

Displaying the default vs current is good idea, but the current proposal might be a bit odd:

If nothing is set we will have:

annotate.ignoreblanklines=False

If something is set and is the same as the default we'll have

annotate.ignoreblanklines=False

If something is set and is different from the repo we will have

annotate.ignoreblanklines=True (default: False)

The first two cases can't be distinguish while they probably should, and the default value is displayed differently between the first and third case.

231

Note: we I think we should use yes/no instead of True and False.

This revision now requires changes to proceed.Wed, Aug 7, 10:23 AM

@pulkit I couldn't figure out why the devel-warnings are coming. It's being shown for the following
items in configitems.py. IIUC, it should be visible when we are trying to read a config item which is
not in the registered items. But, we are iterating over registered items only.

coreconfigitem('help', br'hidden-command\..*',
    default=False,
    generic=True,
)
coreconfigitem('help', br'hidden-topic\..*',
    default=False,
    generic=True,
)
coreconfigitem('hostsecurity', '.*:fingerprints$',
    default=list,
    generic=True,
)
coreconfigitem('hostsecurity', '.*:verifycertsfile$',
    default=None,
    generic=True,
)

Also, why would we want walkregisteredconfig() to return section, name, value, defaultvalue ?
If we do that, then it would require modification in ui.walkregisteredconfig() and commands.config().

@av6 If you have a strong opinion on replacing True/False with yes/no, I can do that.

@pulkit I couldn't figure out why the devel-warnings are coming. It's being shown for the following
items in configitems.py. IIUC, it should be visible when we are trying to read a config item which is
not in the registered items. But, we are iterating over registered items only.

There are "generic" config option. So it does not hold the option "name", but a regexp to match valid entry.

@av6 If you have a strong opinion on replacing True/False with yes/no, I can do that.

Not sure is @av6 has a strong opinion, but I do :-)

@pulkit I couldn't figure out why the devel-warnings are coming. It's being shown for the following
items in configitems.py. IIUC, it should be visible when we are trying to read a config item which is
not in the registered items. But, we are iterating over registered items only.

There are "generic" config option. So it does not hold the option "name", but a regexp to match valid entry.

Fixed.

@av6 If you have a strong opinion on replacing True/False with yes/no, I can do that.

Not sure is @av6 has a strong opinion, but I do :-)

Fixed this too. Sorry I was supposed to type @marmoute instead of @av6. My bad.

pulkit added a comment.Fri, Aug 9, 2:46 PM

@pulkit I couldn't figure out why the devel-warnings are coming. It's being shown for the following
items in configitems.py. IIUC, it should be visible when we are trying to read a config item which is
not in the registered items. But, we are iterating over registered items only.

There are "generic" config option. So it does not hold the option "name", but a regexp to match valid entry.

@av6 If you have a strong opinion on replacing True/False with yes/no, I can do that.

Not sure is @av6 has a strong opinion, but I do :-)

I don't want to discuss whether yes/no are better or True/False but when showing the default value, let's show the exact same which is there even though they mean the same?

pulkit added inline comments.Fri, Aug 9, 2:52 PM
mercurial/commands.py
1888

unrequired change I guess

tests/test-config.t
230

In the second case above,

How about something like:

annotate.ignoreblanklines=False  (default: False)

i.e. we show (default ..) if the user has set that config option.

990

Can you add some tests for hg config <some-config> --registered?

marmoute added inline comments.Sat, Aug 10, 2:56 PM
tests/test-config.t
232

the (default: False) should be (default: no) too.

navaneeth.suresh added inline comments.
tests/test-config.t
232

that change was there in my patch one revision before. i was asked by @pulkit to modify it as of now. would you recommend writing no to fm.data() also? the defaultvalue was not in pycompat.bytestr() after @yuja's suggestion. it would be better if we consider that as a string and change the function accordingly if we want to modify it. otherwise, it should print no and store False, print False on hg showconfig --Tjson in defaultvalue.

marmoute added inline comments.Sun, Aug 11, 7:04 AM
tests/test-config.t
232

We should be consistent when printing. And we should also be consistent when recording stuff in data.

I think it make sense to print human friendly version ('yes/no') and use the actual boolean value in data()/json

tests/test-config.t
232

okay. then, i should revert it to the previous revision.

Should we hide devel.* like we do experimental when verbose isn’t applied? Presumably we don’t want regular users to know about them.

Should we hide devel.* like we do experimental when verbose isn’t applied? Presumably we don’t want regular users to know about them.

Done.

I missed the debug one too, sorry. (Although I’m not sure why that also isn’t devel., so maybe it isn’t a big deal. Curious what others think.)

We will need more than just matching the categoie name to detect experimental/deprecated config. There are such option outside of the experimental section, and we still need to hide them by default. You can grep for # experimental config: to see some example.

We will need more than just matching the categoie name to detect experimental/deprecated config. There are such option outside of the experimental section, and we still need to hide them by default. You can grep for # experimental config: to see some example.

I only see one of them rebase.experimental.inmemory. I think it will be nice to have configs option correctly put inside experimental section instead of adding more filtering here.

I get 28 of them.

I think we need to add an experimental or status argument ot the config registration.

> ag --nofilename '# experimental config:.*' -o | sort -u
# experimental config: censor.policy
# experimental config: cmdserver.max-repo-cache
# experimental config: cmdserver.message-encodings
# experimental config: commands.grep.all-files
# experimental config: commands.status.skipstates
# experimental config: convert.ignoreancestorcheck
# experimental config: experimental.graphshorten
# experimental config: experimental.graphstyle.*
# experimental config: experimental.hook-track-tags
# experimental config: format.chunkcachesize
# experimental config: format.generaldelta
# experimental config: format.internal-phase
# experimental config: format.manifestcachesize
# experimental config: format.maxchainlen
# experimental config: fsmonitor.verbose
# experimental config: merge.preferancestor
# experimental config: patchbomb.publicurl
# experimental config: perf.all-timing
# experimental config: perf.parentscount
# experimental config: perf.presleep
# experimental config: perf.run-limits
# experimental config: perf.stub
# experimental config: repack.chainorphansbysize
# experimental config: sparse.missingwarning
# experimental config: storage.new-repo-backend
# experimental config: storage.sqlite.compression
# experimental config: web.view

I get 28 of them.
I think we need to add an experimental or status argument ot the config registration.

Created D6728 to add an experimental argument to the config registrar and made it as a parent revision.

durin42 accepted this revision as: durin42.Thu, Aug 15, 11:13 AM
durin42 added a subscriber: durin42.

Looks good to me, but I want someone else to put eyes on it before it gets pushed.

navaneeth.suresh marked an inline comment as done.Thu, Aug 15, 3:42 PM
navaneeth.suresh updated this revision to Diff 16216.
av6 added a comment.Fri, Aug 16, 3:51 AM

It looks fine to me, one thing that could be improved is the output format of list values. For example, progress.format: its default value is shown to be ['topic', 'bar', 'number', 'estimate'], but it doesn't match hg help config.progress.format (it says default: topic bar number estimate), and that is also not the format that users would need to use for the actual value in hgrc or using --config flag.

mercurial/ui.py
825–826

Nit: this condition could be made into another if ...: continue block just like the one below, making things less indented.

av6 requested changes to this revision.Sat, Aug 17, 8:55 AM
This revision now requires changes to proceed.Sat, Aug 17, 8:55 AM
av6 added inline comments.Sat, Aug 17, 8:59 AM
mercurial/ui.py
830

Nit: originally I was fine with two different if ...: continue blocks here, and I still think it would be better. Not only the condition here is fairly complex and spans multiple lines (so not easy to read), but this comment is not quite correct. We always omit items with dynamic defaults and generic items, but experimental (& etc) items are only omitted unless --verbose is given. Two comments in two if-blocks with two separate conditions here would be more readable.

av6 added inline comments.Wed, Aug 21, 12:48 AM
mercurial/commands.py
1893–1904

Let's rename this variable to better reflect its intent. AFAIU, a better name would be rawvalue or origvalue.

av6 accepted this revision.Thu, Aug 22, 11:27 PM

@marmoute did you want this flag marked as experimental or am I misremembering?

In D6709#99166, @av6 wrote:

@marmoute did you want this flag marked as experimental or am I misremembering?

@av6 @marmoute told me to add EXPERIMENTAL in the output of the config options which are experimental but, not come under the section experimental.
There are 28 of them. For example, sparse.missingwarning(EXPERIMENTAL)=yes (default: None). We had this conversation in IRC. I think you are
referring to that.

av6 requested changes to this revision.Sat, Aug 24, 1:24 AM

I'm strongly -1 on adding qualifiers straight to the keys. The primary usage scenario here is scripts (as explained in issue6014) -- regular users lived without this feature for years and didn't care (enough to file a bug) about getting all config options, or their default values, or the experimental status (they could and still can read it in hg help config). --registered is needed for things like shell completions, and the output needs to be clear and parseable (and consistent with what regular showconfig shows).

Even adding "(default: x)" to values already complicates the parsing process to get multiple config options in a script. Yeah, a script can get individual values one by one, but it can take ages because of the start-up time (again, this is important for interactive scripts like shell completions).

Let's add a test to this patch that demonstrates that scripts can easily use this new functionality. Something like $ hg showconfig --registered -T '{name}={value}\n' | egrep '^(color|diff)'. Notice how this uses -T, which is recommended, but people writing scripts in the wild may not even use, and extra things in the output (i.e. anything other than just key and value) will trip their scripts up. Maaaybe if we put a strongly worded note in hg help showconfig about scripting and usage of -T, maybe then adding "(EXPERIMENTAL)" would make sense. But I think the current format of adding it to the key is weird and it would be better to add it to the end of the line, similar to what experimental CLI flags have in the output of hg help -v command (if at all, because, again, that is not how regular showconfig behaves and we do have both defaults and experimental statuses in hg help config).

Sorry for coming up with this new condition only now, but I thought everyone was already on the same page.

This revision now requires changes to proceed.Sat, Aug 24, 1:24 AM

@av6 I have updated the tests. @marmoute can we just get rid of (EXPERIMENTAL) in the output? If we want to do that, changes have to be made in ui.walkregisteredconfig(). But, that's a hack which appends to either name or value and as @av6 mentioned, the user will see it on using with -T which they might not wanted.