Comment by seba_dos1
11 hours ago
These commits reaching the reviewer are a sign of either not knowing how to use git or not respecting their time. You clean things up and split into logical chunks when you get ready to push into a shared place.
11 hours ago
These commits reaching the reviewer are a sign of either not knowing how to use git or not respecting their time. You clean things up and split into logical chunks when you get ready to push into a shared place.
Why would the reviewer look at the commit messages instead of the code?
1. Open PR page in whatever tool you're using
2. Read title + description to see what's up
3. Swap to diff and start reading through the changes
4. Comment and/or approve
I've never heard anyone bothering to read the previous commit messages for a second, why would they care?
In some cases, reviewing PR diffs commit-by-commit (and with the logs as the narration of the diff-by-diff story) is a substantial improvement over reviewing the entire PR diff. Concrete examples...
* A method or function that has code you realize needs to be shared...the code may need to be moved and also modified to accommodate its shared purpose. Separating the migration from any substantive modifications allows you to review the migration commit with the assistance of git's diff.colorMoved feature. It becomes easier to understand what changes are due to the migration, and what changes were added for more effective sharing.
* PRs sometimes contain mechanical work that is easy to review in isolation. Added or removed arguments, function renames, etc. No big deal if it's two or three instances, but if it's dozens or hundreds of instances, it's easier for the humans to review all of those consistent changes together, rather than having them mixed in with other things one has to reason about.
* Sometimes a flow of commits can help follow a difficult chain of reasoning. PR developer claims that condition X can never occur, but the code is complex enough that it's difficult to verify. However, by transforming the code in targeted ways that are possible to reason about, the complexity might be reducible to the point where the claim becomes obvious. One frequent example I see of this is of function/method arguments that are actually unnecessary, but it wasn't obvious until after some code transformations.
Because it's a useful abstraction. If you only look at PRs and don't ever care about commits, why are they even being sent to reviewer in the first place? Just send a diff file.
Having atomic commits lets you actually benefit from having them. Suddenly you don't have to perform weird dances with interconnected PRs with dependencies as "PR too big" is not such a problem anymore as long as commits are digestible; you can have things property bisectable; you can preserve shared authorship; you can range-diff and have a better view on what and how changed between review passes, and so on...
The unit of change is commit, and PRs group commits you want someone to pull. If you don't want or need any of that, you're just sending a patch file in a needlessly elaborate way.
> If you only look at PRs and don't ever care about commits, why are they even being sent to reviewer in the first place? Just send a diff file.
This is in fact what hg does with amending changesets and yes it works far better. Keep PRs small and atomic and you never need to worry about what happens intra-pr. If you need bigger units of work that's what stacking is for.
1 reply →
>Swap to diff and start reading through the changes
this forces the reviewer to view the entire diff at once, which can greatly increase the cognitive load vs. being able to view diffs of logical units of work
for tiny PRs it may not matter, but for substantial PRs it can matter a lot
What if the shared place is the place where you run a bunch of CI? Then you push your work early to a branch to see the results, fix them etc.
You can do whatever you want with stuff nobody else looks at. I do too.
I meant "shared place" as an open review request or a shared branch rather than shared underlying infrastructure. Shared by people's minds.
You can always force-push a cleaned up version of your branch when you are ready for review, or start a new one and delete the WIP one.
You can, but instead you can also just squash merge in one click. And avoid that people merge there dozens of fixes if you allow anything but squash merge.
I hate (and fear) force-pushing and "cleaning up" git history as much as other people dislike squash-merging =)
It just feels wrong to force push, destroying stuff that used to be there.
And I don't have the time or energy to bisect through my shitty PR commits and combine them into something clean looking - I can just squash instead.
2 replies →
What are examples of better ones. I don’t get the let me show the world my work and I’m not a fan of large PR
if you mean better messages, it's not really that. those junk messages should be rewritten and if the commits don't stand alone, merged together with rebase. it's the "logical chunks" the parent mentioned.
it's hard to say fully, but unless a changeset is quite small or otherwise is basically 0% or 100%, there are usually smaller steps.
like kind of contrived but say you have one function that uses a helper. if there's a bug in the function, and it turns out to fix that it makes a lot more sense to change the return type of the helper, you would make commit 1 to change the return type, then commit 2 fix the bug. would these be separate PRs? probably not to me but I guess it depends on your project workflow. keeping them in separate commits even if they're small lets you bisect more easily later on in case there was some unforseen or untested problem that was introduced, leading you to smaller chunks of code to check for the cause.
If the code base is idempotent, I don't think showing commit history is helpful. It also makes rebases more complex than needed down the line. Thus I'd rather squash on merge.
I've never considered how an engineer approaches a problem. As long as I can understand the fundamental change and it passes preflights/CI I don't care if it was scryed from a crystal ball.
This does mean it is on the onus of the engineer to explain their change in natural language. In their own words of course.
3 replies →
Haha, good luck working with a team with more than 2 people. A good reviewer looks at the end-state and does not care about individual commits. If im curious about a specific change i just look at the blame.
> A good reviewer looks at the end-state and does not care about individual commits.
Then I must be a bad reviewer. In a past job, I had a colleague who meticulously crafted his commits - his PRs were a joy to review because I could go commit by commit in logical chunks, rather than wading through a single 3k line diff. I tried to do the same for him and hope I succeeded.
And then someone comments on a thing, they change it and force-push another "clean" history on top and all of your work is wasted because the PR is now completely different =)
Why are those not just separate PRs? Or if they really needed to be merged at once - they should still be separate PRs but on a feature branch
2 replies →
Split the PR rather than force me to wade through your commit history. Use graphite or something else that allows you to stack PRs.
1 reply →
Sometimes I have to go back and fix a bug that appeared during another branch. Having the original commits helps me bisect it.
Not often, but given that it costs me nothing to have it all in my tree, I'd rather have it than not.
You review code not to verify the actual output of the code, but the code itself. For bugs, for maintainability. Commit hygiene is part of that.
I have no troubles working on big FLOSS projects where reviews usually happen at the commit level :)
So if a PR consists of 20 commits, they review every single commit linearly without looking at the end result first?
1 reply →