D6709: config: add --registered flag to show all known configs

Sun Sep 15 13:08:21 UTC 2019

This revision now requires changes to proceed.
marmoute added a comment.
marmoute requested changes to this revision.

  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 <https://phab.mercurial-scm.org/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.

INLINE COMMENTS

> commands.py:1896
> +                          ('false', 'no')]:
> +                value = value.replace(*tfopt)
> +            if (opts.get('registered') and

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.

> ui.py:796
> +            if isinstance(itemdefault, list):
> +                itemdefault = ' '.join(itemdefault)
>          return itemdefault

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.

> ui.py:835
> +                    # Omit devel, debug and experimental config items unless
> +                    # --verbose is specified.
> +                    continue

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.

> ui.py:841
> +                if item.experimental:
> +                    name = ''.join([name, '(EXPERIMENTAL)'])
> +                yield section, name, value

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)

> test-basic.t:5
>    $ hg config
> -  devel.all-warnings=true
> +  devel.all-warnings=yes
>    devel.default-date=0 0

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.

> test-config.t:244
> +  bookmarks.pushing=
> +  bundle.mainreporoot=$TESTTMP (default: )
> +  chgserver.idletimeout=3600

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 ?

> test-config.t:391
> +  push.pushvars.server=no
> +  rebase.experimental.inmemory=no
> +  rebase.singletransaction=no

This should be flag (EXPERIMENTAL).

> test-config.t:553
> +  bundle.mainreporoot=$TESTTMP (default: )
> +  censor.policy(EXPERIMENTAL)=abort (default: None)
> +  chgserver.idletimeout=3600

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.

> test-config.t:630-653
> +  debug.dirstate.delaywrite=0
> +  devel.all-warnings=yes (default: no)
> +  devel.bundle.delta=
> +  devel.bundle2.debug=no
> +  devel.cache-vfs=None
> +  devel.check-locks=no
> +  devel.check-relroot=no

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

> test-config.t:672-746
> +  experimental.auto-publish=publish
> +  experimental.bundle-phases=no
> +  experimental.bundle2-advertise=yes
> +  experimental.bundle2-output-capture=no
> +  experimental.bundle2.pushback=no
> +  experimental.bundle2lazylocking=no
> +  experimental.bundlecomplevel=None

Theses should be flagged `(EXPERIMENTAL)`

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D6709/new/

REVISION DETAIL
  https://phab.mercurial-scm.org/D6709

To: navaneeth.suresh, #hg-reviewers, av6, marmoute, durin42
Cc: durin42, mharbison72, yuja, pulkit, marmoute, av6, mjpieters, mercurial-devel