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

[Martin von Zweigbergk]

On Thu, Mar 23, 2017 at 1:21 PM, Kevin Bullock
<kbullock+mercurial at ringworld.org> wrote:
>> On Mar 23, 2017, at 13:27, Jun Wu <quark at fb.com> 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.