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

121–130 of 224 posts

Re: Some of us like "interdiff" code review

#121

Post by riquito »

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…

You can add to your ~/.gitconfig

   [rebase]
        autosquash = true
from then on `git rebase -i origin/main` automatically reorder fixup and squash commits. It's a small thing but greatly improved my workflow

Re: Some of us like "interdiff" code review

#123

Post by juped »

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'…

Nope, none of it was knowingly done, and plenty of teams are almost trivially convertible to the normal workflow, even without inventing a buzzword like TFA did!

Though plenty aren't. I get it. (But one of the magic phrases that really works well is "this is what git, itself, does, and there's a man page installed on your system at this very moment explaining it")

Re: Some of us like "interdiff" code review

#124

Post by clktmr »

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.

Re: Some of us like "interdiff" code review

#125

Post by dietr1ch »

riquito wrote: post #121
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…

You can add to your ~/.gitconfig [rebase] autosquash = true from then on `git rebase -i origin/main` automatically reorder fixup and squash commits. It's a small thing but greatly improved my workflow

Squashing is nice because it keeps the amount of commits minimal, but as mentioned in the article, it has the problem that now reviewers have to figure out what changed since their last review, which can get hard to do if the tree of changes originally proposed needed updates in dependencies of the commits they reviewed.

I really like the idea that I'm working on a tree (often a stack/list) of changes, and as the review progress I get this cross product of iterations over time like [a-better-way-interdiff-review-aka-git-range-diff](https://gist.github.com/thoughtpolice/9c45287550a56b2047c631...) showcases. In the end, the time dimension is useless after things get submitted, so the repository only gets the latest state of the change tree committed, which is simple and like the auto-squashed version you mention, but during review reviewers get to see the change tree evolve in an easy way.

Re: Some of us like "interdiff" code review

#126

Post by keybored »

As the author is aware of[1][2] the Git project uses this interdiff approach with email.

- Patch series (PR equivalent) go through a round of reviews

- Each version has a cover letter (like PR desription) unless it’s only a one-patch series

- Each version has that optional cover letter with each patch as a reply email to it

- The next version is a reply to the previous cover letter

- And each version 2 and above cover letter has a git-range-diff in it (courtesy of git-format-patch)

- And also a human/manual summary of changes between versions

- Optionally you can have little comments of each patch that are not part of the commit/patch message: just put it between the three dashes and the diff. Or use Git Notes and let it handle it for you (it will put it in the same space).

In turn there are no “address feedback” commits in the final (merged) series. Only the changes themselves.

Just look at any `[PATCH v2 0/...` or higher (v3...) email on the mailing list: https://lore.kernel.org/git/

Of course this isn’t the easiest workflow:

- Email

- You ought to keep track of the base commit between versions (you could have rebased on the main branch)

- You need to store versions of your branches

- You need to keep track of who to CC on the emails. Well, perhaps not if they are the same people throughout, but it is good courtesy to add people who reply to these versions to the CC list

- You need to harvest the email message id on the cover letters and use that `In-Reply-To`

Phew!

But this is quite sublime for reviewers and people who come back to the series years later:[3]

- All of the review in the same thread overview

- Each version moves the thread to the right

- Each patch (to be commit) is commented on individually

- You can reply to the commit message and the diff by quoting them directly

- The contributor will both give you the range diff (which will highlight diff changes and metadata changes like edited commit messages) and a manual summary of the changes

[1] https://news.ycombinator.com/item?id=41511649

[2] And it is with some trepidation that I bring this up because of the aversion some people have to email workflows. Because the “interdiff style” of PRs is useful!

[3] Git Notes `amlog` records the message id of the patch email where the commit came from

Re: Some of us like "interdiff" code review

#127

Post by samatman »

Posts like this add fuel to the fire of my conviction that the future of revision control is Pijul, or something much like it.

I can't completely justify the intuition here, but this seems like an artificial distinction forced by a snapshot model of a codebase. It's literally about the same patches, but arranged differently to give clarity to the history of the project.

So it seems obvious on that level that a system where patches are the fundamental stratum has an advantage there, although I confess that I have yet to use Pijul for a substantial project, so I can't describe in detail how it might make this better. I'm just going off things like how a cherry-pick in Pijul applies the actual change from one branch to the other, rather than duplicating it over.

I get frustrated a bit when I think about the gap between the kind of difference a leap forward in revision control could make for our profession, and the amount of resources the Pijul project has to work with. So I'm just putting this out there: MSFT bought GitHub for about 9 billion dollars in 2024 money. Anything which displaces it would be worth more than that. That's a lot of leverage for anyone forward-looking enough to apply it.

Re: Some of us like "interdiff" code review

#128

Post by Guvante »

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…

Git aliases are magical BTW

Re: Some of us like "interdiff" code review

#129

Post by kspacewalk2 »

aseipp wrote: post #31
zeotroph wrote: post #29

Earlier quoted context omitted.

From my interaction with the free part of GitHub, "diff soup" describes it very well. Does the paid version do anything better? What about GitLab, can this get near Gerrit? And then there are the external services which try to make GitHub less painful (and quite pricey, especially compared to a selfhosted Gerrit), by providing stacked diff support, did you look at these?

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 been merged ahead of yours, the diff will include the changes that have been rebased onto.

If you decide to register an account on GitLab, simulate the MR and prove to yourself that ~90% of your interdiff post has been implemented by GitLab for about a decade, kindly donate the $10 to your nearest homeless shelter.

Re: Some of us like "interdiff" code review

#130

Post by travisb »

samatman wrote: post #127

Posts like this add fuel to the fire of my conviction that the future of revision control is Pijul, or something much like it. I can't completely justify the intuition here, but this seems like an artificial distinction forced by a snapshot model of a codebase. It's literally about the same patches, but arranged differently to give clarity to the history of the project. So it seems obvious on that level that a system…

While patches and snapshots are duals of each other, patches are generally less easy to reason about and work with, in no small part because compilers and humans work on the snapshot, not the patch.

Having worked extensively with patches as the semantic unit via patch(1) and quilt and stgit, I'm very skeptical that a VCS based on patches is actually superior in most circumstances.

In this particular case, perhaps it would be helpful to view code reviews as a snapshot where the differences (possibly intra-review revision differences) are highlighted instead of as patches.

Post reply on HN