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

181–190 of 224 posts

Re: Some of us like "interdiff" code review

#181

Post by dgfitz »

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.

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

I would dislike this as a reviewer. Now I need to keep your stack of changes in my head, in order.

Just make the change and push it, if people can’t review the diff you have different problems.

Re: Some of us like "interdiff" code review

#183

Post by another-acct »

keybored wrote: post #126

As the author is aware of[1][2] the Git project uses this interdiff approach with email. - Patch series (PR equivalent) go through a round of reviews - Each version has a cover letter (like PR desription) unless it’s only a one-patch series - Each version has that optional cover letter with each patch as a reply email to it - The next version is a reply to the previous cover letter - And each version 2 and above cove…

This is the comment I've been looking for! :) High-five!

Some remarks:

> - The next version is a reply to the previous cover letter

Not necessarily; sometimes the new version is not posted in-reply-to anything, but the cover letter includes a reference (usually message-id + web-archive URL) in the body. Depends on the project I guess.

> And each version 2 and above cover letter has a git-range-diff in it (courtesy of git-format-patch)

Wow, amazing. I'd been doing it manually; it's amazing that git can do this automatically now!

> Or use Git Notes and let it handle it for you

Finally someone knows about it :)

> - You ought to keep track of the base commit between versions (you could have rebased on the main branch) > - You need to store versions of your branches

The latter solves the former. Also, patch sets are best formatted with "--base"; then people on the list know exactly what to apply the patches on top of.

I go farther: assuming you also push the patch set to some public URL (for easy fetching by the reviewers), those URLs then count as read-only and permanent, forever. Never delete them, never rebase them.

> - You need to keep track of who to CC on the emails

This is best solved by adding Cc: tags to the individual commit messages. Three reasons: (a) the final git log will capture who was CC'd, (b) these Cc: tags (being parts of the commit messages) survive rebases, (c) you can CC different patches to different people (the cover letter should be CC'd to everyone); which is handy when you modify multiple subsystems in the same series.

> You need to harvest the email message id on the cover letters and use that `In-Reply-To`

git-format-patch can prepare a skeleton for the cover letter too, and once edited manually, git-send-email can send it out together with the actual patches -- and handle the in-reply-to automatically.

Your point does stand if you send v2 "in-reply-to" v1. Or, if you send multiple patch sets (e.g. for multiple inter-operating projects) in-reply-to a common meta-cover-letter.

> [3] Git Notes `amlog` records the message id of the patch email where the commit came from

Never heard of "amlog"; however, you can pass "--message-id" to git-am, and then the message-ID of the patch email becomes part of the commit message. That's incredibly useful with mailing list archives that let you search for discussions by message-id.

Re: Some of us like "interdiff" code review

#184

Post by another-acct »

kspacewalk2 wrote: post #132
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. (Tha…

Agreed 100%, and it's hard to believe how many corporate software developers actively resist this concept.

Once the code works, many refuse to invest any time into properly restructuring it for presentation (review) and for posterity (git commit log).

Re: Some of us like "interdiff" code review

#185

Post by steveklabnik »

zoogeny wrote: post #177
steveklabnik wrote: post #153

Earlier quoted context omitted.

The people I know who prefer stacked diffs argue that it makes it easier to integrate changes faster, no matter the size. Part of this is because unlike a PR on GitHub, you can land parts of a stack: to use the example from the article, if the "small refactor" diff is good to go, it can be landed without landing the "new api" and "migrate API users" diffs. Centering commits rather than branches has the effect of maki…

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 on Thursday. Later that day, the refactor commit is approved, but the new API is still under discussion. That discussion takes a full day to get signed off. On Friday, the full PR/stack has been reviewed and all changes are ready to land.

In this scenario, the refactor lands from the stack on Thursday, and the rest lands Friday. In the PR world, both end up landing Friday, because the refactor, as part of the PR, doesn’t land until the whole PR does.

Re: Some of us like "interdiff" code review

#186

Post by another-acct »

zeotroph wrote: post #77
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…

> Developers just aren't very good at doing this. GitHub provided a way to contribute, but also to avoid learning to rebase, thus making it more welcoming to devs who only know about commit and pull - that is what made it so popular. The squash then rebase or merge step is done on server side. Plus it has a very "harmless" UI, but that hides a lot of details (patchsets) and the layout wastes so much space imo. This a…

IMO you are spot on. GitHub's worst sin is that it has mis-educated new generations of developers. My 16yo son uses github every day; I've needed to explain fetch + rebase to him several times. It just doesn't seem to stick; it seems foreign to the entire community he's collaborating with.

Re: Some of us like "interdiff" code review

#187

Post by another-acct »

aseipp wrote: post #102
planetpluta wrote: post #99

I’m confused by how pushing to new branches would work on GitHub (or is the point that it doesn’t…)? Are you able to change the branch of a PR from `v1` to `v2` without making a new PR?

Yes, the point is that it basically doesn't support that. Well, OK. You can push two branches, v1 and v2, each with the commits. Then to do pairwise diffs, you type in the commit object hashes directly into the URL bar to diff the two objects in the repository using the 'blobs' API but like... I don't think that qualifies so much as "supporting" it as much as an absurd hack, right? > Are you able to change the branch…

Whenever you force push v2, v3, v4 of your branch called "foobar", you can also push branches called "foobar-v2", "foobar-v3", "foobar-v4" (pointing to identical commit hashes, respectively). The "foobar" branch is what refreshes the PR. There are no PRs for the versioned (and effectively read-only) branches, they are there for reviewer reference.

Re: Some of us like "interdiff" code review

#188

Post by another-acct »

jeffbee wrote: post #14
ajkjk wrote: post #10

Earlier quoted context omitted.

Better it be a deep dive into developer sanity, a more important but comparatively under-valued metric.

For me, GitHub PR review drives me crazy. It's good for exactly one round of exchange. After that nobody can tell what the heck is going on. So my self-reported mental health would be worse. But on non-subjective metrics it seems like LLVM PRs on GitHub are gathering noticeably less discussion than they used to enjoy as Phabricator diffs.

> For me, GitHub PR review drives me crazy. It's good for exactly one round of exchange. After that nobody can tell what the heck is going on.

Matches my experience totally. It devolves into a heap of garbage. In comparison, with (incremental) mailing list-based review, it's not difficult to go up to v7 or so.

> But on non-subjective metrics it seems like LLVM PRs on GitHub are gathering noticeably less discussion than they used to enjoy as Phabricator diffs.

That could be a consequence of GitHub making it harder to comment sensibly.

Re: Some of us like "interdiff" code review

#189

Post by another-acct »

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.

> or just want to experience and enjoy my excellent (and occasionally eclectic) taste

Thanks for the good laugh :)

Seriously though, your CV is impressive. I hope you'll land well, and quickly. In my (very recent) job hunt experience, the job market is currently mortally ill; the more senior and experienced you are, the more the insane interviewing and HR practices, and the inexplicable rejections, will hurt your soul.

A friend of mine sent me the following links:

https://danluu.com/hiring-lemons/

https://danluu.com/programmer-moneyball/

https://danluu.com/algorithms-interviews/

Good luck!

Re: Some of us like "interdiff" code review

#190

Post by majormajor »

gloryjulio wrote: post #17
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…

When we publish a stack of commits, our ci ensures that every commit is build and tested individually. There is no consistency issue Squash and merge actually makes the above goal harder. With rebase + small commits, all we need to make sure is that every commit pass all the build signals and tests during ci

What is "publishing a stack of commits"?

Is that putting it up for review? Or are you not doing a PR workflow at all, in which case this doesn't really relate to the article.

Is the expectation that the developer either never commits stuff in a broken state during development or that they go back and rewrite or squash the sequence before pushing it for review?

Post reply on HN