Proposal: Make cfbot fail on patches not created by "git format-patch"
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.
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
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
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
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.
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.
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
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
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
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”
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
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.