Re: Patchwork (was Re: Governance revisited)
From: Alexandre Julliard <julliard(a)winehq.org>
If you expect anything to happen, you'll need to make much more concrete suggestions, and provide examples to make us understand how what you are suggesting would work in practice
Fair enough. Some disclaimers upfront: I'm basically thinking out loud, I have no idea what your typical workflow looks like. I'm also not restrained by any knowledge of emacs and what can be done in its plugins, macro's, user-defined commands or whatever they're called. But I know I could code the proposal below in e.g Perl or C, so I guess it should be possible in Lisp also. Who's going to code that is a different matter altogether, let's first see if we can think of a solution which would work. Let's assume a patch is submitted to wine-patches. A bot would receive it and store it in a database (Patchwork already seems to do a fine job on this). The bot could do some simple checks, like does it apply cleanly, no C++ comments, whatever automatic checks we can come up with. If one of the checks fails, the status would be set to RFC (Request For Change, I'm using the Patchwork statuses here) and the author would receive an automatic email explaining what's wrong, otherwise the status would be set to New. Note that this should save you some time, weeding out "obviously" wrong patches. On to your workflow. You'd issue a Meta-Ctrl-Shift-whatever emacs command. Behind the scenes, this would send a request to the webserver which would reply with a list of queued patches. This list could be sorted by a score. Authors who consistently submit good patches would get a higher score than, well, me. This score could be computed as the number of accepted patches minus the number of rejected patches. A patch already reviewed (and recommended) by a subsystem maintainer could get a real big score boost so it ends up near the top of the list. Patches which have been in the queue for a few days could get extra points. Patches with RFC status wouldn't be shown at all or get a very low score. Anyway, you end up with a list of patches on your screen with the least problematic near the top. This should help ensure that these get through pretty quickly. So you select a patch which is then retrieved from the server. It could be automatically applied, a build started, whatever suits you best. Comments made on it by other developers could be shown to you. After testing is complete, you issue a "decision" command. You can decide to accept the patch, that would send an "accept" message to the webserver and perhaps it could be comitted to your git repo. If the patch needs more work, you'll be prompted for a message. That message will be mailed to the author with a copy to win-devel and it will be sent to the webserver which will change the patch status to RFC. If you think the patch is beyond hope it can be rejected outright (again with a message to author and wine-devel). Or you can decide not to decide on it today and sleep on it for a night. Then the patch will just show up in the list of pending patches again tomorrow, with a little bit higher priority (since it has been in the queue longer). Back to the list of pending patches, repeat.
and how it would be different from what we are doing now.
It would make sure the author always receives some kind of feedback (either from the bot, other developers or yourself) and would make sure patches don't get lost (patches are automatically entered into the system and only leave the system when the author withdraws them or when you make a final decision). Ge van Geldorp.
Ge van Geldorp wrote:
It would make sure the author always receives some kind of feedback (either from the bot, other developers or yourself) and would make sure patches don't get lost (patches are automatically entered into the system and only leave the system when the author withdraws them or when you make a final decision).
Seems like that is a system that doesn't scale well at all, as it requires Alexandre to specifically respond to each and every patch. It also seems like it encourages patch submitters to not polish their patches themselves and just submit a higher volume of low quality patches for Alexandre to review, since the onus will then be on him to respond. The current system, which leaves the responsibility for the patch with the submitter both scales better, and encourages patch submitters to think about their patches more. Responding to each and every patch seems like it would be a waste of Alexandre's time. We should encourage more people to participate in the patch review process, so that we have more reviewers and a more scalable process. btw. Is there any reason that you can't request a review of your patches, or report the problem that you're trying to fix in bugzilla, as I suggested elsewhere? Mike
On Thursday 28 September 2006 05:49, Mike McCormack wrote:
Seems like that is a system that doesn't scale well at all, as it requires Alexandre to specifically respond to each and every patch.
He still has to take an action to review each patch now, and presumably some action to remove it from his queue (speculation since we don't have his process documented - which is why I asked if we could get a vncrec file showing a patch reveiew/application/rejection session, so I could document it). If the process fits into this workflow without disruption the cost should actually be less since it saves having a conversation about why the patch was rejected.
It also seems like it encourages patch submitters to not polish their patches themselves and just submit a higher volume of low quality patches for Alexandre to review, since the onus will then be on him to respond.
You seem to be assuming people are submitting patches they *know* will not be accepted (discounting ones submitted for the purposes of record only where the submitter says they don't expect it to be committed). This would be pathological behaviour since it would require more work on the part of the submitter as well as on the part of Alexandre. In fact the present process is likely to involve more work since it requires people to speculate about why the patch was rejected or passed over, and if they get it wrong, resubmit. Often there wasn't even anything wrong in the first place (the "oops" bitbucket) so all the speculation and rework will be a pointless exercise. If the patch is reworked, it's submitted again, has to be reviewed again, wait, rinse, repeat. That will result in more patches than if people are told the actual (rather than speculative) reason it was not applied.
The current system, which leaves the responsibility for the patch with the submitter both scales better, and encourages patch submitters to think about their patches more.
(the following sounds somewhat harsher than it's intended to but I couldn't figure out a better way to say it) If you can call speculation thinking, but I don't know what you mean by scales better. Speculative review increases the chance that Alexandre has to spend time reviewing more wrong patches because other people guessed wrong. The current system has literally cost me tens of thousands of dollars in wasted developer time on just a handful of patches (not including time I have wasted on it personally), so if by "scales better" you mean "passes off a relatively small cost off (sometimes without actually removing that cost) to others magnified by huge factors" then yes I guess it does "scale better", but scaling up the expense doesn't seem to be a good idea to me. Maybe some other employers don't mind throwing money away like this (Jeremy?), but I do.
Responding to each and every patch seems like it would be a waste of Alexandre's time. We should encourage more people to participate in the patch review process, so that we have more reviewers and a more scalable process.
It's more of an heuristic than a determinitive process unless the reviewers know for certain Alexandre's reasons for rejecting a patch (assuming an unapplied patch has in fact been rejected), which requires either telepathy or that he tell them. The current process results in regular "oops" situations leading to no feedback. There are the "oops, must have missed that patch", and "oops, I thought I did reply to tell you what was wrong with that patch", both of which I have seen multiple times. These at least could be improved with a suitable system in place and result in some improvement even if the speculative post-rejection-review process is kept. I'm not sure why you seem to be opposed to even attempting to find a better process that will work for everybody. The attempt to do so may well fail, but the surest way to fail is not to try. -- troy(a)troy.rollo.name - Sydney, Australia
On 9/27/06, Mike McCormack <mike(a)codeweavers.com> wrote:
Responding to each and every patch seems like it would be a waste of Alexandre's time. We should encourage more people to participate in the patch review process, so that we have more reviewers and a more scalable process.
+1 -- James Hawkins
From: Mike McCormack [mailto:mike(a)codeweavers.com]
Seems like that is a system that doesn't scale well at all, as it requires Alexandre to specifically respond to each and every patch.
No, it doesn't require that. It requires *someone* to respond, that could be a fellow developer on wine-devel. A comment added via the web interface or a message about the patch on wine-devel would set the status to RFC, meaning the patch wouldn't show up in Alexandres list (or with a very low priority). It would be the responsibility of the author to set the status back to New if he thinks that's appropriate. Sorry if that wasn't clear from the message you replied to, that message was explicitly aimed at the work Alexandre does, there's more to the system than just that. For the automatic status update to work, we would need to make an automatic connection between wine-devel messages and the patch, could be done using the In-Reply-To header or making sure each message sent out on wine-patches has a unique ID in its subject, a reply to that message would (in most email clients) copy the subject including the unique ID. In the end, when the number of developers grows, the number of reviewers grows too (every developer is a potential reviewer). Seems to scale pretty well actually.
It also seems like it encourages patch submitters to not polish their patches themselves and just submit a higher volume of low quality patches for Alexandre to review, since the onus will then be on him to respond.
First of all, I don't see the encouragement and secondly, how does the current system prevent that?
The current system, which leaves the responsibility for the patch with the submitter both scales better, and encourages patch submitters to think about their patches more.
I'm not sure why you think responsibility for the patch would shift. It would still be the authors responsibility to write acceptable code. The only thing that would change is that the author gets feedback at the earliest possible moment, be it from the bot, peer review or Alexandre.
We should encourage more people to participate in the patch review process, so that we have more reviewers and a more scalable process.
Absolutely. The proposed system doesn't change the review process, it allows peer review too. It just acts as a kind of safety net. Authority and responsibility should go hand-in-hand. I hope it's clear that I don't have a problem with Alexandre having the authority to make the final decision on whether a patch goes in or not, I just believe that with that authority comes the responsibility to inform the author if a patch isn't acceptable in it's current form. Hopefully a fellow developer has already reviewed the patch and told the author something is wrong but in the end we as developers are not psychic and simply cannot know what Alexandre thinks about a patch.
btw. Is there any reason that you can't request a review of your patches, or report the problem that you're trying to fix in bugzilla, as I suggested elsewhere?
How would that improve Alexandres productivity? As pointed out by Troy, it just means he has to look at a patch twice before sending a reply. Not to mention the time it costs the author. Shouldn't we be looking at the productivity of everyone involved in Wine development and not just at Alexandres productivity (although I acknowledge his special position)? I'm a bit surprised (and, to be honest, also a little bit annoyed) about the low value you seem to place on the time contributed by the developers. Ge van Geldorp.
Ge van Geldorp wrote:
From: Mike McCormack [mailto:mike(a)codeweavers.com]
Seems like that is a system that doesn't scale well at all, as it requires Alexandre to specifically respond to each and every patch.
No, it doesn't require that. It requires *someone* to respond, that could be a fellow developer on wine-devel. A comment added via the web interface or a So in a sense you will require some one to respond for any incoming e-mail to wine-patches. And if no one does, Alexandre don't get to see the status? What if he already applied the patch? Now you slowing down what would have worked just fine before.
For the automatic status update to work, we would need to make an automatic connection between wine-devel messages and the patch, could be done using the In-Reply-To header or making sure each message sent out on wine-patches has a unique ID in its subject, a reply to that message would (in most email clients) copy the subject including the unique ID. Ok now you making it dependant on an e-mail client. Yet that's exactly what we are trying avoid with GIT.
In the end, when the number of developers grows, the number of reviewers grows too (every developer is a potential reviewer). Seems to scale pretty well actually. Not really. You can't make some one to review something that don't understand. Or if they are busy/on vacation/away from PC/etc. So in the end you'll end up with either huge queue that no one wants to touch. Or a "clean up" jobs that will once in a while go and mark all patches as old and will require authors to resubmit. How's that better then what we use now?
It also seems like it encourages patch submitters to not polish their patches themselves and just submit a higher volume of low quality patches for Alexandre to review, since the onus will then be on him to respond. First of all, I don't see the encouragement and secondly, how does the current system prevent that? When people well send out right hacks and expect some one to tell then what they really should do. Current system allows to no waste any time on such craft.
The current system, which leaves the responsibility for the patch with the submitter both scales better, and encourages patch submitters to think about their patches more. I'm not sure why you think responsibility for the patch would shift. It would still be the authors responsibility to write acceptable code. The only thing that would change is that the author gets feedback at the earliest possible moment, be it from the bot, peer review or Alexandre. No, by requiring some one to respond you making them responsible (at least until they respond). Of course responses like "sucks" wouldn't be nice, so some one who does understand the subject will have to spend their time to understand the patch, write a review of the patch and send it. And you want that ASAP! Which means whenever patch arrives in wine-patches some one (most likely more then one person) will stop everything they are doing, and start writing a review?
Vitaliy
On Thursday 28 September 2006 05:49, Mike McCormack wrote:
Seems like that is a system that doesn't scale well at all, as it requires Alexandre to specifically respond to each and every patch.
He still has to take an action to review each patch now, and presumably some action to remove it from his queue (speculation since we don't have his process documented - which is why I asked if we could get a vncrec file showing a patch reveiew/application/rejection session, so I could document it). If the process fits into this workflow without disruption the cost should actually be less since it saves having a conversation about why the patch was rejected.
It also seems like it encourages patch submitters to not polish their patches themselves and just submit a higher volume of low quality patches for Alexandre to review, since the onus will then be on him to respond.
You seem to be assuming people are submitting patches they *know* will not be accepted (discounting ones submitted for the purposes of record only where the submitter says they don't expect it to be committed). This would be pathological behaviour since it would require more work on the part of the submitter as well as on the part of Alexandre. In fact the present process is likely to involve more work since it requires people to speculate about why the patch was rejected or passed over, and if they get it wrong, resubmit. Often there wasn't even anything wrong in the first place (the "oops" bitbucket) so all the speculation and rework will be a pointless exercise. If the patch is reworked, it's submitted again, has to be reviewed again, wait, rinse, repeat. That will result in more patches than if people are told the actual (rather than speculative) reason it was not applied.
The current system, which leaves the responsibility for the patch with the submitter both scales better, and encourages patch submitters to think about their patches more.
(the following sounds somewhat harsher than it's intended to but I couldn't figure out a better way to say it) If you can call speculation thinking, but I don't know what you mean by scales better. Speculative review increases the chance that Alexandre has to spend time reviewing more wrong patches because other people guessed wrong. The current system has literally cost me tens of thousands of dollars in wasted developer time on just a handful of patches (not including time I have wasted on it personally), so if by "scales better" you mean "passes off a relatively small cost off (sometimes without actually removing that cost) to others magnified by huge factors" then yes I guess it does "scale better", but scaling up the expense doesn't seem to be a good idea to me. Maybe some other employers don't mind throwing money away like this (Jeremy?), but I do.
Responding to each and every patch seems like it would be a waste of Alexandre's time. We should encourage more people to participate in the patch review process, so that we have more reviewers and a more scalable process.
It's more of an heuristic than a determinitive process unless the reviewers know for certain Alexandre's reasons for rejecting a patch (assuming an unapplied patch has in fact been rejected), which requires either telepathy or that he tell them. The current process results in regular "oops" situations leading to no feedback. There are the "oops, must have missed that patch", and "oops, I thought I did reply to tell you what was wrong with that patch", both of which I have seen multiple times. These at least could be improved with a suitable system in place and result in some improvement even if the speculative post-rejection-review process is kept. I'm not sure why you seem to be opposed to even attempting to find a better process that will work for everybody. The attempt to do so may well fail, but the surest way to fail is not to try. -- troy(a)troy.rollo.name - Sydney, Australia
participants (6)
-
Ge van Geldorp -
James Hawkins -
Mike McCormack -
Troy Rollo -
Troy Rollo -
Vitaliy Margolen