Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Unified versus Split Diff

matklad.github.io

Post reply on HN
Print view View original

61–70 of 184 posts

Re: Unified versus Split Diff

#61

Post by thfuran »

eru wrote: post #46
yarekt wrote: post #34

Earlier quoted context omitted.

This is called Continuous Integration, and its a shame that the term got nicked to now mean "that thing that builds our PRs somewhere". The idea was that everyone pushes to main all the time, which basically reduces integration time to 0, as everyone is doing it every couple of minutes on big teams. After a while you learn how to not step on people's toes (Introduce new classes incrementally, use docblocs documenting…

Everyone committing to the main branch all the time sounds like a nightmare to me, to be honest. I would probably seriously consider quitting if this was forced on me. (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?

Let alone pushing every couple of minutes.

Re: Unified versus Split Diff

#62

Post by eru »

lapcat wrote: post #57
friendzis wrote: post #2

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

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

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.

Re: Unified versus Split Diff

#63

Post by brikelly »

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 - and one of our most popular features is our sequence diagram diff that shows runtime differences between a PR and the main branch).

Re: Unified versus Split Diff

#66

Post by atombender »

CraigJPerry wrote: post #25
klauserc wrote: post #14

Earlier quoted context omitted.

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…

One area where vscode+gitlens plugin beats IntelliJ, the plugin adds in fully featured editable RHS to diff view

IntelliJ's spli diff view is a full editor with code completion, formatting, symbol lookup, and everything.

Re: Unified versus Split Diff

#67

Post by pmezard »

bernds74 wrote: post #44
crdrost wrote: post #29

Earlier quoted context omitted.

I go back and forth on this. 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, t…

"I would have done it from this other approach". I've seen that, and it's not good when you get the feeling of "code review is when someone who hasn't thought about your problem tells you how you should have solved it". People sometimes feel they have to add value as a reviewer, and casually discarding other people's work is the way to do it. Fortunately it's not something I have to deal with at my current job.

Another way to frame it is "code review is not the right place to validate design decisions".

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.

Re: Unified versus Split Diff

#68

Post by buro9 »

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/dif...

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

Re: Unified versus Split Diff

#69

Post by inferiorhuman »

jakub_g wrote: post #31

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

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).

Re: Unified versus Split Diff

#70

Post by Arch-TK »

eru wrote: post #46
yarekt wrote: post #34

Earlier quoted context omitted.

This is called Continuous Integration, and its a shame that the term got nicked to now mean "that thing that builds our PRs somewhere". The idea was that everyone pushes to main all the time, which basically reduces integration time to 0, as everyone is doing it every couple of minutes on big teams. After a while you learn how to not step on people's toes (Introduce new classes incrementally, use docblocs documenting…

Everyone committing to the main branch all the time sounds like a nightmare to me, to be honest. I would probably seriously consider quitting if this was forced on me. (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?

To be fair. I have never really heard of pushing to master as a CI technique for git. You would push to your own publicly visible branch and some script would automatically attempt to merge into a CI branch unless there's merge conflicts in which case you get notified. Then as you add things you get notified if the whole thing is breaking and get made aware of upcoming issues.
Post reply on HN