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

Some of us like "interdiff" code review

gist.github.com

Post reply on HN
Print view View original

131–140 of 224 posts

Re: Some of us like "interdiff" code review

#131

Post by pinkorchid »

dietr1ch wrote: post #125
riquito wrote: post #121

Earlier quoted context omitted.

You can add to your ~/.gitconfig [rebase] autosquash = true from then on `git rebase -i origin/main` automatically reorder fixup and squash commits. It's a small thing but greatly improved my workflow

Squashing is nice because it keeps the amount of commits minimal, but as mentioned in the article, it has the problem that now reviewers have to figure out what changed since their last review, which can get hard to do if the tree of changes originally proposed needed updates in dependencies of the commits they reviewed. I really like the idea that I'm working on a tree (often a stack/list) of changes, and as the rev…

> the problem that now reviewers have to figure out what changed

That's something that the review tool can solve, and I agree that github doesn't handle it well. But other code review tools can show diffs between revisions independently of how the commits have been rebased or squashed over time.

Re: Some of us like "interdiff" code review

#132

Post by kspacewalk2 »

globular-toast wrote: post #60

GitLab supports this. Every time someone pushes or force pushes it tags that as a version which you can diff. If your developers know how to generate new commits then you can do it right away with GitLab. The problem is generating the new commits. Developers just aren't very good at doing this. They can modify a single commit just fine, but modify a commit that isn't the latest commit involves a rebase. Magit has the…

Rewriting history and breaking N sloppy commits into M well-thought-out, logical commits is an essential git-based version control skill for developers. Thus, interactive rebase should be considered essential for anyone using git for anything non-trivial. It's TUI-like interface is a bit quirky for some people, but it is rock-solid once you figure it out and therefore worth investing a bit of time into learning. (That also describes git in its entirety quite well).

Re: Some of us like "interdiff" code review

#133

Post by tome »

Piraty wrote: post #88
Vinnl wrote: post #83

I'm using mostly this workflow with GitHub, with the main disadvantages being that it's more work on my side, and not obvious to my collaborators. But it does carry the same advantages of allowing reviewers to view diffs with just their feedback incorporated, without breaking `git blame` and `git bisect`. When I incorporate a reviewer's feedback, I'll commit that with `git commit --fixup `. I'll then push that up and…

https://news.ycombinator.com/item?id=37086022

Wow, my jaw is on the floor. This is such a good idea! I already had the idea of tracking issues in the same repo as the code -- not that I actually use that idea -- but I didn't have the idea of doing PRs in the repo itself. Love it!

Re: Some of us like "interdiff" code review

#134

Post by ipaddr »

faangguyindia wrote: post #109
Vinnl wrote: post #83

I'm using mostly this workflow with GitHub, with the main disadvantages being that it's more work on my side, and not obvious to my collaborators. But it does carry the same advantages of allowing reviewers to view diffs with just their feedback incorporated, without breaking `git blame` and `git bisect`. When I incorporate a reviewer's feedback, I'll commit that with `git commit --fixup `. I'll then push that up and…

Biggest problem working with Git is remembering all these flags and commands. If you do it everyday for years, sure you remember most of them. But for weekend hackers or those who specialise in some other areas, it creates a lot of frustration. I often forget all these flags etc.... My brother made this https://github.com/zerocorebeta/Option-K This enables me to simply write in thermal, "interactive rebase main, auto…

If you are a weekend warrior you are not working with a team. Just commit and push.

Re: Some of us like "interdiff" code review

#135

Post by SlySherZ »

aseipp wrote: post #40
deathanatos wrote: post #35

Yes! This is what I imagine in my head as a real code review style, not the stuff Github does. Glad to have a name for it. I'd add I'd also like my review system to be able to kick patches "out" of the review once they're ready. E.g., the small bugfixes that you make while working on that bigger feature should hopefully be small, isolated patches, ones that are going to find consensus with a reviewer quite quickly. O…

I keep saying this over and over but, Gerrit basically does that. :) You can see the relationships between any two patches on Gerrit, and more importantly, Gerrit shows you each patch individually . So you can see in a series A -> B -> C that yeah, B is small, let's go ahead and get that in. Part of this is that UX has some really smart ideas like the "Attention Set". The attention set is basically "Which people need…

Do you know how to create a local branch that tracks a gerrit commit? Usually I just do git commit --amend to update gerrit, but then I lose access to my patch's history (it's still on gerrit, but I want it locally).

Re: Some of us like "interdiff" code review

#136

Post by rosmax_1337 »

aseipp wrote: post #33
rosmax_1337 wrote: post #20

Earlier quoted context omitted.

What is the meaning of life?

I know this is in jest, but I'll just take the opportunity to respond by posting my favorite poem. The relationship between it and your question -- well, that's for you to decide. The birds have vanished down the sky. Now the last cloud drains away. We sit together, the mountain and me, until only the mountain remains. -- Zazen on Ching-t’ing Mountain

The question was sincere yet playful. I appreciate your answer. Here's a poem I like.

  O'er all the hilltops
  Is quiet now,
  In all the treetops
  Hearest thou
  Hardly a breath;
  The birds are asleep in the trees:
  Wait, soon like these
  Thou too shalt rest.
-- Wanderer's Nightsong II, Goethe

Re: Some of us like "interdiff" code review

#137

Post by rfoo »

steveklabnik wrote: post #94
strken wrote: post #45

This is interesting. At work we use PRs like the author uses commits, and in fact we squash-and-merge them at the end, but our approach requires rebasing the later PRs whenever we make a change to the earlier PRs. This can be quite laborious, falls afoul of the "don't force-push" rule, takes a long time for engineers to learn, and tends to break existing code review comments in the GitHub interface, but works out oka…

> Perhaps a new source control management tool could have first class support for higher level concepts than just commits and branches, or perhaps that would be even worse to use. You should check out jj, sapling, or mercurial.

If you are stuck with git, try git-branchless. At least it makes rebasing your patch stack easier (git sync && git submit).

Re: Some of us like "interdiff" code review

#138

Post by keybored »

faangguyindia wrote: post #109
Vinnl wrote: post #83

I'm using mostly this workflow with GitHub, with the main disadvantages being that it's more work on my side, and not obvious to my collaborators. But it does carry the same advantages of allowing reviewers to view diffs with just their feedback incorporated, without breaking `git blame` and `git bisect`. When I incorporate a reviewer's feedback, I'll commit that with `git commit --fixup `. I'll then push that up and…

Biggest problem working with Git is remembering all these flags and commands. If you do it everyday for years, sure you remember most of them. But for weekend hackers or those who specialise in some other areas, it creates a lot of frustration. I often forget all these flags etc.... My brother made this https://github.com/zerocorebeta/Option-K This enables me to simply write in thermal, "interactive rebase main, auto…

You’ve only mentioned your bros GitHub projects four times in the last two days. Time to step it up maybe?

Re: Some of us like "interdiff" code review

#139

Post by meling »

torarnv wrote: post #79
mstachowiak wrote: post #54

It's always exciting to see new approaches to code reviews - GitHub has its strengths, but it’s far from perfect. For the scenario you’ve outlined, have you thought about splitting the 3 patches into separate, dependent pull requests? While GitHub doesn’t natively support this, the right code review tool (shameless plug - I’m part of a team building one called GitContext) should allow you to keep pull requests small…

As far as I know, splitting the series into individual PRs only works if you have commit rights to the repository, so you can base one PR on a different branch (in the main repository) than main. As an outside contributor, with a fork of the repository, your three PRs will incrementally contain change A, A+B, and A+B+C, making the review of the last two PRs harder, because you need to review diffs for code you're alr…

Not sure about the fork workflow but otherwise it is possible to change the base branch (manually on GH’s web interface) so that you don’t have to see the original branches and review the changes from A to B and from B to C. Maybe this is not possible with fork workflows?

Re: Some of us like "interdiff" code review

#140

Post by zoogeny »

I've heard people argue for this strategy a few times but I am not convinced. Most projects I work on have feature branches that get squashed into a single commit (erasing the history of the branch). If we even have the case described by the author we would just do the three steps (refactor, new api, update) as 3 commits.

One thing that has been a solid practice for me is to avoid long-lived branches. That seems to be what leads to this kind of multi-stage commit scenario. Someone wants to work for several days (or worse, weeks) on a feature and then drop the whole thing all at once. I strongly prefer to move things into main every day (or multiple times a day). One way to do this is to use feature flags to allow work-in-progress commits to land without causing problems. This also has the advantage (if systems are set up correctly) to enable on dev and staging environments for testing. It is also a hop-and-skip jump away from blue/green rollouts.

I don't want ways to make big commits easier to review. I want to force the team to commit small changes early and often. I understand not everyone agrees.

Post reply on HN