My 2cents here.
I mostly agree with Milan on these points. I recognize that it potentially
means more merge work; so its a good idea to discuss each project on
developers@ at least to coordinate with any one else who might be working on
the same subsystems. (My guess is that most of the time there are no other
such parties.)
With respect to code review troubles -- I actually think these more minor
changes are easier to code review and serve an important purpose in that
regard. That is, it is easier to get people who may be reasonably familiar
with C, but not with specific subsystems, to review changes. However, most of
these kinds of changes need no such particular expertise. So it gives people a
less intimidating way to start contributing, whether in the form of code
review, or in the form of small improvements.
Ultimately, growing the overall developer base *is* an important goal for the
project.
Of course, all of this needs to be tempered with reasonable amounts of
"sanity". Nobody wants to spend their days sifting through code review
requests for "meaningless" changes.
- Garrett
Garrett D'Amore
[email protected]
On May 17, 2012, at 5:38 AM, Milan Jurik wrote:
> Hi Gordon,
>
> first of all, I am moving this to discuss@
>
> On 15.05.2012 18:06, Gordon Ross wrote:
>> In a recent code review discussion, we found ourselves in a discussion
>> of the general dislike for changes that touch a lot of places in our
>> code.
>> I've been meaning to write something about this for a while, so here
>> is my attempt to explain.
>>
>> In an open source project, our most precious commodity is the time of
>> our volunteers. (A fact I was reminded of by this excellent talk
>> http://video.google.com/videoplay?docid=-4216011961522818645 by Ben
>> Collins-Sussman and Brian Fitzpatrick).
>>
>> Any change you make generally costs the project the time of some
>> reviewers (our precious commodity).
>> That time is proportional to the number of places you touch.
>>
>> Further, you should assume that any changes you make need to be merged
>> by someone working on a source base derived from the one you're
>> changing, and often several people people need to spend their time on
>> this. The more places you change, the more "cost" this forces onto
>> your "merge victims".
>>
>> What I take from this is the guidance: Make it worth their while.
>> If you're going to make a change in a way that touches a lot of
>> places, first try to find a way to do it that does not require
>> changing a lot of places. Then if you can't, only make the change if
>> doing so gives the reviewer and merge victims some non-trivial "pay
>> back" in exchange for their efforts.
>>
>> This is why I generally dislike sweeping "cleanup" changes. They tend
>> to touch a lot of places, and not give me much in return for my review
>> and/or merge efforts. On the other hand, if you're adding a major new
>> feature, I'll likely forgive some cleanup in and around what you
>> touch.
>
> I am well-aware that I am doing a lot of small and big changes with
> minimal impact on the functionality, e.g. with lint 12.3 changes I
> touched frequently parts of gate. This fulfill what you are writing about.
> And yes, it consumed a lot of time for reviewers. Time which could be
> spent by something else, like the next ZFS.
> I hope I tried to push for reviews gently. If not then I am sorry.
>
> So I take your e-mail as something which is against my work here
> and I would like to explain my position.
>
> I have reasons for such changes, even if they look unnecessary.
> Based on personal experience in several Solaris gates.
> If the rules are followed less strictly and/or code styles diverge
> and/or code is not updated over years then it is leading to
> generally worse code in these ways:
>
> 1) new people starting to look at the source code are scared from
> obvious problems of the source code and "mess" in it
>
> 2) analysis of the code is taking longer because you have to think
> as many very different developers
>
> 3) people are afraid to touch obvious problems because "they
> have to exist for some less-obvious reasons"
>
> And I believe we should use as much as possible from tools we can
> to make our code better in ways of functionality and also readability.
>
> Yes, lint 12.3 changes brought some strange things in (cases with
> unreachable empty commands) but for such cases I tried to do more,
> e.g. by removal of code duplicities and/or removal of the dead code.
> And this I consider as benefit to the code base.
>
> If I did not fix all of them then I could miss those few real.
> You could see from final changes that I missed some during my work
> on it.
>
> Throwing in tons of new code with super-uber-cool functionality look
> nice in short term. But if you need to look at such code 10 years later
> you are scared.
>
> I will continue with fixing corner cases and doing clean up of the
> code as long as I will find reviewers and advocates taking some
> value from it. If members of dev-council and/or advocates will say
> "Stop, we are not interested" then I have no other options than
> to respect it.
>
> I am not paid by any party to work on Illumos and I am not using
> it for anything commercial, only for home systems. So my contribs
> are usually very small, limited to time I have and want to spend
> and I can stop with them and survive :-)
>
> Yes, such changes can bring headache to backporters to private gates.
> As ex-RPE I know even these effects.
>
> Additionally, experience from other open source communities is that
> some big contributors are starting on these clean ups. It would be
> bad idea to stop them and expect that all new people will come
> just with great things.
>
> Best regards,
>
> Milan
>
> P.S.: I am sorry for late reply, my mail server selected to queue
> few e-mails for 2 days for me and they arrive finally now.
>
>
> -------------------------------------------
> illumos-discuss
> Archives: https://www.listbox.com/member/archive/182180/=now
> RSS Feed: https://www.listbox.com/member/archive/rss/182180/22003744-45f01c1f
> Modify Your Subscription: https://www.listbox.com/member/?&;
> Powered by Listbox: http://www.listbox.com
-------------------------------------------
illumos-discuss
Archives: https://www.listbox.com/member/archive/182180/=now
RSS Feed: https://www.listbox.com/member/archive/rss/182180/21175430-2e6923be
Modify Your Subscription:
https://www.listbox.com/member/?member_id=21175430&id_secret=21175430-6a77cda4
Powered by Listbox: http://www.listbox.com
