Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Some of us like "interdiff" code review

gist.github.com

Post reply on HN
Print view View original

171–180 of 224 posts

Re: Some of us like "interdiff" code review

#171

Post by another-acct »

zoogeny wrote: post #140

I've heard people argue for this strategy a few times but I am not convinced. Most projects I work on have feature branches that get squashed into a single commit (erasing the history of the branch). If we even have the case described by the author we would just do the three steps (refactor, new api, update) as 3 commits. One thing that has been a solid practice for me is to avoid long-lived branches. That seems to b…

> have feature branches that get squashed into a single commit (erasing the history of the branch)

Terrible.

It makes git-blame and git-bisect essentially unusable.

If you have a regression, git-bisect can help you narrow it down to a single patch. Because of that, you want to have fifty 160-line patches in the git history, for a particular feature, rather than one 8000 line patch.

If a given line of code looks fishy, you want git-blame (or a series of git-blame commands) to lead you to a 160-line commit, with its own detailed commit message, rather than to a 8000-line commit.

You also want to preserve the order of the original commits. Just reading through the individual commit messages, in order, a few years later, can be super helpful for understanding the original design. (Of course the original patch set has to be constructed in dependency order; it needs to compile at every stage, and so on. That's a separate development step that comes on top of just implementing the functionality. The code must be presented in logical stages as well.)

Re: Some of us like "interdiff" code review

#172

Post by another-acct »

kspacewalk2 wrote: post #129
aseipp wrote: post #31

Earlier quoted context omitted.

No, paying for GH doesn't make the code review experience any better. It's identical across public/cloud/enterprise GH. I do not know if GitLab does anything different; I've never used it in anger. I'd bet $10 the answer is "no, it's basically just the same as GitHub", though. If you want a service that adds stacking on top of GitHub, my conclusion after some research is that https://graphite.dev/ is the best option.…

>I do not know if GitLab does anything different; I've never used it in anger. I'd bet $10 the answer is "no, it's basically just the same as GitHub", though. You would bet incorrectly then. GitLab does essentially what you're describing, the only difference being that it compares different iterations of the force-push "naively", so if your force-push includes for example a rebase onto master because another MR has b…

> GitLab [...] compares different iterations of the force-push "naively", so if your force-push includes for example a rebase onto master because another MR has been merged ahead of yours, the diff will include the changes that have been rebased onto

That's quite the deal breaker IMO; for example it couldn't be used to compare a backport series (targeting an older stable branch, for example) against the original commit range on the master branch.

Re: Some of us like "interdiff" code review

#173

Post by Izkata »

crabbone wrote: post #170
Izkata wrote: post #95

As someone who has been on a maintenance team for years and regularly has to dig through the history to figure things out, I strongly prefer the original "bad" version with 7 individual commits. Yes "git blame" takes a little bit of extra work to get through all the commits, but knowing what initial mistakes were made and refactors done makes it much easier to tell what the original intent was. For example, if "fix b…

This is solving the problem with the wrong tool. What you need is documentation. But people working on the project you have to maintain didn't write one. So, you are trying to use git blame, astrology and ouija board to guess what the project authors wanted. And, while doing so, you are arguing for never cleaning the house, keeping all the garbage where it falls. Which will only make your situation worse, because the…

> What you need is documentation. But people working on the project you have to maintain didn't write one.

Oh there is documentation, but it's old and contradictory. Commits are self-organizing by history.

> So, you are trying to use git blame, astrology and ouija board to guess what the project authors wanted.

There's no guessing involved. Keep the original commits and it's right there in the commit message and order of changes.

Guessing is what happens when you destroy the history by using rebases like this.

> And, while doing so, you are arguing for never cleaning the house, keeping all the garbage where it falls. Which will only make your situation worse, because the history will bloat with a lot of contradictory or false information.

I'm not saying "don't refactor" at all. Clean the code up as much as you want. The history however is completely hidden and not causing any clutter, it's there when you need it and invisible the rest of the time, so erasing useful history is just busywork for negative benefit.

Any apparent contradictions are also easily resolved because commits have timestamps and ordering and are part of the larger completely automatically generated history.

Re: Some of us like "interdiff" code review

#174

Post by another-acct »

aseipp wrote: post #104
lima wrote: post #85

Earlier quoted context omitted.

Many teams use topics for this.

Yeah, but you can't really discuss the topic itself , right? I do think this is a weakness of Gerrit. It doesn't really capture "big picture" stuff nearly so well. At least on GH you can read the top-level comment, which is independent of the commits inside it. Most of the time I was deep in Gerrit doing review or writing patches, it was because the architectural decisions had already been made elsewhere. I guess it'…

On a mailing list, you used to be able to write up the big picture in the "cover letter" (patch#0). Design-level discussions would generally occur in a subthread of patch#0. Also, once the patch set was fully reviewed, the maintainer could choose to apply the branches on a side branch at first (assuming the series was originally posted with proper "--base" information), then merge said side branch into master at once. This would preserve proper development history, plus the merge commit provides space for capturing the big picture language from the cover letter in the git commit log.

Re: Some of us like "interdiff" code review

#175

Post by lolc »

zeotroph wrote: post #38
deathanatos wrote: post #35

Yes! This is what I imagine in my head as a real code review style, not the stuff Github does. Glad to have a name for it. I'd add I'd also like my review system to be able to kick patches "out" of the review once they're ready. E.g., the small bugfixes that you make while working on that bigger feature should hopefully be small, isolated patches, ones that are going to find consensus with a reviewer quite quickly. O…

That's exactly what Gerrit can do. When you push an x-b-c-d-e chain, these show up stacked in the UI, but you can easily cherry-pick b onto main (see that the CI passes, and the usual review), and rebase everything on top of that. If it is x, the bottom one, you can directly submit it and continue with the others.

All this rebasing sounds like constant pain to pull from Gerrit. Does it actually create new branches v2 v3 after a rebase? Or how do I switch my local checkout to the rebased branch from remote?

Re: Some of us like "interdiff" code review

#176

Post by another-acct »

codeapprove wrote: post #53

Love the blog post, it's great to see people actually thinking about how code review should work! I've used four different code review systems extensively, all with different strengths and weaknesses: Critique (Google internal), Gerrit (at Google, but same as external), GitHub (duh), and CodeApprove (the one I built). Critique was far and away the best, but it only works because it's perfectly fit to Google's monorep…

> GitHub is [...] not very reviewer or team friendly though

The problem is that reviewers / maintainers are much scarcer than contributors. Workflows and UIs should optimize for reviewer throughput, IMO.

(I've never used the three other tools you mention, so my argument is general.)

Re: Some of us like "interdiff" code review

#177

Post by zoogeny »

steveklabnik wrote: post #153
zoogeny wrote: post #140

I've heard people argue for this strategy a few times but I am not convinced. Most projects I work on have feature branches that get squashed into a single commit (erasing the history of the branch). If we even have the case described by the author we would just do the three steps (refactor, new api, update) as 3 commits. One thing that has been a solid practice for me is to avoid long-lived branches. That seems to b…

The people I know who prefer stacked diffs argue that it makes it easier to integrate changes faster, no matter the size. Part of this is because unlike a PR on GitHub, you can land parts of a stack: to use the example from the article, if the "small refactor" diff is good to go, it can be landed without landing the "new api" and "migrate API users" diffs. Centering commits rather than branches has the effect of maki…

I'm not sure I understand your exact logic, so let me talk through a simplified scenario for arguments sake. Imagine each change takes 1 day. Imagine the team releases code to production every day. On Monday a developer picks up the task, does the refactor, puts up a PR and the refactor is shipped Tuesday morning. Tuesday he works on the api and it ships Wednesday. Wednesday he finishes the migration which ships on Thursday.

In the alternate scenario the entire stacked commits go out together on Thursday after the entire 3 days of work is completed. So I do not see how it would make it easier to integrate changes faster. This problem becomes worse, as you can imagine, if the second task in the stack takes a week vs. a day. I believe this kind of logic can be extended to any timescale.

Re: Some of us like "interdiff" code review

#178

Post by another-acct »

aseipp wrote: post #105
wodenokoto wrote: post #59

When doing code reviews, I think it is annoying that every time I comment on a line, PR author gets a notification. This is not a simultaneous, real-time thing. I'm in the middle of doing my review, and my comments are not ready to be read. Maybe I'll change my mind on my comment on line 8 when I reach line 80.

GitHub lets you do this, but only when writing a review for someone else. Not when addressing someone else's review, in which case your complaint 100% stands. And yes, it absolutely drives me nuts, honestly. Why can I batch review comments, but not resolutions! Another thing Gerrit gets right! GitHub... Please...

... Another thing that mailing list-based development gets right ;) Most MUAs should know about a concept called "Drafts". I can have as many draft messages concurrently as I want, I can work on them over several days, and I can send them out (in practice) as a batch.

Re: Some of us like "interdiff" code review

#179

Post by another-acct »

Piraty wrote: post #90

using github's UI as preferred way to interact with code in review is a bad idea, as it encourages lazy from-the-couch-just-yolo-approve-it-looks-alright style of review. this is where gerrit+mail based workflows shine as reviewer is more encouraged to apply the series, compile/run it in their env (which might differ from your's); here is an example [0]. here are some useful notes on how to have a purely branch centr…

> mail based workflows shine as reviewer is more encouraged to apply the series, compile/run it in their env

not to mention: if the reviewer does this for every version of the posted series, on appropriately named (versioned) local branches, then they can trivially run git-range-diff between adjacent versions!

Re: Some of us like "interdiff" code review

#180

Post by another-acct »

clktmr wrote: post #124

I agree in general, but running git bisect on individual PR commits is just doing it wrong. There will always be commits that break stuff temporarily. Run git bisect only on the merge commits instead, which are typically already tested by CI.

> I agree in general, but running git bisect on individual PR commits is just doing it wrong. There will always be commits that break stuff temporarily.

That's unacceptable in my book. Before submitting any patch set for review, the contributor is responsible for ensuring that the series builds (compiles) at every stage -- at every patch boundary. Specifically so that a later git bisect never fail to build any patch across the series.

This requries the contributor to construct the series from the bottom up, as a graph of dependencies, serialized into a patch set (kind of a "topological sort"). It usually means an entirely separate "second pass" during development, where the already working and tested (test-case-covered) code is reorganized (rebased / reconstructed), just for determining the proper patch boundaries, the patch order, and cleaning up the commit messages. The series of commit messages should read a bit like a math textbook -- start with the basics, then build upon them.

Furthermore, the patch set should preferably also pass the test suite at every stage (i.e., not just build at every stage). Existent code / features should never be functionally regressed, even temporarily. It's possible to write code like this; it just takes a lot more work -- in a way you need to see the future, see the end goal at the beginning. That's why it's usually done with a separate, second pass of development.

Post reply on HN