Commit and Pull Request Etiquette

[Hallberg-NOAA]

Commits are the units in which git records changes, while pull requests are our essential tool for code review, management, and quality control. Within this discussion we would like to address several considerations about the size, granularity, and levels of documentation for commits, pull requests to branches (e.g., dev/ncar) on one of the institutional forks (e.g., NCAR/MOM6), and consolidating pull requests to the common main branch.

To start the discussion, I would like to offer the following observations or suggestions:

• Commits are the units for many approaches to debugging. Every commit that is present in a PR should therefore compile and run.

• All commits should have comments adequate to describe (1) what they contain, (2) why they were introduced, (3) what are the anticipated consequences, and (4) what tests has the commit passed. For example, a statement like "All answers and output are bitwise identical in the MOM6-regressions and TCs." would cover the last two points in many cases (provided that the code has actually been tested this way).

• All answer changing PRs should be as small as possible, with any non-answer changing code changes separated out, to clearly highlight the code changes. In many cases, answer changing PRs should be avoided by adding a runtime bugfix flag.

• Single-commit PRs can be very handy, in which case the documentation of the commit should meet the requirements for the documentation of the PR.

• It takes about an hour to run all of the automated testing required to assess whether a PR is acceptable, and longer if there are changes to answers or output that must be documented. There can be a case for merging small and trivial commits into a single PR to limit the testing burden.

• Multi-commit PRs should have an organizing theme or principle that brings them together.

• Multi-commit PRs should come with messages that describe the overall purpose of the PR, and a list of the commits in the PR.

• Pulls from the institutional forks to the main branch should have documentation describing what they contain, but they can rely on the underlying PRs for the details of the commits.

[sanAkel]

@Hallberg-NOAA
All sounds great! 👍

Would suggestion a template for a PR that the requester would have to fill in. And also, if all the boxes have not been checked, state the reason(s) or expect ...? (statement of consequences! e.g., delay?)

[angus-g]

docs for templates here: https://docs.github.com/en/github/building-a-strong-community/about-issue-and-pull-request-templates

[marshallward]

Following on Bob's second point:

All commits should have comments adequate to describe (1) what they contain, (2) why they were introduced, (3) what are the anticipated consequences, and (4) what tests has the commit passed.

I believe the code would benefit from better documentation of our code changes in the commit logs. In particular, I think we should try to avoid single-line commit messages (git commit -am "your mesg").

The git commit message is designed to be a proxy for an email, since Linux (for which git was created) is both patched and documented through email patches, and archived in its mailing lists.

When we use git commit -m "module X was updated", it is equivalent to sending an email containing your patch, which may have a brief subject but has no body explaining what we have done. Code often explains the "how" (on good days, at least) but rarely explains the "why".

I would encourage people to start using long-form commit logs with subjects and bodies to help produce some self-documentation of the changes. Just omitting -m and writing out the log will improve our logs and possibly promote better habits in other areas.

---

As for how to do this, I admit it can be difficult and don't always know myself how to best achieve it. But I can at least describe my own process.

1. When starting a new feature, try to work in development branch, which you have no intention of submitting to a shared branch. Commit as often as you like, and push any changes to your private repository if you like. I may even use git commit -m "new code" 😱 .

2. When you reach some milestone in your project, gather up all those commits into a single commit, to be applied to your proposed feature branch. This would be a long-form commit with a clean subject (under 50 characters, as suggested by man git commit) followed by the commit summary.

This would also be the point where we define the "atomicity" of our commits and can ask whether the code builds or passes testing. But we should that as a separate discussion.

3. Finally, when your feature is complete, and decomposed into a well-defined and atomic set of commits, it's ready to send to GitHub as a Pull Request. This is where we define the "unit of review", i.e. large enough to matter, but small enough that we can read and judge it.

Obviously it's very hard to get the perfect sizes right, which is part of the reason for this discussion, but I believe this is a good model of development which will help us to produce more workable commit log.

---

As a second use case, suppose you create a clean, beautiful PR with a perfect set of commits, but now your perfect code is failing some tests. Let's say something very minor like trailing whitespace or an undocumented variable.

Recently I have started taking the following approach:

1. Rather than create a new commit, use git commit --amend to modify the existing commit (after adding the new files).
2. git push --force to your private GitHub repository where the PR is hosted. This will update the PR on GitHub's side and re-run the tests.

HOWEVER!! If I were to do this on a shared branch like main, then we have a big problem! But for local changes, this is OK.

---

These are just suggestions, and I don't mean to suggest we should immediately start enforcing rules like this. But I do think they are good habits to develop and discuss further.