Phorge: Going Public

Individual, atomic ideas are expressed as a single diff which maps to a single commit. Representing one idea as multiple commits is discouraged, though Phabricator does keep its own history of how a patch evolves should you ever need to refer to it.

There is not one exact correct philosophy about how small the atomic unit is or what constitutes too much work, but the general wisdom is something to the order of "do one thing."

I believe a PR should "do one thing," so PRs with multiple commits should be safe to flatten, else they likely contain too much. Of all of the things Google is rightly criticized for, their methodologies towards code review are pretty good and this is more or less what it felt like working in pseudo-Perforce. What if you needed multiple changes? Well, it was certainly more cumbersome than needed, but serieses work great here. Not only that, but patch serieses are already well established in the software engineering canon! Plenty of projects have accepted patch serieses over mailing lists for decades.

I know many people have seen Github-style workflows work pretty well and so are unlikely to be receptive to the idea that some of the fundamentals could be wrong, but my advice is please consider it. It's not like there's one exactly correct way to handle merging changes, but I've found that the process of focusing on atomic sets of changes individually has improved my discipline as a software engineer. It may be a bit too wanky to call it "enlightening," but it's pretty close.

1) At issue here is not (just) the portion of the history in the changes under review, but the choice of where that history starts. That's what the commenters above mean by "diff based" and "branch based". If I start a Git branch on HEAD@{yesterday}, and HEAD@{today} renames a function that I'm using, then there is a semantic difference between approving solely my changes and approving my branch with information about its branch point. And in a workflow where only my changes are under review without further history, it's entirely possible for me to submit a set of changes that simply don't sensibly apply to a specific target branch, because the tooling did not reflect that my source branch is different.

Note that all of this true even if "my changes" takes the form of a series of patches, such as a [PATCH m/n] series posted to a mailing list using an LKML-style workflow!

2) All that said, I do not know of any tool that makes it particularly easy to manage both of these pieces of history in a review. All the Git-based review tools I've seen (GitHub, GitLab, Gitea, etc.) make it fairly easy to see where the remainder of history is, view the final state of a modified file with history (e.g., it's easy to see the blame view of a modified file), etc. But they don't default to showing you the commit-by-commit history; they show you the diff between the beginning and end for review. Meanwhile, the [PATCH m/n] workflow (e.g., git format-patch, including Sourcehut's tooling on top) is in fact much closer to the Phabricator one in spirit; while it does permit you to see the commit-by-commit history, it has no idea what the history before the first patch is, and it's difficult to even tell if a patch got merged!

So in a practical sense, there isn't really a tool that lets you work with full history. You have to make some form of compromise. In practice, I think the most common compromise is to constrain yourself to making one commit per code review; if the history is complex enough to really want commit-by-commit review, you're probably best off making multiple reviews anyway.

(You might object - what if the code doesn't build or work right between your first and second commits? But that's already a bad place to be, because ordinary git bisect will have false build failures if it lands between those two points. There is git bisect --first-parent, but first, your project had better be doing --no-ff merges to make this useful, and second, the effect of it is to ignore the detailed history, so if you're going to ask people to do that, why burden them with the detailed history in the first place?)

The other compromise here, weirdly enough, is Phabricator itself. Because Phabricator reviews diffs and not branches, you can submit two independent code reviews for two dependent commits, and Phabricator won't care. If you try to do this with any of the pull-request-style tools, the second code review will include both changes because they know history; Phabricator doesn't, and it will show just the second change if all you sent it is "arc diff HEAD^". And then you can "arc land" those on your own. (But you have to be careful about getting the order right, because, again, Phabricator doesn't know how to do that for you.)