Merge standard/policy/recommendation for the pytest repository

[nicoddemus]

Hi folks,

I don't think we have ever explicitly discussed which strategy to use when merging PRs into the pytest repository, we have been throwing comments here and there with personal preferences but never managed to come up with a policy or recommendation regarding that.

I think PR commit history in pytest can be divided in:

1. Miscellaneous commits:

   • Implement X
   • Fix test_a
   • Add myself to AUTHORS
   • !fixup Fix test_a
   • Update tests/test_integration.py
   • Update tests/test_integration.py

   Above and in the next examples, read that as the commit history of a single PR.

2. Separate commits related to the same topic:

   • Implement X
   • Add myself to AUTHORS
   • Update CHANGELOG for X

3. Separate commits, each with their own topic (refactorings, renames, etc), but still have a larger topic/purpose.

   • Refactor class X in preparation for feature Y
   • Remove unused method
   • Implement feature Y

4. Separate commits, each with their own topic, but without a larger topic/purpose other than improve the code base (using more modern techniques, improve typing, removing clutter, etc).

   • Improve internal names in X
   • Add type annotations to Y
   • Remove unnecessary dict access
   • Remove unreachable code due to EOL Python

IMHO having a readable commit history is the most important thing. Often I use git blame to understand why a line of code was written the way it is, so it helps that the commit that changed that line tells the entire history (a large commit with Implement feature X) rather than a "random commit" (!fixup fix type annotation in update_fixture).

So I would vote for these strategies for each type of commit history:

1. Squash: I think this commit history is "messy" and just squashing them into a single commit is best.
2. Squash: While the commit history is not messy, I don't see much value in keeping the related changes separate.
3. Merge: each commit is valuable on its own, even if they serve a common topic.
4. Merge: each commit is valuable on its own, could even technically be separate PRs (not saying they should, just to mention that each commit would make sense sense even as separate PRs).

I think it would be interesting for us to reach a common ground here, and even mention this somewhere in the contribution docs to have an official standing regarding this subject.

cc @The-Compiler @RonnyPfannschmidt @bluetech @webknjaz

[webknjaz]

I do care about history too. In fact, I usually teach advanced Git to my mentees because I believe it's that important.
I think that (2) is the most controversial one, because the line between what's atomic and not is subjective. Natural merge commits preserve actual history. And it's good for git paleontology (though, I heard from a teammate that they confuse bisect). I prefer those unless the incoming PR is a mess. It's the most accurate representation of the flow of thought. It'd even inject commits with attempts to solve something and their reverts just to keep that in history (with proper explanation in commit messages, of course).
Merging branches with several atomic commits is good for this reason. It also let's people revert those commits, not entire PRs. I think that more often than not, those squashes are nowhere near being atomic. So I treat them as a stop gap. After all, when commits are composed, the authors already grouped related changes across files. Combining many things together breaks that composition and atomicity. This also moves the responsibility of writing a new commit message to the merger, which means they have to ensure to capture all the context that corresponds to the new fake-merge commit. I haven't seen many maintainers do that well, not really. Some people would say that the context is somewhere on GitHub but I don't think that's a good thing as it ruins Git's ability to hold information offline, and people can't then make proper use of its distributed nature, adding a dependency on the internet.
Commits like adding oneself to authors are atomic. They don't really relate to the main theme of PRs so it feels strange to bundle them into commits saying "Implement X". Change notes are similar but may need to be separated when the refer to the PR number as it's not known at the time of commit and only become available post PR creation.

P.S. It'd be good to also make a policy of making PRs from forks so that temporary upstream branches don't pollute other people's forks.

[nicoddemus]

@webknjaz thanks for the response!

Merging branches with several atomic commits is good for this reason. It also let's people revert those commits, not entire PRs. I think that more often than not, those squashes are nowhere near being atomic. So I treat them as a stop gap. After all, when commits are composed, the authors already grouped related changes across files. Combining many things together breaks that composition and atomicity.

I understand what you are trying to say, but I disagree: if I want to revert the PR which added feature X and had separate commits for changelog and authors, I want to revert everything, not only the commit that implements feature X. But TBH my preference for squashing is not really related to ease of reverting (after all it is easy to revert all commits of a merge), but that I don't see any value of seeing a commit "Add changelog for feature X" when looking at git blame later. I find much more useful to be able to see the whole commit that added feature X, including authors, changelog, docs, etc. I realize of course that it is possible to find the related commit with a bit of digging, but this is an extra step which should be unnecessary.

This also moves the responsibility of writing a new commit message to the merger, which means they have to ensure to capture all the context that corresponds to the new fake-merge commit. I haven't seen many maintainers do that well, not really.

This is a good point, and one I try to do well when squash/merging: I hardly ever use the default message suggested by GitHub, almost always I rewrite the commit message in that case.

Commits like adding oneself to authors are atomic. They don't really relate to the main theme of PRs so it feels strange to bundle them into commits saying "Implement X". Change notes are similar but may need to be separated when the refer to the PR number as it's not known at the time of commit and only become available post PR creation.

I disagree here, I think adding authors and changelog are part of the "whole" change.

P.S. It'd be good to also make a policy of making PRs from forks so that temporary upstream branches don't pollute other people's forks.

Agreed. That's kind already encouraged today, but I'm not sure it is actually written anywhere...

@webknjaz thanks again for the response. I understand all your points, and by responding I'm not trying to convince you from my POV, clearly you have put lot of thought into this topic too. In this case perhaps we will have to agree to disagree, hehehe.

[RonnyPfannschmidt]

I like the concept of having a decision tree

Some contributions are better squashed,others are best elaborated

Also communication for engaging a contributor is necessary.

I agree with the rough sentiment and wonder if there's a 3rd part system that formally specifies it,so we can refer to it

[Pierre-Sassoulas]

Some contributions are better squashed, others are best elaborated

👍

I would add that if they are better elaborated the commit history should/would be cleaned up properly and rebased. The preferred choice of the author is generally visible in how good the commit history is. And sometime the difference doesn't matter that much.

Personally, if the author rebased and created a great commit history because there's multiple separate/atomic thing in the merge request I'm going to merge, if there's still pre-commit autofix and fixup I'm going to squash. Adding one's name to the contributor list does not deserve a commit in itself imo, so I'm going to consider it's a non rebased commit history and squash. Then deal with the partial revert if/when it happens.

[nicoddemus]

I agree with the rough sentiment

You mean this section?

So I would vote for these strategies for each type of commit history:

1. Squash: I think this commit history is "messy" and just squashing them into a single commit is best.
2. Squash: While the commit history is not messy, I don't see much value in keeping the related changes separate.
3. Merge: each commit is valuable on its own, even if they serve a common topic.
4. Merge: each commit is valuable on its own, could even technically be separate PRs (not saying they should, just to mention that each commit would make sense sense even as separate PRs).

[RonnyPfannschmidt]

yes

[nicoddemus]

Folks,

Thanks for everyone who answered, seems like the majority agree with the examples e resolution suggested in the post.

If nobody objects/would like to comment, I will add a section to CONTRIBUTING.rst documenting that sometime next week.

[nicoddemus]

PR created: #12672