3 ms·

Thanks for mentioning this. It seemed odd to me, too, so I spent some time trying to work it out. As a reviewer, I'm not sure how I'm supposed to assess databas

Thanks for mentioning this. It seemed odd to me, too, so I spent some time trying to work it out. As a reviewer, I'm not sure how I'm supposed to assess database or API changes without knowing how they're intended to be used. And deploying them independently seems odd, too, especially if you need to roll it all back. I think in my ideal world there would be a clean history and I could review a PR commit-by-commit. But, you can't just approve a single commit, so there's a tooling problem there. And most CI runs on an entire push rather than individual commits. And increasingly I see devs using git as an offsite backup for whatever change they just made, rather than breaking commits up into logical chunks. In that workflow, squashed merges make the most sense. It's probably flawed, but the mental model I came up with is each stacked PR collapses into what would have been an individual commit in a clean PR, with the advantage of being able to be reviewed separately from the other changes and forced to clear CI. And then the whole stack becomes what would have been a clean PR in the old model. That, I can kinda see the benefit of. But, merging only part of the stack into trunk is the mental hurdle I can't clear; it'd be like merging only some commits from a PR. It kinda reminds me of when projects used CVS. I've really only seen stacked PRs used on projects where history is little more than an audit log. I'm keen to see how this gets employed by open source projects. I think there's a disconnect and it's likely I'm not going to really get it until I see it.

ollysb 2mo ago
This is my confusion, we already have commits to bundle changes, why not simply allow commits to be reviewed independently within a pr?
- rendaw 2mo ago
  Doesn't this have the same issue? If you need database changes that also need query changes or api changes, then you need to modify multiple commits.
- flexagoon 2mo ago
  This is similar to how things like Gerrit do code review, and it's pretty nice
- eddythompson80 2mo ago
  Ok, then how do you approve/take the first 2 commits and not the last one? Or how do you insert a commit in between 2 commits or address feedback on a given commit? The person making the change is now going to have to run multiple confusing interactive rebases and git shenanigans, the rewrite the history of the PR branch on every feedback, then you have to re-review all the commits again because they are all different. It’s possible of course to push all that complexity on the tooling. Have GitHub and git provide tooling for doing all that within the context of a single branch/PR. But why is that better? Multiple branches are easier to manage in git, and as long as they don’t conflict on the merge. Obviously if a feedback on PR#1 causes a conflict in PR#2 which causes a conflict in PR#3 it’s still tedious, but it’s a lot more doable than managing interactive rebases on every feedback comment.
  - Kinrany 2mo ago
    Jujutsu makes all this easy
    - Degorath 2mo ago
      Jujutsu makes the author-side of it all fairly nice, indeed. But the reviewer-side has been so horribly abandoned by github that it has been frustrating me for a long while now.
      - eddythompson80 2mo ago
        A JujutsuHub.com business opportunity presents itself. How many tokens do you have?
        Degorath 2mo ago
        Already working on that!
defmacr0 2mo ago
Yeah, clean, neatly seperated and logically independent PRs are very nice for reviewers, but usually it requires one to complete the whole feature and then go back and think about the best way to seperate it into a series of smaller changes again. It works for projects like linux where there is tons of motivated manpower such that requiring authors put in a day of additional effort to make a change as presentable as possible is acceptable.
- nirvdrum 2mo ago
  I usually just amend commits as I go and don’t find it all that onerous. Sometimes it gets tricky, particularly if a rebase effectively changes what the code would have looked like. But, with LLMs even that’s gotten much easier.
necovek 2mo ago
Regarding reviewing commit-by-commit, I actually do believe a branch is a unit of work to be reviewed, as long as it is fully standalone and implements a usecase end-to-end (no matter how small, and small it should be). Commits on a branch are a tool for a developer, and they will go back and forth a bit as they learn more about it, perhaps explore a path, and then go back on it. With bzr (Bazaar, since abandoned by Canonical, but maintainers forked it as Breezy), you had a nested history: top-line merges look like squashed merges in git, but you simply do a bzr log -n1 and get to see the next level of commits in each merged branch and you can understand the build process and explore what other things original author tried out which did not work. It is simply a way to get the best of both worlds IMO (it was noticeably slower than git, though). I did find it hard to get the product and design to adopt a similar mindset of developing a feature iteratively, so it was usually the developer who'd come up with in-between designs and UX flows while they converge to the final design over multiple small branches.

ollysb 2mo ago

This is my confusion, we already have commits to bundle changes, why not simply allow commits to be reviewed independently within a pr?

rendaw 2mo ago
Doesn't this have the same issue? If you need database changes that also need query changes or api changes, then you need to modify multiple commits.
flexagoon 2mo ago
This is similar to how things like Gerrit do code review, and it's pretty nice
eddythompson80 2mo ago
Ok, then how do you approve/take the first 2 commits and not the last one? Or how do you insert a commit in between 2 commits or address feedback on a given commit? The person making the change is now going to have to run multiple confusing interactive rebases and git shenanigans, the rewrite the history of the PR branch on every feedback, then you have to re-review all the commits again because they are all different. It’s possible of course to push all that complexity on the tooling. Have GitHub and git provide tooling for doing all that within the context of a single branch/PR. But why is that better? Multiple branches are easier to manage in git, and as long as they don’t conflict on the merge. Obviously if a feedback on PR#1 causes a conflict in PR#2 which causes a conflict in PR#3 it’s still tedious, but it’s a lot more doable than managing interactive rebases on every feedback comment.
- Kinrany 2mo ago
  Jujutsu makes all this easy
  - Degorath 2mo ago
    Jujutsu makes the author-side of it all fairly nice, indeed. But the reviewer-side has been so horribly abandoned by github that it has been frustrating me for a long while now.
    - eddythompson80 2mo ago
      A JujutsuHub.com business opportunity presents itself. How many tokens do you have?
      - Degorath 2mo ago
        Already working on that!

defmacr0 2mo ago

Yeah, clean, neatly seperated and logically independent PRs are very nice for reviewers, but usually it requires one to complete the whole feature and then go back and think about the best way to seperate it into a series of smaller changes again. It works for projects like linux where there is tons of motivated manpower such that requiring authors put in a day of additional effort to make a change as presentable as possible is acceptable.

nirvdrum 2mo ago
I usually just amend commits as I go and don’t find it all that onerous. Sometimes it gets tricky, particularly if a rebase effectively changes what the code would have looked like. But, with LLMs even that’s gotten much easier.

necovek 2mo ago

Regarding reviewing commit-by-commit, I actually do believe a branch is a unit of work to be reviewed, as long as it is fully standalone and implements a usecase end-to-end (no matter how small, and small it should be). Commits on a branch are a tool for a developer, and they will go back and forth a bit as they learn more about it, perhaps explore a path, and then go back on it. With bzr (Bazaar, since abandoned by Canonical, but maintainers forked it as Breezy), you had a nested history: top-line merges look like squashed merges in git, but you simply do a bzr log -n1 and get to see the next level of commits in each merged branch and you can understand the build process and explore what other things original author tried out which did not work. It is simply a way to get the best of both worlds IMO (it was noticeably slower than git, though). I did find it hard to get the product and design to adopt a similar mindset of developing a feature iteratively, so it was usually the developer who'd come up with in-between designs and UX flows while they converge to the final design over multiple small branches.