Wrong series name applied to a patch series

[Daniel Axtens]

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.

        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.

Regards,
Daniel
>> ... and we do have code to do this, which is what makes me a bit
>> suspicious that you've just lost a race.
>
> Regards,
> Bin