After JetBrains discontinued Upsource, we tried several code review tools and it shocked us that many of the tools (both commercial and open source) don't have built-in tools to review incrementally. It's just just on big soup of code, or you have to create new PR's for each small change. Now we have to use JetBrains SpaceCode, which still lacks many of the niceties of Upsource.
Some of us like "interdiff" code review
91–100 of 224 posts
Re: Some of us like "interdiff" code review
#92We're working with gitlab and not a day goes by where I don't miss gerrit with it's fast and functional ui, good and practical diff viewer, patch-sets, topics, review workflow, and manifests...
The thing that gitlab does really well, is making it clear how good gerrit is.
Re: Some of us like "interdiff" code review
#93It'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…
Why not just do good old mergetrains with pullrequest A points to branch B amd then B points to master, merge B into master and thereafter point A back to master or am I missing the point?
This is called "stacked diffs" and it's a good workflow; the issue is that it's annoying to use on GitHub without tooling. The "point A back to master" bit isn't easy/obvious with pull requests.
Re: Some of us like "interdiff" code review
#94This 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.
Re: Some of us like "interdiff" code review
#95As 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 bob review", "fix alice review", or "minor" introduced something that wasn't noticed until later, by having them separate we can tell whether it was intended functionality or a bug. This has happened to us a whole bunch of times with rarely seen edge cases, so the bug wasn't found until years later, or some other part of the code was masking the issue so it didn't manifest as a bug until years later. At least one of these was even caused in a "linting" commit, and all of these were much more easily fixable because we could tell the bug was introduced in one of these code-review-update commits, rather than the core feature commit.
Re: Some of us like "interdiff" code review
#96I'm midway through, but a nitpick: > You're on your own figuring out the Commit IDs and punching them into the URL bar if you want something more granular. There's a "commits" tab at the top, like: Conversation | [Commits] | Checks | Files changed Example: https://github.com/raspberrypi/linux/pull/6330/commits
I more meant that it's hard to diff between arbitrary commits without using the URL bar. The results are also... Weird. For example, let's say you have the base B, and commit X: B ---> X Now someone pushed to main, so you rebase on B' B' ---> X Now, you modify X to address something (maybe just a spelling error) B' ---> X' Now you push the new rebased branch. Question: how do you view the difference between X' and X?…
At my last job, the workflow was mostly merge based instead of rebasing. After you create a branch, you merge from develop of there’s anything new you want or to resolve conflict. And the whole PR will be squashed and merged (there’s an ID referring to the ticket). Force pushing to an open PR was frowned upon.
I think it reflected the email based approach a bit better as you can’t alter the first patch you sent, only send new ones. So even if the history of a PR is messy, we preserve its chronological aspect alongside the discussion.
Re: Some of us like "interdiff" code review
#97I'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)
Re: Some of us like "interdiff" code review
#98100% agree that this is ideal, the way Github does it is completely godawful and it's a tragedy that so many people have it normalized for them. We did this with Phabricator, although it was a somewhat-manual process, helped along by having some command line macros for updating all the reviews at once. But better still would be an explicit UI for it.
I am the author and used the phrase "Code review is a pretty good idea, in general" in the opening very specifically, because it used be one of the selling points listed on the Phabricator homepage. :) I miss it.
Every day I have to use Jira and GitHub instead of Phab is a rather painful day. Sadly those days are increasing.
Maybe one day I'll be able to use Gerrit (it sounds great), and then I can be only annoyed at Jira.
Re: Some of us like "interdiff" code review
#99I’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?
Re: Some of us like "interdiff" code review
#100I'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…
Yes, fixup commits are a good way to approach this, though I don't personally like them, though. I think Sapling's "absorb" command which works on an underlying SCCS weave to automatically absorb changes into the relevant diff is much more elegant. It prompts you with an interactive UI. (For me, it sorta falls into the same space as rebasing a series that has multiple branches on it. You need --update-refs for that. Because otherwise it's like, why am I, the human, doing the work of tracking the graph relationships and manually punching in the commits, and moving the branches, and doing all this bullshit? Computers are good at graphs! Let them handle it!)
There is also a `git absorb`, but it isn't as robust as Sapling's implementation[1].
Really the problem isn't interactive rebase or not. It's mostly a problem of the UX of the review tool itself more than anything, and the kind of "cycle" it promotes. I mentioned it elsewhere here, but fixup commits for example still won't solve the problem of GitHub showing you diffs between baselines, for example, which can absolutely ruin a review if the baseline is large (e.g. you rebased on top of 10 new commits.)
I do have problems with Git's UX beyond this, but the original post is mostly a gripe about GitHub.
[1] There is an example in this GitHub issue that captures the difference between the two underlying algorithms: https://github.com/martinvonz/jj/issues/170