https://bugs.winehq.org/show_bug.cgi?id=48353
Bug ID: 48353 Summary: Use In-Reply-To when identifying patch series Product: Wine-Testbot Version: unspecified Hardware: x86 OS: Linux Status: NEW Severity: normal Priority: P2 Component: unknown Assignee: wine-bugs@winehq.org Reporter: fgouget@codeweavers.com Distribution: ---
The TestBot detects patchsets from the n/N pattern in the subject and then matches the different parts together based on the From email address and the total number of parts. This means if the TestBot receives the parts of two patchsets with the same number of parts and from the same developer at the same time it is likely to mix elements of the two patchsets, and thus fail to test them.
However it is customary for patches in a series to all reference the first email in the patch series through the In-Reply-To and Message-ID fields. This could be used to resolve ambiguities in such a case.
However we may not want to use this as the sole way to attach parts to the patchset in case setting In-Reply-To is not possible for some reason.
https://bugs.winehq.org/show_bug.cgi?id=48353
--- Comment #1 from François Gouget fgouget@codeweavers.com --- The TestBot also does not use the version naming convention to put the parts together: [PATCH 1/3] vs. [PATCH v2 1/3] or [PATCH 1/3 v2]
https://bugs.winehq.org/show_bug.cgi?id=48353
François Gouget fgouget@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|wine-bugs@winehq.org |fgouget@codeweavers.com
https://bugs.winehq.org/show_bug.cgi?id=48353
François Gouget fgouget@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Severity|normal |major
https://bugs.winehq.org/show_bug.cgi?id=48353
--- Comment #2 from François Gouget fgouget@codeweavers.com --- Support for versions helps but there can still be cases where it's not sufficient.
Unfortunately In-Reply-To is tricky to use when preserving compatibility with unthreaded patch series. Here's a proposed series of assumptions:
* Most developers send threaded patch series but it is still necessary to support unthreaded patch series.
* Whether a developer sends threaded or unthreaded patch series depends on his toolset and thus a developer is unlikely to mix threaded and unthreaded patch series. So it is not worth worrying about them interfering.
* While patch series can be temporally mixed up when the TestBot receives them, they have a clean temporal separation when the developer sends them.
* Because unthreaded patch series are less common, it's ok if the TestBot sometimes gets confused when they get mixed up on reception.
For the following it may be useful to check the TestBot's database schema as a reference: https://source.winehq.org/git/tools.git/blob/HEAD:/testbot/doc/winetestbot-s...
-----
For reference, here's a description of the TestBot's current algorithm for dealing with patch series:
* When receiving a part N email: - Look for a PendingPatchSet object with the same sender, version and total part count. - If there is no match, create a PendingPatchSet object and a matching PendingPatch object. - If there is a match and it already contains a PendingPatch object with the same part number, throw away the current email. - Otherwise create a PendingPatch object for the current email.
Given two patch series (a1, a2, a3) and (b1, b2, b3) where the emails are received in this order a1, a2, b2, a3, b1, b3; this will result in a viable (a1, a2, a3) patch series, b2 getting dropped, and an incomplete patch series (b1, b3). Such 'zombie' patch series will break any attempt to resend the patch series up to 48 hours later (and any attempt will add another 24/48 hours). This is the biggest drawback of this algorithm.
This algorithm could also result in broken patch series if the emails are received in this order for instance: a1, b2, a2, a3, b1, b3. Then it would form a broken patch series (a1, b2, a3), drop a2, and leave an incomplete zombie patch series (b1, b3).
-----
And a basic algorithm to deal with both threaded and unthreaded patch series:
1. Modify PendingPatchSets to have a single field: PK Id - The patch series id.
2. Modify PendingPatches to have the following fields: PK/FK PendingPatchSetId - The patch series potential thread id, that is In-Reply-To || Message-Id. PK No - The part number (from the Subject field). I FromEmail - The email of the sender (i.e. the From field minus the name). I Version - Patch series version (from the Subject field). I TotalParts - The total number of parts (from the Subject field). FK PatchId - Identifies the Patch object which itself points to the job testing it.
3. When receiving a part 1 email with In-Reply-To set: - This means there is a cover and that the patch series uses threading. - Set PendingPatchSetId = In-Reply-To on the PendingPatch object. - Create a matching PendingPatchSet object if necessary.
4. When receiving a part N >= 2 email with In-Reply-To set: - This means the patch series uses threading. - Set PendingPatchSetId = In-Reply-To on the PendingPatch object. - Create a matching PendingPatchSet object if necessary.
5. When receiving a part N email with no In-Reply-To field: - If it is a part 1 we don't know if the email is part of a threaded patch series or not. If it is a part N >= 2, then we know it's an unthreaded patch series. - Collect all PendingPatch objects with the same sender, version, part number, and total part count. Put their PendingPatchSetId field in the Excluded set: this email cannot be part of the same patch series as them. - Loop on all PendingPatch objects with the same sender, version and total part count, excluding those where PendingPatchSetId is in the Excluded set. - If there is no match, this email is in a new patch series so create a new PendingPatch object with PendingPatchSetId = Message-Id and the associated PendingPatchSet object. - Otherwise pick the first PendingPatch object in that set and create a new PendingPatch object with the same PendingPatchSetId. Note that this means that for unthreaded patch series, the PendingPatchSetId may not be the Message-Id of part 1. It is instead the Message-Id of the first part that was received.
6. In all cases, once the new PendingPatch object has been created, check for jobs that can be scheduled in the usual way.
Note that this algorithm does better than the current TestBot algorithm. Given the same two patch series (a1, a2, a3) and (b1, b2, b3) where the emails are received in the same order a1, a2, b2, a3, b1, b3; this will either create the two correct patch series, or two broken ones (a1, a2, b3) and (b1, b2, a3). But at least it will not create a zombie incomplete patch series to mess things up for the next 48 hours.
With the alternative a1, b2, a2, a3, b1, b3 order this would still definitely result in broken patch series: (a1, b2, a3) and (b1, a2, b3), but still no zombie.
-----
Note that the above algorithm does not actually take the temporal send time separation into account. One can do slightly better by tweaking this algorithm as follows:
* Add a SendTime field to the PendingPatch object. Set it based on the email's Date field.
* Instead of picking the first PendingPatch object in point 6, pick the one with the closest SendTime field: this is the email which is most likely to be part of the same patch series.
In the same conditions as above this algorithm the SendTime would almost guarantee that a3 gets assigned to (a1, a2), resulting in two correct patch series.
The SendTime would however not help if the emails are received in the following order: a1, b2, a2, a3, b1, b3: this would still result in the broken patch series: (a1, b2, b3) and (b1, a2, a), but still no zombie.
To improve the results one would have to delay assigning parts to PendingPatchSet objects until more parts arrive which makes the algorithm even more complex.
But this only matters for collisions where at least one patch series is unthreaded and both have the same version which should be pretty rare. So taking the SendTime into account may not be worth it at all.
https://bugs.winehq.org/show_bug.cgi?id=48353
François Gouget fgouget@codeweavers.com changed:
What |Removed |Added ---------------------------------------------------------------------------- Assignee|fgouget@codeweavers.com |wine-bugs@winehq.org
-
WineHQ Bugzilla