Patchwork [2,of,2,RFC] config: introduce platform specific sections

login
register
mail settings
Submitter Matt Harbison
Date March 20, 2015, 10:36 p.m.
Message ID <636f51ff2cfc9bbd665f.1426890986@MATT7H-PC.attotech.com>
Download mbox | patch
Permalink /patch/8210/
State Deferred
Headers show

Comments

Matt Harbison - March 20, 2015, 10:36 p.m.
# HG changeset patch
# User Matt Harbison <matt_harbison@yahoo.com>
# Date 1426888879 14400
#      Fri Mar 20 18:01:19 2015 -0400
# Node ID 636f51ff2cfc9bbd665f1b3bf198d780a2afffe8
# Parent  6b3aff8a06f1cf7057a9bdcdc5e301dde0801828
config: introduce platform specific sections

The current method of dealing with platform differences is to use environment
variables and/or refactor config files into shared and platform specific files,
and %include them as appropriate.  When dealing with several versions of
multiple platforms in a corporate environment, it is much more convenient to
have configuration options contained within a single portable file, with an
occasional platform specific tweak, because the settings are always in a single
location and the only setup is to correctly place a single file.

This change allows the section name to be augmented with the system name, and
the section is either loaded (and retargeted to the base section name), or
ignored, depending upon the current platform.  No special precendence is given
to the platform section- it can be overwritten by a subsequent section.  The
ignored area starts at the beginning of the platform specific section and ends
at the start of the next section, so include and unset directives are considered
part of the previous section.

For example:

        [alias]
        bc = bcompare

        [alias.windows]
        bc = beyondcompare3

        [extensions]
        mercurial_keyring = ~/hgext/mercurial_keyring/mercurial_keyring.py

        [extensions.windows]
        # Bundled with TortoiseHG
        mercurial_keyring =

will use the 'beyondcompare3' extdiff tool on Windows only, falling back to
'bcompare' elsewhere.  The bundled keyring extension is used on Windows,
otherwise it is loaded from the user's home directory.

Core Mercurial could benefit some from this too, since the BeyondCompare config
for Linux was recently duplicated and renamed for OS X in 14d647d25c70, with
only the executable name changed.  It could probably help other projects that
bundle Mercurial- it looks like thg appended '-win' to the terminaltools
executable name to avoid this problem.

I opted to use the section name instead of trying to change individual keys for
simplicity and compatibility.  There are currently no section names defined with
a '.' in core.  It would be strange if a 3rd party did so, because the way the
file is parsed, the entire text between '[' and ']' was treated as the name.
But with --config on the command line, the section ends at the first '.'.
Therefore, if a 3rd party did use '.' in the name, it couldn't be overridden on
the command line.  I don't think that is a problem here, because it is less
typing to simply omit the current platform name when overriding with --config.

I didn't give any consideration to other platform attributes, like architecture
or OS version.  I have no idea if that would be useful in the future, but I
don't see any reason why the version can't simply be appended on in the future
if it is.  'platform.system()' returned sane results on Windows 7, OS X 10.6.8,
Fedora 16 and FreeBSD 10.1.  I'm not sure if the name part should be moved into
util, in case other more exotic systems need to tweak the value.
Matt Mackall - March 23, 2015, 8:02 p.m.
On Fri, 2015-03-20 at 18:36 -0400, Matt Harbison wrote:
> # HG changeset patch
> # User Matt Harbison <matt_harbison@yahoo.com>
> # Date 1426888879 14400
> #      Fri Mar 20 18:01:19 2015 -0400
> # Node ID 636f51ff2cfc9bbd665f1b3bf198d780a2afffe8
> # Parent  6b3aff8a06f1cf7057a9bdcdc5e301dde0801828
> config: introduce platform specific sections

Please take a peek at Greg's patches where he's proposing using
[foo.bar] for something else. I can't say I'm excited by either
proposal.
Matt Harbison - March 26, 2015, 1:15 a.m.
On Mon, 23 Mar 2015 16:02:16 -0400, Matt Mackall <mpm@selenic.com> wrote:

> On Fri, 2015-03-20 at 18:36 -0400, Matt Harbison wrote:
>> # HG changeset patch
>> # User Matt Harbison <matt_harbison@yahoo.com>
>> # Date 1426888879 14400
>> #      Fri Mar 20 18:01:19 2015 -0400
>> # Node ID 636f51ff2cfc9bbd665f1b3bf198d780a2afffe8
>> # Parent  6b3aff8a06f1cf7057a9bdcdc5e301dde0801828
>> config: introduce platform specific sections
>
> Please take a peek at Greg's patches where he's proposing using
> [foo.bar] for something else. I can't say I'm excited by either
> proposal.
>

Yep, I saw he hadn't ruled that out yet and pinged him on Monday.

In the meantime, do you have any alternate ideas for this?  I wasn't sure  
how your

   new.style =
   old.style.path =

suggestion would apply to this, since that probably requires knowledge of  
what is possible in each section.  ([extensions] for example allows a  
leading 'hgext.', so it isn't a general split on '.' and compare  
algorithm.)

--Matt
Yuya Nishihara - March 26, 2015, 2:35 p.m.
On Wed, 25 Mar 2015 21:15:27 -0400, Matt Harbison wrote:
> On Mon, 23 Mar 2015 16:02:16 -0400, Matt Mackall <mpm@selenic.com> wrote:
> > On Fri, 2015-03-20 at 18:36 -0400, Matt Harbison wrote:
> >> # HG changeset patch
> >> # User Matt Harbison <matt_harbison@yahoo.com>
> >> # Date 1426888879 14400
> >> #      Fri Mar 20 18:01:19 2015 -0400
> >> # Node ID 636f51ff2cfc9bbd665f1b3bf198d780a2afffe8
> >> # Parent  6b3aff8a06f1cf7057a9bdcdc5e301dde0801828
> >> config: introduce platform specific sections
> >
> > Please take a peek at Greg's patches where he's proposing using
> > [foo.bar] for something else. I can't say I'm excited by either
> > proposal.
> >
> 
> Yep, I saw he hadn't ruled that out yet and pinged him on Monday.
> 
> In the meantime, do you have any alternate ideas for this?  I wasn't sure  
> how your
> 
>    new.style =
>    old.style.path =
> 
> suggestion would apply to this, since that probably requires knowledge of  
> what is possible in each section.  ([extensions] for example allows a  
> leading 'hgext.', so it isn't a general split on '.' and compare  
> algorithm.)

As the magic is done at config.parse() layer, how about %if like a C
preprocessor?

  [systest]
  %if $platform == 'win32'  # or %ifeq(...,...) ?
  key = windowsvalue
  %elif $platform == 'linux'
  key = linuxvalue
  %endif

(I don't know if mpm likes it. ;)

Regards,
Matt Harbison - March 26, 2015, 11:59 p.m.
On Thu, 26 Mar 2015 10:35:03 -0400, Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 25 Mar 2015 21:15:27 -0400, Matt Harbison wrote:
>> On Mon, 23 Mar 2015 16:02:16 -0400, Matt Mackall <mpm@selenic.com>  
>> wrote:
>> > On Fri, 2015-03-20 at 18:36 -0400, Matt Harbison wrote:
>> >> # HG changeset patch
>> >> # User Matt Harbison <matt_harbison@yahoo.com>
>> >> # Date 1426888879 14400
>> >> #      Fri Mar 20 18:01:19 2015 -0400
>> >> # Node ID 636f51ff2cfc9bbd665f1b3bf198d780a2afffe8
>> >> # Parent  6b3aff8a06f1cf7057a9bdcdc5e301dde0801828
>> >> config: introduce platform specific sections
>> >
>> > Please take a peek at Greg's patches where he's proposing using
>> > [foo.bar] for something else. I can't say I'm excited by either
>> > proposal.
>> >
>>
>> Yep, I saw he hadn't ruled that out yet and pinged him on Monday.
>>
>> In the meantime, do you have any alternate ideas for this?  I wasn't  
>> sure
>> how your
>>
>>    new.style =
>>    old.style.path =
>>
>> suggestion would apply to this, since that probably requires knowledge  
>> of
>> what is possible in each section.  ([extensions] for example allows a
>> leading 'hgext.', so it isn't a general split on '.' and compare
>> algorithm.)
>
> As the magic is done at config.parse() layer, how about %if like a C
> preprocessor?
>
>   [systest]
>   %if $platform == 'win32'  # or %ifeq(...,...) ?
>   key = windowsvalue
>   %elif $platform == 'linux'
>   key = linuxvalue
>   %endif
>
> (I don't know if mpm likes it. ;)
>

Interesting thought, thanks.  I'll take a look at it this weekend.

Any thoughts about the form of the $platform part, with an eye toward  
future expansion?  I didn't think about it before (and don't have a use  
for it currently), but it seems like env variables could be checked here  
too.  The %ifeq() makes me wonder if the templater methods can be used  
too, but I don't want to make this tooo complicated.  I just don't want to  
cut future expansion off at the knees (yes, I know the original proposal  
did just that).

--Matt
Gregory Szorc - March 28, 2015, 7:19 p.m.
On Thu, Mar 26, 2015 at 7:35 AM, Yuya Nishihara <yuya@tcha.org> wrote:

> On Wed, 25 Mar 2015 21:15:27 -0400, Matt Harbison wrote:
> > On Mon, 23 Mar 2015 16:02:16 -0400, Matt Mackall <mpm@selenic.com>
> wrote:
> > > On Fri, 2015-03-20 at 18:36 -0400, Matt Harbison wrote:
> > >> # HG changeset patch
> > >> # User Matt Harbison <matt_harbison@yahoo.com>
> > >> # Date 1426888879 14400
> > >> #      Fri Mar 20 18:01:19 2015 -0400
> > >> # Node ID 636f51ff2cfc9bbd665f1b3bf198d780a2afffe8
> > >> # Parent  6b3aff8a06f1cf7057a9bdcdc5e301dde0801828
> > >> config: introduce platform specific sections
> > >
> > > Please take a peek at Greg's patches where he's proposing using
> > > [foo.bar] for something else. I can't say I'm excited by either
> > > proposal.
> > >
> >
> > Yep, I saw he hadn't ruled that out yet and pinged him on Monday.
> >
> > In the meantime, do you have any alternate ideas for this?  I wasn't sure
> > how your
> >
> >    new.style =
> >    old.style.path =
> >
> > suggestion would apply to this, since that probably requires knowledge of
> > what is possible in each section.  ([extensions] for example allows a
> > leading 'hgext.', so it isn't a general split on '.' and compare
> > algorithm.)
>
> As the magic is done at config.parse() layer, how about %if like a C
> preprocessor?
>
>   [systest]
>   %if $platform == 'win32'  # or %ifeq(...,...) ?
>   key = windowsvalue
>   %elif $platform == 'linux'
>   key = linuxvalue
>   %endif
>
> (I don't know if mpm likes it. ;)
>
>
I'm also not a huge fan of adopting [X.platform] for platform-specific
bits. That prevents us from using [X.Y] sections for anything else, which I
think is a bit short-sighted.

I kinda like the preprocessor idea. Although, the more I interact with
preprocessors, the more I learn that it is often best to avoid them. Once
you introduce a preprocessing step, any tool that parses the file needs to
know about preprocessing. And since Mercurial doesn't have a built-in
config editor, this would further limit the ability for 3rd party tools to
rewrite Mercurial's mostly-ini config files. (The "mercurial-setup" tool I
wrote for Mozilla is one such tool.)

I'm a much bigger fan of sticking to defined data formats as much as
possible. i.e. "use ini."

Here are some alternatives.

Introduce special options within sections to denote properties. e.g. all
UPPERCASE options could be reserved for metadata:

[systest]
PLATFORM = win32
key = windowsvalue

[systest]
PLATFORM = linux
key = linuxvalue

[systest]
PLATFORM = linux, osx
key = posixvalue

Or, put special syntax in the section title after a space.

[systest platform=win32]
key = windowsvalue

[systest platform=linux,osx]
key = posixvalue

Not as powerful as preprocessing, yes. But it preserves ini compatibility,
which I don't think should be under-estimated.

(FWIW, the %include directive supported in config files today already
breaks ini compatibility.)
Matt Harbison - March 29, 2015, 1:12 a.m.
On Sat, 28 Mar 2015 15:19:50 -0400, Gregory Szorc  
<gregory.szorc@gmail.com> wrote:

> On Thu, Mar 26, 2015 at 7:35 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>
>> On Wed, 25 Mar 2015 21:15:27 -0400, Matt Harbison wrote:
>> > On Mon, 23 Mar 2015 16:02:16 -0400, Matt Mackall <mpm@selenic.com>
>> wrote:
>> > > On Fri, 2015-03-20 at 18:36 -0400, Matt Harbison wrote:
>> > >> # HG changeset patch
>> > >> # User Matt Harbison <matt_harbison@yahoo.com>
>> > >> # Date 1426888879 14400
>> > >> #      Fri Mar 20 18:01:19 2015 -0400
>> > >> # Node ID 636f51ff2cfc9bbd665f1b3bf198d780a2afffe8
>> > >> # Parent  6b3aff8a06f1cf7057a9bdcdc5e301dde0801828
>> > >> config: introduce platform specific sections
>> > >
>> > > Please take a peek at Greg's patches where he's proposing using
>> > > [foo.bar] for something else. I can't say I'm excited by either
>> > > proposal.
>> > >
>> >
>> > Yep, I saw he hadn't ruled that out yet and pinged him on Monday.
>> >
>> > In the meantime, do you have any alternate ideas for this?  I wasn't  
>> sure
>> > how your
>> >
>> >    new.style =
>> >    old.style.path =
>> >
>> > suggestion would apply to this, since that probably requires  
>> knowledge of
>> > what is possible in each section.  ([extensions] for example allows a
>> > leading 'hgext.', so it isn't a general split on '.' and compare
>> > algorithm.)
>>
>> As the magic is done at config.parse() layer, how about %if like a C
>> preprocessor?
>>
>>   [systest]
>>   %if $platform == 'win32'  # or %ifeq(...,...) ?
>>   key = windowsvalue
>>   %elif $platform == 'linux'
>>   key = linuxvalue
>>   %endif
>>
>> (I don't know if mpm likes it. ;)
>>
>>
> I'm also not a huge fan of adopting [X.platform] for platform-specific
> bits. That prevents us from using [X.Y] sections for anything else,  
> which I
> think is a bit short-sighted.

Yeah, I was concerned about that too.  I only ignored it because I  
couldn't think of something else that needed to apply at a section level.   
And without a concrete second example, I tend to over-engineer the future  
compatibility aspects.  But it's a fair point.

> I kinda like the preprocessor idea. Although, the more I interact with
> preprocessors, the more I learn that it is often best to avoid them. Once
> you introduce a preprocessing step, any tool that parses the file needs  
> to
> know about preprocessing. And since Mercurial doesn't have a built-in
> config editor, this would further limit the ability for 3rd party tools  
> to
> rewrite Mercurial's mostly-ini config files. (The "mercurial-setup" tool  
> I
> wrote for Mozilla is one such tool.)

That was also a concern, even with the syntax I introduced.  Would an  
editing tool be smart enough to skip over the regular [systest] section to  
edit the platform specific section, and fall back if not present?  I  
suppose overwriting both isn't horrible, just undesirable.

> I'm a much bigger fan of sticking to defined data formats as much as
> possible. i.e. "use ini."
>
> Here are some alternatives.
>
> Introduce special options within sections to denote properties. e.g. all
> UPPERCASE options could be reserved for metadata:
>
> [systest]
> PLATFORM = win32
> key = windowsvalue
>
> [systest]
> PLATFORM = linux
> key = linuxvalue
>
> [systest]
> PLATFORM = linux, osx
> key = posixvalue

I assume the metadata is controlling for the entire section, not just the  
next line?  Given that config.py reads a line and updates the dictionary  
immediately, I wonder if 3rd party editors can screw this up when they  
don't recognize the metadata, and put other variables in the section  
first.  (A 3rd party editor reordering keys seems like a bad idea, but it  
hasn't really mattered to this point, other than maybe comments.)

> Or, put special syntax in the section title after a space.
>
> [systest platform=win32]
> key = windowsvalue
>
> [systest platform=linux,osx]
> key = posixvalue
>
> Not as powerful as preprocessing, yes. But it preserves ini  
> compatibility,
> which I don't think should be under-estimated.

I like this.  It isn't far off from what I originally proposed (split on '  
' instead of '.', then split again on '='), so it should be fairly  
simple.  I think I'll try this, and use Yuya's idea as a backup.

Thinking toward the future, should we disallow a second space, so that  
multiple properties can be specified in the future?  e.g.

[systest platform=win32 arch=amd64]

Is there any reason to care about quoting values on the right side of '=',  
maybe to allow for spaces in the value?  Are there existing string parsing  
methods that can be reused for consistency?

The only other thing I can think of that might be useful here is to be  
able to match a string based on regex or something, like the OS version.   
(There are a couple of things that work differently in 10.10 vs 10.6 that  
I've noticed with thg.)  Obviously '=' won't work for that, so are there  
characters that we should reserve after the first space for future uses?


> (FWIW, the %include directive supported in config files today already
> breaks ini compatibility.)
Gregory Szorc - March 31, 2015, 5:12 a.m.
On Sat, Mar 28, 2015 at 6:12 PM, Matt Harbison <mharbison72@gmail.com>
wrote:

> On Sat, 28 Mar 2015 15:19:50 -0400, Gregory Szorc <gregory.szorc@gmail.com>
> wrote:
>
>  On Thu, Mar 26, 2015 at 7:35 AM, Yuya Nishihara <yuya@tcha.org> wrote:
>>
>>  On Wed, 25 Mar 2015 21:15:27 -0400, Matt Harbison wrote:
>>> > On Mon, 23 Mar 2015 16:02:16 -0400, Matt Mackall <mpm@selenic.com>
>>> wrote:
>>> > > On Fri, 2015-03-20 at 18:36 -0400, Matt Harbison wrote:
>>> > >> # HG changeset patch
>>> > >> # User Matt Harbison <matt_harbison@yahoo.com>
>>> > >> # Date 1426888879 14400
>>> > >> #      Fri Mar 20 18:01:19 2015 -0400
>>> > >> # Node ID 636f51ff2cfc9bbd665f1b3bf198d780a2afffe8
>>> > >> # Parent  6b3aff8a06f1cf7057a9bdcdc5e301dde0801828
>>> > >> config: introduce platform specific sections
>>> > >
>>> > > Please take a peek at Greg's patches where he's proposing using
>>> > > [foo.bar] for something else. I can't say I'm excited by either
>>> > > proposal.
>>> > >
>>> >
>>> > Yep, I saw he hadn't ruled that out yet and pinged him on Monday.
>>> >
>>> > In the meantime, do you have any alternate ideas for this?  I wasn't
>>> sure
>>> > how your
>>> >
>>> >    new.style =
>>> >    old.style.path =
>>> >
>>> > suggestion would apply to this, since that probably requires knowledge
>>> of
>>> > what is possible in each section.  ([extensions] for example allows a
>>> > leading 'hgext.', so it isn't a general split on '.' and compare
>>> > algorithm.)
>>>
>>> As the magic is done at config.parse() layer, how about %if like a C
>>> preprocessor?
>>>
>>>   [systest]
>>>   %if $platform == 'win32'  # or %ifeq(...,...) ?
>>>   key = windowsvalue
>>>   %elif $platform == 'linux'
>>>   key = linuxvalue
>>>   %endif
>>>
>>> (I don't know if mpm likes it. ;)
>>>
>>>
>>>  I'm also not a huge fan of adopting [X.platform] for platform-specific
>> bits. That prevents us from using [X.Y] sections for anything else, which
>> I
>> think is a bit short-sighted.
>>
>
> Yeah, I was concerned about that too.  I only ignored it because I
> couldn't think of something else that needed to apply at a section level.
> And without a concrete second example, I tend to over-engineer the future
> compatibility aspects.  But it's a fair point.
>
>  I kinda like the preprocessor idea. Although, the more I interact with
>> preprocessors, the more I learn that it is often best to avoid them. Once
>> you introduce a preprocessing step, any tool that parses the file needs to
>> know about preprocessing. And since Mercurial doesn't have a built-in
>> config editor, this would further limit the ability for 3rd party tools to
>> rewrite Mercurial's mostly-ini config files. (The "mercurial-setup" tool I
>> wrote for Mozilla is one such tool.)
>>
>
> That was also a concern, even with the syntax I introduced.  Would an
> editing tool be smart enough to skip over the regular [systest] section to
> edit the platform specific section, and fall back if not present?  I
> suppose overwriting both isn't horrible, just undesirable.
>
>  I'm a much bigger fan of sticking to defined data formats as much as
>> possible. i.e. "use ini."
>>
>> Here are some alternatives.
>>
>> Introduce special options within sections to denote properties. e.g. all
>> UPPERCASE options could be reserved for metadata:
>>
>> [systest]
>> PLATFORM = win32
>> key = windowsvalue
>>
>> [systest]
>> PLATFORM = linux
>> key = linuxvalue
>>
>> [systest]
>> PLATFORM = linux, osx
>> key = posixvalue
>>
>
> I assume the metadata is controlling for the entire section, not just the
> next line?  Given that config.py reads a line and updates the dictionary
> immediately, I wonder if 3rd party editors can screw this up when they
> don't recognize the metadata, and put other variables in the section
> first.  (A 3rd party editor reordering keys seems like a bad idea, but it
> hasn't really mattered to this point, other than maybe comments.)
>

I would think this metadata would apply to the whole section and order
wouldn't be matter. Mercurial's config parser would essentially do a
post-processing step to apply special metadata options.


>  Or, put special syntax in the section title after a space.
>>
>> [systest platform=win32]
>> key = windowsvalue
>>
>> [systest platform=linux,osx]
>> key = posixvalue
>>
>> Not as powerful as preprocessing, yes. But it preserves ini compatibility,
>> which I don't think should be under-estimated.
>>
>
> I like this.  It isn't far off from what I originally proposed (split on '
> ' instead of '.', then split again on '='), so it should be fairly simple.
> I think I'll try this, and use Yuya's idea as a backup.
>
> Thinking toward the future, should we disallow a second space, so that
> multiple properties can be specified in the future?  e.g.
>
> [systest platform=win32 arch=amd64]
>

I don't know!


>
> Is there any reason to care about quoting values on the right side of '=',
> maybe to allow for spaces in the value?  Are there existing string parsing
> methods that can be reused for consistency?
>

I also don't know!


>
> The only other thing I can think of that might be useful here is to be
> able to match a string based on regex or something, like the OS version.
> (There are a couple of things that work differently in 10.10 vs 10.6 that
> I've noticed with thg.)  Obviously '=' won't work for that, so are there
> characters that we should reserve after the first space for future uses?


Now you are starting to design a a custom config format. I would suggest
avoiding this if at all possible and to work around platform differences in
the core code or extension.

Just my $0.02.
Matt Mackall - March 31, 2015, 1:42 p.m.
On Mon, 2015-03-30 at 22:12 -0700, Gregory Szorc wrote:
> On Sat, Mar 28, 2015 at 6:12 PM, Matt Harbison <mharbison72@gmail.com>
> wrote:
> 
> > On Sat, 28 Mar 2015 15:19:50 -0400, Gregory Szorc <gregory.szorc@gmail.com>
> > wrote:
> >
> >  On Thu, Mar 26, 2015 at 7:35 AM, Yuya Nishihara <yuya@tcha.org> wrote:
> >>
> >>  On Wed, 25 Mar 2015 21:15:27 -0400, Matt Harbison wrote:
> >>> > On Mon, 23 Mar 2015 16:02:16 -0400, Matt Mackall <mpm@selenic.com>
> >>> wrote:
> >>> > > On Fri, 2015-03-20 at 18:36 -0400, Matt Harbison wrote:
> >>> > >> # HG changeset patch
> >>> > >> # User Matt Harbison <matt_harbison@yahoo.com>
> >>> > >> # Date 1426888879 14400
> >>> > >> #      Fri Mar 20 18:01:19 2015 -0400
> >>> > >> # Node ID 636f51ff2cfc9bbd665f1b3bf198d780a2afffe8
> >>> > >> # Parent  6b3aff8a06f1cf7057a9bdcdc5e301dde0801828
> >>> > >> config: introduce platform specific sections
> >>> > >
> >>> > > Please take a peek at Greg's patches where he's proposing using
> >>> > > [foo.bar] for something else. I can't say I'm excited by either
> >>> > > proposal.
> >>> > >
> >>> >
> >>> > Yep, I saw he hadn't ruled that out yet and pinged him on Monday.
> >>> >
> >>> > In the meantime, do you have any alternate ideas for this?  I wasn't
> >>> sure
> >>> > how your
> >>> >
> >>> >    new.style =
> >>> >    old.style.path =
> >>> >
> >>> > suggestion would apply to this, since that probably requires knowledge
> >>> of
> >>> > what is possible in each section.  ([extensions] for example allows a
> >>> > leading 'hgext.', so it isn't a general split on '.' and compare
> >>> > algorithm.)
> >>>
> >>> As the magic is done at config.parse() layer, how about %if like a C
> >>> preprocessor?
> >>>
> >>>   [systest]
> >>>   %if $platform == 'win32'  # or %ifeq(...,...) ?
> >>>   key = windowsvalue
> >>>   %elif $platform == 'linux'
> >>>   key = linuxvalue
> >>>   %endif
> >>>
> >>> (I don't know if mpm likes it. ;)
> >>>
> >>>
> >>>  I'm also not a huge fan of adopting [X.platform] for platform-specific
> >> bits. That prevents us from using [X.Y] sections for anything else, which
> >> I
> >> think is a bit short-sighted.
> >>
> >
> > Yeah, I was concerned about that too.  I only ignored it because I
> > couldn't think of something else that needed to apply at a section level.
> > And without a concrete second example, I tend to over-engineer the future
> > compatibility aspects.  But it's a fair point.
> >
> >  I kinda like the preprocessor idea. Although, the more I interact with
> >> preprocessors, the more I learn that it is often best to avoid them. Once
> >> you introduce a preprocessing step, any tool that parses the file needs to
> >> know about preprocessing. And since Mercurial doesn't have a built-in
> >> config editor, this would further limit the ability for 3rd party tools to
> >> rewrite Mercurial's mostly-ini config files. (The "mercurial-setup" tool I
> >> wrote for Mozilla is one such tool.)
> >>
> >
> > That was also a concern, even with the syntax I introduced.  Would an
> > editing tool be smart enough to skip over the regular [systest] section to
> > edit the platform specific section, and fall back if not present?  I
> > suppose overwriting both isn't horrible, just undesirable.
> >
> >  I'm a much bigger fan of sticking to defined data formats as much as
> >> possible. i.e. "use ini."
> >>
> >> Here are some alternatives.
> >>
> >> Introduce special options within sections to denote properties. e.g. all
> >> UPPERCASE options could be reserved for metadata:
> >>
> >> [systest]
> >> PLATFORM = win32
> >> key = windowsvalue
> >>
> >> [systest]
> >> PLATFORM = linux
> >> key = linuxvalue
> >>
> >> [systest]
> >> PLATFORM = linux, osx
> >> key = posixvalue
> >>
> >
> > I assume the metadata is controlling for the entire section, not just the
> > next line?  Given that config.py reads a line and updates the dictionary
> > immediately, I wonder if 3rd party editors can screw this up when they
> > don't recognize the metadata, and put other variables in the section
> > first.  (A 3rd party editor reordering keys seems like a bad idea, but it
> > hasn't really mattered to this point, other than maybe comments.)
> >
> 
> I would think this metadata would apply to the whole section and order
> wouldn't be matter. Mercurial's config parser would essentially do a
> post-processing step to apply special metadata options.
> 
> 
> >  Or, put special syntax in the section title after a space.
> >>
> >> [systest platform=win32]
> >> key = windowsvalue
> >>
> >> [systest platform=linux,osx]
> >> key = posixvalue
> >>
> >> Not as powerful as preprocessing, yes. But it preserves ini compatibility,
> >> which I don't think should be under-estimated.
> >>
> >
> > I like this.  It isn't far off from what I originally proposed (split on '
> > ' instead of '.', then split again on '='), so it should be fairly simple.
> > I think I'll try this, and use Yuya's idea as a backup.
> >
> > Thinking toward the future, should we disallow a second space, so that
> > multiple properties can be specified in the future?  e.g.
> >
> > [systest platform=win32 arch=amd64]
> >
> 
> I don't know!
> 
> 
> >
> > Is there any reason to care about quoting values on the right side of '=',
> > maybe to allow for spaces in the value?  Are there existing string parsing
> > methods that can be reused for consistency?
> >
> 
> I also don't know!
> 
> 
> >
> > The only other thing I can think of that might be useful here is to be
> > able to match a string based on regex or something, like the OS version.
> > (There are a couple of things that work differently in 10.10 vs 10.6 that
> > I've noticed with thg.)  Obviously '=' won't work for that, so are there
> > characters that we should reserve after the first space for future uses?
> 
> 
> Now you are starting to design a a custom config format. I would suggest
> avoiding this if at all possible and to work around platform differences in
> the core code or extension.

I'm not excited by any of the options here. The best option I've seen so
far is #if which is nice and open-ended (and comment-like), and I think
it's equivalent to all the other alternatives from an automated editing
standpoint[1].

That said, I don't think the marginal utility of the feature is high
enough to justify it. Matt's the first person to ever ask for it that
I'm aware of.

[1] Doomed to fail, just like editing already-very-complex existing
format.
Gregory Szorc - April 2, 2015, 12:10 a.m.
On Tue, Mar 31, 2015 at 6:42 AM, Matt Mackall <mpm@selenic.com> wrote:

> On Mon, 2015-03-30 at 22:12 -0700, Gregory Szorc wrote:
> > On Sat, Mar 28, 2015 at 6:12 PM, Matt Harbison <mharbison72@gmail.com>
> > wrote:
> >
> > > On Sat, 28 Mar 2015 15:19:50 -0400, Gregory Szorc <
> gregory.szorc@gmail.com>
> > > wrote:
> > >
> > >  On Thu, Mar 26, 2015 at 7:35 AM, Yuya Nishihara <yuya@tcha.org>
> wrote:
> > >>
> > >>  On Wed, 25 Mar 2015 21:15:27 -0400, Matt Harbison wrote:
> > >>> > On Mon, 23 Mar 2015 16:02:16 -0400, Matt Mackall <mpm@selenic.com>
> > >>> wrote:
> > >>> > > On Fri, 2015-03-20 at 18:36 -0400, Matt Harbison wrote:
> > >>> > >> # HG changeset patch
> > >>> > >> # User Matt Harbison <matt_harbison@yahoo.com>
> > >>> > >> # Date 1426888879 14400
> > >>> > >> #      Fri Mar 20 18:01:19 2015 -0400
> > >>> > >> # Node ID 636f51ff2cfc9bbd665f1b3bf198d780a2afffe8
> > >>> > >> # Parent  6b3aff8a06f1cf7057a9bdcdc5e301dde0801828
> > >>> > >> config: introduce platform specific sections
> > >>> > >
> > >>> > > Please take a peek at Greg's patches where he's proposing using
> > >>> > > [foo.bar] for something else. I can't say I'm excited by either
> > >>> > > proposal.
> > >>> > >
> > >>> >
> > >>> > Yep, I saw he hadn't ruled that out yet and pinged him on Monday.
> > >>> >
> > >>> > In the meantime, do you have any alternate ideas for this?  I
> wasn't
> > >>> sure
> > >>> > how your
> > >>> >
> > >>> >    new.style =
> > >>> >    old.style.path =
> > >>> >
> > >>> > suggestion would apply to this, since that probably requires
> knowledge
> > >>> of
> > >>> > what is possible in each section.  ([extensions] for example
> allows a
> > >>> > leading 'hgext.', so it isn't a general split on '.' and compare
> > >>> > algorithm.)
> > >>>
> > >>> As the magic is done at config.parse() layer, how about %if like a C
> > >>> preprocessor?
> > >>>
> > >>>   [systest]
> > >>>   %if $platform == 'win32'  # or %ifeq(...,...) ?
> > >>>   key = windowsvalue
> > >>>   %elif $platform == 'linux'
> > >>>   key = linuxvalue
> > >>>   %endif
> > >>>
> > >>> (I don't know if mpm likes it. ;)
> > >>>
> > >>>
> > >>>  I'm also not a huge fan of adopting [X.platform] for
> platform-specific
> > >> bits. That prevents us from using [X.Y] sections for anything else,
> which
> > >> I
> > >> think is a bit short-sighted.
> > >>
> > >
> > > Yeah, I was concerned about that too.  I only ignored it because I
> > > couldn't think of something else that needed to apply at a section
> level.
> > > And without a concrete second example, I tend to over-engineer the
> future
> > > compatibility aspects.  But it's a fair point.
> > >
> > >  I kinda like the preprocessor idea. Although, the more I interact with
> > >> preprocessors, the more I learn that it is often best to avoid them.
> Once
> > >> you introduce a preprocessing step, any tool that parses the file
> needs to
> > >> know about preprocessing. And since Mercurial doesn't have a built-in
> > >> config editor, this would further limit the ability for 3rd party
> tools to
> > >> rewrite Mercurial's mostly-ini config files. (The "mercurial-setup"
> tool I
> > >> wrote for Mozilla is one such tool.)
> > >>
> > >
> > > That was also a concern, even with the syntax I introduced.  Would an
> > > editing tool be smart enough to skip over the regular [systest]
> section to
> > > edit the platform specific section, and fall back if not present?  I
> > > suppose overwriting both isn't horrible, just undesirable.
> > >
> > >  I'm a much bigger fan of sticking to defined data formats as much as
> > >> possible. i.e. "use ini."
> > >>
> > >> Here are some alternatives.
> > >>
> > >> Introduce special options within sections to denote properties. e.g.
> all
> > >> UPPERCASE options could be reserved for metadata:
> > >>
> > >> [systest]
> > >> PLATFORM = win32
> > >> key = windowsvalue
> > >>
> > >> [systest]
> > >> PLATFORM = linux
> > >> key = linuxvalue
> > >>
> > >> [systest]
> > >> PLATFORM = linux, osx
> > >> key = posixvalue
> > >>
> > >
> > > I assume the metadata is controlling for the entire section, not just
> the
> > > next line?  Given that config.py reads a line and updates the
> dictionary
> > > immediately, I wonder if 3rd party editors can screw this up when they
> > > don't recognize the metadata, and put other variables in the section
> > > first.  (A 3rd party editor reordering keys seems like a bad idea, but
> it
> > > hasn't really mattered to this point, other than maybe comments.)
> > >
> >
> > I would think this metadata would apply to the whole section and order
> > wouldn't be matter. Mercurial's config parser would essentially do a
> > post-processing step to apply special metadata options.
> >
> >
> > >  Or, put special syntax in the section title after a space.
> > >>
> > >> [systest platform=win32]
> > >> key = windowsvalue
> > >>
> > >> [systest platform=linux,osx]
> > >> key = posixvalue
> > >>
> > >> Not as powerful as preprocessing, yes. But it preserves ini
> compatibility,
> > >> which I don't think should be under-estimated.
> > >>
> > >
> > > I like this.  It isn't far off from what I originally proposed (split
> on '
> > > ' instead of '.', then split again on '='), so it should be fairly
> simple.
> > > I think I'll try this, and use Yuya's idea as a backup.
> > >
> > > Thinking toward the future, should we disallow a second space, so that
> > > multiple properties can be specified in the future?  e.g.
> > >
> > > [systest platform=win32 arch=amd64]
> > >
> >
> > I don't know!
> >
> >
> > >
> > > Is there any reason to care about quoting values on the right side of
> '=',
> > > maybe to allow for spaces in the value?  Are there existing string
> parsing
> > > methods that can be reused for consistency?
> > >
> >
> > I also don't know!
> >
> >
> > >
> > > The only other thing I can think of that might be useful here is to be
> > > able to match a string based on regex or something, like the OS
> version.
> > > (There are a couple of things that work differently in 10.10 vs 10.6
> that
> > > I've noticed with thg.)  Obviously '=' won't work for that, so are
> there
> > > characters that we should reserve after the first space for future
> uses?
> >
> >
> > Now you are starting to design a a custom config format. I would suggest
> > avoiding this if at all possible and to work around platform differences
> in
> > the core code or extension.
>
> I'm not excited by any of the options here. The best option I've seen so
> far is #if which is nice and open-ended (and comment-like), and I think
> it's equivalent to all the other alternatives from an automated editing
> standpoint[1].
>
> That said, I don't think the marginal utility of the feature is high
> enough to justify it. Matt's the first person to ever ask for it that
> I'm aware of.
>
> [1] Doomed to fail, just like editing already-very-complex existing
> format.


FWIW, we're using configobj [1] for doing hgrc parsing and rewriting. It
preserves comments and ordering. The only time I've seen it choke is when
someone uses %include.

Maybe "progressive ui" will solve a lot of problems. But until there is a
better story for helping users configure an optimal config (this includes
server-specific suggestions), I'll continue to insist that the config
format is "portable" so those of us that don't have the power to rewrite
hgrc files on clients can make the hg experience suck less using config
editing tools.

[1] https://pypi.python.org/pypi/configobj/5.0.6
Matt Harbison - April 2, 2015, 2:23 a.m.
On Wed, 01 Apr 2015 20:10:53 -0400, Gregory Szorc  
<gregory.szorc@gmail.com> wrote:

> On Tue, Mar 31, 2015 at 6:42 AM, Matt Mackall <mpm@selenic.com> wrote:
>
>> On Mon, 2015-03-30 at 22:12 -0700, Gregory Szorc wrote:
>> > On Sat, Mar 28, 2015 at 6:12 PM, Matt Harbison <mharbison72@gmail.com>
>> > wrote:
>> >
>> > > On Sat, 28 Mar 2015 15:19:50 -0400, Gregory Szorc <
>> gregory.szorc@gmail.com>
>> > > wrote:
>> > >
>> > >  On Thu, Mar 26, 2015 at 7:35 AM, Yuya Nishihara <yuya@tcha.org>
>> wrote:
>> > >>
>> > >>  On Wed, 25 Mar 2015 21:15:27 -0400, Matt Harbison wrote:
>> > >>> > On Mon, 23 Mar 2015 16:02:16 -0400, Matt Mackall  
>> <mpm@selenic.com>
>> > >>> wrote:
>> > >>> > > On Fri, 2015-03-20 at 18:36 -0400, Matt Harbison wrote:
>> > >>> > >> # HG changeset patch
>> > >>> > >> # User Matt Harbison <matt_harbison@yahoo.com>
>> > >>> > >> # Date 1426888879 14400
>> > >>> > >> #      Fri Mar 20 18:01:19 2015 -0400
>> > >>> > >> # Node ID 636f51ff2cfc9bbd665f1b3bf198d780a2afffe8
>> > >>> > >> # Parent  6b3aff8a06f1cf7057a9bdcdc5e301dde0801828
>> > >>> > >> config: introduce platform specific sections
>> > >>> > >
>> > >>> > > Please take a peek at Greg's patches where he's proposing  
>> using
>> > >>> > > [foo.bar] for something else. I can't say I'm excited by  
>> either
>> > >>> > > proposal.
>> > >>> > >
>> > >>> >
>> > >>> > Yep, I saw he hadn't ruled that out yet and pinged him on  
>> Monday.
>> > >>> >
>> > >>> > In the meantime, do you have any alternate ideas for this?  I
>> wasn't
>> > >>> sure
>> > >>> > how your
>> > >>> >
>> > >>> >    new.style =
>> > >>> >    old.style.path =
>> > >>> >
>> > >>> > suggestion would apply to this, since that probably requires
>> knowledge
>> > >>> of
>> > >>> > what is possible in each section.  ([extensions] for example
>> allows a
>> > >>> > leading 'hgext.', so it isn't a general split on '.' and compare
>> > >>> > algorithm.)
>> > >>>
>> > >>> As the magic is done at config.parse() layer, how about %if like  
>> a C
>> > >>> preprocessor?
>> > >>>
>> > >>>   [systest]
>> > >>>   %if $platform == 'win32'  # or %ifeq(...,...) ?
>> > >>>   key = windowsvalue
>> > >>>   %elif $platform == 'linux'
>> > >>>   key = linuxvalue
>> > >>>   %endif
>> > >>>
>> > >>> (I don't know if mpm likes it. ;)
>> > >>>
>> > >>>
>> > >>>  I'm also not a huge fan of adopting [X.platform] for
>> platform-specific
>> > >> bits. That prevents us from using [X.Y] sections for anything else,
>> which
>> > >> I
>> > >> think is a bit short-sighted.
>> > >>
>> > >
>> > > Yeah, I was concerned about that too.  I only ignored it because I
>> > > couldn't think of something else that needed to apply at a section
>> level.
>> > > And without a concrete second example, I tend to over-engineer the
>> future
>> > > compatibility aspects.  But it's a fair point.
>> > >
>> > >  I kinda like the preprocessor idea. Although, the more I interact  
>> with
>> > >> preprocessors, the more I learn that it is often best to avoid  
>> them.
>> Once
>> > >> you introduce a preprocessing step, any tool that parses the file
>> needs to
>> > >> know about preprocessing. And since Mercurial doesn't have a  
>> built-in
>> > >> config editor, this would further limit the ability for 3rd party
>> tools to
>> > >> rewrite Mercurial's mostly-ini config files. (The "mercurial-setup"
>> tool I
>> > >> wrote for Mozilla is one such tool.)
>> > >>
>> > >
>> > > That was also a concern, even with the syntax I introduced.  Would  
>> an
>> > > editing tool be smart enough to skip over the regular [systest]
>> section to
>> > > edit the platform specific section, and fall back if not present?  I
>> > > suppose overwriting both isn't horrible, just undesirable.
>> > >
>> > >  I'm a much bigger fan of sticking to defined data formats as much  
>> as
>> > >> possible. i.e. "use ini."
>> > >>
>> > >> Here are some alternatives.
>> > >>
>> > >> Introduce special options within sections to denote properties.  
>> e.g.
>> all
>> > >> UPPERCASE options could be reserved for metadata:
>> > >>
>> > >> [systest]
>> > >> PLATFORM = win32
>> > >> key = windowsvalue
>> > >>
>> > >> [systest]
>> > >> PLATFORM = linux
>> > >> key = linuxvalue
>> > >>
>> > >> [systest]
>> > >> PLATFORM = linux, osx
>> > >> key = posixvalue
>> > >>
>> > >
>> > > I assume the metadata is controlling for the entire section, not  
>> just
>> the
>> > > next line?  Given that config.py reads a line and updates the
>> dictionary
>> > > immediately, I wonder if 3rd party editors can screw this up when  
>> they
>> > > don't recognize the metadata, and put other variables in the section
>> > > first.  (A 3rd party editor reordering keys seems like a bad idea,  
>> but
>> it
>> > > hasn't really mattered to this point, other than maybe comments.)
>> > >
>> >
>> > I would think this metadata would apply to the whole section and order
>> > wouldn't be matter. Mercurial's config parser would essentially do a
>> > post-processing step to apply special metadata options.
>> >
>> >
>> > >  Or, put special syntax in the section title after a space.
>> > >>
>> > >> [systest platform=win32]
>> > >> key = windowsvalue
>> > >>
>> > >> [systest platform=linux,osx]
>> > >> key = posixvalue
>> > >>
>> > >> Not as powerful as preprocessing, yes. But it preserves ini
>> compatibility,
>> > >> which I don't think should be under-estimated.
>> > >>
>> > >
>> > > I like this.  It isn't far off from what I originally proposed  
>> (split
>> on '
>> > > ' instead of '.', then split again on '='), so it should be fairly
>> simple.
>> > > I think I'll try this, and use Yuya's idea as a backup.
>> > >
>> > > Thinking toward the future, should we disallow a second space, so  
>> that
>> > > multiple properties can be specified in the future?  e.g.
>> > >
>> > > [systest platform=win32 arch=amd64]
>> > >
>> >
>> > I don't know!
>> >
>> >
>> > >
>> > > Is there any reason to care about quoting values on the right side  
>> of
>> '=',
>> > > maybe to allow for spaces in the value?  Are there existing string
>> parsing
>> > > methods that can be reused for consistency?
>> > >
>> >
>> > I also don't know!
>> >
>> >
>> > >
>> > > The only other thing I can think of that might be useful here is to  
>> be
>> > > able to match a string based on regex or something, like the OS
>> version.
>> > > (There are a couple of things that work differently in 10.10 vs 10.6
>> that
>> > > I've noticed with thg.)  Obviously '=' won't work for that, so are
>> there
>> > > characters that we should reserve after the first space for future
>> uses?
>> >
>> >
>> > Now you are starting to design a a custom config format. I would  
>> suggest
>> > avoiding this if at all possible and to work around platform  
>> differences
>> in
>> > the core code or extension.
>>
>> I'm not excited by any of the options here. The best option I've seen so
>> far is #if which is nice and open-ended (and comment-like), and I think
>> it's equivalent to all the other alternatives from an automated editing
>> standpoint[1].
>>
>> That said, I don't think the marginal utility of the feature is high
>> enough to justify it. Matt's the first person to ever ask for it that
>> I'm aware of.
>>
>> [1] Doomed to fail, just like editing already-very-complex existing
>> format.
>
>
> FWIW, we're using configobj [1] for doing hgrc parsing and rewriting. It
> preserves comments and ordering. The only time I've seen it choke is when
> someone uses %include.
>
> Maybe "progressive ui" will solve a lot of problems.

That might help some.  Certainly some of the settings in that series I've  
enabled by default.

Basically, I took the hgrc files on 4 of the machines I use most often,  
took out the junk that has accumulated, and split the common stuff into a  
baseline file that everyone in the department should be using.  (Some  
progressive ui things, but also corporate friendly things- eol and  
keyring, and some specific revset aliases).  The rest is user specific  
stuff, some portable (ui.username, etc), some not so much (merge tool,  
extdiff tool, alias for it).

The thing is, I've got 2 OS X systems, 3 Windows systems, 2 Linux systems  
and 2 FreeBSD systems.  It would be nice to only have to maintain the one  
personal hgrc, instead of 9 separate platform specific copies.  Sync with  
visual diff tool.  I don't see a better way to do it.

Maybe it's not a huge problem in practice, so I'll let it go for now.  I  
thought I found a couple other people asking for it before I started, but  
only found one when I went back to look yesterday[0].  If it becomes a  
hassle, I'll bring it up again with better justification.

--Matt

[0] http://stackoverflow.com/questions/6301864/cross-platform-hgrc-solution


> But until there is a
> better story for helping users configure an optimal config (this includes
> server-specific suggestions), I'll continue to insist that the config
> format is "portable" so those of us that don't have the power to rewrite
> hgrc files on clients can make the hg experience suck less using config
> editing tools.
>
> [1] https://pypi.python.org/pypi/configobj/5.0.6

Patch

diff --git a/mercurial/config.py b/mercurial/config.py
--- a/mercurial/config.py
+++ b/mercurial/config.py
@@ -7,7 +7,7 @@ 
 
 from i18n import _
 import error, util
-import os, errno
+import os, errno, platform
 
 class config(object):
     def __init__(self, data=None):
@@ -91,6 +91,8 @@  class config(object):
         item = None
         line = 0
         cont = False
+        skip = False
+        system = platform.system()
 
         for l in data.splitlines(True):
             line += 1
@@ -111,6 +113,8 @@  class config(object):
                 cont = False
             m = includere.match(l)
             if m:
+                if skip:
+                    continue
                 inc = util.expandpath(m.group(1))
                 base = os.path.dirname(src)
                 inc = os.path.normpath(os.path.join(base, inc))
@@ -128,11 +132,30 @@  class config(object):
             m = sectionre.match(l)
             if m:
                 section = m.group(1)
+
+                parts = section.split('.', 1)
+                if len(parts) == 2:
+                    if not system:
+                        raise error.ParseError(
+                                    _('the current system name is unavailable'),
+                                     "%s:%s" % (src, line))
+                    if parts[1] != system.lower():
+                        skip = True
+                        continue
+
+                    section = parts[0]
+
+                skip = False
+
                 if remap:
                     section = remap.get(section, section)
                 if section not in self:
                     self._data[section] = util.sortdict()
                 continue
+
+            if skip:
+                continue
+
             m = itemre.match(l)
             if m:
                 item = m.group(1)
diff --git a/tests/test-config.t b/tests/test-config.t
--- a/tests/test-config.t
+++ b/tests/test-config.t
@@ -90,3 +90,46 @@  Test exit code when no config matches
 
   $ hg config Section.idontexist
   [1]
+
+Test platform specific overrides
+
+  $ cat >> $HGRCPATH <<EOF
+  > [systest]
+  > key = value
+  > 
+  > [systest.windows]
+  > key = windowsvalue
+  > 
+  > [systest.linux]
+  > key = linuxvalue
+  > 
+  > [systest.darwin]
+  > key = macvalue
+  > 
+  > [systest]
+  > key2 = value2
+  > EOF
+
+#if windows
+  $ hg showconfig systest | sort
+  systest.key2=value2
+  systest.key=windowsvalue
+#endif
+
+#if linux
+  $ hg showconfig systest | sort
+  systest.key2=value2
+  systest.key=linuxvalue
+#endif
+
+#if osx
+  $ hg showconfig systest | sort
+  systest.key2=value2
+  systest.key=macvalue
+#endif
+
+A command line override of the vanilla section takes precedent
+
+  $ hg showconfig systest --config "systest.key=override" | sort
+  systest.key2=value2
+  systest.key=override