Re: commitfest.postgresql.org is no longer fit for purpose

From: Thomas Munro <thomas(dot)munro(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Bruce Momjian <bruce(at)momjian(dot)us>, Dmitry Dolgov <9erthalion6(at)gmail(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: commitfest.postgresql.org is no longer fit for purpose
Date: 2024-05-20 05:49:40
Message-ID: CA+hUKGJGT7cS91xDJHR0s941Ps9DB8uyqZ8HQ=OaiC=ueCj81w@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Mon, May 20, 2024 at 1:09 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> (The cfbot tends to discourage this, since as soon as one of the
> patches is committed it no longer knows how to apply the rest.
> Can we improve on that tooling somehow?)

Cfbot currently applies patches with (GNU) patch
--no-backup-if-mismatch -p1 -V none -f. The -f means that any
questions of the form "Reversed (or previously applied) patch
detected! Assume -R? [y]" is answered with "yes", and the operation
fails. I wondered if --forward would be better. It's the right idea,
but it seems to be useless in practice for this purpose because the
command still fails:

tmunro(at)phonebox postgresql % patch --forward -p1 < x.patch || echo XXX
failed $?
patching file 'src/backend/postmaster/postmaster.c'
Ignoring previously applied (or reversed) patch.
1 out of 1 hunks ignored--saving rejects to
'src/backend/postmaster/postmaster.c.rej'
XXX failed 1

I wondered if it might be distinguishable from other kinds of failure
that should stop progress, but nope:

patch's exit status is 0 if all hunks are applied successfully, 1
if some hunks cannot be applied or there were merge conflicts,
and 2 if there is more serious trouble. When applying a set of
patches in a loop it behooves you to check this exit status so
you don't apply a later patch to a partially patched file.

I guess I could parse stdout or whatever that is and detect
all-hunks-ignored condition, but that doesn't sound like fun...

Perhaps cfbot should test explicitly for patches that have already
been applied with something like "git apply --reverse --check", and
skip them. That would work for exact patches, but of course it would
be confused by any tweaks made before committing. If going that way,
it might make sense to switch to git apply/am (instead of GNU patch),
to avoid contradictory conclusions.

The reason I was using GNU patch in the first place is that it is/was
a little more tolerant of some of the patches people used to send a
few years back, but now I expect everyone uses git format-patch and
would be prepared to change their ways if not. In the past we had a
couple of cases of the reverse, that is, GNU patch couldn't apply
something that format-patch produced (some edge case of renaming,
IIRC) and I'm sorry that I never got around to changing that.

Sometimes I question the sanity of the whole thing. Considering
cfbot's original "zero-effort CI" goal (or I guess "zero-extra-effort"
would be better), I was curious about what other projects had the same
idea, or whether we're really just starting at the "wrong end", and
came up with:

https://github.com/getpatchwork/patchwork
http://vger.kernel.org/bpfconf2022_material/lsfmmbpf2022-bpf-ci.pdf
<-- example user
https://github.com/patchew-project/patchew

Actually cfbot requires more effort than those, because it's driven
first by Commitfest app registration. Those projects are extremists
IIUC: just write to a mailing list, no other bureaucracy at all (at
least for most participants, presumably administrators can adjust the
status in some database when things go wrong?). We're actually
halfway to Gitlab et al already, with a web account and interaction
required to start the process of submitting a patch for consideration.
What I'm less clear on is who else has come up with the "bitrot" test
idea, either at the mailing list or web extremist ends of the scale.
Those are also generic tools, and cfbot obviously knows lots of things
about PostgreSQL, like the "highlights" and probably more things I'm
forgetting.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message John Naylor 2024-05-20 06:05:32 Re: Lowering the minimum value for maintenance_work_mem
Previous Message Hayato Kuroda (Fujitsu) 2024-05-20 04:58:31 RE: Proposal: Filter irrelevant change before reassemble transactions during logical decoding