| av6 | |
| marmoute | |
| durin42 | |
| baymax |
| hg-reviewers |
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.
| Lint Skipped |
| Unit Tests Skipped |
| 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. | |
@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.
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?
| 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? | |
| tests/test-config.t | ||
|---|---|---|
| 232 | the (default: False) should be (default: no) too. | |
| 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. | |
| 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.
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.
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
Created D6728 to add an experimental argument to the config registrar and made it as a parent revision.
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. | |
| 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. | |
| mercurial/commands.py | ||
|---|---|---|
| 1893–1894 | Let's rename this variable to better reflect its intent. AFAIU, a better name would be rawvalue or origvalue. | |
@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.
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.
@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.
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.)
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. | |
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.
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 | ||
|---|---|---|
| 1896 | 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 | 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) | |
Alternatively, what if we just dropped the (default: x), (EXPERIMENTAL), and whatever else complicates parsing if HGPLAIN is set?
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:
* 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:
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
+1. I also agree that splitting it into patches will help ease the discussion and review.
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.
There seems to have been no activities on this Diff for the past 3 Months.
By policy, we are automatically moving it out of the need-review state.
Please, move it back to need-review without hesitation if this diff should still be discussed.
:baymax:need-review-idle: