Hacker News new | ask | show | jobs
by cardanome 911 days ago
How the hell is someone supposed to review your pull request if you don't take the time to clean it up?

I normally go through every single individual commit when reviewing something and find the commit messages extremely helpful to understand what some change is supposed to do.

Yes, cleaning up your commits takes some time butt I don't see an alternative if you don't work alone and want your code to stay maintainable.
1 comments
Kwpolska 911 days ago
I review the pull request as a whole, looking at the diff between main and the latest commit on the branch (i.e. what GitHub/etc show by default). Reading commit-by-commit means you’d read code that the author knows is wrong and had already fixed it, but you’re cluttering your mind with it. During re-reviews, I usually look at the diff between the last commit I reviewed and the newest commit.
link
cardanome 911 days ago
> Reading commit-by-commit means you’d read code that the author knows is wrong and had already fixed it

If the commit is wrong, it shouldn't be there. I expect every commit in a Pull Request to be functional on its own or I am not going to approve it in the first place. Git has tools to rewrite your commit history and you should use them.

The whole point is that I should be able to revert individual commits without code breaking. At least that is the ideal. A clean version history matters a lot of the people maintaining your code down the line.

link
fhreviewable 910 days ago
In my experience, this is very team-specific. Some teams want squash merges and ignore individual commits and only look at the latest version, while others care about the "history" and will tidy it up in the PR and then merge all the commits from the PR. Though I've found the latter to be much more rare, that's why some tools (like Reviewable https://docs.reviewable.io/reviews.html?highlight=commit-by-...) have a commit-by-commit option but the default is to combine them for review.
link
wirrbel 911 days ago
Yes and no.

I think what you say is definitely the goal for day-to-day contributions.

However, there are changes to a code base that are more "Manhattan project" in nature where not all changes can be neatly packaged into their own commits, OR the PR author kind of needs to re-do their coding on a clean room branch. Which is significant overhead.

Being able to undo a commit is a means to an end, not the ultimative goal.

link