Thread: 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.
Re: Proposal: Make cfbot fail on patches not created by "git format-patch"
From
Daniel Gustafsson
Date:
> 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
Re: Proposal: Make cfbot fail on patches not created by "git format-patch"
From
Jacob Champion
Date:
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
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
Re: Proposal: Make cfbot fail on patches not created by "git format-patch"
From
Jacob Champion
Date:
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
Re: Proposal: Make cfbot fail on patches not created by "git format-patch"
From
Florents Tselai
Date:
> 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”