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

191–200 of 224 posts

Re: Some of us like "interdiff" code review

#191

Post by majormajor »

tome wrote: post #70
majormajor wrote: post #13

Earlier quoted context omitted.

Without squashing it's hard for me to commit as small and often as I would like. Some things I want out of the final series of commits: 1) everything builds. If I need to revert something or roll back a commit, the resulting point of the codebase is valid and functional and has all passing tests. 2) features are logically grouped and consistent - kinda similar to the first, but it's not just that I want the build to…

I think there's an ambiguity here between squashing every commit in the PR into a single one, and squashing fixup commits made as responses to review into the commits that originated them. For example, if the original commit series was Do a small refactor before I can start adding the test Add the test for the feature Do a small refactor before I can start adding the feature Work in progress Complete sub-feature 1 Wo…

I think either of these are fine - and the latter is certainly nice but also requires more work - but both require some sort of "squashing."

I don't understand the proposed workflow of "commit early and often" without any sort of squashing of WIP.

Re: Some of us like "interdiff" code review

#192

Post by cormacrelf »

dijksterhuis wrote: post #169
aseipp wrote: post #15

I am the person who wrote this. AMA EDIT: Also, I'm not sure if this is against the rules, but I also need a new job as of recently. I like working on dev tools and other hard problems. If you liked reading this, want me to make your dev team more productive, or just want to experience and enjoy my excellent (and occasionally eclectic ) taste, the email is in my profile.

What do you think of GitLab set up with Merge Request Dependencies + Squash+Merge merge strategy? I remember it being pretty easy to have that series of MRs pattern set up. On merging 3x MRs it would be 3x merge commits w/ a single squash commit for each. Regardless of the MR’s commit history. Tradeoffs are having to merge earlier series branches into later series branches after changes are made during review. But pe…

Maybe it’s better selfhosted, but GitLab is almost unbearably slow. I booted up a Gerrit instance to compare and simply rendering a MR page is maybe 10 seconds vs zero. GitHub is still 10x faster. GitLab manages to be almost that slow for cached pages, making you wait, then realise it’s outdated, and load again, totalling maybe 20 seconds just to “go back to the MR list”. Its awful.

Whatever it is you think you might like about GitLab in theory, it’s much worse when this is your reality. When it takes that long to render a single MR, you do not want to be creating more of them than you have to, and you certainly don’t want to make yourself and the rest of your team navigate between MRs to do code review.

Re: Some of us like "interdiff" code review

#193

Post by zoogeny »

steveklabnik wrote: post #185
zoogeny wrote: post #177

Earlier quoted context omitted.

I'm not sure I understand your exact logic, so let me talk through a simplified scenario for arguments sake. Imagine each change takes 1 day. Imagine the team releases code to production every day. On Monday a developer picks up the task, does the refactor, puts up a PR and the refactor is shipped Tuesday morning. Tuesday he works on the api and it ships Wednesday. Wednesday he finishes the migration which ships on T…

Do you only send PRs with one commit in them every time? I find in my work that if I were doing a three-commit series like this, I would end up sending in a PR on Thursday, containing all three commits. This is because I can’t be sure the refactor works well until I use the results of it with the new code, for example. In the scenario I’m talking about, to adapt it to yours, both developers send out their PR or stack…

Every PR is a single commit. The only extra commits on a PR would be "Review feedback", etc. But it would be squash merge and the final commit message updated to include any important details related to the changes from the feedback.

Consider the case from the article. refactor: 25, api: 500, migration: 50. Lets say 25 = 1 day of work so we have refactor: 1 day, api: 20 days, migration: 2 days. I'll decrease api to 5 days (1 week) for simplification. You pick up the task on Monday and do the refactor. Now, you have 5 days of work ahead of you on the api. Don't you feel you are looking over your back hoping no one on your team touches those lines? And then on day 5 someone does the refactor but slightly different. You have a commit so you have to do some rebase magic when you catch up to master. No thank you. And you doubled the work, your coworker wouldn't have had to do it if you had checked it in 5 days ago.

Consider a deadline. Consider a major production bug in the api that you discover when you release it. In my plan, you end up releasing the api early so you catch the bug early. You can petition for more resources to fix the problem since you have extra days which were reserved for the migrarion. If you try a big-bang release you find the api bug later, decreasing the likelihood that you can fix it within the deadline. I think of amortizing the risk of my releases across time rather than allowing the risk to accrue.

I could continue to defend this with more examples but the real fact is there is a tradeoff here like tabs vs. spaces or emacs vs. vim. But that tradeoff is ship fast vs. reduce interruptions for developers. Because creating a PR, reviewing a PR and releasing a change are interruptions for developers. And some developers will find ways to reduce interruptions. I have my preference for shipping fast for the reasons above and my experiences between cultures that result from the choice of prioritization in the tradeoff.

Re: Some of us like "interdiff" code review

#194

Post by cryptonector »

crabbone wrote: post #168
cryptonector wrote: post #62

> Interlude: Can you please just tell me if git rebase is evil or not so that we can derail the entire discussion over it? Ha, that's funny. But yes, please we need interdiffs in GitHub and GitLab. I want to have my PRs/MRs always rebased and I don't want "fix code review comments" commits.

Why would you want that in GitHub or Gitlab? :| The Web interface is atrocious no matter what you are using it for. Just the fact that you need to edit text in a dysfunctional Web editor negates all the benefits of any workflow you can imagine... The most natural way to deal with PRs is to use your editor you use to write your code. Here you have all the tools necessary to navigate the code, to look up the history of…

> Why would you want that in GitHub or Gitlab? :| The Web interface is atrocious

Yes, but I have to use it. I'm part of a captive audience.

> The most natural way to deal with PRs is to use your editor you use to write your code. Here you have all the tools necessary to navigate the code, to look up the history of the change, to run tests and so on.

Absolutely, and I do just this a lot of the time (though admittedly not all the time). The problem is that the review comments have to be left in the web UI. I would be happy to use e-mail, but e-mail is not what my colleagues want, and it's not really necessarily the right tool (e.g., if e-mail and GH/GL have different retention policies at some $WORK, or if the rules at some $WORK demand use of GH/GL for code review comments.

Re: Some of us like "interdiff" code review

#195

Post by ants_everywhere »

jrochkind1 wrote: post #118
mizzao wrote: post #97

Earlier quoted context omitted.

I was going to say... interactive rebase addresses a lot of the "diff soup" comments that the writer complains about. It's really only done by disciplined engineering teams though (who bother to learn some more advanced features of git)

I realized reading the first part of this article that I often want to set up a sequence of PR's, for very similar reasons as in the begininng of OP. Say, a prefatory refactor, then the main work, then some data cleanup. When I do this, the problem with rebase is that it kind of breaks the additional "next in sequence" PRs "on top", or at least requires (confusing to me) cleanup in all of them when I rebase the base.

Gerrit supports this workflow. Patchsets can form a "relation chain."

One common workflow is you have a change of, say, 3-4 patches. You get code reviewed on them all, updating each patch set according to the reviews. Later patch sets can't be submitted until the earlier ones are. You can have tooling telling people not to review the later patch sets in the chain until the earlier ones have passed code review.

Re: Some of us like "interdiff" code review

#196

Post by mattpallissard »

mizzao wrote: post #97
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…

I was going to say... interactive rebase addresses a lot of the "diff soup" comments that the writer complains about. It's really only done by disciplined engineering teams though (who bother to learn some more advanced features of git)

Oh, I don't know, I hardly consider myself super disciplined. It's just a matter of memorizing a command or two.

    git commit --fixup=commit HASH
    git rebase -i HASH^ #yes, include ^
    # save and close the rebase window that appears
    git push origin branch --force
edit, and you can re-word a commit with

    git commit --fixup=amend:HASH

Re: Some of us like "interdiff" code review

#197

Post by dmurray »

shambulatron wrote: post #152
jrochkind1 wrote: post #118

Earlier quoted context omitted.

I realized reading the first part of this article that I often want to set up a sequence of PR's, for very similar reasons as in the begininng of OP. Say, a prefatory refactor, then the main work, then some data cleanup. When I do this, the problem with rebase is that it kind of breaks the additional "next in sequence" PRs "on top", or at least requires (confusing to me) cleanup in all of them when I rebase the base.

Have you come across `git rebase --update-refs`? This automatically moves your "intermediate" branches during a rebase and sounds like it could be useful in your situation.

There's also `git rebase --onto`, which effectively does the reverse - you tell git you've already rebased the part of this branch that overlapped with the "intermediate" branch, and it just needs to take care of the rest.

Re: Some of us like "interdiff" code review

#198

Post by tvaughan »

mattpallissard wrote: post #196
mizzao wrote: post #97

Earlier quoted context omitted.

I was going to say... interactive rebase addresses a lot of the "diff soup" comments that the writer complains about. It's really only done by disciplined engineering teams though (who bother to learn some more advanced features of git)

Oh, I don't know, I hardly consider myself super disciplined. It's just a matter of memorizing a command or two. git commit --fixup=commit HASH git rebase -i HASH^ #yes, include ^ # save and close the rebase window that appears git push origin branch --force edit, and you can re-word a commit with git commit --fixup=amend:HASH

I think the "hard part" for people is keeping commits focused on just one change. Commits should be crafted to help reviewers, but what I see mostly is a bunch of random commits that look more like a backup of each days progress.

Re: Some of us like "interdiff" code review

#199

Post by User23 »

mizzao wrote: post #97
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…

I was going to say... interactive rebase addresses a lot of the "diff soup" comments that the writer complains about. It's really only done by disciplined engineering teams though (who bother to learn some more advanced features of git)

Or just use a good tool, like Magit.

Re: Some of us like "interdiff" code review

#200

Post by sgerenser »

another-acct wrote: post #180
clktmr wrote: post #124

I agree in general, but running git bisect on individual PR commits is just doing it wrong. There will always be commits that break stuff temporarily. Run git bisect only on the merge commits instead, which are typically already tested by CI.

> I agree in general, but running git bisect on individual PR commits is just doing it wrong. There will always be commits that break stuff temporarily. That's unacceptable in my book. Before submitting any patch set for review, the contributor is responsible for ensuring that the series builds (compiles) at every stage -- at every patch boundary. Specifically so that a later git bisect never fail to build any patch…

I don’t know what world you live in, but I’ve never worked in an organization where more than 1% of the developers would go through all that extra work for every PR.
Post reply on HN