Hello,
http://source.winehq.org/patches has the following legend for the Pending patch status:
Pending * The patch is not obviously correct at first glance. Making a more convincing argument, preferably in the form of a test case, may help. * Waiting for feedback from the main developer in that area.
It's very confusing, and absolutely not clear what is required from the patch submitter, especially since *there is no any feedback on the patch*. 'Rejected' at least requies some sort of feedback, while 'Pending' doesn't. To me 'Pending' looks like a silent case of 'Reject', but without any obligation to explain why.
I find myself on somewhat shaky ground when I see a bunch of my patches in the pending state, especially if they already contain the tests and main developer in the area I'm changing is Alexandre :).
Is it possible to get at least some feedback for pending patches? Pretty please?
Dmitry Timoshkov dmitry@baikal.ru writes:
It's very confusing, and absolutely not clear what is required from the patch submitter, especially since *there is no any feedback on the patch*. 'Rejected' at least requies some sort of feedback, while 'Pending' doesn't. To me 'Pending' looks like a silent case of 'Reject', but without any obligation to explain why.
I find myself on somewhat shaky ground when I see a bunch of my patches in the pending state, especially if they already contain the tests and main developer in the area I'm changing is Alexandre :).
Is it possible to get at least some feedback for pending patches? Pretty please?
The pending state is feedback. It means that the patch is not clearly correct, but that it's complicated to articulate exactly why. Like it says, you should try to make it more convincing, either by simplifying the patch, or writing a test case.
For instance your patch 84692 says that "tests confirm that", but you don't say which tests, and there are no new tests or fixed todos in the patch, so it looks suspicious. Yes, I could dig out the tests myself and investigate it in detail, but when it gets to that point I usually just move on to the next patch, hence "pending".
Alexandre Julliard julliard@winehq.org wrote:
The pending state is feedback. It means that the patch is not clearly correct, but that it's complicated to articulate exactly why. Like it says, you should try to make it more convincing, either by simplifying the patch, or writing a test case.
I'm sorry, but that's not a feedback, and casual contributors may even not be aware of that patch tracking page. And as I mentioned if the patch already contains the tests it's not really obvious what should be added in addition. In the light of recent discussions about friendliness to users in bugzilla, I think that developers deserve at least small fraction of friendliness as well (Alexandre, you are nice and friendly all the time, but at least I sometimes feel like sending the patches to a blackwhole).
For instance your patch 84692 says that "tests confirm that", but you don't say which tests, and there are no new tests or fixed todos in the patch, so it looks suspicious. Yes, I could dig out the tests myself and investigate it in detail, but when it gets to that point I usually just move on to the next patch, hence "pending".
There is not much tests for SetParent, and 84692 suggests to look at the tests added by http://www.winehq.org/pipermail/wine-patches/2011-February/098711.html
WM_SHOWWINDOW at the start and at the end of every message sequence means that ShowWindow() should be used to hide and show the window during SetParent call processing.
But even the same patch sent another day after the tests http://www.winehq.org/pipermail/wine-patches/2011-February/098748.html didn't get any feedback and died in the pending state.
Taking an opportunity to discuss other my patches :) I'd like to get a comment to 84685 as well.
Thanks.
Dmitry Timoshkov dmitry@baikal.ru writes:
I'm sorry, but that's not a feedback, and casual contributors may even not be aware of that patch tracking page. And as I mentioned if the patch already contains the tests it's not really obvious what should be added in addition. In the light of recent discussions about friendliness to users in bugzilla, I think that developers deserve at least small fraction of friendliness as well (Alexandre, you are nice and friendly all the time, but at least I sometimes feel like sending the patches to a blackwhole).
If you want to write a script that sends a nice friendly mail every time a patch changes status I'd be happy to use it.
There is not much tests for SetParent, and 84692 suggests to look at the tests added by http://www.winehq.org/pipermail/wine-patches/2011-February/098711.html
WM_SHOWWINDOW at the start and at the end of every message sequence means that ShowWindow() should be used to hide and show the window during SetParent call processing.
That's the sort of explanation you should have included in your patch, instead of expecting me to dig through the source or the list archive to find it. Plus of course some explanation as to why the test can't be marked as succeeding despite the change.
Taking an opportunity to discuss other my patches :) I'd like to get a comment to 84685 as well.
Try making it simpler.
Alexandre Julliard julliard@winehq.org wrote:
WM_SHOWWINDOW at the start and at the end of every message sequence means that ShowWindow() should be used to hide and show the window during SetParent call processing.
That's the sort of explanation you should have included in your patch, instead of expecting me to dig through the source or the list archive to find it. Plus of course some explanation as to why the test can't be marked as succeeding despite the change.
There are other things inside of SetParent that break message sequences, that's why the patches were prepended with a comment explaining what's wrong with Setparent and saying that it's the first step.
Taking an opportunity to discuss other my patches :) I'd like to get a comment to 84685 as well.
Try making it simpler.
Thanks.
Alexandre Julliard julliard@winehq.org wrote:
WM_SHOWWINDOW at the start and at the end of every message sequence means that ShowWindow() should be used to hide and show the window during SetParent call processing.
That's the sort of explanation you should have included in your patch, instead of expecting me to dig through the source or the list archive to find it. Plus of course some explanation as to why the test can't be marked as succeeding despite the change.
Alexandre, do I need to provide more clarifications?
Alexandre,
On 03/28/2012 10:17 AM, Alexandre Julliard wrote:
Dmitry Timoshkov dmitry@baikal.ru writes:
It's very confusing, and absolutely not clear what is required from the patch submitter, especially since *there is no any feedback on the patch*. 'Rejected' at least requies some sort of feedback, while 'Pending' doesn't. To me 'Pending' looks like a silent case of 'Reject', but without any obligation to explain why.
I find myself on somewhat shaky ground when I see a bunch of my patches in the pending state, especially if they already contain the tests and main developer in the area I'm changing is Alexandre :).
Is it possible to get at least some feedback for pending patches? Pretty please?
The pending state is feedback. It means that the patch is not clearly
yes, but the worst possible feedback.
New people assume you or the area maintainer need to still make up their mind on the patch but that's not the case, it is a done deal.
correct, but that it's complicated to articulate exactly why. Like it says, you should try to make it more convincing, either by simplifying the patch, or writing a test case.
Iff one knows you and knows to read in between the lines then the above can be reworded as: "Current implementation rejected; idea might have some merit."
That's what "Pending" means most of the times.
Now for the other meanings of "Pending": - "Waiting for feedback from the main developer in that area." Don't remember to have ever seen that. Those patches stay normally in "New" as you don't look at them before the area developer ACKs.
- "preferably in the form of a test case" That should be "Needs tests".
- With the two (minor) other meanings of "Pending" handled by other patch states we can rename "Pending" to "Convince me" or "Needs work". That would make it more obvious what is meant.
For instance your patch 84692 says that "tests confirm that", but you don't say which tests, and there are no new tests or fixed todos in the patch, so it looks suspicious. Yes, I could dig out the tests myself and investigate it in detail, but when it gets to that point I usually just move on to the next patch, hence "pending".
IMHO that should have been "Needs tests"; that would have forced Dmitry to point you to the tests.
bye michael
Michael Stefaniuc mstefani@redhat.com writes:
The pending state is feedback. It means that the patch is not clearly
yes, but the worst possible feedback.
New people assume you or the area maintainer need to still make up their mind on the patch but that's not the case, it is a done deal.
Not necessarily. Patches are automatically marked pending as soon as I look at them, until they get a more permanent resolution. There can be many different reasons why they don't get one.
Iff one knows you and knows to read in between the lines then the above can be reworded as: "Current implementation rejected; idea might have some merit."
That's what "Pending" means most of the times.
Sometimes, yes.
Now for the other meanings of "Pending":
- "Waiting for feedback from the main developer in that area." Don't remember to have ever seen that. Those patches stay normally in
"New" as you don't look at them before the area developer ACKs.
No, I usually do look at them. Sometimes I go back and mark them as New again, but not always.
- "preferably in the form of a test case" That should be "Needs tests".
It would be, in cases where tests are clearly the right way. There are cases where it's not clear whether a test is feasible, and using "Needs tests" may lead the submitter down the wrong path.
- With the two (minor) other meanings of "Pending" handled by other
patch states we can rename "Pending" to "Convince me" or "Needs work". That would make it more obvious what is meant.
I can certainly rename "pending" to "convince me" if it helps, but I think it's important to have some sort of open-ended resolution to leave room for developer's judgement. There are many cases where I'm genuinely undecided about the right resolution, and I feel it's preferable to show the undecision and leave it to the developer to try to address one way or the other, even if that's of course harder than being told exactly what to do.
On Wed, Mar 28, 2012 at 12:01 PM, Alexandre Julliard julliard@winehq.orgwrote:
Michael Stefaniuc mstefani@redhat.com writes:
The pending state is feedback. It means that the patch is not clearly
yes, but the worst possible feedback.
New people assume you or the area maintainer need to still make up their mind on the patch but that's not the case, it is a done deal.
Not necessarily. Patches are automatically marked pending as soon as I look at them, until they get a more permanent resolution. There can be many different reasons why they don't get one.
Iff one knows you and knows to read in between the lines then the above can be reworded as: "Current implementation rejected; idea might have some merit."
That's what "Pending" means most of the times.
Sometimes, yes.
Now for the other meanings of "Pending":
- "Waiting for feedback from the main developer in that area." Don't remember to have ever seen that. Those patches stay normally in
"New" as you don't look at them before the area developer ACKs.
No, I usually do look at them. Sometimes I go back and mark them as New again, but not always.
- "preferably in the form of a test case" That should be "Needs tests".
It would be, in cases where tests are clearly the right way. There are cases where it's not clear whether a test is feasible, and using "Needs tests" may lead the submitter down the wrong path.
- With the two (minor) other meanings of "Pending" handled by other
patch states we can rename "Pending" to "Convince me" or "Needs work". That would make it more obvious what is meant.
I can certainly rename "pending" to "convince me" if it helps, but I think it's important to have some sort of open-ended resolution to leave room for developer's judgement. There are many cases where I'm genuinely undecided about the right resolution, and I feel it's preferable to show the undecision and leave it to the developer to try to address one way or the other, even if that's of course harder than being told exactly what to do.
-- Alexandre Julliard julliard@winehq.org
I think the general feeling is that Pending should be renamed to "Decision pending" and that more feedback is needed at least in the form of "this is the wrong approach" or "this may be the right approach, explain yourself better". But the general feeling is that "Pending" currently means "Not good enough" and never gets looked at again. I agree it's very confusing.
J. Leclanche
Jerome Leclanche adys.wh@gmail.com wrote:
I think the general feeling is that Pending should be renamed to "Decision pending" and that more feedback is needed at least in the form of "this is the wrong approach" or "this may be the right approach, explain yourself better". But the general feeling is that "Pending" currently means "Not good enough" and never gets looked at again. I agree it's very confusing.
It should be in the best ineterests of the project to provide as much feedback as possible, and should improve not only amount of accepted code (by encouraging developers provide more comments/explanations/tests/etc. and more actively discuss possible ways to fix a particular bug), but also code quality and help developers better understand what could be improved in future submissions. Silently marking a patch as 'pending' doesn't help with that at all.
Consider that if two my patches for SetParent would be accepted one year ago but not one year later (without a single change but just afer asking once again what's wrong with them), that fundamental SetParent bug could be already fixed long time ago.
On 09/04/12 16:50, Dmitry Timoshkov wrote:
It should be in the best ineterests of the project to provide as much feedback as possible, and should improve not only amount of accepted code (by encouraging developers provide more comments/explanations/tests/etc. and more actively discuss possible ways to fix a particular bug), but also code quality and help developers better understand what could be improved in future submissions. Silently marking a patch as 'pending' doesn't help with that at all. Consider that if two my patches for SetParent would be accepted one year ago but not one year later (without a single change but just afer asking once again what's wrong with them), that fundamental SetParent bug could be already fixed long time ago.
I agree a lot of developers would benefit from feedback, however that does not appear to be the Wine way of doing business. Maybe a halfway measure would be to automatically notify the developer that their patch has been marked as pending and then the developer can ask.
Jeff Latimer lats@yless4u.com.au wrote:
I agree a lot of developers would benefit from feedback, however that does not appear to be the Wine way of doing business. Maybe a halfway measure would be to automatically notify the developer that their patch has been marked as pending and then the developer can ask.
Something like "Your patch is pending, you have to ask to get an answer why."? But the answer known, and it is * The patch is not obviously correct at first glance. Making a more convincing argument, preferably in the form of a test case, may help. * Waiting for feedback from the main developer in that area.
And how is that better than current situation when in order to get a feedback a developer have to ask?
Q: What's wrong with my patch xxx? A: The patch is not obviously correct at first glance... Q: What can I do? A: Making a more convincing argument, preferably in the form of a test case, may help.
... or may not help.
http://wiki.winehq.org/SubmittingPatches suggests "If your patch disappears from http://source.winehq.org/patches/ without being committed, improve it (perhaps by adding more tests) and resend."
The patch disappears from the patch tracker in a month. You have plenty time for 11 resends in an year, don't you?
On Tue, Apr 10, 2012 at 04:52:42PM +0900, Dmitry Timoshkov wrote:
Jeff Latimer lats@yless4u.com.au wrote:
I agree a lot of developers would benefit from feedback, however that does not appear to be the Wine way of doing business. Maybe a halfway measure would be to automatically notify the developer that their patch has been marked as pending and then the developer can ask.
Something like "Your patch is pending, you have to ask to get an answer why."? But the answer known, and it is
- The patch is not obviously correct at first glance. Making a more
convincing argument, preferably in the form of a test case, may help.
- Waiting for feedback from the main developer in that area.
And how is that better than current situation when in order to get a feedback a developer have to ask?
Q: What's wrong with my patch xxx? A: The patch is not obviously correct at first glance... Q: What can I do? A: Making a more convincing argument, preferably in the form of a test case, may help.
... or may not help.
http://wiki.winehq.org/SubmittingPatches suggests "If your patch disappears from http://source.winehq.org/patches/ without being committed, improve it (perhaps by adding more tests) and resend."
The patch disappears from the patch tracker in a month. You have plenty time for 11 resends in an year, don't you?
Also Alexandre to some parts comments on bad patches these days. :)
Ciao, Marcus
Marcus Meissner marcus@jet.franken.de wrote:
Also Alexandre to some parts comments on bad patches these days. :)
If by 'bad patches' you mean patches in the rejected state that's not the subject of this thread.
-
Alexandre Julliard -
Dmitry Timoshkov -
Jeff Latimer -
Jerome Leclanche -
Marcus Meissner -
Michael Stefaniuc