Making it easier to contribute non-functional changes

[Johan Tibell]

On Mon, Oct 4, 2010 at 4:49 PM, Ian Lynagh <igloo at earth.li> wrote:
> FWIW, I graphed the number of patches per year in unix and base here:
>    http://lambda.haskell.org/~igloo/lib_patches.png
> (Library_submissions started in Oct 2006) but I'm not sure it really
> tells us much, as it doesn't say how many patches were "maintenance"
> rather than "development".

Thanks for putting together the graph. Do you have a feeling for how
many patches are created by people without commit access? Quite a few
patches to these libraries are made by e.g. Simon M.

>> Non-functional changes should be allowed without review by the
>> libraries list.
>
> As Ross said, simple, obvious improvements are applied without going
> through the process. A common example is patches that just add haddock
> docs to previously doc-less functions.

Does this apply to "obvious" improvements by the general Haskell
community or only by a set of core contributes (e.g. GHC developers).
If one believes one has such an improvement, is there a description of
how to get it committed?

> Note that some patches are obvious when being created, but are hard to
> sanity check afterwards. For example, when making a patch that just
> moves functions around, it's easy to be confident it's safe when
> creating the patch, but when someone else sees it it just looks like a
> load of code being added and removed. It's even worse if other changes
> are made in the same patch.

>From my experience these things are not being looked at by people on
libraries@ so I don't think having a public review of non-functional
patches decreases the likelihood of breakages. A contributor could of
course ask for a review if he/she thinks it's necessary. The commiter
could also have a look to make sure things don't break.

> In general, it makes life easier for the committers if the libraries
> process is used, so it is not down to us to review the patches.

I understand that, but it also seems to prevent us from cleaning up
our base libraries.

>> or attached to a "please merge" ticket on the bug tracker.
>
> That's not what 'merge' is for. I improved the description a couple of
> weeks ago, so it's hopefully clearer now:
>    fixed in HEAD; please merge to STABLE   Next status will be 'merge'
>
> The right state is 'patch':
>    please review   Next status will be 'patch'

Got it. Thanks for the clarification.

>> Only changes that don't affect the API or performance (in non-trivial
>> ways) should be allowed in without a review.
>
> But you need some amount of review in order to know what a patch does.
> For example, it was only when reading a recent patch
>
>    Sun Aug 29 13:02:45 BST 2010
>      * Performance improvements to Data.Map

But that patch was sent to the libraries list for review, as it
should. I was very much aware what changes the patch made!

>> It's possible that once a commit that should have been reviewed gets
>> commited. I suggest we adopt an "ask for forgiveness, not for
>> permission" policy in these cases. We can always roll back changes.
>
> IIRC, part of the motivation for the library process was that changes
> were being made in the HEAD, getting into release, and then people
> discovered that they caused problems.

Perhaps we could have a commit mailing list like Sterling suggested?

>> someone (e.g. Ian)
>
> You mentioned my name a few times; I'm not too keen on proposals that
> mean more work for me!

I didn't intend it to! Read that as <whoever actually has commit
access and commits stuff>.

> If you do have a patch that doesn't need to go through the libraries
> process, the best thing to do is to file a ticket and set its status to
> 'patch'. Assigning it to me is unlikely to get it applied faster, but
> means no-one else is likely to look at it.

Sure. That sounds reasonable. As part of this proposal it would be
nice if we could clarify who does have commit access to our core
libraries. I have no idea and I doubt most people have.

>> Aside: Asking who has commit access in an email to the libraries list
>> will quickly get you a link to the libraries process (I tried!).
>
> Well, if you want to get a patch committed, then that's normally what
> you need, so that seems like a reasonable response to me.

But it will also make sure that people don't bother with smaller patches.

>> I assume that contributor test their patches, just as they would test
>> them before submitting them to a package not maintained by the
>> libraries list.
>
> FWIW, it's quite common for submitted patches to break the build (e.g.
> platform-specific breakage, or new code has warnings, or even just silly
> mistakes that couldn't possibly compile (e.g. there was one in the past
> week which added an unmatched "endif" to a makefile)).

libraries@ review doesn't change that, as people there don't normally
check for these kind of problems (i.e. they don't actually compile and
run the code).

We use a process for submitting code that's applicable to well
polished libraries, but the libraries the process governs are anything
but. For example, there are several of use who would like to make
containers into a nice polished library, but I doubt we will actually
do so if each patch needs to go through the libraries process.

Thanks,
Johan