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

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

tests/test-config.t
238

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

Could you please suggest something better?

tests/test-config.t
238

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
228

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

238

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

827

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
228

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.

229

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
228

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.

988

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
230

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

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

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
230

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
230

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.