Horton
Menu

Thread

  1. Proposal: Make cfbot fail on patches not created by "git format-patch"

    Jelte Fennema-Nio <me@jeltef.nl> — 2025-05-16T15:52:53Z

    In the "Scaling PostgreSQL Development" unconference session. One of the
problems that came up was that people don't follow "best practices". The
response to that was that people don't know what the best practices are
(nor that they are important to follow), because we don't enforce them.
Based on the discussion there I'm planning to make the cfbot fail to apply
a patch in the following two cases:

1. If a patch is not created by "git format-patch" (but cfbot will still
use "patch" to apply the patch in case "git am" fails)
2. If the commit message has no body (so only a title)

Does anyone have strong opposition to this? To be clear, it means we don't
run CI on patches created by  piping "git diff" to a file anymore, as a way
to nudge submitters into providing useful commit messages.

Communication wise, I plan to show this in the CF app as "Fails apply"
(instead of "Needs Rebase"). When clicking the "Fails apply" link, it would
then show a log as to why the apply failed. need to be created using git
format patch, and should have a descriptive commit message.

  2. Re: Proposal: Make cfbot fail on patches not created by "git format-patch"

    Daniel Gustafsson <daniel@yesql.se> — 2025-05-16T16:05:38Z

    > On 16 May 2025, at 11:52, Jelte Fennema-Nio <me@jeltef.nl> wrote:

> Does anyone have strong opposition to this? To be clear, it means we don't run CI on patches created by  piping "git diff" to a file anymore, as a way to nudge submitters into providing useful commit messages.

Disclaimer: I wasn't in the session (due to conflicting interesting sessions)
so I might be raising points/questions already answered.

Is this really lowering the bar for new contributors?  I've always held "be
liberal in what you accept" as a gold standard for projects I'm involved in, to
remove barriers to entry.  Good commit messages are obviously very important,
but having your patch rejected (yes, I know, failing to apply) might not be
strongest motivator for achieving this.

--
Daniel Gustafsson

  3. Re: Proposal: Make cfbot fail on patches not created by "git format-patch"

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-16T16:12:47Z

    Jelte Fennema-Nio <me@jeltef.nl> writes:
> Based on the discussion there I'm planning to make the cfbot fail to apply
> a patch in the following two cases:
> ...
> To be clear, it means we don't
> run CI on patches created by  piping "git diff" to a file anymore, as a way
> to nudge submitters into providing useful commit messages.

That outcome seems entirely horrible to me.  If you want to flag the lack
of a commit message somehow, fine, but don't prevent CI from running.

			regards, tom lane

  4. Re: Proposal: Make cfbot fail on patches not created by "git format-patch"

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-05-16T16:24:10Z

    On Fri, May 16, 2025 at 12:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> That outcome seems entirely horrible to me.  If you want to flag the lack
> of a commit message somehow, fine, but don't prevent CI from running.

Personally I also prefer nudges to gates. Just like people already
deprioritize "Waiting on Author" entries a bit, having an obvious
"Patch Needs Work" note might gently help newcomers iterate on their
first submissions (or even communicate where a patch is in the
lifecycle! e.g. a Bugfix entry where the patch is marked incomplete
might motivate someone to jump in to fix it).

--Jacob

  5. Re: Proposal: Make cfbot fail on patches not created by "git format-patch"

    Jelte Fennema-Nio <me@jeltef.nl> — 2025-05-16T17:45:01Z

    On Fri, 16 May 2025 at 12:24, Jacob Champion
<jacob.champion@enterprisedb.com> wrote:
>
> On Fri, May 16, 2025 at 12:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > That outcome seems entirely horrible to me.  If you want to flag the lack
> > of a commit message somehow, fine, but don't prevent CI from running.
>
> Personally I also prefer nudges to gates.

Okay, reasonable feedback. How about we add a CI job that does a
"quality check". That's much less strong, as all the other tests will
still run, but people would get a failing CI job which tells them that
something is wrong. We could also include a pgindent in that CI job.

  6. Re: Proposal: Make cfbot fail on patches not created by "git format-patch"

    Jelte Fennema-Nio <me@jeltef.nl> — 2025-05-16T17:55:08Z

    On Fri, 16 May 2025 at 12:05, Daniel Gustafsson <daniel@yesql.se> wrote:
>
> > On 16 May 2025, at 11:52, Jelte Fennema-Nio <me@jeltef.nl> wrote:
>
> > Does anyone have strong opposition to this? To be clear, it means we don't run CI on patches created by  piping "git diff" to a file anymore, as a way to nudge submitters into providing useful commit messages.
>
> Disclaimer: I wasn't in the session (due to conflicting interesting sessions)
> so I might be raising points/questions already answered.
>
> Is this really lowering the bar for new contributors?  I've always held "be
> liberal in what you accept" as a gold standard for projects I'm involved in, to
> remove barriers to entry.  Good commit messages are obviously very important,
> but having your patch rejected (yes, I know, failing to apply) might not be
> strongest motivator for achieving this.

Lowering the bar for new contributors wasn't the purpose of this
change in policy. It's meant to reduce the work that committers and
reviewers have to do, which then in turn would result in quicker
reviews/commits. In my experience with other open source projects new
contributors are usually fine with adhering to project standards, if
they are told what those standards are. e.g. these days basically
every popular open source project is running a CI job that fails if
the auto-formatter fails.

  7. Re: Proposal: Make cfbot fail on patches not created by "git format-patch"

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-05-16T17:55:29Z

    Jelte Fennema-Nio <me@jeltef.nl> writes:
> Okay, reasonable feedback. How about we add a CI job that does a
> "quality check". That's much less strong, as all the other tests will
> still run, but people would get a failing CI job which tells them that
> something is wrong. We could also include a pgindent in that CI job.

WFM

			regards, tom lane

  8. Re: Proposal: Make cfbot fail on patches not created by "git format-patch"

    Aleksander Alekseev <aleksander@timescale.com> — 2025-05-19T13:22:43Z

    Hi,

> > Is this really lowering the bar for new contributors?  I've always held "be
> > liberal in what you accept" as a gold standard for projects I'm involved in, to
> > remove barriers to entry.  Good commit messages are obviously very important,
> > but having your patch rejected (yes, I know, failing to apply) might not be
> > strongest motivator for achieving this.
>
> Lowering the bar for new contributors wasn't the purpose of this
> change in policy. It's meant to reduce the work that committers and
> reviewers have to do, which then in turn would result in quicker
> reviews/commits. In my experience with other open source projects new
> contributors are usually fine with adhering to project standards, if
> they are told what those standards are. e.g. these days basically
> every popular open source project is running a CI job that fails if
> the auto-formatter fails.

I appreciate your desire to address named problems, but I don't think
the proposed steps will help much.

In my experience people who have been contributing for some time use
format-patch and provide at least a draft of the commit message,
because they know it's more convenient both for the reviewers (the
patch has better chances to be reviewed and tested), and for the
authors to rebase the patch after a while. Newcomers sometimes submit
patches that don't even target the `master` branch, and they don't
know we have cfbot.

-- 
Best regards,
Aleksander Alekseev

  9. Re: Proposal: Make cfbot fail on patches not created by "git format-patch"

    Jacob Champion <jacob.champion@enterprisedb.com> — 2025-05-19T15:10:19Z

    On Mon, May 19, 2025 at 6:23 AM Aleksander Alekseev
<aleksander@timescale.com> wrote:
> In my experience people who have been contributing for some time use
> format-patch and provide at least a draft of the commit message,
> because they know it's more convenient both for the reviewers (the
> patch has better chances to be reviewed and tested), and for the
> authors to rebase the patch after a while. Newcomers sometimes submit
> patches that don't even target the `master` branch, and they don't
> know we have cfbot.

While I don't necessarily disagree with these two endpoints, I also
think there are a number of contributors who occupy a spot somewhere
in between -- and there were _many_ people at the unconference session
who were interested in automatically communicating our community norms
in some way. I think that's enough motivation to try something like
Jelte's latest "quality check" proposal.

--Jacob

  10. Re: Proposal: Make cfbot fail on patches not created by "git format-patch"

    Florents Tselai <florents.tselai@gmail.com> — 2025-05-19T15:29:15Z

    
> On 19 May 2025, at 6:10 PM, Jacob Champion <jacob.champion@enterprisedb.com> wrote:
> 
> On Mon, May 19, 2025 at 6:23 AM Aleksander Alekseev
> <aleksander@timescale.com> wrote:
>> In my experience people who have been contributing for some time use
>> format-patch and provide at least a draft of the commit message,
>> because they know it's more convenient both for the reviewers (the
>> patch has better chances to be reviewed and tested), and for the
>> authors to rebase the patch after a while. Newcomers sometimes submit
>> patches that don't even target the `master` branch, and they don't
>> know we have cfbot.
> 
> While I don't necessarily disagree with these two endpoints, I also
> think there are a number of contributors who occupy a spot somewhere
> in between -- and there were _many_ people at the unconference session
> who were interested in automatically communicating our community norms
> in some way. I think that's enough motivation to try something like
> Jelte's latest "quality check" proposal.
> 
> —Jacob
> 
> 

What would help new comers I think is having some recipes to work with git the pg-hackers way:
Not many devs use format-patch and share files any more;
instead they `git checkout -b` and submitt a PR which is usually merged / squash merged.

Even “rebasing” is not as popular a term as one would hope. 

In fact, I think what would help is providing some potential “copy rebase command” tooltip for the “Needs rebase status”,
similar to the “copy git checkout commands”

  11. Re: Proposal: Make cfbot fail on patches not created by "git format-patch"

    Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> — 2025-05-30T04:18:52Z

    On Fri, May 16, 2025 at 1:45 PM Jelte Fennema-Nio <me@jeltef.nl> wrote:

> On Fri, 16 May 2025 at 12:24, Jacob Champion
> <jacob.champion@enterprisedb.com> wrote:
> >
> > On Fri, May 16, 2025 at 12:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > That outcome seems entirely horrible to me.  If you want to flag the
> lack
> > > of a commit message somehow, fine, but don't prevent CI from running.
> >
> > Personally I also prefer nudges to gates.
>
> Okay, reasonable feedback. How about we add a CI job that does a
> "quality check". That's much less strong, as all the other tests will
> still run, but people would get a failing CI job which tells them that
> something is wrong. We could also include a pgindent in that CI job.
>
>
> +1. Knowing whether to use git am or patch to apply the patch itself will
save reviewers' time.

-- 
Best Wishes,
Ashutosh Bapat

  12. Re: Proposal: Make cfbot fail on patches not created by "git format-patch"

    David G. Johnston <david.g.johnston@gmail.com> — 2025-05-30T04:38:30Z

    On Thursday, May 29, 2025, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:
>
> On Fri, May 16, 2025 at 1:45 PM Jelte Fennema-Nio <me@jeltef.nl> wrote:
>
>> On Fri, 16 May 2025 at 12:24, Jacob Champion
>> <jacob.champion@enterprisedb.com> wrote:
>> >
>> > On Fri, May 16, 2025 at 12:12 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> > > That outcome seems entirely horrible to me.  If you want to flag the
>> lack
>> > > of a commit message somehow, fine, but don't prevent CI from running.
>> >
>> > Personally I also prefer nudges to gates.
>>
>> Okay, reasonable feedback. How about we add a CI job that does a
>> "quality check". That's much less strong, as all the other tests will
>> still run, but people would get a failing CI job which tells them that
>> something is wrong. We could also include a pgindent in that CI job.
>>
>>
>> +1. Knowing whether to use git am or patch to apply the patch itself will
> save reviewers' time.
>

Just tossing this out there: we have a nice shell script that applies
patches in a directory to the checked out branch.  Why not place that
script into the postgres repo instead of having it in pgcommitfest?

That doesn’t preclude having the apply step of the process do more
work/checks/feedback without impacting “tests passed or failed”.  Does this
need to run on CirrusCI (personal builds) or just within the commitfest app?

David J.