12 ms·

Unified versus Split Diff

315 points by ingve 3y ago

friendzis 3y ago
> For a large change, I don’t want to do a “diff review”, I want to do a proper code review of a codebase at a particular instant in time, paying specific attention to the recently changed areas, 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.
- dylukes 3y ago
  > 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. 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?"
  - phillipcarter 3y ago
    As a counterpoint, individual code ownership can be a fantastic model too. It lets engineers specialize and takes advantage of social systems that form naturally anyways. I’ve not personally seen group ownership work well, and in practice, it’s still an individual who knows a given area the best.
    - sam_lowry_ 3y ago
      > reducing individual code ownership 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.
      - diarrhea 3y ago
        > an organization that is set up to reduce code ownership, and they struggle to attract talent 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?
        sam_lowry_ 3y ago
        Projects are fulfilling because they have public utility. The no individual code ownership policy is hard to bear for inquisitive minds, though. Thus the talent shortage.
        ilyt 3y ago
        You assume projects are fulfilling because they have "public utility" (whatever the fuck that means). That's your assumption, not objective fact. 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.
- lvncelot 3y ago
  I agree about what to expect from a code review - but for me, certain ways of working need to be in place for this to function properly. 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.
- robertlagrant 3y ago
  The problem is that this isn't a great way to catch areas of the codebase that should've been updated, but weren't.
- lapcat 3y ago
  In my book, superficial code review produces only superficial results. This is how so many bugs get shipped despite the ubiquity of code review. 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?
  - eru 3y ago
    Automated tests aren't so much to make sure that the test you just wrote works. You are right that manual testing can do that better sometimes. 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.
    - lapcat 3y ago
      To be clear, I'm not against automation. What we do for a living is write code, so when I say "automation is just more possibly buggy code", I'm not opposed writing more code (though I am opposed to overengineering). My point is that I don't place all of my faith in automated tests, and I'm not opposed to rolling up my sleeves, getting my hands dirty, and doing manual labor. Manual labor can be tedious, though, and that's probably why a lot of engineers hate it.
      - samus 3y ago
- delaaxe 3y ago
  I'd call the latter an audit
- jerf 3y ago
  I think you have some good truths in your post, but I also think they're rendered somewhat irrelevant by the fact that a "diff review" is, in my repeated and frequent experience, simply incomprehensible. I can't do any review with an incomprehensible mass of red and green lines interspersed with each other. It's not a useful view of the code, or at least not a useful primary view. 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.
- apocalyptic0n3 3y ago
  > unless the person doing the review is some sort of principal engineer mostly responsible for the code at large [..] In my book a general code review is simple sanity check by a second pair of eyes 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).
- codemac 3y ago
  Most of modern technology company culture has strayed too far from doing more formal processes for managing development. 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.
- gumby 3y ago
  For most changes I want more context than -c provides, because I see that your change is syntactically correct, but semantically, was your change from < to <= appropriate? I need to see the definition of the things you're comparing to. 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.
ibizaman 3y ago
I do the same! I checkout the branch of the PR then do a mixed reset to origin/main. This way, the git status shows all the changes. It’s really neat indeed.
juliangmp 3y ago
I haven't thought about this in detail much before but honestly that "view" the author proposed does look pretty nice. Though I think that something like difftastic is still a bit better, though I haven't properly integrated it into my workflow.
lelanthran 3y ago
I'm probably missing something here: author says that the split diff doesn't work for him, but doesn't say why. 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?
- friendzis 3y ago
  LHS shows current code, without any modifications. By showing full context in one view (with code browsing capabilities!) and changes in the other, the author is able to browse the code and see what changes, if any, are applied in a particular context. 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.
  - lelanthran 3y ago
    > LHS shows current code, without any modifications. By showing full context in one view (with code browsing capabilities!) and changes in the other, the author is able to browse the code and see what changes, if any, are applied in a particular context. 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".
- klauserc 3y ago
  The author writes > 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)
jessekv 3y ago
Meld works pretty well for these use cases. https://meldmerge.org/ https://meldmerge.org/
- ktpsns 3y ago
  A honorable mention is also https://kdiff3.sourceforge.net/ https://kdiff3.sourceforge.net/ which has a lot of nice views.
  - hbcondo714 3y ago
    That repo was last updated in 2014. Latest repo is here: https://invent.kde.org/sdk/kdiff3 https://invent.kde.org/sdk/kdiff3
- bsimpson 3y ago
  Looks a lot like FileMerge, which comes with the Apple developer tools.
  - __jonas 3y ago
    I love this style of diff, I am missing it in vscode after switching from JetBrains
nhinck2 3y ago
I think it's kind of funny that the left view is Word track changes. 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.
klauserc 3y ago
Ooooh, I need to try this! 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.
teddyh 3y ago
I often hear KDiff3 recommended in these cases. There are many graphical diff visualizers. I often use the ediff functions in Emacs.
- shiroiuma 3y ago
  Even though I'm a KDE fan, I usually use Meld for diffing, especially since I've set it up with a lot of custom filters for certain cases. However, when I want to compare 3 different files, kdiff3 is my go-to tool.
difosfor 3y ago
I’m surprised how we’re all still using line based diffs and even seemingly stupid ones at that. But the tales I hear from across the pond of paid git alternatives which diff based on understanding the programming language seem to be pretty bad as well. Though for other reasons?
- guiraldelli 3y ago
  It might be because of formatting, but that is exactly what I would have expected them to handle well. 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. [1]: https://github.com/Wilfred/difftastic https://github.com/Wilfred/difftastic
  - shiroiuma 3y ago
    Why would you want to diff code and ignore formatting? If it's just whitespace changes, any decent diff program should have an option to ignore that, but if it's more substantial, I would want to see that in a PR. 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.
- Karliss 3y ago
  The way I look at it, I doubt there will be widespread use of structural diffing while code editing itself is based on plain text files consisting of lines and characters. 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.
knubie 3y ago
A third (fourth?) option worth mentioning here is difftastic[0], which uses "structural" diffing (as opposed to line diffing) for more granular diff highlighting. [0] https://github.com/Wilfred/difftastic https://github.com/Wilfred/difftastic
- eviks 3y ago
  This is a great approach indeed, pity it's not integrated widely enough (think it only recently got a proper structured json output other tools could use)
- m463 3y ago
  ediff in emacs does this. Refine is what highlights the words that are different.
  - ParetoOptimal 3y ago
    This blog post also details using difftastic within emacs and has neat comparisons: https://shivjm.blog/better-magit-diffs/ https://shivjm.blog/better-magit-diffs/
- videlov 3y ago
  Thank you for sharing this project! I have been searching for something like this for some time
  - grotorea 3y ago
    I also wonder if it's possible to go beyond this project and have git itself work on the syntax level instead of pure text.
    - ilyt 3y ago
      Git doesn't store diffs on logical level. Git operates on snapshots of trees. Commit is not "a collection of changes", it's a snapshot of a tree with attached predecessor of it. 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
bilekas 3y ago
This might be a useage thing but author seems to already point out that for a large code review, these two options aren't sufficient, but I've never seen these tools offer themselves as a solution to a large review. They're a diff tool. The author's approach is creative but seems like it's the wrong tool for the job.
jakub_g 3y ago
FYI if using GitHub, on github.com pull request page, press `.`, or change domain name to github.dev. 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
- vasergen 3y ago
  thanks for the tip
- inferiorhuman 3y ago
  Disadvantages: * 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).
  - jakub_g 3y ago
    GH still has a split view available. Above the diff, you have a settings icon which shows dropdown where you can changed between unified and split diff.
    - inferiorhuman 3y ago
      Well that's certainly more intuitive than the old split / unified button. Unfortunately that basically revives the old split diff view and doesn't provide the useful out-of-band comments. Basically from a quick glance the buzzword view has a lot of good UI ideas that should have made it into the normal views as they have nothing to with actually running in a buzzword environment. 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.
altairprime 3y ago
That desired view looks a lot like Kaleidoscope (ksdiff). I thought it was a screenshot of that at first.
JNRowe 3y ago
That kinda feels like how I review with vim. * 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.
- kerneis 3y ago
  What do you mean exactly by "push as a comment" and "pulls text comments"? Is it some sort of custom logic specific to your work place?
  - JNRowe 3y ago
    Oops, poorly worded and using internal terminology. I'll try again ;) 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. ¹ https://cli.github.com/ https://cli.github.com/ ² https://git-scm.com/docs/git-notes https://git-scm.com/docs/git-notes ³ https://git-scm.com/docs/git-interpret-trailers https://git-scm.com/docs/git-interpret-trailers
avgcorrection 3y ago
> Sadly, this format of review isn’t well supported by the tools — everyone seems to be happy reviewing diffs, rather than the actual code? Maybe us plebs just use the diff viewer (or GitHub or) and the IDE/editor/terminal as separate applications.
yawboakye 3y ago
looks like the code review is happening too late. here the post’s author is also trying—hard—to reconstruct the mental states and models of the code author, especially in the large/significant update case. i argue that this workflow is a relic of the past, and should be replaced by a two-phase flow where in the beginning the essential ideas of the new changes are proposed, discussed, approved. then lines of code may be added or removed. the update is semantically grouped (perhaps via a commit) to correspond to key decisions made during phase one. 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.
amelius 3y ago
How does this work for deleted lines, since these don't have a corresponding line in the "current" view.
- matklad 3y ago
  Like this: https://user-images.githubusercontent.com/1711539/277602101-79179583-2c2e-46db-90c6-8acfd4f57b0e.png https://user-images.githubusercontent.com/1711539/277602101-...
  - videlov 3y ago
    I quite like your diff visualisation idea, thanks for sharing. Since I am currently working on a git client (gitbutler), I'm gonna experiment a little bit to see how it feels in practice
  - pierrebai 3y ago
    Your examples don't explain how both side can possibly be aligned when the differences are large. If they don't align, then the two view are independent and it's hard to see the correspondence. If they do align then... I'm not sure what is the difference between your suggestion and split diff, except th eleft side has less highlights. So, as I understand it, you are advocating split diff with SUBTLER grey highlights in the current version of the file?
iot_devs 3y ago
This is an huge issue for us in a very big and complex codebase with a lot of engineers working on it. 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.
- zmj 3y ago
  > the tests always pass > 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.
  - thfuran 3y ago
    Do you have tests that verify that an abstraction hasn't sprung a leak?
- talent_deprived 3y ago
  A somewhat minor nitpick, the word huge begins with a consonant sound, not a vowel sound. It would be correct to write "a huge issue" not "an huge issue".
  - beaugunderson 3y ago
    depends if you pronounce it 'uge or not ;P (which is why you see some people write an history, because of the 'istory pronunciation)
vifon 3y ago
> The only thing I don’t get is automatic synchronization between magit status buffer, and the file that’s currently open in the editor. That is, to view the current file and the diff on the side, I have to manually open the diff and scroll it to the point I am currently looking at. Probably not a perfect solution but `scroll-all-mode` should be pretty close, at least within a single file at a time.
pmontra 3y ago
In my experience unified diff is good for small changes. Split diff like meld is good for many changes in a long file. Many diffs in a long long file, you should not have such a file. 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.
- tome 3y ago
  Interesting. Seems to be the exact opposite preference to https://news.ycombinator.com/item?id=37997203 https://news.ycombinator.com/item?id=37997203!
brikelly 3y ago
It's interesting to me that the industry still relies so heavily on diffs of code listings as a primary method for code reviews. 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 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).
yboris 3y ago
My favorite tool ever: diff2html-cli - https://diff2html.xyz/ https://diff2html.xyz/ https://www.npmjs.com/package/diff2html-cli https://www.npmjs.com/package/diff2html-cli See the diff as HTML (side by side or unified)
dinckelman 3y ago
For me, it's always unified diff if i'm making changes to entire blocks of code, but split, if it's just parts of it. Easier to read, in my opinion
buro9 3y ago
I miss some of the capabilities from p4merge such as the split diff having a margin inbetween showing which parts of the file correspond to the other file, whilst also showing the impact of the diff on both sides: https://www.perforce.com/manuals/p4merge/Content/P4Merge/diff-merge.text_diff.html#Diffing_text_files https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif... I also like their 3-way merge capability https://www.perforce.com/manuals/p4merge/Content/P4Merge/diff-merge.p4merge.html#Navigate_diffs 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
grilledcheez 3y ago
> 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 It sounds like the author really wants pair programming
- btown 3y ago
  There are certainly situations where, as a reviewer, 5 minutes of goto-definition on a PR branch, or a well-placed debug breakpoint while running a test, is all you need to write a more prescriptive comment than “I don’t understand how this data flows” - and thus avoid both a follow up meeting and the need to pair program in the first place. Pairing absolutely has its place, but it’s not always the most optimal use of time.
btbuildem 3y ago
I was curious to see if anyone tried leveraging an LLM for summarizing diffs, and of course they have. An example here: https://github.com/anc95/ChatGPT-CodeReview/pull/21 https://github.com/anc95/ChatGPT-CodeReview/pull/21 It seems to me it misses the mark a little -- the text is so verbose, it's easier to read the code itself.
- emporas 3y ago
  I put GPT and Llama 70B to write me comments about the Rust language source code. They are both superb at this task. I haven't used any other weaker LLMs in the same fashion, with less billions of parameters, but i can imagine many of them, in the range of 7B to 13B, will perform good enough. I will test more LLMs soon though. 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.
phailhaus 3y ago
If you hit `.` in GitHub, you'll get dropped into a full IDE inside the browser. I've found this to be invaluable for reviews, because it lets you see the changes within the context of the entire file, rather than just seeing snippets. I'm much more likely to catch subtle design issues that way.
- jtreminio 3y ago
  This is game changing, thank you for telling us this.
- Offpics 3y ago
  Thats bonkers, thanks
- flanbiscuit 3y ago
  ok this is really cool, thanks for the tip! I've been in this IDE before but only when making edits to files directly in Github. I did not know I could shortcut to it directly from a commit diff. By the way, this shortcut also works in Gitlab (just tried it)
larperdoodle 3y ago
I do all my large code reviews in IntelliJ. Seems like it's an exact match of what this guy wants? Diff view + full view + fully featured IDE for all his navigation, verification and search needs
- krzyk 3y ago
  Would be great, but it needs integration. In my case there is no such thing for bitbucket server.
- sa46 3y ago
  What the author wants (and I do too) is a normal, full fledged editor that highlights the code under review. Bonus points for easy commenting. 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.
e12e 3y ago
What is "a gpr script"? It looks like a small rust file that presumably compile to binary custom utility? https://github.com/matklad/config/blob/master/xtool/src/gpr.rs https://github.com/matklad/config/blob/master/xtool/src/gpr....
- gpvos 3y ago
  It seems to be a script/program that just happens to be called gpr. I don't know the meaning of the abbreviation.
password4321 3y ago
Semi-related, I recently discovered https://github.com/dandavison/delta https://github.com/dandavison/delta: A syntax-highlighting pager for git, diff, and grep output It appears to enable choosing between unified and split views for each of those tools.
- nh2 3y ago
  I'm currently waiting on the integration between Delta and Difftastic: https://github.com/dandavison/delta/issues/535 https://github.com/dandavison/delta/issues/535 Difftastic now has JSON output, whic should make it much easier to build this.
dndn1 3y ago
Neat, I also far prefer using in-editor staging & commiting workflows which are excellent by necessity; history tools by comparison are weak unless I'm missing something. So this seems really useful. I might reuse the pattern to step through tutorial-type developments directly in-editor for presentations and videos.
ramesh31 3y ago
The only real solution to this is "avoid big diffs at all cost". This is why the industry as a whole has mostly dropped git flow and moved back to trunk based development.
swatcoder 3y ago
In contemporary software "engineering" culture, this points at a much deeper issue than the scope of diffs that get presented in review tools. 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.
gpvos 3y ago
The author complains that he doesn't see all of the code, but in the first image it looks like the right-hand pane shows exactly that (though with diff highlighting). Am I missing something?
- zzzeek 3y ago
  agree, I use side-by-side diff tools and the right side is where "the code" actually is. dont really get what issue the author has with this unless it's that the highlighting distracts them.
wellpast 3y ago
> For a large change, I don’t want to do a “diff review”, I want to do a proper code review of a codebase at a particular instant in time, paying specific attention to the recently changed areas, but mostly just doing general review, as if I am writing the code. 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. 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.
moritzwarhier 3y ago
I admit I don't understand some of the problems mentioned in the article. 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.
aaaronic 3y ago
I feel like I used to use a tool called Beyond Compare that presented _many_ different views of a diff (and even supported N-way diffs) back around 2012.
- recov 3y ago
  It's still around, and still very useful for all kinds of comparisons when you need a UI https://www.scootersoftware.com/ https://www.scootersoftware.com/
  - Too 3y ago
    It’s still around and has hardly changed one pixel since 2012! 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.
    - roryokane 3y ago
      P4Merge (https://www.perforce.com/products/helix-core-apps/merge-diff-tool-p4merge https://www.perforce.com/products/helix-core-apps/merge-diff...) is a similarly powerful (and similarly ugly) tool for plain text diffs and three-way merges. It’s free, and though the 500 MB download includes Perforce, you can install P4Merge by itself. Unlike Beyond Compare, though, P4Merge cannot highlight syntax in the compared files.
Too 3y ago
What the author wants is git blame with highlights. 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.
kazinator 3y ago
For code review, the top one works for me. 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.
nuancebydefault 3y ago
I'm amazed that default diff tools that come with git CLI or even git UI clients are so freakin' the opposite of intelligent. Even github or Atlassian web interfaces. No matter unified or split. 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.
- Vogtinator 3y ago
  You can usually hide/ignore whitespace changes which result in the kind of diff you want.
  - nuancebydefault 3y ago
    In practice, reviewers are "nitpicking" on white space as well. Before passing my code to a reviewer, it helps to just show the changes of just that.
pierrebai 3y ago
"everyone seems to be happy reviewing diffs, rather than the actual code?" 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.
robertnicholas 3y ago
[dead]
IvanJohnnie 3y ago
[dead]
benson110 3y ago
[dead]

315 points by ingve 3y ago

> For a large change, I don’t want to do a “diff review”, I want to do a proper code review of a codebase at a particular instant in time, paying specific attention to the recently changed areas, 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.

dylukes 3y ago
> 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. 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?"
- phillipcarter 3y ago
  As a counterpoint, individual code ownership can be a fantastic model too. It lets engineers specialize and takes advantage of social systems that form naturally anyways. I’ve not personally seen group ownership work well, and in practice, it’s still an individual who knows a given area the best.
  - sam_lowry_ 3y ago
    > reducing individual code ownership 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.
    - diarrhea 3y ago
      > an organization that is set up to reduce code ownership, and they struggle to attract talent 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?
      - sam_lowry_ 3y ago
        Projects are fulfilling because they have public utility. The no individual code ownership policy is hard to bear for inquisitive minds, though. Thus the talent shortage.
        ilyt 3y ago
        You assume projects are fulfilling because they have "public utility" (whatever the fuck that means). That's your assumption, not objective fact. 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.
lvncelot 3y ago
I agree about what to expect from a code review - but for me, certain ways of working need to be in place for this to function properly. 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.
robertlagrant 3y ago
The problem is that this isn't a great way to catch areas of the codebase that should've been updated, but weren't.
lapcat 3y ago
In my book, superficial code review produces only superficial results. This is how so many bugs get shipped despite the ubiquity of code review. 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?
- eru 3y ago
  Automated tests aren't so much to make sure that the test you just wrote works. You are right that manual testing can do that better sometimes. 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.
  - lapcat 3y ago
    To be clear, I'm not against automation. What we do for a living is write code, so when I say "automation is just more possibly buggy code", I'm not opposed writing more code (though I am opposed to overengineering). My point is that I don't place all of my faith in automated tests, and I'm not opposed to rolling up my sleeves, getting my hands dirty, and doing manual labor. Manual labor can be tedious, though, and that's probably why a lot of engineers hate it.
    - samus 3y ago
delaaxe 3y ago
I'd call the latter an audit
jerf 3y ago
I think you have some good truths in your post, but I also think they're rendered somewhat irrelevant by the fact that a "diff review" is, in my repeated and frequent experience, simply incomprehensible. I can't do any review with an incomprehensible mass of red and green lines interspersed with each other. It's not a useful view of the code, or at least not a useful primary view. 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.
apocalyptic0n3 3y ago
> unless the person doing the review is some sort of principal engineer mostly responsible for the code at large [..] In my book a general code review is simple sanity check by a second pair of eyes 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).
codemac 3y ago
Most of modern technology company culture has strayed too far from doing more formal processes for managing development. 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.
gumby 3y ago
For most changes I want more context than -c provides, because I see that your change is syntactically correct, but semantically, was your change from < to <= appropriate? I need to see the definition of the things you're comparing to. 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.

ibizaman 3y ago

I do the same! I checkout the branch of the PR then do a mixed reset to origin/main. This way, the git status shows all the changes. It’s really neat indeed.

juliangmp 3y ago

I haven't thought about this in detail much before but honestly that "view" the author proposed does look pretty nice. Though I think that something like difftastic is still a bit better, though I haven't properly integrated it into my workflow.

lelanthran 3y ago

I'm probably missing something here: author says that the split diff doesn't work for him, but doesn't say why. 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?

friendzis 3y ago
LHS shows current code, without any modifications. By showing full context in one view (with code browsing capabilities!) and changes in the other, the author is able to browse the code and see what changes, if any, are applied in a particular context. 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.
- lelanthran 3y ago
  > LHS shows current code, without any modifications. By showing full context in one view (with code browsing capabilities!) and changes in the other, the author is able to browse the code and see what changes, if any, are applied in a particular context. 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".
klauserc 3y ago
The author writes > 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)

jessekv 3y ago

Meld works pretty well for these use cases. https://meldmerge.org/ https://meldmerge.org/

ktpsns 3y ago
A honorable mention is also https://kdiff3.sourceforge.net/ https://kdiff3.sourceforge.net/ which has a lot of nice views.
- hbcondo714 3y ago
  That repo was last updated in 2014. Latest repo is here: https://invent.kde.org/sdk/kdiff3 https://invent.kde.org/sdk/kdiff3
bsimpson 3y ago
Looks a lot like FileMerge, which comes with the Apple developer tools.
- __jonas 3y ago
  I love this style of diff, I am missing it in vscode after switching from JetBrains

nhinck2 3y ago

I think it's kind of funny that the left view is Word track changes. 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.

klauserc 3y ago

Ooooh, I need to try this! 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.

teddyh 3y ago

I often hear KDiff3 recommended in these cases. There are many graphical diff visualizers. I often use the ediff functions in Emacs.

shiroiuma 3y ago
Even though I'm a KDE fan, I usually use Meld for diffing, especially since I've set it up with a lot of custom filters for certain cases. However, when I want to compare 3 different files, kdiff3 is my go-to tool.

difosfor 3y ago

I’m surprised how we’re all still using line based diffs and even seemingly stupid ones at that. But the tales I hear from across the pond of paid git alternatives which diff based on understanding the programming language seem to be pretty bad as well. Though for other reasons?

guiraldelli 3y ago
It might be because of formatting, but that is exactly what I would have expected them to handle well. 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. [1]: https://github.com/Wilfred/difftastic https://github.com/Wilfred/difftastic
- shiroiuma 3y ago
  Why would you want to diff code and ignore formatting? If it's just whitespace changes, any decent diff program should have an option to ignore that, but if it's more substantial, I would want to see that in a PR. 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.
Karliss 3y ago
The way I look at it, I doubt there will be widespread use of structural diffing while code editing itself is based on plain text files consisting of lines and characters. 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.

knubie 3y ago

A third (fourth?) option worth mentioning here is difftastic[0], which uses "structural" diffing (as opposed to line diffing) for more granular diff highlighting. [0] https://github.com/Wilfred/difftastic https://github.com/Wilfred/difftastic

eviks 3y ago
This is a great approach indeed, pity it's not integrated widely enough (think it only recently got a proper structured json output other tools could use)
m463 3y ago
ediff in emacs does this. Refine is what highlights the words that are different.
- ParetoOptimal 3y ago
  This blog post also details using difftastic within emacs and has neat comparisons: https://shivjm.blog/better-magit-diffs/ https://shivjm.blog/better-magit-diffs/
videlov 3y ago
Thank you for sharing this project! I have been searching for something like this for some time
- grotorea 3y ago
  I also wonder if it's possible to go beyond this project and have git itself work on the syntax level instead of pure text.
  - ilyt 3y ago
    Git doesn't store diffs on logical level. Git operates on snapshots of trees. Commit is not "a collection of changes", it's a snapshot of a tree with attached predecessor of it. 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

bilekas 3y ago

This might be a useage thing but author seems to already point out that for a large code review, these two options aren't sufficient, but I've never seen these tools offer themselves as a solution to a large review. They're a diff tool. The author's approach is creative but seems like it's the wrong tool for the job.

jakub_g 3y ago

FYI if using GitHub, on github.com pull request page, press `.`, or change domain name to github.dev. 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

vasergen 3y ago
thanks for the tip
inferiorhuman 3y ago
Disadvantages: * 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).
- jakub_g 3y ago
  GH still has a split view available. Above the diff, you have a settings icon which shows dropdown where you can changed between unified and split diff.
  - inferiorhuman 3y ago
    Well that's certainly more intuitive than the old split / unified button. Unfortunately that basically revives the old split diff view and doesn't provide the useful out-of-band comments. Basically from a quick glance the buzzword view has a lot of good UI ideas that should have made it into the normal views as they have nothing to with actually running in a buzzword environment. 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.

altairprime 3y ago

That desired view looks a lot like Kaleidoscope (ksdiff). I thought it was a screenshot of that at first.

JNRowe 3y ago

That kinda feels like how I review with vim. * 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.

kerneis 3y ago
What do you mean exactly by "push as a comment" and "pulls text comments"? Is it some sort of custom logic specific to your work place?
- JNRowe 3y ago
  Oops, poorly worded and using internal terminology. I'll try again ;) 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. ¹ https://cli.github.com/ https://cli.github.com/ ² https://git-scm.com/docs/git-notes https://git-scm.com/docs/git-notes ³ https://git-scm.com/docs/git-interpret-trailers https://git-scm.com/docs/git-interpret-trailers

avgcorrection 3y ago

> Sadly, this format of review isn’t well supported by the tools — everyone seems to be happy reviewing diffs, rather than the actual code? Maybe us plebs just use the diff viewer (or GitHub or) and the IDE/editor/terminal as separate applications.

yawboakye 3y ago

looks like the code review is happening too late. here the post’s author is also trying—hard—to reconstruct the mental states and models of the code author, especially in the large/significant update case. i argue that this workflow is a relic of the past, and should be replaced by a two-phase flow where in the beginning the essential ideas of the new changes are proposed, discussed, approved. then lines of code may be added or removed. the update is semantically grouped (perhaps via a commit) to correspond to key decisions made during phase one. 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.

amelius 3y ago

How does this work for deleted lines, since these don't have a corresponding line in the "current" view.

matklad 3y ago
Like this: https://user-images.githubusercontent.com/1711539/277602101-79179583-2c2e-46db-90c6-8acfd4f57b0e.png https://user-images.githubusercontent.com/1711539/277602101-...
- videlov 3y ago
  I quite like your diff visualisation idea, thanks for sharing. Since I am currently working on a git client (gitbutler), I'm gonna experiment a little bit to see how it feels in practice
- pierrebai 3y ago
  Your examples don't explain how both side can possibly be aligned when the differences are large. If they don't align, then the two view are independent and it's hard to see the correspondence. If they do align then... I'm not sure what is the difference between your suggestion and split diff, except th eleft side has less highlights. So, as I understand it, you are advocating split diff with SUBTLER grey highlights in the current version of the file?

iot_devs 3y ago

This is an huge issue for us in a very big and complex codebase with a lot of engineers working on it. 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.

zmj 3y ago
> the tests always pass > 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.
- thfuran 3y ago
  Do you have tests that verify that an abstraction hasn't sprung a leak?
talent_deprived 3y ago
A somewhat minor nitpick, the word huge begins with a consonant sound, not a vowel sound. It would be correct to write "a huge issue" not "an huge issue".
- beaugunderson 3y ago
  depends if you pronounce it 'uge or not ;P (which is why you see some people write an history, because of the 'istory pronunciation)

vifon 3y ago

> The only thing I don’t get is automatic synchronization between magit status buffer, and the file that’s currently open in the editor. That is, to view the current file and the diff on the side, I have to manually open the diff and scroll it to the point I am currently looking at. Probably not a perfect solution but `scroll-all-mode` should be pretty close, at least within a single file at a time.

pmontra 3y ago

In my experience unified diff is good for small changes. Split diff like meld is good for many changes in a long file. Many diffs in a long long file, you should not have such a file. 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.

tome 3y ago
Interesting. Seems to be the exact opposite preference to https://news.ycombinator.com/item?id=37997203 https://news.ycombinator.com/item?id=37997203!

brikelly 3y ago

It's interesting to me that the industry still relies so heavily on diffs of code listings as a primary method for code reviews. 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 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).

yboris 3y ago

My favorite tool ever: diff2html-cli - https://diff2html.xyz/ https://diff2html.xyz/ https://www.npmjs.com/package/diff2html-cli https://www.npmjs.com/package/diff2html-cli See the diff as HTML (side by side or unified)

dinckelman 3y ago

For me, it's always unified diff if i'm making changes to entire blocks of code, but split, if it's just parts of it. Easier to read, in my opinion

buro9 3y ago

I miss some of the capabilities from p4merge such as the split diff having a margin inbetween showing which parts of the file correspond to the other file, whilst also showing the impact of the diff on both sides: https://www.perforce.com/manuals/p4merge/Content/P4Merge/diff-merge.text_diff.html#Diffing_text_files https://www.perforce.com/manuals/p4merge/Content/P4Merge/dif... I also like their 3-way merge capability https://www.perforce.com/manuals/p4merge/Content/P4Merge/diff-merge.p4merge.html#Navigate_diffs 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

grilledcheez 3y ago

> 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 It sounds like the author really wants pair programming

btown 3y ago
There are certainly situations where, as a reviewer, 5 minutes of goto-definition on a PR branch, or a well-placed debug breakpoint while running a test, is all you need to write a more prescriptive comment than “I don’t understand how this data flows” - and thus avoid both a follow up meeting and the need to pair program in the first place. Pairing absolutely has its place, but it’s not always the most optimal use of time.

btbuildem 3y ago

I was curious to see if anyone tried leveraging an LLM for summarizing diffs, and of course they have. An example here: https://github.com/anc95/ChatGPT-CodeReview/pull/21 https://github.com/anc95/ChatGPT-CodeReview/pull/21 It seems to me it misses the mark a little -- the text is so verbose, it's easier to read the code itself.

emporas 3y ago
I put GPT and Llama 70B to write me comments about the Rust language source code. They are both superb at this task. I haven't used any other weaker LLMs in the same fashion, with less billions of parameters, but i can imagine many of them, in the range of 7B to 13B, will perform good enough. I will test more LLMs soon though. 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.

phailhaus 3y ago

If you hit `.` in GitHub, you'll get dropped into a full IDE inside the browser. I've found this to be invaluable for reviews, because it lets you see the changes within the context of the entire file, rather than just seeing snippets. I'm much more likely to catch subtle design issues that way.

jtreminio 3y ago
This is game changing, thank you for telling us this.
Offpics 3y ago
Thats bonkers, thanks
flanbiscuit 3y ago
ok this is really cool, thanks for the tip! I've been in this IDE before but only when making edits to files directly in Github. I did not know I could shortcut to it directly from a commit diff. By the way, this shortcut also works in Gitlab (just tried it)

larperdoodle 3y ago

I do all my large code reviews in IntelliJ. Seems like it's an exact match of what this guy wants? Diff view + full view + fully featured IDE for all his navigation, verification and search needs

krzyk 3y ago
Would be great, but it needs integration. In my case there is no such thing for bitbucket server.
sa46 3y ago
What the author wants (and I do too) is a normal, full fledged editor that highlights the code under review. Bonus points for easy commenting. 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.

e12e 3y ago

What is "a gpr script"? It looks like a small rust file that presumably compile to binary custom utility? https://github.com/matklad/config/blob/master/xtool/src/gpr.rs https://github.com/matklad/config/blob/master/xtool/src/gpr....

gpvos 3y ago
It seems to be a script/program that just happens to be called gpr. I don't know the meaning of the abbreviation.

password4321 3y ago

Semi-related, I recently discovered https://github.com/dandavison/delta https://github.com/dandavison/delta: A syntax-highlighting pager for git, diff, and grep output It appears to enable choosing between unified and split views for each of those tools.

nh2 3y ago
I'm currently waiting on the integration between Delta and Difftastic: https://github.com/dandavison/delta/issues/535 https://github.com/dandavison/delta/issues/535 Difftastic now has JSON output, whic should make it much easier to build this.

dndn1 3y ago

Neat, I also far prefer using in-editor staging & commiting workflows which are excellent by necessity; history tools by comparison are weak unless I'm missing something. So this seems really useful. I might reuse the pattern to step through tutorial-type developments directly in-editor for presentations and videos.

ramesh31 3y ago

The only real solution to this is "avoid big diffs at all cost". This is why the industry as a whole has mostly dropped git flow and moved back to trunk based development.

swatcoder 3y ago

In contemporary software "engineering" culture, this points at a much deeper issue than the scope of diffs that get presented in review tools. 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.

gpvos 3y ago

The author complains that he doesn't see all of the code, but in the first image it looks like the right-hand pane shows exactly that (though with diff highlighting). Am I missing something?

zzzeek 3y ago
agree, I use side-by-side diff tools and the right side is where "the code" actually is. dont really get what issue the author has with this unless it's that the highlighting distracts them.

wellpast 3y ago

> For a large change, I don’t want to do a “diff review”, I want to do a proper code review of a codebase at a particular instant in time, paying specific attention to the recently changed areas, but mostly just doing general review, as if I am writing the code. 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. 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.

moritzwarhier 3y ago

I admit I don't understand some of the problems mentioned in the article. 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.

aaaronic 3y ago

I feel like I used to use a tool called Beyond Compare that presented _many_ different views of a diff (and even supported N-way diffs) back around 2012.

recov 3y ago
It's still around, and still very useful for all kinds of comparisons when you need a UI https://www.scootersoftware.com/ https://www.scootersoftware.com/
- Too 3y ago
  It’s still around and has hardly changed one pixel since 2012! 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.
  - roryokane 3y ago
    P4Merge (https://www.perforce.com/products/helix-core-apps/merge-diff-tool-p4merge https://www.perforce.com/products/helix-core-apps/merge-diff...) is a similarly powerful (and similarly ugly) tool for plain text diffs and three-way merges. It’s free, and though the 500 MB download includes Perforce, you can install P4Merge by itself. Unlike Beyond Compare, though, P4Merge cannot highlight syntax in the compared files.

Too 3y ago

What the author wants is git blame with highlights. 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.

kazinator 3y ago

For code review, the top one works for me. 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.

nuancebydefault 3y ago

I'm amazed that default diff tools that come with git CLI or even git UI clients are so freakin' the opposite of intelligent. Even github or Atlassian web interfaces. No matter unified or split. 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.

Vogtinator 3y ago
You can usually hide/ignore whitespace changes which result in the kind of diff you want.
- nuancebydefault 3y ago
  In practice, reviewers are "nitpicking" on white space as well. Before passing my code to a reviewer, it helps to just show the changes of just that.

pierrebai 3y ago

"everyone seems to be happy reviewing diffs, rather than the actual code?" 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.

robertnicholas 3y ago

[dead]

IvanJohnnie 3y ago

[dead]

benson110 3y ago

[dead]