The issue isn't `Fix linting issues`, the issue is a `Fix linting issues` commit for issues you introduced in the code you'll be pushing with that `Fix linting issues` commit. Nobody wants to see you pre-push dev history -- it's not at all interesting. So rebase and squash/fixup such commits, split/merge commits -- do what you have to to make the history you do push to the upstream useful.

Now, if you find linting issues in the upstream and fix those, then a `Fix linting issues` commit is perfectly fine.

It makes git bisect more difficult than it needs to be.

> What is wrong with simply pushing a "Fix linting issues" in a new commit?

if you want every individual commit be buildable then it is a no-go. it's also a no-go if you don't squash your prs.

“Fix lint” commits also taint git blame.

You could perhaps add some kind of filter to git blame to automatically skip commits whose message is “fix lint” but at some point the cure is worse than the disease.

I also see people argue that merge commits make git bisect harder than squashing in the first place but there is a third option: rebase and fast forward. If every commit in your branch is clean and stand-alone that’s viable. Linter fix commits break that paradigm.

> What is wrong with simply pushing a "Fix linting issues" in a new commit?

Everything.

1. git blame is obfuscated. "Fix lint" is not helpful or relevant. Tell me what actually changed.

2. git log is noisy. More stuff is harder to read than less stuff, and you're making me read more stuff.

3. git bisect is difficult. Interspersed broken commits requires more time to sift through. (Lint is a poor example, say it's "fix server config")

4. git cherry-pick is tedious. When you copy the dev change to a release branch (let's say you maintain 1.x, 2.x, etc), you must include errata.

In some teams, you are not allowed to submit any commit that breaks the build, and a lint failure would be considered a broken build.

> What is wrong with simply pushing a "Fix linting issues" in a new commit? It's self-contained and very well describes the (single) purpose of the commit.

Because I care a lot more about the logical history of the source code versus the actual history of the source code. Commits like "fix linting issues" are more like artifacts of the development process. The commit exists likely because you forgot to run the linter earlier, and only found the issue after creating the commit. Logically speaking, those fixes belong with the relevant change.

Now, if you're just going to squash the PR you're working on, then sure. The extra commit totally doesn't matter. Make as many of those commits as you want.

But if your PR is 5 commits that you want to "rebase & merge" as-is (which is something I do pretty frequently), and you care about the history being easy to navigate, then it can totally make sense to do fixups before merging instead of creating new "fix linting issues" commits. If the follow the latter, then your commit history winds up being littered with those sorts of things. And fewer commits will likely pass tests (because you created new commits to fix tests).

I want to be VERY CLEAR, that I do not agree with your use of the word "wrong." I do not want to call your workflow wrong. Communicating the nuances and subtleties of workflows over text here is very difficult. For example, one take-away from my comment here is that you might think I always work this way. But I don't! I use "squash & merge" plenty myself, in which case, git-absorb is less useful. And sometimes breaking a change down into small atomic commits is actually a ton of work, and in that case, I might judge it to not be worth it. And still yet other times, I might stack PRs (although that's rare because I find it very annoying). It always depends.

> But otherwise I think it's a little bit problematic since it makes it harder for the reviewer to inspect and understand what changes have been done. But it also makes it harder for a developer since not all fixes are context-free, e.g. a fix for an issue found in the PR cannot always be attributed to the self-contained and single commit in your branch but it's the composition of multiple commits that actually makes this bug appear.

I'm having a hard time understanding your concern about the reviewer. But one aspect of breaking things down into commits is absolutely to make it easier for the reviewer. Most of my PRs should be read as a sequence of changes, and not as one single diff. This is extremely useful when, in order to make a change, you first need to do some refactoring. When possible (but not always, because sometimes it's hard), I like to split the refactor into one commit and then the actual interesting change into another commit. I believe this makes it easier for reviewers, because now you don't need to mentally separate "okay this change is just refactoring, so no behavior changes" and "okay this part is where the behavioral change is."

In the case where you can't easily attribute a fix to one commit, then absolutely, create a new commit! There aren't any hard rules here. It doesn't have to be perfect. I just personally don't want to live in a world where there are tons of "fix lint" commits scattered through the project's history. But of course, this is in tension with workflow. Because there are multiple ways to avoid "fix lint" commits. One of those is "squash & merge." But if you don't want to use "squash & merge" in a particular case, then the only way to avoid "fix lint" commits is to do fixups. And git-absorb will help you find the commits to create fixups for automatically. That's it.

> I’ve been using autofixup for this and it’s been ok but not great, it can be quite slow as things grown, and it doesn’t say anything when there was no match so it’s easy to miss. How does absorb surface that?

I haven't used autofixup, but:

* git-absorb has always been pretty snappy. I don't think it scales with repository size.

* If there's no match, then the things that don't match stay in the staging area and don't make it into a commit. git-absorb will also note this in its output after running it.

GitHub's quirks definitely make life much harder than it needs to be, but I've been using `git machete` for months now with great success in my team. The __one__ thing GitHub has that makes it all work is the fact that if you merge the parentmost branch, its immediate child will retarget its base branch.

I think if I had full "control" over my company's SCM workflows I would use a tool that considers a branch as a workspace and every commit in the branch becomes its own PR (personal preference, but in my experience it also motivates people to split changes more), but alas.

The term Stacked PRs already sounds like a term that was invented specifically in order to communicate in a GitHub-influenced context. Because Stacked PRs are just a reinvention of being able to review a commit at a time (the stack part is straightforward).

I assume you refer to https://github.com/torbiak/git-autofixup. I have also used it, and its ok but not perfect.

It's same kind of thing people tell me about ripgrep. "Why bother with ripgrep, grep has always been fast enough for me." That might well be true. Or maybe we value time differently. Or maybe none of your use cases involve searching more than 100K lines of code, in which case, the speed difference between GNU grep and ripgrep is likely imperceptible. Or maybe being faster unlocks different workflows. (The last one is my favorite. I think it's super common but little is written about it. Once something becomes fast enough, it often changes the way you interact with it in fundamental and powerful ways.)

Because I have git-absorb, I tend to be a bit more fearless when it comes to fixups. It works well enough that I can be pretty confident that it will take most of the tedium away. So in practice, maybe git-absorb means I wind up with fewer squashes and fewer "fix lint" commits because I don't want to deal with finding the right commit.

My use of git-absorb tends to be bursty. Usually when I'm trying to prepare a PR for review or something along those lines. It is especially useful when I have changes that I want to be fixed up into more than one distinct commit. git-absorb will just do it automatically for me. The manual approach is not just about finding the right commit. It also means I need to go through git-add in patch mode to select the right things to put into a fixup commit, and then select the other things for the other commits. So it actually winds up saving a fair bit of work in some cases.

Another example is renaming. Maybe PR review revealed the names of some functions were bad. Maybe I introduced two functions to be renamed in two distinct commits. I could do

first rename -> first commit -> second rename -> second commit

Or I could do:

both renames -> git add -p -> first commit -> second commit

Or I could do:

both renames -> git absorb

In practice, my workflow involves all three of these things. But that last git-absorb option eats into some of the first two options and makes it much nicer.

Can you give me an example of criticism that is not negative? As far as I know, all forms of criticisms involve pointing out a flaw or fault. There's constructive criticism, but it's still fundamentally negative.

Either way, feel free to replace the word "negative" with "criticism" in my comment if you want. It expresses the same thing I intended to express: I disagree with the criticism.

If we're not Pollyannas and we're all "adults who can handle criticism," then you should also be able to handle criticism of criticism. It goes both ways.

I'd be in favour of auto-stickying this. I see a lot of e-ink spilled over arguments that boil down to whether or not it's ok to comment about not liking some aspect of the subject under discussion. There are good reasons not to criticize in some situations, but I don't think they apply here. Either way the arguments are tiresome. We should agree to ban criticism, or agree not to argue about it (barring special circumstances).

I had to look up the reference, and based on the wikipedia plot summary at least, I admit I don't quite get the relevance. I expected a plot where someone handles criticism quite badly and suffers as a result, but in fact the plot was actually about someone who handled criticism very well instead, and improved the lives of others as a result?

So now I'm curious! In what way does Pollyanna relate to adults who can't handle critique? Have I got the wrong Pollyanna by any chance? xD

It won't matter until it does, but $@ expands arguments into separate words but those expansions are themselves only word-preserved if the $@ is itself quoted. The [ will probably get away with what you want, but its use in $(git add) and $(git last-sha) almost certainly will not

  $ cat > tmp.sh <<'FOO'
  for a in $@;   do echo "a=$a"; done
  echo
  for b in "$@"; do echo "b=$b"; done
  echo
  for c in $*;   do echo "c=$c"; done
  echo
  for d in "$*"; do echo "d=$d"; done
  echo
  FOO

  $ bash tmp.sh 'alpha beta' $'charlie\ndelta'
  a=alpha
  a=beta
  a=charlie
  a=delta

  b=alpha beta
  b=charlie
  delta

  c=alpha
  c=beta
  c=charlie
  c=delta

  d=alpha beta charlie
  delta

Yeah, it's definitely less powerful that what absorb is doing. I wasn't trying to argue that it was equivalent. I just wanted to share a bash one-liner that I've had success with in case others find it helpful.

> What if I want some parts of it into one commit and another parts into another?

Looks like absorb will automatically break out every hunk into a separate fixup commit. My one-liner will create 1 fixup commit for everything that's staged. That's typically what I need, but on the occasions it's not, I use `git add -p`, as kadoban mentioned, to stage exactly what I want for each commit.

Then you use `git gui`, which is part of the git distribution itself, or `tig` if TUIs are your thing. I have a key binding for `git commit --squash=%(commit)` in my tig config, so I can interactively select lines or hunks to stage and then the target commit for the squash.

That sounds like what `git add -p` is for, stage part of the current changes.

Might be able to use the multimode flag in the fzf command above and it should let you select more than one using Tab and Shift+Tab.

Your alias seems like a completely unecessary complexity. If you want to meld new changes into your branch head you can just alias “git commit --amend”, you don’t need that mess.

Absorb will find the commits to fix up for each change in the working copy, it doesn’t just merge everything into the head.

I guess the crate version is easier to soft reset?

This was 100% my inspiration. I used emacs+magit for years. After switching away from emacs for dev work, I still cracked it open for git interactions for another year or so. Eventually, I moved entirely to the shell with a bunch of aliases to replicate my magit workflow.

Worse in fact, since magit lets you fixup, squash, or instafix.

I rarely refer to commits by their IDs in these cases, though. More often than not I use the relative `HEAD~n`, when it's easy to determine `n` visually. With a few aliases, the manual fixup workflow really doesn't take much effort.

I love lazygit so much. I use it all the time, and it's constantly improving my workflow.

lazygit also my go-to for this and git in general

This sounds just right.

That's exactly what it does by default.

Only if you pass --and-rebase does it actually do the autosquash rebase for you.

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.

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

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

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

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.

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.

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

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

It solves a problem that can be solved better by good habits and a solid understanding of git.

Yeah, it singlehandedly turned me on to Mercurial when I was there, and has massively shaped the way I use git ever since.

Nit: Or @~2 (@ is HEAD, so @^^ is HEAD^^).

> At the risk of sounding judgemental, I think this preference for always squashing PRs comes from a place of either not understanding atomic commits, not caring about the benefits of them, or just choosing to be lazy. In any case, the loss of history inevitably comes at a cost of making reverting and cherry-picking changes more difficult later, as well as losing the context of why a change was made.

1) Why are you ever reverting/cherry-picking at a more granular level than an entire PR anyway? The PR is the thing that gets signed-off on, and the thing that goes through the CI build/tests, so why wouldn't that be the thing kept as an atomic unit?

2) I don't think I've ever cared about the context for a specific commit within a PR once the PR has been merged. What kind of information do you expect to get out of it?

Edit: How does it remove the context for a change or make `git bisect` useless? How big are your PRs that you can't get enough context from finding the PR commit to know why a particular change was made?

Since we're taking the risk of being judgmental, I'd suggest the preference for not squashing branches to preserve the developer's local commit history when merged into the main line suggests a misunderstanding between branches and commits, but if it works for you more power to you.

What are your thoughts on the "ship, show, ask" workflow? [1]

In that workflow, small stuff is simply pushed, which allows PRs to be more single focused and more atomic. Perhaps your only objection is direct pushes to master? I am really curious if that workflow otherwise addresses all of the downsides you stated while still allowing for all PRs to be uniformly rebase-squash merged.

[1] https://martinfowler.com/articles/ship-show-ask.html

git bisect is still perfectly useful. It will locate the squashed commit/pr which is more than enough for me to zero in on whatever the bug is.

If you go all in on atomic commits, you're probably going to be rebasing a lot. IME, that's a lot of time spent trying to make a "clean" history all for the benefit of looking back infrequently at what broke.

Can't you just inspect your merged branches and bisect those instead if using the squash strat?

There can be another reason. I do care about the benefits, but I don't think there really are any.

People say what they mean.

“Atomic” sometimes means conceptually: does one thing. Sometimes it means “builds”. Or “works”. And yes: they do mean every single commit builds. Not just on main.

You don’t have to put words in people’s mouth.

Yes, atomic commits is not a very descriptive term. But main/master is stable is only a necessary condition, not a sufficient one.

If you squash (or mix from the beginning) several unrelated changes into a single commit, main/master would be stable.

AFAIK atomic commits means nothing can be taken away without breaking the change and nothing needs to be added to make the change work. How to express that without 2 clauses is indeed a good question.

It has always been enough for me to trace a change back to its original pull request. Extra granularity might be nice sometimes but never essential. And I do a fair bit of digging through git history. Not sure what makes it "not an ok practice". Squashing works fine and lets the 80% of people who don't care spend their efforts thinking about something other than a perfect git history.

For what it's worth, you're right.

git log --first-parent

That's why you squash and make the commit message readable.

I have used standard github and graphite reviews. I tend to prefer what I mentioned in my original post than graphite stacked PR review (which are essentially atomic commits)

Yes I also advocate for squash-before-merge, so a lot of little commits is doesn't show up in the main history.

> For me personally, the sweet spot is currently a mix of stacked-commits and the PR workflow: Use a single commit as the unit of review, and polish that commit using a PR, and use the commit descriptions within that PR to tell the story of you responding to feedback. Then, squash merge that commit.

To me time spent on commit polishing (and dealing with conflicts) is time not spent on product. Author comments on PR review, squash-before-merge, and sit-together with reviewer for big PRs to me seems a better compromise. I don't think super polished git history is worth the extra effort for most types of product, as long as I can track a change down to a PR discussion that is enough to me. From there I can track PR review commit changes individually if needed.

Like it is so uncommon for me to go digging on git history that deeply, usually all I care is "code behaving weird && line changed < 1 month ago then probably a bug introduced then"

Of course if you are working on aviation software and the like maybe the priorities are different. But I have spent way too much time dealing with rebase conflicts when trying to chop up my PRs into smaller commits. Dealing with these conflicts often introduces bugs too.

Why are people talking about stacked PRs/MRs? Shouldn't they be called queued? A stack is LIFO and a queue is FIFO.

(Of course in some special case you might want to merge a later one earlier, but I don't think that's the normal case people are talking about.)