Commit messsages and pull request merge policy

GeoServer GeoServer Developer
1

Hi team,

I’ve been meaning to bring this up for a while but held off since I know we’re all juggling a lot right now. Please don’t take this as a request to add more time to PR reviews. Instead, I’m hoping that addressing this will actually make the process easier moving forward.

It seems there’s been an increasing trend of squash-merging or rebasing pull requests with multiple commits that don’t contain meaningful messages about the changes. Unfortunately, I couldn’t find the section where we had linked examples of strong commit messages, but I’d like to revisit this topic.

To provide context, here’s a recent example (not to single anyone out):

* commit 6218a5e95648383ac354c820569e30b4fcb2e0e9
| Author: Alessio Fabiani <alessio.fabiani@geosolutionsgroup.com>
| Date:   Mon Dec 9 12:07:22 2024 +0100
|
|     GEOS-11625: Add "Challenge Anonymous Sessions" Option to AuthKey Filter (#8066)
|
|     * AuthKey Filter checking for Anon session too
|
|     * AuthKey Filter Stateless Anonym Challenge option
|
|     * AuthKey Filter Stateless Anonym Challenge option
|
|     * Documentation
|
|     * Explicit imports
|
|     * QA checks
|
|     * QA checks
|
|     * Cleanup the Security Context in order to try making the Windows build tests more stable
|
|     * Workaround github windows build
|
|     * Update doc/en/user/source/extensions/authkey/index.rst
|
|     Co-authored-by: Andrea Aime <andrea.aime@gmail.com>

Many of these commit messages don’t clearly explain the changes, making it difficult to track the purpose of each update. Ideally, the commit history should be self-explanatory without needing to rely on JIRA or other external references, especially since our tooling might change in the future.

Here’s another example, this time with several commits:

* commit c026af0664b3473bcd81c8a23e029bcbec9f9e88
| Author: ROPARTZ Erwan DTSI/DSI <erwan.ropartz@orange.com>
| Date:   Mon Jan 27 14:23:40 2025 +0100
|
|     Revert LinkedList
|
* commit deb8d4442426f30e7d9c7cd46b3656d259b8fa7b
| Author: ROPARTZ Erwan DTSI/DSI <erwan.ropartz@orange.com>
| Date:   Fri Jan 24 19:02:14 2025 +0100
|
|     PMD
|
* commit 9e14ca30f3b6cffeb11e177354669ce877e7986d
| Author: ROPARTZ Erwan DTSI/DSI <erwan.ropartz@orange.com>
| Date:   Fri Jan 24 15:01:48 2025 +0100
|
|     Code review
|
* commit a279fd84806b94cd6b085731f75ad9354b1fa0ba
| Author: ROPARTZ Erwan DTSI/DSI <erwan.ropartz@orange.com>
| Date:   Tue Jan 21 12:23:57 2025 +0100
|
|     Add dedicated test to check insert order
|
* commit b6015478c395cbbc9a20c12ec68bf064b2a6c66d
| Author: ROPARTZ Erwan DTSI/DSI <erwan.ropartz@orange.com>
| Date:   Fri Jan 10 14:33:33 2025 +0100
|
|     Fix PMD
|
* commit cc559710c76378401746ad3019d9585cb6afcea0
| Author: ROPARTZ Erwan DTSI/DSI <erwan.ropartz@orange.com>
| Date:   Fri Jan 10 13:17:49 2025 +0100
|
|     Fix Syntax
|
* commit ec62cf708d781498581b9e0c00d25c636ae71c4a
| Author: ROPARTZ Erwan DTSI/DSI <erwan.ropartz@orange.com>
| Date:   Fri Jan 10 12:46:20 2025 +0100
|
|     Fix GEOS-11673: WFS-T] : Features order in transaction not respected

These kinds of entries make it hard to determine the intent behind each change. While we all occasionally create placeholder or intermediate commits (e.g., “tmp” commits), I believe we should prioritize finalizing clear and informative commit messages before merging.

My suggestion is that the developer requesting the PR review should be responsible for ensuring the final commit title and message are concise and descriptive. This would be similar to how we expect them to resolve merge conflicts.

I’d love to hear your thoughts on this—am I alone in this concern, or do you agree it’s worth addressing?

Cheers,

Gabe

2 Likes
2

Hi Gabriel,
interesting, mildly contentious, and with some practical application difficulties all at the same time… a few observations:

  • Having a full explanation in the commit message is a non-starter IMHO, we cannot ask committers to repeat the level of detail provided in the ticket. I believe we’ll eventually switch to another issue tracking, yes, but it’s not a problem, as long as we remember to include the GEOS-XYZW reference in the new issue title. E.g. we could have a Github issue titled “GEOS-11625: Add ‘Challenge Anonymous Sessions’ Option to AuthKey Filter” and the problem is gone, one can easily search by id to find the issue, with full details, screenshots, and eventual discussion.
  • I also dislike all the small/temporary commit messages that provide no useful information, but I’m also aware of the git rebase gymnastic I have to go through every time I make a pull request, and I’m not sure knowledge to keep a cleaner commit history is commonplace. For those PRs I normally end up squash-merging, especially when I already had to go back and forth 10 times with the contributor about other more critical stuff (code issues).
  • When squash merging and backporting, the backport is still composed of many small commits… that’s something that I would like to see solved but I have higher priorities in my spare time to do list (e.g. CITE tests). Anyone interested in having a look? Maybe the backport bot has some settings to avoid it, or code changes are needed.
  • The request to keep the history to a minimum is right there in the checklist BTW, the issue is that we’re not enforcing it… is there any bot that can control the final commit message for common misbehaviors and just prevent the merge? In the end, that’s the only way we’ll get it done systematically, asking the merger to pay more attention won’t get us very far.
  • Btw, I apologize for the GEOS-11673 example, I forgot to squash merge there.
3

By the way, we could have cleaner squash-merge messages by changing a configuration in github:

image
image488×332 24.5 KB

I’d go for “pull request title” (the description would contain the checklist, not informative IMHO).

Cheers
Andrea

4

Hi Andrea,

all good, we’re on the same page. And since you’re the one making the most work on pull request reviews,
I’d rather have us enforce the policy that a pull request is considered ready for review when it complies with the single-commit policy,
so the burden is on the committer, not the reviewer. The same way the committer is in charge of fixing merge conflicts
may his/her branch become out of date with main.
If it’s not common knowledge then it rather be learnt once than repeatedly forcing the merger to over work or concede.

I don’t know of good bots/hooks that could automate the check but we could look into it.

On Thu, Jan 30, 2025 at 5:58 AM Andrea Aime via OSGeo Discourse <noreply@discourse.osgeo.org> wrote:

aaime-geosolutions
January 30

Hi Gabriel,
interesting, mildly contentious, and with some practical application difficulties all at the same time… a few observations:

  • Having a full explanation in the commit message is a non-starter IMHO, we cannot ask committers to repeat the level of detail provided in the ticket. I believe we’ll eventually switch to another issue tracking, yes, but it’s not a problem, as long as we remember to include the GEOS-XYZW reference in the new issue title. E.g. we could have a Github issue titled “GEOS-11625: Add ‘Challenge Anonymous Sessions’ Option to AuthKey Filter” and the problem is gone, one can easily search by id to find the issue, with full details, screenshots, and eventual discussion.
  • I also dislike all the small/temporary commit messages that provide no useful information, but I’m also aware of the git rebase gymnastic I have to go through every time I make a pull request, and I’m not sure knowledge to keep a cleaner commit history is commonplace. For those PRs I normally end up squash-merging, especially when I already had to go back and forth 10 times with the contributor about other more critical stuff (code issues).
  • When squash merging and backporting, the backport is still composed of many small commits… that’s something that I would like to see solved but I have higher priorities in my spare time to do list (e.g. CITE tests). Anyone interested in having a look? Maybe the backport bot has some settings to avoid it, or code changes are needed.
  • The request to keep the history to a minimum is right there in the checklist BTW, the issue is that we’re not enforcing it… is there any bot that can control the final commit message for common misbehaviors and just prevent the merge? In the end, that’s the only way we’ll get it done systematically, asking the merger to pay more attention won’t get us very far.
  • Btw, I apologize for the GEOS-11673 example, I forgot to squash merge there.

Visit Topic or reply to this email to respond.

To unsubscribe from these emails, click here.

5

Hi Gabriel,
there are also cases of multiple commits that are valid according to policy:

although I’m sure we’ll start seeing “[GEOS-1234] Pmd fixes. Work already!!”
The bot could however give a more general error message, and remind committers of the policy, rather than being specific about the pattern it’s actually checking for.

I think there are many, here’s one that seems to be customizable via regexes (for Jody’s joy!):
https://github.com/GsActions/commit-message-checker

With a bot like that, we could be a bit more strict and create tickets also for the commits that are not user visible, marking them as “tasks”

6

I think the key thing that is helpful for me is [GEOS-1234] so I can find the associated ticket and mark it resolved.

Of course I would like to move to GitHub issues.

7

Is there a reason why we don’t merge with a merge commit?
Now we’re losing the commit messages. The merge of pr #8231 results in
only the title being applied:

commit 56fded1de7ff5257091802ad245bf268df54fad3 (HEAD -> main, upstream/main, upstream/HEAD)
Author: Gabriel Roldan <[gabriel.roldan@camptocamp.com](mailto:gabriel.roldan@camptocamp.com)>
Date: Tue Feb 4 11:36:51 2025 -0300

[GEOS-11696] AdminRequestCallback not loaded due to spring bean name conflict (#8231)

commit 6c42ab917046c29c73abf618ffa3533eddc2d04c
Author: Jody Garnett <[jody.garnett@gmail.com](mailto:jody.garnett@gmail.com)>
Date: Thu Jan 30 23:05:58 2025 -0800

[GEOS-11346] Collect CSP information and revise based on feedback

While it had two well crafted commit messages explaining the rationale of each one:

commit c358dd84dbbd543772162ebff21072d6fa3bb4fa (groldan/GEOS-11696_adminrequestcallback_bean_name_clash, GEOS-11696_adminrequestcallback_bean_name_clash)
Author: Gabriel Roldan <[gabriel.roldan@camptocamp.com](mailto:gabriel.roldan@camptocamp.com)>
Date: Fri Jan 31 13:12:15 2025 -0300

[GEOS-11696] fix org.geoserver.sldservice.rest.RasterizerTest

`org.geoserver.sldservice.rest.RasterizerTest` lost the logged in user
set up at by the base class `CatalogRESTTestSupport`.

`getAsServletResponse(uri)` requests weren't failing per-se, as they
returned 200, but the layer wasn't found in `RasterizerController`.

`getAsDOM()` requests failed at parsing the response, but the underlying
reason was the same, the layer is not found since `AdminRequest` is set
but the user was `null`.

This patch removes the unnecessary override and refresh of the spring
application context, hence fixing the root cause of the problem.

commit 2ef59aa51b20f18dac717dfc2c6d4a6d434810bb
Author: Gabriel Roldan <[gabriel.roldan@camptocamp.com](mailto:gabriel.roldan@camptocamp.com)>
Date: Sun Jan 26 18:40:09 2025 -0300

[GEOS-11696] AdminRequestCallback not loaded due to spring bean name conflict

Rename spring bean `adminRequestCallback` to
`wicketAdminRequestCallback` in
`src/web/core/src/main/java/applicationContext.xml` to avoid component
name clash with restconfig's
`org.geoserver.rest.catalog.AdminRequestCallback`.

The later is loaded through component scan and ignored due to name
clashing when both gs-restconfig and gs-web-core are in the classpath.

There are two backport pr’s, shall I merge them with a merge commit?

8

Hi Gabriel,
yes, policy, one ticket, one commit, as per checklist.
And we want a linear history, so no merge commits, but rebase merge on PRs that are clean, squash merge for those that are not.
Unfortunately policy is not respected consistently, often by mistake or ignorance about it.

That PR in particular needed to be squash merged because you pushed two commits in it.
I’m all in favour of bots that enforce the policy better and will prevent merges that are using “merge”,
or are merging multiple commits for the same Jira ticket.

I understand that avoiding merge commits can cause troubles for those that try to keep a fork in synch,
but that’s also the intent, keeping a fork should not be easy, merging changes in the mainline should be the preferred path.

9

That’s fine, my point is it’s a shame to lose the nice commit messages. Like in, we’ve got a lot of squashed merges
with several unuseful commit messages, and in this case there were two well crafted ones and got lost, only the title survived.

That oughta be because the “Allow squash merging” setting in the repo was changed?

In any case I’m good, I’ll make sure to make pr’s with a single commit, I just thought the messages for these
two were going to be kept as they used to.

Cheers,

10

2c

Have you come across the https://www.conventionalcommits.org/ spec?

Both in general, and then this specific FAQ:

Do all my contributors need to use the Conventional Commits specification?

No! If you use a squash based workflow on Git lead maintainers can clean up the commit messages as they’re merged—adding no workload to casual committers. A common workflow for this is to have your git system automatically squash commits from a pull request and present a form for the lead maintainer to enter the proper git commit message for the merge.

11

Hi Peter,
yes we can do it, and I normally try to do it, however:

  • it places another (small) chore on the reviewer
  • the backport bot will perform a backport with the original commits (which has to be squash merged again)

So it would be nice not to have to go through that three times…