[lolinder]

I prefer to have clear commits that tell a tidy story. For example:

* Refactor function `foo` to accept a second parameter

* Add function `bar`

* Use `bar` and `foo` in component `Baz` to implement feature #X

If you give me a commit history like this, I can easily validate that each step in your claimed process does what you describe.

If you instead give me a messy history and ask me to read the diff, you might know that the change to file `Something.ts` on line 125 was conceptually part of the refactor to `foo`, but I'll have to piece that together myself. It's not obvious to the person who didn't write the code what the purpose of any given change was supposed to be.

This isn't a huge deal if your team's process is such that each step above is a PR on its own, but if your PRs are at the coarseness of a full feature, it's helpful to break down the sub-steps into smaller (but sane and readable) diffs.

[88913527]

This is reasonable, but the problem I encounter is how stifling it seems to ask others to structure their work so specifically. By way of comparison, getting compliance on conventional commit messages is a challenge, and that's an appreciably smaller ask than this.

[lolinder]

Oh, for sure. This is how I structure my own PRs, but I've certainly never bothered to ask a coworker to do so, I just appreciate it when I see it.

That said, OP is in an environment where it sounds like this kind of structure is already the cultural norm.

[eitland]

From another one who tries to do the same (but doesn't enforce it):

Thanks!

[krainboltgreene]

Funny that two of your commits don't actually tell us why they exist, one simply describes the diff (which you should never need lol?) and the other proxies that responsibility to some other system.

You could have simply randomized the text in each commit, put the ticket id and the one "why" in the merge commit body and gotten the same end result amount of real information in the end.

[lolinder]

The first line of the commit message isn't about including information that couldn't be gleaned from the commit. That can be done in subsequent lines. The first line is for two purposes:

* Priming the reader so they are able to quickly interpret what they're seeing when they open the commit.

* Making it easy to search or scan for a specific change.

The last commit message in my example would probably have included the name of the feature as well as the ticket number, but I couldn't be bothered to invent an actual feature name.

DRY doesn't really apply to technical writing, at least not as extremely as you seem to think it should. Headings are supposed to summarize the contents, and that's what commit messages are: headings.

[krainboltgreene]

> Making it easy to search or scan for a specific change.

I'm trying to imagine the near infinite terms I would have to search for to find the commit where I "changed from a hash to a set".

Regardless, every other thing you said could also just be done in the central PR body (and thus the merge commit) and be much easier to access.

Instead of "priming the reader" it's infinitely more helpful to tell the reader why you did something, because you can't extract that from a diff.

[lolinder]

> Instead of "priming the reader" it's infinitely more helpful to tell the reader why you did something, because you can't extract that from a diff.

Again, that can go in the PR body or in subsequent lines. You have ~50 characters in that first line, which is never going to be enough to fully explain anything.

I'm also not suggesting that you eliminate the PR body: that should also include more context. All I'm suggesting is that taking the trouble to organize your commits into discrete units helps reviewers to understand how you perceive the various changes in a single PR as being related to one another, and no amount of text in the PR body will provide the same benefit as being able to look at several distinct diffs containing related changes.

[fijiaarone]

I like to leave comments like this too:

loop i up to n times

break when false

check value returned is not null

[dilyevsky]

In the context of Github PR you can’t leave reviews on commits other than what’s currently the tip commit of the pr branch so structuring this way is just wasted effort.

What you should be doing is breaking down PRs more finely so that your unrelated refactors are all separate single-commit PRs. That ofc requires that your pr review round trip time is fast

[lolinder]

I'm pretty sure I've left comments on a commit before in a GitHub PR. The comment just goes in the right place in the PR diff, assuming no changes, or comments can actually be attached to commits themselves (which is what happens when a comment becomes stale—it retains a reference to the original commit).