Consider not squashing commits when merging, to support contributors

[aufdenkampe]

We understand the reasons commonly argued for squashing commit history when merging.

The practice of squashing history, however, makes it much more burdensome for any contributor who is working from a fork, such as ours: https://github.com/LimnoTech/pygeoapi (used to deploy https://api.water.usgs.gov/). This is because it requires tedious manual investigations to figure out which, if any, of our earlier pull requests have been incorporated in each official release or the main branch. Then, once we have figured that out, we need to rebase some, but not all, of our branches to align with the current state of the upstream repo. This is a lot of tedious work that we believe adds needless friction to contributing to this project.

The alternative, not squashing commit history when merging, substantially streamlines all of that work, because commit tags and history are preserved among all forks and branches. We are not alone in recognizing that squashing history is harmful. For example, see https://dev.to/wesen/squash-commits-considered-harmful-ob1

@tomkralidis, @webb-ben, @kalxas, please consider changing your practices around squashing history. The community of contributors will appreciate it.

cc. @dblodgett-usgs, @ewojtylko, @ptomasula, @mikemahoney218-usgs, @sjordan29, @rajadain, @kieranbartels

[tomkralidis]

@aufdenkampe thanks for the information. We can discuss at the next pygeoapi meeting, however pygeoapi squashes PRs to be able to safely revert and backport commits in a concise manner. Perhaps squashing from your fork before / during a pygeoapi PR can be helpful.

[aufdenkampe]

@tomkralidis, thank you for considering our suggestion!

There are other benefits to not squashing history, described in the post I linked above, including an argument that squashing actually makes back-porting and bug-sleuthing more difficult.

The idea of squashing our commits before issuing a PR might not really solve our problem if you continue to squash commits, because our commit tag will be replaced by yours. Not squashing commits preserves our commit tags.

We look forward to hearing what the core team decides.

[francbartoli]

@aufdenkampe this is just to argue my -1 on the above request. It’s not clear to me why pygeoapi should change some of its practices to make the workflow easier for a downstream project that wants to maintain a custom fork and not the other way around.
As of matter of fact, your proposal would make our maintenance workflow way more complicated and the usability of the history definitely worse.
Perhaps you could start using pygeoapi as a library and maintain your custom code on top of it?

[dblodgett-usgs]

Hi All --

I feel I should speak up here in defense of @aufdenkampe -- he is representing a very real issue that we are experiencing in several production deployments of pygeoapi. Our teams are doing our best to contribute bug fixes and important features to pygeoapi and we've had more difficulty managing our git history on our contributions than feels necessary. Perhaps we are not seeing things from each other's point of view or perhaps this is just a really hard problem.

Either way, I would appreciate it if some specific rebuttal to the points offered in Anthony's cited post would be provided before downvoting and telling us we are wrong and we should just use the library.

My read is that the best practice here is for developers to take care in crafting a clean commit log that tracks compact and cohesive blocks of changes rather than using PR-level squashed-commit policy to achieve the same goal. The iteration cycles / PR review and release timelines are long and convoluted enough that we inevitably have to move forward with changes destined for pygeoapi core (especially bug fixes) in our production deployment or we end up in a protracted "waiting for others" blocked state.

To be clear, we do use pygeoapi as a library ... one that we contribute to and use in production.

So my proposal would be to expect PRs to have a compact commit history and that part of PR review could require a person to rebase a commit history that doesn't match geopython convention.

Our other option is to maintain a stronger separation between our forks and the upstream library code and just move much slower in general. I don't think that's necessary and it would feel like a real shame to give up agility that we could achieve through a slight change to the way the community tracks change.

Definitely work discussion at the next community call. I appreciate everyone engaging on this -- it's definitely in the category of "sticky open source problems that will never go away".

Regards -- Dave

[francbartoli]

Hi @dblodgett-usgs,

thanks for jumping in the discussion, it is worth considering other points of view. However, as a member of the project's governance I do believe the effort here is to preserve the speed’s pace of our team and its preferences in the developer experience.
I don’t think there is a best practice here but just a preference and my -1 stands for my own as I normally do use squashing.
Also, when I make some reviews for medium/big PR, I tend to focus on the design rather than on ephimerel changes to the history.
Again, without any criticism on your point, I believe it’s just a priority for the project.
Anyway, if the PSC wants to explore this request you could open a RFC with the proposal and reasons to enforce this workflow so that we can formally vote for it. To be honest, you shouldn’t expect I would express a different vote for that, sorry.
Besides that, I agree on the fact that a PR should have a compact commit history as a general rule but I disagree to spend our time to rebase or enforce this convention for no real benefits (over a squash action at the end of a review) in the pygeoapi’s governance.

To be clear, we do use pygeoapi as a library ... one that we contribute to and use in production.

if you are using pygeoapi as a dependency and pinning it either to a master's commit or a tag what's the issue with the history? Sorry but actually I don't understand how a pip install pygeoapi workflow and the pygeoapi history would affect your downstream projects.
I would expect you could extend and customize pygeoapi at certain level, would be a valid alternative instead of having a fork with your current git history workflow?

[aufdenkampe]

Response below

[aufdenkampe]

@francbartoli, I think it's possible that we have done a poor job at communicating the USGS Water Mission Area work with pygeoapi and our collective motivation for proposing this suggestion to the pygeoapi maintainer team.

We would very much like to use a pip install pygeoapi workflow, as we love what you have built! Unfortunately, since we first starting using pygeoapi in 2022, there have been many reasons that it could not work for us. These include missing features that had not yet been developed from the OGC-API specifications, missing configuration options, bugs, and other issues that blocked us from installing pygeoapi alongside our custom OGC-Processes extensions, mostly related to dependency conflicts (#2321 is the most recent; for previous see https://code.usgs.gov/wma/nhgf/pygeoapi/-/work_items/34). Recognizing that the pygeoapi core dev team was already doing everything possible to meet the needs of the growing community, we decided to become contributors.

As contributors to pygeoapi who are not part of the core team, we need to maintain a fork to prepare and test our contributions before issuing a pull request. Given the number of Pull Requests that have been merged, it appears that our contributions have some value to this project:

• fix coords for non-cube query #1432
• Fix for Starlette CORS #1741
• Add License and Extent information to Collections #1861
• Updates for parsing with STAC browser #1890
• EDR Inheritance  #1972
• Coverage json time field fix for xarray provider #2064
• Filtering Geospatial Data #2165
• Fix STAC API endpoints on Starlette #2153

As active contributors and committed users of 'pygeoapi' for high profile deployments, we are very much interested maintaining the safety and integrity of software while also lowering the development friction for all contributors (including the core team). I did not intend to imply that our proposal would require your core team to ever rebase. On the contrary, we would like to minimize rebasing by all contributors. Presently, the practice of squashing commits requires all outside contributors to continually rebase their development branches, then manually explore which PR has been incorporated, then cherry pick commits that have not yet made it into the latest release from geopython. Our proposal to stop squashing commits would substantially streamline workflows for outside contributors while minimally impacting the workflows of the core team. We believe that once the core team made the switch, the total effort by the core team to manage pull requests should be the same.

Please consider this request as a potential win-win for all. You win because you will expand the number of developers who are continually contribute to making pygeoapi better and better with each release.

[tomkralidis]

@aufdenkampe thanks for the discussion. It goes without saying that pygeoapi greatly appreciates all contributions to the project, including your organization/team.

Squashing commits allows maintainers to succinctly backport, cherry-pick and revert in a very simple and straightforward manner:

• removes superfluous commits (and removal of commit messages of the form test again or bump CI)
• logical and simple/easy reverting and backporting: this is priority 1, and has worked for many Geopython projects for close to two decades, as well as many FOSS projects overall. If a single PR addresses two distinct issues, we typically do not squash merge in this case, but commits need to be "clean". Of course, we only squash in the context of a PR and never against master branch per se.

PRs should solve one issue or implement one feature/enhancement. In that context, a single commit (whether done so in the PR, or put forth as a squash/merge as part of accepting the work) allows us to isolate and backport/revert with ease. How a project integrates a given PR (or not) is in the remit of the project proper.

If the approach seems simple, that is the intention. This is a feature, not a limitation. I strongly maintain that pygeoapi needs to move slowly and surely, with simple and sustainable processes. Running git cherry-pick and git revert as required over time against clean, logical commits aligns with this simplicity.

-1

[aufdenkampe]

@tomkralidis, thank you for considering our suggestion and for providing clear guidance and reasoning for your decision. We appreciate that. We'll continue to provide small single-issue PRs and will squash commits on our end to simplify our rebasing as we sync with your main branch.

We look forward to the continuing collaboration and very much appreciate all the impressive work you are doing for the OGC and FOSS communities! Thank you!