I'd say the biggest drawback of that approach is churn; loads of extra commits and code changes for code review comments.
Of course, that becomes a non-issue if you use a squash merge approach, instead of retain all commits (which in this approach have a lot more of a "WIP" feel to it).
Do code review comments have value after merging? I would also posit they're easier to find if they're in an external location (like github/lab, gerrit, etc), unless you also wrote tools that can figure out which commits and comments belong to a certain feature.
I think the point previous user is making or I would add is: it is essentially up to you to decide what to keep and what not.
Simple example: say a comment points out some issue with the changes (say, lack of unit test on feature a), author could say that emulating scroll events in unit tests is complex and the mocks provide no real value to actual in-browser behavior.
This comment is something you may want to remove for the final merge or add a TODO: in the code, or leave the comments as a discussion for future but meanwhile push so prod people can test, etc.
You could move these comments to your saas git provider tools (e.g. github issues or comments or wiki for the final merge).
In any case the thing I like about this approach is that essentially *forces* users to actually checkout the damn thing and go through its code. I think this is something I would really want to test out with my team. I have a junior coming soon to my team (it's his first project and employment ever, he's 18), and I will test this approach with him and see how it works before I propose it to some of my peers and seniors.
1. `git log --first-parent --pretty=oneline` will show you merge commits but not the commits that were merged. If your merge commits have non-default messages, this effectively makes for a "flat" log in which CR commits don't appear. Unfortunately not all git GUIs support this (e.g. github).
2. If a comment genuinely isn't actionable with a code change take the discussion to another forum (email, slack, or for recording, the issue tracker). Then once the design disagreement is resolved, go back to using CR comments.
> In the case it is or it really needs an explanation it stays in the codebase, otherwise you take it, e.g. to GitHub issues or PR comments.
Another thing that's easy, but often overlooked: putting relevant URLs in comments. The best comments explain why some approach was taken; linking to a discussion on JIRA, or the Stack Overflow answer we c̶o̶p̶y̶-̶p̶a̶s̶t̶e̶d̶ ̶f̶r̶o̶m̶ used as inspiration, is (a) quicker than writing a whole explanation, and (b) gives better evidence of what's going on (e.g. that the CTO explicitly asked for this-or-that; or that the SO question now has a better answer; etc.)
I prefer clean code that doesn’t have comments except as doc strings for various parsers on declarations and in areas that are particularly surprising.
If I want to know why something was done, I expect to be able to use git log or git blame on the file or line and get find a commit message, issue tracker link or pull request reference that I can pursue.
But then a single minor refactor means you have to dig through history after puzzling over something for long enough to decide that it's definitely no obvious instead of just seeing an inline explanation (or at least a link to one).
I've never gotten the point of squash-merge. The normal "merge into release branch, never fast-forward" workflow retains more useful information. Every release commit has 2 parents: the previous release commit, and the former head of the feature branch it got its changes from. If you want a chain of releases, you go down the path of commits labeled "RELEASE". If you want information on the changes, go down the other path.
I regularly use "git blame" on lines of code to try and figure out what the specific reasoning was on a line of code. I'm going to have a very different interpretation depending on if the resulting commit says "implement feature xyz" vs "fix issue abc on feature xyz".
Squash murders that added context. At best you're optimizing for the wrong thing, and worst it's outright vandalism.
I assumed the code review commits would be squashed before the merge. But you raise a valid point about the code review process being lost. One way to get around that is to keep the feature branches (or maybe just tags) forever and do all the squashing on the integration branch. This does require some kind of convention that records on which branch/tag the code review for a particular feature can be found.
The advantage of this kind of approach is it doesn't require any special tooling, but could certainly be made easier with special tooling.
Or just don't squash. There's no need. In fact, if you're doing a review process like this, it would be active vandalism to to squash, since there is a big difference between "code said x, commented on by reviewer, changed to y" vs "code says y".
If you don't squash then you're essentially dedicating the history to code reviews because it won't be much use for anything else. If you leave broken commits on the main branch then it becomes really hard to bisect to find regressions. And that's the only reason for keeping history really.
Of course, that becomes a non-issue if you use a squash merge approach, instead of retain all commits (which in this approach have a lot more of a "WIP" feel to it).
Do code review comments have value after merging? I would also posit they're easier to find if they're in an external location (like github/lab, gerrit, etc), unless you also wrote tools that can figure out which commits and comments belong to a certain feature.