Page MenuHomePhabricator

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

Authored by navaneeth.suresh on Aug 2 2019, 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.

I will be sending a series of patches following this to adding defaults
and experimental/devel/debug statuses, displaying the outputs in a
user-friendly format, etc.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
pulkit added a subscriber: pulkit.Aug 7 2019, 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.Aug 7 2019, 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.Aug 7 2019, 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.Aug 9 2019, 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.Aug 9 2019, 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.Aug 10 2019, 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.Aug 11 2019, 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.Aug 15 2019, 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.Aug 15 2019, 3:42 PM
navaneeth.suresh updated this revision to Diff 16216.
av6 added a comment.Aug 16 2019, 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.Aug 17 2019, 8:55 AM
This revision now requires changes to proceed.Aug 17 2019, 8:55 AM
av6 added inline comments.Aug 17 2019, 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.Aug 21 2019, 12:48 AM
mercurial/commands.py
1898

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

av6 accepted this revision.Aug 22 2019, 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.Aug 24 2019, 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.Aug 24 2019, 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.

durin42 accepted this revision.Sep 11 2019, 11:22 AM

I'm okay with this as-is. Barring an objection in a week or so I'll queue it.

(It's also possible I've missed something important when I read through the extensive review history here. Apologies if true, but please restate your concerns if you still have them on the current version of the patch.)

av6 added a comment.Sep 13 2019, 12:12 AM

I'll be okay with this too, once we get rid of (EXPERIMENTAL) everywhere.

tests/test-config.t
962

Let's change this to hg showconfig cmdserver --registered --verbose -T '{name}={value}\n', no grep. cmdserver section has a variety of options, including experimental ones, plus that would test showing config sections by name with --registered.

marmoute requested changes to this revision.Sep 15 2019, 9:07 AM

Sorry for the late review. I though this diff was taken while I was in vacation/unstacking-post-vacation. I just recently reappeared on my rader. I made multiple comments about the implementation, its output, and unexpected output changes.

In D6709#99199, @av6 wrote:

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

I disagree that the primary use is script only. As a suer I have wanted this for a long time and I am happy to finally have a way to get this data without grepping the source.

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

May we should drop the (default: x) part unless --verbose is specified. That would make sense to me. Since experimental/devel/debug(/deprecated?) only appears with --verbose that might work just fine for you ?

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

+1 for changing the place where it appears.

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

mercurial/commands.py
1901

This part seems fishy as it seems to apply to all hg config run, not only --registered one. I don't think we want that outside of --registered, transforming the config content the user put explicitly is likely to confuses them. In addition, we might end up transformation non-boolean value that happens to match these entry.

Instead we should simply use transform boolean to 'yes', 'no' when we meet them. I suggest adding a formatconfigvalue(value) function that would do so and that we could use instead of pycompat.bytestr(value) in various places.

mercurial/ui.py
796

Two feedbacks

First, this does not seems the right location to do this transformation. The method is expected to return the default value of a config. In case of a list, we should return a list, not transform it to text. If you need to transform the list to text for display purpose, this should happens at the layers responsible for display.

Second, the code parsing list at https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/config.py#l203 show that list formatting can get a bit more complicated. Since the list can be space separated (as used here, any value containings spaces will be need to be quoted (and quote within it escaped…).

This could be handled by formatconfigvalue function discussed earlier.

833

small feedback: We should probably not rely on hardcoded section name and rely on attributes on the config items only, it would be more robust.

839

Here is an example the possible inconsistency introduced by explicite section matching: not all experimental item will be tagged experimental.

Formatting wise, happening (EXPERIMENTAL) like that to the name seems weird and does not match what we are doing of other (EXPERIMENTAL) flag, (eg: command --flag)

tests/test-basic.t
5 ↗(On Diff #16306)

This are unrelated (and maybe unwanted) changes from the transformation I flagged earlier.

If we really want them, they should be in their own changesets. But I don't think we do.

tests/test-config.t
244

This empty default is a bit strange. Maybe we want to explicitly quote empty string ? Or maybe this default value should be None and not "") or use <empty> or something. @av6 what do you think ?

391

This should be flag (EXPERIMENTAL).

553

the (EXPERIMENTAL) flag should be at the end:

censor.policy=abort (EXPERIMENTAL)

In addition this (default: None) display looks wrong. "None" is an internal python value with no meaning in our config interface to the user. People cannot use "None" is their config file to mean an underlying None python reference, so we should not display that to them.

630–653

We should get a dedicated attribute form them too. Something like (DEVELOPER) or (DEBUG)

672–746

Theses should be flagged (EXPERIMENTAL)

This revision now requires changes to proceed.Sep 15 2019, 9:07 AM
In D6709#99199, @av6 wrote:

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

I disagree that the primary use is script only. As a suer I have wanted this for a long time and I am happy to finally have a way to get this data without grepping the source.

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

May we should drop the (default: x) part unless --verbose is specified. That would make sense to me. Since experimental/devel/debug(/deprecated?) only appears with --verbose that might work just fine for you ?

Alternatively, what if we just dropped the (default: x), (EXPERIMENTAL), and whatever else complicates parsing if HGPLAIN is set?

av6 added a comment.Sep 17 2019, 4:58 AM
In D6709#99199, @av6 wrote:

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

I disagree that the primary use is script only. As a suer I have wanted this for a long time and I am happy to finally have a way to get this data without grepping the source.

Oh, now I see what you wanted from this patch. But you want it as a developer who works with and on hg, not just as a user (hopefully regular users don't have to grep the source). This distinction makes things easier to design, because we now have simply two (plus one) cases:

  • --registered will simply extend the list of config options to walk from options found somewhere in system/user/repo config files to every config option ever registered (excluding experimental/devel/debug ones unless -v is given*)
  • existing --debug mode of showconfig will show the (EXPERIMENTAL) and (default: x) information in addition to what it shows now and can be used with --registered (and registered but unset options can have "<registered>" source, similar to the existing "<tweakdefaults>" source)
  • anyone can still use -T to access key, value, defaultvalue, source and status in whatever format they want, and --registered will work as expected from its name (i.e. just iterating over a different set of config options)

* This is still not great, because if you set an experimental config option and run showconfig with no flags, it will be in the output. With --registered as it is implemented right now it won't be shown without -v. We should either show any options when they are set regardless of what status they have, or not require -v at all.

Making one flag do one thing is good UI. That's why I want --registered alone to not modify any output of showconfig. Doing only one thing is also easier for help text to explain and for users to understand (even without reading help text, just based on the name of the flag).

And to make the review process easier, I propose doing this in 3 or 4 patches:

  • adding --registered: will simply show all registered options in key=value format
  • adding defaults and experimental/devel/debug statuses to showconfig --debug output (this one can be split into two)
  • showing config options in a human-friendly format
In D6709#100698, @av6 wrote:
In D6709#99199, @av6 wrote:

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

I disagree that the primary use is script only. As a suer I have wanted this for a long time and I am happy to finally have a way to get this data without grepping the source.

Oh, now I see what you wanted from this patch. But you want it as a developer who works with and on hg, not just as a user (hopefully regular users don't have to grep the source). This distinction makes things easier to design, because we now have simply two (plus one) cases:

I have talked with regular user who also crave for it because their keep wondering what's the exact form of the option their are looking for.

  • --registered will simply extend the list of config options to walk from options found somewhere in system/user/repo config files to every config option ever registered (excluding experimental/devel/debug ones unless -v is given*)
  • existing --debug mode of showconfig will show the (EXPERIMENTAL) and (default: x) information in addition to what it shows now and can be used with --registered (and registered but unset options can have "<registered>" source, similar to the existing "<tweakdefaults>" source)
  • anyone can still use -T to access key, value, defaultvalue, source and status in whatever format they want, and --registered will work as expected from its name (i.e. just iterating over a different set of config options)

* This is still not great, because if you set an experimental config option and run showconfig with no flags, it will be in the output. With --registered as it is implemented right now it won't be shown without -v. We should either show any options when they are set regardless of what status they have, or not require -v at all.
Making one flag do one thing is good UI. That's why I want --registered alone to not modify any output of showconfig. Doing only one thing is also easier for help text to explain and for users to understand (even without reading help text, just based on the name of the flag).

That is an interesting point. I'll have to think about it for a bit.

And to make the review process easier, I propose doing this in 3 or 4 patches:

  • adding --registered: will simply show all registered options in key=value format
  • adding defaults and experimental/devel/debug statuses to showconfig --debug output (this one can be split into two)
  • showing config options in a human-friendly format
In D6709#100698, @av6 wrote:

And to make the review process easier, I propose doing this in 3 or 4 patches:

  • adding --registered: will simply show all registered options in key=value format
  • adding defaults and experimental/devel/debug statuses to showconfig --debug output (this one can be split into two)
  • showing config options in a human-friendly format

+1. I also agree that splitting it into patches will help ease the discussion and review.

navaneeth.suresh edited the summary of this revision. (Show Details)Oct 4 2019, 1:39 PM
navaneeth.suresh updated this revision to Diff 16800.

Now I understand that this patch does an enormous number of changes. I've split this patch into three. Will be working on each of them separately from now on.