[Bug 47042] New: Ignore replies to patchset parts

[wine-bugs at winehq.org]

https://bugs.winehq.org/show_bug.cgi?id=47042

            Bug ID: 47042
           Summary: Ignore replies to patchset parts
           Product: Wine-Testbot
           Version: unspecified
          Hardware: x86
                OS: Linux
            Status: NEW
          Severity: normal
          Priority: P2
         Component: unknown
          Assignee: wine-bugs at winehq.org
          Reporter: fgouget at codeweavers.com
      Distribution: ---

The TestBot treats every email as a potential patch.

This makes sense as replies to a patch could be a new version for that patch
which means it should be tested.

But replies to parts of patches are different. Here is what happened due to the
failure to recognize this.

I'm not sure why but the TestBot must have received the following emails in
this order (this is not the order of the send times but it matches the order in
which  I received the emails in my inbox):

* [PATCH 1/5] ntoskrnl.exe: Implement PsLookupThreadByThreadId.
  https://www.winehq.org/pipermail/wine-devel/2019-April/144085.html

  Part 1/5 in a patch series. The TestBot created a PendingPatchSet object to
track the different parts.

* Re: [PATCH 1/5] ntoskrnl.exe: Implement PsLookupThreadByThreadId.
  https://www.winehq.org/pipermail/wine-devel/2019-April/144088.html

  The TestBot decided this looked like a patch because the HTML version
contains lines that start with "+++ ". Thus it treated this email as a new
version of the previous patch, like it usually does, and replaced patch 1 in
the PendingPatchSet.

* Re: [PATCH 2/5] ntoskrnl.exe: Implement KeAreApcsDisabled using critical
region functions.
  https://www.winehq.org/pipermail/wine-devel/2019-April/144086.html

  The TestBot created a job to test this patch but used the updated part 1 of
the patch set which in fact was an HTML email. This resulted in an invalid
patch and thus a failure to apply.


The TestBot probably did two things wrong:

1. It took a patch from an HTML attachment.
   The TestBot already rejects those but somehow the $Part->effective_type ne
"text/html" test in Patch::NewPatch() did not work.

2. It replaced part 1 of a patchset with a new version.
   One cannot send a new version of a part in a patchset: the whole patchset
must be resubmitted (*). So the TestBot should never replace a part in a
patchset.

   It looks like this check should be done in PendingPatchSets::NewSubmission()
near the $Set->Parts->GetItem($PartNo) call.

   The issue here is that we may receive emails out of order. So we could
receive a reply to part 1, and thus add it as part 1, before we receive the
actual email for part 1.



(*) Otherwise we'd have to keep the patchsets around and create new jobs for
all parts that follow the replaced part. And if multiple parts are replaced...
madness!

-- 
Do not reply to this email, post in Bugzilla using the
above URL to reply.
You are receiving this mail because:
You are watching all bug changes.