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

101–110 of 224 posts

Re: Some of us like "interdiff" code review

#101

Post by aseipp »

mizzao wrote: post #97
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…

I was going to say... interactive rebase addresses a lot of the "diff soup" comments that the writer complains about. It's really only done by disciplined engineering teams though (who bother to learn some more advanced features of git)

Interactive rebase is fine, I used it probably 50x a day for many years. The problem isn't really with Git here, it's more a bunch of interrelated problems with GitHub's UX, and the fact that many people culturally have never approached the problem in any other way. A lot of places basically just outright copy GitHub's UX, which I think is a good example of this.

For example, using 'git absorb' or autosquash doesn't solve the problem of the review UX just doing badly when you say "What is the difference between the last version of commit Y, and the new version." If those changes include a rebase, it shows you the entire diff including the rebase. That's often not good, but sometimes extremely necessary. (See my other posts in the thread about how Gerrit does this better.) And if you have to do something like rebase, then solve a conflict as a result of the rebase, it is basically unavoidable that you have to do all of them at once if you want the CI to stay clean (which I find important, at least.)

Also, a lot of teams just culturally hate shit like autosquash, or any squash/rebasing at all. Do I think they're misinformed? Yes. Do I think they could use tools that make those flows 1000x better and faster? Yes. But at the end of the day default user interface and design philosophy of one of the most popular software forges in the world does actually matter and have downstream cultural impact. So you have to interrogate that and break it down for people. GitHub does not really encourage, explain, or smooth out workflows like this. People learn it, then hate anything else. (I mean, fucking hell, Git rebase didn't even have --update-refs, a huge QOL improvement for stacked diffs, until like 2022!) So, you often don't have a choice. I've had many places where engineers despised GH force pushes, but only because... GitHub's UX is bad at tracking addressed/reviewed comments on a PR after a force push! They get hung by their own noose, so to speak, and don't even know it.

I do have other problems with Git, but this post is mostly about GitHub/Gitlab style PR review flows. The same problems still happen even with autosquash or tools that accomplish the same thing.

Re: Some of us like "interdiff" code review

#102

Post by aseipp »

planetpluta wrote: post #99

I’m confused by how pushing to new branches would work on GitHub (or is the point that it doesn’t…)? Are you able to change the branch of a PR from `v1` to `v2` without making a new PR?

Yes, the point is that it basically doesn't support that.

Well, OK. You can push two branches, v1 and v2, each with the commits. Then to do pairwise diffs, you type in the commit object hashes directly into the URL bar to diff the two objects in the repository using the 'blobs' API but like... I don't think that qualifies so much as "supporting" it as much as an absurd hack, right?

> Are you able to change the branch of a PR from `v1` to `v2` without making a new PR?

No, you have to open a whole different PR.

Re: Some of us like "interdiff" code review

#103

Post by globular-toast »

zeotroph wrote: post #77
globular-toast wrote: post #60

GitLab supports this. Every time someone pushes or force pushes it tags that as a version which you can diff. If your developers know how to generate new commits then you can do it right away with GitLab. The problem is generating the new commits. Developers just aren't very good at doing this. They can modify a single commit just fine, but modify a commit that isn't the latest commit involves a rebase. Magit has the…

> Developers just aren't very good at doing this. GitHub provided a way to contribute, but also to avoid learning to rebase, thus making it more welcoming to devs who only know about commit and pull - that is what made it so popular. The squash then rebase or merge step is done on server side. Plus it has a very "harmless" UI, but that hides a lot of details (patchsets) and the layout wastes so much space imo. This a…

Yeah, this annoys me too. I actually find the "forced merge" (`--no-ff`) style even worse because you can never tell if you're looking at "real" commits or just crap because they couldn't be bothered to rebase.

Must say, though, I think "history" is completely the wrong way to think about version control. It's not about tracking history, it's about tracking versions. History is the crap, unrebased commits. Rebasing turns the history (throwaway, works in progress) into versions.

Re: Some of us like "interdiff" code review

#104

Post by aseipp »

lima wrote: post #85
zeotroph wrote: post #41

Earlier quoted context omitted.

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.

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's one of the tradeoffs to Gerrit only being a code review tool. Phabricator also didn't suffer from this so much because you could just create a ticket to discuss things in the exact same space. Gerrit is amazingly extensible though so plugging this in is definitely possible, at least.

Re: Some of us like "interdiff" code review

#105

Post by aseipp »

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...

Re: Some of us like "interdiff" code review

#106

Post by dogleash »

mstachowiak wrote: post #54

It's always exciting to see new approaches to code reviews - GitHub has its strengths, but it’s far from perfect. For the scenario you’ve outlined, have you thought about splitting the 3 patches into separate, dependent pull requests? While GitHub doesn’t natively support this, the right code review tool (shameless plug - I’m part of a team building one called GitContext) should allow you to keep pull requests small…

> It's always exciting to see new approaches to code reviews - GitHub has its strengths, but it’s far from perfect.

This is nice sentiment, it's positive reception to an idea and polite to the incumbent.

But it's so thoroughly not a new idea. It's literally the workflow git was designed to support, and is core to many long-standing criticisms about GitHub's approach for as long as GitHub has had pull requests.

And I'm over here wondering why this idea took *checks calendar* over 15 years to graduate from the denigrated mailing list degens and into hip trendy development circles.

I thought we were knowingly choosing shit workflows because we had to support the long-standing refusal by so many software devs to properly learn one of their most-used tools. That's why I chose the tools I chose, and built the workflows I built, when I migrated a company to git. Nobody gets fired for buying IBM after all.

Re: Some of us like "interdiff" code review

#107

Post by eddd-ddde »

steveklabnik wrote: post #94
strken wrote: post #45

This is interesting. At work we use PRs like the author uses commits, and in fact we squash-and-merge them at the end, but our approach requires rebasing the later PRs whenever we make a change to the earlier PRs. This can be quite laborious, falls afoul of the "don't force-push" rule, takes a long time for engineers to learn, and tends to break existing code review comments in the GitHub interface, but works out oka…

> Perhaps a new source control management tool could have first class support for higher level concepts than just commits and branches, or perhaps that would be even worse to use. You should check out jj, sapling, or mercurial.

+1 for jj

It simplifies my workflow a lot when compared to plain git. I love not having to track and stage manually, plus undo is godsend.

Re: Some of us like "interdiff" code review

#108

Post by aseipp »

dogleash wrote: post #106
mstachowiak wrote: post #54

It's always exciting to see new approaches to code reviews - GitHub has its strengths, but it’s far from perfect. For the scenario you’ve outlined, have you thought about splitting the 3 patches into separate, dependent pull requests? While GitHub doesn’t natively support this, the right code review tool (shameless plug - I’m part of a team building one called GitContext) should allow you to keep pull requests small…

> It's always exciting to see new approaches to code reviews - GitHub has its strengths, but it’s far from perfect. This is nice sentiment, it's positive reception to an idea and polite to the incumbent. But it's so thoroughly not a new idea. It's literally the workflow git was designed to support, and is core to many long-standing criticisms about GitHub's approach for as long as GitHub has had pull requests. And I'…

I mean, the answer is simple. Even if email-based flows use range-diff, which is the correct conceptual model, all the actual details of using email are, I would estimate, about 1,000x shittier in 2024 than using GitHub in 2008 when I signed up for the beta as user #3000-something.

Email flows fucking suck ass. Yes I have used them. No, I won't budge on this, and no, I'm not going to go proselytize on LKML or Sourcehut or whatever about it, in Rome I'll just do as the Romans even if I think it sucks. But I've used every strategy you can think of to submit patches, and I can't really blame anyone for not wading through 500 gallons of horrendous bullshit known as the mailing list experience in order to glean the important things from it (like range-diff), even if I'm willing to do it because I have high pain tolerance or am a hired gun for someone's project.

Also, to be fair, Gerrit was released in 2009, and as the creator of ReviewBoard (in this thread!) also noted it supports interdiffs, and supported them for multiple version control backends, was released in 2006! This was not a totally foreign concept, it's just that for reasons, GitHub won, and the defaults chosen by the most popular software forge in history tend to have downstream cultural consequences, both good and bad, as you note.

Re: Some of us like "interdiff" code review

#109

Post by faangguyindia »

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…

Biggest problem working with Git is remembering all these flags and commands.

If you do it everyday for years, sure you remember most of them.

But for weekend hackers or those who specialise in some other areas, it creates a lot of frustration.

I often forget all these flags etc....

My brother made this https://github.com/zerocorebeta/Option-K

This enables me to simply write in thermal, "interactive rebase main, auto squash.

And then I hit option+k and it replaces it with the above command.

Re: Some of us like "interdiff" code review

#110

Post by dogleash »

aseipp wrote: post #108
dogleash wrote: post #106

Earlier quoted context omitted.

> It's always exciting to see new approaches to code reviews - GitHub has its strengths, but it’s far from perfect. This is nice sentiment, it's positive reception to an idea and polite to the incumbent. But it's so thoroughly not a new idea. It's literally the workflow git was designed to support, and is core to many long-standing criticisms about GitHub's approach for as long as GitHub has had pull requests. And I'…

I mean, the answer is simple. Even if email-based flows use range-diff, which is the correct conceptual model, all the actual details of using email are, I would estimate, about 1,000x shittier in 2024 than using GitHub in 2008 when I signed up for the beta as user #3000-something. Email flows fucking suck ass. Yes I have used them. No, I won't budge on this, and no, I'm not going to go proselytize on LKML or Sourceh…

Dude, I'm not making a defense of mailing list workflows here. I'm just pondering the nature of the world where despite all the yapping about git I've seen floating around on the internet for as long as I've been lurking social media, the yappers are just recently keying in on something.
Post reply on HN