Hi everyone,
Wine patches currently have a status described in http://source.winehq.org/patches, yet for patches with the status of 'New', the status becomes confusing.
The legend describes 'New' status as "Patch not even looked at yet, there's still hope...". This is ideal for new patches submitted within that 24-hour commit cycle.
But I'm finding it difficult to follow patches that remain with a status of 'New' for longer than the 24-hour patch cycle. Obviously, on the weekend, patches are held over till Monday, so a longer lead-time is expected. During weekdays, however, it is unclear what is happening with some patches. This, ultimately, raises the question of timeliness.
Has the patch been looked at? If it has, the status often describes what action was taken - committed, rejected, superseded, etc. This is fine, but some patches remain with a status of 'New'.
Experience has told me that patches remaining with a status of 'New' are incorrect in some way. But this is not always the case.
If the patch is incorrect, but close to being accepted, the patch's status should reflect this, by changing to something like "Revision needed". Of course, the "Rejected" status is also appropriate, depending on the severity of coding error.
Also, there are likely to be many times when the maintainer has not had time to evaluate some patches. This means the patch is not new (i.e. recently submitted), but is awaiting review. Once again, I believe the patch status should reflect this situation. The status could be "Not yet reviewed".
In summary, the 'New' status should be reserved for patches that are actually new.
Just some thoughts.
There's also "Pending".
On 30/07/13 04:16, Hugh McMaster wrote:
Hi everyone,
Wine patches currently have a status described in http://source.winehq.org/patches, yet for patches with the status of 'New', the status becomes confusing.
The legend describes 'New' status as "Patch not even looked at yet, there's still hope...". This is ideal for new patches submitted within that 24-hour commit cycle.
But I'm finding it difficult to follow patches that remain with a status of 'New' for longer than the 24-hour patch cycle. Obviously, on the weekend, patches are held over till Monday, so a longer lead-time is expected. During weekdays, however, it is unclear what is happening with some patches. This, ultimately, raises the question of timeliness.
Has the patch been looked at? If it has, the status often describes what action was taken - committed, rejected, superseded, etc. This is fine, but some patches remain with a status of 'New'.
Experience has told me that patches remaining with a status of 'New' are incorrect in some way. But this is not always the case.
If the patch is incorrect, but close to being accepted, the patch's status should reflect this, by changing to something like "Revision needed". Of course, the "Rejected" status is also appropriate, depending on the severity of coding error.
Also, there are likely to be many times when the maintainer has not had time to evaluate some patches. This means the patch is not new (i.e. recently submitted), but is awaiting review. Once again, I believe the patch status should reflect this situation. The status could be "Not yet reviewed".
In summary, the 'New' status should be reserved for patches that are actually new.
Just some thoughts.
On Tuesday, 30 July 2013 9:04 PM, Ken Sharp wrote:
There's also "Pending".
Hi Ken,
Yes, I know about the 'Pending' status. The patches page describes this status in two ways:
1. The patch is not obviously correct at first glance. Making a more convincing argument, preferably in the form of a test case, may help. 2. Waiting for feedback from the main developer in that area.
Both are quite different. Perhaps these should be split with different labels as well - particularly as the first requires revisions, while the second implies that the patch may be accepted as is.
Anyway, to come back to the point, leaving a patch as 'New' is problematic. 'Pending' is more informative, yet perhaps not descriptive enough to cover patches that stay as 'New' over a 24-hour patch cycle.
I think I've seen patches stay in the "New" state in the following cases: * He's convinced you do not have the ability to write a patch he would accept. (There's a common pattern where people will take feedback and attempt to revise their patch to account for it, but not really understand the feedback or how to apply it. The only way to get an acceptable patch in this situation would be to do the work for the submitter.) * He's convinced you're taking a completely wrong approach. (And generally someone has explained this in reply to a previous revision of that patch.) * He thinks there's a good chance you'll revise the patch without his intervention, and is waiting to see if that happens. * The patch is difficult to review, and he's putting it off. * He's travelling and does not have access to a machine that can successfully run the Wine test suite, and he thinks the patch might break the tests.
But I'm guessing based on imperfect memory, so I could be wrong.
Hi Vincent,
You raise some very good points.
On Wednesday, 31 July 2013 2:53 AM, Vincent Povirk wrote:
I think I've seen patches stay in the "New" state in the following cases:
- He's convinced you do not have the ability to write a patch he would accept. (There's a common pattern where people will take feedback and
attempt to revise their patch to account for it, but not really understand the feedback or how to apply it. The only way to get an acceptable patch in this situation would be to do the work for the submitter.)
- He's convinced you're taking a completely wrong approach. (And generally someone has explained this in reply to a previous revision of that
patch.)
Wouldn't these two points simply earn a 'Rejected' status and/or some kind of comment on the wine-devel list?
- He thinks there's a good chance you'll revise the patch without his intervention, and is waiting to see if that happens.
This is good, but only if an error is obvious or more research yields a better means of doing the proposed action. But again, a 'Revision needed' status would help clarify the situation in this case.
- The patch is difficult to review, and he's putting it off.
There are some statuses (e.g. 'needs splitting) to counteract this. But patches are/can be difficult to review. Again, a status such as 'Not yet reviewed' would help.
- He's travelling and does not have access to a machine that can successfully run the Wine test suite, and he thinks the patch might break the
tests.
A 'Not yet reviewed' status or similar would probably be best.
Wouldn't these two points simply earn a 'Rejected' status and/or some kind of comment on the wine-devel list?
Applying a "Rejected" status would require looking at the patch.
But it could be nice to have a status that means "I will not even read a patch from this person to do this specific thing without substantial changes that are not being made."
- He thinks there's a good chance you'll revise the patch without his intervention, and is waiting to see if that happens.
This is good, but only if an error is obvious or more research yields a better means of doing the proposed action. But again, a 'Revision needed' status would help clarify the situation in this case.
But maybe revision isn't needed. Determining that would require reviewing the patch.
- The patch is difficult to review, and he's putting it off.
There are some statuses (e.g. 'needs splitting) to counteract this. But patches are/can be difficult to review. Again, a status such as 'Not yet reviewed' would help.
- He's travelling and does not have access to a machine that can successfully run the Wine test suite, and he thinks the patch might break the
tests.
A 'Not yet reviewed' status or similar would probably be best.
But "Not yet reviewed" is what "New" is supposed to be. The problem is there are situations where patches are never reviewed and no one is told why.
- The patch is difficult to review, and he's putting it off.
There are some statuses (e.g. 'needs splitting) to counteract this. But patches are/can be difficult to review. Again, a status such as 'Not yet reviewed' would help.
- He's travelling and does not have access to a machine that can
successfully run the Wine test suite, and he thinks the patch might break the tests.
A 'Not yet reviewed' status or similar would probably be best.
But "Not yet reviewed" is what "New" is supposed to be. The problem is there are situations where patches are never reviewed and no one is told why.
This is the point then. Adding some new status categories would help to address this situation. I'll draft some possibilities. Your list from earlier in this thread would make a good reference point.
On Thursday, 1 August 2013 12:19 AM, Vincent Povirk wrote:
The problem is there are situations where patches are never reviewed and no one is told why.
After some thought, it occurred to me that the said patches are being reviewed; they just never have their status altered.
So, one possible way to work through this issue is to split the meaning of 'Pending' into two parts.
'Pending' currently describes two actions for patches:
* 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.
We could have: * P1: "The patch is not obviously correct at first glance. [Revision needed / Anything equivalent to go here]." * P2: "Waiting for feedback from the main developer in that area."
Changing the patch status would then give an indication of action to be taken by the developer. Of course, while these ideas are likely to provide greater clarity, the downside is that there is more work involved for the maintainer.
-
Hugh McMaster -
Ken Sharp -
Vincent Povirk