← Back to context

Comment by bjackman

1 year ago

Can anyone comment on the code review experience? (I assume similar to Gitea but I haven't tried either).

I recently did some moderately serious code review in GitHub and discovered it's a baby's toy version of a code review tool. It seems it would be unusable for serious engineering work unless you totally design your source control model around making that work, at the expense of all the other things that influence how you wanna manage your history.

I am mostly used to Gerrit which has a very reasonable basic model but a lot of rough edges and some performance issues.

Suddenly I realised I don't think I've ever actually used a review tool I really like! I wonder if our industry is just getting by without one?

IIRC the Gitlab one was slightly better than GitHub but I can't remember too much about it.

4 comments

bjackman

Reply
acidburnNSA  1 year ago

I've used Phabricator and GitHub to design nuclear power plant design code... seems pretty serious. Can you tell us more about what's missing in your opinion from making the reviews usable? Curious what I've been missing out on!

  • bjackman  1 year ago

    The fundamental issue is that GitHub thinks the artifact you are reviewing is code. But the artifact I want to review is a series of commits. I make review comments like "please split this into a separate commit", "please add info XYZ to the commit message".

    GitHub doesn't offer any way to review changes to those things. If the author force-pushes (which is normal and healthy if you are iterating on a series of patches, instead of on a blob of code) there's no way to diff the details I want to look at.

    Compare Gerrit where for each individual commit you can diff between two versions of that commit, with a side-by-side UI showing the comments inline that the changes were made in response to.

    From speaking to friends I believe this is because "why would you force-push? Just push a new commit called 'respond to review comments'". When I said "but now your commit log is a mess" they say "no, you just squash the whole PR into a single commit when you merge it. So... yes, your commit log is a mess. Bear in mind there is also no support for dependencies between PRs. So basically, you are throttled to one in-flight PR per area of work at a time. So... your commits are gonna end up being huge. Not really viable for a large project.

    I have noticed that there are major projects like k8s underway on Github and they seem to get by, so maybe I'm missing something. I know that Go allow PRs but if you wanna do serious contributions they will funnel you to contribute via Gerrit instead.

  • CraigRood  1 year ago

    I miss Phabricator. Whilst it was certainly overkill for personal use, the additional applications meant I could do so much in one place and have everything integrated. Funny how what we have now, including GitHub feels a step back.