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

[Martin von Zweigbergk]

On Wed, Mar 22, 2017 at 10:23 AM, Jun Wu <quark at fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark at fb.com>
> # 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.