Subject: Commitfest app release on Feb 17 with many improvements

From: Jelte Fennema-Nio <[email-masked]>
Date: Fri, 31 Jan 2025 15:23:56 +0100

At the FOSDEM dev meeting we discussed potential improvements to the
commitfest app and how to handle deploying future changes with minimal
disruption to existing workflows. We're going to try a new approach:
announcing a new commitfest release ~2 weeks in advance, including
release notes. That way people can try out the changes on the staging
environment and raise concerns/objections. So hereby such an
announcement:

The next commitfest app release will take place on February 17 and
will contain the following user facing changes:

1. Showing CFBot status on patch and commitfest pages
2. Showing a link to the CFBot github diff on patch and commitfest pages
3. Showing total additions/deletions of the most recent patchset on
patch and commitfest pages
4. Showing additions/deletions of the first patch of the most recent
patchset on the patch page
5. Showing the version and number of patches of the most recent
patchset on the patch page
6. Canonical links for the patch page are now /patch/{patchid} instead
of /{cfid}/{patchid}
7. If you subscribed for emails or patches where you're an author, you
will now get an email when your patch needs a rebase.
8. Clicking headers will toggle sorting, instead of always enabling it
9. Allow sorting patches by name
10. Allow sorting patches by total additions/deletions (added together
for total lines changed)
11. Add ID column to commitfest page (allows sorting)
12. Allow sorting using the header of closed patches too
13. Fix dropdown direction for dropdowns at the bottom of the page
(they now drop up)
14. The previous GiHub and CirrusCI links are now integrated into the
new CFBot status row of the patch page
15. The previous GitHub checkout instructions are now integrated into
the new CFBot status row as a button that copies the commands to the
clipboard

You can try out these changes on the staging environment[1] with
pgtest:pgtest as user & password for the http auth. If you want to
make edits you need to create a new account, read-only doesn't need an
account.

[1]: https://commitfest-test.postgresql.org/

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Sat, 1 Feb 2025 12:03:24 +0100

On Fri, 31 Jan 2025 at 15:23, Jelte Fennema-Nio <[email-masked]> wrote:
> You can try out these changes on the staging environment

It seems that someone tried it out, and found a 500 error when
searching by Message-ID. That's fixed now.

I also fixed some broken links to CirrusCI on the patch page.

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 3 Feb 2025 14:12:47 -0600

> environment and raise concerns/objections. So hereby such an

Thanks for doing this!

> the new CFBot status row as a button that copies the commands to the

I'm not sure #4 will be very helpful, but the rest looks reasonable to me.

-- 
nathan

---

From: Álvaro Herrera <[email-masked]>
Date: Tue, 4 Feb 2025 17:27:18 +0100

> patchset on the patch page

I think this has potential for excessive clutter.  I propose that, if
we're going to have a column with addition/deletions, we have two
things:

1. the total number of addition and deletions in the whole patch
   series, for the last patchset version posted.  This would be shown in
   the column.
2. a per-patch breakdown of the number of addition and deletions for the
   last patchset version posted, one line per patch.  This would appear
   in a tooltip on the column for the totals, to avoid interface
   clutter.

> 10. Allow sorting patches by total additions/deletions (added together

Seems reasonable, though I don't see much of an use-case for this.

-- 
Álvaro Herrera         PostgreSQL Developer  —  https://www.EnterpriseDB.com/
"Postgres is bloatware by design: it was built to house
 PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002)

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Tue, 4 Feb 2025 23:52:00 +0100

>    series, for the last patchset version posted.  This would be shown in

To be clear: the only stats column currently in the staging
environment shows exactly this. Changes 4 and 5 are *not* visible in
columns on the commitfest page, that info is currently only visible on
the patch details page. (For context for others: during the FOSDEM
meeting 4 was a column on the commitfest page, but I reverted that
after the feedback there)

So, I guess clutter wise the current staging is acceptable to you.

>    in a tooltip on the column for the totals, to avoid interface

Statistics for each patch would definitely be nice. Not trivial to add
though, because it would need cfbot to get those statistics for each
patch. I created a github issue for this idea to track it[1]

> Seems reasonable, though I don't see much of an use-case for this.

My main idea behind this is to allow people to easily find some small
patchsets, for when they have ~30 minutes to review something. Or for
new contributors to find something to review without getting
overwhelmed.

[1]: https://github.com/postgres/pgcommitfest/issues/24

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Mon, 17 Feb 2025 10:02:51 +0100

> will contain the following user facing changes:

In my part of the world it's February 17th now. So this is now deployed.

A big recommendation, check the "Notify on all where author" checkbox
on your profile page[1]. That way you'll get notified when one of your
patches needs a rebase.

The cfbot statuses will get filled in over the next few (12?) hours.
The first few are already available[2]. If you notice any problems,
please let me know.

Since the last message on this thread I also fixed the "claim as
committer" button, which was broken due to the new patch URLs.

> 13. Fix dropdown direction for dropdowns at the bottom of the page

I didn't mention it in my previous email, but this was fixed by Jacob
Brazeal btw. He also added some initial dummy data to the repo to make
it much easier for new developers to get started. Thanks Jacob!

[1]: https://commitfest.postgresql.org/account/profile/
[2]: https://commitfest.postgresql.org/52/?text=&status=-1&targetversion=-1&author=-1&reviewer=-1&sortkey=6

---

From: Thomas Munro <[email-masked]>
Date: Tue, 18 Feb 2025 10:33:12 +1300

> The first few are already available[2]. If you notice any problems,

This looks fantastic.  Thanks so much for working on it!  And all the
other usability improvements too.

---

From: Peter Geoghegan <[email-masked]>
Date: Mon, 17 Feb 2025 18:38:07 -0500

> other usability improvements too.

+1. I very much welcome a more formal release process for the CF app,
though with shorter feedback cycles.

On that note: It'd be nice if there was a way to see a list of open
patches that I am associated with in any way, either as an author or
as a reviewer (or as a committer). The list of patches would ideally
be organized into two groups: those in the open CF, and those in the
next CF.

I like "Useful links that you can use and bookmark", but I'd prefer if
there was only one link that I needed to bookmark. That is, I'd like
it if "Your entries in the current commitfest", "Your entries in the
open commitfest", and "Entries that you are reviewing in current
commitfest" were all merged into one dashboard view (that also
included "Entries that you are reviewing in the open commitfest").
Essentially, "patches that you're involved in in any capacity that
haven't already been closed out", organized along the same lines as
the existing links (every patch fitting into 1 or those 4 categories),
and presented to me as a single page of patches/items.

I'm not arguing that you should get rid of "Your entries in the
current commitfest", "Your entries in the open commitfest", and
"Entries that you are reviewing in current commitfest". Though I will
say that if you did get rid of them, I wouldn't object (I'd expect to
see the same structure when I viewed the new dashboard view, which
would be all I'd ever want to use).

Thanks
--
Peter Geoghegan

---

From: Michael Paquier <[email-masked]>
Date: Tue, 18 Feb 2025 08:46:17 +0900

> though with shorter feedback cycles.

Interesting evolution of the CF app.  Thanks for all that.
--
Michael

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 18 Feb 2025 16:15:11 -0500

> Seems reasonable, though I don't see much of an use-case for this.

Looking at the live version, I can sort the "Stats" column from smallest
to largest, but not from largest to smallest.  Is this intended?

-- 
  Bruce Momjian  <[email-masked]>        https://momjian.us
  EDB                                      https://enterprisedb.com

  Do not let urgent matters crowd out time for investment in the future.

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Tue, 18 Feb 2025 23:39:54 +0100

> to largest, but not from largest to smallest.  Is this intended?

The next release to prod in ~2 weeks will have that as one of the new
features (courtesy of Akshat).

You can try it out on the staging website[1] already, with pgtest as
user and password for the HTTP auth.

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Tue, 18 Feb 2025 23:57:36 +0100

On Tue, 18 Feb 2025 at 00:38, Peter Geoghegan <[email-masked]> wrote:
> +1.

Thanks all, I'm also very happy that it's deployed.

> I very much welcome a more formal release process for the CF app,

Do you mean here that you'd like less than two weeks between the
announced staging deploy and prod deploy?

> commitfest" were all merged into one dashboard view

I think that indeed would be very useful, but also not a small task to
do well. The links were pretty easy to implement, so I went for that
first.

Overall from what I've heard after talking to people there are a few
main use cases for the commitfest app.

1. As a committer/reviewer/submitter, keep track of all the patches
that you are interested in
2. As a reviewer, find new patches (and from the opposite side, have
reviewers find your submitted patches)
3. As a submitter, trigger CFBot runs
4. Get details on a specific patch (does it pass CI, look at a diff in
a web view, check out the change locally).
5. If there's other usecases please let me know.

I don't use it much for 1 myself. I've been mostly using my
"Interesting" label in my email client for tracking interesting
threads. So instead I focussed mostly on improving 4, and a bit of 2
for now. Now that there's some more useful patch info available for
each patch, I probably also want to start keeping track of patches
that I'm interested in, on the cf app.

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Wed, 19 Feb 2025 00:01:40 +0100

> do well. The links were pretty easy to implement, so I went for that

I created an issue to not forget about that:
https://github.com/postgres/pgcommitfest/issues/36

---

From: Bruce Momjian <[email-masked]>
Date: Tue, 18 Feb 2025 19:25:07 -0500

> [1]: https://commitfest-test.postgresql.org/

Yes, works perfectly in test, thanks.

---

From: Thomas Munro <[email-masked]>
Date: Thu, 20 Feb 2025 17:16:01 +1300

> Thanks all, I'm also very happy that it's deployed.

Now that we have CI information in an interactive webapp, do you think
it would make sense to offer ways to control it a bit?

[ ] Enable optional NetBSD task
[ ] Enable optional OpenBSD task
[ ] Enable optional Windows + MinGW task
[ ] Request fresh CI run

Until we have the new proper API endpoint up and running, perhaps we
could include hidden comments on the page the cfbot scrapes to
synchronise its submission list...

The last one would affect cfbot's scheduling choices, ie do this one
next please, and could be cleared as soon as a new CI result arrives.
I suppose the other ones could cause cfbot to add "ci-os-also:"
annotations to its commit message, like the existing "ci-os-only:",
except not exclusive.  You probably know if a patch has portability
implications and want to turn those on, but I assume they are still
too slow and expensive to enable by default.

---

From: Tatsuo Ishii <[email-masked]>
Date: Thu, 20 Feb 2025 18:53:29 +0900 (JST)

> [1]: https://commitfest-test.postgresql.org/

I noticed some of entries are shown with the author field being empty.
e.g. https://commitfest.postgresql.org/patch/5525/

Is this normal?
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

---

From: Thomas Munro <[email-masked]>
Date: Thu, 20 Feb 2025 23:16:35 +1300

> e.g. https://commitfest.postgresql.org/patch/5525/

When the layout of https://commitfest.postgresql.org/52/ changed,
cfbot's web scraping logic could no longer find the authors :-(.
That's a stupid problem to have, that we are going to solve with a new
machine-friendly API, but let me try to fix it quickly...

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Thu, 20 Feb 2025 11:53:12 +0100

> machine-friendly API, but let me try to fix it quickly...

I believe Tatsuo was talking about the author field on the commitfest
app, not the cfbot.

Assuming I'm correct there, I think for the link you shared simply no
Author was added when the user created the entry. In the next
commitfest release (March 4th), that should not happen anymor (or at
least not by accident) because then it will start to autofill yourself
as author when you submit a patch.

---

From: Thomas Munro <[email-masked]>
Date: Fri, 21 Feb 2025 00:22:56 +1300

> I believe Tatsuo was talking about the author field on the commitfest

Oh.  Well the author was also missing from the commit messages, and
that's now fixed.

---

From: vignesh C <[email-masked]>
Date: Thu, 6 Mar 2025 21:05:54 +0530

> 1. Showing CFBot status on patch and commitfest pages

I noticed that the "Truncate logs by max_log_size" commitfest entry
says it requires a rebase in commitfest at [1].
CFBot says the following error at [2]:
++<<<<<<< ours
 +     <varlistentry id="guc-md5-password-warnings"
xreflabel="md5_password_warnings">
 +      <term><varname>md5_password_warnings</varname> (<type>boolean</type>)
 +      <indexterm>
 +       <primary><varname>md5_password_warnings</varname>
configuration parameter</primary>
++=======
+      <varlistentry id="guc-max-log-size" xreflabel="max_log_size">
+       <term><varname>max_log_size</varname> (<type>integer</type>)
+       <indexterm>
+        <primary><varname>max_log_size</varname> configuration
parameter</primary>
++>>>>>>> theirs

But it applies neatly for me and Jim also at [3]. Any idea why patch
apply fails with CFbot whereas it passes in our environment?

[1] - https://commitfest.postgresql.org/patch/5272/
[2] - http://cfbot.cputube.org/patch_5272.log
[3] - https://www.postgresql.org/message-id/4c3a4c2f-fa6b-4d56-8947-fba2c06066fd%40uni-muenster.de

Regards,
Vignesh

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Thu, 6 Mar 2025 17:00:52 +0100

> apply fails with CFbot whereas it passes in our environment?

Okay, the cause of this seems to be that the CFbot currently uses "git
apply --3way", not "git am --3way". For some reason "git apply" fails
to apply the patch while "git am" succeeds. I'll check this weekend if
I can change the logic to use "git am" instead.

---

From: Tom Lane <[email-masked]>
Date: Thu, 06 Mar 2025 12:10:33 -0500

> I can change the logic to use "git am" instead.

Please see if you can make it use patch(1).  IME git is too
stiff-necked about slightly stale patches no matter which
subcommand you use.

			regards, tom lane

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Thu, 6 Mar 2025 18:39:03 +0100

> stiff-necked about slightly stale patches no matter which

It was using patch(1) in the past for that reason, but with --3way I
was able to get "git apply" to apply patches that patch(1) was unable
to apply. Also with the git command you get much more useful conflict
output.

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Fri, 7 Mar 2025 00:48:36 +0100

> to apply. Also with the git command you get much more useful conflict

Okay, I went for the approach of just trying everything until one
works. Starting with "git am", then patch(1), and as a final attempt
"git apply". Patch 5272 applies correctly now. I've removed any
backoff caused by repeated failures for all existing patches, so all
should be retried in the next ~48 hours.

---

From: Tom Lane <[email-masked]>
Date: Thu, 06 Mar 2025 22:08:28 -0500

Jelte Fennema-Nio <[email-masked]> writes:
> "git apply".

+1, thanks!

			regards, tom lane

---

From: Andreas Karlsson <[email-masked]>
Date: Fri, 7 Mar 2025 11:26:29 +0100

> should be retried in the next ~48 hours.

Out of curiosity: do you track which method works? I would expect 
everything to be applied with either git am or patch which can be 
applied with git apply making git apply technically unnecessary.

Andreas

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Fri, 7 Mar 2025 11:49:23 +0100

> applied with git apply making git apply technically unnecessary.

I think we need all of them...
- git apply is unable to apply the patch in 5272 on top of main
- git am, works with that 5272, but cannot apply any patch not created
by git format-patch (for instance 5522)
- I've been unable to get patch(1) not to return a failure if the
patch is already applied. So applying e.g. 5522 twice. git apply
handles that fine.

---

From: Tom Lane <[email-masked]>
Date: Fri, 07 Mar 2025 11:42:35 -0500

> patch is already applied. So applying e.g. 5522 twice. git apply

Hm, don't you *want* a failure if the patch is already applied?

			regards, tom lane

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Fri, 7 Mar 2025 18:52:38 +0100

> Hm, don't you *want* a failure if the patch is already applied?

It's pretty common that in a larger patchset the first 1-2
small/trivial patches get committed before the rest. Having to then
send an additional email, resubmitting the rest of the patchset
because CFBot fails when re-applying those is kind of silly. This
avoids that.

---

From: Tom Lane <[email-masked]>
Date: Fri, 07 Mar 2025 12:57:25 -0500

> because CFBot fails when re-applying those is kind of silly. This

Got it.  That is indeed an improvement, though of course it only
helps if those patches were applied verbatim.

			regards, tom lane

---

From: vignesh C <[email-masked]>
Date: Sun, 9 Mar 2025 07:50:54 +0530

> 1. Showing CFBot status on patch and commitfest pages

Couple of suggestions: a) No need to show CI status as "Needs rebase,"
"Not processed," etc., for committed patches. b) Can we add a filter
for "Needs rebase!"? This would help the CommitFest manager easily
list patches that need updating.

Regards,
Vignesh

---

From: Jelte Fennema-Nio <[email-masked]>
Date: Sun, 9 Mar 2025 14:35:34 +0100

On Sun, 9 Mar 2025 at 03:21, vignesh C <[email-masked]> wrote:
> "Not processed," etc., for committed patches.

Do you mean specifically for committed ones? Or just for any patch
with a "closed" status.

> b) Can we add a filter

That should be pretty easy to implement. But is that really what we
want? In the next release, sorting by "failing since" is implemented.
It sounds like that could be enough instead. i.e. do we really only
want to call out patches that need a rebase? Or also ones that have
been failing in CI for a long time?

I'm even wondering if this whole flow still makes sense. Do we really
want to send an email to  the mailing list about this? And if so, why
is someone doing that manually? If people subscribe to updates for
patches that they authored, then they get these "needs rebase"
automatically. Should we maybe simply default that option to true? And
for instance send a notification automatically to all people with a
"needs rebase" CI status whenever we start a new commitfest.

---

From: vignesh C <[email-masked]>
Date: Tue, 11 Mar 2025 09:17:16 +0530

> Do you mean specifically for committed ones? Or just for any patch

It is for any closed status patches.

> "needs rebase" CI status whenever we start a new commitfest.

It will be good if you can send a notification automatically to the
patch requiring a rebase when the CFBot first identifies this(not only
during the start of commitfest) and probably send this notification
once in a day for 3 or so days and then change the status to "waiting
on author" if a new rebased version is not posted in this time.

Regards,
Vignesh