Wrong series name applied to a patch series

[Bin Meng]

Hi Daniel,

On Tue, Sep 17, 2019 at 2:43 PM Daniel Axtens <dja at axtens.net> wrote:
>
> Hi Bin Meng,
>
> >> > So the parsing logic of patchwork has something wrong.
> >>
> >> Thanks for pointing this out. I'll have a more detailed look when I get
> >> a minute, but we do have some known issues around parallelism: if
> >> multiple messages are being parsed at the same time, we can end up
> >> getting things wrong, and this happens from time to time on
> >> OzLabs. That's probably what went wrong... although it is weird for it
> >> to happen to multiple revisions of the same series... anyway as I say
> >> I'll take a look when I get a chance.
> >
> > Thanks for looking into this.
> >
> > I've just seen almost the exact the same issue as v4. Please check the
> > following example (happens to be the same series, but v8)
> >
> > http://patchwork.ozlabs.org/project/qemu-devel/list/?series=129494
> > whose series name was set to: [v8,01/32] riscv: hw: Remove duplicated
> > "hw/hw.h" inclusion, as it was parsed by patchwork as a single patch
> > series.
> >
> > http://patchwork.ozlabs.org/project/qemu-devel/list/?series=129493
> > whose series name was set to: riscv: sifive_u: Improve the emulation
> > fidelity of sifive_u machine, which is the cover letter's name
> > [00/32].
> >
> > A little difference from the v4 example before, is that the [01/32]
> > was given a series ID 129494 and the reset of the patches was given a
> > series ID 129493, but v4 is on the contrary.
> >
> > I was refreshing the QEMU patchwork immediately after I sent the
> > series, and I saw at first all patches in series 129493 was given a
> > series name "Untitled series #129493", at the time when series 129494
> > was already given a name "[v8,01/32] riscv: hw: Remove duplicated
> > "hw/hw.h" inclusion". I suspect what happened when patchwork parser
> > parsed the v8 series:
> >
> > * The cover letter arrived very late than [v8, 01/32] and some other
> > patches in series 129493
> > * When patchwork saw patch [v8, 01/32], it did not see the cover
> > letter [v8, 00/32], and since it's perfectly OK for a patch series
> > without a cover letter, hence patchwork assigned the patch [v8, 01/32]
> > name as its series name
> > * Finally when the cover letter arrived at the mailing list, patchwork
> > fixed up the series 129493 name, replacing "Untitled series #129493"
> > with the cover letter name
> >
> > I am not sure if this is a bug that could be categorized as
> > "parallelism", because I think when patchwork parsing the very first
> > patch in a series, like [v8, 01/32], it should check its "In-Reply-To"
> > and when it refers to a patch email that has not arrived, patchwork
> > should wait instead of setting the series name blindly to the first
> > patch name.
>
> We actually already have this logic in models.py:
>
>         # we allow "upgrading of series names. Names from different
>         # sources are prioritized:
>         #
>         # 1. user-provided names
>         # 2. cover letter-based names
>         # 3. first patch-based (i.e. 01/nn) names
>         #
>         # Names are never "downgraded" - a cover letter received after
>         # the first patch will result in the name being upgraded to a
>         # cover letter-based name, but receiving the first patch after
>         # the cover letter will not change the name of the series.
>         #
>         # If none of the above are available, the name will be null.

Thank you for pointing out the sources. Good to know patchwork already
has such logic!

>
>         if not self.name:
>             self.name = self._format_name(cover)
>         else:
>             try:
>                 name = Patch.objects.get(series=self, number=1).name
>             except Patch.DoesNotExist:
>                 name = None
>
>             if self.name == name:
>                 self.name = self._format_name(cover)
>
>         self.save()
>
> >> > For the latter case, what I might guess is happening is that [01/28]
> >> > hit the list before [00/28], so there was no series to append to.
> >> >
> >> > But even if that's the case, I think patchwork has all the information
> >> > and can still correct the mistake later when it sees all other
> >> > message's "In-Reply-To"s are referring to the same cover letter and
> >> > should group them into one series.
>
> It's a parallelism problem because we end up getting two different
> series created: 00 and 01 are processed simultaneously, we end up with
> two series, and we don't have logic to collapse series. We can avoid it
> by serialising parsing, but that requires serialisation across
> processes. I think the FreeDesktop fork does that, but it requires
> getting file locks right, and that's a Difficult Problem in general.
>

OK, now I understand. So it's a known issue :)

Regards,
Bin