Git-absorb: Git commit –fixup, but automatic
github.com
github.com
It seems like some would reply and say PRs should just be one commit. Or that they will be squashed anyway. And sure, that is sometimes the case. But not always. I tend to prefer logically small commits when possible, and it's not always practical to break them up across multiple PRs. Perhaps partially due to how GitHub works.
I use this workflow on all of my projects on GitHub.
> Perhaps partially due to how GitHub works.
That’s definitely a major factor, I’d like to use stacked PRs they sound really neat, but GitHub.
Also even with stacked PRs I figure sometimes you’re at the top of the stack and you change things which affect multiple commits / prs anyway, surely in that case you need to fixup other commits than the ToS?
> it doesn’t say anything when there was no match
that's what it should do
> it can be quite slow as things grown
How? All the slowness (on large repos) I've seen has been fixed.
No it is not.
> How?
I don’t know, that’s just an observation from using it, semi regularly I autofixup changes and it takes a while to do anything.
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.
It has nothing to do with github.
Which is a GitHub-centric perspective.
That additional layer allows finer grained history and to mostly avoid (unreviewed) rebasing. Many teams find those properties valuable.
I guess another pet peeve for me.
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.
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
There is a notion of a “Pollyanna mode” in schematherapy. What it means is ignoring negative facts and challenges with an outwardly positive attitude and avoiding addressing the issues themselves.
This certainly can be harmful to oneself. Another harmful thing is hating and bashing oneself for mistakes and faults and I won’t make a comparative judgement, but a healthy way is supposed to be along the lines of speaking up openly about what bothers you and thinking what can be done about it if at all.
The plot summary of the book is likely not what you should be reading as it's become an idiom. Something like Wiktionary or another dictionary would be a better place to look it up.
In this case, it's not about being able to receive criticism, but about being reticent about _giving_ it.
This is a good and perhaps under-appreciated point. When I first read the term "Polyanna" I made the same mistake as GP. I think if you read "The Prince" to find out what "Machiavellian" meant you'd be no better than when you started. Even terms like "Kafkaesque" have taken on lives of their own and are probably better not thought of as mere literary references.
Machiavelli's "Discourses" is the one that will really confuse a reader looking to understand the colloquial meaning of "Machiavellian". In this book, Machiavelli lays out a vision of a healthy "republic" (or more precisely, res publica) which benefits the people who live in it. Among other things, Machiavelli argues that republics actually benefit from multiple competing factions, and from some kind of checks and balances. Apparently these ideas influenced several of the people who helped draft the Constitution of the United States.
Now why Machiavelli had two such different books on how governments worked is another interesting question...
> adjective
> uk /ˌmæk.i.əˈvel.i.ən/ us /ˌmæk.i.əˈvel.i.ən/
> using clever but often dishonest methods that deceive people so that you can win power or control
(from https://dictionary.cambridge.org/dictionary/english/machiave... )
Ymmv, but I think that's far from the point of the book, and isn't even the main topic. It's hard for me to imagine taking a person who'd never heard the term, letting them read the book, and then asking them to propose a definition, would produce anything like the above.
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.
But the interesting thing about this is "the eye of the beholder". I suppose to some folks, all criticism does seem to be negative... which is why code reviews can turn into nightmares... and helps explain why I recently received a very angry phone call from another engineer because of some changes that were made to "his code".
For the purposes of this discussion, the oddity seems to be the folks jumping to defend the software they had no part in creating. Why does the criticism result in negative feelings for them? I can understand the author taking issue with it, but someone else's criticism should not impact another's ability to utilize the software for whatever purposes.
1. There were many comments providing criticism that I disagreed with.
2. I felt that the vibe given by the comments at the time was not commensurate with how much utility the tool brought me in my own workflow.
So it seemed useful to point this out. That is, that there was a gap in the commentary that I thought I could fill.
Obviously I won't be using this phrasing again because it spawned off a huge meta sub-thread that I think is generally a huge waste of time and space.
> For the purposes of this discussion, the oddity seems to be the folks jumping to defend the software they had no part in creating. Why does the criticism result in negative feelings for them? I can understand the author taking issue with it, but someone else's criticism should not impact another's ability to utilize the software for whatever purposes.
Again, it cuts both ways. Why isn't it odd for folks to jump in and express criticism of software they had no part in creating? If folks can express negative criticism, then why can't I express positive feedback?
I didn't say people shouldn't make those comments. I said it was unwarranted. Or in other words, that it was undeserved and that I disagreed with it. Or that some balance was appropriate. This has literally nothing to do with my ability to utilize the software. I don't know why you're going down that road.
Besides, consider this negative comment:
https://news.ycombinator.com/item?id=41653797
I even already said I would use different wording next time.
And I never said folks shouldn't make those comments. I clarified that too.
I share the sentiment about the "logical small commits", and hence I don't see that adding a new fix commit is problematic as long as it is self-contained and purposeful, but perhaps I don't understand what is the problem that this tool is trying to solve.
It says
> You have fixes for the bugs, but you don't want to shove them all into an opaque commit that says fixes
So my understanding is that bugs were found in the PR and now you want to fix those bugs by not introducing separate "Fix A", "Fix B" and "Fix C" commits but you want to rewrite the history of existing N commits into N' commits so that it blends those A, B and C fixes in.
Maybe I can see this being somewhat useful only ever if those N commits are pushed either directly as series as patches to the main branch or as a single merge commit, and you want to keep the S/N ratio reasonable.
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.
Say I have an open MR from a branch that I’ve pushed three commits to so far. I pushed these three commits individually and the pipeline ran green each time.
A coworker of mine points out some errors to me.
I have to touch files that I previously touched in the past three commits again.
I am tempted to commit these changes in one commit. But I decide to try git absorb instead.
So instead of adding one fourth, green commit to my MR, my use of git absorb rewrites all three of my previous commits.
But actually, the changes I was about to put in the fourth commit only work when taken together.
Splitting them up and rewriting the previous three commits will result in build failure if you try to build from the new first commit or the new second commit.
I don’t notice that because I’m on the new third commit.
I force push the three new commits to my branch for my MR. Gitlab runs the pipeline on the last of the commits. Everything looks fine to me.
Your team lead approves the MR and pushes the merge button.
Three months later he scolds me for a broken bisect.
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.
I suppose a purist might say you should go clean up the history so that you have no failing commits, but I don't know anyone who is a true purist. You don't have to be.
Instead of living in a word of black-or-white, join me in embracing the grey. I don't treat commit history as something that must be perfect. I just try to treat history like I treat the source code itself: I write it for other humans. Is the history always perfectly comprehensible? Nope. Just like the source code.
I guess at the end it's all weighted tradeoffs for me too. I just put less weight on legibility and more on the ability to work on branches co-operatively without force-pushing.
And this is also why workflow questions are hard. Because "working on branches co-operatively" was previously unstated. It's not universal. I rarely work on branches co-operatively in a long term sort of way. Sometimes I might "take over" a branch to get it merged or similar, but rewriting history in that context is still fine.
It is always the case, even among so-called purists, that you don't rewrite history that you're sharing with others. (Unless you've both agreed to it. But even then, it better be short-lived because it's annoying.)
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.
Do you advocate for making code easier to read and understand by humans? What would you do if someone told you, "no I don't want to waste my time trying to make it perfect." It's the same misunderstanding. I try to treat code history like I treat code: I do my best to make it comprehensible to humans. And yes, sometimes multiple PRs is better. But sometimes multiple commits in one PR are better. I use both strategies.
If the resulting squash is too big / touches too many things, it’s because I didn’t split the MR where I probably should have.
Because to me, the bigger waste of time is fiddling with all of the commits in the MR just to make it so that the MR can be merged without squashing.
If someone needs fine-grained history about how exactly a specific feature or bug fix came to be, they can always go to the MR itself and look at what happened there.
Sometimes I do that. But sometimes I want the history I've curated in my PR to be preserved without the overhead of creating separate PRs (which then often need to be stacked, introducing problems of its own). In which case, I avoid things like "fixup lint" commits. And that's where git-absorb shines. And saving time is exactly the point! git-absorb helps you curate history faster.
> If someone needs fine-grained history about how exactly a specific feature or bug fix came to be, they can always go to the MR itself and look at what happened there.
That's a lot more work than just reading the commit log. But I agree, one can do this ifneedbe. But it's nice to avoid when possible.
Yeah, that’s true. I suppose that we may be working in somewhat different environments.
For example, when someone has big public projects that get a lot of eyeballs on them, like your ripgrep and other projects, it makes a lot of sense to spend extra time making the git log a thing that can be read on its own completely offline.
For the things I work on at my job, there are just a few of us writing the code itself and those coworkers that work on the repos I do will usually check every MR from everyone else. And anyone else in the company working on other repos is usually likely to browse code via the GitLab anyway, if they are interested in actually looking at any of the code that are in the repos of my team. Onboarding new coworkers on the team is also mostly centered around the docs we have, and talking with other people in the team and the company, rather than digging through the git history.
And for me when I am looking to understand some code from our internal repos it’s usually that way too, that I use the GitLab UI a lot, and I look at the MRs that were merged, and I check the associated Jira ticket, and I might ask some coworkers on Slack to explain something.
Most of the time, no one outside of our team is very interested in the code of our repos at all. Discussions revolve around other aspects like the APIs we expose, and the schema of the data we store in tables in the databases, and the Kafka events we produce. That sort of thing.
And everyone at my job will necessarily need to be online when they are working anyways, so that they can use Slack etc. So we are one click away from our GitLab instance when we work. On the off-chance that someone is trying to do some work offline while being on the go and without Internet access available, they probably have some specific kind of work in mind that is sufficiently independent of specific details that they can do without that history.
FWIW, at my previous role, where I worked on an internal proprietary codebase, we all followed this same philosophy of curating commits. It was primarily to facilitate code review though, and not source history. Stacking PRs on GitHub is really truly annoying in my experience. (Although there is some tooling out there that purports to improve the experience.)
Now, if you find linting issues in the upstream and fix those, then a `Fix linting issues` commit is perfectly fine.
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.
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.
Are people fixup'ing a lot more than I do? I might do it once or twice per MR and it's never a large burden to fix the right commit.
If things get really out of hand such that the whole thing is a mess I just squash. Whatever history I had before is almost by definition gross and wrong in this scenario, and I don't mind losing it.
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.
> gfx='git commit --fixup $(git log $(git merge-base main HEAD)..HEAD --oneline| fzf| cut -d" " -f1)'
It shows you the commits on the current branch and lets you select one via fzf. It then creates the fixup commit based on the commit you selected.
> 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.
Once you mentioned `git add -p` I realised that this is pretty much what I do already, except with a far more efficient way of selecting the relevant commit to do it to.
Muchas gracias.
alias git_main_branch='git rev-parse --abbrev-ref origin/HEAD | cut -d/ -f2'
alias gapa='git add --patch'
alias grbm='git rebase -i --autosquash $(git_main_branch)'
alias gfx='git commit --fixup $(git log $(git_main_branch)..HEAD --oneline| fzf| cut -d" " -f1)'
alias gmru="git for-each-ref --sort=-committerdate --count=50 refs/heads/ --format='%(HEAD) %(refname:short) | %(committerdate:relative) | %(contents:subject)'| fzf | sed -e 's/^[^[[:alnum:]]]*[[:space:]]*//' | cut -d' ' -f1| xargs -I _ git checkout _"
alias gfixup="git commit -v --fixup HEAD && GIT_SEQUENCE_EDITOR=touch git rebase -i --stat --autosquash --autostash HEAD~2"
From what I understand it does the same thing as this crate for the most part. All I do after is:
git push —force-with-lease
Not sure what you get from the crate otherwise
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 read the rough how it works and it now makes sense. I might give it a try. Thanks!
(Edit: typo)
# make a fixup commit for the last time the file was modified
cff = "!f() { [ -n $@ ] && git add $@ && git commit --fixup $(git last-sha $@); }; f"
# Get latest sha for file(s)
last-sha = log -n1 --pretty=format:%h --grep 'fixup!' --invert-grep
$ 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 fzfCommit() {
local FZF_PROMPT="${FZF_PROMPT:=Commit: }"
git log --oneline | fzf --border --prompt="$FZF_PROMPT" --height=10 --preview="git show {+1} --color=always" --no-sort --reverse | cut -d' ' -f1 | tr '\n' ' ' | sed 's/[[:space:]]$//';
}
function gfixup {
local commit=$(FZF_PROMPT='Fixup Commit: ' fzfCommit)
if [[ -z "$commit" ]]; then
return 1
fi
set -x
git commit --fixup "$commit" --allow-empty > /dev/null || return 1
git rebase --interactive "$commit"~ --autosquash || return 1
}I prefer having full control over my commit history, and this tool is too much magic for my taste. I'm sure that it could be improved so that mistakes are rare, but I'm not sure I would trust it enough to not have to review its work anyway.
Only if you pass --and-rebase does it actually do the autosquash rebase for you.
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.
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.
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.
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.
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.
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)
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.
Logical change 1, implement Hello World:
puts "Hello, World!"
def hello_world
puts "Hello, World"
end
def hello_world(greeting)
puts "#{greeting}, World"
end
def hello_world(greeting, recipient)
puts "#{greeting}, #{recipient}"
end
def greet_recipient(greeting, recipient)
puts "#{greeting}, #{recipient}"
end
def greet_recipient(greeting="Hello", recipient)
puts "#{greeting}, #{recipient}"
end
def greet_recipient(greeting="Hell", recipient)
puts "#{greeting}, #{recipient}"
end
def greet_recipient(greeting="Hello", recipient)
puts "#{greeting}, #{recipient}"
end
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.
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.
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.
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 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.
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.
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.
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.
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?
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.
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.
"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.
- 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.
And one key to good git history is people splitting up big PRs in multiple commits.
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.)
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.
Yeah exactly.
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.
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.
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.
> 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?
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.
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 :)
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.
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.
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.
https://jordanelver.co.uk/blog/2020/06/04/fixing-commits-wit...
git-absorb appears to take it one level further. However from the README I'm not clear on exactly what it will do in specific situations:
Does git-absorb automatically associate the most recent commit unique to a branch for a given file and apply the diff to said commit? Or what is the precise workflow and outcome in terms of which edits go into which commits?
Edamagit for vscode users is not as good but it does this particular workflow great.
fixup is "stage changes, users selects past commit to fixup, commit staged changes"
absorb is "stage changes, git-absorb figures out which past commit to fixup, commit staged changes"
Interactive git rebase is by far my favorite Git tool to use, it scratches a particular itch to create perfect logically atomic commits.
That said, sometimes this kind of history editing tends to backfire spectacularly because these crafted perfect commits have actually never been compiled and tested.
Some people find doing so this boring and annoying. For whatever reason my brain finds it zen and satisfying.
I also frequently try to find when some bug or change was introduced in the get history, so that I can understand why it was done through the context. Context. When it is just thrown in some random commit that doesn't even have much of a commit message, it is utterly useless. When it is part of a commit that is atomic and targeted at one thing, it is trivial to see why it was done. Even when the comment isn't super helpful (which is common despite intentions, because predicting the future and what will be useful in the future is very difficult) it's still valuable because you can see the code in its full context and it's often clear what it's doing.
So yes, I am a big believer in clean commit history with atomic, single commits that represent the task someone was doing. For example, If it was a bug fix, then the bug fix will be in its own commit with a commit message describing the bug that is being fixed.
The answer is: some do, some don't.
I wouldn't say I put the effort in to get to 100% clean but something like 95% clean makes future me significantly less enraged at past me's mistakes.
As far as I remember, I've had to check history for about 2/3 of bugs I've fixed.
In a perfect world each system would be documented and tested well enough that correctness could be derived from first principles, but we do not live in a perfect world :(
If I'm doing a `git blame` or digging through the annotate gutter in GitHub I prefer to find the appropriate commit context, not a commit with title "typo" or "oops" or "trying something" or "WIP".
In the latter case you can still do the `git log` archaeology to find the actual context but I'd rather not.
Some teams will squash all merges to avoid this problem, but why not have the best of both worlds by merging relatively clean branches?
The need for code archeology depends on a project. When you writing a lot of new code it's probably less important than in a legacy thing where most changes are convoluted tweaks made for non-obvious reasons.
All I see is a tool that guesses where to aggregate a fixup commits when I can't be bothered to view and think about that myself.
I can trivialize useful software too. See how ridiculous you sound? That's what software does! It makes our job easier.
It's true that git-absorb guesses, but since you clearly haven't used the tool, you don't know how good it is at guessing. Moreover, false positives and false negatives are not the same in this scenario. A false positive would be very annoying, and I don't think that's ever happened for me in the years I've been using git-absorb. False negatives happen more frequently, but it's fine, because it tells you and then you just fall back to what you would have done manually for whatever it couldn't find a commit for.
Going back in time to fix the past just to sort your changes into the "correct" bin or vicinity of changes just seems unnecessarily complex. I am also unaware of what use-cases this supports. Are teams out there forcing commits to have a stricter scope than just "anything in the codebase"? Perhaps it's a monorepo pattern? I have no idea.
Either way, you have to `git push --force` to modify your PR/MR. May as well use the stock workflow and learn how to clean up your branch.
I also endorse `git commit --amend` for anyone that identifies as a "micro-committer" but doesn't need to preserve intermediate changes.
Edit: re-reading comments - it sounds sort of like some teams prefer to bin changes like this in order to make a PR/MR browsable by commit, rather than inspect one big broad diff. Is that the case?
There've been plenty of times I haven't done a fixup because I couldn't be arsed to check exactly which of the three ancestor commits in my branch the tweak actually belongs with. I wouldn't even consider splitting into three fixups if there were three tweaks that belonged to three different ancestors. It sounds like this tool/workflow will do all that work with a single invocation.
I fixup^ frequently, often often with conflict to resolve in the process, and I have never ever thought 'if only something would automatically choose the target commit for me', even if it was advanced AI why would I trust it to be what I wanted?
^my alias is:
!f(){ target="$(test -n "$1" && git rev-parse "$1" || git fzsha rev-parse)"; git commit --fixup="$target" ${@:2} && EDITOR=true git rebase -i --autostash --autosquash "$target^"; }; f
In our projects we only enable squash merge in GitHub and the PRs can have any commits you want. The squashed commit includes link to PR, and PR has detailed summary (which wouldn’t be practical in the commit message).
Developers can be as f'n messy as they want on their branches, no one cares because it never hits main.
My history is read much more than it is checked out. And definitely much more than it is stress-tested by some CI. Readability matters more.
I’m having a hard time imagining all the supposed “commit for coffee break” committers who also are die-hard bisect users that go nuts when someone doesn’t run the whole CI on every commit.[1]
And if you don’t have that many commits to check because you “squash merge”: `git bisect start --first-parent` gives you exactly the same result.
Has anyone compared the two?
I definitely want to have a "fixes" commit on my feature branch. You should do whatever you want on a feature branch so long as your trunk has a clean history.
This sounds like someone wanted to lift a feature of changesets in mercurial into git. I don't think this is safe and probably breaks a lot of people's mental model of git changelogs being an immutable data structure.
This tool creates `fixup!` commits. These commits aren’t rewrite commits. They are commits which later can be used to rewrite the history.
You can use this tool, not rewrite your branch, and then rewrite the branch right before merge.
> This sounds like someone wanted to lift a feature of changesets in mercurial into git. I don't think this is safe and probably breaks a lot of people's mental model of git changelogs being an immutable data structure.
Rewriting is a huge part of the Git culture (and well-supported). Meanwhile Mercurial seems to have rewriting as a sort of add-on.
> The command essentially looks at the lines that were modified, finds a changeset modifying those lines, and amends that changeset to include your uncommitted changes.
also you can write HEAD~2 as @^^ if you want to save a couple keystrokes!
btw you could be doing `git commit --amend` if you just want to add to the last commit.
Sure I do. The whole branch will be squashed anyway before it's merged in, and a single "fixes" commit while still on its own branch will be easier to track in a PR for addressing everything pointed out earlier.
I mean, don't let me stop anyone from using this or --fixup if this is your flow, but this solves a problem neither I nor anyone in my last 10 years of working with code has.
I also disagree with the framing: some policy can always be enforced if you have some kind of authority over the other person. Now we’re veering away from arguing on the technical merits.
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, making `git bisect` pretty much worthless, and losing the context of why a change was made.
This workflow really shines when it points you to the actual atomic commit that introduced the issue. Then you're simply a `git revert` away from undoing it, without risking breaking anything else, barring functional conflicts, of course. Try doing that with a squashed commit.
> Then you're simply a `git revert` away from undoing it, without risking breaking anything else
This needs careful qualification. On GitHub at least, it is difficult to ensure every commit passes CI. This can result in skipping during bisect for a busted commit. It doesn't happen often enough in my experience too convince me to give up a cleaner history, but it's a downside we should acknowledge.
Ideally GitHub would make testing each individual commit easier.
In either case the brand new revert could break something. Who knows, it’s a new state.
> It doesn't happen often enough in my experience too convince me to give up a cleaner history, but it's a downside we should acknowledge.
There are tools for that.
I understand there are tools for testing each individual commit. You'll notice that I didn't say it's impossible to ensure every commit is tested. Needing to use random tools to do it is exactly what makes it difficult. And the tool you linked says literally nothing about using it in CI. How good is its CI integration? There's more to it than just running it. On top of all of that, there are more fundamental problems, like increasing CI times/costs dramatically for PRs split into multiple fine grained commits.
Again, anyone here can go look at my projects on GitHub. I try hard to follow atomic commits. I think it's worth doing, even with the downsides. But we shouldn't try to make things look rosier than they actually are.
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?
Because it often isn't. I don't know about your experience, but in all the teams I've worked in throughout my career the discipline to keep PRs atomic is almost never maintained, and sometimes just doesn't make sense. Sometimes you start working on a change, but spot an issue that is either too trivial to go through the PR/review process, or closely related to the work you started but worthy of a separate commit. Other times large PRs are unavoidable, especially for refactorings, where you want to propose a larger change but the history of the progress is valuable.
I find conventional commits helpful when deciding what makes an atomic change. By forcing a commit to be of a single type (feature, fix, refactor, etc.) it's easier to determine what belongs together and what not. But a PR can contain different commit types with related changes, and squashing them all when merging doesn't make the PR itself atomic.
> 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?
Oh, plenty. For one, when looking at `git blame` to determine why a change was made, I hope to find this information in the commit message. This is what commit messages are for anyway. If all commits have this information, following the history of a set of changes becomes much easier. This is helpful not just during code reviews, but after the merge as well, for any new members of the team trying to understand the codebase, or even the author themself in the future.
In my experience at least, PRs are atomic in that they always leave main in a "good state" (where good is pretty loosely defined as 'the tests had to pass once').
Sometimes you might make a few small changes in a PR, but they still go through a review. If they're too big, you might ask someone to split it out into two PRs.
Obviously special cases exist for things like large refactoring, but those should be rare and can be handled on a case by case basis.
But regardless, even if a PR has multiple small changes, I wouldn't revert or cherry-pick just part of it. Just do the whole thing or not at all.
> Oh, plenty. For one, when looking at `git blame` to determine why a change was made, I hope to find this information in the commit message. This is what commit messages are for anyway. If all commits have this information, following the history of a set of changes becomes much easier. This is helpful not just during code reviews, but after the merge as well, for any new members of the team trying to understand the codebase, or even the author themself in the future.
Yeah but the context for `git blame` is still there when doing a squash merge, and the commit message should still be relevant and useful.
My point isn't that history isn't useful, it's that the specific individual commits that make up a PR don't provide more useful context than the combined PR commit itself does.
I don't need to know that a typo was fixed in iteration 5 of feedback in the PR that was introduced in iteration 3. It's not relevant once the PR is merged.
> I don't need to know that a typo was fixed in iteration 5 of feedback in the PR that was introduced in iteration 3. It's not relevant once the PR is merged.
I agree, these commits should never exist beyond the PR. But I go back to my point about atomic commits being misunderstood. It's not about keeping the history of _all_ changes made, but about keeping history of the most relevant ones in order to make future workflows easier. A few months from now you likely won't care about a typo fix, but you will care about _why_ some change was made, which is what good commits should answer in their commit message. Deciding what "atomic" truly means is often arbitrary, but I've found that making more granular commits is much more helpful than having fewer and larger commits. The same argument applies to the scope and size of PRs as well.
I already know if I’m doing a fix, a refactor, a “chore” etc. Conventional commits just happen to be the ugliest way you can express those “types” in what looks like English.
But I've worked with many, many developers who don't strictly separate commits by type this way. I myself am tempted to do a fix in the same commit as a refactor many times. Conventional commits simply suggest, well, a convention for how to make this separation cleaner and more explicit, so that the intent can be communicated better within a team. I've found this helpful as a guide for making atomic changes. Whether or not you write your commit messages in a certain way is beside the point. But let me know if you come up with a prettier way to communicate this in a team.
The PR could contain three refactoring commits and one whitespace cleanup commit. One main change. The only thing that breaks anything is the main change. Because those others were no-ops as far as the code is concerned.
Can't you just inspect your merged branches and bisect those instead if using the squash strat?
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.
I'm not a fan of this. It allows contributors to abandon any established coding standards, while placing more burden on maintainers to fix issues later. This in practice never happens, so the quality of the codebase degrades over time. Not to mention that it allows malicious actors to merge changes much more easily.
As for "ship, show, ask" specifically, I have similar reservations. I think all changes should go through a review process, even if someone just glances at the changes. It not only gives the opportunity to leave feedback, but also serves as communication so that everyone is aware of the proposed changes. Also, making the decision of whether to choose "ship", "show" or "ask" might only work for senior and well disciplined developers. I think that in most teams you have a mixture of experience, so you'd inevitably end up in situations where a change should've been "show" or "ask", but was "ship", and viceversa. I don't think teams will ever align on a single strategy to make a correct decision, since it's based on subjective opinion to begin with. Always following the same workflow gets rid of these uncertainties.
First, I assume a few constraints on "ship, show, ask." First constraint is that all changes go for at least one, if not multiple self reviews. Second, the ability to ship is not outright granted. Until someone has demonstrated they can ship reasonable, working code- they are not given write permissions. They must "ask."
Next, ship is not a given. Things that are not interesting are shipped. This enhances the communication part of PRs. If a PR is sent, it is important or wants extra review. That removes the noise of everything being a PR, and removes the noise from PRs where minor changes are smuggled in for the sake of efficiency. This also removes a lot of nitpicking as well. When asked to CR, we often feel we need to CR, and if our only feedback are minor things- then that is the feedback. 'Perfect' becomes the enemy of good enough. Saving time, efficiency are huge. Do CR when it is useful, and only then.
Last, post merge reviews can still be done by the maintainers to ensure things are going smoothly. Hence, post merge review is an option for both ship and show. This means the coding lifecycle is blocked in CR only when the CR is valuable.
That CR is blocking I feel is a very important drawback. Typos linger because time is finite and only those that do not appreciate that will try to fix everything. The team then gets bogged down spending too much time fixing small things. Leaving the small things is not ideal either. If the overhead to fix small things is tiny, suddenly it takes 5 minutes to do what would otherwise require another person and possibly the next day.
For example, A PR preceded by renaming a few files is powerful. That is a game changer compared to a hard to review CR, staged PRs, or simply never doing the renaming because it runs the diff.
Blanket CR can also ossify a project. Most projects I've seen are written quickly and in large chunks. If they are reviewed at all, it is a lot of 2000 line "LGTM", zero comment reviews. Then when careful CR is done (often by the "scaling" team), suddenly things that are original become gospel. The CR debates the absolute best naming, original intent, spending magnitudes more time than was originally put into the code in the first place. Meanwhile the code was done quickly, slightly worse than good enough- and now good enough is not even good enough. The CR now demands a far higher quality bar compared to the original code when it was written. Fixing a 50k line code base at 500 lines per day is a way to _not_ fix that codebase. The original bad code is therefor fossilized into the project. Time and effort are not infinite.
That turned into a bit of a rant. I wanted to demonstrate that:
(A) there are guard rails on SSA so it is not just a free for all.
(B) selectively requesting CR when it is valuable amplifies the value of CR.
(C) removing the noise of everything is a PR makes PRs stronger as communication tools.
(D) post merge review of all code can still meet the goal of reviewing everything.
I'll tie this back to git absorb now. Git absorb rebases, which arguably invalidates any CR that was done. Ship-Show-Ask allows for there to be more explicit trust where someone does their git absorb, redoes the self review and then merges. Further, the workflow helps clean up PRs to not be as many commits.
Though, I do think the discussion of workflows and CR is really fascinating. Having been essentially just the one senior, or one of two on most all projects I've worked on in the last decade, - I'm certainly (overly) eager to discuss in detail the workflows.
I do think there's no right or wrong answer with these things. Everyone will have different opinions about their ideal workflows, especially senior engineers. The important thing is to find a good balance that everyone on the team is satisfied with, but there will always be concessions, as in any relationship.
What I have found, though, is that the more uncertainties a workflow has, the higher the chances of misunderstanding, or someone not following it in the way someone else thinks they should. So with the way "ship, show, ask" is described, and what you mention here, is that there are many constraints, and decisions to be made about what constitutes an "interesting" change, or what is worthy to be shipped without a CR and what isn't. If the team is mostly senior, and there's a lot of trust involved, this workflow might work, and it might reduce the friction of blocking CRs. But there are a lot of variables involved there.
OTOH, by always requiring CRs and approvals for shipping, there are far less decisions to be made. The workflow is simple, fixed and known to everyone. Sure, sometimes CRs step into nitpicky territory, but this is usually solved by having some conventions, such as making clear what is a nitpick, what is a minor/major suggestion, and what is a blocker. This way nitpicks and minor suggestions can be safely ignored (as long as they're ackgnowledged in some way), and they're not blockers for merging.
In practice I've found that while this workflow does take considerable time and effort, in the vast majority of cases it leads to higher quality code. Even addressing nitpicks and typo fixes is an improvement in most cases. These can be easily automated by suggestions in GitHub, which can be batched and committed directly from the web UI, so it doesn't take much effort at all.
> Last, post merge reviews can still be done by the maintainers to ensure things are going smoothly.
Sure, but is this diligently done? And how well does it scale if the project has many contributors, but few maintainers? Placing the burden on maintainers to ensure certain quality standards is unrealistic. Moreover, it assumes that maintainers have the final say in these matters, instead of their changes also being worthy of discussion, which happens during a CR. I think that the responsibility for ensuring things are running smoothly should be shared by every member of the team, including external contributors.
> Most projects I've seen are written quickly and in large chunks. If they are reviewed at all, it is a lot of 2000 line "LGTM", zero comment reviews.
Those are examples of not doing CRs correctly, not a testament that CRs are not worth the time and effort. You can just as well run into this issue with the SSA workflow. As with any issue with team dynamics, this should be resolved by communicating and aligning on core values and practices the team finds important.
> The CR debates the absolute best naming, original intent, spending magnitudes more time than was originally put into the code in the first place.
Yes, this is to be expected. It takes much more effort to read and understand code than it does to write it. It takes additional time and effort to leave a written comprehensible review, and communicate all of this asynchronously. This scales linearly for every member in the team, and there's no way around it. But the question is whether deciding to invest the time and effort in a well established CR process ultimately results in higher quality software being shipped. In my experience, it does, and it's worth it.
That said, CRs are not the only or, even failproof, method of quality assurance. As mentioned in the SSA article, pair programming is another method, but it has its own drawbacks and limitations. All of these things help, and are best used in combination. They can be uncomfortable, tedious and seem pointless at times, but deciding to not do them because of this is doing a disservice to the final product.
FWIW, I have not seen much trouble in the ambiguity. Things like "move method" refactor, typo fix, add additional test cases, comment clarification, are just shipped. If the update makes an important clarification on a comment that the team should know about, show is interesting. If the code fixes a bug, would be mentioned in a stand up- show is good. Ask is good for a large refactor where there is room for error, when something does not seem clean, merits any type of double check or second opinion, or and especially when it is a first foray into a different part of the codebase.
> Sure, but is (post merge reviews) diligently done?
The beauty I would say is that it does not have to be diligently done. Feature, not a bug.
If the lead maintainers are super busy, it is okay. Perhaps they will have time later and can do things in bulk, or even not at all. The process is no worse and does not stop when the majority of lead maintainers are on vacation.
Interesting changes are still notified via email (thru PRs). This highlights what a person should be looking at.
I think this attitude gives more trust to the team and contributors. It is not the responsibility of just the maintainers and seniors to (pre or post) review everything. No blame game of: "how did this bug get through, and more importantly, how did it get through review??"
> And how well does it scale if the project has many contributors, but few maintainers
Good question. Offhand I would say no worse. Contributors would all be required to ask, which is no different than 'review everything.' The difference would be for maintainers, they would not be required to always have other maintainers do CR in every case. A second potential difference, contributors might be promoted more quickly to have write permission. That in turn helps scale the efforts of the maintainers.
I think this shines on the other extreme when there are very few maintainers. OSS requiring week long turn around times will have trouble onboarding contributors to be regular maintainers. In industry, trusting developers relatively quickly I think is healthy.
The mentality to be looking to give people write permission sooner rather than later IMO is good.
> Those are examples of not doing CRs correctly, not a testament that CRs are not worth the time and effort.
My apologies for not conveying the example clearly. The examples I'm thinking of usually had no review. These are older code bases written long ago before CR. Or, these are startup quality code bases where a few founders jammed out tons of code. Or, code that was put out during a crunch, almost always successive crunches, where the constant pressure to ship was everything. Or, some Greenfield project that was jammed out and then handed over for maintenance and enhancement. Just large codebase that were never great. I'm also implying there is often a different standard for originally authored code compared to changing existing code.
My point is there is a quality bar that is suddenly higher, a lot higher, than the existing code.
> But the question is whether deciding to invest the time and effort in a well established CR process ultimately results in higher quality software being shipped. In my experience, it does, and it's worth it.
I agree, though not sure if actually always worth it. Projects are rarely labelled a failure, but many are. That can manifest as simply too much time going by without enough being shipped.
Though, good CR culture is indeed very valuable. I think it is more an exception than the rule.
I think that complements SSA at the same time. A strong CR culture with review everything might be hard to distinguish from SSA. In that sense, "ask" could be thought of as "second opinion." When a team gets crunched, there is more flexibility. Or, if the team is immature w.r.t to CR, or if there is a significant lack of reviewers- SSA removes barriers.
The idea is to capture 95% of the benefit of CR with 50% the cost.
> Sure, sometimes CRs step into nitpicky territory
Pointing out typos IMO is useful. Though, I've learned to personally delete any CR feedback I write that starts with "nitpick." I no longer believe it to actually be all that helpful or productive. It is a signal that a style discussion is due. It is important I think to respect that a CR is a blocking thing. Keeping someone an extra whatever amount of time for nitpicks is missing the bigger picture of what is important IMO
----
I very much appreciate the dialog and perspective! Thank you for your considered feedback here. I largely agree with your points, particularly the ones I did not respond to.
I suffer a bit too from the lesson of things taking too long is it's own failure mode. Success is not at all guaranteed, even if quality and solid code is shipped. At the same time, solid and quality code is required to scale, and is required for most success. That is to say that projects do fail very much in spite of quality engineering.
Step one isn’t to find some way to accept squash-merge. Step one is to find some reason for squash-merge.
> always squashing PRs is not a good strategy [because of the following reason]
Hence my question,if workflow X mostly solves the following reasons, then always squashing is potentially a reasonable strategy.
I think you are asking, why do that at all?
The answer is IMO quite involved, nuanced, and not at all one sized fits all.
FWIW: (1) all commits to master become atomic if a passing CI is required before rebase squash-merge.
(2) rebase creates a linear history. Non linear history is very difficult to bisect, and can really be complicated. I don't see much positives from that complexity.
(3) generally PRs are functional sized pieces of work. (That is ambiguous.) If it helps, PRs are more boulders than pebbles. Having boulders in the history is more useful than every small pebble. Net benefit is the history then becomes a smattering of well marked minor changes and a set of functional changes.
Side note, I like anything that helps the workflow of "touch clean code only, touch only the code you need to, clean the code before you touch it". (Clean defined there as "not a pile of illogical, obtusely written crap - riddled with latent bugs). In other words, make the code simple and obvious first, then modify the simple and obvious code. I give little credit to those that spend days studying code to make a one-line "masterful" change. That does not scale, everyone has to do that, and there are thousands of lines. All things in their own context though...
(4) utility and communication for commits. IMO ideally a PR is initiated with a single commit that is written "for the history". This commit description is both descriptive for the reviewers, the git history, and is the complete text for the PR. This creates the most value IMO. I view PRs as ephemeral, any extra text written for the sake of PR is overhead. Then, any following commits are pushed to communicate to the reviewers. The audience is no longer a future maintainer, nor someone looking at a PR to get a handle of the in flight changes, but is someone that has looked at the diff. Thus, commit comments like, "address TOCTOU concern", "fix typo", "add null check", "add test case", "address feedback" all then make sense. On squash-merge, those comments are discarded and the merge is just the "commit written for the history". Otherwise, without squash - those commits have a much broader audience and become long lived. I believe this typically creates a much more useful history, eliminates a lot of noise, clarifies the audience of each commit message, and finally removes any overhead where a detailed description is written in a PR but then never captured in the history (wasteful &puts context in a location that is ephemeral and a step removed)
> > always squashing PRs is not a good strategy [because of the following reason]
I am at least coming from the same position that the OP is. Because I agree with this:
> > IMO always squashing PRs is not a good strategy. Sometimes you do want to preserve the change history, particularly if the PR does more than a single atomic change, which in practice is very common. There shouldn't be a static merge type preference at all, and this should be chosen on a case-by-case basis.
> Hence my question,if workflow X mostly solves the following reasons, then always squashing is potentially a reasonable strategy.
If OP has no problems with their current strategy (where they may squash but they are free not to) then you aren’t solving any problems. For them.
They don’t have a problem that they need to solve. You are the one who wants to fit this squash-square into their circle.
What onus do they have to read some Everything Is A Pattern dot com link in order to come to terms with a policy that they don’t need?
> I think you are asking, why do that at all?
> The answer is IMO quite involved, nuanced, and not at all one sized fits all.
Unlike squashing.
> FWIW: (1) all commits to master become atomic if a passing CI is required before rebase squash-merge.
I don’t know what atomic means. But `git log --first-parent` is equivalent to a fully squashed history. Which means that `git log --first-parent` is atomic if the squashed alternative reality is.
> (2) rebase creates a linear history.
`git bisect start --first-parent`.
There seems to be a pattern here.
> Non linear history is very difficult to bisect, and can really be complicated. I don't see much positives from that complexity.
Non-linear history can involve everything from twenty-head octopus merges to eight different root commits to backmerging twelve times before a feature branch is merged. But does saying no to the straightjacket of always-squash mean that you have to support every DAG under the sun? No.
Most sane everyday histories are limited to branching off the main branch, rebasing until it is done and then merging it back. One merge. Granted some people don’t like rebase or don’t know how to use it so they make back merges.
... And if those people make completely useless histories on that branch? Then they can squash of course. Because we’re arguing against the corporate always-squash policy. Not saying that you should never do it.
> (3) generally PRs are functional sized pieces of work. (That is ambiguous.) If it helps, PRs are more boulders than pebbles. Having boulders in the history is more useful than every small pebble. Net benefit is the history then becomes a smattering of well marked minor changes and a set of functional changes.
People can make however many commits are appropriate for the scope of the PR. It can be twenty or one.
See how the squash-always have to make the world around their “merge” strategy pristine (according to them) in order for it to work? “Just have perfect-sized PRs.” “Just have perfect-sized issues.”
> Side note, I like anything that helps the workflow of "touch clean code only, touch only the code you need to, clean the code before you touch it". (Clean defined there as "not a pile of illogical, obtusely written crap - riddled with latent bugs). In other words, make the code simple and obvious first, then modify the simple and obvious code. I give little credit to those that spend days studying code to make a one-line "masterful" change. That does not scale, everyone has to do that, and there are thousands of lines. All things in their own context though...
Okay.
> (4) utility and communication for commits. IMO ideally a PR is initiated with a single commit that is written "for the history". This commit description is both descriptive for the reviewers, the git history, and is the complete text for the PR. This creates the most value IMO.
Okay. You have still gotten to the explanation of why always-squash-merge is necessary for PRs. (We have covered the previous stuff. `--first-parent`)
We could talk about the ideal world. In an ideal world we could choose better tools that are good for this very granular PR structure. But in the real world we often end up with GitHub or some lookalike. Which is not good for granular PRs, dependent PRs, “stacked PRs” or whatever the trendy name is for the people who fetishize PRs over everything else.
In the real world we still have git(1) to be flexible around the limitations of these team-imposed tools.
And even with better tools I like being able to use several commits for one PR. Because sometimes you end up needing five preparation commits in order to solve the issue you were setting out to do. And that is often a hell of a lot more “agile” then going back to the issue tracker and making five issues in preparation for that final commit. Then making six pull requests that depend on each other.
> I view PRs as ephemeral, any extra text written for the sake of PR is overhead. Then, any following commits are pushed to communicate to the reviewers. The audience is no longer a future maintainer, nor someone looking at a PR to get a handle of the in flight changes, but is someone that has looked at the diff.
Okay.
I think that a PR can evolve to encompass more “atomic” changes then what was initially planned. That has happened to me. Me and the reviewer find out that it makes sense to make some more changes. Not “fix typos” on that same PR but changes that deserve to be commits in their own right.
There is also room for “fix typos” commits that you can then rebase away once you are done. With Git you have that flexibility.
> Thus, commit comments like, "address TOCTOU concern", "fix typo", "add null check", "add test case", "address feedback" all then make sense. On squash-merge, those comments are discarded and the merge is just the "commit written for the history". Otherwise, without squash - those commits have a much broader audience and become long lived.
Do we really have to tediously go through the same thing every time the squash-always camp tries to argue for their one-true-policy? Look at what OP said:
> > Sometimes you do want to preserve the change history, particularly if the PR does more than a single atomic change, which in practice is very common.
Which is what I just went through.
Yes. Rebase that typo fix which you introduced. That’s noise.
Probably don’t rebase that commit which made a refactor which should have been done three years ago.
> I believe this typically creates a much more useful history, eliminates a lot of noise, clarifies the audience of each commit message, and finally removes any overhead where a detailed description is written in a PR but then never captured in the history (wasteful &puts context in a location that is ephemeral and a step removed)
What me and OP has argued for allows you to do all that. And in addition to that it is also much more flexible.
That looks like a very wrong process to me. Why would you even want to do that?
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.
“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.
> Instead of manually finding commit SHAs for git commit --fixup
Assuming you are using fixups, is this actually a problem?
I could see this being a possibility if you are: A. not practicing atomic commits or B. have so many commits in your branch that this is a chore.
A. seems unlikely if you are already using fixups and B. seems like a problem worth solving properly rather than going around.
To sum up, I'm not convinced by the elevator pitch. However, I am keenly aware that the workflows of developers differ vastly across industry, company size, technology etc. I'd be interested to understand what problems this or similar tools solve?
The open source example is hard to fix AFAIK. Everything needs to be a PR, some changes to older code bases are simply going to be either large or multi-stepped (many commits, and sending them all as stacked PRs is often not efficient enough to be effective). In industry, I think there are more solutions available. Though, overall, I am very curious how you would go about solving B.
No. They made a whole tool around a not-actually problem.
Sure you can have a 30-commit branch. Find some OSS project where someone has been doing a large project on a branch for two months because the maintainer hasn’t accepted it yet.
I do try to split the work if it is very clearly isolated, but that usually means less than 3 PRs. I have tried graphite `gt absorb` (which might use this project?) and it still creates a mess.
What I do that I wish more people did is that I heavily comment my own PRs with information that doesn't make sense in comments (for example on line X I add a comment: moved here from Y file).
> You have fixes for the bugs, but you don't want to shove them all into an opaque commit that says fixes
I actually like this, but split each fix in its own commit and during review I answer to comments with: "fixed in commit {commit-sha}". So _often_ bugs are introduced during PR review, if the fixes are isolated it is easier to see what changes between review rounds.
For GitHub/GitLab reviews, I'm totally with you - this makes it more convenient for the reviewer to check that/how you've responded to feedback.
But now if you merge this without squashing, your Git history on the main branch contains all your revisions, which makes operations like bisecting or blame more complicated.
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.
This provides both a good review experience and a tidy history. If you find a strange bug later and can't grasp how someone could have come up with that, the PR still has all the history.
Together with tools such as git-branchless, this also makes working on stacked-PRs a breeze.
(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.)
Someone named it that way and it stuck.
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.