6 ms·

Maybe I am being to much of a purist, but retroactively modifying commits and history? Why? Stuff happens, so do mistakes. Fix the mistakes, make another commit

Maybe I am being to much of a purist, but retroactively modifying commits and history? Why? Stuff happens, so do mistakes. Fix the mistakes, make another commit, and go on with your life.

rom1v 2y ago
https://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html https://www.mail-archive.com/dri-devel@lists.sourceforge.net...
gorset 2y ago
The use case is when you look at a branch as a series of patches. Reviewing a clean set of commits is much easier than a branch full of mistakes and wrong paths taken. Useful when we optimize for reviewing and good history for future maintenance. This has been important and useful when I’ve worked on big mission critical backend system, but I also understand it might not be the most important factor for a new project’s success.
- dclowd9901 2y ago
  So this wouldn’t work very well in workflows that flatten merges to a trunk?
  - travisjungroth 2y ago
    It would mostly be unnecessary. The separate commits won’t matter after the PR if they’re getting squashed. Debatably, if you’re making changes during a PR review, it could be helpful to make those changes in relevant commits. That way if someone goes through them during the PR, they get one clean “story”, rather than see the pre-PR commits and the conversation after.
  - mst 2y ago
    I like for non-trivial stuff to have a branch with a series of logical commits, cleanly rebased atop main, then use -no-ff to force a merge commit anyway. That way you can the whole branch appears as a single commit/diff atop main in primary history but you can dig in to the original components of it if/when that's useful. The primary obstacle to doing this for me is, if I'm honest, not having automated it sufficiently that I can't forget to do that.
  - chippiewill 2y ago
    Tools where they allow you to squash merge tend to be fairly incompatible with stack diff workflows generally. It's tricky to review individual commits on Github and Gitlab and they don't even allow you to leave comments on the commit messages. In areas where people do review individual commits they tend to use tools that support that workflow. Git uses email patches where each commit is sent as a separate email. Tools like Gerrit do code review directly on a commit by commit basis and the usual strategy to get it into trunk is to either "fast forward" (advance the HEAD of the trunk branch to that commit) or to cherry-pick it.
- keybored 2y ago
  A branch with some base is already a series of commits. I don’t get where the conceptual re-imagining is here.
  - gorset 2y ago
    Patch series comes from the linux kernel workflow, which git was developed to support. https://kernelnewbies.org/PatchSeries https://kernelnewbies.org/PatchSeries In this workflow you review every commit and not just the branch diff. Each commit is crafted carefully, and a well crafter series of commits can make even very large changes a brief to review. It takes a certain skill to do this well. As the page above says > Crafting patches is one of the core activities in contributing code to the kernel, it takes practice and thought. This is in contrast to using git more as a distributed filesystem where you don't care particularly much about the history, and you typically squash commits before merging etc. It's simpler and easier to work this way, but you lose some of the nice attributes of the linux kernel workflow.
    - keybored 2y ago
      That’s a nice summary. What I don’t like about the Git documentation as I’ve read it is that they go between “patch” and “commit” in some places without stopping and explaining what the difference is. It makes sense to them. It’s obvious. But it isn’t necessarily obvious to most people. A patch is a patch proper plus a commit message encoded in a format that git am understands. That’s fine. And the core developers understand that you cannot transmit a commit snapshot via email (or you shouldn’t). But I prefer to mostly stick to “commit” in the abstract sense, whether that to-be-commit is from a pull or from an email (or: it’s in the form of an email and it could be applied as a commit). git rebase talks about “patch series” I think. Without explaining it. Why not “commit series”? Sometimes it seems like talking about your changes by the way it happened to be transmitted. It’s like talking about “attachments” instead of commits because you happened to send them via email as an attachment (instead of inline). Then you now have “stacked diffs” or “stacked commits”. Which are just a series of commits. Or a branch of commits (implicitly grounded by a base commit). For a while I was wondering what stacked diffs/stacked PRs/stacked patches and if I was missing out. When it just turned out to be, as you explain, essentially the Linux Kernel style of being able to review a commit in isolation. But in a sort of context that pull request inhabitants can understand. I prefer to mostly talk about these things as “commits”. (At several times writing those paragraphs above I wondered if I would be able to string together them in a coherent way)
strunz 2y ago
Commits should be a contained change that can be understood as a logical piece of history, and reverted if necessary. If you have to look at multiple commits for 1 logical change to the code, it's much more difficult to figured out what the intention was, if it was correct, and how it can be reverted.
- notlinus 2y ago
  Do you always make all of your logical code changes in single, atomic commits? What if you have to modify a feature later on? I'm sorry, but this is ridiculous. There's nothing wrong with having multiple commits in a row modifying something. That's how git works. GitHub's PR merge workflow really messed up the meta game, I tell you what...
  - brigandish 2y ago
    I don't think you're arguing against the point being made. Logical change 1, implement Hello World: puts "Hello, World!" Logical change 2, make it a method: def hello_world puts "Hello, World" end Logical change 3, take an argument for the greeting: def hello_world(greeting) puts "#{greeting}, World" end Logical change 4, take an argument for the recipient: def hello_world(greeting, recipient) puts "#{greeting}, #{recipient}" end Logical change 5, improve the method name by showing the intent: def greet_recipient(greeting, recipient) puts "#{greeting}, #{recipient}" end Logical change 6, defaults…: def greet_recipient(greeting="Hello", recipient) puts "#{greeting}, #{recipient}" end And so on. These could each be their own feature or part of a feature etc. What they are, though, is logically atomic. If you want to add a default recipient later, or next, that is its own logical commit. If you roll it back, each rollback will lead you to a logical difference, not some typo fix, e.g. def greet_recipient(greeting="Hell", recipient) puts "#{greeting}, #{recipient}" end plus the missing "o" def greet_recipient(greeting="Hello", recipient) puts "#{greeting}, #{recipient}" end Are one logical commit, for which I'd use a fixup or `git commit --amend` or whatever. What one considers logically atomic may differ from person to person, language to language, feature to feature… but they can be differentiated from things like typos quite easily. Personally, I make numerous commits in a feature branch, (transparently, too, my typos included - I'm not proud:) and before requesting review I'll clean up using rebase into as few logical commits as possible, and upon acceptance either squash it all to one or leave it as is, depending on the team's culture/needs.
nextaccountic 2y ago
If it is unreleased it may make the life of reviewers easier. If they look at the commits at all, that is. But if not even reviewers are looking at commits I question whether a PR should be chunked in commits at all - why not squash all commits into a single one, so that every PR is composed of exactly one commit? Could maybe separating the changes in commits convey some information?
- toastal 2y ago
  Normally creating a new feature involves independent actions like updating dependency, refactoring some module beforehand, & so on. If these things can be broken up in a way that make review & reading logs easier, why would you squash the whole history—especially if that history is already good. In as much as it would be ridiculous to go to the other extreme of committing ever character/line as a separate commit, just collapsing the whole history isn’t a good approach. There isn’t a hardened rule about where the line is but that is okay & up to developer discretion on how they want to ‘tell the story’ to a reviewer (or their future self) about how certain chunks should be read together.
ozim 2y ago
I am not history purist in that way. I don’t care about fixes for mistakes, if they are in your local branch do the cleanup before publish and if it is your own published branch force push is ok. Unless of course your changes were merged to common code already - then they become not touchable - just make a new commit with fix don’t try to be smart, common code rolls only forward and fixes are new commits.
globular-toast 2y ago
Why store git history at all? It's useless if you don't take care of it. Have you ever used git history for anything? People use it to find the source of regressions (you can do it quite quickly using git bisect).
- froh 2y ago
  the git history of the linux kernel documents the Linux kernel detailed Design, the reasoning how things fit together. I've used it several times to understand why things are done the way they are. The discussion leading to this design is encoded in the revisions of the patch series, browsable on patchworks. I agree to your point that the git history has to be taken care of to be useful.
  - globular-toast 2y ago
    My question was, of course, rhetorical. This and hunting regressions are two great reasons to keep history. "What did John do on Tuesday morning" is not a good reason to keep history. Neither is "This change was made as part of a sprawling "fix" commit that happened on Thursday afternoon". The important point is if you are going to keep history at all it needs to be kept in a state that is actually useful, meaning useful for the first two things, not the last two. Otherwise it's just a waste of disk space. The git history is meticulously maintained which is what makes it actually useful.
- keybored 2y ago
  Happens on all these threads. - Thread: This helps you make a useful Git history - Counter-point: Why are you using a version control system to make a useful history? Why does the history matter? I have been developing software for eighty years and neither I nor anyone I’ve met have cared.
- jeltz 2y ago
  I use the git history all the time. For example the git history of PostgreSQL is amazing but I have also worked with proprietary software with good git history. And one key to good git history is people splitting up big PRs in multiple commits.
eviks 2y ago
Because it's easier to understand later on when it's not spread out
IshKebab 2y ago
The point of history is so that other people (or your future self) can look back at the history of the code and understand how it changed and why changes were made. There's zero value in retaining mistakes that were never merged into `master`. In fact there is negative value because it makes the history harder to follow. For example would you rather see "review fixes" in Git blame, or the actual useful commit message? You can argue that you can't be bothered to make your history nice; fine. But you can't say it's wrong to do that. (Personally I think you should squash commits for a MR; if it's too big to review as one commit it's too big for one MR - except for branches that multiple people have worked on over an extended period.)
- Terr_ 2y ago
  > There's zero value in retaining mistakes that were never merged into `master`. Not to be too pedantic about this, it sounds like you mean flawed commits that were merged into master, but never had a chance to be the HEAD of master? (In other words, they always arrived along with a fix-commit too, so that nobody checking out code could've hit the bug.) > In fact there is negative value because it makes the history harder to follow. It also hampers your ability to use tools like git-bisect to detect when behavior changed, especially if some of the bad commits won't compile or pass unit tests.
  - IshKebab 2y ago
    > flawed commits that were merged into master, but never had a chance to be the HEAD of master Yeah exactly.
theptip 2y ago
It sounds like you have never heard of the Stacked Diffs workflow. Here is an overview: https://newsletter.pragmaticengineer.com/p/stacked-diffs https://newsletter.pragmaticengineer.com/p/stacked-diffs It’s a perfectly reasonable preference to eschew this workflow, but definitely worth understanding the pros and cons.
- dpkirchner 2y ago
  Every explanation of stacked diffs I've seen (including this one) makes me think there is some secret magic not being shared that will make the whole idea "click". If the problem is devs having to wait too long for PRs to be reviewed, breaking tasks up in to smaller diffs should exacerbate the problem, wouldn't it? If diff 2..n follow diff 1 then a review of 1 blocks everything else you're doing, eh? At least the traditional route, branching everything straight off of main, means you can merge 2..n and rebase 1 once it passes, leaving you less blocked. There must be something I'm missing because there are a lot of people promoting stacking diffs.
  - radlad 2y ago
    > If the problem is devs having to wait too long for PRs to be reviewed, breaking tasks up in to smaller diffs should exacerbate the problem, wouldn't it? I find small diffs to be much easier to review and as a result I review them much practically as soon as they are posted. And I'm less likely to miss something.
    - dpkirchner 2y ago
      For sure, I agree. However there is also a context switching cost and IMO switching between your IC work and, say, a dozen small diffs is rougher than a couple larger diffs. That opinion said I haven't experienced fast reviews myself, so maybe that's what I'm missing.
  - steveklabnik 2y ago
    I recently mentally came around to stacked PRs, though I currently work mostly solo, and so don't use them. But I'm happy to try and talk with you about it. Maybe because I learned so recently it might help. > If diff 2..n follow diff 1 then a review of 1 blocks everything else you're doing, eh? At least the traditional route, branching everything straight off of main, means you can merge 2..n and rebase 1 once it passes, leaving you less blocked. I am a little confused about the scenario you're describing here: with the PR based flow, you can't merge the second half of the PR while waiting on some change to the first commit. So let me try to suggest a scenario. We're working on a web app. We're going to add a new feature. This requires some backend changes, and then some front end changes that use those back end changes. For the immediate purposes here, we're going to presume these are in the same PR. I'll talk about them as two PRs in a moment. We're also going to assume for the sake of simplicity that there's two commits in this PR: one for backend, one for frontend. With the PR workflow, you're asking for review from two people: a backend expert, and a frontend expert. Both are going to request changes, and you're gonna respond to all of them, and eventually things get to a good place and the PR is merged. Let's say that process takes a week. There's a few possible issues here: the first is, both people are going to get notifications for every change in the PR, even if it's not in the code that they need to review. This creates additional review fatigue for both reviewers. They also may need to check what's happened since the last time they reviewed things, and github doesn't always make that easy, though they do help sometimes in some circumstances. Another issue is, say that the backend changes are good to go, but there's more frontend work to do: because it's in the same PR, the backend change will not be integrated until the frontend change is done. And the backend reviewer will still be getting those pings. Notably, if the frontend change is ready first, they'll also be getting pings from more backend reviews too. In the stacked diff flow, and using those tools, each reviewer only needs to review their half individually. They will not get pinged for changes to the other half. They will be presented with only the diffs that affect the parts that they need to review. And, because you can land each diff in the stack whenever it's at the top (or bottom, depending on how you visualize things, I guess) of the stack, as soon as the backend commit is good to go, it can land, and the frontend commit can land at a later time. We've popped it off the stack. And no matter who finishes first, they won't get review pings for the other part's work. So, you might say: yeah, that's exactly why I'd make them as two separate PRs. And that may be a good idea. But to do that, you have two choices: wait until the backend stuff is fully done, and then start on the frontend stuff. That can work, sure, but maybe what you want to do needs the frontend work to influence the backend work, so treating it as one logical change before any of it hits master makes sense. So to do that, you make a frontend PR that, instead of branching off of master, branches off of the backend PR. Well, now you've reinvented stacked diffs, but you don't have any of the tooling to make that nice! Once the backend lands, you'll have to rebase the frontend off of master, stuff like that. ... does any of that make sense?
Izkata 2y ago
For me it's not purism, but practicality: The top comment right now mentions using this to apply linting changes to the original commit that introduced the linting violation, but I've seen linting commits introduce bugs often enough that I think those should always be a new separate commit, so a future maintainer can easily see why that kind of bug was introduced and what the fix is.
baq 2y ago
the history you're merging to the main branch is much more useful clean than accurate.
chippiewill 2y ago
Amusingly it actually makes you less of a "purist" to not modify commits. Git was created by Linus Torvalds to support Linux Kernel development The Linux Kernel is heavily built around reviewing each commit individually via patchsets sent as emails and rewriting history to make it cleaner. Git history is incredibly valuable if its curated, at my previous employer we used Gerrit and pretty much every change would have a well crafted multi-line commit message that explained what was being changed and most importantly _why_. If I wanted to understand why a bit of the codebase had been authored in a particular way it was incredibly easy because I could just git blame the line and there'd be a detailed commit message explaining the rationale. In the Pull Request world I do agree with what you say - but only if you squash merge. Commits that say "fix typo" or "oops" or "wip" aren't useful long term.
cryptonector 2y ago
If you haven't pushed then you want to eventually push clean history. That's what rebasing is about.

Maybe I am being to much of a purist, but retroactively modifying commits and history? Why? Stuff happens, so do mistakes. Fix the mistakes, make another commit, and go on with your life.

rom1v 2y ago

https://www.mail-archive.com/dri-devel@lists.sourceforge.net/msg39091.html https://www.mail-archive.com/dri-devel@lists.sourceforge.net...

gorset 2y ago

The use case is when you look at a branch as a series of patches. Reviewing a clean set of commits is much easier than a branch full of mistakes and wrong paths taken. Useful when we optimize for reviewing and good history for future maintenance. This has been important and useful when I’ve worked on big mission critical backend system, but I also understand it might not be the most important factor for a new project’s success.

dclowd9901 2y ago
So this wouldn’t work very well in workflows that flatten merges to a trunk?
- travisjungroth 2y ago
  It would mostly be unnecessary. The separate commits won’t matter after the PR if they’re getting squashed. Debatably, if you’re making changes during a PR review, it could be helpful to make those changes in relevant commits. That way if someone goes through them during the PR, they get one clean “story”, rather than see the pre-PR commits and the conversation after.
- mst 2y ago
  I like for non-trivial stuff to have a branch with a series of logical commits, cleanly rebased atop main, then use -no-ff to force a merge commit anyway. That way you can the whole branch appears as a single commit/diff atop main in primary history but you can dig in to the original components of it if/when that's useful. The primary obstacle to doing this for me is, if I'm honest, not having automated it sufficiently that I can't forget to do that.
- chippiewill 2y ago
  Tools where they allow you to squash merge tend to be fairly incompatible with stack diff workflows generally. It's tricky to review individual commits on Github and Gitlab and they don't even allow you to leave comments on the commit messages. In areas where people do review individual commits they tend to use tools that support that workflow. Git uses email patches where each commit is sent as a separate email. Tools like Gerrit do code review directly on a commit by commit basis and the usual strategy to get it into trunk is to either "fast forward" (advance the HEAD of the trunk branch to that commit) or to cherry-pick it.
keybored 2y ago
A branch with some base is already a series of commits. I don’t get where the conceptual re-imagining is here.
- gorset 2y ago
  Patch series comes from the linux kernel workflow, which git was developed to support. https://kernelnewbies.org/PatchSeries https://kernelnewbies.org/PatchSeries In this workflow you review every commit and not just the branch diff. Each commit is crafted carefully, and a well crafter series of commits can make even very large changes a brief to review. It takes a certain skill to do this well. As the page above says > Crafting patches is one of the core activities in contributing code to the kernel, it takes practice and thought. This is in contrast to using git more as a distributed filesystem where you don't care particularly much about the history, and you typically squash commits before merging etc. It's simpler and easier to work this way, but you lose some of the nice attributes of the linux kernel workflow.
  - keybored 2y ago
    That’s a nice summary. What I don’t like about the Git documentation as I’ve read it is that they go between “patch” and “commit” in some places without stopping and explaining what the difference is. It makes sense to them. It’s obvious. But it isn’t necessarily obvious to most people. A patch is a patch proper plus a commit message encoded in a format that git am understands. That’s fine. And the core developers understand that you cannot transmit a commit snapshot via email (or you shouldn’t). But I prefer to mostly stick to “commit” in the abstract sense, whether that to-be-commit is from a pull or from an email (or: it’s in the form of an email and it could be applied as a commit). git rebase talks about “patch series” I think. Without explaining it. Why not “commit series”? Sometimes it seems like talking about your changes by the way it happened to be transmitted. It’s like talking about “attachments” instead of commits because you happened to send them via email as an attachment (instead of inline). Then you now have “stacked diffs” or “stacked commits”. Which are just a series of commits. Or a branch of commits (implicitly grounded by a base commit). For a while I was wondering what stacked diffs/stacked PRs/stacked patches and if I was missing out. When it just turned out to be, as you explain, essentially the Linux Kernel style of being able to review a commit in isolation. But in a sort of context that pull request inhabitants can understand. I prefer to mostly talk about these things as “commits”. (At several times writing those paragraphs above I wondered if I would be able to string together them in a coherent way)

strunz 2y ago

Commits should be a contained change that can be understood as a logical piece of history, and reverted if necessary. If you have to look at multiple commits for 1 logical change to the code, it's much more difficult to figured out what the intention was, if it was correct, and how it can be reverted.

notlinus 2y ago
Do you always make all of your logical code changes in single, atomic commits? What if you have to modify a feature later on? I'm sorry, but this is ridiculous. There's nothing wrong with having multiple commits in a row modifying something. That's how git works. GitHub's PR merge workflow really messed up the meta game, I tell you what...
- brigandish 2y ago
  I don't think you're arguing against the point being made. Logical change 1, implement Hello World: puts "Hello, World!" Logical change 2, make it a method: def hello_world puts "Hello, World" end Logical change 3, take an argument for the greeting: def hello_world(greeting) puts "#{greeting}, World" end Logical change 4, take an argument for the recipient: def hello_world(greeting, recipient) puts "#{greeting}, #{recipient}" end Logical change 5, improve the method name by showing the intent: def greet_recipient(greeting, recipient) puts "#{greeting}, #{recipient}" end Logical change 6, defaults…: def greet_recipient(greeting="Hello", recipient) puts "#{greeting}, #{recipient}" end And so on. These could each be their own feature or part of a feature etc. What they are, though, is logically atomic. If you want to add a default recipient later, or next, that is its own logical commit. If you roll it back, each rollback will lead you to a logical difference, not some typo fix, e.g. def greet_recipient(greeting="Hell", recipient) puts "#{greeting}, #{recipient}" end plus the missing "o" def greet_recipient(greeting="Hello", recipient) puts "#{greeting}, #{recipient}" end Are one logical commit, for which I'd use a fixup or `git commit --amend` or whatever. What one considers logically atomic may differ from person to person, language to language, feature to feature… but they can be differentiated from things like typos quite easily. Personally, I make numerous commits in a feature branch, (transparently, too, my typos included - I'm not proud:) and before requesting review I'll clean up using rebase into as few logical commits as possible, and upon acceptance either squash it all to one or leave it as is, depending on the team's culture/needs.

nextaccountic 2y ago

If it is unreleased it may make the life of reviewers easier. If they look at the commits at all, that is. But if not even reviewers are looking at commits I question whether a PR should be chunked in commits at all - why not squash all commits into a single one, so that every PR is composed of exactly one commit? Could maybe separating the changes in commits convey some information?

toastal 2y ago
Normally creating a new feature involves independent actions like updating dependency, refactoring some module beforehand, & so on. If these things can be broken up in a way that make review & reading logs easier, why would you squash the whole history—especially if that history is already good. In as much as it would be ridiculous to go to the other extreme of committing ever character/line as a separate commit, just collapsing the whole history isn’t a good approach. There isn’t a hardened rule about where the line is but that is okay & up to developer discretion on how they want to ‘tell the story’ to a reviewer (or their future self) about how certain chunks should be read together.

ozim 2y ago

I am not history purist in that way. I don’t care about fixes for mistakes, if they are in your local branch do the cleanup before publish and if it is your own published branch force push is ok. Unless of course your changes were merged to common code already - then they become not touchable - just make a new commit with fix don’t try to be smart, common code rolls only forward and fixes are new commits.

globular-toast 2y ago

Why store git history at all? It's useless if you don't take care of it. Have you ever used git history for anything? People use it to find the source of regressions (you can do it quite quickly using git bisect).

froh 2y ago
the git history of the linux kernel documents the Linux kernel detailed Design, the reasoning how things fit together. I've used it several times to understand why things are done the way they are. The discussion leading to this design is encoded in the revisions of the patch series, browsable on patchworks. I agree to your point that the git history has to be taken care of to be useful.
- globular-toast 2y ago
  My question was, of course, rhetorical. This and hunting regressions are two great reasons to keep history. "What did John do on Tuesday morning" is not a good reason to keep history. Neither is "This change was made as part of a sprawling "fix" commit that happened on Thursday afternoon". The important point is if you are going to keep history at all it needs to be kept in a state that is actually useful, meaning useful for the first two things, not the last two. Otherwise it's just a waste of disk space. The git history is meticulously maintained which is what makes it actually useful.
keybored 2y ago
Happens on all these threads. - Thread: This helps you make a useful Git history - Counter-point: Why are you using a version control system to make a useful history? Why does the history matter? I have been developing software for eighty years and neither I nor anyone I’ve met have cared.
jeltz 2y ago
I use the git history all the time. For example the git history of PostgreSQL is amazing but I have also worked with proprietary software with good git history. And one key to good git history is people splitting up big PRs in multiple commits.

eviks 2y ago

Because it's easier to understand later on when it's not spread out

IshKebab 2y ago

The point of history is so that other people (or your future self) can look back at the history of the code and understand how it changed and why changes were made. There's zero value in retaining mistakes that were never merged into `master`. In fact there is negative value because it makes the history harder to follow. For example would you rather see "review fixes" in Git blame, or the actual useful commit message? You can argue that you can't be bothered to make your history nice; fine. But you can't say it's wrong to do that. (Personally I think you should squash commits for a MR; if it's too big to review as one commit it's too big for one MR - except for branches that multiple people have worked on over an extended period.)

Terr_ 2y ago
> There's zero value in retaining mistakes that were never merged into `master`. Not to be too pedantic about this, it sounds like you mean flawed commits that were merged into master, but never had a chance to be the HEAD of master? (In other words, they always arrived along with a fix-commit too, so that nobody checking out code could've hit the bug.) > In fact there is negative value because it makes the history harder to follow. It also hampers your ability to use tools like git-bisect to detect when behavior changed, especially if some of the bad commits won't compile or pass unit tests.
- IshKebab 2y ago
  > flawed commits that were merged into master, but never had a chance to be the HEAD of master Yeah exactly.

theptip 2y ago

It sounds like you have never heard of the Stacked Diffs workflow. Here is an overview: https://newsletter.pragmaticengineer.com/p/stacked-diffs https://newsletter.pragmaticengineer.com/p/stacked-diffs It’s a perfectly reasonable preference to eschew this workflow, but definitely worth understanding the pros and cons.

dpkirchner 2y ago
Every explanation of stacked diffs I've seen (including this one) makes me think there is some secret magic not being shared that will make the whole idea "click". If the problem is devs having to wait too long for PRs to be reviewed, breaking tasks up in to smaller diffs should exacerbate the problem, wouldn't it? If diff 2..n follow diff 1 then a review of 1 blocks everything else you're doing, eh? At least the traditional route, branching everything straight off of main, means you can merge 2..n and rebase 1 once it passes, leaving you less blocked. There must be something I'm missing because there are a lot of people promoting stacking diffs.
- radlad 2y ago
  > If the problem is devs having to wait too long for PRs to be reviewed, breaking tasks up in to smaller diffs should exacerbate the problem, wouldn't it? I find small diffs to be much easier to review and as a result I review them much practically as soon as they are posted. And I'm less likely to miss something.
  - dpkirchner 2y ago
    For sure, I agree. However there is also a context switching cost and IMO switching between your IC work and, say, a dozen small diffs is rougher than a couple larger diffs. That opinion said I haven't experienced fast reviews myself, so maybe that's what I'm missing.
- steveklabnik 2y ago
  I recently mentally came around to stacked PRs, though I currently work mostly solo, and so don't use them. But I'm happy to try and talk with you about it. Maybe because I learned so recently it might help. > If diff 2..n follow diff 1 then a review of 1 blocks everything else you're doing, eh? At least the traditional route, branching everything straight off of main, means you can merge 2..n and rebase 1 once it passes, leaving you less blocked. I am a little confused about the scenario you're describing here: with the PR based flow, you can't merge the second half of the PR while waiting on some change to the first commit. So let me try to suggest a scenario. We're working on a web app. We're going to add a new feature. This requires some backend changes, and then some front end changes that use those back end changes. For the immediate purposes here, we're going to presume these are in the same PR. I'll talk about them as two PRs in a moment. We're also going to assume for the sake of simplicity that there's two commits in this PR: one for backend, one for frontend. With the PR workflow, you're asking for review from two people: a backend expert, and a frontend expert. Both are going to request changes, and you're gonna respond to all of them, and eventually things get to a good place and the PR is merged. Let's say that process takes a week. There's a few possible issues here: the first is, both people are going to get notifications for every change in the PR, even if it's not in the code that they need to review. This creates additional review fatigue for both reviewers. They also may need to check what's happened since the last time they reviewed things, and github doesn't always make that easy, though they do help sometimes in some circumstances. Another issue is, say that the backend changes are good to go, but there's more frontend work to do: because it's in the same PR, the backend change will not be integrated until the frontend change is done. And the backend reviewer will still be getting those pings. Notably, if the frontend change is ready first, they'll also be getting pings from more backend reviews too. In the stacked diff flow, and using those tools, each reviewer only needs to review their half individually. They will not get pinged for changes to the other half. They will be presented with only the diffs that affect the parts that they need to review. And, because you can land each diff in the stack whenever it's at the top (or bottom, depending on how you visualize things, I guess) of the stack, as soon as the backend commit is good to go, it can land, and the frontend commit can land at a later time. We've popped it off the stack. And no matter who finishes first, they won't get review pings for the other part's work. So, you might say: yeah, that's exactly why I'd make them as two separate PRs. And that may be a good idea. But to do that, you have two choices: wait until the backend stuff is fully done, and then start on the frontend stuff. That can work, sure, but maybe what you want to do needs the frontend work to influence the backend work, so treating it as one logical change before any of it hits master makes sense. So to do that, you make a frontend PR that, instead of branching off of master, branches off of the backend PR. Well, now you've reinvented stacked diffs, but you don't have any of the tooling to make that nice! Once the backend lands, you'll have to rebase the frontend off of master, stuff like that. ... does any of that make sense?

Izkata 2y ago

For me it's not purism, but practicality: The top comment right now mentions using this to apply linting changes to the original commit that introduced the linting violation, but I've seen linting commits introduce bugs often enough that I think those should always be a new separate commit, so a future maintainer can easily see why that kind of bug was introduced and what the fix is.

baq 2y ago

the history you're merging to the main branch is much more useful clean than accurate.

chippiewill 2y ago

Amusingly it actually makes you less of a "purist" to not modify commits. Git was created by Linus Torvalds to support Linux Kernel development The Linux Kernel is heavily built around reviewing each commit individually via patchsets sent as emails and rewriting history to make it cleaner. Git history is incredibly valuable if its curated, at my previous employer we used Gerrit and pretty much every change would have a well crafted multi-line commit message that explained what was being changed and most importantly _why_. If I wanted to understand why a bit of the codebase had been authored in a particular way it was incredibly easy because I could just git blame the line and there'd be a detailed commit message explaining the rationale. In the Pull Request world I do agree with what you say - but only if you squash merge. Commits that say "fix typo" or "oops" or "wip" aren't useful long term.

cryptonector 2y ago

If you haven't pushed then you want to eventually push clean history. That's what rebasing is about.