[HN Gopher] Why some of us like "interdiff" code review systems ...
___________________________________________________________________
Why some of us like "interdiff" code review systems (not GitHub)
Author : todsacerdoti
Score : 24 points
Date : 2024-09-10 20:39 UTC (2 hours ago)
(HTM) web link (gist.github.com)
(TXT) w3m dump (gist.github.com)
| zdw wrote:
| I've generally found that code review first, and rebase-centric
| systems like Gerrit tend to be much easier to review code in. One
| of the best parts of this is native support for stacking multiple
| patches, so people make smaller patches that are easier to
| review.
|
| Code review in Github feels like a bad afterthought - the space-
| wasting interface that looks more like a forum thread, the
| inability to track over rebases, etc.
| jeffbee wrote:
| Someone should do a deep dive into developer productivity after
| LLVM switched from Phabricator to GitHub. How many other major
| projects have done a switch like that?
| ajkjk wrote:
| Better it be a deep dive into developer sanity, a more
| important but comparatively under-valued metric.
| ams92 wrote:
| Most of the complaints here could be solved by having smaller
| pull requests and then squashing commits when it's time to merge.
| eximius wrote:
| Well, yes. Or by having stacked branches and rebasing your
| branches, etc.
|
| The point is that GitHub/git's default experience makes this
| harder to do than a system that bakes it in.
| gloryjulio wrote:
| We use stacked commits + rebase only in our company. The commit
| history is linear and it's very easy to revert changes. I don't
| see any advantage of using merging instead of rebase
|
| I am not sure why we need to squash commits. We encourage the
| opposite where you should commit small and often. So if we need
| to revert any commit, it's less painful to do so.
| majormajor wrote:
| Without squashing it's hard for me to commit as small and
| often as I would like.
|
| Some things I want out of the final series of commits:
|
| 1) everything builds. If I need to revert something or roll
| back a commit, the resulting point of the codebase is valid
| and functional and has all passing tests.
|
| 2) features are logically grouped and consistent - kinda
| similar to the first, but it's not just that I want the build
| to pass, I don't want, say, module A to be not yet ready for
| feature flag X but module B to expect the feature flag to
| work. In the original article, this is to say that I want the
| three commits listed, but not one halfway through the
| "migrate API users" step.
|
| But when I'm developing I _do_ want to commit halfway through
| steps. I might commit 50 lines of changes that I 'm confident
| in and then try the next 50 lines and decide I want to throw
| them away and try a different way. I might just want to push
| a central copy of what I've got at the end of the day in case
| my laptop breaks overnight (it's rare, but happens!). I might
| want to push something WIP for a coworker to take an initial
| look at with no intent of it being ready to land.
|
| But I don't want any of those inconsistent/not-buildable/not-
| runnable states to be in the permanent history. It fucks with
| things like git bisect and git blame.
| lozenge wrote:
| Not really. The idea is to split work into separate stages
| which are reviewed separately, but as a whole.
|
| In the example: "small refactor 25LOC -> new API 500LOC ->
| migrate API users 50LOC"
|
| Making a PR of the small refactor will probably garner comments
| about "why is this necessary".
|
| Opening two PRs at the same time is clutter as GitHub presents
| them as separate.
|
| As well, sometimes CI won't pass on one of the stages meaning
| it can't be a separate PR, but it would still be useful in the
| code review to see it as a separate stage.
| OJFord wrote:
| I create my GitHub PRs like a reviewer's going to look at the
| commits if it's too large overall. I'm also fairly sure they
| don't, because it's basically never worth doing so in their PRs
| ('fix the test', 'merge origin/master', 'address review comment',
| etc. commits).
|
| I suppose I agree GitHub doesn't help with this / implicitly
| opposes it (I don't see the 'explicitly and' claim justified
| though?) but there's nothing about a PR that isn't a 'series' of
| 'patches'. Maybe GitHub's just responding to the way most people
| use it, and making that easier/better instead of being
| principled? Doesn't mean we can't be.
| enasterosophes wrote:
| Nice, this taught me about `git range-diff` which wasn't on my
| radar before.
|
| Is the conclusion likely to be that the author thinks Gerrit is
| good, or is there some nuance I didn't pick up? I've used Gerrit
| before and in hindsight I much prefer it to other ways of doing
| code review.
| ajkjk wrote:
| 100% agree that this is ideal, the way Github does it is
| completely godawful and it's a tragedy that so many people have
| it normalized for them.
|
| We did this with Phabricator, although it was a somewhat-manual
| process, helped along by having some command line macros for
| updating all the reviews at once. But better still would be an
| explicit UI for it.
| fosterfriends wrote:
| I agree with the argument laid out here. Series of small diffs
| with versions is a fantastic clean model.
|
| When creating Graphite on top of GitHub, we chose to only support
| rebase model (despite the chaos that creates in GitHub timeline
| events). We also added "versions" support, which wasn't too hard
| because GitHub holds on to old commits even if you force push
| over them.
|
| A lot of what we try to build is the exact ideas this author is
| championing, in a way that's compatible on top of GitHub. My
| dream is us and others help usher Eng back towards the patterns
| Phabricator and Gerrit helped start :)
___________________________________________________________________
(page generated 2024-09-10 23:00 UTC)