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.

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.

So this wouldn’t work very well in workflows that flatten merges to a trunk?

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.

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.

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.

A branch with some base is already a series of commits. I don’t get where the conceptual re-imagining is here.

Patch series comes from the linux kernel workflow, which git was developed to support. 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.

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)

I think part of the confusion is because 'patch' and 'commit' (really snapshot) are duals of each other, but in practice have important technical differences. When speaking abstractly about 'changes' it often doesn't much matter which term is used, but most interactions are with 'commits' so that tends to be the default term to use.

However, sometimes the details matter. For example, a 'patch' (diff + description) tends to be small enough to transfer conveniently, and human friendly. Patches do not describe their relationship to other patches, so it makes sense to talk about a series of patches which must be applied in sequence to accomplish some larger goal. 'patch' doesn't imply any particular storage format, so sometimes saying 'attachment' or 'patch file' or 'email body' is an important distinction. Git documentation assumes you know what you want. It might help to think of patches as "outside time", though of course any particular version of a patch will only apply to a subset of all snapshots.

A 'commit' (snapshot + description), on the other hand, tends to be large in that it describes the entire state of the codebase and contains (implicit or explicit) metadata describing its predecessor(s) -- possibly a large graph of them. People talk about a 'commit series' all the time, but use the terms 'history' and 'branch' instead.

Stacked PRs are built on top of commits and branches to gain the advantages of a patch series while retaining the advantages of snapshots. There's no industry consensus on if they are preferable, let alone how to best implement them (eg. rebase, rebase+squash, merge-squash, merge+first-parent, etc.), so different people have different ideas about what they look like. It isn't correct to say they are just a series of commits, because sometimes they are implemented as a series of (implicit or anonymous) branches. One of the few agreed-upon features of stacked PRs (or its other names) is that it is a sequence of 'changes' which are ordered to both satisfy change dependencies and broken into smaller pieces which "tells a story" to reviewers.

It certainly matters when you e.g. find a patch in the wild and have no idea what commit it was originally based on. Metadata fiddling becomes more important in email workflows.

Conceptually, a branch is a series of tree states. The patches are derived from that.

Conceptually a branch is the last snapshot in a series of snapshots.

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.

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...

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.

If those are all different commits, now imagine the fun of interactively rebasing them on top of a main branch where some other thing changed "Hello, World" to "Greetings dear Globe."

Why is someone else writing my feature? Do we not have a ticket system? ;-)

More importantly, when using a feature branch one should really rebase master into the feature branch regularly, and any other branches that might touch the same places, it takes care of little surprises like this.

> Do we not have a ticket system?

Maybe we do, maybe we don't. Maybe there are two tickets with a mutual dependence.

Even without that, if you add a parameter to a method, and later someone uses it in a totally different PR, then you effectively can't revert the creation of the parameter. How are you going to track that dependency. I've heard the arguments about logical commits and the ability to cleanly revert them. I just haven't seen any code where that would actually work, even if the trouble was taking artisanally curate the commit history.

> Maybe we do, maybe we don't.

If I'm in a workplace like that then I wouldn't be surprised to find a poor git flow.

> How are you going to track that dependency.

The commit graph (which is linear due to rebasing). The extra parameter commit comes before the commits using it. If you want to remove that parameter then you have to revert the features that rely on it or refactor them, there's no magical way to keep dependent code if you want to revert the code it depends on. Having logically atomic commits makes this easier, not more difficult.

> Maybe there are two tickets with a mutual dependence.

Then rebase on each other's work until its ready for review (or, even better, don't work on dependent features in the same sprint). Again, logical, atomic commits would make that easier.

The real point is, what do you do when someone introduces a parameter in two functions, one is related to your work but the other isn't, and they bundled both changes in one commit, and the changes aren't even related. Now that's a mess.

If the second parameter isn't supposed to be there I'd just make another PR removing it.

If we use commit ancestry to determine logical dependence then this "revert the bug commit" thing won't work unless the bug just so happens to be in the very last commit.

You've created a straw man. No one can make code magically not depend on other code if it does - it's a tautology. No one can argue for better organisation if it's not wanted - feel free to not use a ticket system, don't communicate work, don't rebase work that is interconnected, and don't use logical, atomic commits. I'm not sure that Git is going to be a help given those requirements.

As I've already pointed out, I'm not sure what you're arguing against but it's none of the points made in this thread.

To clarify, I meant a scenario where that very simplest version is already upstream, and your feature-branch contains 5-6 commits which are all different refactoring-steps or making the string literals override-able or abstracted away.

When you need to rebase that onto main/master, there would be 5-6 stops to fix conflicts in a 3-way diff, and I don't think the kind of work in that example is compelling enough to justify that as opposed to having one (squashed) commit that simply states that you improved the method in several ways.

P.S.: Sometimes more commits can make refactoring easier instead, often when it comes to splitting file rename/move operations from changing their content.

I'm not against squashing, especially for merging into master (although, again, that should also constitute a logical whole), but doesn't rebasing a feature branch onto master first take care of this?

`git rerere` is your friend.

Yes, always. Of course I make mistakes, but the idea is that any patch I put up for review, if it cleanly applies, should not break any user and any tests. It's much more liberating knowing that bisects actually work and each commit does exactly what it says on the tin (no followup "fix"/"minor" commits that actually do something completely different). Incremental improvements are compatible with this approach. Also see https://gist.github.com/thoughtpolice/9c45287550a56b2047c631... .

GitHub's PR flow is what brought this mess of fixup commits. In the previously common mailing list flow the normal practice is to amend your patches based on feedback. Translated to a PR flow this means rewriting the branch.

I'd say pull request should be reverted if necessary, but an idea that each commit should be revertable and expectation that project should work after it is unneeded complexity for me.

I'd also point out that it's bad to have commits in the main branch where compilation or unit tests would fail, even when each bad-commit always arrives with an immediate "oops, fixed" commit trailing it.

It messes up the team's ability to use tools like git-bisect to pinpoint behavior changes, and is generally more noise than signal.

At a bare minimum, those spans should get squashed before they leave the PR or feature branch.

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

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?

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.

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).

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.

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.

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.

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.

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.)

> 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.

> flawed commits that were merged into master, but never had a chance to be the HEAD of master

Yeah exactly.

It sounds like you have never heard of the Stacked Diffs workflow.

Here is an overview: 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.

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.

> 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.

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.

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?

> 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.

As I understand it, based on what's in the article anyway, diff 2 is started from diff 1 even when they are totally different tasks, so you'll be blocked in the sense that you'll have to rebase or merge if the review finds a problem.

I figure you might as well branch 2 off of main while you wait for the go-ahead to merge 1 into main. Stacked diffs don't seem to meaningfully change this flow.

> 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.

I guess part of it is I have never worked in an environment where the frontend and backend work didn't need to be closely coordinated anyway, so those pings aren't a big deal. Everyone needs to know what's going on, to minimize rework.

> ... does any of that make sense?

I dunno. Maybe my brain isn't compatible, heh.

> I figure you might as well branch 2 off of main while you wait for the go-ahead to merge 1 into main.

Ah, so sure, if you're talking about two separate pull requests, then that makes sense. But the equivalent with stacked diffs just simply wouldn't be stacked. These discussions are inherently on work that is dependent on previous work.

> so those pings aren't a big deal.

Yeah I wasn't sure if my specific suggestion would be too specific. Examples are hard.

> Maybe my brain isn't compatible, heh.

It's all good, I felt the same way for a long time. Maybe if you are bored and play around with Gerrit, (which is admittedly a bit janky) it'll make more sense. Or maybe not :)

In my experience with stacked diffs the smaller diffs doesn't exacerbate the problem because they are pipelined.

When getting merge approval takes at least one or two business days (say because of time zones or your colleagues are doing deep work which they don't want to drop every fifteen minutes to do a review) the pipelining is necessary to be unblocked.

In my experience five 100-line diffs get reviewed much faster than one 500-line diff.

Especially when you follow best practices like splitting refactors from functional work; the canonical stacked diff for me is “refactor to make the upcoming change easy; add some tests for the existing functionality I want to protect; small diff to actually make the change”.

1 & 2 don’t generally require caffeine to review. 3 probably requires engaging the brain. If you put everything together you need to be on high alert for the whole review.

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.

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

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.

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