Hi Daniel, On Tue, Sep 17, 2019 at 2:43 PM Daniel Axtens <d...@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 _______________________________________________ Patchwork mailing list Patchwork@lists.ozlabs.org https://lists.ozlabs.org/listinfo/patchwork
