Subject: Re: Opinion poll: Sending an automated email to a thread when it gets added to the commitfest

From: Jelte Fennema-Nio <[email-masked]>
Date: Thu, 15 Aug 2024 09:59:47 +0200

On Thu, 15 Aug 2024 at 05:17, Euler Taveira <[email-masked]> wrote:
> +1. Regarding the CI link, I would be good if the CF entry automatically adds a
> link to the CI run. It can be a separate field or even add it to "Links".

I'm on it. I think this email should be a subset of the info on the CF
entry webpage, so I'll first change the cf entry page to include all
this info.

> I'm not sure about 4, you can always check the latest patch in the CF entry (it
> is usually the "latest attachment") and that's what the cfbot uses to run.

This is definitely a personal preference thing, but I like reading
patches on GitHub much better than looking at raw patch files. It has
syntax highlighting and has those little arrow buttons at the top of a
diff, to show more context about the file.

I realized a 5th thing that I would want in the email and cf entry page

5. A copy-pastable set of git command that checks out the patch by
downloading it from the cfbot repo like this:

git config branch.cf/5107.remote
https://github.com/postgresql-cfbot/postgresql.git
git config branch.cf/5107.merge refs/heads/cf/5107
git checkout -b cf/5107
git pull

> If I understand your proposal correctly, there will be another email to the
> thread if the previous CF was closed and someone opened a new CF entry.
> Sometimes some CF entries are about the same thread.

Yeah, that's correct. If a new CF entry is created for an existing
thread a new email would be sent. But to be clear, if CF entry is
pushed to the next commitfest, **no** new email would be sent.

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 15 Aug 2024 15:33:17 +0200

> git config branch.cf/5107.merge refs/heads/cf/5107

Maybe this kind of thing should rather be on the linked-to web page, not 
in every email.

But a more serious concern here is that the patches created by the cfbot 
are not canonical.  There are various heuristics when they get applied. 
I would prefer that people work with the actual patches sent by email, 
at least unless they know exactly what they are doing.  We don't want to 
create parallel worlds of patches that are like 90% similar but not 
really identical.

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Thu, 15 Aug 2024 16:01:47 +0200

> Maybe this kind of thing should rather be on the linked-to web page, not

Yeah, I'll first put a code snippet on the page for the commitfest entry.

> create parallel worlds of patches that are like 90% similar but not

I'm not really sure what kind of heuristics and resulting differences
you're worried about here. The heuristics it uses are very simple and
are good enough for our CI. Basically they are:
1. Unzip/untar based on file extension
2. Apply patches using "patch" in alphabetic order

Also, when I apply patches myself, I use heuristics too. And my
heuristics are probably different from yours. So I'd expect that many
people using the exact same heuristic would only make the situation
better. Especially because if people don't know exactly what they are
doing, then their heuristics are probably not as good as the one of
our cfbot. I know I've struggled a lot the first few times when I was
manually applying patches.

---

From: Matthias van de Meent <[email-masked]>
Date: Thu, 15 Aug 2024 19:25:15 +0200

(sorry for the formatting, my mobile phone doesn't have the capabilities I
usually get when using my laptop)

> manually applying patches.

One serious issue with this is that in cases of apply failures, CFBot
delays, or other issues, the CFBot repo won't contain the latest version of
the series' patchsets. E.g. a hacker can  accidentally send an incremental
patch, or an unrelated patch to fix an issue mentioned in the thread
without splitting into a different thread, etc. This can easily cause users
(and CFBot) to test and review the wrong patch, esp. when the mail thread
proper is not looked by the reviewer, which would be somewhat promoted by a
CFA+github -centric workflow.

Apart from the above issue, I'm -0.5 on what to me equates with automated
spam to -hackers: the volume of mails would put this around the 16th most
common sender on -hackers, with about 400 mails/year (based on 80 new
patches for next CF, and 5 CFs/year, combined with Robert's 2023 statistics
at [0]).

I also don't quite like the suggested contents of such mail: (1) and (2)
are essentially duplicative information, and because CF's entries' IDs are
not shown in the app the "with ID 0000" part of (1) is practically useless
(better use the CFE's title), (3) would best be stored and/or integrated in
the CFA, as would (4). Additionally, (4) isn't canonical/guaranteed to be
up-to-date, see above. As for the "copy-pastable git commands" suggestion,
I'm not sure that's applicable, for the same reasons that (4) won't work
reliably. CFBot's repo to me seems more like an internal implementation
detail of CFBot than an authorative source of patchset diffs.

Maybe we could instead automate CF mail thread registration by allowing
registration of threadless CF entries (as 'preliminary'), and detecting
(and subsequently linking) new threads containing references to those CF
entries, with e.g. an  "CF: https://commitfest.postgresql.org/49/4980/"
directive in the new thread's initial mail's text. This would give the
benefits of requiring no second mail for CF referencing purposes, be it
automated or manual.
Alternatively, we could allow threads for new entries to be started through
the CF app (which would automatically insert the right form data into the
mail), providing an alternative avenue to registering patches that doesn't
have the chicken-and-egg problem you're trying to address here.

Kind regards,

Matthias van de Meent
Neon (https://neon.tech)

[0] https://rhaas.blogspot.com/2024/01/who-contributed-to-postgresql.html

---

From: Peter Eisentraut <[email-masked]>
Date: Fri, 16 Aug 2024 09:12:50 +0200

On 15.08.24 19:25, Matthias van de Meent wrote:
> Apart from the above issue, I'm -0.5 on what to me equates with 
> automated spam to -hackers: the volume of mails would put this around 
> the 16th most common sender on -hackers, with about 400 mails/year 
> (based on 80 new patches for next CF, and 5 CFs/year, combined with 
> Robert's 2023 statistics at [0]).

Yeah, I'd rather not open the can of worms that we send automated emails 
to this list at all.  If we do this, then there will be other requests, 
and why this one and not that one.  If people want to get emails from 
the commitfest app, it should be that you subscribe there and it sends 
those emails to those who want them.

> are essentially duplicative information, and because CF's entries' IDs 
> are not shown in the app the "with ID 0000" part of (1) is practically 
> useless (better use the CFE's title), (3) would best be stored and/or 
> integrated in the CFA, as would (4). Additionally, (4) isn't 
> canonical/guaranteed to be up-to-date, see above. As for the 
> "copy-pastable git commands" suggestion, I'm not sure that's applicable, 
> for the same reasons that (4) won't work reliably. CFBot's repo to me 
> seems more like an internal implementation detail of CFBot than an 
> authorative source of patchset diffs.

I agree.  And this also smells a bit like "my favorite workflow".  Maybe 
start with a blog post or a wiki page if you want to suggest this.

---

From: Tom Lane <[email-masked]>
Date: Fri, 16 Aug 2024 10:43:15 -0400

> those emails to those who want them.

That would destroy the one good argument for this, which was to
provide an easy way to get from a mail list thread (in the archives)
to the corresponding CF entry or entries.  You pull up the "flat"
thread, search for the bot mail, and click the link.  Without that
I see little point at all.

However, there are other ways to accomplish that.  I liked the
suggestion of extending the CF webapp with a way to search for entries
mentioning a particular mail message ID.  I dunno how hard it'd be to
get it to recognize *any* message-ID from a thread, but surely at
least the head message ought not be too hard to match.

			regards, tom lane

---

From: Peter Geoghegan <[email-masked]>
Date: Fri, 16 Aug 2024 14:12:49 -0400

> are not canonical.  There are various heuristics when they get applied.

It's true that the code required for CFBot to simply apply a patch is
nontrivial. We're accounting for various edge-cases, and soldiering
on, where we deem that it makes sense. I'm referring to those cases
where "git am" won't work perfectly, but we have a pretty good chance
of successfully generating what the patch author intended (I assume
that that's what you meant by "heuristics").

One reason why this works better than might be expected is
because...we then test the result (that's the whole point, of course).
Obviously, if we apply a patch in a way that isn't quite perfectly
clean (according to whatever the "git am" criteria is), but that CFBot
is nevertheless capable of applying, and we then find that the end
result passes all tests, that gives us a fairly strong signal. We can
have high confidence that CFBot has done the right thing at that
point. We can reasonably present the resulting feature branch as a
"known good" usable version of the patch.

Of course you can quibble with this. Fundamentally, a patch file can
never be updated, but we want to apply it on top of a moving target
(as best we can). We're always making some kind of trade-off. I just
don't think that the heuristics that humans might choose to apply are
necessarily much better on average. Humans are bad at routine boring
things, but good at noticing and coping with special cases.

> create parallel worlds of patches that are like 90% similar but not

There's a reason why tech companies place so much emphasis on offering
a "low friction experience". The aggregate effect on user/consumer
behavior is huge. I'm not saying that this is good or bad (let's not
get into that now); just that it is an empirical fact that people tend
to behave like that. We want more reviewers. Why not try to meet
people where they are a bit more?

I have to admit that I think that I'd be far more likely to quickly
test out a patch if I'm presented with a workflow that makes the setup
as painless as possible. Particularly if I'm all but guaranteed to get
a working Postgres server with the patch applied (which is possible
when I'm building exactly the same feature branch as the one that
passed CI by CFBot). I'm entirely willing to believe that it wouldn't
work that way for you, but I'm confident that a lot of people are in
the same camp as me.

-- 
Peter Geoghegan

---

From: Robert Haas <[email-masked]>
Date: Mon, 19 Aug 2024 09:10:12 -0400

On Fri, Aug 16, 2024 at 3:13 AM Peter Eisentraut <[email-masked]> wrote:
> to this list at all.

+1.

> If people want to get emails from

+1.

-- 
Robert Haas
EDB: http://www.enterprisedb.com

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Mon, 19 Aug 2024 15:31:17 +0200

> least the head message ought not be too hard to match.

I sent a patch to support this on the commitfest app to Magnus
off-list. It was pretty easy to implement, even for *any* message-ID.

---

From: Tom Lane <[email-masked]>
Date: Mon, 19 Aug 2024 10:16:08 -0400

> off-list. It was pretty easy to implement, even for *any* message-ID.

Cool, thank you!

			regards, tom lane

---

From: David Rowley <[email-masked]>
Date: Thu, 12 Sep 2024 14:07:37 +1200

> pushed to the next commitfest, **no** new email would be sent.

I was thinking about this today when looking at [1], you can see at
the end of that email someone posted a link to the CF entry.
Unfortunately, that was for the Jan-2024 CF, which is not really that
useful to look at today.

It looks like only about 35% of patches in this CF have "num CFs" = 1,
so it might be annoying to be taken to an old entry 65% of the time
you click the proposed URL. Maybe instead of the URLs showing the CF
number that the patch was first added to, if it could have a reference
in the URL that looks up the maximum CF which contains the patch and
show that one. e.g. https://commitfest.postgresql.org/latest/4751/ .
Without that, if you try to modify the status of a patch in an older
CF, you'll just be told that you can't do that.  (it is true you can
click the latest CF in the status column, but that's at least one more
click than I'd have hoped)

David

[1] https://postgr.es/m/[email-masked]

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Thu, 12 Sep 2024 10:44:04 +0200

On Thu, 12 Sept 2024 at 04:07, David Rowley <[email-masked]> wrote:
> Maybe instead of the URLs showing the CF

Yeah agreed, that's why I added support for
https://commitfest.postgresql.org/patch/4751/ a while ago. That URL
will be easily discoverable on the commitfest entry page soon (patch
for this is in flight).

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Tue, 24 Sep 2024 23:54:42 -0700

> least the head message ought not be too hard to match.

This is now deployed, so you can now find a CF entry based on the email ID.

A bunch of other improvements also got deployed:
- Improved homepage[1] (now with useful and bookmarkable links at the top)
- More links on the cf entry page[2] (cfbot results, github diff, and
stable link to entry itself)
- Instructions on how to checkout an cfbot entry

CFBot traffic lights directly on the cfentry and probably the
commifest list page are the next thing I'm planning to work on

After that I'll take a look at sending opt-in emails

Another thing that I'm interested in adding is some metric of patch
size, so it's easier to find small patches that are thus hopefully
"easy" to review. To accommodate multi-patch emails, I'm thinking of
showing lines changed in the first patch and lines changed in all
patches together. Possibly showing it clearly, if significantly more
lines were deleted than added, so it's easy to spot effective
refactorings.

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Wed, 25 Sep 2024 00:10:53 -0700

> - More links on the cf entry page[2] (cfbot results, github diff, and

[1]: https://commitfest.postgresql.org/
[2]: https://commitfest.postgresql.org/patch/5070

---

From: Robert Haas <[email-masked]>
Date: Thu, 26 Sep 2024 11:05:52 -0400

> lines were deleted than added, so it's easy to spot effective

I like this general idea. Anything that helps us figure out what to
pay attention to in the CommitFest is great stuff. Focusing on the
first patch seems odd to me, though: often the earlier patches will be
preparatory patches, so small, and the big patch is someplace near the
end of the series.

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Thu, 26 Sep 2024 11:57:21 -0700

On Thu, 26 Sept 2024 at 08:06, Robert Haas <[email-masked]> wrote:
> Focusing on the first patch seems odd to me, though

Indeed the first few patches will often be small, and the big patch
will appear later. When I split patches up, those small patches should
usually be reviewable without looking at the big patch in detail, and
hopefully they shouldn't be too contentious: e.g. a comment
improvement or some small refactor. But often those patches don't seem
to be reviewed significantly quicker or merged significantly earlier
than the big patch. That makes it seem to me that even though they
should be relatively low-risk to commit and low-effort to review,
reviewers are scared away by the sheer number of patches in the
patchset, or by the size of the final patch. That's why I thought it
could be useful to specifically show the size of the first patch in
addition to the total patchset size, so that reviewers can easily spot
some small hopefully easy to review patch at the start of a patchset.

---

From: Robert Haas <[email-masked]>
Date: Fri, 27 Sep 2024 08:08:31 -0400

> some small hopefully easy to review patch at the start of a patchset.

Fair enough! Personally what I'd want to know is how large the biggest
patch is, but I see your point, too.