3 ms·

Looking at the way GitHub are selling this “feature”, I feel like some of the engineers who are going to be excited about this feature for “reviewability” reaso

Looking at the way GitHub are selling this “feature”, I feel like some of the engineers who are going to be excited about this feature for “reviewability” reasons are, in particular, those who’ve forgotten that they should be splitting changes into multiple logical commits inside a PR. And instead of that they’re now going to use multiple, single commit branches and stack them because stacked PRs are a “new” “feature”. And the upshot for the LLM providers is that they get to charge for n reviews, instead of one.

eddythompson80 2mo ago
I think plenty of people explained the difference between stacked PRs and individual commits in 1 PR. You’re not wrong, but it’s just a matter of path of least resistance. There is no way to leave a comment on a particular commit in a PR. Also all commits addressing PR feedback get tucked at the bottom of the commit list. Unless you do some crazy git gymnastics and rewrite the PR history and confuse everyone. With stacked PRs you also (as a maintainer or a reviewer) have the option to merge some and not others. Like here are 3 stacked PRs, one for provisioning some AWS or Azure resources that I’ll need, one for implementing the APIs using those resources, and one for updating the UI to use the new APIs. You can then say “let’s get the 2 backend PRs in and hold off on the UI as we’re changing that entire view”. You can’t do that with multiple commits in a PR without asking the person to redo the PR, then you’re back to git-foo. Yes, GitHub could have made the UI allow a “per commit” comments somehow, then allow you to select the set of commits to include in the merge somehow, then write a blog post on how to manage “Address PR comments #1” commits. But the stacked PRs solve all that. Not to mention how people treat commits as their own internal save states. I always enable “squash and merge” option because I think it makes a lot more sense to have 1 commit on main per PR where all the context of the change is either in the commit message or the linked PR. Also LLM providers charge per token. Charging per “work unit” is still not a solved problem. You can’t charge per “review” when your cost is per token. Just like airlines can’t charge “per ticket”, they have to charge differently depending on the destination. Unless you invent some bs arbitrage to lure users and eventually bait and switch on them.
IshKebab 2mo ago
Splitting changes into multiple commits is a much worse experience if you actually have self-contained dependent changes. 1. The whole review interface isn't set up for reviewing individual commits. 2. You can't merge changes progressively. 3. CI doesn't run on each commit. 4. If you have linear history (good idea IMO) you'll lose your nice commit history when you merge it. This is much better.
- beaker52 2mo ago
  You can read each individual commit and review the PR as a whole. This is the way many people design and review PRs. You don’t need to merge those changes progressively. If you do, you go through exactly the same process of creating a separate branch and PR. The only difference is that GH has now added some UI and automation for rebasing and merging the PRs. In the past we would have explained the chaining in the PR and rebased manually. You don’t need CI to run on each commit. You only lose your commit history if you squash merge, many people don’t, and you don’t have to either. The arguments come from angle that doesn’t appear to be aware that stacked PRs were a thing before GH made these UX improvements.
  - IshKebab 2mo ago
    Look at some level you want a "thing that is reviewed and has CI run on it" right? Unless you work alone you need that. Let us call that unit of work, a flob. When you have written and submitted a flob for review, you often want to continue your work on top of that, and then you may end up with a second dependent flob that is finished before the first flob is merged. You want both to be reviewed. You want CI to run on both. It's simply a much better experience if flobs are PRs rather than commits. I dunno how else to put it. > stacked PRs were a thing before GH made these UX improvements Not in a way that worked properly. You could sort of do it for PRs within a fork, but it was impossible across forks which is the way most open source GitHub PRs are done.
    - beaker52 2mo ago
      > Look at some level you want a "thing that is reviewed and has CI run on it" right? Yeah, it’s called a branch, or PR. It’s a set of changes you want to sign off. It seems like you want CI run on every commit, which seems rather unnecessary. And if you don’t, well, that’s always been the case. > Not in a way that worked properly. GitHub operates git. No git changes have happened. It’s just commits and branches, in git. So anything that worked before, works exactly the same now, but with buttons taking out some of the small amount of effort you had to put in.

eddythompson80 2mo ago

I think plenty of people explained the difference between stacked PRs and individual commits in 1 PR. You’re not wrong, but it’s just a matter of path of least resistance. There is no way to leave a comment on a particular commit in a PR. Also all commits addressing PR feedback get tucked at the bottom of the commit list. Unless you do some crazy git gymnastics and rewrite the PR history and confuse everyone. With stacked PRs you also (as a maintainer or a reviewer) have the option to merge some and not others. Like here are 3 stacked PRs, one for provisioning some AWS or Azure resources that I’ll need, one for implementing the APIs using those resources, and one for updating the UI to use the new APIs. You can then say “let’s get the 2 backend PRs in and hold off on the UI as we’re changing that entire view”. You can’t do that with multiple commits in a PR without asking the person to redo the PR, then you’re back to git-foo. Yes, GitHub could have made the UI allow a “per commit” comments somehow, then allow you to select the set of commits to include in the merge somehow, then write a blog post on how to manage “Address PR comments #1” commits. But the stacked PRs solve all that. Not to mention how people treat commits as their own internal save states. I always enable “squash and merge” option because I think it makes a lot more sense to have 1 commit on main per PR where all the context of the change is either in the commit message or the linked PR. Also LLM providers charge per token. Charging per “work unit” is still not a solved problem. You can’t charge per “review” when your cost is per token. Just like airlines can’t charge “per ticket”, they have to charge differently depending on the destination. Unless you invent some bs arbitrage to lure users and eventually bait and switch on them.

IshKebab 2mo ago

Splitting changes into multiple commits is a much worse experience if you actually have self-contained dependent changes. 1. The whole review interface isn't set up for reviewing individual commits. 2. You can't merge changes progressively. 3. CI doesn't run on each commit. 4. If you have linear history (good idea IMO) you'll lose your nice commit history when you merge it. This is much better.

beaker52 2mo ago
You can read each individual commit and review the PR as a whole. This is the way many people design and review PRs. You don’t need to merge those changes progressively. If you do, you go through exactly the same process of creating a separate branch and PR. The only difference is that GH has now added some UI and automation for rebasing and merging the PRs. In the past we would have explained the chaining in the PR and rebased manually. You don’t need CI to run on each commit. You only lose your commit history if you squash merge, many people don’t, and you don’t have to either. The arguments come from angle that doesn’t appear to be aware that stacked PRs were a thing before GH made these UX improvements.
- IshKebab 2mo ago
  Look at some level you want a "thing that is reviewed and has CI run on it" right? Unless you work alone you need that. Let us call that unit of work, a flob. When you have written and submitted a flob for review, you often want to continue your work on top of that, and then you may end up with a second dependent flob that is finished before the first flob is merged. You want both to be reviewed. You want CI to run on both. It's simply a much better experience if flobs are PRs rather than commits. I dunno how else to put it. > stacked PRs were a thing before GH made these UX improvements Not in a way that worked properly. You could sort of do it for PRs within a fork, but it was impossible across forks which is the way most open source GitHub PRs are done.
  - beaker52 2mo ago
    > Look at some level you want a "thing that is reviewed and has CI run on it" right? Yeah, it’s called a branch, or PR. It’s a set of changes you want to sign off. It seems like you want CI run on every commit, which seems rather unnecessary. And if you don’t, well, that’s always been the case. > Not in a way that worked properly. GitHub operates git. No git changes have happened. It’s just commits and branches, in git. So anything that worked before, works exactly the same now, but with buttons taking out some of the small amount of effort you had to put in.