In Praise of Stacked PRs

It sounds like you're talking about a bunch of different cases, and I'm having trouble untangling them.

If there's a simple refactoring everybody agrees is good whether or not your overall goal ends up making sense, then yes, by all means merge that. But that doesn't require stacking unless your review process is slow. In which case I still think the right solution is to speed up review, not to stack.

For the cases where we don't know the full story, my first thought is that we never know the full story. So there I try to instead find the smallest unit of work that everybody agrees is a step forward.

When that's not possible, where the unit of work still seems pretty large, instead of breaking that up into a bunch of stacked diffs that shouldn't be merged until we really understand something (which to me sounds like a large PR in disguise), I think a better option is a spike, where we intentionally do a quick, throwaway version of some change as a way of learning about the change. Instead of trying to do good code along the way to good understanding, we just go for the understanding. Once we have thrown out that scratch code, we then go back with our new knowledge for a proper PR.

So I'm still not seeing where I would use stacked diffs, except in this case here: https://news.ycombinator.com/item?id=32215346

The premise of stacked diffs seems that we won't learn anything significant from reviewing or deploying code. (If we did learn something valuable, then the things stacked on top could be up for a lot of rework or might be thrown out altogether.) I think that has a lot of bad effects, but one of the biggest for me is that the bigger the inventory of code (whether in one big PR or a stacked set of smaller ones), the more a reviewer will feel obliged to say, "LGTM" and let it go, because they know there's not much point in saying, "Actually, I think this whole thing could be better approached by X."

So like you I'm entirely for small, reviewable lumps that are easily merged. I just think they should be then reviewed quickly, so that stacking or agglomerating is unnecessary.

> One of the things I learned from the Lean folks was to look for inventory; it's one of the 7 Wastes. [1] In physical manufacturing, it's pretty obvious, because it's physical stuff sitting around on the journey to becoming actually useful.

I guess this is not true anymore post covid outbreak? Pretty sure a lot of companies would kill to have inventory of their raw materials right now...

I use auto-formatters so that I don't have to deal with coworkers telling me to add a new line or that I put too many/few levels of indentation.

Formatters do nothing for code quality. Linters can help quality but it's more of a last line of defense for common errors, not a signal that your code is 'good'.

That is, in fact, my whole point. There are multiple ways to use your tools--don't assume that people who prefer to use things a different way are ignorant. We completely agree. My challenge is to the frequency at which there is actual need to limit the allowed workflows as a result of different preferences. In other words, let there be a diversity of workflows and deal with it.

More often than not I've seen such limits used to "reign in" "misbehaving" engineers or because, as I've suggested, people sometimes don't know how to use a tool to its full capacity. Sometimes it's purely people wanting to force their preference on others. I've had to walk senior engineers through (or just manually clean up when they clearly have no idea how git works) branches that people royally mess up because they merged something in 8 different ways because some other person told them rewriting history is evil or some previous organization taught them to only push merge commits to PRs or protected all branches or something...

There can both be organizations that need to enforce policy to make their business successful and people that don't really understand the full gamut of what's possible with a tool and restrict workflows to stuff they understand. I don't think I equivocated the two. I simply expressed frustration from all the times I've anecdotally encountered people to aren't fluent with git making decisions for others who are fluent with git on how git should be applied.

> As far as I know, Phabricator does not let you view or comment on individual commits in a code review (which I use at work). Let me know if you know differently and I might switch to doing that.

Isn't there a "commits" tab when viewing a revision? Also Phabricator is no longer maintained for the last year.

> Can you advise me on running CI on each commit in the PR in GitHub? As far as I can see, it's technically possible in that you can run arbitrary code as part of your CI, but there's no convenient way to do it. (In particular, I would want to be able to view the CI runs for each individual commit, i.e. render the little checkmark/x next to each commit in GitHub, which seems like it would require a lot of integration via the API.)

You'd use the https://docs.github.com/en/rest/commits/statuses API to publish the little checkmark. More info: https://docs.github.com/en/pull-requests/collaborating-with-....

However, before you do that, consider: this one I think is a slight impedance mismatch between how users conceptually think about using GH and how GH actions are designed to be triggered. The most simple thing to do if you want every commit checked and tested is just make a GH action that runs your checks on push, which gets run on every commit and updates the status. If you already have a push action, just remove the "main" branch restriction. The PR action is designed to do PR-scoped things. You can, if you really only want to check commits once they are opened for PR, iterate over the commit list and manually trigger workflows to test and check each one (you can even just manually trigger your existing push action if you add the manual trigger option and specify the commit sha yourself and it will run, I think). But, is the list of commits that get pushed to the repo and the list of commits you want to eventually test for inclusion into main really that different, at the end of the day?