Solving patch series collisions with domains?
On Thu, 5 Mar 2020, Marvin wrote: [...]
=== debiant (build log) ===
error: patch failed: dlls/d3d11/device.c:941 error: patch failed: dlls/d3d11/state.c:1079 error: patch failed: dlls/wined3d/device.c:3600 error: patch failed: include/wine/wined3d.h:2037 Task: Patch failed to apply
The TestBot mixed up this 5-parts wined3d patch series with the other 5-parts patch series for qcap / qedit. One was a v2 and not the other so taking into account the version would have avoided the issue in this case (the patch for this should go in next monday). But this could just as well have happened with unversioned patch series: [PATCH 1/5] wined3d: ... vs. [PATCH 1/5] qcap/capturegraph: ... The subject parsing code extracts a property I have been calling the 'domain' (for lack of a better term) which is a word that's sometimes added in the square brackets. It's usually used to indicate the Git repository the patch should go in: [vkd3d 1/2], [tools], etc. But it could also be used to avoid collisions between patch series: where the TestBot identifies patch series based on the (Sender, Version, PartCount) tuple, it could add the domain to the mix: (Sender, Domain, Version, PartCount). This would allow sending two patch series with the same number of parts without causing a collision: [wined3d 1/5] wined3d: ... + [qcap 1/5] qcap/capturegraph: ... or [wined3d 1/5] wined3d: ... + [PATCH 1/5] qcap/capturegraph: ... or [PATCH 1/5] wined3d: ... + [PATCH qcap 1/5] qcap/capturegraph: ... PATCH can be thrown in to help identify patches, or omitted. It makes no difference to the TestBot. Not using a domain would work just as well in the absence of a collision. And if two patch series risk colliding, adding a domain would help avoid the collision. The only drawback is one has to anticipate and remember to specify the domain before sending the emails. Would taking the domain make sense? Would it be worth it? -- Francois Gouget <fgouget(a)codeweavers.com>
I think it'd make more sense and be simpler to use the From: field of the email (not the patch), assuming you have access to that. Or failing that, the patch author would probably be OK, sending a series with patches by multiple people is unusual.
On Fri, Mar 6, 2020 at 8:27 AM Vincent Povirk (they/them) <vincent(a)codeweavers.com> wrote:
I think it'd make more sense and be simpler to use the From: field of the email (not the patch), assuming you have access to that. Or failing that, the patch author would probably be OK, sending a series with patches by multiple people is unusual.
Hi Vincent, As I read it, the issue here is not about determining the sender/author/... but rather that multiple patch series from the _same_ person, each with the _same_ number of patches can be confused. The 'domain' is a proposal for an _additional_ way of distinguishing them. Thanks, Jeff
As I read it, the issue here is not about determining the sender/author/... but rather that multiple patch series from the _same_ person, each with the _same_ number of patches can be confused. The 'domain' is a proposal for an _additional_ way of distinguishing them.
Why isn't temporal difference enough in this case? Did the series interleave?
On Fri, 6 Mar 2020, Vincent Povirk (they/them) wrote:
I think it'd make more sense and be simpler to use the From: field of the email (not the patch), assuming you have access to that.
That's already what the TestBot does:
the TestBot identifies patch series based on the (Sender, Version, PartCount) tuple,
But making use of the Message-Id and In-Reply-To fields could help. The would be 3 or 4 cases to handle: 1. Unthreaded patch series (still seems to happen sometimes). 2. Threaded with all parts being replies to the first one. 3. Threaded with all parts being replies to the cover (which is not a patch and thus ignored by the TestBot). 4. Threaded with each part being a reply to the previous one. I don't think case 4 happens in practice so I'm going to assume it can be ignored for now. -- Francois Gouget <fgouget(a)codeweavers.com>
On 3/6/20 8:18 AM, Francois Gouget wrote:
On Thu, 5 Mar 2020, Marvin wrote: [...]
=== debiant (build log) ===
error: patch failed: dlls/d3d11/device.c:941 error: patch failed: dlls/d3d11/state.c:1079 error: patch failed: dlls/wined3d/device.c:3600 error: patch failed: include/wine/wined3d.h:2037 Task: Patch failed to apply
The TestBot mixed up this 5-parts wined3d patch series with the other 5-parts patch series for qcap / qedit.
One was a v2 and not the other so taking into account the version would have avoided the issue in this case (the patch for this should go in next monday).
But this could just as well have happened with unversioned patch series: [PATCH 1/5] wined3d: ... vs. [PATCH 1/5] qcap/capturegraph: ...
The subject parsing code extracts a property I have been calling the 'domain' (for lack of a better term) which is a word that's sometimes added in the square brackets. It's usually used to indicate the Git repository the patch should go in: [vkd3d 1/2], [tools], etc.
But it could also be used to avoid collisions between patch series: where the TestBot identifies patch series based on the (Sender, Version, PartCount) tuple, it could add the domain to the mix: (Sender, Domain, Version, PartCount).
This would allow sending two patch series with the same number of parts without causing a collision:
[wined3d 1/5] wined3d: ... + [qcap 1/5] qcap/capturegraph: ... or [wined3d 1/5] wined3d: ... + [PATCH 1/5] qcap/capturegraph: ... or [PATCH 1/5] wined3d: ... + [PATCH qcap 1/5] qcap/capturegraph: ...
PATCH can be thrown in to help identify patches, or omitted. It makes no difference to the TestBot.
Not using a domain would work just as well in the absence of a collision. And if two patch series risk colliding, adding a domain would help avoid the collision. The only drawback is one has to anticipate and remember to specify the domain before sending the emails.
Would taking the domain make sense? Would it be worth it?
Wait, so why isn't checking In-Reply-To enough here?
On Fri, 6 Mar 2020, Zebediah Figura wrote: [...]
Would taking the domain make sense? Would it be worth it?
Wait, so why isn't checking In-Reply-To enough here?
To make sure there's no confusion: the TestBot is not taking In-Reply-To into account so far. Also taking In-Reply-To into account while preserving compatibility with unthreaded patch series is a bit tricky whereas domain support would be simpler. But I agree that having support for In-Reply-To is the sane way to go. So I looked into it and updated the corresponding bug with a proposed way forward: https://bugs.winehq.org/show_bug.cgi?id=48353 For now I'm sending the patch to take the patch series version into account and I'm keeping In-Reply-To support in my todo list. -- Francois Gouget <fgouget(a)codeweavers.com>
participants (4)
-
Francois Gouget -
Jeff Smith -
Vincent Povirk (they/them) -
Zebediah Figura