9 ms·

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

by jauco 3y ago

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?

joachimma 3y ago
If you leave the comments, doesn't the code get cluttered? If you remove the comments, don't you lose information? 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.
- jauco 3y ago
  Oh, yeah I didn't explain that part. 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.
  - nurbl 3y ago
    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.
FPGAhacker 3y ago
What did you do with all the comments at the end of the review?
- F-W-M 3y ago
  If they are in there own commits, you just drop them. Honestly the average level of git skills, doesn't allow such a workflow.
  - jauco 3y ago
    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)
    - okl 3y ago
      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.
    - avgcorrection 3y ago
      > 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.
IshKebab 3y ago
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.
- mike_hearn 3y ago
  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.
  - IshKebab 3y ago
    How do you request a review and ensure that branches are reviewed before merging?
    - mike_hearn 3y ago
      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.
vander_elst 3y ago
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.
seeekr 3y ago
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.
Corsome 3y ago
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. Thanks for sharing!
sevazhidkov 3y ago
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). https://youtu.be/MUqvXHEjmus https://youtu.be/MUqvXHEjmus Transcript: https://www.janestreet.com/tech-talks/janestreet-code-review/ https://www.janestreet.com/tech-talks/janestreet-code-review...
SonOfLilit 3y ago
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.
Cthulhu_ 3y ago
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.
- mike_hearn 3y ago
  A couple of ways to mitigate that: 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.
  - epolanski 3y ago
    This is a very interesting point you make and I fully agree with: If a comment genuinely isn't actionable with a code change. 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.
    - chriswarbo 3y ago
      > 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.)
palata 3y ago
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. Nice posts about it: https://drewdevault.com/2018/07/02/Email-driven-git.html https://drewdevault.com/2018/07/02/Email-driven-git.html https://drewdevault.com/2022/07/25/Code-review-with-aerc.html https://drewdevault.com/2022/07/25/Code-review-with-aerc.htm...
- palata 3y ago
  I'd love to know why I have been downvoted on that...
  - wordpad25 3y ago
    the answer is in one of the first lines in your linked article "Email isn’t as sexy as GitHub"
- jauco 3y ago
  You know, I never really tried that. But I will do so when the opportunity arrives!
  - palata 3y ago
    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): https://git-send-email.io/ https://git-send-email.io/
- worldsayshi 3y ago
  Don't you lose a bit of important information when you create a patch? Like which commit it started from.
Etheryte 3y ago
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.
- palata 3y ago
  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.
- chriswarbo 3y ago
  > without stashing your changes 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'.
palata 3y ago
> you share the review by pushing to the branch. How do you deal with conflicts between multiple reviewers?
- withinboredom 3y ago
  The same way you deal with any other conflicts, you won't be able to push without pulling first.
  - palata 3y ago
    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.
    - withinboredom 3y ago
      Deleting three lines to keep both comments doesn’t seem too bad. You might even just delete your comment entirely if it’s for the same thing.
      - palata 3y ago
        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.
        withinboredom 3y ago
        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.
mike_hearn 3y ago
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.
nurbl 3y ago
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".
d4rkp4ttern 3y ago
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)
- xvilka 3y ago
  I recommend trying b4[1] tool. [1] https://b4.docs.kernel.org/en/latest/index.html https://b4.docs.kernel.org/en/latest/index.html
- foundart 3y ago
  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. https://github.com/whenceforth/local-code-review https://github.com/whenceforth/local-code-review
- jauco 3y ago
  In vs code you check out the pr branch and then do cmd-shift-p “show differences with” and then select “main”
  - d4rkp4ttern 3y ago
    I don’t use vscode, but does this show the diffs in the gutter within the IDE ? Or does it simply display the diffs in a separate pane?
rvdginste 3y ago
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.
killthebuddha 3y ago
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: - 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 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.
doctorpangloss 3y ago
> 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?
imoreno 3y ago
Neat. What drawbacks have you seen? 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.
jasonpeacock 3y ago
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. [1] https://chelseatroy.com/2019/12/18/reviewing-pull-requests/ https://chelseatroy.com/2019/12/18/reviewing-pull-requests/

by jauco 3y ago

joachimma 3y ago

If you leave the comments, doesn't the code get cluttered? If you remove the comments, don't you lose information? 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.

jauco 3y ago
Oh, yeah I didn't explain that part. 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.
- nurbl 3y ago
  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.

FPGAhacker 3y ago

What did you do with all the comments at the end of the review?

F-W-M 3y ago
If they are in there own commits, you just drop them. Honestly the average level of git skills, doesn't allow such a workflow.
- jauco 3y ago
  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)
  - okl 3y ago
    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.
  - avgcorrection 3y ago
    > 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.

IshKebab 3y ago

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.

mike_hearn 3y ago
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.
- IshKebab 3y ago
  How do you request a review and ensure that branches are reviewed before merging?
  - mike_hearn 3y ago
    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.

vander_elst 3y ago

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.

seeekr 3y ago

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.

Corsome 3y ago

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. Thanks for sharing!

sevazhidkov 3y ago

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). https://youtu.be/MUqvXHEjmus https://youtu.be/MUqvXHEjmus Transcript: https://www.janestreet.com/tech-talks/janestreet-code-review/ https://www.janestreet.com/tech-talks/janestreet-code-review...

SonOfLilit 3y ago

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.

Cthulhu_ 3y ago

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.

mike_hearn 3y ago
A couple of ways to mitigate that: 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.
- epolanski 3y ago
  This is a very interesting point you make and I fully agree with: If a comment genuinely isn't actionable with a code change. 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.
  - chriswarbo 3y ago
    > 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.)

palata 3y ago

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. Nice posts about it: https://drewdevault.com/2018/07/02/Email-driven-git.html https://drewdevault.com/2018/07/02/Email-driven-git.html https://drewdevault.com/2022/07/25/Code-review-with-aerc.html https://drewdevault.com/2022/07/25/Code-review-with-aerc.htm...

palata 3y ago
I'd love to know why I have been downvoted on that...
- wordpad25 3y ago
  the answer is in one of the first lines in your linked article "Email isn’t as sexy as GitHub"
jauco 3y ago
You know, I never really tried that. But I will do so when the opportunity arrives!
- palata 3y ago
  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): https://git-send-email.io/ https://git-send-email.io/
worldsayshi 3y ago
Don't you lose a bit of important information when you create a patch? Like which commit it started from.

Etheryte 3y ago

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.

palata 3y ago
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.
chriswarbo 3y ago
> without stashing your changes 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'.

palata 3y ago

> you share the review by pushing to the branch. How do you deal with conflicts between multiple reviewers?

withinboredom 3y ago
The same way you deal with any other conflicts, you won't be able to push without pulling first.
- palata 3y ago
  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.
  - withinboredom 3y ago
    Deleting three lines to keep both comments doesn’t seem too bad. You might even just delete your comment entirely if it’s for the same thing.
    - palata 3y ago
      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.
      - withinboredom 3y ago
        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.

mike_hearn 3y ago

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.

nurbl 3y ago

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".

d4rkp4ttern 3y ago

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)

xvilka 3y ago
I recommend trying b4[1] tool. [1] https://b4.docs.kernel.org/en/latest/index.html https://b4.docs.kernel.org/en/latest/index.html
foundart 3y ago
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. https://github.com/whenceforth/local-code-review https://github.com/whenceforth/local-code-review
jauco 3y ago
In vs code you check out the pr branch and then do cmd-shift-p “show differences with” and then select “main”
- d4rkp4ttern 3y ago
  I don’t use vscode, but does this show the diffs in the gutter within the IDE ? Or does it simply display the diffs in a separate pane?

rvdginste 3y ago

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.

killthebuddha 3y ago

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: - 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 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.

doctorpangloss 3y ago

> 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?

imoreno 3y ago

Neat. What drawbacks have you seen? 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.

jasonpeacock 3y ago

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. [1] https://chelseatroy.com/2019/12/18/reviewing-pull-requests/ https://chelseatroy.com/2019/12/18/reviewing-pull-requests/