[illuminator83]

I always tell my engineers to create atomic commits and we usually review commit by commit. Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review. I expect people to understand how to clean their commit history - if they don't I teach them. The senior people who are capable of structured work - e.g. are used to contribute open source projects - do it anyway. Because messiness is usually not tolerated by maintainers of important projects.

You find people how aren't able to craft clean commits and PRs usually thrive in environments in which people are either work mostly alone or in which cooperation is enforced by external circumstances (like being in the same team in a company). As soon as developers many are free to choose whom to associate with and whose code they accept - rules are usually made and enforced.

[nitwit005]

The reason for that tidiness with some open source projects is because they want to conserve the valuable time of the maintainers, and are willing to expend a lot of time from people wanting changes to do that.

That's not the situation in a normal corporate environment. You want to reduce total time expended (or total cost, at least). It's going to be cheaper to just have a chat with your coworker when a PR is confusing.

[sigseg1v]

> Obviously commits like "fixed review comments" or "removed some left-over comments" or "fixed typo" should not be pushed into a PR you asked others to review.

Could you explain this a bit more? I'm having trouble visualizing the end to end process.

1. Someone has what they feel is a complete change and submits a PR for review.

2. The reviewers read part of it, first half looks good, and halfway through they have concerns and request changes.

3. The submitter now has to fix those concerns. If they are not allowed to push an additional commit to do this, how do you propose they accomplish this? Certainly they should not force push a public branch, right? That would cause pain to any reviewer who fetched their changes, and also break the features on GitHub such as them marking which files they have already read in the UI. But if we cannot push new commits and we cannot edit the existing commits, what is the method you are suggesting?

[illuminator83]

Everytime you push commits to a PR you decide what you push exactly.

The first time you push, you should have squashed/rebased your changes into a structure that make sense. Atomic commits are best. Could even be a single commit. Sometimes it makes sense to have multiple commits. E.g. - introducing a new API - moving other code to use the new API - deleting the old API

This could also be a single commit. That is really up to you/your team.

And yes, you rebase/squash and force push new commits. Every team I had in the past 12 years routinely used force-push for PR iterations.

Turns out, when writing production code, other people rarely checkout and work simultaneously on branches of half-finished stuff of other people. It is and should be very, very rare. Very occasionally it happens that someone bases their work on the branch another developer. In these cases, people just carefully rebase their branch on "origin/other-branch" after a fetch. You can't rely on people not force push anyway. Even if you agreed on it, sometimes this needs to be done. (e.g. commit a very large binary file by accident). So you need work in a way which assumes that somebody might have force-pushed their private branch.

Multiple people working on the same branch without a PR process is indeed messy and you should never force push when you do that. They key here is to avoid working with multiple people on the same branch in the first place. I've seen this happening only when: - Work items are too big and not broken down enough (branches are actively developed for for several weeks/months ). Usually and indication of lack of architecture and product leadership. If you do this, you have lots of other interesting problems as well. You are prototyping really but pretending you don't. - You are consciously experimenting and prototyping. Make whatever mess you want - in code and history. You are going to iterate so much and so messy that whatever you produce can't a product. Figure out what you want and need to do and start with a clean implementation afterwards. And maybe delete that messy branch eventually.

So, we have two modes: - Prototyping - which means you are allowed to make a mess because you throw it away anyway. No one, including you, cares much what you code and your repo history looks like. - Production - you write code and repo history for eternity. You do it right for the sake of everybody's sanity.