Wrong series name applied to a patch series
bmeng.cn at gmail.com
Wed Sep 18 13:48:36 AEST 2019
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)
> name = Patch.objects.get(series=self, number=1).name
> except Patch.DoesNotExist:
> name = None
> if self.name == name:
> self.name = self._format_name(cover)
> >> > 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 :)
More information about the Patchwork