Thread

Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-01-31T14:23:56Z

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/

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-02-01T11:03:24Z

On Fri, 31 Jan 2025 at 15:23, Jelte Fennema-Nio <postgres@jeltef.nl> 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.

Re: Commitfest app release on Feb 17 with many improvements

Nathan Bossart <nathandbossart@gmail.com> — 2025-02-03T20:12:47Z

On Fri, Jan 31, 2025 at 03:23:56PM +0100, Jelte Fennema-Nio wrote:
> 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:

Thanks for doing this!

> 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

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

-- 
nathan

Re: Commitfest app release on Feb 17 with many improvements

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-04T16:27:18Z

On 2025-Jan-31, Jelte Fennema-Nio wrote:

> 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

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
> for total lines changed)

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)

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-02-04T22:52:00Z

On Tue, 4 Feb 2025 at 17:27, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> 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.

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.

> 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.

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]

> > 10. Allow sorting patches by total additions/deletions (added together
> > for total lines changed)
>
> 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

Re: Commitfest app release on Feb 17 with many improvements

Alvaro Herrera <alvherre@alvh.no-ip.org> — 2025-02-05T08:46:30Z

On 2025-Feb-04, Jelte Fennema-Nio wrote:

> 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.

Check.

> On Tue, 4 Feb 2025 at 17:27, Álvaro Herrera <alvherre@alvh.no-ip.org> wrote:

> > 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.
> 
> 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]

Understood, thanks.

> > > 10. Allow sorting patches by total additions/deletions (added together
> > > for total lines changed)
> >
> > 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.

Ah, okay, sounds good.

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-02-17T09:02:51Z

On Fri, 31 Jan 2025 at 15:23, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> The next commitfest app release will take place on February 17 and
> 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
> (they now drop up)

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

Re: Commitfest app release on Feb 17 with many improvements

Thomas Munro <thomas.munro@gmail.com> — 2025-02-17T21:33:12Z

On Mon, Feb 17, 2025 at 10:03 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> 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.

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

Re: Commitfest app release on Feb 17 with many improvements

Peter Geoghegan <pg@bowt.ie> — 2025-02-17T23:38:07Z

On Mon, Feb 17, 2025 at 4:33 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> This looks fantastic.  Thanks so much for working on it!  And all the
> 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

Re: Commitfest app release on Feb 17 with many improvements

Michael Paquier <michael@paquier.xyz> — 2025-02-17T23:46:17Z

On Mon, Feb 17, 2025 at 06:38:07PM -0500, Peter Geoghegan wrote:
> +1. I very much welcome a more formal release process for the CF app,
> though with shorter feedback cycles.

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

Re: Commitfest app release on Feb 17 with many improvements

Bruce Momjian <bruce@momjian.us> — 2025-02-18T21:15:11Z

On Tue, Feb  4, 2025 at 05:27:18PM +0100, Álvaro Herrera wrote:
> On 2025-Jan-31, Jelte Fennema-Nio wrote:
> 
> > 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
> 
> 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
> > for total lines changed)
> 
> 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  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

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

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-02-18T22:39:54Z

On Tue, 18 Feb 2025 at 22:15, Bruce Momjian <bruce@momjian.us> wrote:
> Looking at the live version, I can sort the "Stats" column from smallest
> 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.

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

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-02-18T22:57:36Z

On Tue, 18 Feb 2025 at 00:38, Peter Geoghegan <pg@bowt.ie> wrote:
>
> On Mon, Feb 17, 2025 at 4:33 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > This looks fantastic.  Thanks so much for working on it!  And all the
> > other usability improvements too.
>
> +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,
> though with shorter feedback cycles.

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

> 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

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.

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-02-18T23:01:40Z

On Tue, 18 Feb 2025 at 23:57, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> > 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
>
> 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.

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

Re: Commitfest app release on Feb 17 with many improvements

Bruce Momjian <bruce@momjian.us> — 2025-02-19T00:25:07Z

On Tue, Feb 18, 2025 at 11:39:54PM +0100, Jelte Fennema-Nio wrote:
> On Tue, 18 Feb 2025 at 22:15, Bruce Momjian <bruce@momjian.us> wrote:
> > Looking at the live version, I can sort the "Stats" column from smallest
> > 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.
> 
> [1]: https://commitfest-test.postgresql.org/

Yes, works perfectly in test, thanks.

-- 
  Bruce Momjian  <bruce@momjian.us>        https://momjian.us
  EDB                                      https://enterprisedb.com

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

Re: Commitfest app release on Feb 17 with many improvements

Thomas Munro <thomas.munro@gmail.com> — 2025-02-20T04:16:01Z

On Wed, Feb 19, 2025 at 11:57 AM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> On Tue, 18 Feb 2025 at 00:38, Peter Geoghegan <pg@bowt.ie> wrote:
> > On Mon, Feb 17, 2025 at 4:33 PM Thomas Munro <thomas.munro@gmail.com> wrote:
> > > This looks fantastic.  Thanks so much for working on it!  And all the
> > > other usability improvements too.
> >
> > +1.
>
> 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.

Re: Commitfest app release on Feb 17 with many improvements

Tatsuo Ishii <ishii@postgresql.org> — 2025-02-20T09:53:29Z

> 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/

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

Re: Commitfest app release on Feb 17 with many improvements

Thomas Munro <thomas.munro@gmail.com> — 2025-02-20T10:16:35Z

On Thu, Feb 20, 2025 at 10:53 PM Tatsuo Ishii <ishii@postgresql.org> wrote:
> I noticed some of entries are shown with the author field being empty.
> 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...

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-02-20T10:53:12Z

On Thu, 20 Feb 2025 at 11:07, Thomas Munro <thomas.munro@gmail.com> wrote:
>
> On Thu, Feb 20, 2025 at 10:53 PM Tatsuo Ishii <ishii@postgresql.org> wrote:
> > I noticed some of entries are shown with the author field being empty.
> > 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...

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.

Re: Commitfest app release on Feb 17 with many improvements

Tatsuo Ishii <ishii@postgresql.org> — 2025-02-20T11:01:13Z

>> On Thu, Feb 20, 2025 at 10:53 PM Tatsuo Ishii <ishii@postgresql.org> wrote:
>> > I noticed some of entries are shown with the author field being empty.
>> > 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...
> 
> I believe Tatsuo was talking about the author field on the commitfest
> app, not the cfbot.

Yes, I was talking abot the commitfest app.

> 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.

Oh, I thought the field was automatically created but looks like it
was wrong assumption. Thanks for the explanation.

Best reagards,
--
Tatsuo Ishii
SRA OSS K.K.
English: http://www.sraoss.co.jp/index_en/
Japanese:http://www.sraoss.co.jp

Re: Commitfest app release on Feb 17 with many improvements

Thomas Munro <thomas.munro@gmail.com> — 2025-02-20T11:22:56Z

On Thu, Feb 20, 2025 at 11:53 PM Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> On Thu, 20 Feb 2025 at 11:07, Thomas Munro <thomas.munro@gmail.com> wrote:
> > 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...
>
> I believe Tatsuo was talking about the author field on the commitfest
> app, not the cfbot.

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

Re: Commitfest app release on Feb 17 with many improvements

vignesh C <vignesh21@gmail.com> — 2025-03-06T15:35:54Z

On Fri, 31 Jan 2025 at 19:54, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> 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

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

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-03-06T16:00:52Z

On Thu, 6 Mar 2025 at 16:36, vignesh C <vignesh21@gmail.com> wrote:
> 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?

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.

Re: Commitfest app release on Feb 17 with many improvements

Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-06T17:10:33Z

Jelte Fennema-Nio <postgres@jeltef.nl> writes:
> 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.

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

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-03-06T17:39:03Z

On Thu, 6 Mar 2025 at 18:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

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.

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-03-06T23:48:36Z

On Thu, 6 Mar 2025 at 18:39, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> On Thu, 6 Mar 2025 at 18:10, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > 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.
>
> 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.

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.

Re: Commitfest app release on Feb 17 with many improvements

Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-07T03:08:28Z

Jelte Fennema-Nio <postgres@jeltef.nl> writes:
> 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".

+1, thanks!

			regards, tom lane

Re: Commitfest app release on Feb 17 with many improvements

Andreas Karlsson <andreas@proxel.se> — 2025-03-07T10:26:29Z

On 3/7/25 12:48 AM, Jelte Fennema-Nio wrote:
> 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.

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

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-03-07T10:49:23Z

On Fri, 7 Mar 2025 at 11:26, Andreas Karlsson <andreas@proxel.se> wrote:
> 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.

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.

Re: Commitfest app release on Feb 17 with many improvements

Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-07T16:42:35Z

Jelte Fennema-Nio <postgres@jeltef.nl> writes:
> On Fri, 7 Mar 2025 at 11:26, Andreas Karlsson <andreas@proxel.se> wrote:
>> 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.

> 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.

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

			regards, tom lane

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-03-07T17:52:38Z

On Fri, 7 Mar 2025 at 17:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> 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.

Re: Commitfest app release on Feb 17 with many improvements

Tom Lane <tgl@sss.pgh.pa.us> — 2025-03-07T17:57:25Z

Jelte Fennema-Nio <postgres@jeltef.nl> writes:
> On Fri, 7 Mar 2025 at 17:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 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.

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

			regards, tom lane

Re: Commitfest app release on Feb 17 with many improvements

vignesh C <vignesh21@gmail.com> — 2025-03-09T02:20:54Z

On Fri, 31 Jan 2025 at 19:54, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> 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

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

Re: Commitfest app release on Feb 17 with many improvements

Jelte Fennema-Nio <postgres@jeltef.nl> — 2025-03-09T13:35:34Z

On Sun, 9 Mar 2025 at 03:21, vignesh C <vignesh21@gmail.com> wrote:
> Couple of suggestions: a) No need to show CI status as "Needs rebase,"
> "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
> for "Needs rebase!"? This would help the CommitFest manager easily
> list patches that need updating.

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.

Re: Commitfest app release on Feb 17 with many improvements

vignesh C <vignesh21@gmail.com> — 2025-03-11T03:47:16Z

On Sun, 9 Mar 2025 at 19:05, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>
> On Sun, 9 Mar 2025 at 03:21, vignesh C <vignesh21@gmail.com> wrote:
> > Couple of suggestions: a) No need to show CI status as "Needs rebase,"
> > "Not processed," etc., for committed patches.
>
> Do you mean specifically for committed ones? Or just for any patch
> with a "closed" status.

It is for any closed status patches.

> > b) Can we add a filter
> > for "Needs rebase!"? This would help the CommitFest manager easily
> > list patches that need updating.
>
> 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.

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