Patchwork eol: support alternate location for .hgeol file (issue3975)

login
register
mail settings
Submitter Jorge Acereda
Date July 10, 2013, 3:24 p.m.
Message ID <3487e904f78164e70cf9.1373469857@jacereda-l2.brainstorm.redlan>
Download mbox | patch
Permalink /patch/1830/
State Superseded, archived
Headers show

Comments

Jorge Acereda - July 10, 2013, 3:24 p.m.
# HG changeset patch
# User Jorge Acereda <jacereda@brainstorm.es>
# Date 1373469140 -7200
# Branch stable
# Node ID 3487e904f78164e70cf91552419e1ecf235d8daf
# Parent  fbdac607bff3dfc0056268ca77e524a5ddd4f665
eol: support alternate location for .hgeol file (issue3975)

Adds a 'config' setting to the [eol] section that can be used to
specify an alternate location for the .hgeol file. This can help when
dealing with lots of subrepos.

When both the .hgeol and the setting are present, .hgeol takes
precedence.
Martin Geisler - July 11, 2013, 7:03 a.m.
Jorge Acereda <jacereda@brainstorm.es> writes:

> # HG changeset patch
> # User Jorge Acereda <jacereda@brainstorm.es>
> # Date 1373469140 -7200
> # Branch stable
> # Node ID 3487e904f78164e70cf91552419e1ecf235d8daf
> # Parent  fbdac607bff3dfc0056268ca77e524a5ddd4f665
> eol: support alternate location for .hgeol file (issue3975)
>
> Adds a 'config' setting to the [eol] section that can be used to
> specify an alternate location for the .hgeol file. This can help when
> dealing with lots of subrepos.

How does it help? Wont you now have to add

  [eol]
  config = ~/shared-cfg/hgeol

to .hg/hgrc in all your subrepos? If so, then adding .hgeol files to
each repo sound easier.
Jorge Acereda - July 11, 2013, 7:28 a.m.
On Jul 11, 2013, at 9:03 AM, Martin Geisler wrote:

> Jorge Acereda <jacereda@brainstorm.es> writes:
> 
>> # HG changeset patch
>> # User Jorge Acereda <jacereda@brainstorm.es>
>> # Date 1373469140 -7200
>> # Branch stable
>> # Node ID 3487e904f78164e70cf91552419e1ecf235d8daf
>> # Parent  fbdac607bff3dfc0056268ca77e524a5ddd4f665
>> eol: support alternate location for .hgeol file (issue3975)
>> 
>> Adds a 'config' setting to the [eol] section that can be used to
>> specify an alternate location for the .hgeol file. This can help when
>> dealing with lots of subrepos.
> 
> How does it help? Wont you now have to add
> 
>  [eol]
>  config = ~/shared-cfg/hgeol
> 
> to .hg/hgrc in all your subrepos? If so, then adding .hgeol files to
> each repo sound easier.


shared-cfg is a mercurial repo shared by all the team.


My $HOME/.hgrc looks like:

%include ~/shared-cfg/hgrc
...

And ~/shared-cfg/hgrc has:

[eol]
config = ~/shared-cfg/hgeol
...
Martin Geisler - July 11, 2013, 1:50 p.m.
Jorge Acereda <jacereda@brainstorm.es> writes:

> On Jul 11, 2013, at 9:03 AM, Martin Geisler wrote:
>
>> Jorge Acereda <jacereda@brainstorm.es> writes:
>> 
>>> # HG changeset patch
>>> # User Jorge Acereda <jacereda@brainstorm.es>
>>> # Date 1373469140 -7200
>>> # Branch stable
>>> # Node ID 3487e904f78164e70cf91552419e1ecf235d8daf
>>> # Parent  fbdac607bff3dfc0056268ca77e524a5ddd4f665
>>> eol: support alternate location for .hgeol file (issue3975)
>>> 
>>> Adds a 'config' setting to the [eol] section that can be used to
>>> specify an alternate location for the .hgeol file. This can help when
>>> dealing with lots of subrepos.
>> 
>> How does it help? Wont you now have to add
>> 
>>  [eol]
>>  config = ~/shared-cfg/hgeol
>> 
>> to .hg/hgrc in all your subrepos? If so, then adding .hgeol files to
>> each repo sound easier.
>
>
> shared-cfg is a mercurial repo shared by all the team.
>
>
> My $HOME/.hgrc looks like:
>
> %include ~/shared-cfg/hgrc
> ...
>
> And ~/shared-cfg/hgrc has:
>
> [eol]
> config = ~/shared-cfg/hgeol
> ...

Mkay... I can see how that can work in a company-controlled
installation, but it's still a bit unusual to put such config into the
~/.hgrc files -- it kind of means that you cannot use Mercurial for
anything *but* the company project... and all projects in the company
must share the same eol settings too.
Martin Geisler - July 11, 2013, 2:01 p.m.
Jorge Acereda <jacereda@brainstorm.es> writes:

> # HG changeset patch
> # User Jorge Acereda <jacereda@brainstorm.es>
> # Date 1373469140 -7200
> # Branch stable
> # Node ID 3487e904f78164e70cf91552419e1ecf235d8daf
> # Parent  fbdac607bff3dfc0056268ca77e524a5ddd4f665
> eol: support alternate location for .hgeol file (issue3975)

I'm still not completely convinced that this is a good idea, but here
are some review comments on your patch.

> Adds a 'config' setting to the [eol] section that can be used to
> specify an alternate location for the .hgeol file. This can help when
> dealing with lots of subrepos.
>
> When both the .hgeol and the setting are present, .hgeol takes
> precedence.

You would need to add a test for this. You should be able to find a
test-eol.t file where you can add the necessary test.

> diff -r fbdac607bff3 -r 3487e904f781 hgext/eol.py
> --- a/hgext/eol.py	Mon Jul 01 18:07:33 2013 -0500
> +++ b/hgext/eol.py	Wed Jul 10 17:12:20 2013 +0200
> @@ -6,8 +6,20 @@
>  Unix/Mac, thereby letting everybody use their OS native line endings.
>  
>  The extension reads its configuration from a versioned ``.hgeol``
> -configuration file found in the root of the working copy. The
> -``.hgeol`` file use the same syntax as all other Mercurial
> +configuration file found in the root of the working copy.
> +
> +An alternate location for the ``.hgeol`` configuration file can be
> +specified in a ``config`` setting in the ``[eol]`` section of your
> +``hgrc``. This can be useful when dealing with lots of subrepos. When
> +the ``.hgeol`` file is present in a repository, it will take
> +precedence over the ``config`` setting.

I think 'config' is too generic -- what about using 'eolpath' instead?

Falling back to the configured path feels backwards: when configure
something I expect it to override the normal behavior.

Maybe one could instead read both files (if they exist)? So by default
Mercurial reads no file, when the eol extension is enabled it reads
<repo>/.hgeol by default and with your setting you can extend the list
of files read. That somehow sounds better to my ears.

> +Example ``.hgrc`` file specifying the location of the ``.hgeol`` file::
> +
> +  [eol]
> +  config = ~/shared-cfg/hgeol
> +
> +The ``.hgeol`` file use the same syntax as all other Mercurial
>  configuration files. It uses two sections, ``[patterns]`` and
>  ``[repository]``.
>  
> @@ -207,7 +219,12 @@
>                  if node is None:
>                      # Cannot use workingctx.data() since it would load
>                      # and cache the filters before we configure them.
> -                    data = repo.wfile('.hgeol').read()
> +                    try:
> +                        f = repo.wfile('.hgeol')
> +                    except IOError:
> +                        f = open(util.expandpath(ui.config('eol', 'config')))

I guess this will read a relative path relative to the current working
directory? It should probably be changed to be relative to the
repository root.
Jorge Acereda - July 11, 2013, 3:50 p.m.
On Jul 11, 2013, at 4:01 PM, Martin Geisler wrote:

> Jorge Acereda <jacereda@brainstorm.es> writes:
> 
>> # HG changeset patch
>> # User Jorge Acereda <jacereda@brainstorm.es>
>> # Date 1373469140 -7200
>> # Branch stable
>> # Node ID 3487e904f78164e70cf91552419e1ecf235d8daf
>> # Parent  fbdac607bff3dfc0056268ca77e524a5ddd4f665
>> eol: support alternate location for .hgeol file (issue3975)
> 
> I'm still not completely convinced that this is a good idea, but here
> are some review comments on your patch.

Nor am I, if you have a good solution for this scenario I'd be glad to hear it.

Basically I have 80 subrepos and I don't want to maintain 80 .hgeol files.


> 
>> Adds a 'config' setting to the [eol] section that can be used to
>> specify an alternate location for the .hgeol file. This can help when
>> dealing with lots of subrepos.
>> 
>> When both the .hgeol and the setting are present, .hgeol takes
>> precedence.
> 
> You would need to add a test for this. You should be able to find a
> test-eol.t file where you can add the necessary test.

I'll do if there's a chance this will be accepted.

> 
>> diff -r fbdac607bff3 -r 3487e904f781 hgext/eol.py
>> --- a/hgext/eol.py	Mon Jul 01 18:07:33 2013 -0500
>> +++ b/hgext/eol.py	Wed Jul 10 17:12:20 2013 +0200
>> @@ -6,8 +6,20 @@
>> Unix/Mac, thereby letting everybody use their OS native line endings.
>> 
>> The extension reads its configuration from a versioned ``.hgeol``
>> -configuration file found in the root of the working copy. The
>> -``.hgeol`` file use the same syntax as all other Mercurial
>> +configuration file found in the root of the working copy.
>> +
>> +An alternate location for the ``.hgeol`` configuration file can be
>> +specified in a ``config`` setting in the ``[eol]`` section of your
>> +``hgrc``. This can be useful when dealing with lots of subrepos. When
>> +the ``.hgeol`` file is present in a repository, it will take
>> +precedence over the ``config`` setting.
> 
> I think 'config' is too generic -- what about using 'eolpath' instead?

eol.eolpath seems a bit redundant. 'configpath'?

> 
> Falling back to the configured path feels backwards: when configure
> something I expect it to override the normal behavior.

You might want to keep 78 subrepos sharing the same hgeol and 2 subrepos might have special needs that can be defined via their own .hgeol.


> Maybe one could instead read both files (if they exist)? So by default
> Mercurial reads no file, when the eol extension is enabled it reads
> <repo>/.hgeol by default and with your setting you can extend the list
> of files read. That somehow sounds better to my ears.

Would be cool to specify something like:

[eol]
configpaths = ~/sharedcfg/hgeol .hgeol

to specify that both files should be read, but I'm not familiar with the .ini syntax. How do you specify a list in a .ini?

> 
>> +Example ``.hgrc`` file specifying the location of the ``.hgeol`` file::
>> +
>> +  [eol]
>> +  config = ~/shared-cfg/hgeol
>> +
>> +The ``.hgeol`` file use the same syntax as all other Mercurial
>> configuration files. It uses two sections, ``[patterns]`` and
>> ``[repository]``.
>> 
>> @@ -207,7 +219,12 @@
>>                 if node is None:
>>                     # Cannot use workingctx.data() since it would load
>>                     # and cache the filters before we configure them.
>> -                    data = repo.wfile('.hgeol').read()
>> +                    try:
>> +                        f = repo.wfile('.hgeol')
>> +                    except IOError:
>> +                        f = open(util.expandpath(ui.config('eol', 'config')))
> 
> I guess this will read a relative path relative to the current working
> directory? It should probably be changed to be relative to the
> repository root.


I was thinking only absolute paths would be useful. Is there any utility to open a path relative to the root?


> 
> -- 
> Martin Geisler
Jorge Acereda - July 11, 2013, 3:59 p.m.
On Jul 11, 2013, at 3:50 PM, Martin Geisler wrote:

> Jorge Acereda <jacereda@brainstorm.es> writes:
>> 
>> shared-cfg is a mercurial repo shared by all the team.
>> 
>> 
>> My $HOME/.hgrc looks like:
>> 
>> %include ~/shared-cfg/hgrc
>> ...
>> 
>> And ~/shared-cfg/hgrc has:
>> 
>> [eol]
>> config = ~/shared-cfg/hgeol
>> ...
> 
> Mkay... I can see how that can work in a company-controlled
> installation, but it's still a bit unusual to put such config into the
> ~/.hgrc files -- it kind of means that you cannot use Mercurial for
> anything *but* the company project... and all projects in the company
> must share the same eol settings too.

Not necessarily all of them, that's why the per-repo .hgeol takes precedence.
Laurens Holst - July 13, 2013, 6:14 p.m.
Op 11-7-2013 16:01, Martin Geisler schreef:
> I guess this will read a relative path relative to the current working
> directory? It should probably be changed to be relative to the
> repository root.

Paths should be resolved relative to the file in which they are specified.

Be wary of the ui.ignore mess.

~Laurens
Martin Geisler - July 15, 2013, 8:05 a.m.
Jorge Acereda <jacereda@brainstorm.es> writes:

> On Jul 11, 2013, at 4:01 PM, Martin Geisler wrote:
>
>> Jorge Acereda <jacereda@brainstorm.es> writes:
>> 
>>> # HG changeset patch
>>> # User Jorge Acereda <jacereda@brainstorm.es>
>>> # Date 1373469140 -7200
>>> # Branch stable
>>> # Node ID 3487e904f78164e70cf91552419e1ecf235d8daf
>>> # Parent  fbdac607bff3dfc0056268ca77e524a5ddd4f665
>>> eol: support alternate location for .hgeol file (issue3975)
>> 
>> I'm still not completely convinced that this is a good idea, but here
>> are some review comments on your patch.
>
> Nor am I, if you have a good solution for this scenario I'd be glad to
> hear it.
>
> Basically I have 80 subrepos and I don't want to maintain 80 .hgeol
> files.

Here is a possible alternative: Writing a update-subrepo-hgeol.sh script
in the outer repository should be simple. It would do

  #!/bin/sh

  hg debugsub | grep '^path' | cut -c 6- | while read -r sub; do
      cp dot.hgeol "$sub/.hgeol"
  done

That is, it copies dot.hgeol from the outer repository into the
subrepositories. Maybe you've already rejected this for some reason?

>>> +An alternate location for the ``.hgeol`` configuration file can be
>>> +specified in a ``config`` setting in the ``[eol]`` section of your
>>> +``hgrc``. This can be useful when dealing with lots of subrepos.
>>> When +the ``.hgeol`` file is present in a repository, it will take
>>> +precedence over the ``config`` setting.
>> 
>> I think 'config' is too generic -- what about using 'eolpath' instead?
>
> eol.eolpath seems a bit redundant. 'configpath'?

Yeah, that sounds better to my ears.

>> Falling back to the configured path feels backwards: when configure
>> something I expect it to override the normal behavior.
>
> You might want to keep 78 subrepos sharing the same hgeol and 2
> subrepos might have special needs that can be defined via their own
> .hgeol.
>
>
>> Maybe one could instead read both files (if they exist)? So by
>> default Mercurial reads no file, when the eol extension is enabled it
>> reads <repo>/.hgeol by default and with your setting you can extend
>> the list of files read. That somehow sounds better to my ears.
>
> Would be cool to specify something like:
>
> [eol]
> configpaths = ~/sharedcfg/hgeol .hgeol
>
> to specify that both files should be read, but I'm not familiar with
> the .ini syntax. How do you specify a list in a .ini?

We have a ui.configlist function to read lists from .ini files and 'hg
help config' says that:

    List values are separated by whitespace or comma, except when values
    are placed in double quotation marks:

      allow_read = "John Doe, PhD", brian, betty

    Quotation marks can be escaped by prefixing them with a backslash.
    Only quotation marks at the beginning of a word is counted as a
    quotation (e.g., "foo"bar baz" is the list of "foo"bar" and "baz").

So the syntax you use above would work.

>>> +Example ``.hgrc`` file specifying the location of the ``.hgeol`` file::
>>> +
>>> +  [eol]
>>> +  config = ~/shared-cfg/hgeol
>>> +
>>> +The ``.hgeol`` file use the same syntax as all other Mercurial
>>> configuration files. It uses two sections, ``[patterns]`` and
>>> ``[repository]``.
>>> 
>>> @@ -207,7 +219,12 @@
>>>                 if node is None:
>>>                     # Cannot use workingctx.data() since it would load
>>>                     # and cache the filters before we configure them.
>>> -                    data = repo.wfile('.hgeol').read()
>>> +                    try:
>>> +                        f = repo.wfile('.hgeol')
>>> +                    except IOError:
>>> +                        f = open(util.expandpath(ui.config('eol', 'config')))
>> 
>> I guess this will read a relative path relative to the current working
>> directory? It should probably be changed to be relative to the
>> repository root.
>
>
> I was thinking only absolute paths would be useful. Is there any
> utility to open a path relative to the root?

Yes, I believe what would be the wfile function above. But I think
Laurens is right that the paths should be relative to the path of the
current config file. I don't remember what utility functions we have for
that.
Jorge Acereda - July 15, 2013, 5:35 p.m.
On Jul 13, 2013, at 8:14 PM, Laurens Holst wrote:

> Op 11-7-2013 16:01, Martin Geisler schreef:
> Paths should be resolved relative to the file in which they are specified.

Right, that's much better. Is there any helper routine for that?


> 
> Be wary of the ui.ignore mess.

Hmmm, was there any problem with that feature? Any reference?

Thanks,
  Jorge
Jorge Acereda - July 15, 2013, 6:02 p.m.
On Jul 15, 2013, at 10:05 AM, Martin Geisler wrote:

> Jorge Acereda <jacereda@brainstorm.es> writes:
> 
>> On Jul 11, 2013, at 4:01 PM, Martin Geisler wrote:
>> 
>>> Jorge Acereda <jacereda@brainstorm.es> writes:
>>> 
>>>> # HG changeset patch
>>>> # User Jorge Acereda <jacereda@brainstorm.es>
>>>> # Date 1373469140 -7200
>>>> # Branch stable
>>>> # Node ID 3487e904f78164e70cf91552419e1ecf235d8daf
>>>> # Parent  fbdac607bff3dfc0056268ca77e524a5ddd4f665
>>>> eol: support alternate location for .hgeol file (issue3975)
>>> 
>>> I'm still not completely convinced that this is a good idea, but here
>>> are some review comments on your patch.
>> 
>> Nor am I, if you have a good solution for this scenario I'd be glad to
>> hear it.
>> 
>> Basically I have 80 subrepos and I don't want to maintain 80 .hgeol
>> files.
> 
> Here is a possible alternative: Writing a update-subrepo-hgeol.sh script
> in the outer repository should be simple. It would do
> 
>  #!/bin/sh
> 
>  hg debugsub | grep '^path' | cut -c 6- | while read -r sub; do
>      cp dot.hgeol "$sub/.hgeol"
>  done
> 
> That is, it copies dot.hgeol from the outer repository into the
> subrepositories. Maybe you've already rejected this for some reason?


I considered something like that (based on 'onsub' extension, I didn't know about 'debugsub' and looking at the name perhaps I shouldn't depend too much on it...).

Well, I only told part of the story. Besides those 80 subrepos I have ~6 shell repos. 

The script would need to be applied in all of them, because all of those shell repos have one or several subrepos that are unique to them.

The script would introduce unnecessary artificial activity in the subrepos. Not a big deal, but...

The shared-config repository (the one that contains all the mercurial settings except ui.username and auth) is already in place and has proven quite useful. Given that we want to have all the same aliases/extensions in all of our dev machines, looks like that repo is a sane way to accomplish something like that. Also, we are a small company and ensuring everyone synchronizes it is as easy as yelling. So, this seems like the obvious place for hgeol configuration.

Laurens mentioned ui.ignore (which I happen to use and find it quite useful for the same reason as this proposal and encouraged me to try to write something like that for .hgeol). Your script could be used as well for maintaining .hgignore, but at some point looks like someone decided it was a good idea. Is there anything that makes .hgeol different from .hgignore?

Taking .hgeol/.hgignore out of the repositories seems like a good thing to me. I might be wrong and perhaps there're gotchas that I didn't consider... (Laurens refers to some 'ui.ignore mess'?)
Laurens Holst - July 16, 2013, 8:30 a.m.
Op 15-07-13 19:35, Jorge Acereda schreef:
> On Jul 13, 2013, at 8:14 PM, Laurens Holst wrote:
>
>> Op 11-7-2013 16:01, Martin Geisler schreef:
>> Paths should be resolved relative to the file in which they are specified.
> Right, that's much better. Is there any helper routine for that?

Probably not. I think that might’ve been the reason the ui.ignore bug 
was never fixed.

Config settings need to get an additional field associated with them 
which remembers the path where they were specified; the path of the 
configuration file, or the working directory when on the command line.

Any implementation for this will also help fix ui.ignore.

>> Be wary of the ui.ignore mess.
> Hmmm, was there any problem with that feature? Any reference?

Basically, relative paths are useless there because they are resolved 
relative to the working directory and thus entirely unreliably. Some links:

http://www.selenic.com/pipermail/mercurial-devel/2010-June/021462.html
http://stackoverflow.com/questions/7796542/make-hgignore-in-a-mercurial-repository-available-to-all-subrepos 
(discussed in the answer)

I’m sure there’s been more discussion but you’ll have to search the 
archives for it yourself :). Hint:

https://www.google.nl/search?q=site%3Aselenic.com%2Fpipermail%2Fmercurial-devel%2F%20ui.hgignore

~Laurens
Jorge Acereda - July 19, 2013, 12:22 p.m.
On Jul 16, 2013, at 10:30 AM, Laurens Holst wrote:

> Op 15-07-13 19:35, Jorge Acereda schreef:
>> On Jul 13, 2013, at 8:14 PM, Laurens Holst wrote:
>> 
>>> Op 11-7-2013 16:01, Martin Geisler schreef:
>>> Paths should be resolved relative to the file in which they are specified.
>> Right, that's much better. Is there any helper routine for that?
> 
> Probably not. I think that might’ve been the reason the ui.ignore bug was never fixed.

Looks like ui.configpath() is the answer here... Take a look at the V2 thread of this patch.
Perhaps dirstate._ignore() should also use ui.configpath()?

Patch

diff -r fbdac607bff3 -r 3487e904f781 hgext/eol.py
--- a/hgext/eol.py	Mon Jul 01 18:07:33 2013 -0500
+++ b/hgext/eol.py	Wed Jul 10 17:12:20 2013 +0200
@@ -6,8 +6,20 @@ 
 Unix/Mac, thereby letting everybody use their OS native line endings.
 
 The extension reads its configuration from a versioned ``.hgeol``
-configuration file found in the root of the working copy. The
-``.hgeol`` file use the same syntax as all other Mercurial
+configuration file found in the root of the working copy.
+
+An alternate location for the ``.hgeol`` configuration file can be
+specified in a ``config`` setting in the ``[eol]`` section of your
+``hgrc``. This can be useful when dealing with lots of subrepos. When
+the ``.hgeol`` file is present in a repository, it will take
+precedence over the ``config`` setting.
+
+Example ``.hgrc`` file specifying the location of the ``.hgeol`` file::
+
+  [eol]
+  config = ~/shared-cfg/hgeol
+
+The ``.hgeol`` file use the same syntax as all other Mercurial
 configuration files. It uses two sections, ``[patterns]`` and
 ``[repository]``.
 
@@ -207,7 +219,12 @@ 
                 if node is None:
                     # Cannot use workingctx.data() since it would load
                     # and cache the filters before we configure them.
-                    data = repo.wfile('.hgeol').read()
+                    try:
+                        f = repo.wfile('.hgeol')
+                    except IOError:
+                        f = open(util.expandpath(ui.config('eol', 'config')))
+                    data = f.read()
+                    f.close()
                 else:
                     data = repo[node]['.hgeol'].data()
                 return eolfile(ui, repo.root, data)