I could see this being useful if there was a script, that moved the comments out into either the commit message or a separate file.
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.
Honestly the average level of git skills, doesn't allow such a workflow.
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)
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 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.
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.
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.
Thanks for sharing!
https://youtu.be/MUqvXHEjmus Transcript: https://www.janestreet.com/tech-talks/janestreet-code-review...
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.
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.
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.)
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.
Sometimes even longer :)
is my all time favorite.
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.
The advantage of this kind of approach is it doesn't require any special tooling, but could certainly be made easier with special tooling.
Solves that issue entirely. Let branches hold their history, let the release/main branch hold the history of stable builds.
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.
1. Broken intermediate commits hamper git bisect.
2. Fixup commits are noisy in git log, git blame, etc.
Note: there's zero issue with NON-BROKEN, NON-FIXUP commits. Separating large changes is even preferable for the reasons you stated.
---
tl;dr Broken intermediate commits are bad; whole intermediate commits are good.
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.
Nice posts about it:
https://drewdevault.com/2018/07/02/Email-driven-git.html
https://drewdevault.com/2022/07/25/Code-review-with-aerc.htm...
This is a nice tutorial, too, but I would recommend reading Drew Devault's posts first (and watching his short videos):
So the reviewer receives the commits, with their descriptions, authors, everything. Just like pulling a branch, really.
Source: https://git-scm.com/docs/git-format-patch#_base_tree_informa...
A question I have with the process is: can email based workflow still work with for a continuous integration/delivery pipeline?
Doesn't sourcehut already support that? (https://man.sr.ht/builds.sr.ht/)
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?
SourceHut has a CI feature integrated with the mailing-lists: https://man.sr.ht/builds.sr.ht/
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'.
How do you deal with conflicts between multiple reviewers?
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.
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.
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.
> 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?
But on a team, I’d say there is no Most Valuable Player.
• 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.
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.
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.
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.
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.
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 easily think of several with talented somewhat thorny, highly opinionated sr level engineers where this would be a problem. I've literally had to have a talk with someone on the spectrum who would occasionally do this very thing (ie. just push up a commit w/ a variety of fixes) because he felt it was more expedient than explaining in a PR. The person who complained felt like they were being bullied.
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?
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.
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".
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)
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.
- 2023-08-10.00.notes.md
- 2023-08-10.01.notes.md
- 2023-08-11.00.changelog.md
- 2023-08-11.01.changelog.md
- 2023-08-11.00.review.md
- 2023-08-11.01.review.md
- 2023-08-11.02.review.md
- 2023-08-11.00.project.md
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?
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.
Sure, there's still discussion when needed, but otherwise take it as a collaborative effort where you're asynchronously pair-programming.
[1] https://chelseatroy.com/2019/12/18/reviewing-pull-requests/
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?