13 ms·

Code review can be better

391 points by sealeck 1y ago

hydroxideOH- 1y ago
I use the GitHub Pull Request extension in VSCode to do the same thing (reviewing code locally in my editor). It works pretty well, and you can add/review comments directly in the editor.
- cebert 1y ago
  I use this a lot too. Also, if you open a PR on the GitHub website and press the “.” key, it opens the review in VSCode, which I consider a much better web experience.
  - reilly3000 1y ago
    TIL thanks.
- ivanjermakov 1y ago
  It's better, but still quite deep vendor lock-in (in both GitHub and VSCode).
  - hydroxideOH- 1y ago
    Well my employer chooses to use GitHub so I don’t have a choice there. And it’s vendor lock-in VSCode but that’s already my primary editor so it means there’s no need to learn another tool just for code review.
  - NortySpock 1y ago
    GitHub may be dominant, but it's not like it doesn't have competitors nipping at its heels (GitLab, BitBucket come to mind). VSCode is open source, and there are plenty of IDEs... I guess I'm just focused on different lock-in concerns than you are.
  - cyberax 1y ago
    JetBrains IDEs can do the same.
    - plonq 1y ago
      Unfortunately it’s not feature complete - you can’t paste images in review comments, for example. Still very useful for large PRs though.
- deleted 1y ago
  [deleted]
- pm90 1y ago
  Same! Its much nicer now especially since Github seems to be pretty arbitrary/rigid about when it hides files that have "too many changes". Its so much nicer to see/navigate around such changes quickly in VSCode vs trying to do the same in the web interface. I suspect that since this is possible with VSCode/Github, its probably extensible to other providers editors.
ivanjermakov 1y ago
I find the idea of using git for code reviews directly quite compelling. Working with the change locally as you were the one who made it is very convenient, considering the clunky read-only web UI. I didn't get why stick with the requirement that review is a single commit? To keep git-review implementation simple? I wonder if approach where every reviewer commits their comments/fixes to the PR branch directly would work as well as I think it would. One might not even need any additional tools to make it convenient to work with. This idea seems like a hybrid of traditional github flow and a way Linux development is organized via mailing lists and patches.
- spike021 1y ago
  is github's PR considered read-only? i've had team members edit a correction as a "suggestion" comment and i can approve it to be added as a commit on my branch.
  - ivanjermakov 1y ago
    By read-only I meant that you can't fully interact with the code: run/debug it, use intellisense, etc.
    - spike021 1y ago
      Can't you just check out the branch of the repo locally into your ide? i'm still confused what limitation you are talking about.
      - ivanjermakov 1y ago
        Described well in the post. This way you have to switch between ide and web diff viewer, redundant and not convenient.
- teiferer 1y ago
  > I didn't get why stick with the requirement that review is a single commit Yeah that is pretty weird. If 5 people review my code, do they all mangle the same review commit? We don't do that with code either, feels like it's defeating the point. Review would need to be commits on top of the reviewed commit. If there are 5 reviews of the same commit, then they all branch out from that commit. And to address them, there is another commit which also lives besides them. Each commit change process becomes a branch with stacked commits beinf branches chained on top of one another. Each of the commits in those chained branches then has comment commits attached. Those comment commits could even form chains if a discussion is happening. Then when everybody is happy, each branch gets squashed into a single commit and those then get rebased on the main branch. You likely want to make new commits for that though to preserve the discussions for a while. And that's the crux: That data lives outside the main branch, but needs to live somewhere.
MutedEstate45 1y ago
Agree with your pain points. One thing id add is GitHub makes you reapprove every PR after each push. As an OSS contributor it’s exhausting to chase re-approvals for minor tweaks.
- Ar-Curunir 1y ago
  Hm that’s not the case for my repositories? Maybe you have a setting enabled for that?
- pie_flavor 1y ago
  This is a security setting that the author has chosen to enable.
- irjustin 1y ago
  mmmm this is up to each repo/maintainer's settings. To be fair you don't know if one line change is going to absolutely compromise a flow. OSS needs to maintain a level of disconnect to be safe vs fast.
  - MutedEstate45 1y ago
    Good to know! Never been a maintainer before so I thought that was required.
  - o11c 1y ago
    Adding fixup commits (specifying the specific commit they will be squashed into), to be squashed by the bot before merge, handles that.
faangguyindia 1y ago
Essentially, you are turning fork/branch induced changes to "precommit" review like workflow which is great. I was on a lookout for best "precommit" review tool and zeroed on Magit, gitui, Sublime Merge. I am not an emac user, so i'll have to learn this.
- xeonmc 1y ago
  In theory this functionality would be best suited as a git subcommand. I suggest `git-precom` for conciseness.
  - faangguyindia 1y ago
    Git already has `git add -p` but demands a lot from user.
    - Pxtl 1y ago
      Git demands a lot from user in general.
koolba 1y ago
> When I review code, I like to pull the source branch locally. Then I soft-reset the code to mere base, so that the code looks as if it was written by me. This is eerily similar to how I review large changes that do not have a clear set of commits. The real problem is working with people that don’t realize that if you don’t break work down into small self contained units, everybody else is going to have to do it individually. Nobody can honestly say they can review tons of diffs to a ton of files and truly understand what they’ve reviewed. The whole is more than just the sum of the parts.
- stitched2gethr 1y ago
  For those that want an easy button. Here ya go. ``` review () { if [[ -n $(git status -s) ]] then echo 'must start with clean tree!' return 1 fi git checkout pristine # a branch that I never commit to git rebase origin/master branch="$1" git branch -D "$branch" git checkout "$branch" git rebase origin/master git reset --soft origin/master git reset nvim -c ':G' # opens neovim with the fugitive plugin - replace with your favorite editor git reset --hard git status -s | awk '{ print $2 }' | xargs rm git checkout pristine git branch -D "$branch" } ```
  - 000ooo000 1y ago
    Having a PR worktree is good with this kind of workflow.
- cedws 1y ago
  Crafting good commits, and good PRs out of those commits is a skill just like how writing good code is. Unfortunately, too many people suck at the former.
  - Maxion 1y ago
    This does also tie in directly with tickets and the overall workflow the team has. I find this to have a huge effect on how managable PRs are. I feel the majority of devs are quite oblivious to the code they produce, they simply keep coding untill they fill the acceptence criteria. No matter if the result is 200 lines in 1 file, or 1 000 lines in 30 files.
tomasreimers 1y ago
Just taking a step back, it is SO COOL to me to be reading about stacked pull requests on HN. When we started graphite.dev years ago that was a workflow most developers had never heard of unless they had previously been at FB / Google. Fun to see how fast code review can change over 3-4yrs :)
- jacobegold 1y ago
  hell yeah
- foota 1y ago
  I miss the fig workflow :-(
  - kyrra 1y ago
    Try `jj`, as others have mentioned. It's being built by the team that built/maintains fig, and the are porting all their learnings into that.
    - ndr 1y ago
      jj is cool solo, but it doesn't seem of much help when maintaining a stack of PRs neatly updated on github
      - abound 1y ago
        It requires a bit of scripting between the `gh` CLI and `jj`, but it's totally doable to maintain even complex stacks of PRs on GitHub with jj. One thing I've found at $DAYJOB is that I have to set the PR's "base" branch to "main" before I push updated commits (and then switch it back to the parent after), otherwise CI thinks my PR contains everything on main and goes nuts emailing half the company to come review it.
        ndr 1y ago
        Is there something that does this? I've played with git town which is great for what it is. But at $DAYJOB we are now all on graphite and that stacking is super neat. The web part is frustratingly slow, but they got stacking working really well.
loeg 1y ago
I've used Reviewboard and Phabricator and both seem "fine" to me. Superior to Github (at the time, anyway).
moonlion_eth 1y ago
ersc.io
- loeg 1y ago
  Say more.
- citizenpaul 1y ago
  I got into using Jujutsu this year. I'm liking it so far. Is there a beta access in the works?
- toastal 1y ago
  Shame it’s Jujutsu & not something based on actual Patch Theory (patches are commutative). I think Patch Theory is one of the ways out of merge conflict hell.
jacobegold 1y ago
It's so cool that Git is considering first class change IDs!! That's huge! This sounds similar to what we had at Facebook to track revisions in Phabricator diffs. Curious if anyone knows the best place to read about this?
- 3036e4 1y ago
  The fundamental problem is that git doesn't track branches in any sane way. Maybe it would be better to fix that? Fossil remembers what branch a commit was committed on, so the task branch itself is a change ID. That might be tricky to solve while also allowing git commands to mess with history of course. Fossil doesn't have that problem.
gatane 1y ago
>remote-first web-interface https://youtu.be/Qscq3l0g0B8 https://youtu.be/Qscq3l0g0B8
shmerl 1y ago
I was recently looking for something that at least presents a nice diff that resembles code review one in neovim. This is a pretty cool tool for it: https://github.com/sindrets/diffview.nvim https://github.com/sindrets/diffview.nvim On the branch that you are reviewing, you can do something like this: :DiffviewOpen origin/HEAD...HEAD
godelski 1y ago
While I like the post and agree with everything the author talked about I find that this is not my problem. Despite having a similar workflow (classic vim user). The problem I have and I think a lot of others have too is that review just doesn't actually exist. LGTMs are not reviews, yet so common. I'm not sure there's even a tech solution to this class of problems and it is down to culture. LGTMs exist because it satisfies the "letter of the law" but not the spirit. Classic bureaucracy problem combined with classic engineer problems. It feels like there are simple solutions but LGTMs are a hack. You try to solve this by requiring reviews but LGTMs are just a hack to that. Fundamentally you just can't measure the quality of a review[0]. Us techie types and bureaucrats have a similar failure mode: we like measurements. But a measurement of any kind is meaningless without context. Part of the problem is that businesses treat reviewing as a second class citizen. It's not "actual work" so shouldn't be given preference, which excuses the LGTM style reviews. Us engineers are used to looking at metrics without context and get lulled into a false sense of security, or convince ourselves that we can find a tech solution to this stuff. I'm sure someone's going to propose a LLM reviewer and hey, it might help, but it won't address the root problems. The only way to get good code reviews is for them to be done by someone capable of writing the code in the first place. Until the LLMs can do all the coding they won't make this problem go away, even if they can improve upon the LGTM bar. But that's barely a bar, it's sitting on the floor. The problem is cultural. The problem is that code reviews are just as essential to the process as writing the code itself. You'll notice that companies that do good code review already do this. Then it is about making this easier to do! Reducing friction is something that should happen and we should work on, but you could make it all trivial and it wouldn't make code reviews better if they aren't treated as first class citizens. So while I like the post and think the tech here is cool, you can't engineer your way out of a social problem. I'm not saying "don't solve engineering problems that exist in the same space" but I'm making the comment because I think it is easy to ignore the social problem by focusing on the engineering problem(s). I mean the engineering problems are magnitudes easier lol. But let's be real, avoiding addressing this, and similar, problems only adds debt. I don't know what the solution is[1], but I think we need to talk about it. [0] Then there's the dual to LGTM! Code reviews exist and are detailed but petty and overly nitpicky. This is also hacky, but in a very different way. It is a misunderstanding of what review (or quality control) is. There's always room for criticism as nothing you do, ever, will be perfect. But finding problems is the easy part. The hard part is figuring out what problems are important and how to properly triage them. It doesn't take a genius to complain, but it does take an expert to critique. That's why the dual can even be more harmful as it slows progress needlessly and encourages the classic nerdy petty bickering over inconsequential nuances or over unknowns (as opposed to important nuances and known unknowns). If QC sees their jobs as finding problems and/or their bosses measure their performance based on how many problems they find then there's a steady state solution as the devs write code with the intentional errors that QC can pick up on, so they fulfill their metric of finding issues, and can also easily be fixed. This also matches the letter but not the spirit. This is why AI won't be able to step in without having the capacity of writing the code in the first place, which solves the entire problem by making it go away (even if agents are doing this process). [1] Nothing said here actually presents a solution. Yes, I say "treat them as first class citizens" but that's not a solution. Anyone trying to say this, or similar things, is a solution is refusing to look at all the complexities that exist. It's as obtuse as saying "creating a search engine is easy. All you need to do is index all (or most) of the sites across the web." There's so much more to the problem. It's easy to over simplify these types of issues, which is a big part of why they still exist.
back2dafucha 1y ago
[flagged]
kjgkjhfkjf 1y ago
If you want to remain relevant in the AI-enabled software engineering future, you MUST get very good at reviewing code that you did not write. AI can already write very good code. I have led teams of senior+ software engineers for many years. AI can write better code than most of them can at this point. Educational establishments MUST prioritize teaching code review skills, and other high-level leadership skills.
- ZYbCRq22HbJ2y7 1y ago
  > AI can already write very good code Debatable, with same experience, depends on the language, existing patterns, code base, base prompts, and complexity of a task
  - netghost 1y ago
    How about AI can write large amounts of code that might look good out of context.
    - ZYbCRq22HbJ2y7 1y ago
      Yeah, LLMs can do that very well, IMO. As an experienced reviewer, the "shape" of the code shouldn't inform correctness, but it can be easy to fall into this pattern when you review code. In my experience, LLMs tend to conflate shape and correctness.
      - dragonwriter 1y ago
        > As an experienced reviewer, the "shape" of the code shouldn't inform correctness, but it can be easy to fall into this pattern when you review code. For human written code, shape correlates somewhat with correctness, largely because the shape and the correctness are both driven by the human thought patterns generating the code. LLMs are trained very well at reproducing the shape of expected outputs, but the mechanism is different than humans and not represented the same way in the shape of the outputs. So the correlation is, at best, weaker with the LLMs, if it is present at all. This is also much the same effect that makes LLMs convincing purveyors of BS in natural language, but magnified for code because people are more used to people bluffing with shape using natural language, but churning out high-volume, well-shaped, crappy substance code is not a particularly useful skill for humans to develop, and so not a frequently encountered skill. And so, prior to AI code, reviewers weren't faced with it a lot.
Areibman 1y ago
The biggest grip I have with Github is the app is painfully slow. And by slow, I mean browser tab might freeze level slow. Shockingly, the best code review tool I've ever used was Azure DevOps.
- wenc 1y ago
  When I worked at a Microsoft shop, I used Azure DevOps. To be honest, it's actually not bad for .NET stuff. It fits the .NET development life cycle like Visual Studio fits C#.
- awesome_dude 1y ago
  nit: gripe, not grip :-P
- echelon 1y ago
  > The biggest grip I have with Github is the app is painfully slow. And by slow, I mean browser tab might freeze level slow. Javascript at scale combined with teams that have to move fast and ship features is a recipe for this. At least it's not Atlassian.
  - lmm 1y ago
    Stash (now BitBucket Server) had the best code review going, head and shoulders above GitHub to the point I thought GitHub would obviously adopt their approach. But I imagine Atlassian has now made it slow and useless like they do with all their products and acquisitions.
    - dotancohen 1y ago
      Bit Bucket had a git-related tool called Stash? I love Bit Bucket, but I'm glad I did not know about that.
      - lmm 1y ago
        There was a locally-hosted Git server platform called Stash. Atlassian bought it, rebranded it as "BitBucket Server" (positioned similarly to GitHub Enterprise or self-hosted GitLab) and gradually made it look and feel like BitBucket (the cloud product), even though they're actually completely separate codebases (or at least used to be).
shayief 1y ago
Gitpatch attempts to solve this. Supports versioned patches and patch stacks (aka stacked PRs). Also handles force-pushes in stacks correctly even without Change-IDs using heuristics based on title, author date etc. It should also be unusually fast. Disclosure: I'm the author. I'm not convinced that review comments as commits make thing easier, but I think storing them in git in some way is a good idea (i.e. git annotations or in commit messages after merge etc)
kissgyorgy 1y ago
putting the review into git notes might have worked better. It's not attached to tje lines directly, but the commit and it can stay as part of the repo
jbmsf 1y ago
Recently, I've been wondering about the point of code review as a whole. When I started my career, no one did code review. I'm old. At some point, my first company grew; we hired new people and started to offshore. Suddenly, you couldn't rely on developers having good judgement... or at least being responsible for fixing their own mess. Code review was a tool I discovered and made mandatory. A few years later, everyone converged on GitHub, PRs, and code review. What we were already doing now became the default. Many, many years layer, I work with a 100% remote team that is mostly experienced and 75% or more of our work is writing code that looks like code we've already written. Most code review is low value. Yes, we do catch issues in review, especially with newer hires, but it's not obviously worth the delay of a review cycle. Our current policy is to trust the author to opt-in for review. So far, this approach works, but I doubt it will scale. My point? We have a lot of posts about code review and related tools and not enough about whether to review and how to make reviews useful.
- deleted 1y ago
  [deleted]
- Feeble 1y ago
  I am very much in the same position right now. My dev team has introduced mandatory code reviews for every change and I can see their output plummeting. It also seems that most code reviews done are mostly syntax and code format related - noone actually seems to run the code or look at the actual logic if it makes sense. I think its easy to add processes under the good intention of "making the code more robust and clean", but I never heard anyone discuss what is the cost of this process to the team's efficiency.
  - mgaunard 1y ago
    You need to validate things like syntax upfront so that such things don't make it to review to begin with. I'm not a fan of automatic syntax formatting but you can have some degree of pre-commit checks.
  - alkonaut 1y ago
    The value of having more people actually see the code will be there even if it’s just an unnecessary syntax nitpick.
6LLvveMx2koXfwn 1y ago
> But modifying code under review turned out to be tricky. GitLab enables this - make the suggestion in-line which the original dev can either accept or decline.
- globular-toast 1y ago
  Kind of. Don't you have to type the change into the browser? Which means your change might not even be syntactically correct. It would be far better if you could make the change locally then somehow and that straight to GitLab. Also how does it work with multiple commits? Which commit does it amend?
pjmlp 1y ago
I never did proper code review, other than when being lucky that we got a team of top devs in specific projects. More often than not, it either doesn't exist, or turns out in a kind of architecture fetishism that the lead devs/architects have from conferences or space ship enterprise architecture. Already without this garbage it feels so much better, than arguing about SOLID, clean code, hexagonal architecture, member functions being with an underscore, explicit types or not,...
_kidlike 1y ago
me and my team have been doing code reviews purely within IntelliJ, for something like 6 years. We started doing it "by hand", by checking out the branch and comparing with master, then using Github for comments. Now there's official support and tooling for reviews (at least in IDEA, but probably in the others too), where you also get in-line highlighting of changed lines, comments, status checks, etc... I feel sorry for anyone still using GitHub itself (or GitLab or whatever). It's horrible for anything more than a few lines of changes here and there.
3036e4 1y ago
What bothered me for a long time with code reviews is that almost all useful things they catch (i.e. not nit-picking about subjective minor things that doesn't really matter) are much too late in the process. Not rarely the only (if any) useful outcome of a review is that everything has to be done from scratch in a different ways (completely new design) or that it is abandoned since it turns out it should never have been done at all. It always seems as if the code review is the only time when all stakeholders really gets involved and starts thinking about a change. There may be some discussion earlier on in a jira ticket or meeting, and with some luck someone even wrote a design spec, but there will still often be someone from a different team or distant part of the organization that only hears about the change when they see the code review. This includes me. I often only notice that some other team implemented something stupid because I suddenly get a notification that someone posted a code review for some part of the code that I watch for changes. Not that I know how to fix that. You can't have everyone in the entire company spend time looking at every possible thing that might be developed in the near future. Or can you? I don't know. That doesn't seem to ever happen anyway. At university in the 1990's in a course about development processes there wasn't only code reviews but also design reviews, and that isn't something I ever encountered in the wild (in any formal sense) but I don't know if even a design review process would be able to catch all the things you would want to catch BEFORE starting to implement something.
- epolanski 1y ago
  > and that isn't something I ever encountered in the wild (in any formal sense) Because in the software engineering world there is very little engineering involved. That being said, I also think that the industry is unwilling to accept the slowliness of the proper engineering process for various reasons, including non criticality of most software and the possibility to amend bugs and errors on the fly. Other engineering fields enjoy no such luxuries, the bridge either holds the train or it doesn't, you either nailed the manufacturing plant or there's little room for fixing, the plane's engine either works or not Different stakes and patching opportunities lend to different practices.
  - ozim 1y ago
keniu 1y ago
we need code review, do not let AI control us.
aitchnyu 1y ago
Tangential, long ago I wanted to use a repo-backed (IIRC tied to Mercurial) backend for issues. They were also flat files. We put too many plugins into Redmine and it died frequently.
germandiago 1y ago
I am happy with Gerrit but I am sure I do not know even how to use 20% of its capacity. The patchsets get stacked up and you know where you left off if there are different changes and that is very cool.
cbryant91 1y ago
good fine
brabel 1y ago
Author has not tried IDE plugin for GitHub / Bitbucket reviews!? We review code directly in IntelliJ with full support for commenting, navigation, even merging without leaving the IDE. Solves all problems the author has.
maunke 1y ago
Very nice to read. Sourcehut is missing in the list; it’s built on the classical concept of sending patches / issues / bugs / discussion threads via email and it integrates this concept into its mailing lists and ci solution that also sends back the ci status / log via email. Drew Devault published helpful resources for sending and reviewing patches via email on git-send-email.io and git-am.io
eafkuor 1y ago
> Alas, when I want to actually leave feedback on the PR, I have to open the browser, navigate to the relevant line in the diff, and (after waiting for several HTTP round-trips) type my suggestion into a text area This doesn't seem like much of a problem, does it? It's a matter of alt-tab and a click or two. Also, what is the point of having reviews in the git history?
liampulles 1y ago
This is how I learned to do code review when I was a new junior dev. I would write my review comments on another junior's code, and then our team lead would go write their comments that we missed, and then both of us juniors would read and see what we missed. It was a good way to learn about coding and reviewing I think.
liampulles 1y ago
Here's an alternative I've wondered about: Instead of one person writing code, and another reviewing it - instead you have one person write the first pass and then have another person adjust it and merge it in. And vice-versa; the roles rotate. Anyone tried something like this? How did it go?
- thinkindie 1y ago
  That’s basically an async pair programming session, isn’t it?
  - liampulles 1y ago
    Yes I think so. Have you tried it?
    - thinkindie 1y ago
      Just cases where PR were submitted close to someone holidays and I assigned it to someone else in the team to bring it over the line. But otherwise I have worked with sync pair programming only.
- mattikl 1y ago
  I've noticed for a long time that if I have participated in writing the code under review, I'm able to provide much more insight. I think what you're suggesting starts from thinking the code as "our code" instead of my code vs. your code, which so easily happens with pull requests. And learning to work iteratively instead of trying to be too perfect from the start, which goes well with methodologies like TDD.
- romanovcode 1y ago
  Great idea, if you're fine with development time to take twice as long.
  - ramon156 1y ago
    The alternative is a merge that potentially has more bugs. Trunk based is definitely not twice as long, rather 1.5x on average
codeman001 1y ago
I use CodeRabbit that helps, but it does not fix the two root issues. I run their free VS code plugin to review local commits first, which catches nits, generates summaries, and keeps me in my editor. The PR bot then adds structure so humans focus on design and invariants. Review state still lives in the forge, not in Git, and interdiffs still depend on history. If Git gets a stable Change-Id, storing review metadata in Git becomes realistic. Until then this is a pragmatic upgrade that reduces friction without changing the fundamental. https://www.coderabbit.ai/ide https://www.coderabbit.ai/ide
z3t4 1y ago
Leave the comments in the commit messages and make many small commits! That way they don't change the actual source and they're specific for that version of the code.
sgt 1y ago
Articles from Tigerbeetle I tend to just upvote and then click on the link. I just know it's going to be quality stuff.
bkolobara 1y ago
I have been working on the PR implementation for lubeno[1] and have been thinking a lot about the code review process. A big issue is that every team has a slightly different workflow, with different rules and requirements. The way GitHub is structured is a result of how the GitHub team works. They built the best tool for themselves with their "just keep appending commits to a PR" workflow. Either you need to have enough flexibility so that the tool can be adapted to everyone's existing workflow. Or you need to be opinionated about your workflow (GitHub) and force everyone to match it in some way. And in most cases this works very well, because people just want you to tell them the best way of doing things and not spend time figuring out what the best workflow would look like. [1]: https://lubeno.dev https://lubeno.dev
icy 1y ago
Worth mentioning that Tangled has support for stacked pull requests and a unique round-based PR flow with interdiffing: https://blog.tangled.sh/stacking https://blog.tangled.sh/stacking
mike_hearn 1y ago
I've used a more hard-core version of this in my own company and have been meaning to write a blog post about it for years. For now an HN comment will suffice. Here's my version, the rationale and the findings. WORKFLOW Every repository is personal and reviewer merges, kernel style. Merging is taking ownership: the reviewer merges into their own tree when they are happy and not before. By implication there is always one primary code reviewer, there is never a situation where someone chooses three reviewers and they all wait for someone else to do the work. The primary reviewer are on the hook for the deliverable as much as the reviewee is. There is no web based review tool. Git is managed by a server configured with Gitolite. Everyone gets their own git repository under their own name, into which they clone the product repository. Everyone can push into everyone else's repos, but only to branches matching /rr/{username}/something and this is how you open a pull request. Hydraulic is an IntelliJ shop and the JetBrains git UI is really good, so it's easy to browse open RRs (review requests) and check them out locally. Reviewing means pushing changes onto the rr branch. Either the reviewer makes the change directly (much faster than nitpicky comment roundtrips), or they add a //FIXME comment that IntelliJ is configured to render in lurid yellow and purple for visibility. It's up to the reviewee to clear all the FIXMEs before a change will be merged. Because IntelliJ is very good at refactoring, what you find is that reviewers are willing to make much bigger improvements to a change than you'd normally get via web based review discussions. All the benefits the article discusses are there except 100x because IntelliJ is so good at static analysis. A lot of bugs that sneak past regular code review are caught this way because reviewers can see live static analysis results. Sometimes during a review you want to ask questions. 90% of the time, this is because the code isn't well documented enough and the solution is to put the question in a //FIXME that's cleared by adding more comments. Sometimes that would be inappropriate because the conversation would have no value to others, and it can be resolved via chat. Both reviewee and reviewer are expected to properly squash and rebase things. It's usually easier to let commits pile up during the review so both sides have state on the changes, and the reviewer then squashes code review commits into the work before merging. To keep this easy most review requests should turn into one or two commits at most. There should not be cases where people are submitting an RR with 25 "WIP" commits that are all tangled up. So it does require discipline, but this isn't much different to normal development. RATIONALE 1. Conventional code review can be an exhausting experience, especially for junior developers who make more mistakes. Every piece of work comes back with dozens of nitpicky comments that don't seem important and which is a lot of drudge work to apply. It leads to frustration, burnout and interpersonal conflicts. Reviewees may not understand what is being asked of them, resulting in wasted time. So, latency is often much lower if the reviewer just makes the changes directly in their IDE and pushes. People can then study the commits and learn from them. 2. Conventional projects can struggle to scale up because the codebase becomes a commons. Like in a communist state things degrade and litter piles up, because nobody is fully responsible. Junior developers or devs under time pressure quickly work out who will give them the easiest code review experience and send all the reviews to them. CODEOWNERS are the next step, but it's rare that the structure of your source tree matches the hierarchy of technical management in your organization so this can be a bad fit. Instead of improving widely shared code people end up copy/pasting it to avoid bringing in more mandatory reviewers. It's also easy for important but rarely changed directories to be left out, resulting in changes to core code slowing down because it'd require the founder of the company to approve a trivial refactoring PR. FINDINGS Well, it worked well for me at small scale (decent sized codebase but a small team). I never scaled it up to a big team although it was inspired by problems seen managing a big team. Because most questions are answered by improving code comments rather than replying in a web UI the answers can help LLMs. LLMs work really well in my codebase and I think it's partly due to the plentiful documentation. Sometimes the lack of a web UI for browsing code was an issue. I experimented with using IntelliJ link format, but of course not everyone wants to use IntelliJ. I could have set up a web UI over git just for source browsing, without the full GitHub experience, but in the end never bothered. Gitolite is a very UNIXy set of Perl scripts. You need a gray beard to use it well. I thought about SaaSifying this workflow but it never seemed worth it.
kungfufrog 1y ago
Anyone know what editor the author is using in the first screenshot showing two panels side by side?
- 000ooo000 1y ago
  Looks like VSCode in macOS, maybe with some custom CSS.
- wry_discontent 1y ago
  They mention magit, which makes me think Emacs, but it looks like there's lot of custom UI stuff.
- l2dy 1y ago
  Probably VS Code with https://github.com/kahole/edamagit https://github.com/kahole/edamagit
sc68cal 1y ago
This brings back memories of https://opendev.org/ttygroup/gertty https://opendev.org/ttygroup/gertty when I was contributing to OpenStack
ramon156 1y ago
I'm of the hot opinion that a reviewer shouldn't be running code. The one making the code is responsible for the fix, code reviews are just about maintainability. If your PR did not fix the issue or implement the feature, that's on you, not the reviewer.
- OskarS 1y ago
  I don't know about "shouldn't", I think it's fine if they do. But I basically agree, at some fundamental level, you have to have some trust in your coworkers. If someone says "This fixes X", and they haven't even tried running it or testing it, they shouldn't be your coworker. The purpose of code reviews shouldn't be "is this person honest?" or "is this person totally incompetent?". If they're not, it's a much bigger issue, one that shouldn't be dealt with through code reviews. Very different situation if it's open source or an external contribution, of course.
- allknowingfrog 1y ago
  The author mentioned that he doesn't want to make suggestions that don't actually work. That seems like a pretty valid reason to run the code.
elisemaya31 1y ago
[dead]
actinium226 1y ago
I like the idea of max 500 lines for custom solutions to problems that have "existing" solutions, might have to steal that.
jFriedensreich 1y ago
Its pretty clear to a growing number of devs what a review tool should look like. It is more a matter of what needs to happen so this becomes a usable and sustainable reality and what shape of organisation/ players can make this happen in the right way. - git itself wont go much further than the change-id which is already a huge win (thanks to jj, git butler, gerrit and other teams) - graphite and github clearly showed they are not interested in solving this for anyone but their userslaves and have obviously opposing incentives. - there are dozens of semi abandoned cli tools trying this without any traction, a cli can be a part of a solution but is just a small part What we need: - usable fully local - core team support for vscode not just a broken afterthought by someone from the broader community - web UI for usecases where vscode does not fit (possibly via vscode web or other ways to reuse as much of the interface work that went into the vscode integration) - the core needs to be usable from a cli or library with clear boundaries so other editor teams can build as great integrations as the reference but fitting their native ui concepts - it needs to work for commits, branches, stacked commits and any snapshot an agent creates as well as reviewing a devs own work before pushing - it needs to incorporate CI/CD signals natively, meta did great UI work on this and its crucial to not ignore all that progress but build on top of it - it needs to be as fine grained as the situation requires and with editability at every step. Why can i just accept one line in cursor but there is nothing like that when reviewing a humans code? Why can i fix a typo without any effort when reviewing in cursor when i have to go through at least 5 clicks to do the same when fixing a typo of a human. - It needs to by fully incremental, when a pr is fixed there needs to be a simple way to review just the fix and not re-review the whole pr or the full file
octodoctor 1y ago
[dead]
deterministic 1y ago
I have never (in my 30+ years career) worked for a company that required formal code reviews. And yet have managed to deliver a ton of commercially successful software. I am pretty sure that adding a review step would have slowed me down tremendously. Without adding any commercial value. However I can imagine that code reviews would work well for inexperienced developers being reviewed by more experienced developers?
- phendrenad2 1y ago
  At least in the US, code reviews are essentially mandated by legal compliance in publicly-traded companies. The law (SOX) says something like "no one person can destroy the company by making an engineering change", so viola, code review was invented. Private companies can probably get away with not doing code reviews, but often private companies want to pass security reviews like SOC 2. > I am pretty sure that adding a review step would have slowed me down tremendously This is very true. Everyone should work on a personal side-project at some point and realize just how much code review slows things down.

391 points by sealeck 1y ago

I use the GitHub Pull Request extension in VSCode to do the same thing (reviewing code locally in my editor). It works pretty well, and you can add/review comments directly in the editor.

cebert 1y ago
I use this a lot too. Also, if you open a PR on the GitHub website and press the “.” key, it opens the review in VSCode, which I consider a much better web experience.
- reilly3000 1y ago
  TIL thanks.
ivanjermakov 1y ago
It's better, but still quite deep vendor lock-in (in both GitHub and VSCode).
- hydroxideOH- 1y ago
  Well my employer chooses to use GitHub so I don’t have a choice there. And it’s vendor lock-in VSCode but that’s already my primary editor so it means there’s no need to learn another tool just for code review.
- NortySpock 1y ago
  GitHub may be dominant, but it's not like it doesn't have competitors nipping at its heels (GitLab, BitBucket come to mind). VSCode is open source, and there are plenty of IDEs... I guess I'm just focused on different lock-in concerns than you are.
- cyberax 1y ago
  JetBrains IDEs can do the same.
  - plonq 1y ago
    Unfortunately it’s not feature complete - you can’t paste images in review comments, for example. Still very useful for large PRs though.
deleted 1y ago
[deleted]
pm90 1y ago
Same! Its much nicer now especially since Github seems to be pretty arbitrary/rigid about when it hides files that have "too many changes". Its so much nicer to see/navigate around such changes quickly in VSCode vs trying to do the same in the web interface. I suspect that since this is possible with VSCode/Github, its probably extensible to other providers editors.

ivanjermakov 1y ago

I find the idea of using git for code reviews directly quite compelling. Working with the change locally as you were the one who made it is very convenient, considering the clunky read-only web UI. I didn't get why stick with the requirement that review is a single commit? To keep git-review implementation simple? I wonder if approach where every reviewer commits their comments/fixes to the PR branch directly would work as well as I think it would. One might not even need any additional tools to make it convenient to work with. This idea seems like a hybrid of traditional github flow and a way Linux development is organized via mailing lists and patches.

spike021 1y ago
is github's PR considered read-only? i've had team members edit a correction as a "suggestion" comment and i can approve it to be added as a commit on my branch.
- ivanjermakov 1y ago
  By read-only I meant that you can't fully interact with the code: run/debug it, use intellisense, etc.
  - spike021 1y ago
    Can't you just check out the branch of the repo locally into your ide? i'm still confused what limitation you are talking about.
    - ivanjermakov 1y ago
      Described well in the post. This way you have to switch between ide and web diff viewer, redundant and not convenient.
teiferer 1y ago
> I didn't get why stick with the requirement that review is a single commit Yeah that is pretty weird. If 5 people review my code, do they all mangle the same review commit? We don't do that with code either, feels like it's defeating the point. Review would need to be commits on top of the reviewed commit. If there are 5 reviews of the same commit, then they all branch out from that commit. And to address them, there is another commit which also lives besides them. Each commit change process becomes a branch with stacked commits beinf branches chained on top of one another. Each of the commits in those chained branches then has comment commits attached. Those comment commits could even form chains if a discussion is happening. Then when everybody is happy, each branch gets squashed into a single commit and those then get rebased on the main branch. You likely want to make new commits for that though to preserve the discussions for a while. And that's the crux: That data lives outside the main branch, but needs to live somewhere.

MutedEstate45 1y ago

Agree with your pain points. One thing id add is GitHub makes you reapprove every PR after each push. As an OSS contributor it’s exhausting to chase re-approvals for minor tweaks.

Ar-Curunir 1y ago
Hm that’s not the case for my repositories? Maybe you have a setting enabled for that?
pie_flavor 1y ago
This is a security setting that the author has chosen to enable.
irjustin 1y ago
mmmm this is up to each repo/maintainer's settings. To be fair you don't know if one line change is going to absolutely compromise a flow. OSS needs to maintain a level of disconnect to be safe vs fast.
- MutedEstate45 1y ago
  Good to know! Never been a maintainer before so I thought that was required.
- o11c 1y ago
  Adding fixup commits (specifying the specific commit they will be squashed into), to be squashed by the bot before merge, handles that.

faangguyindia 1y ago

Essentially, you are turning fork/branch induced changes to "precommit" review like workflow which is great. I was on a lookout for best "precommit" review tool and zeroed on Magit, gitui, Sublime Merge. I am not an emac user, so i'll have to learn this.

xeonmc 1y ago
In theory this functionality would be best suited as a git subcommand. I suggest `git-precom` for conciseness.
- faangguyindia 1y ago
  Git already has `git add -p` but demands a lot from user.
  - Pxtl 1y ago
    Git demands a lot from user in general.

koolba 1y ago

> When I review code, I like to pull the source branch locally. Then I soft-reset the code to mere base, so that the code looks as if it was written by me. This is eerily similar to how I review large changes that do not have a clear set of commits. The real problem is working with people that don’t realize that if you don’t break work down into small self contained units, everybody else is going to have to do it individually. Nobody can honestly say they can review tons of diffs to a ton of files and truly understand what they’ve reviewed. The whole is more than just the sum of the parts.

stitched2gethr 1y ago
For those that want an easy button. Here ya go. ``` review () { if [[ -n $(git status -s) ]] then echo 'must start with clean tree!' return 1 fi git checkout pristine # a branch that I never commit to git rebase origin/master branch="$1" git branch -D "$branch" git checkout "$branch" git rebase origin/master git reset --soft origin/master git reset nvim -c ':G' # opens neovim with the fugitive plugin - replace with your favorite editor git reset --hard git status -s | awk '{ print $2 }' | xargs rm git checkout pristine git branch -D "$branch" } ```
- 000ooo000 1y ago
  Having a PR worktree is good with this kind of workflow.
cedws 1y ago
Crafting good commits, and good PRs out of those commits is a skill just like how writing good code is. Unfortunately, too many people suck at the former.
- Maxion 1y ago
  This does also tie in directly with tickets and the overall workflow the team has. I find this to have a huge effect on how managable PRs are. I feel the majority of devs are quite oblivious to the code they produce, they simply keep coding untill they fill the acceptence criteria. No matter if the result is 200 lines in 1 file, or 1 000 lines in 30 files.

tomasreimers 1y ago

Just taking a step back, it is SO COOL to me to be reading about stacked pull requests on HN. When we started graphite.dev years ago that was a workflow most developers had never heard of unless they had previously been at FB / Google. Fun to see how fast code review can change over 3-4yrs :)

jacobegold 1y ago
hell yeah
foota 1y ago
I miss the fig workflow :-(
- kyrra 1y ago
  Try `jj`, as others have mentioned. It's being built by the team that built/maintains fig, and the are porting all their learnings into that.
  - ndr 1y ago
    jj is cool solo, but it doesn't seem of much help when maintaining a stack of PRs neatly updated on github
    - abound 1y ago
      It requires a bit of scripting between the `gh` CLI and `jj`, but it's totally doable to maintain even complex stacks of PRs on GitHub with jj. One thing I've found at $DAYJOB is that I have to set the PR's "base" branch to "main" before I push updated commits (and then switch it back to the parent after), otherwise CI thinks my PR contains everything on main and goes nuts emailing half the company to come review it.
      - ndr 1y ago
        Is there something that does this? I've played with git town which is great for what it is. But at $DAYJOB we are now all on graphite and that stacking is super neat. The web part is frustratingly slow, but they got stacking working really well.

loeg 1y ago

I've used Reviewboard and Phabricator and both seem "fine" to me. Superior to Github (at the time, anyway).

moonlion_eth 1y ago

ersc.io

loeg 1y ago
Say more.
citizenpaul 1y ago
I got into using Jujutsu this year. I'm liking it so far. Is there a beta access in the works?
toastal 1y ago
Shame it’s Jujutsu & not something based on actual Patch Theory (patches are commutative). I think Patch Theory is one of the ways out of merge conflict hell.

jacobegold 1y ago

It's so cool that Git is considering first class change IDs!! That's huge! This sounds similar to what we had at Facebook to track revisions in Phabricator diffs. Curious if anyone knows the best place to read about this?

3036e4 1y ago
The fundamental problem is that git doesn't track branches in any sane way. Maybe it would be better to fix that? Fossil remembers what branch a commit was committed on, so the task branch itself is a change ID. That might be tricky to solve while also allowing git commands to mess with history of course. Fossil doesn't have that problem.

gatane 1y ago

>remote-first web-interface https://youtu.be/Qscq3l0g0B8 https://youtu.be/Qscq3l0g0B8

shmerl 1y ago

I was recently looking for something that at least presents a nice diff that resembles code review one in neovim. This is a pretty cool tool for it: https://github.com/sindrets/diffview.nvim https://github.com/sindrets/diffview.nvim On the branch that you are reviewing, you can do something like this: :DiffviewOpen origin/HEAD...HEAD

godelski 1y ago

While I like the post and agree with everything the author talked about I find that this is not my problem. Despite having a similar workflow (classic vim user). The problem I have and I think a lot of others have too is that review just doesn't actually exist. LGTMs are not reviews, yet so common. I'm not sure there's even a tech solution to this class of problems and it is down to culture. LGTMs exist because it satisfies the "letter of the law" but not the spirit. Classic bureaucracy problem combined with classic engineer problems. It feels like there are simple solutions but LGTMs are a hack. You try to solve this by requiring reviews but LGTMs are just a hack to that. Fundamentally you just can't measure the quality of a review[0]. Us techie types and bureaucrats have a similar failure mode: we like measurements. But a measurement of any kind is meaningless without context. Part of the problem is that businesses treat reviewing as a second class citizen. It's not "actual work" so shouldn't be given preference, which excuses the LGTM style reviews. Us engineers are used to looking at metrics without context and get lulled into a false sense of security, or convince ourselves that we can find a tech solution to this stuff. I'm sure someone's going to propose a LLM reviewer and hey, it might help, but it won't address the root problems. The only way to get good code reviews is for them to be done by someone capable of writing the code in the first place. Until the LLMs can do all the coding they won't make this problem go away, even if they can improve upon the LGTM bar. But that's barely a bar, it's sitting on the floor. The problem is cultural. The problem is that code reviews are just as essential to the process as writing the code itself. You'll notice that companies that do good code review already do this. Then it is about making this easier to do! Reducing friction is something that should happen and we should work on, but you could make it all trivial and it wouldn't make code reviews better if they aren't treated as first class citizens. So while I like the post and think the tech here is cool, you can't engineer your way out of a social problem. I'm not saying "don't solve engineering problems that exist in the same space" but I'm making the comment because I think it is easy to ignore the social problem by focusing on the engineering problem(s). I mean the engineering problems are magnitudes easier lol. But let's be real, avoiding addressing this, and similar, problems only adds debt. I don't know what the solution is[1], but I think we need to talk about it. [0] Then there's the dual to LGTM! Code reviews exist and are detailed but petty and overly nitpicky. This is also hacky, but in a very different way. It is a misunderstanding of what review (or quality control) is. There's always room for criticism as nothing you do, ever, will be perfect. But finding problems is the easy part. The hard part is figuring out what problems are important and how to properly triage them. It doesn't take a genius to complain, but it does take an expert to critique. That's why the dual can even be more harmful as it slows progress needlessly and encourages the classic nerdy petty bickering over inconsequential nuances or over unknowns (as opposed to important nuances and known unknowns). If QC sees their jobs as finding problems and/or their bosses measure their performance based on how many problems they find then there's a steady state solution as the devs write code with the intentional errors that QC can pick up on, so they fulfill their metric of finding issues, and can also easily be fixed. This also matches the letter but not the spirit. This is why AI won't be able to step in without having the capacity of writing the code in the first place, which solves the entire problem by making it go away (even if agents are doing this process). [1] Nothing said here actually presents a solution. Yes, I say "treat them as first class citizens" but that's not a solution. Anyone trying to say this, or similar things, is a solution is refusing to look at all the complexities that exist. It's as obtuse as saying "creating a search engine is easy. All you need to do is index all (or most) of the sites across the web." There's so much more to the problem. It's easy to over simplify these types of issues, which is a big part of why they still exist.

back2dafucha 1y ago

[flagged]

kjgkjhfkjf 1y ago

If you want to remain relevant in the AI-enabled software engineering future, you MUST get very good at reviewing code that you did not write. AI can already write very good code. I have led teams of senior+ software engineers for many years. AI can write better code than most of them can at this point. Educational establishments MUST prioritize teaching code review skills, and other high-level leadership skills.

ZYbCRq22HbJ2y7 1y ago
> AI can already write very good code Debatable, with same experience, depends on the language, existing patterns, code base, base prompts, and complexity of a task
- netghost 1y ago
  How about AI can write large amounts of code that might look good out of context.
  - ZYbCRq22HbJ2y7 1y ago
    Yeah, LLMs can do that very well, IMO. As an experienced reviewer, the "shape" of the code shouldn't inform correctness, but it can be easy to fall into this pattern when you review code. In my experience, LLMs tend to conflate shape and correctness.
    - dragonwriter 1y ago
      > As an experienced reviewer, the "shape" of the code shouldn't inform correctness, but it can be easy to fall into this pattern when you review code. For human written code, shape correlates somewhat with correctness, largely because the shape and the correctness are both driven by the human thought patterns generating the code. LLMs are trained very well at reproducing the shape of expected outputs, but the mechanism is different than humans and not represented the same way in the shape of the outputs. So the correlation is, at best, weaker with the LLMs, if it is present at all. This is also much the same effect that makes LLMs convincing purveyors of BS in natural language, but magnified for code because people are more used to people bluffing with shape using natural language, but churning out high-volume, well-shaped, crappy substance code is not a particularly useful skill for humans to develop, and so not a frequently encountered skill. And so, prior to AI code, reviewers weren't faced with it a lot.

Areibman 1y ago

The biggest grip I have with Github is the app is painfully slow. And by slow, I mean browser tab might freeze level slow. Shockingly, the best code review tool I've ever used was Azure DevOps.

wenc 1y ago
When I worked at a Microsoft shop, I used Azure DevOps. To be honest, it's actually not bad for .NET stuff. It fits the .NET development life cycle like Visual Studio fits C#.
awesome_dude 1y ago
nit: gripe, not grip :-P
echelon 1y ago
> The biggest grip I have with Github is the app is painfully slow. And by slow, I mean browser tab might freeze level slow. Javascript at scale combined with teams that have to move fast and ship features is a recipe for this. At least it's not Atlassian.
- lmm 1y ago
  Stash (now BitBucket Server) had the best code review going, head and shoulders above GitHub to the point I thought GitHub would obviously adopt their approach. But I imagine Atlassian has now made it slow and useless like they do with all their products and acquisitions.
  - dotancohen 1y ago
    Bit Bucket had a git-related tool called Stash? I love Bit Bucket, but I'm glad I did not know about that.
    - lmm 1y ago
      There was a locally-hosted Git server platform called Stash. Atlassian bought it, rebranded it as "BitBucket Server" (positioned similarly to GitHub Enterprise or self-hosted GitLab) and gradually made it look and feel like BitBucket (the cloud product), even though they're actually completely separate codebases (or at least used to be).

shayief 1y ago

Gitpatch attempts to solve this. Supports versioned patches and patch stacks (aka stacked PRs). Also handles force-pushes in stacks correctly even without Change-IDs using heuristics based on title, author date etc. It should also be unusually fast. Disclosure: I'm the author. I'm not convinced that review comments as commits make thing easier, but I think storing them in git in some way is a good idea (i.e. git annotations or in commit messages after merge etc)

kissgyorgy 1y ago

putting the review into git notes might have worked better. It's not attached to tje lines directly, but the commit and it can stay as part of the repo

jbmsf 1y ago

Recently, I've been wondering about the point of code review as a whole. When I started my career, no one did code review. I'm old. At some point, my first company grew; we hired new people and started to offshore. Suddenly, you couldn't rely on developers having good judgement... or at least being responsible for fixing their own mess. Code review was a tool I discovered and made mandatory. A few years later, everyone converged on GitHub, PRs, and code review. What we were already doing now became the default. Many, many years layer, I work with a 100% remote team that is mostly experienced and 75% or more of our work is writing code that looks like code we've already written. Most code review is low value. Yes, we do catch issues in review, especially with newer hires, but it's not obviously worth the delay of a review cycle. Our current policy is to trust the author to opt-in for review. So far, this approach works, but I doubt it will scale. My point? We have a lot of posts about code review and related tools and not enough about whether to review and how to make reviews useful.

deleted 1y ago
[deleted]
Feeble 1y ago
I am very much in the same position right now. My dev team has introduced mandatory code reviews for every change and I can see their output plummeting. It also seems that most code reviews done are mostly syntax and code format related - noone actually seems to run the code or look at the actual logic if it makes sense. I think its easy to add processes under the good intention of "making the code more robust and clean", but I never heard anyone discuss what is the cost of this process to the team's efficiency.
- mgaunard 1y ago
  You need to validate things like syntax upfront so that such things don't make it to review to begin with. I'm not a fan of automatic syntax formatting but you can have some degree of pre-commit checks.
- alkonaut 1y ago
  The value of having more people actually see the code will be there even if it’s just an unnecessary syntax nitpick.

6LLvveMx2koXfwn 1y ago

> But modifying code under review turned out to be tricky. GitLab enables this - make the suggestion in-line which the original dev can either accept or decline.

globular-toast 1y ago
Kind of. Don't you have to type the change into the browser? Which means your change might not even be syntactically correct. It would be far better if you could make the change locally then somehow and that straight to GitLab. Also how does it work with multiple commits? Which commit does it amend?

pjmlp 1y ago

I never did proper code review, other than when being lucky that we got a team of top devs in specific projects. More often than not, it either doesn't exist, or turns out in a kind of architecture fetishism that the lead devs/architects have from conferences or space ship enterprise architecture. Already without this garbage it feels so much better, than arguing about SOLID, clean code, hexagonal architecture, member functions being with an underscore, explicit types or not,...

_kidlike 1y ago

me and my team have been doing code reviews purely within IntelliJ, for something like 6 years. We started doing it "by hand", by checking out the branch and comparing with master, then using Github for comments. Now there's official support and tooling for reviews (at least in IDEA, but probably in the others too), where you also get in-line highlighting of changed lines, comments, status checks, etc... I feel sorry for anyone still using GitHub itself (or GitLab or whatever). It's horrible for anything more than a few lines of changes here and there.

3036e4 1y ago

What bothered me for a long time with code reviews is that almost all useful things they catch (i.e. not nit-picking about subjective minor things that doesn't really matter) are much too late in the process. Not rarely the only (if any) useful outcome of a review is that everything has to be done from scratch in a different ways (completely new design) or that it is abandoned since it turns out it should never have been done at all. It always seems as if the code review is the only time when all stakeholders really gets involved and starts thinking about a change. There may be some discussion earlier on in a jira ticket or meeting, and with some luck someone even wrote a design spec, but there will still often be someone from a different team or distant part of the organization that only hears about the change when they see the code review. This includes me. I often only notice that some other team implemented something stupid because I suddenly get a notification that someone posted a code review for some part of the code that I watch for changes. Not that I know how to fix that. You can't have everyone in the entire company spend time looking at every possible thing that might be developed in the near future. Or can you? I don't know. That doesn't seem to ever happen anyway. At university in the 1990's in a course about development processes there wasn't only code reviews but also design reviews, and that isn't something I ever encountered in the wild (in any formal sense) but I don't know if even a design review process would be able to catch all the things you would want to catch BEFORE starting to implement something.

epolanski 1y ago
> and that isn't something I ever encountered in the wild (in any formal sense) Because in the software engineering world there is very little engineering involved. That being said, I also think that the industry is unwilling to accept the slowliness of the proper engineering process for various reasons, including non criticality of most software and the possibility to amend bugs and errors on the fly. Other engineering fields enjoy no such luxuries, the bridge either holds the train or it doesn't, you either nailed the manufacturing plant or there's little room for fixing, the plane's engine either works or not Different stakes and patching opportunities lend to different practices.
- ozim 1y ago

keniu 1y ago

we need code review, do not let AI control us.

aitchnyu 1y ago

Tangential, long ago I wanted to use a repo-backed (IIRC tied to Mercurial) backend for issues. They were also flat files. We put too many plugins into Redmine and it died frequently.

germandiago 1y ago

I am happy with Gerrit but I am sure I do not know even how to use 20% of its capacity. The patchsets get stacked up and you know where you left off if there are different changes and that is very cool.

cbryant91 1y ago

good fine

brabel 1y ago

Author has not tried IDE plugin for GitHub / Bitbucket reviews!? We review code directly in IntelliJ with full support for commenting, navigation, even merging without leaving the IDE. Solves all problems the author has.

maunke 1y ago

Very nice to read. Sourcehut is missing in the list; it’s built on the classical concept of sending patches / issues / bugs / discussion threads via email and it integrates this concept into its mailing lists and ci solution that also sends back the ci status / log via email. Drew Devault published helpful resources for sending and reviewing patches via email on git-send-email.io and git-am.io

eafkuor 1y ago

> Alas, when I want to actually leave feedback on the PR, I have to open the browser, navigate to the relevant line in the diff, and (after waiting for several HTTP round-trips) type my suggestion into a text area This doesn't seem like much of a problem, does it? It's a matter of alt-tab and a click or two. Also, what is the point of having reviews in the git history?

liampulles 1y ago

This is how I learned to do code review when I was a new junior dev. I would write my review comments on another junior's code, and then our team lead would go write their comments that we missed, and then both of us juniors would read and see what we missed. It was a good way to learn about coding and reviewing I think.

liampulles 1y ago

Here's an alternative I've wondered about: Instead of one person writing code, and another reviewing it - instead you have one person write the first pass and then have another person adjust it and merge it in. And vice-versa; the roles rotate. Anyone tried something like this? How did it go?

thinkindie 1y ago
That’s basically an async pair programming session, isn’t it?
- liampulles 1y ago
  Yes I think so. Have you tried it?
  - thinkindie 1y ago
    Just cases where PR were submitted close to someone holidays and I assigned it to someone else in the team to bring it over the line. But otherwise I have worked with sync pair programming only.
mattikl 1y ago
I've noticed for a long time that if I have participated in writing the code under review, I'm able to provide much more insight. I think what you're suggesting starts from thinking the code as "our code" instead of my code vs. your code, which so easily happens with pull requests. And learning to work iteratively instead of trying to be too perfect from the start, which goes well with methodologies like TDD.
romanovcode 1y ago
Great idea, if you're fine with development time to take twice as long.
- ramon156 1y ago
  The alternative is a merge that potentially has more bugs. Trunk based is definitely not twice as long, rather 1.5x on average

codeman001 1y ago

I use CodeRabbit that helps, but it does not fix the two root issues. I run their free VS code plugin to review local commits first, which catches nits, generates summaries, and keeps me in my editor. The PR bot then adds structure so humans focus on design and invariants. Review state still lives in the forge, not in Git, and interdiffs still depend on history. If Git gets a stable Change-Id, storing review metadata in Git becomes realistic. Until then this is a pragmatic upgrade that reduces friction without changing the fundamental. https://www.coderabbit.ai/ide https://www.coderabbit.ai/ide

z3t4 1y ago

Leave the comments in the commit messages and make many small commits! That way they don't change the actual source and they're specific for that version of the code.

sgt 1y ago

Articles from Tigerbeetle I tend to just upvote and then click on the link. I just know it's going to be quality stuff.

bkolobara 1y ago

I have been working on the PR implementation for lubeno[1] and have been thinking a lot about the code review process. A big issue is that every team has a slightly different workflow, with different rules and requirements. The way GitHub is structured is a result of how the GitHub team works. They built the best tool for themselves with their "just keep appending commits to a PR" workflow. Either you need to have enough flexibility so that the tool can be adapted to everyone's existing workflow. Or you need to be opinionated about your workflow (GitHub) and force everyone to match it in some way. And in most cases this works very well, because people just want you to tell them the best way of doing things and not spend time figuring out what the best workflow would look like. [1]: https://lubeno.dev https://lubeno.dev

icy 1y ago

Worth mentioning that Tangled has support for stacked pull requests and a unique round-based PR flow with interdiffing: https://blog.tangled.sh/stacking https://blog.tangled.sh/stacking

mike_hearn 1y ago

I've used a more hard-core version of this in my own company and have been meaning to write a blog post about it for years. For now an HN comment will suffice. Here's my version, the rationale and the findings. WORKFLOW Every repository is personal and reviewer merges, kernel style. Merging is taking ownership: the reviewer merges into their own tree when they are happy and not before. By implication there is always one primary code reviewer, there is never a situation where someone chooses three reviewers and they all wait for someone else to do the work. The primary reviewer are on the hook for the deliverable as much as the reviewee is. There is no web based review tool. Git is managed by a server configured with Gitolite. Everyone gets their own git repository under their own name, into which they clone the product repository. Everyone can push into everyone else's repos, but only to branches matching /rr/{username}/something and this is how you open a pull request. Hydraulic is an IntelliJ shop and the JetBrains git UI is really good, so it's easy to browse open RRs (review requests) and check them out locally. Reviewing means pushing changes onto the rr branch. Either the reviewer makes the change directly (much faster than nitpicky comment roundtrips), or they add a //FIXME comment that IntelliJ is configured to render in lurid yellow and purple for visibility. It's up to the reviewee to clear all the FIXMEs before a change will be merged. Because IntelliJ is very good at refactoring, what you find is that reviewers are willing to make much bigger improvements to a change than you'd normally get via web based review discussions. All the benefits the article discusses are there except 100x because IntelliJ is so good at static analysis. A lot of bugs that sneak past regular code review are caught this way because reviewers can see live static analysis results. Sometimes during a review you want to ask questions. 90% of the time, this is because the code isn't well documented enough and the solution is to put the question in a //FIXME that's cleared by adding more comments. Sometimes that would be inappropriate because the conversation would have no value to others, and it can be resolved via chat. Both reviewee and reviewer are expected to properly squash and rebase things. It's usually easier to let commits pile up during the review so both sides have state on the changes, and the reviewer then squashes code review commits into the work before merging. To keep this easy most review requests should turn into one or two commits at most. There should not be cases where people are submitting an RR with 25 "WIP" commits that are all tangled up. So it does require discipline, but this isn't much different to normal development. RATIONALE 1. Conventional code review can be an exhausting experience, especially for junior developers who make more mistakes. Every piece of work comes back with dozens of nitpicky comments that don't seem important and which is a lot of drudge work to apply. It leads to frustration, burnout and interpersonal conflicts. Reviewees may not understand what is being asked of them, resulting in wasted time. So, latency is often much lower if the reviewer just makes the changes directly in their IDE and pushes. People can then study the commits and learn from them. 2. Conventional projects can struggle to scale up because the codebase becomes a commons. Like in a communist state things degrade and litter piles up, because nobody is fully responsible. Junior developers or devs under time pressure quickly work out who will give them the easiest code review experience and send all the reviews to them. CODEOWNERS are the next step, but it's rare that the structure of your source tree matches the hierarchy of technical management in your organization so this can be a bad fit. Instead of improving widely shared code people end up copy/pasting it to avoid bringing in more mandatory reviewers. It's also easy for important but rarely changed directories to be left out, resulting in changes to core code slowing down because it'd require the founder of the company to approve a trivial refactoring PR. FINDINGS Well, it worked well for me at small scale (decent sized codebase but a small team). I never scaled it up to a big team although it was inspired by problems seen managing a big team. Because most questions are answered by improving code comments rather than replying in a web UI the answers can help LLMs. LLMs work really well in my codebase and I think it's partly due to the plentiful documentation. Sometimes the lack of a web UI for browsing code was an issue. I experimented with using IntelliJ link format, but of course not everyone wants to use IntelliJ. I could have set up a web UI over git just for source browsing, without the full GitHub experience, but in the end never bothered. Gitolite is a very UNIXy set of Perl scripts. You need a gray beard to use it well. I thought about SaaSifying this workflow but it never seemed worth it.

kungfufrog 1y ago

Anyone know what editor the author is using in the first screenshot showing two panels side by side?

000ooo000 1y ago
Looks like VSCode in macOS, maybe with some custom CSS.
wry_discontent 1y ago
They mention magit, which makes me think Emacs, but it looks like there's lot of custom UI stuff.
l2dy 1y ago
Probably VS Code with https://github.com/kahole/edamagit https://github.com/kahole/edamagit

sc68cal 1y ago

This brings back memories of https://opendev.org/ttygroup/gertty https://opendev.org/ttygroup/gertty when I was contributing to OpenStack

ramon156 1y ago

I'm of the hot opinion that a reviewer shouldn't be running code. The one making the code is responsible for the fix, code reviews are just about maintainability. If your PR did not fix the issue or implement the feature, that's on you, not the reviewer.

OskarS 1y ago
I don't know about "shouldn't", I think it's fine if they do. But I basically agree, at some fundamental level, you have to have some trust in your coworkers. If someone says "This fixes X", and they haven't even tried running it or testing it, they shouldn't be your coworker. The purpose of code reviews shouldn't be "is this person honest?" or "is this person totally incompetent?". If they're not, it's a much bigger issue, one that shouldn't be dealt with through code reviews. Very different situation if it's open source or an external contribution, of course.
allknowingfrog 1y ago
The author mentioned that he doesn't want to make suggestions that don't actually work. That seems like a pretty valid reason to run the code.

elisemaya31 1y ago

[dead]

actinium226 1y ago

I like the idea of max 500 lines for custom solutions to problems that have "existing" solutions, might have to steal that.

jFriedensreich 1y ago

Its pretty clear to a growing number of devs what a review tool should look like. It is more a matter of what needs to happen so this becomes a usable and sustainable reality and what shape of organisation/ players can make this happen in the right way. - git itself wont go much further than the change-id which is already a huge win (thanks to jj, git butler, gerrit and other teams) - graphite and github clearly showed they are not interested in solving this for anyone but their userslaves and have obviously opposing incentives. - there are dozens of semi abandoned cli tools trying this without any traction, a cli can be a part of a solution but is just a small part What we need: - usable fully local - core team support for vscode not just a broken afterthought by someone from the broader community - web UI for usecases where vscode does not fit (possibly via vscode web or other ways to reuse as much of the interface work that went into the vscode integration) - the core needs to be usable from a cli or library with clear boundaries so other editor teams can build as great integrations as the reference but fitting their native ui concepts - it needs to work for commits, branches, stacked commits and any snapshot an agent creates as well as reviewing a devs own work before pushing - it needs to incorporate CI/CD signals natively, meta did great UI work on this and its crucial to not ignore all that progress but build on top of it - it needs to be as fine grained as the situation requires and with editability at every step. Why can i just accept one line in cursor but there is nothing like that when reviewing a humans code? Why can i fix a typo without any effort when reviewing in cursor when i have to go through at least 5 clicks to do the same when fixing a typo of a human. - It needs to by fully incremental, when a pr is fixed there needs to be a simple way to review just the fix and not re-review the whole pr or the full file

octodoctor 1y ago

[dead]

deterministic 1y ago

I have never (in my 30+ years career) worked for a company that required formal code reviews. And yet have managed to deliver a ton of commercially successful software. I am pretty sure that adding a review step would have slowed me down tremendously. Without adding any commercial value. However I can imagine that code reviews would work well for inexperienced developers being reviewed by more experienced developers?

phendrenad2 1y ago
At least in the US, code reviews are essentially mandated by legal compliance in publicly-traded companies. The law (SOX) says something like "no one person can destroy the company by making an engineering change", so viola, code review was invented. Private companies can probably get away with not doing code reviews, but often private companies want to pass security reviews like SOC 2. > I am pretty sure that adding a review step would have slowed me down tremendously This is very true. Everyone should work on a personal side-project at some point and realize just how much code review slows things down.