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

Jun Wu quark at fb.com
Thu Mar 23 14:27:37 EDT 2017


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 <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.


More information about the Mercurial-devel mailing list