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

81–90 of 224 posts

Re: Some of us like "interdiff" code review

#81

Post by snatchpiesinger »

zeotroph wrote: post #76
snatchpiesinger wrote: post #73

Another way to look at this that this is 3 linearly dependent PRs masquerading as one. Make each a distinct PR and the problem goes away, especially if you can mark the PR to depend on another one (on Gitlab you can, not sure about Github). If you want to see each change as a single logical unit, then they will each be a distinct merge commit on your master branch, use `git log --first-parent-only master` to only see…

> [GitLab] you can mark the PR to depend on another How much user interaction does that require, and how is this visualized in the review UI? Gerrit creates this dependency with a single `git push`.

It's a bit cumbersome, and I think only recently you can make longer dependency chains. It's certainly not automated away with just git commands, but maybe there there is a Gitlab API way. The only way I know is to "edit" the PR (or MR in Gitlab speak) and paste the URL into some "depends on" field, then save.

There are certainly other problems as well, like you might have an MR 1 from feature1 to master, and MR 2 from feature2 to master which in turn depends on MR 1. Most likely your feature2 branch is off your feature1 branch, so it contains feature1's changes when compared to master, and that's what is shown in the Gitlab review UI. This makes reviewing MR 2's changes in parallel to MR 1 frankly impossible.

Having said that, I still think that this would be the right way to organize this kind of work, however Gitlab's execution is not great, unfortunately. Any of this is probably impossible in Github too. I wonder if Gerrit gets this right, I have no experience with it.

edit:

One interesting point of MR dependencies in Gitlab is that I think you can depend on MRs from other projects. This is sometimes useful if you have dependent changes across projects.

Re: Some of us like "interdiff" code review

#82

Post by tombl »

jacobegold wrote: post #51
aseipp wrote: post #21

Earlier quoted context omitted.

We strongly considered Graphite as an alternative to Gerrit at my job that I mentioned at the start of this post (which I am no longer at, actually) because it does look like an absolutely excellent product, I will admit. You should all be proud of a smart design and smart set of tools. But there's a really really really really really really big problem. Me and the other main engineer on our team used a custom fronte…

We (Graphite) love Jujutsu – comes up in conversation all the time here. A prior version of the CLI is open source, the core data model (using git refs to store some extra data about what a branch's parent is) is still the same. https://github.com/withgraphite/graphite-cli We've talked about supporting other clients, but don't currently have the bandwidth to build something like that – definitely something I am perso…

I think the cli repo went private a while ago.

Re: Some of us like "interdiff" code review

#83

Post by Vinnl »

I'm using mostly this workflow with GitHub, with the main disadvantages being that it's more work on my side, and not obvious to my collaborators. But it does carry the same advantages of allowing reviewers to view diffs with just their feedback incorporated, without breaking `git blame` and `git bisect`.

When I incorporate a reviewer's feedback, I'll commit that with `git commit --fixup `. I'll then push that up and leave a comment reply to the review feedback sharing the fixup commit hash.

Then when the PR is approved and I'm about to merge, I'll do

    git rebase --interactive origin/main --autosquash
This will then combine the fixup commits with the correct original commits. I then do a final `git push --force-with-lease` and merge it. (Make sure to note force push before the review is done, because then reviewers lose the ability to see what you added since their last review.)

This relies heavily on autocomplete in my terminal, so that I only have to type `git re` to get to that long command above, for example. And it's a bit clunky, so using a tool that supports and encourages this workflow would be nice.

But given that I'm stuck with GitHub, it's OK.

Re: Some of us like "interdiff" code review

#85

Post by lima »

zeotroph wrote: post #41
zdw wrote: post #2

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, e…

There is one thing I miss on Gerrit when you push a stack of commits: A central place to talk about the whole of the stack, not just individual commits. This "big picture", but still technical stuff, too often happens in the issue tracker. But where to place it, I have no idea. This stack is just too ephemeral and and can be completely different on the next push.

Many teams use topics for this.

Re: Some of us like "interdiff" code review

#86

Post by trashburger »

snatchpiesinger wrote: post #75
trashburger wrote: post #72

Earlier quoted context omitted.

By doing this, you break commit atomicity and make bisects hell. Please don’t do this. Commits aren’t perfect at first for sure, but they should be by the time you make them reviewable.

It's fine to break commit atomicity on feature branches. You can use git bisect --first-parent on you development/master branch.

I completely disagree. In doing so you lose all visibility into the components and gradual evolution of the code that atomic commits provide. Same thing with squashing (which is just the worst).

Re: Some of us like "interdiff" code review

#87

Post by aseipp »

snatchpiesinger wrote: post #73

Another way to look at this that this is 3 linearly dependent PRs masquerading as one. Make each a distinct PR and the problem goes away, especially if you can mark the PR to depend on another one (on Gitlab you can, not sure about Github). If you want to see each change as a single logical unit, then they will each be a distinct merge commit on your master branch, use `git log --first-parent-only master` to only see…

> Another way to look at this that this is 3 linearly dependent PRs masquerading as one.

The first commit in the example is not dependent on anything else and may not exist at all. Rather its existence is illusory to show that sometimes when you write commits, a few things happen at once. You might just churn out a doc fix, a bug fix, and a small other thing all at once. It's just the nature of the work.

> (on Gitlab you can, not sure about Github)

You cannot do this on GitHub without write access to the repository, so it's effectively a non-option for anyone who is not a committer in an open-source context. Don't ask me why this limitation exists.

If you do have write access, you can kind of do some similar things like open N pull requests where each PR 1...N has commits 1...N. Then you do something like "Read every PR, then merge only the last one which contains all N commits, and close all the others." Weird but OK, I guess.

Still, organizing this is a pain, and I don't think GitHub really emphasizes it -- and also rebasing the dependent branches really requires you to use something like --update-refs to make it sane at all. So, you can also use tools like https://github.com/spacedentist/spr> in order to organize it for you. Not the end of the world considering everyone uses 50 different Git wrappers, but not ideal.

Re: Some of us like "interdiff" code review

#88

Post by Piraty »

Vinnl wrote: post #83

I'm using mostly this workflow with GitHub, with the main disadvantages being that it's more work on my side, and not obvious to my collaborators. But it does carry the same advantages of allowing reviewers to view diffs with just their feedback incorporated, without breaking `git blame` and `git bisect`. When I incorporate a reviewer's feedback, I'll commit that with `git commit --fixup `. I'll then push that up and…

https://news.ycombinator.com/item?id=37086022

Re: Some of us like "interdiff" code review

#89

Post by sureglymop »

fHr wrote: post #57
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.…

Never going to understand those finance teams who think like 20k/yr for any enterprise deal is a good deal to onboard more customers and increase reach.

Because they don't need that much reach if even a single customer nets them 20k/yr.

Re: Some of us like "interdiff" code review

#90

Post by Piraty »

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 centric review process regardless of a webUI: [1]

[0]: https://drewdevault.com/2022/07/25/Code-review-with-aerc.htm... [1]: https://news.ycombinator.com/item?id=37086022

Post reply on HN