Re: [PATCH 01 of 13 V3] scmutil: add a method to convert environment variables to config items

[Martin von Zweigbergk via Mercurial-devel]

On Thu, Mar 23, 2017 at 1:21 PM, Kevin Bullock
 wrote:
>> On Mar 23, 2017, at 13:27, Jun Wu  wrote:
>>
>> Thanks for the detailed review and especially for raising the pattern
>> issue! I think you have a very good point, and I actually prefer the less
>> commits way.
>>
>> I'd like to provide the reason for this particular series. The general
>> question will be left for others to comment.
>>
>> The reason was simple - I was involuntarily avoiding changing "ui.py" and
>> "scmutil.py" together. I should have just changed them together in a single
>> commit called "change rcpath return type and update its users", despite the
>> OneChangePerPatch wiki [1] says "and" indicates something wrong.
>
> I wouldn't consider "change a function's signature and update its callers" to 
> be separate changes

+1

> -- in fact, we often make such changes and summarize them simply as "change 
> function's signature". That implies strongly enough that the call sites are 
> updated too.
>
> If there are _lots_ of call sites, then sometimes it's helpful to add a new 
> function, and one by one transition call sites to use the new function. A 
> good recent example of this is 279cbde7bf3d::fb1b5cd17664.
>
> But in the case of your series, I'd suggest an order very similar to Martin's:
>
> [01] scmutil: extract rc.d listing function from rccomponents
> [02] scmutil: split osrcpath to return default.d paths (API)
> [03] scmutil: rename rcpath to rccomponents (API)
> [04] scmutil: implement rccomponents to return a list of [('path', 
> '/path/to/hgrc'), ...], but NOT consult environment yet -- this also implies 
> changing the call sites (API)

Agreed. I actually meant for that to be the case (i.e. not introducing
'items' types yet), but I should have clarified it as you did. Thanks.

> [05] run-tests: drop environment variables affecting configs
> [06] scmutil: add a method to convert environment variables to config items
> [07] scmutil: define a list of configs overriding system rc, but not users

I would fold these two into the next. I think we just disagree on
that, but I wanted to mention it in case it was just not clear (I
didn't say it explicitly, I think).

> [08] config: let env variables override system hgrc (BC) -- here update 
> commands.py and ui.py to deal with 'items' entries (i.e., fold most of your 
> V3 patches 9 and 10 into this)
> [09] ui: simplify geteditor
> [10] pager: do not read from environment variable
>
>> The fact that most codemods are done one-commit-per-file, and per-file
>> commit message exists for a good reason [2] made me wonder if a changeset
>> should just change one file. But that shouldn't be a strict rule anyway.
>
> One logical change can definitely span more than one file. We generally go 
> file-by-file only when there's a bunch of call sites. And in those cases, it 
> seems to always be the right approach to introduce the new API with a new 
> name, convert all the callers incrementally, and then deprecate/remove the 
> old API.
>
> This series is a more localized refactor, so I don't think that approach is 
> called for here.
>
> pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
> Kevin R. Bullock
>

Thanks to both of you for your replies. I'm glad we three seem to
mostly agree about this.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH 01 of 13 V3] scmutil: add a method to convert environment variables to config items

[Kevin Bullock]

> On Mar 23, 2017, at 13:27, Jun Wu  wrote:
> 
> Thanks for the detailed review and especially for raising the pattern
> issue! I think you have a very good point, and I actually prefer the less
> commits way.
> 
> I'd like to provide the reason for this particular series. The general
> question will be left for others to comment.
> 
> The reason was simple - I was involuntarily avoiding changing "ui.py" and
> "scmutil.py" together. I should have just changed them together in a single
> commit called "change rcpath return type and update its users", despite the
> OneChangePerPatch wiki [1] says "and" indicates something wrong.

I wouldn't consider "change a function's signature and update its callers" to 
be separate changes -- in fact, we often make such changes and summarize them 
simply as "change function's signature". That implies strongly enough that the 
call sites are updated too.

If there are _lots_ of call sites, then sometimes it's helpful to add a new 
function, and one by one transition call sites to use the new function. A good 
recent example of this is 279cbde7bf3d::fb1b5cd17664.

But in the case of your series, I'd suggest an order very similar to Martin's:

[01] scmutil: extract rc.d listing function from rccomponents
[02] scmutil: split osrcpath to return default.d paths (API)
[03] scmutil: rename rcpath to rccomponents (API)
[04] scmutil: implement rccomponents to return a list of [('path', 
'/path/to/hgrc'), ...], but NOT consult environment yet -- this also implies 
changing the call sites (API)
[05] run-tests: drop environment variables affecting configs
[06] scmutil: add a method to convert environment variables to config items
[07] scmutil: define a list of configs overriding system rc, but not users
[08] config: let env variables override system hgrc (BC) -- here update 
commands.py and ui.py to deal with 'items' entries (i.e., fold most of your V3 
patches 9 and 10 into this)
[09] ui: simplify geteditor
[10] pager: do not read from environment variable

> The fact that most codemods are done one-commit-per-file, and per-file
> commit message exists for a good reason [2] made me wonder if a changeset
> should just change one file. But that shouldn't be a strict rule anyway.

One logical change can definitely span more than one file. We generally go 
file-by-file only when there's a bunch of call sites. And in those cases, it 
seems to always be the right approach to introduce the new API with a new name, 
convert all the callers incrementally, and then deprecate/remove the old API.

This series is a more localized refactor, so I don't think that approach is 
called for here.

pacem in terris / мир / शान्ति / ‎‫سَلاَم‬ / 平和
Kevin R. Bullock

___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Re: [PATCH 01 of 13 V3] scmutil: add a method to convert environment variables to config items

[Jun Wu]

Thanks for the detailed review and especially for raising the pattern
issue! I think you have a very good point, and I actually prefer the less
commits way.

I'd like to provide the reason for this particular series. The general
question will be left for others to comment.

The reason was simple - I was involuntarily avoiding changing "ui.py" and
"scmutil.py" together. I should have just changed them together in a single
commit called "change rcpath return type and update its users", despite the
OneChangePerPatch wiki [1] says "and" indicates something wrong.

The fact that most codemods are done one-commit-per-file, and per-file
commit message exists for a good reason [2] made me wonder if a changeset
should just change one file. But that shouldn't be a strict rule anyway.

At the point, I wonder if we can change the rule a little bit so we could
prefer to do codemod in a single commit. The usual benefit of small patch -
"reviewability, selectability, and bisectability" seem less effective for
repetitive changes. It also seems helpful to reduce overhead of reviewing
and tooling, and the code history seems cleaner, with a better density of
information.

[1]: https://www.mercurial-scm.org/wiki/OneChangePerPatch
[2]: https://news.ycombinator.com/item?id=11670080

Excerpts from Martin von Zweigbergk's message of 2017-03-22 22:28:37 -0700:
> On Wed, Mar 22, 2017 at 10:23 AM, Jun Wu  wrote:
> > # HG changeset patch
> > # User Jun Wu 
> > # Date 1489449998 25200
> > #  Mon Mar 13 17:06:38 2017 -0700
> > # Node ID 04259bd73d263306f16e25bd4e6bc53faf80911c
> > # Parent  55c6788c54e2faf80ec14f2b0844bfe429012bc3
> > # Available At https://bitbucket.org/quark-zju/hg-draft 
> > #  hg pull https://bitbucket.org/quark-zju/hg-draft  -r 
> > 04259bd73d26
> > scmutil: add a method to convert environment variables to config items
> 
> First of all, thanks for working on this!
> 
> I just have a comment on the structure of of this series for now. This
> is the current structure:
> 
> [01] scmutil: add a method to convert environment variables to config items
> [02] scmutil: define a list of configs overriding system rc, but not users
> [03] scmutil: split osrcpath to return default.d paths (API)
> [04] scmutil: copy rcpath to rcpath2
> [05] scmutil: use _rccomponents in rcpath2
> [06] scmutil: implement rccomponents to return multiple config sources
> [07] scmutil: extract rc.d listing function from rccomponents
> [08] run-tests: drop environment variables affecting configs
> [09] ui: use scmutil.rccomponents to load configs (BC)
> [10] config: list environment variables in debug output
> [11] scmutil: remove rcpath (API)
> [12] ui: simplify geteditor
> [13] pager: do not read from environment variable
> 
> Patches 1,2,4,5,6,7 all introduce and/or update dead code (AFAICT).
> The dead code becomes live only in patch 9. This is a common pattern
> on this list, so I may very well be a minority in disliking it, but I
> do really dislike it. When reviewing the patches, I end up looking at
> the description, then I ignore the content and go to the next patch,
> because I don't yet know how the added code will be used. And since
> there are no tests exercising it, it doesn't really matter if it's
> working or not anyway, so it's safe to ignore it. Then, when I get to
> patch that hooks things up (patch 9 in this case), I have to try to
> recall the commit messages of all the previous patches to understand
> what all the things that changed were. Again, the structure you follow
> is common on this list, so maybe most people don't have the problem I
> have with it (and also, I don't mean to pick on you; this series was
> just an example).
> 
> I would really have preferred something like this (and since I haven't
> followed all the changes in the patches, it may very well not work as
> I think it would):
> 
> [01] scmutil: extract rc.d listing function from rccomponents
> [02] scmutil: split osrcpath to return default.d paths (API)
> [03] scmutil: rename rcpath to rccomponents (API)
> [04] scmutil: implement rccomponents to return multiple config sources
> [05] run-tests: drop environment variables affecting configs
> [06] config: let env variables override system hgrc (BC)
> [07] config: list environment variables in debug output
> [08] ui: simplify geteditor
> [09] pager: do not read from environment variable
> 
> Patch 6 above would be a fold of your patches 1,2, and 9, so it would
> be a larger patch to review. However, as I tried to explain above,
> that's effectively what I review at that point anyway, and having it
> all in one patch makes it much easier for me as a reviewer.
> 
> I'm sure there there are things I missed that make the above not quite
> possible, but hopefully it's close enough to possible that you at
> least get the idea. And then you get to decide what to do with the
> idea, of course :-) Perhaps you decide that it's a bad idea. Or
> perhaps you decide it has some 

Re: [PATCH 01 of 13 V3] scmutil: add a method to convert environment variables to config items

[Martin von Zweigbergk via Mercurial-devel]

On Wed, Mar 22, 2017 at 10:23 AM, Jun Wu  wrote:
> # HG changeset patch
> # User Jun Wu 
> # Date 1489449998 25200
> #  Mon Mar 13 17:06:38 2017 -0700
> # Node ID 04259bd73d263306f16e25bd4e6bc53faf80911c
> # Parent  55c6788c54e2faf80ec14f2b0844bfe429012bc3
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #  hg pull https://bitbucket.org/quark-zju/hg-draft -r 
> 04259bd73d26
> scmutil: add a method to convert environment variables to config items

First of all, thanks for working on this!

I just have a comment on the structure of of this series for now. This
is the current structure:

[01] scmutil: add a method to convert environment variables to config items
[02] scmutil: define a list of configs overriding system rc, but not users
[03] scmutil: split osrcpath to return default.d paths (API)
[04] scmutil: copy rcpath to rcpath2
[05] scmutil: use _rccomponents in rcpath2
[06] scmutil: implement rccomponents to return multiple config sources
[07] scmutil: extract rc.d listing function from rccomponents
[08] run-tests: drop environment variables affecting configs
[09] ui: use scmutil.rccomponents to load configs (BC)
[10] config: list environment variables in debug output
[11] scmutil: remove rcpath (API)
[12] ui: simplify geteditor
[13] pager: do not read from environment variable

Patches 1,2,4,5,6,7 all introduce and/or update dead code (AFAICT).
The dead code becomes live only in patch 9. This is a common pattern
on this list, so I may very well be a minority in disliking it, but I
do really dislike it. When reviewing the patches, I end up looking at
the description, then I ignore the content and go to the next patch,
because I don't yet know how the added code will be used. And since
there are no tests exercising it, it doesn't really matter if it's
working or not anyway, so it's safe to ignore it. Then, when I get to
patch that hooks things up (patch 9 in this case), I have to try to
recall the commit messages of all the previous patches to understand
what all the things that changed were. Again, the structure you follow
is common on this list, so maybe most people don't have the problem I
have with it (and also, I don't mean to pick on you; this series was
just an example).

I would really have preferred something like this (and since I haven't
followed all the changes in the patches, it may very well not work as
I think it would):

[01] scmutil: extract rc.d listing function from rccomponents
[02] scmutil: split osrcpath to return default.d paths (API)
[03] scmutil: rename rcpath to rccomponents (API)
[04] scmutil: implement rccomponents to return multiple config sources
[05] run-tests: drop environment variables affecting configs
[06] config: let env variables override system hgrc (BC)
[07] config: list environment variables in debug output
[08] ui: simplify geteditor
[09] pager: do not read from environment variable

Patch 6 above would be a fold of your patches 1,2, and 9, so it would
be a larger patch to review. However, as I tried to explain above,
that's effectively what I review at that point anyway, and having it
all in one patch makes it much easier for me as a reviewer.

I'm sure there there are things I missed that make the above not quite
possible, but hopefully it's close enough to possible that you at
least get the idea. And then you get to decide what to do with the
idea, of course :-) Perhaps you decide that it's a bad idea. Or
perhaps you decide it has some value, and you consider it for next
time.

Again, this is directed not only at Jun. I'm happy to hear what others
think as well.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

[PATCH 01 of 13 V3] scmutil: add a method to convert environment variables to config items

[Jun Wu]

# HG changeset patch
# User Jun Wu 
# Date 1489449998 25200
#  Mon Mar 13 17:06:38 2017 -0700
# Node ID 04259bd73d263306f16e25bd4e6bc53faf80911c
# Parent  55c6788c54e2faf80ec14f2b0844bfe429012bc3
# Available At https://bitbucket.org/quark-zju/hg-draft
#  hg pull https://bitbucket.org/quark-zju/hg-draft -r 04259bd73d26
scmutil: add a method to convert environment variables to config items

We will put this "config generated by environment variables" thing in a
desired layer - ex. above system configs, below user configs.

The method was designed to be reusable if we want more complex layers - like
multiple environment-generated configs; or test environment configs using a
different environ dict.

The method seems to be more appropriate fitting here than "config.py"
because "config.py" only has data structure and is unware of special actual
config or environments. I think it's cleaner to keep config.py pure and free
from environment or config names.

The same applies to ui.fixconfig, trusted or untrusted handling, and likely
"ui.compat" in the future. We may want to a new file like "uiconfig.py" if
we want to make "ui.config" a thing and move logic aware of specific config
items there.

diff --git a/mercurial/scmutil.py b/mercurial/scmutil.py
--- a/mercurial/scmutil.py
+++ b/mercurial/scmutil.py
@@ -405,4 +405,18 @@ def osrcpath():
 return path
 
+def envconfig(envlist, env=None):
+'''[(section, name, value, source)] extracted from environment variables
+
+envlist is a list of (envname, section, configname)
+'''
+if env is None:
+env = encoding.environ
+result = []
+for envname, section, configname in envlist:
+if envname not in env:
+continue
+result.append((section, configname, env[envname], '$%s' % envname))
+return result
+
 _rcpath = None
 
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel