Making it easier to contribute non-functional changes

[Ian Lynagh]

On Mon, Oct 04, 2010 at 12:03:22PM -0400, Johan Tibell wrote:
> 
> I argue that's the reason
> we don't see many small, but important, changes to the core libraries.

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

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

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.

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.

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

> 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

that I discovered that it added DEPRECATED pragmas for two functions.

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

> someone (e.g. Ian)

You mentioned my name a few times; I'm not too keen on proposals that
mean more work for me!

> My suggestion is that a contributor without commit access simply
> creates a ticket and assigns it to someone with commit access (e.g.
> Ian).

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.

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

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


Thanks
Ian