1 commit == 1 reviewable unit == 1 PR == 1 CL == 1 feature == 1 fix is a perfectly reasonable way of working.
I used to work at companies where no one squashed their commits and the entire git logs were filled with 80% non-sense like "temp" or "bad" or "working" with the other 20% being coherent changes. What's the point of doing this I ask?
I don’t quite get the situation. After merging a PR/MR (with the squash feature of Gitlab or GitHub) the equality you describe holds and there’s a nice sequential git log.
I quite like that I can tell my colleagues they can commit whenever they feel like it (and should probably commit more often than they feel like) because it becomes one clean commit in the end anyways.
In my experience the situation with GitHub is either that
- you apply the commits as-is on the target branch and end up with a ton of pointless "fix this" and "fix that" commits that were created during the review process,
- you squash everything and lose what might otherwise have been meaningful, useful commit boundaries for the purpose of bisecting, reverting or just investigating the history of changes or
- you rework your commits and force push changes to the branch you want to pull and create confusion as to what changed because that's not usually how review is conducted with GitHub.
I think the pull request is a bad abstraction. Not terrible, and certainly easy to grasp, but bad enough that everything you do with it is a compromise. At the very best, if you apply with the squash+rebase strategy and limit the scope of your PRs to what makes perfect sense as just one commit in the target branch, it's just a convoluted way of working with individual commits.
I wish git worked like some of the other dvcs (bazaar/breezy). The default merge type from a branch should be a merge commit and git log should only show the first parent (commits directly on the current branch).
This gives you a very clean log on main (it only shows commits directly on this branch). There is no need to squash, rebase, or anything else. And if you want to dig down into individual commits that happened on a branch, you can!
This can be a bit replicated by forcing a merge type of merge and setting an alias of log to "log --first-parent", but since that isn't the default, that isn't what you see when you look at the commit logs on platforms like github.
I never understood why git decided to show a flat list of every commit that happened, even if the commit originally happened on a separate branch.
there is an inbetween .... i insist people interactively rebase those commits out. In some contexts it is actually important to have traceability of iterative proof of work towards the final result.
Well are we talking about commits pre- or post-merge? I don’t care how many commits you put into the PR / MR as long as they squash down to a single commit upon merge.
The reason people like to work this way is that it lets you very effectively respond to review. You address them by amending the commit to incorporate the feedback.
This enables good interdiff review, so you can re-review just the new stuff in the new version of the commit and not the entire thing all over again.
Comments
1 commit == 1 reviewable unit == 1 PR == 1 CL == 1 feature == 1 fix is a perfectly reasonable way of working.
I used to work at companies where no one squashed their commits and the entire git logs were filled with 80% non-sense like "temp" or "bad" or "working" with the other 20% being coherent changes. What's the point of doing this I ask?
I don’t quite get the situation. After merging a PR/MR (with the squash feature of Gitlab or GitHub) the equality you describe holds and there’s a nice sequential git log.
I quite like that I can tell my colleagues they can commit whenever they feel like it (and should probably commit more often than they feel like) because it becomes one clean commit in the end anyways.
In my experience the situation with GitHub is either that
- you apply the commits as-is on the target branch and end up with a ton of pointless "fix this" and "fix that" commits that were created during the review process,
- you squash everything and lose what might otherwise have been meaningful, useful commit boundaries for the purpose of bisecting, reverting or just investigating the history of changes or
- you rework your commits and force push changes to the branch you want to pull and create confusion as to what changed because that's not usually how review is conducted with GitHub.
I think the pull request is a bad abstraction. Not terrible, and certainly easy to grasp, but bad enough that everything you do with it is a compromise. At the very best, if you apply with the squash+rebase strategy and limit the scope of your PRs to what makes perfect sense as just one commit in the target branch, it's just a convoluted way of working with individual commits.
I wish git worked like some of the other dvcs (bazaar/breezy). The default merge type from a branch should be a merge commit and git log should only show the first parent (commits directly on the current branch).
This gives you a very clean log on main (it only shows commits directly on this branch). There is no need to squash, rebase, or anything else. And if you want to dig down into individual commits that happened on a branch, you can!
This can be a bit replicated by forcing a merge type of merge and setting an alias of log to "log --first-parent", but since that isn't the default, that isn't what you see when you look at the commit logs on platforms like github.
I never understood why git decided to show a flat list of every commit that happened, even if the commit originally happened on a separate branch.
there is an inbetween .... i insist people interactively rebase those commits out. In some contexts it is actually important to have traceability of iterative proof of work towards the final result.
Well are we talking about commits pre- or post-merge? I don’t care how many commits you put into the PR / MR as long as they squash down to a single commit upon merge.
When you work this way, each commit is expected to be able to land independently.
Doesn’t sound like it leaves much room for error. How do you address PR / MR comments? Force push?
The reason people like to work this way is that it lets you very effectively respond to review. You address them by amending the commit to incorporate the feedback.
This enables good interdiff review, so you can re-review just the new stuff in the new version of the commit and not the entire thing all over again.
it lets you maintain version history when working, then most workflows auto squash on merge