[deleted]
Some of us like "interdiff" code review
161–170 of 224 posts
Re: Some of us like "interdiff" code review
#162I really don't like either idea, and don't understand why is this such a big issue...
Working with Web interface for anything code-related is a huge downside. Workflow doesn't matter at this point, the usability of this approach, no matter what it does is so bad, it's not worth discussing further.
The best way it ever worked for me is that:
1. PR author creates a PR and gets assigned a reviewer.
2. The reviewer leaves a commit with
# REVIEW(reviewer): Comments
4. If reviewer is happy, the author gets to organize the commits in whatever sequence they want (this would typically involve something like squashing everything, removing review comments if they are no longer necessary, and then splitting the PR into logical parts). Otherwise we go back to (2).
No need for complicated workflows, no need for any kind of external system...
Re: Some of us like "interdiff" code review
#163Earlier 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…
> all the actual details of using email are, I would estimate, about 1,000x shittier in 2024 than using GitHub in 2008
Disagree about "all". Tracking patches in need of review is better done in a good MUA than on github. I can suspend a review mid-series, and continue with the next patch two days later. Writing comments as manually numbered, plaintext paragraphs, inserted at the right locations of the original patch is also lightyears better than the clunky github interface. For one, github doesn't even let you attach comments to commit message lines. For another, github's data model ties comments to lines of the cumulative diff, not to lines of specific patches. This is incredibly annoying, it can cause your comment for patch X to show up under patch Y, just because patch Y includes context from patch X.
Edited to add: github also has no support for git-notes. git-notes is essential for maintaining patch-level changelogs between rebases. Those patch-level changelogs are super helpful to reviewers. The command line git utilities, such as git-format-patch, git-rebase, git-range-diff, all support git-notes.
Re: Some of us like "interdiff" code review
#164It'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…
As mentioned elsewhere in this thread, this is also the approach that Sapling follows. As for GitContext, how do you keep track of commits across fixups, rebases, reordering, etc.?
What's the point of keeping track of commits? I honestly never understood people wanting that. Is this for some kind of weird accounting / social-score system where the number of commits decides your yearly bonus?
It's useful to see how the system evolved (because you might want to go back a bit and redo the newer stuff), but it's pointless to see the mistakes made along the way, for example, unless you have some administrative use for that.
Similarly, if a sequence of commits doesn't make sense as committed, but would make better sense if split into a different sequence: then I see no problem doing that. What's the point of keeping history in a bad shape? It's just harder to work with, if it's in a bad shape, but gives no practical advantages.
Re: Some of us like "interdiff" code review
#165Earlier 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…
> 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.
> ~90% of your post
This is literally what GitHub does, down to the very word, and it it is inferior to Gerrit, and it is not sufficient to get 90% of the way, the last 10% matters. As I have explained a dozen times in this thread. Lol.
Re: Some of us like "interdiff" code review
#166Earlier 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.…
Graphite is OK. It's not great.
It's very buggy. My stacks frequently just hang and become unmergeable.
It frequently gets confused about its state and "gt sync" thinks that the upstream has changed even when it hasn't.
Because of the three states involved (local, GitHub, and Graphite), there is a lot of "syncing". "gt submit", "gt track", "gt untrack", "gt sync" are all needed constantly, which adds mental overhead over the usual git pull/push.
You end up with a lot of force-push churn in the GitHub PR activity when re-stacking. In fact, I dread syncing anything because in 6-PR stack it could cause an avalanche of force-pushes. This is of course a side effect of the GitHub PRs model and their rebase approach.
The web UI is pretty terrible. It's full of AI crap, and there's a "meme library". It kind of seems like they want people to live in their app, taking over from GitHub. But they don't really offer a reason whatsoever to go to graphite.dev instead of GitHub. Graphite doesn't really help the review process.
They really want you to sell you their merge queue product.
In short, when it works it's fine. It's nice to do a series of PRs and then watch as it merges them. But it's not a tool I enjoy using, for the above reasons.
I'd prefer a lightweight tool that just managed PRs and then did the merge magic. Graphite just wants to be too much.
Re: Some of us like "interdiff" code review
#167Earlier quoted context omitted.
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.
If you're asking "Why did this take 15 years for people to understand" and my reply is "Because it was under 1000 layers of other bullshit", then that's the answer to your pontification. It has nothing to do with whether you think email is good or not. You pondered, I answered. That simple.
> Because it was under 1000 layers of other bullshit
Not only because of that.
git-range-diff, while absolutely a killer feature, is a relatively new feature of git as well (a bit similarly to "git rebase --update-refs" -- which I've just learned of from you https://news.ycombinator.com/item?id=41511241>, so thanks for that :)).
Namely, git-range-diff existed out-of-tree as "git tbdiff" https://github.com/trast/tbdiff> originally. It was ported to git proper in August 2018 https://github.com/git/git/commit/d9c66f0b5bfd>; so it's not a feature people could have used "15 years ago".
(FWIW, before git-range-diff was a thing, and also before I had learned about git-tbdiff, I had developed a silly little script for myself, for doing nearly the same. Several other people did the same for themselves, too. Incremental review was vital for most serious maintainers, so it was a no-brainer to run "git format-patch" on two versions of a series, and colordiff those. The same workflow is essential for comparing a backport to the original (upstream) version of the series. Of course my stupid little script couldn't recognize reorderings of patches, or a subject line rewrite while the patch body stayed mostly the same.)
Re: Some of us like "interdiff" code review
#168> Interlude: Can you please just tell me if git rebase is evil or not so that we can derail the entire discussion over it? Ha, that's funny. But yes, please we need interdiffs in GitHub and GitLab. I want to have my PRs/MRs always rebased and I don't want "fix code review comments" commits.
Why would you want that in GitHub or Gitlab? :| The Web interface is atrocious no matter what you are using it for. Just the fact that you need to edit text in a dysfunctional Web editor negates all the benefits of any workflow you can imagine...
The most natural way to deal with PRs is to use your editor you use to write your code. Here you have all the tools necessary to navigate the code, to look up the history of the change, to run tests and so on.
This is just not the part that needs to be done by a Git hosting service. Having to do it through a Web interface just feels like random punishment...
Re: Some of us like "interdiff" code review
#169I am the person who wrote this. AMA EDIT: Also, I'm not sure if this is against the rules, but I also need a new job as of recently. I like working on dev tools and other hard problems. If you liked reading this, want me to make your dev team more productive, or just want to experience and enjoy my excellent (and occasionally eclectic ) taste, the email is in my profile.
What do you think of GitLab set up with Merge Request Dependencies + Squash+Merge merge strategy?
I remember it being pretty easy to have that series of MRs pattern set up. On merging 3x MRs it would be 3x merge commits w/ a single squash commit for each. Regardless of the MR’s commit history.
Tradeoffs are having to merge earlier series branches into later series branches after changes are made during review.
But people can do what they want to the commit history during review. Don’t matter as it just gets squashed.
Been a while since I’ve done this mind, been slumming it with GitHub so I might be looking at it with rose tinted sunglasses.
Re: Some of us like "interdiff" code review
#170As 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 history will bloat with a lot of contradictory or false information.