I have this "one weird trick" that I think is great, but somehow no-one else wants to use:
At one point I worked with a fellow dev where we did PR reviews just as commits on top of the branch. So you'd check out their feature branch, run the equivalent of 'compare working tree with <main>' for your editor and then look through the changes.
Because you're comparing the working tree you can leave comments... by typing source code comments (we used '// RVW(jauco)' to mark them). You can fix typo's just by, well, fixing the typo and you share the review by pushing to the branch.
I found it to have the following advantages
- PR comments usually break when you rebase or merge (because they reference a line number). These obviously won't, at worst there will be a merge conflict that you handle the obvious way.
- If your PR comment requests explanation then the answer needs to be moved from the PR to the codebase, for future use. Writing comments in code means that you just need to clean up the conversation into a comment (or you can just leave it)
- You can use a proper text editor for writing the comments
- You can use code navigation while reading the changes (like, go to definition etc)
- Because of these the review gets a more active feel. You're more inclined to engage with the code rather then just reading it while leaning back in your chair
However, people tend to look at me like I'm crazy for suggesting something like this. OTOH tools like this look completely crazy to me. Why do all this work to get a worse version of something you already have?
Have you ever tried the git e-mail workflow? I think you may like it, I'd be curious to have your opinion about how it compares to your workflow above :-).
The idea is that you just send your patch by e-mail, and the reviewer just applies it with `git am <the patch>`. So you don't even have to sync on a branch. Then the reviewer can comment directly on the patch. You answer with new versions of the patch, and get new reviews. It's minimal, actually decentralized (unlike, say, GitHub), and I believe it has the advantages of your workflow above.
I have been giving this one a lot of thought. I jumped to sourcehut for my personal projects and was amazed how viscerally pleasing a [PATCH] email thread could be. I am convinced there is a big psychological difference between discussions via Pull Requests and discussions via email/mailing lists. I can’t pin point it but it is almost palpable.
A question I have with the process is: can email based workflow still work with for a continuous integration/delivery pipeline?
Same :-). I also love how people don't have to create an account to send a patch. They just need to send me an e-mail. That is not a problem on GitHub (because everybody is forced to have an account there), but it is if you want to host your repo somewhere else.
> can email based workflow still work with for a continuous integration/delivery pipeline?
The biggest drawback is the inability to use on a computer without email setup, e.g. not from the main computer, or from mobile devices - smartphone, tablet. In my case it's not uncommon to do a quick review of some PRs using this Web-based workflow. Now with newer GitHub application it's even easier.
You don't send a diff, you send an e-mail formatted by git. So you get all the commits you put in it, it's really just like seeing the commits in a GitHub PR.
So the reviewer receives the commits, with their descriptions, authors, everything. Just like pulling a branch, really.
You do lose the information about where to apply the patch. This isn't a big problem for rebase-friendly crowd. But people who prefer the merge strategy strictly may not like it.
AFAICS there's `--base=auto` which captures explicit parent's sha in the patch text output (there's also a git config option to do that automatically).
Thank you for the information! This is the first time I'm hearing about it. Would you happen to know how this information is used? Is it just for the maintainer, or is there a tool that can use it? (I couldn't find any other reference)
It is really easier than it looks! Took me a while to try it, but then I realized it's really super easy: run the git command to create the e-mail, send the e-mail, and on the reviewer side just run the command to apply it.
This is a nice tutorial, too, but I would recommend reading Drew Devault's posts first (and watching his short videos):
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.
I’ve recently listened to a talk from Jane Street about their review system: they use this method exactly with a helper tool that keeps track of what you’ve already reviewed in the PR, minus any changes that were committed after review. PR author also “reviews” the branch, so the code review comments and changes would show up on self-re-review (because they were not reviewed).
The core problem with this workflow is that it's rigid around doing things The One True Way. The fact that you have a specific comment delimiter is a good example of that. I feel like this doesn't really have any upsides compared to, say, reviewing code on Github. If you want you can always check out the branch and inspect it locally, but you also have the option of doing it all on the web without stashing your changes if the change is not large. Many online review tools also support go to definition, suggesting code changes inline, etc. All of this means more flexibility, everyone can review the way that works for them, not just The One True Way.
Hmm I don't fully agree with this. By using the PR workflow on GitHub, your One True Way is that people need to create an account on GitHub and use their web interface to make the review.
You always have to enforce some kind of conventions, but I wouldn't say GitHub is the most flexible. With an e-mail workflow, everyone can use the e-mail client they want and review the patches the way they want, without being forced to sign up on a website at all.
You can do reviews in a separate clone, to avoid interrupting any changes you're making (reading the diff only needs a 'git fetch', so won't interfere with anything; but making changes is easiest with a full check-out).
You could use 'git worktree' to manage that; but I'm happy with 'git clone' and 'rm -r'.
I also used to do this in a couple of small startups. I loved having the review history just stored in git, and how commits that fix a problem also delete the comment about it, so when reviewing fixes I make sure every newly deleted comment is taken care of, then git grep "CR:" and if it comes back empty we're good to go.
However, I got a feeling it wouldn't scale well to large teams. Specifically because you have to manually keep very strict about not mixing reviewed and unreviewed code, and if you do it's hard to disentangle them and decide what's in scope for a review.
Along the same lines: when I want to locally review someone’s PR, I want to see their changes as “dirty” relative to main, so that the changes show up nicely highlighted in PyCharm in the “gutter” (left side). This has to be such a common need, but I am surprised that I have to do convoluted things like: create a new branch, switch to it, and then:
git merge --no-commit --no-ff pr-branch
Am I missing an easier workflow/command to achieve the above?
(Granted this is not so bad, and was suggested by GPT4, but I wondered if there might be a more straightforward way)
Here's a script I use for this. It's designed for use with github. Run it in your copy of a repo with a PR number and it will create a 'review' branch that has the changes uncommitted. Works great with IDEs with a good diff experience.
We did that at a previous job too. I also thought it seemed a bit weird at the beginning (too simple?), but I loved it. Being able to do reviews from the comfort of magit instead of having to deal with a janky web GUI was great.
As long as there's an agreed upon comment format, it's really easy for the reviewee to find all the comment and adress them one by one.
I don't remember any drawbacks really, but we were a small team and the reviews were mostly "one on one".
In GH you'd click "resolve" to mark a discussion as finished. Here you just remove the comments to do the same (they're still in the git history, so you don't loose them forever)
This doesn't need to be an extra step. If the comment is something like: "potential null pointer here" you can just fix the bug and remove the comment in one go, the reviewer will see what you changed from their comment commit so you don't loose information.
In some cases the discussion is in the code until right before the merge when the PR author removes the block of comments. Often by summarising it into one comment for future readers who might have the same questions. But that's not more work than resolving comments in github. Not that you're looking through the changes anyway and your editor is open at the change _in edit mode_ so reading and editing isn't two separate steps.
When we did this, I think we had some git hooks to make sure that no comments in "review form" were left when merging. So you'd have to go through all the comments and at least remove them.
I'm going one-up you in craziness, and suggest Chelsea Troy's idea[1] that you don't bother commenting - after checking out the PR locally you just make the changes and then update the PR.
Sure, there's still discussion when needed, but otherwise take it as a collaborative effort where you're asynchronously pair-programming.
That sounds like a great idea! Difficult to see any significant downsides. OTOH checking out a branch and doing a commit involves slightly more friction that just clicking some links. I probably wouldn't bother if I had GitHub or Gitlab set up. But if I was starting a new company and didn't have them set up yet this sounds like genius.
I guess you can do a similar thing with issues just by having a folder of markdown files for each issue and appending comments to them.
If some conventions could be agreed you could have GUI support.
It's not that hard if you use an IDE. I review work this way all the time. It involves:
1. Click the fetch icon. New work to review shows up.
2. Right click to compare the branches, take a look through the diffs.
3. If I see something I want to check, click "Checkout" in the branch selector and start making the changes, commit, push.
4. Otherwise when done, use git merge and delete the origin branch (I have some shell functions for this).
One thing I've found hard to settle on is what message to use in the merge commits. I don't insist all changes have tickets because it'd just be redundant work for the case of small cleanups, fixing bugs you spotted whilst doing other stuff. But then the default merge commit message isn't terribly helpful, and writing one that is can end up redundant.
Another problem is that the railway lines can get quite hairy if people aren't disciplined about constantly rebasing. You can of course rebase for them, but then you lose the ability to view only the merge commits to master.
To request a review you push the changes into a special branch namespace in the reviewer's repository. IntelliJ uses / as a way to organize branches into folders, so it's easy to see what review requests come from who. You can also inform them in other ways (CCing on a ticket, email, chat, etc).
We use a kernel model in which there is no shared repository, so it's impossible for branches to be merged without being reviewed because it's the reviewer who merges them when they're satisfied.
> You can fix typo's just by, well, fixing the typo... You're more inclined to engage with the code rather then just reading it...
This is all the value, isn't it?
Anything that forces the "reviewer" to become an "editor" suddenly massively increases the value of the "review."
People like the GitHub workflow because... then they don't have to do that work.
This is fine. I'm not sure how worse developers become better ones, except by fixing their mistakes. So if you're a huge corporation and you have the luxury to have a lot of redundant, in-training employees, many fresh out of school with no useful experience, great, reviews by senior people, so long as the junior person's code isn't actually written for them, can transmit some training value.
> However, people tend to look at me like I'm crazy for suggesting something like this
If you're a tiny startup and you have a junior developer, you've sort of already screwed up haven't you? Either you want to train them, and that's a huge mistake because you don't have the bandwidth for that. Or you do their work for them, which is like, why have them around at all then?
You're not crazy and not alone. At Hydraulic we do the exact same thing. It has other advantages you didn't mention:
• It yields much more collaborative code reviews, because for small tweaks it's quicker to just fix the thing you want changed than ask someone else to do it. After all, you've already got the code checked out and in your IDE. This is especially true when working with easily refactorable languages like Java or Kotlin and IDEs with great diff viewing support like IntelliJ, because if you don't like the symbol name someone picked you just change it. No need for a round-trip. This is huge because it stops reviewers/reviewees getting exhausted from "nitpicking" and fosters teamwork.
• You can see the IDE's static analysis outputs, which you won't see in a web based PR viewer. This can often let you spot bugs that might otherwise escape notice.
• You can easily run the unit tests, or add a quick test if you notice it's missing. If it fails, no problem, just commit the failing test, push and let the author know that they have more work to do.
To implement this workflow we use Gitolite on our "HQ" server that we use for everything. Everyone gets their own UNIX user and their own personal git clone of the product repos. Access controls are in force, so if you want someone to review something you push it into their repo but it has to be under a branch named rr/$PUSHER_USERNAME/change-name. So your PR queue is visible in your IDE's git branch explorer. Do a fetch and new work shows up to review, add commits or merge it and then delete the branch to close the review. The merge preserves the history of the review session.
We have it wired up to TeamCity and YouTrack (yup, I'm a JetBrains fan). Everyone also gets their own private TeamCity project. The combination of this means that devs can push private branches to get things nice and clean in CI without disturbing anyone else or polluting their UIs - there are no "draft PRs" or other self-contradictory concepts in this workflow. Ownership of branches is always crystal clear also, even for experimental branches or projects. Once a PR is merged into the tech lead's master branch, YouTrack notices the commit has gone green there and parses issue update commands out of the commit message, finally updating or closing the ticket automatically.
Note that this setup is cheap for small teams. TC and YouTrack are free for small setups, Gitolite is open source, all you need is a server. We pay 90 EUR a month for a dedicated machine at Hetzner that has 128 GB of RAM and 2TB NVMe flash, and everyone gets a UNIX user, so if you want to do remote dev on it or host random files you can do so and it's extremely fast.
Socially this arrangement is kernel-like. The tech lead of the project runs the repo from which releases are done and merge in from teammates, which in turn can merge in from their teammates and so on. Ownership is always clear because the act of merging is also the act of taking responsibility. You can't get a tragedy of the commons where juniors keep picking other juniors to review their work like you can in more conventional free-for-alls.
"it's quicker to just fix the thing you want changed than ask someone else to do it"
Sure, but I feel like there's a big disadvantage here in that the person who did the bad thing isn't actively involved in fixing it. They lose out on acquiring that muscle memory. If this is a Sr. reviewing another Sr.'s PR and it's just a few minor oversights, fine. But if this is a Sr. reviewing a Jr.'s PR and they fix like 10 things in one commit, that seems harmful to the inexperienced dev's development. They don't even get a chance to ask why or provide clarification.
And if it comes off like a bunch of nitpicking, that's a person who likely really needs a little handholding. The whole "give a man a fish vs teach" proverb.
You can certainly pick which approach to use based on your wider goals, sure. Sometimes though changes are a matter of style, or something is easier to show rather than tell, or you realize whilst reviewing the requirements weren't quite right (on you) etc.
For example, if a junior isn't familiar enough with the standard library and misses a trick, you can try to explain in words what the better way is, or you can just fix it and then ask them to study the diff. I don't know what's better for long term learning, but at least you have the option.
You can do a "show don't tell" with a fix suggestion in a Github PR, which one accept as-is or can use for inspiration. I feel like in the majority of cases doing something for someone is a missed opportunity - esp. again in the situation where there are many. A lot of those aren't going to stick for most people if they're simply quickly reviewing it. It also seems like an opportunity for certain personality types to run roughshod over engineers they don't respect.
Yes GitHub is slowly reinventing IDEs in their code review tools. If you do it in the IDE with real commits you can use refactoring aids, search+replace etc.
If someone keeps making the same errors and isn't paying attention to the fixes you can just talk about it with them in a meeting, same as with any other recurring problem you might have with a teammate.
The model we implement is hierarchical, so there's no running roughshod over other people by design. If the reviewer isn't happy they won't merge, and you have to keep going until they are. But, it's also on the reviewer to get the work in because they're being held accountable for delivery too, and they can make changes with the same tools as the authors. So it's a collaboration.
What I mean by running roughshod is now it's incumbent on an engineer with lesser power to overwrite changes they don't agree with or understand. The chance of them doing that is going to be low unless the change is obviously wrong. One has to appreciate this power disparity on a team of mixed skill levels.
I've been doing this for 26 years at 15 companies and have led several teams. I can think of some where this would be fine (ie. everyone has a healthy amount of EQ), but I can think of several with talented but somewhat thorny, opinionated sr level engineers where this would be a problem. I once had to have a talk with someone on the spectrum who would occasionally do this very thing because it was more expedient than explaining in a PR. The person who complained felt like they were being disrespected.
> Socially this arrangement is kernel-like. The tech lead of the project runs the repo from which releases are done and merge in from teammates, which in turn can merge in from their teammates and so on. Ownership is always clear because the act of merging is also the act of taking responsibility. You can't get a tragedy of the commons where juniors keep picking other juniors to review their work like you can in more conventional free-for-alls.
I think the way you work is very interesting. We once discussed about having a process that is more kernel-like.
The fact that everyone has their own remote clone of the (or all) git repositories, does that not introduce overhead? We did that long time ago when we started with git, but thought it introduced extra overhead without any benefit. I assume you do it to make the ownership clear? And I also assume that everyone in your team is very comfortable with git? (yes, I do think developers should know their tools inside-out, but that is sadly rarely the case)
Can you explain in more detail the steps for a code review process? Is it the author that creates and deletes the review branch in the clone of the reviewer? How does the author know that the reviewer finished the review? Is it always the author who pushes his code to the technical lead for merge into master?
There's no overhead because the forks are done serverside and hosted on the same machine. Git knows how to use hard links to rapidly do local clones, so disk space isn't wasted.
Yes, it makes ownership clear and prevents code from being merged without being reviewed. GitHub offers CODEOWNERS files but often ownership doesn't map neatly to source code layout (and nor should it).
Most devs have not been comfortable enough with git to do this when they first joined but they learned quickly enough, and I helped them learn. The operations needed aren't that complicated. For example you don't need to do rebases in this workflow. It's just branching and pushing.
I should write up a proper blog post on the workflow. The code review author creates the branch by pushing into the reviewer's repository. The reviewer deletes the branch once they merge it. The author knows the review was finished because the code either gets merged, or it gets another commit on top that adds requests for changes. The branch in the reviewer's repository is where collaboration happens once the review process starts.
I have been thinking about this approach, because I heard other people doing exactly this.
I am using a pull request flow currently, but have some annoyances:
- I nearly always check out the branch to review (I use a separate worktree with git worktree for this) so that I can explore the code and changes using my IDE; it is annoying to have to switch back and forth between IDE and PR interface to add comments;
- Sometimes I want to do a global review of some part of the code. This is typically not supported out of the box with review systems based on PRs. Sometimes you can work around that by creating dummy branches, but it is annoying;
- Sometimes there are interesting explanations but those are typically "lost"; yes, they are still somewhere in the PR system that was used at the time, in some pull request, tied to a commit hash that is no longer there, because the feature branch was rebased before merge; seriously, once the PR is merged everything that was commented and discussed is lost in practical terms;
So, yeah, I am tempted to try this approach with leaving comments in the code for the author of the branch. I believe it would also make the code review process more efficient.
A similar approach I've been using recently and am so far really, really enjoying is to put all "conversation" into a dedicated "devlog" directory in the project root. We put everything in it: meeting notes, project status updates, memos, changelogs for every pull request, reviews, etc. Every file is markdown and named with a date prefix, a "nonce" for uniqueness, and a category (date.nonce.category.md). So you might have something like:
The file naming includes a small amount of ambiguity and a small opportunity for merge conflicts but in practice it's just not a problem. The best part of this workflow IMO is that it doesn't create a whole new workflow for communication, it treats high-quality async communication as part of the core (pull, read, commit, push, review) development workflow.
Just from your comment, I can see:
* People don't get notifications about new review requests across dozens of repos
* No good way to just get a list of remaining comments (I guess you grep the diff for RVW?)
* Hard to set up "require all comments to be resolved before merging"
These could be automated easily with scripts and git hooks though, then it would be a pretty good flow.
You're not alone, I do that too sometimes! But I've also found that most people have a hard time doing or following along with something like that. Possibly because in the end it does require a whole lot of skill in using git, because you will usually want to rewrite history in some ways, and large numbers of developers are very uncomfortable with that.
I think this has some drawbacks because it seems to require a lot of discipline from all the parties, to keep the source code clean once you are ready to merge and even if you have some presubmit checks if a human makes a mistake and has some typo in the comment, that might end up in the main branch. Ideally all the new changes are small but that's not always possible and the larger the change the higher the probability to leave comments around. I agree it's not the end of the world but such things do add up over time and if you are in a big shop things will definitely get messy, it's just a matter of time.
Wow, that's really cool. If these commits were marked one could quickly drop them during rebate at the end of the review before merging. Or just remove them when the fix is applied.
Which I can imagine could be annoying in some situations :-). But no workflow is perfect.
I think I like the idea that in an e-mail, I can really comment on some patch, and then the author of the patch can decide what to do with it. Maybe the author modified the patch in the meantime, but they will know if my comment is still valid or not. Without me having to handle the conflicts.
I'm thinking mostly about leads who have a lot to review. They probably don't want to have to fix conflicts in comments all the time.
I completely agree, if you don't make many reviews and if it does not happen often.
But still it puts the burden on the reviewer rather than on the patch author. And the reviewer's time is never less precious. If you need a review from Linus Torvalds, I think it makes sense to make it as seamless as possible for him. And that is what the e-mail workflow does, I would say.
It depends on what you value in a review. For me, a review is for me to get a glimpse at new parts of the code and learn what is coming. It is important that I understand the WHY more than the WHAT. I also put code reviews as the highest priority item in my todo list. I will always do a code review next, if there is one to do. If you send me a request, you'll have a review from me before the end of my day.
Why? Because unblocking my teammates is the most important thing I can do, and I expect the same from my teammates. I never want the reason our team is late to be "I had more important things to do" because (cliché incoming), "the is no 'I' in 'team'".
A reviewers time isn't more valuable than mine, and my time is no more valuable than anyone else (assuming accountants don't exist). If changing 3 lines is really an issue, then create a merge-tool for this workflow and add it to your git configuration file.
Sure, but what if your lead has a review list so big that they almost never get to empty it totally? I would consider that the time of such a lead is definitely more valuable than mine. Again, it's easy for me to accept that Linus Torvalds' time is more valuable than mine.
> If changing 3 lines is really an issue, then create a merge-tool for this workflow and add it to your git configuration file.
Or use the e-mail workflow, where the lead can just comment on the patch right away, without having to even pull the branch (and worse, push comment and potentially fix conflicts), right?
No, no git magic. Just source code changes. One commit with the whole review that contains fixes and maybe a few questions. Then a commit from the PR author on top of that that adds explanations and/or fixes for the questions.
Maybe the key required for this is that you have reviews where, if code prompts a question, then more often then not the code should change. At the very least to explain briefly why this approach was chosen over another approach.
If you have code reviews from developers with differing skill levels and the response to the question is just an explanations how the language or framework works then this might become a bit cumbersome (because you have to leave the comment to reply and then remove the whole discussion before merging)
> No, no git magic. Just source code changes. One commit with the whole review that contains fixes and maybe a few questions. Then a commit from the PR author on top of that that adds explanations and/or fixes for the questions.
Reviews on some projects will ask you to make a commit/patch in between your third and fourth (out of seven). And to fix a typo in the fifth commit message. And do fix something in the second commit that leads to conflicts in the next commits. And then to end up with for example seven commits that each “do one thing” or whatever. So if the expectation is that you end up with one “fixed up“ commit at the end of the review process which encapsulates the whole PR then this won't work.
With your approach I imagine going in several rounds where
1. Reviewers leave comments
2. I address them
3. I publish that as review round X
4. I “clean up“ the history (get rid of comments/all review history) and address things like “make a doc commit before commit number 4”
5. Publish that as the “blank slate” (no comments) for the next round
6. Invite the reviewers to the next round
And that could work.
Something more dynamic/less rigorous (with rounds) and you might end up with an excessive amount of history rewriting and conflict resolution.
I think your approach to code review is in general a good idea but don't underestimate the differences in skill level in the average project. I often work with domain experts who are not developers or beginners who are still learning basic git concepts and how to navigate the code base.
At one point I worked with a fellow dev where we did PR reviews just as commits on top of the branch. So you'd check out their feature branch, run the equivalent of 'compare working tree with <main>' for your editor and then look through the changes.
Because you're comparing the working tree you can leave comments... by typing source code comments (we used '// RVW(jauco)' to mark them). You can fix typo's just by, well, fixing the typo and you share the review by pushing to the branch.
I found it to have the following advantages
- PR comments usually break when you rebase or merge (because they reference a line number). These obviously won't, at worst there will be a merge conflict that you handle the obvious way.
- If your PR comment requests explanation then the answer needs to be moved from the PR to the codebase, for future use. Writing comments in code means that you just need to clean up the conversation into a comment (or you can just leave it)
- You can use a proper text editor for writing the comments
- You can use code navigation while reading the changes (like, go to definition etc)
- Because of these the review gets a more active feel. You're more inclined to engage with the code rather then just reading it while leaning back in your chair
However, people tend to look at me like I'm crazy for suggesting something like this. OTOH tools like this look completely crazy to me. Why do all this work to get a worse version of something you already have?