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 <qu...@fb.com> wrote: > > # HG changeset patch > > # User Jun Wu <qu...@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. _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel