New development model check-in.

[Keith Packard]

On Tue, 24 Nov 2009 14:04:21 +1000, Peter Hutterer <peter.hutterer at who-t.net> wrote:

> I thought it was you as CC but that's not always the case, in fact I noticed
> that some patches sent for general review get applied directly to master.
> So in preparing a patchset with several patches, some of them are already
> there by the time I'm ready to send a pull request.

Sorry, I haven't been consistent with this, and I really should be. I
was trying to be "nice" to people who had posted patches that seemed to
be reasonable. I'll try to check myself and let people tell me that they
think things are ready by explicitly Cc:'ing me.

> In two concrete cases:
> - my pull request was reduced to a single patch. That feels a bit meager to
>   send a pull request for.

Not really; if the patch has been reviewed and happens to be sitting in
your repository, a pull is the same as a patch for me (I review in gitk
instead of in the email; not a huge difference).

> - a patch was applied to master and then a day later reviews come in that
>   require changes to this patch. Pull requests tend to be a bit delayed, so
>   that can be caught by those.

Right, that's my fault for not waiting for people to tell me that things
are ready by having a Reviewed-by: line *and* an explicit Cc:.

> Add on on top of that your habit of cherry-picking patches from a pull request
> which makes it harder to have a fast-forward branch.

I think I only did this once, and you're right, it was a bad idea. Won't
happen again. Pulls are all-or-nothing, no cherry-picking allowed.

> - send patches to list for review, only merge into master if asked for

Perhaps instead of using just the Cc: trick, you could just stick a
comment in the patch mail letting me know that this patch has seen
sufficient review and is ready to be merged to master. That should avoid
accidents with addressing.

> - send pull requests

Make sure every patch in the pull request has a Reviewed-by: line.

> - patches in pull requests that aren't suitable should get reverted with an
>   explanation.

Pull requests are all-or-nothing; I don't want to break people's trees.

> I think this might suit Michel a bit better as well, since he could replace
> 'git push origin master' with 'git push fdo master' and the occasional pull
> request. He doesn't need to wait for you applying patches, he needs to keep
> track of public review himself which I think isn't largely different to the
> old "push to master" model.

That's what I'd like to see for any 'subsystem' maintainer -- push out a
patch series to their public tree and, when ready, send me a pull
request. If I keep pull requests simple merges, then their history will
be preserved intact and 'merging' back will be trivial.

> Of course, that doesn't change the delay in getting patches to master.

We've got two outstanding issues which aren't on master; the fb bug that
breaks compositing from some source images and the exa series.  For the
fb thing, I want to see someone try and explain to me why copying data
From one pixmap to another 'helps' in any way. For exa, I need
additional review; I don't have any way of testing that code at this
point.

So, changes I will make:

 1) Wait for people to explicitly ask for patches to be merged.
 2) Pulls are pure merges, no fancy tricks.

Keep your suggestions coming; the goal is to make things 'better' for
everyone, not to slow things down or keep people from getting useful
work done. Please feel free to publish your own trees with stuff you'd
like people to see; the more testing we get on each patch, the better
the final product will be.

-- 
keith.packard at intel.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: not available
Url : http://lists.x.org/archives/xorg-devel/attachments/20091124/df77e6bf/attachment.pgp