Unified versus Split Diff
matklad.github.io
matklad.github.io
obviously every team and ticket is different, but IMO unless the person doing the review is some sort of principal engineer mostly responsible for the code at large, this does not align with I would personally consider a code review. In my book a general code review is simple sanity check by a second pair of eyes, which can result in suggestions to use different API or use an API slightly differently. If commits in the PR are properly ordered/squashed it is relatively easy to review incremental changes in isolation anyway.
I guess the complaint author has is really about review types. I do not have terminology ready at hand, but there is run of the mill sanity check review and then there is deep, architectural feature premerge review. The author complains about the latter when most of the code reviews in web tools are of the former variety.
This is an impoverished view of code review. Code review is a principal mechanism for reducing individual code ownership, for propagating conventions, and for skill transfer.
A good code review starts with a good PR: one that outlines what its goals were and how it achieved them.
First item on a good review then: does the code achieve what it set out to do per the outline?
Second: does it contain tests that validate the claimed functionality?
Third: does it respect the architectural conventions of the system so far?
Fourth: is the style in line with expectations?
Fifth, and finally: do you have any suggestions as to better API usage?
A code review that is nothing more than a sanity check is useless and could have been done by CI infrastructure. Code review is a human process and should maximally take advantage of the things only humans could do. Leave the rest to machines.
An implicit question in several of the above is "will this set a good example for future contributions?"
I am now working in an organization that is set up to reduce code ownership, and they struggle to attract talent, although pay is good and work is fulfilling.
How do they do reduce individual code ownership? Horizontal integration. Developers code, analysts design DB structures (at least nominally), project managers set up meetings. Different silos exist for CICD, cloud roles, core teams.
There are vetting committees everywhere that have the last say on the libraries used and the nitty-gritty details of REST APIs and naming.
It exhilarating to start projects, then see them degrade inevitably into corporate monstrosities.
These might not be related though?
> work is fulfilling
> It exhilarating to start projects, then see them degrade inevitably into corporate monstrosities.
What you describe does not sound pleasant. So it's not fulfilling after all?
The no individual code ownership policy is hard to bear for inquisitive minds, though.
Thus the talent shortage.
I would not work on boring ass project with cripping management problems and think that's fulfilling. I assume you have recruiting problems because most people share that sentiment.
People like to work on interesting stuff without much office politics, that is "fulfilling" to them, even if outcome is app that would be "boring" to the outsider.
I meant working for emeregency services, social security, global medical dossiers, etc This is objectively public utility.
In response to your first point I guess things really depend on where you store PR metadata and how ephemeral/permanent it is. Some teams store that information in the ticket, some fill implementation notes with change request, some add that to the PR, some discuss in their standup (or similar) meeting.
Regarding 2-3, you are right, I just lumped them all under umbrella term.
Maybe we could use terms like "PR review" and "code review". The former is a shallow LGTM check, while the latter may involve code checkout, poking around, architectural discussions, pair/team programming and so on. In my book they are entirely different beasts and web tools are geared (not without justification) towards the former, where both types of diff should serve the purpose.
OP is right though, if the "check out in editor" workflow was much smoother (than quick web view) I would prefer to always do that
Often this includes getting feedback if that change can make problems outside the feature implemented or in the future, like introducing dependencies that cost more in total than they help locally.
On the one hand, I really like constant deep feedback. I really like the consistency benefits of having another person say “that’s too much, I find that unreadable.”
On the other, I have now been at a lot of places where it was very hard to get my code reviewed, latencies of days and sometimes weeks if folks are in a particularly heinous crunchtime... And then when it does get reviewed, the stuff that I worked really hard on to get the right speed or to properly centralize cache invalidation... Suddenly someone is like “I would have done it from this other approach” and you have no idea whether it's tractable or not.
While I have never been at a place that did this, I have in my head the idea that the code should be an unfolding collective conversation, kind of like when folks are all collaborating on a shared Google Doc, I see that you are editing this section and I throw in a quick comment “don't forget to add XYZ” and then jump to a different part that I won't be stepping on their toes with. So the basic idea would be to get everybody to merge to `main` like 2 or 3 times a day if possible. In that case code review really is just “make sure this doesn't break the build or break prod, everything is behind a feature toggle, if I don't like the design I will comment on the code near the design or sketch a proof of concept showing that my approach is superior in clarity or speed or whatever”... Nobody ever takes me up on this culture shift it seems.
I too find that its basically impossible to suggest switching to this workflow, given the weight of all our existing tools. They are so easy to setup, and most come for free (Azure Devops/Pipelines thing) that going off the track is just unthinkable.
(Or rather, I would probably just run on my private git copy, and only pull every once in a while, and ignore that the main branch always changes.)
When / how do you do code review in your suggested workflow?
This trades the integration complexity and problems for some new.. challenges :)
* everybody is good at breaking down code into changes small enough review quickly but not small enough to become tedious and trivial
* people work on CRs whenever they have time. They do generally not post them after people have started going offline, and wait til next morning as a courtesy, since there is no difference between publishing for review at 5pm vs 9am the next day
One side effect of this workflow is that because the pieces are more manageable, less uninterrupted “flow” time is required to complete a bunch of small things than to make one really big change. And others digest the smaller changes easier and knowledge spreads more effectively. And with the time its easy to say “don’t make people review outside working hours.”
My team does stuff on mainline and requires a new review on rebase.
- the code exactly as it is in that branch
- that branch merged into current main
- that branch merged into what is currently running in production (in the case that it's not what is in main e.g. tagged or branched)
This really helps to see whether it's an issue in "your" code, or whether it's a merge issue conflict causing failures.
See https://bors.tech/ for a similar idea.
What I would object to is changing the 'official' master every few minutes automatically.
Basically, first you write your code however you see fit. Then you use git to rewrite history to make the reviewers life easy, and then you give it to the reviewer.
The reviewer doesn't need to know that my original version had a bug that I fixed later. I can make it look like I came up with a bug free version from the start.
In general I find my past efforts to maintain a clean git history were probably not that useful, as long as you're not running e.g. 4 or 5 branches in parallel with crossing history. Branch off, make change, merge back main, PR, is fine.
Sometimes it makes sense to have multiple commits. The next step up the complexity ladder is:
- one preparatory refactoring commit that does _not_ change behaviour
- one simple commit that changes behaviour
Or are all the merge commits just made provisionally to inform you, but don't change master?
Your suggestion seems to help with the former, but not really the latter, or does it?
You might like https://bors.tech/ however.
Not in my experience, no.
The goal is just to keep you aware of what problems you're likely to run into if you tried to push for your change to be merged in at the present moment. If these problems are due to work another team is doing the idea is then that you might begin conversations with that other team to discuss how best to sort these things out.
https://www.atlassian.com/continuous-delivery/continuous-int...
Main caveat is that “prod” is a handful of central places.
So how does code review fit in?
- The first thing is that your main branch needs to be able to tolerate merging incomplete code without breakage. This would probably be resolved with feature toggles, ideally named to match issue/ticket numbers, so that you have to clean it up before closing the ticket. Automatic tests can catch syntax issues but code review would check that your code was indeed either removing a feature toggle (in which case the team should have agreed that the code is mature and turned it on in production) or properly isolated behind a toggle.
- The second thing is that the team needs to be working as a team. This means that the sprint goals are co-owned by multiple people, ideally the whole team takes responsibility to deliver. It never made much sense to me that we segmented out features to individual developers, as if “oh she is out sick this week, well she wasn't working on anything urgent, the work can wait for her,” I understand that this makes performance review slightly easier but I am not convinced by that argument... So what this means for development is that the “war room” that you see for a typical prod outage with a bunch of people chasing down different leads, I want to bring that mentality (without the concomitant stress!) to feature development. So this means that ideally the whole team knows what everyone is working on and code reviews are just part of the organic process of working on a feature together. «Hey you are calling this “accept_states” and I called it “success_states,” I really don't like the fact that you're using “accept” as an imperative verb here, but I could go with “states_to_accept” or “acceptable_states” or we could use “success_states,” which would you rather?» ... You read through each other's code because it legitimately impacts what you are doing elsewhere in the codebase.
- The final ingredient would be release staging. The way a lot of people do branching makes it really easy to forget commits or to roll something back and not rollback the rollback, et cetera. Because we merge everything into main, we get monotonic flow: a commit that is still relevant is merged into main first because everything is, then it gets cherry-picked into the appropriate release branches. (The only exceptions are bugfixing code that is no longer part of the latest version.) The exact tooling to make this doable seems tractable, but perhaps not easy. Per point 1, before the feature branch is permanently flipped, all the code related to it is marked as such and can be merged into a patch release without actually triggering a semver violation (just leave the toggle turned off!)... So in theory the actual feature release is just a single commit. I’d have to see how the rest of this works before I would know exactly how this plays out.
Note that just about every Git-based CI/CD platform does CI/CD incorrectly, things like ACLs and CI/CD config are central statements about who has access to what privileges and should not be branched. A Jenkins which pulls its config from the `ci` or `main` branches will usually save you a lot of headache.
1) obvious code smell, here’s an example using your existing code refactored and the reasons why it is a better fit here
2) you’ve done something I didn’t think of and it’s clearly better than the way I was thinking of it. Here’s why it’s better. Kudos!
Helps that I’m lead/principal in a small biz with like 4 people max writing code. So I kind of know what everyone is working on / what they’re touching with their changes.
Mileage will definitely vary in bigger teams / businesses. 100% helps that my performance isn’t tied to number of PRs reviewed etc.
This type of comment is the hardest to make for me as I know it means redoing a large part of the work from scratch. It would generally be avoided by having quick design reviews before starting to code something difficult or involving sweeping changes. Just highlight how you are going to do it in a few sentences.
On the other hand, letting these kind of things go through usually means you will have to deal with the outcome later, and it will be more painful.
Maybe one way to decide about making this call or not is if you find the submitted version bad enough you are ready to implement the alternative yourself, immediately.
And it happens. With junior devs, or because people are new to the codebase.
That's a good way to put it. I wonder if draft PRs/MRs could be used for that? Make a draft PR/MR with the design, do a design review, then implement the code and finally the code review.
You just discovered pair programming.
It works astonishingly well. The second pair of eyes not only catches errors and envisions expanded use cases, it also prevents you from shirking off to HN. The biggest problem is you need two people, preferably sitting right next to each other with just one person “driving”.
The reason why this didn't catch on is that it's almost like a Mick and Keith sort of relationship. You can't just take any two musicians and throw them together and get the Rolling Stones and the same thing applies to pair programming.
Take a look at Bell Labs during the birth of UNIX for an entire SWARM of interdependent engineers. There's more than just a small element of good fortune! It seems those people were almost meant for one another.
I don't see that effective teamwork should necessarily depend on compatible personalities. I think the better comparison would be to surgery teams, or pilot crews, rather than art.
I think it would be wonderful to install a remote IDE with workspace on a remote server. Or to VNC/RDP into one.
If you're exploring a new concept and want all the ideas and brainstorming you can get in your feedback, "debug" log level is appropriate.
Once that idea has moved down the pipe, you may be down to "info" or "warn" depending on how much conversation has happened around the PR.
It's a code review, not a QA session.
I find that giving a good narrative gets the reviewer thinking like you or at least tells them why you chose one path and not another so that they don't waste time with a "why didn't you do it this way" question.
I keep trying to train my team do to that, but they're so focused on completing tickets they don't want to take the extra time to explain their "whys" and thought processes.
I find myself on the other side of this, all the time.
Suddenly someone comes up with some work that they have done without involving anyone else. I know this codebase, and I know moving that cache invalidation will make the codebase harder to understand, and also runs counter to the multi month effort you had to move all cache handling to the shared library which every other codebase in the product uses.
I try to be very careful about it, "have you considered using this other approach instead?" usually means unless you do this there will be the need for an even bigger rewrite in the future, but nobody can really take comfort in that.
The answer is that post facto code review is really unsuited for collaborative development. You start some work, you better involve other people straight away. Bounce ideas with a partner or two that really knows the codebase so you both understand what should be done and why. Unless you agree what it is your suggested change should achieve, there can be no useful code review!
When you are very a tiny team this mostly follows naturally, but always gets lost when the team grows and are split in several, and when individual developers starts to lose track of the codebase as a whole.
In practice, however, starting from the changes in a system that was previously working well enough is a very effective way of focusing limited human attention on where the problems are likely to be, optimizing for error detection with limited resources over the broader knowledge-dissemination goals espoused in this approach. If we are going to use diffs, then this brings us back around to the topic if the article.
Personally, I find having any form of diff embedded in what I am trying to understand just makes it harder to follow, so I move the diffs onto a secondary screen and use it as a guide and reference to what has been changed. The author of the article seems to want the same, but the mock-up is somewhat misleading, as it only has substitutions and additions. Where something is removed or refactored to another place, the two views will no longer line up as depicted here.
Architectural changes/discussion should be discussed by developers way before PR on slack or in a call. Most features should not change architecture and team should make effort to align architecture all the time at least before someone makes PR. Unless of course PR is PoC to showcase approach.
I'm old enough to have worked in the pre-code-review era. Things were fine. People still learned from each other, software could still be great or terrible, etc. It wasn't appreciably worse or better than things are today.
> An implicit question in several of the above is "will this set a good example for future contributions?"
Which in my experience can be an almost circular requirement. What do you consider a good example? As perfect as perfect can be? Rapid development? Extreme pragmatism?
The more experienced I get, the less I complain about in code review, especially when reviewing for a more junior dev, and especially for frequent comitters. People can only get so much out of any single code review, and any single commit can only do so much damage.
Put another way, code review is also about a level of trust. Will the committer be around next week? Are they on the same team as me? If yes, give them some leeway to commit incremental work and make improvements later. Not all incremental work need occur pre-commit. Mention areas for improvement, sure, but don't go overboard as a gatekeeper.
Things are obviously going to be different when reviewing code from what amounts to a stranger on a mission critical piece of code, etc.
I think this is very important, especially the part about incremental improvement. too many see development as laying concrete where it has to be perfect rather than as an ongoing process.
and personally the only thing I find PR's good for is ensuring jackasses aren't doing stupid shit. And by stupid shit here I mean things like using floats for currency (I caught that w/i the last year), things of that nature.
But my preference is to work with people I can trust and at that point I don't give a crap about a PR or a code review.
E.g. I'm kind of a human compiler; I can say things like, you have undefined behavior here, in a completely unfamiliar code base where the maintainer of that might miss the issue, too busy pointing out that it doesn't respect architectural conventions, and is not tested.
A code review is the "last line of defense" (well, disregarding CI), but in the teams I've worked in, that meant that the general idea of what the PR is introducing has been discussed by multiple people at that point. (This could be either a pairing session, an in-depth explanation, or just a coffee chat, depending on the complexity) This way the PR isn't about reviewing the general strategy (splitting off a new module, introducing a huge dependency, reworking the API surface), but just about reviewing the tactics used to implement that strategy.
Without the communication beforehand, doing only sanity checks on parts of the code in isolation does run the risk of fracturing code ownership. ("What's that module doing?" "Beats me, ask bob")
That all notwithstanding, I like the author's idea of what a diff view should look like, regardless of the "mode" of reviewing.
Call me crazy, but I've always approached code review from the standpoint of a tester: I build and run the code, see whether it does what it's supposed to do, look for possible weak points, and try to break it.
The diffs are the beginning rather than the end. They show you where to start poking around in the code. The proof is in the pudding, though.
You might respond, "That's what testers are for, not engineers." But who can test better than someone who understands the code and knows exactly where to look and what to look for?
I know this attitude puts me in the minority. The majority of engineers seem to have an inborn horror at the prospect of, gasp, manually running code. (They're also strangely averse to using debuggers.) They'll do anything in the world and write vast infrastructure to avoid it. Automate all the things! But in my view, automation is just more possibly buggy code. Who tests the tests? Quis custodiet ipsos custodes?
Automated tests are so that the code you just implemented still works next months, when lots of other people have made unrelated changes, and weren't always aware of the interactions with other parts of the code.
The automation is important, so that the tests get run, even when people forget or are under time pressure.
I fear half of my team can't run the application locally. To be honest, there are lots of moving parts in the codebase, but it's not only that.
Apropos "automation is just more possibly buggy code" is also a good argument for static typing. Good static typing can express (some of) your business logic, so that (some) invalid states are forbidden by the types.
The result is that quite a few invariants that need to be checked via tests in eg Python can be expressed in the type system in eg Haskell. The type checker already exists and is implemented as fairly battle hardened code, so this way you reduce the amount of new, possibly buggy code.
An example: in Go the convention is that when a function can go wrong, it returns a tuple of two values and exactly one of them is supposed to be the null pointer. But the compiler doesn't help you enforce that convention at all, so you need testing (and perhaps careful reasoning).
By contrast, Rust's Result type enlists the compiler to make sure that you return exactly either an error or a business-as-usual value.
I push through and do it anyhow, because this is one of those "they're not paying me to have an endless party" sort of things. But I completely 100% agree with the author that it is not a very useful view.
I don't necessarily disagree with you, but I don't agree either.
A typical workflow should be:
1. Engineer writes code 2. Engineer does manual and automated testing to verify things work correctly 3. Engineer commits, pushes, and creates a PR/MR 4. CI runs test suite 5. Another Engineer reviews PR/MR 6. Approval causes merge which causes deployment to staging 7. QA 8. UAT 9. Repeat steps 1-8 until everybody is happy
In this, we need to place checks on #1 and #2.
In my eyes, QA and UAT are superficial reviews/sanity checks on #2.
#3 is a check in #1. However, it relies on A) previous engineers implementing good tests and B) the current engineer doing the same. This means trusting people are doing the right things, but a Review implies you don't completely trust that.
#5 is a check on #3 and #1. They're essential. Having a full understanding of what the code looks like is more important in my mind than what was actually changed. The changes don't show how other code is interacting with those changes (which should be, but isn't always, captured by unit tests).
That's why I think the article author's preferred view is ideal. In fact, that's roughly what I work with when reviewing without realizing it (diff in GitLab/GitHub/Sublime Merge, code in JetBrains where most of the review happens).
By not having a basic process for defects, features, etc, every engineer has to make up and invent the process for how they will ship their feature through collaboration.
In most cases this ends up with doing little to nothing, with code review being the only form of collaboration on the topic that is in any meaningful detail. So we're left trying to stuff all possible interventions into a single moment.
Often -c is enough, but not always. I don't believe you can make a hard and fast rule. I think all you can really do is have a diff that allows you to 'jump to definition' or 'list callers', just like you can when looking at the code itself.
Then the another layer (which can be git, but also can be any other tool, adding custom diff tool to git is very easy) uses that to generate diffs.
There is zero stopping anyone from adding contextual diffs to Git. Just ask it for content of both commits and feed it to the algorithm.
Yes, git underneath stores data as diffs but they are only vaguely related to logical structure of commits
And that's why we call that lower level compression trick "delta", not "diff".
Linky: https://stackoverflow.com/questions/255202/how-do-i-view-git...
I see that patdiff also provides word-level diffing. You could instead get that feature with `git diff --word-diff` or Delta (https://github.com/dandavison/delta).
* A little scripting around opening the PR, which basically performs a "vimdiff <(git show baseref:file) file"-style dance on the changes(see :h diff). Using vim's tabs is great for this as they're really only views, so you can hold individual buffers open in distinct states at the same time.
* Scroll locking still works as expected in the main view, but you can avoid it in a separate tab when needed.
* [c and ]c move between hunks from the set of changes as they exist in the PR not in the working directory.
* dp and dg allow you to mark hunks as "done" by pushing/pulling the hunk in to the read-only diff buffer so that they're now hidden from the highlighted changes in the live buffer.
* Changes you make in the live buffer are available to commit directly, or push as a comment.
* All your regular editor things work as expected in the current state of the tree; go to definition, build integration, popup docs, etc.
Working like this means you're viewing changes against the PR's base, but have a clean working directory. That, to me, feels like a significant improvement over matklad's solution of having the working directory be in an unclean state to view the changes.
The environment I work in makes this behaviour super nice as changes will often be added with a --fixup commit, and then the tooling mangles them back together with a git-interpret-trailers call to attribute the fixup commit's author to the original commit at merge time. It also pulls text comments out of the PR and attaches a Reviewed-by trailer where appropriate, or the +1 equivalent to tack an Acked-by trailer on.
We use some internal tooling to make things work, but the concepts are generic git and $forge.
Push a comment: I'll make a hand-wavy suggested edit, then a vim mapping basically performs a ":`<,`>w !curl". If you only used GitHub, then piping to something like "gh pr comment"¹ could perform a similar-ish role(albeit a little weaker).
Pull comments: We automate merges so that PR text(including replies) are available in the repo. For the general discussion they're attached as notes², and for code comments(including --fixup commits) they're processed to assign the correct attribution via trailers³ to the original commit at merge time. Most of the attribution functionality could be re-implemented with "git rebase --autosquash" and by providing a "git interpret-trailers" wrapper as $EDITOR.
Code review is hard because the diff always looks reasonable, the tests always pass and all the basic stuff are always checked.
However, it happens often that, even if the changes looks reasonable they are wrong.
The whole architecture may drift after one bad change that looks reasonable.
As always this is not strictly a problem with the tooling, but more of culture and knowledge sharing.
And we are not going to solve it with a better tool.
> it happens often that, even if the changes looks reasonable they are wrong
I’m having trouble reconciling these two statements.
When I review code, I’m rarely looking for bugs. Instead I’m looking for tests that would catch those bugs.
(which is why you see some people write an history, because of the 'istory pronunciation)
His ideal diff is pretty much the same as a split-diff but with redundant context removed (context is only on the left, not on both left and right). What utility does he get out of removing redundant context on the RHS of the pane?
In my understanding the author wants to flip the diff around: instead of looking at changes themselves, the author wants to look at code and see if there are any associated changes.
The article is light on detail, but my guess would be that author wants to look at code and browse to implementation/callsite to check if appropriate changes are there.
That's the bit I don't understand - how does the "changes in the other" help if displayed only as the unified snippet[1]? To my mind, the magic sauce bit is the full code navigation, and there's no reason that the RHS pane has to be limited to a unified diff when it can simply show the whole file with higlighted lines, the way split diff views do on the RHS.
[1] Perhaps (and I'm only guessing here), that the RHS must show the entire unified diff for the entire changeset, and not just the unified diff for the current file. To me, that makes the proposal an upgrade from "either show unified diff, or split-diff, file-by-file".
> I need to run tests, use goto definition and other editor navigation features, apply local changes to check if some things could have been written differently, look at the wider context to notice things that should have been changed, and in general notice anything that might be not quite right with the codebase, irrespective of the historical path to the current state of the code.
The editors/IDEs I've used usually only offer rudimentary support in diffs (goto defn, but only in the same file; maybe auto-completion, but usually not for newly added items; no refactoring functionality)
Sure, but (to me it seems that) that doesn't necessitate the display change he is advocating for.
A diff program that has nice code navigation (and/or other IDE features) would be great, but what does that have to do with whether it is displaying the common split-diff, or his take on the split-diff.
If you use Gitlens's "compare working tree with..." you get the split view and you can edit the "current" version and all IDE tools work.
The diff highlighting can be surprisingly distracting and it can definitely help readability to turn it off. I certainly do turn it off from time to time in my diff viewer so I can see the code with fresh eyes, but I don't have the option to show the unified diff in a split pane.
By the way, this shortcut also works in Gitlab (just tried it)
I also like their 3-way merge capability https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif...
these features never made it into the web based diff tools that are widespread, I think the 3 way diff is a be a good way to show result of a difficult merge
This will open VSCode in browser, with the pull request in diff view.
Advantages:
- you see whole files there
- diff algo is different than on github.com, sometimes more readable for complex diffs
* S-L-O-W
* Glitchy
Which is a travesty because a side by side diff view with an out-of-line list of PR comments could be incredibly useful if it didn't involve spinning up a whole VSCode instance in the browser. In fact it's so useful that GH used to have a split view available without forcing people into their buzzword AI ML crypto blockchain cloud enabled dev environment nonsense.
Good idea, abysmal execution (which pretty much sums up all of GH these days I suppose).
FWIW I'm benchmarking this against a PR where github is showing ~43,000 lines changed so things that are manageable in smaller changesets don't always scale.
Edit: I should also add that while the split view does exist in the non-buzzword environment, and there's even a (ugh) combo box to allow you to navigate to the inline comments, the split view actually hides the comments so they're completely inaccessible. Using the combo box does… absolutely nothing.
Decent ideas, atrocious execution as is tradition.
https://www.npmjs.com/package/diff2html-cli
See the diff as HTML (side by side or unified)
E.g. that script that squashes the PR commits together - why would anyone need this, what is wrong with diffing the PR branch and the target branch, using any tool you want?
What forces you to look at the commit history?
I'm perfectly fine with the git CLI and IntellJ for local review work, and Gitlab web UI for the communicative part of a code review.
Only thing I agree with is, I (sometimes) hate the three-way unified diff in IntelliJ for merge/rebase conflicts. Other times (harmless conflicts), I love it over using the CLI.
Personally, I found the example for the desired diff format confusing.
I prefer split to unified diffs though.
It appears to enable choosing between unified and split views for each of those tools.
https://github.com/dandavison/delta/issues/535
Difftastic now has JSON output, whic should make it much easier to build this.
Maybe us plebs just use the diff viewer (or GitHub or) and the IDE/editor/terminal as separate applications.
This is spot on. In fact, I think modern code review practices over emphasize the historical path to the code at the expensive of lost quality of the present code and code architecture.
"Minimizing diffs" is a feature of modern code and PR practices, whether it's explicit or tacitly something the developers do. Optimizing for minimizing diffs discourages the continuous refactoring that code bases require to stay solid, sound, and visibly correct.
It seems to me it misses the mark a little -- the text is so verbose, it's easier to read the code itself.
Maybe code reviews on the raw source code, does not need to be confusing anymore. Code reviews on the description of the source code is better.
I tried to prompt the LLM to just summarize the code, and i didn't like the result. The description is indeed verbose and somewhat inefficient.
I tried "write some comments about the source code, in the style of codinghorror" and the results were fantastic.
I am very interested, if someone has found some other styles that work just as well.
Edit: All this to say, that in the space of LLMs and source code, there is a start-up which will be a github disruptor, and a new era of code will begin.
When working on a PR myself, I frequently avoid doing small, incremental commits because I find the subtle "these lines changed" annotations in IDEs extremely useful. It helps me find the locations in code that are relevant to my work.
I wish there was a way to configure e.g., IntelliJ to always show these markers relative to `main` instead of the last commit.
https://github.com/matklad/config/blob/master/xtool/src/gpr....
that way the problem of encountering new ideas through the darker medium of code and struggling to comprehend is solved, since now the ideas are presented and evaluated in a human language. the subsequent code review confirms that the implementation adheres to the gaveled proposal, and this can be easily done, even by a junior developer.
They're a diff tool. The author's approach is creative but seems like it's the wrong tool for the job.
I do think it's better actually, never thought about it. It might even function better if you could explode the changes out by clicking.
It is absolutely a good use of a human reviewer's time to build a mental model of the code's runtime behavior. But to do that by manually by reading each line and trying to predict what will happen when it's run is massively inefficient, incomplete, and error-prone. Plus it's susceptible to the "LGTM, fine, just merge it" phenomenon when the PR is large.
Reviews of static code listings won't reveal how ORMs structure their DB queries at runtime, or reflect how dynamically injected/configured components will behave, or any other number of things that are only visible by watching the code execute.
We have commoditized linting, checking for CVEs in dependencies, and static analysis for certain classes of bugs. We should now use fast runtime analysis in the same flow to relieve the burden from human reviewers of having to do line-by-line "telepathy reviews" where they try to magically divine how something will run in production at scale. (Full disclosure, I work at a company doing exactly that - https://appmap.io - and one of our most popular features is our sequence diagram diff that shows runtime differences between a PR and the main branch).
For large PRs with many files the problem is not so big because they are the sum of many small changes, file by file. Maybe a team should aim at small PRs but sometimes having to change X into Y everywhere, with a large X or Y, is an inescapable fact of life.
Probably not a perfect solution but `scroll-all-mode` should be pretty close, at least within a single file at a time.
It sounds like the author really wants pair programming
Pairing absolutely has its place, but it’s not always the most optimal use of time.
In a code review, the code on the right is where the focus is: is that correct?
If that change is merged, the right side version is what the code will be; the left side becomes a historic artifact indicating what the code was.
I glance on the left to understand what is changing: are some aspects changing that are not intended, and such.
There arise situations when a diff is total garbage, because code has moved around while being changed and whatnot. Sometimes unrelated code is diffed together. In the split diff you can still see the new code how it should be, but it's hard to track the changes.
In git, you can influence the diff algorithm to get a different diff, e.g. "git diff --diff-algorithm=minimal", documented as "spend extra time to make sure the smallest possible diff is produced.". This might be similar to GNU diff's --minimal option.
Anyway, I have used a FOSS that does that, difftastic [1], and it does a pretty good job at language diff'ing without the annoyance of formatting as I hypothesised earlier.
If, for some odd reason, I wanted to compare two codebases that had diverged, and one had gotten some serious formatting changes (maybe the new person preferred a different coding style), I'd run them both through an automatic formatter program with the same options. For C code, "indent" is commonly available, for instance.
Structured diffing requires structural understanding of the code. But only tool that is capable to properly understanding the code is compiler you are using to compiler, and even that only if you don't have mistakes. Version control and thus diffing needs to be robust. Knowing how many times I have seen intelisense (or similar IDE) fail due to various reasons, i wouldn't want similar when doing diff. You don't want it to fail just because you used a new language feature which isn't supported by diff tool yet. I have seen failures even in syntax highlighters which only need a very simplified understanding of code.
I could see moving away from line based diffs if we switched to structural code editing and source code was saved as some kind of AST instead of plaintext, and editors enforced that only valid source code(at least in structural sense) can be saved.
There can be middleground like what the difftastic does with fallback to text based diffing. But I consider that as nice to have but optional functionality. Seeing how many developers have strong attachment, to specific tools and their existing workflow, I am not surprised about lack of adoption. Developers might not even be against those specific improvements, but the adoption can be easily blocked by lack of integration, lack of some unrelated functionality in the software that does have the integration, setting up process feeling like more effort than benefits of slightly better diff. I have also heard plenty of times the argument of "If you need fancy tools for code to be readable/workable then the problem is in your code not the tools". While there is some grain of truth in it, that doesn't mean you can't use better tools while still writing code which could be understood without them.
Line based diff is in the good enough territory. There are cases where structural diff can do a lot more but with good fraction actual code edits the output of structural diff will be the same as line based diff+smarter diff viewers which highlight the parts of line that were modified (instead of whole line) and option to hide whitespace changes.
Change some spacing and the whole line is marked. Move an if statement 10 lines up and you see complete blocks of changes. Not even speaking about changed order of function implementations.
This seems a not-so-hard problem solved already a-million-times and I always fall back to use a local UI and configure it to use a commercial differ/merger I once bought a license for. Can't do that for code change reviews in the browser though.
The split view does show the actual new code, just with extra empty lines... so sure it is a bit distracting if a lot of lines have been modified, but it rarely comes that far.
My beef is more with some team's habit of doing too much work in a single pull request. Large diff are just a symptoms of that. I find that it is much more likely to introduce bugs and hard-to-find changes in behaviour in large changes. If you later find that something broke, finding what caused it in a large PR can be a real time waster.
Split diffs align well with modern "papier mache" development, where the goal of a task is to paste the smallest possible change onto whatever existing structure. Participants don't need to understand the whole structure and are expected not to alter it. Module refactoring is strongly discouraged and postponed until there's no other way to proceed on a critical feature. Designing (or refactoring) with an eye for future tasks is considered pointless because nobody understands which JIRA tickets are likely to survive and which will get purged or indefinitely backlogged.
When all you're supposed to be doing is overseeing Copilot as it drafts a new call to your upstream service and adds a perfunctory test that never fails, a split diff does a perfectly fine job of reviewing that work.
Whether this workflow represents durable, quality engineering or is just a way to LARP Katamari Damacy and get paid for it is another matter. As the enshittening continues year after year, it sure tends to look like the latter.
This essay is good food for thought, but it's just a peak into the dark forest that "move fast and break things" has been leading us into.
You can sort of do it with IntelliJ with git blame annotations but it’s clunky. I don’t really care about the author’s commits; I care about the entire set of changes or the changes since my last review.
You can also sort of do it with IntelliJ’s pull request mode but it’s also clunky since it’s not a “real” editor and you lose highlighting if you jump to the source code.
Yet it still feels fresh and there is no competing product that can challenge them. Most other diff tools lack good merge capabilities and are subpar in multitude of other ways.
IntelliJ does this very good. On the margin to your left you can see the age of each line. Recent changes - the one you are reviewing - will be be bright white, while older proven code more faded. All inside the already powerful editor you need to navigate bigger pieces of code.
Except that will not show you removed lines. Forget it, just use the diff feature.
So, as I understand it, you are advocating split diff with SUBTLER grey highlights in the current version of the file?
I might reuse the pattern to step through tutorial-type developments directly in-editor for presentations and videos.