what about cleaning and filtering out useless commits, by soft reseting the branch and commiting just the actual changes to merge. Sure, it can take a lot of time, but it's done once, by you, after all you're supplying the changes. And if it takes a really long time, you're doing it wrong in the first place by submitting PRs with many LOC.
Have you tried rebasing branches with dozens of short useless commits? That's a real waste of time, and everyone needs to do this if you want to update your main branch. So you've now multiplied the amount of wasted time for everyone.
So... squashing?
1. first related change
2. first merge commit
3. second related change
4. second merge commit
5. third related change
6. ...
Instead of just having all related changes in several commits with a merge commit at the end that groups those related changes.
If you have a too-huge PR that can be subdivided, it almost always means you have multiple units of work (feature/fixes) into one branch/PR.
The reason I agree with the article and use squash is that it allows us to have a different history between master and the local branches. Developers can freely subdivide each feature into how many commits are useful to their mindset and workflow, while the master branch will remain clean.
In my experience, the individual scope of each commit on the master branch is almost always correlated to the scope of one code review, and thus one PR.
The one thing you lose with just making one PR per commit is the relationship between a set of commits used to implement a feature. One commit involves some retractoring to make it easier to cleanly implement the feature. The other commit would be the feature and tests, and the last commit would be to add calls/references to that feature.
If these commits were kept in the same branch and merged in a single PR, then it would be easy to see the relation between them. If they were merged as 3 separate PRs, then it would be more difficult to see why the refactoring was done, especially if PRs for unrelated changes were merged in the interim.
I'm not saying that I wouldn't broadly discourage the worst of that but most people do not commit every changed line so it is rarely a problem in practice.
What does become a problem are people who insist on "clean history", "re-order these commits", etc etc at PR stage when you both agree the functional problem is done/fixed and now the negotiation has moved on to your git history. I do contract work (read: move around a lot) and this social anti-pattern comes up very often. Usually one guy who's into it throwing his weight around.
Would it surprise you to learn there is a large crossover because people who do this and people who go forensic on other cosmetic stuff: whitespace, syntax, etc?! :)
It was obvious that the later commit was wrong and I could fix it to do X. If it were squashed I would have thought the original intent was to do Y and spent way more time trying to figure out how to fix it.
Obviously it's not guaranteed but throwing away that information is a bad idea if it's just for some arbitrary aesthetics. Same goes for rebasing to clean up the commit history, which would ostensibly do the same.
My solution is:
- small-scoped PRs
- attempt to keep sub-commits readable, but don't waste time on them
Squashed commits also link to the PR where not only do you get the original commit list, but also get the whole discussion around the code.
I'm also one of those "micro committers".
Because of this attitude, many hundreds pedantic commit rebases after, I'm now a better programmer (and I also spend next to no time with that type of rebases).
This is because I have now a much higher capacity (and speed) of breaking down problems into smaller, self-contained, steps.
There is a common understanding that VCs are just storages. VCs actually map the mental model of the developer. Having a precise, clear, granular VC history has a bidirectional relationship with being a precise, granular, clear developer.
Eh. Not really. A clean history is a good resource for figuring out what was changed and why. Cleaning up history isn't even that much work anyway.
> When you're digging through VCS history due to a bug you often ignore the commit message anyway - if the code did what it seemed to do you wouldn't be there.
Only if the commit messages are shitty, which unfortunately they often are. I try to summarize what I'm changing and what I indent those changes to accomplish, because if there's one thing I hate doing, it's spending more time than necessary to puzzle out what I (or a coworker) was thinking at the time.
VCS history can be a pretty powerful tool, but a messy history discourages the use of it.
really, do people even use git bisect? If you do, you go through some rite of passage and I've seen people including myself make better (not perfect) commits. It remains a black art to gauge what the perfect amount of work in a commit should be.
Unless you're using a garbage client (eg, the git CLI) a rebase to get rid of the "typo" "oops" type commits (when you forgot something) takes I'd say 10-20 seconds.
Most commonly I do this when I have several changes on the go at once and forget to commit a fixed unit test, new import, or something like that. I usually realize it right away, so my commit will usually be "add test" or "amend 2 commits ago" or something to remind myself which one it goes to (and it sticks out, because my other commits are usually in the format "PROJ-1234: Add new --host command-line option").
> When you're digging through VCS history due to a bug you often ignore the commit message anyway - if the code did what it seemed to do you wouldn't be there.
I presume this is doing a bisect or something to narrow down where a bug was introduced?
My experience comes from the opposite side. Usually I identify the line of code causing a bug, it makes me go "wtf, what is this even supposed to be doing?" so I do a git blame. Hopefully the commit message is useful and ideally leads me back to the original bug/ticket, so I can figure out what the original intent/fix was, rather than inadvertently breaking something (or re-causing a different bug it fixed). I'll also note that in code that is well-commented and well-tested, this step is rarely necessary.
As an example, say I come across something like this:
if (port > 443) ignoreErrors = true;
I do agree with you though that rebasing and squashing are absolutely easy and awesome and whoever doesn't agree probably comes at it from a badly run repository with utterly bad practices and it would probably _benefit_ from a proper squashing and rebasing practice.
If the commit message is something like: PROJECT-TICKETNUMBER Description, you can you know... see the ticket/issue along with full details and discussions?
Check those out. It makes the rebase automated with correct commits getting fixed up or squashed as you desire.
This is the most important line from this comment.
If I'm using blame or bisect, it means I did something wrong.
I merge 10 times a day - and if it takes me a minute, because I'm slower than you - that means it's ten minutes a day. Add the cognitive load of the whole process and the added time of explaining it to other team members - 10 minutes is an underestimate.
I'm going to spend that time writing tests and comments, since even you note that they're the better option.
Squash is good enough. I spend my energy improving the right things.
Our master has straight history. We always rebase to merge a PR and we have build scripts that check this as well. You will not be able to merge a PR that tries to get more than one commit onto master, if the commits touch more than one of the modules (we have a monorepo with lots of individual services). Until the PR has been approved we leave individual commits on the branch though as that can be helpful.
I have not had to "mess" with rebases ever. Most rebases apply cleanly and if they don't the conflict resolution is mostly very easy. The 'hardest' conflicts to solve were actually in the end the easiest to solve, because all you had to do was to `git rebase --skip` the appropriate commits. Of course some developers that didn't know better spent hours trying to do those conflict resolutions and complained loudly. Fortunately they were that loud, so we caught it and just did the skipping. Everything applied cleanly without any conflicts after that.
It's pretty awesome to have one commit per ticket on master and especially if something goes wrong. I can easily `git bisect` to find which ticket introduced a bug or a regression and I don't ever have to mess around. All commits have to have a ticket number in the commit message.
It also enables firefighters to almost mechanically solve production issues. Prod is broken after the last production deploy? There's very very likely only one new commit since the last deploy before that (or maybe a few). In any case, just revert to the commit of the previous deploy, lock down the deployments and now you have time to properly solve the issue without time pressure. If there was more than one commit and you can pinpoint which of them caused the issue it would even be very easy to just revert that one commit and let deployments continue. No dealing with 27 commits that make up one ticket.
Know thy tools!
That seems wrong to me - a big advantage to having a monorepo is that when you need to make a change to how two services work together, you can do so with a single atomic commit.
This stuff is also very fluid and changed and improved as we go. If the easy solution turns out to not work well enough for enough common cases _then_ we spend more time making it better. Otherwise 'good enough' does the trick for us. YMMV depending on your company size and developer culture. E.g. you might have too many cowboys that just add exceptions all the time or remove the checks entirely. Nobody can help you there. Fire then or flee ;)
1. Make a commit supplying the test coverage that was evidently lacking.
2. Make a commit supplying the fix.
Everything you say is valid. However, what I just described is the simplest and clearest example of valuable multi-commit-per-ticket git history.
Once the PR is approved and before merging to master, rebase abd squash. The build failing intermediate commit is no longer valuable.
At times what you say will be correct, but it's not at all obvious that what you say is always correct. Suppose that subsequently the authors of the fix in question have a horrible realization that there is something wrong with their fix, and furthermore they are panicking. Now, as a helpful colleague introducing some calm to the situation, wouldn't it be reasonable to recommend that they go back to the state of the codebase where (a) the original bug is present, and (b) it is demonstrated by a test?
But with your squash, no such state exists on main[0]. To my mind it feels a bit janky to have to check out some non-master commit from a feature branch to go to that codebase state.
--
[0] I am not an ideologue but I do kind of like main over master as it is shorter and clearer.
That said, I do not need to have an individual commit to do what you are saying. In fact our default is to remove all branches after the merge to master. But each fix is just one commit anyway. So the state you describe is actually just the commit on master that is before the fix. No need for finding old branches and finding the commit that has the test only.
The thing with the tests is also that this is an ideal scenario. Not everyone does this. I don't always do it either or can't do it. But I strive to. So relying on it would be bad. To do what you said regardless of whether there was a first commit with a test or not I simply need that one squashed commit, which is very likely very small and create a patch (simple diff of that commit and the previous commit meaning) and then remove all changes that aren't tests and apply this to the pre-fix commit. This is very likely very easy because tests are in separate folders and it's a small patch file. Probably takes less than a minute to do. Will not always work 100% but good enough in many cases and I can count on one hand how often this would have been needed in my entire career.
(in fact I did this for a similar thing recently. I needed to apply some of the code from a spike. I created the patch, deleted unwanted stuff from the patch file and then applied it. Easiest ticket I did in a while ;))
Basically: 'git diff <prevCommitHash> > abcd.patch' then 'git checkout <prevCommitHash>'. 'vi abcd. patch' aaand 'patch -p1 < abcd.patch'
Re: Master. That's the name of the branch as per git default. Not much I can do there ;)
It'd be a perfectly valid alternative in some cases to clean the commit history and rebase a number of atomic commits onto master if it would make the history more readable.
(a) You suddenly realize that the production code in the main (not deployed) branch is incorrect
(b) There is no test coverage for the bug
(c) You have time to commit a test reproducing the bug, but a fix will take days.
In this situation you think there is negative value in committing a failing test that will make the build go red? Perhaps you were forgetting that sometimes commits alter production code paths and sometimes they alter test code?
The ideal would be to revert the change if known, or disable deployment on the branch until the issue is fixed with an ignored test*. I feel a workflow where you explicitly break the build [incl. tests] when a bug is discovered isn't ideal.
Our master is alway kept green. If master is not green we are dead in the water. If something happens and master is not green we can not deploy any fixes.
As you say we either revert the breaking commit and push that out fast or we deploy the last known good commit. This may just be HEAD^ if there was only one new commit going out. It might also be several commits ago. Depends on what got merged recently and made it into that deploy.
On bisecting: we keep the actual images for some time. So if you don't need to bisect something rather old, then you don't need to build anything to bisect. Only deploy and check whether you had a good or bad commit. A deploy takes less than a minute usually vs a ~10 minute build.
What value does the merge commit in the PR provide if a PR only has a single commit? Couldn't you essentially halve the number of commits in the commit history by amending the single PR commit message to contain the text that the merge commit will contain anyway?
Rebase should be quick and straightforward. If it's not, you've got bigger problems.
By rebasing, you're taking responsibility for changing the codebase as it exists now, rather than how it used to be.
Years ago I worked at an e-commerce shop.
One fine day we discovered that the checkout process had been broken in a very particular corner case at some point in the past several months.
No one had any idea how it had happened.
Most of the team was trying to work out what could have possibly gone wrong by staring intently at code in their IDEs and debuggers while stepping through reproductions.
I thought, "everyone's already doing that - I'll try a bisect, since we have a reliable reproduction for this inscrutable weirdness."
It took maybe twenty minutes before I could point to a < twenty-line change.
The author of said change was flummoxed at the breakage, as it was nowhere near the actual checkout code, but a few minutes of the whole team staring at it revealed a classic piece of PHP spaghetti insanity that had managed to break the checkout process (I no longer recall the specific issue - might've been global variable name collision).
I think it would have taken hours before we got to an answer without bisect.
Granted, it only worked so well because of good commit hygiene (which I had trained everyone to use when I introduced git and code review to that team), but it showed quite clearly how useful good commit hygiene is when paired with bisect.
Trivial example from yesterday: "this worked last week... what did they even touch inbetween?", look at the most relevant commit diff, context makes it obvious someone just accidentally deleted a line too many while replacing a code block.
For more complex stuff, it's helpful for "what were they trying to do with this line", "what's the requirements document they worked off when they wrote this", "which other versions are likely to have this bug too", "Is this new and I can go ask whoever wrote this about it or is it years old", ...
There's a lot of value in bisecting to find the exact commit, moving from "this is probably the commit that broke it" to "this is exactly the commit which broke it"
Typical workflow:
* branch off master
* Write a test which passes at some point in the past, and fails on HEAD
* rebase the test into the branch at the point in the past, so bisect works
* git bisect to pinpoint the bug
With a non-squashed and a history where each commit builds, `git bisect` points to the exact failing commit and makes the investigation trivial
An additional advantage is that you're not wasting time investigating: the unit test can be used as a regression test
A more common scenario is to run git blame on the files you plan to modify to see what commits were responsible for them. If the message is detailed enough, you can avoid introducing a regression because you altered logic introduced by a commit to fix a bug.
> If there's information in commit messages that isn't evident in the code itself, that seems a terrible way to live.
Good commit messages include why a change was made, which usually isn't really reflected in the code itself. Comments may be outdated if the code is changed without updating the comments, but the git blame output for a line of code is timeless because the associated between that commit and line of code isn't lost until the line itself is updated.
Plus even when the _cause_ is in the code, I constantly check "what's the history of this function?" while trying to figure out whether the code was always intended to do this crazy thing or whether it's been morphed by recent changes. I have "Show selection history" mapped to Ctrl+Shift+G+H in PyCharm.
Other times knowing how it was introduced can give you a sense of what the change the original author was trying to make.
So bisect to me, is a way to figure out where and why the code was changed the way that it was, so I have a better understanding of the changes I can make going forward.
That doesn't mean the change made in the past was correct and must be maintained (obviously something is broken), or that it might be that the obvious solution based on following the code logic is correct. But that doesn't mean I wasted time making sure I best understand the reasoning behind the changes made. But this is also why I don't resort to it very often, because it isn't necessary in all cases (I'd even say it isn't necessary in most cases).
Bisect also allows you to see other changes made in the commit in question, and around that commit, so you get a better overall picture of the logic.
Edit: I should also add that if you use github, squash-merging pull requests is fine because you can pull down and check out the pull request branch and run bisect on that. Or more generally if you don't delete history and keep a record of what commits were squashed to make your giant commit, your friendly local maintenance programmer might grumble slightly but will still be able to work effectively. Just please avoid erasing history entirely.
Customer Service reported a problem with something that my team is responsible for. I knew almost for certain that we didn't break it. I had a vague idea that another team might have broken it by a recent-ish change to a different service that we both rely on but aren't the maintainers of (we have a monorepo). In fact they made multiple changes to it recently (they put me on the PR for awareness).
I don't know the code for that service but it was really easy to check for the occurrence of the bug (literally two clicks in the UI to reproduce or not reproduce). Instead of trying to understand a service that I don't own, which would require mental concentration, I randomly checked out a commit from 4 weeks ago, tested => bug not reproducible and started a bisect. I was actually gonna be stuck in some meetings for the next few hours but bisecting is mechanical, so I was able to just do it on the side (deploying is a little bit of waiting in between, you gotta refresh etc.) and a little later I was able to just paste a very neutral "XYZ is the first bad commit" into the ticket. Totally cuts down on the 'drama' too as you're not simply 'accusing' someone else of causing an issue in _your_ part of the application. It's just there, very neutral, there's the commit that caused it. Git told me!
If you _only_ raise it to the team in question, you have nothing in hand but suspicions. Their natural reaction is most probably going to be "No can't be us, must be something/someone else" and you've wasted an entire day waiting for their response that was predictable anyway and you've also just increased both their and your own irritation level. Their's because they probably feel like you're trying to blame them for something they feel is not their fault. Your own because you've just wasted a day trying to solve this and meanwhile customer service thinks you caused a regression.
A bisect that literally took zero brain cycles and told me which commit caused it before I was even halfway done with my meetings allowed me to let them know exactly which commit caused it without a doubt (unless your company is so broken that they wouldn't even believe the bisect output :)). This also gives them very valuable information because instead of them now trying to have to figure out which of their changes maybe caused this thing I'm 'accusing' them of, they have a single commit, which is directly tied to a single ticket. In this particular case, the developer in question actually knew right away from seeing the commit message/ticket number what particular part of their change was the cause of the issue, without even having to dig through the code.
At the point where I have _not_ done the bisect, I in fact do not actually _know_ who is responsible. I have a suspicion, a hunch, an assumption. Assumptions are bad and need to be validated. I can either let the other team that I suspect validate it. If it turns out that I was wrong, I have both wasted their and my own time and reduced my credibility. What then? I try to suspect another team and try the same thing again? That's what happens in a lot of large companies all the time. Nobody looks into anything and just tries to pawn it off to someone else. If someone tried this more than a couple of times with me, I definitely know what I would do the third time around.
They are more likely to accept a bisect from me, because it shows that I am not simply trying to pawn something off to them based on a mere assumption. It shows that I care and that I validate my assumptions and don't just try to pawn things off to other teams and play 'hot potato'. That would indeed be dysfunctional.
The people experiencing the bug don't know who is responsible so they arbitrarily pick someone. The bug could get passed back and forth endlessly between teams until somebody isolates it a little better and bisecting is a good tool for doing that. Just doing the bisecting rather than arguing with the other team about who should do the bisecting is a way to get things done. If it does turn out to be the other team, it may be easier to convince the other team to take on similar requests in the future.
This class of bug is impossible to find by following the logic of the code because it's not in the code.
For this reason I prefer people to rebase on the integration branch instead of merging it in to stay current; unless there is a reason not to. And a lot of open source projects require PRs that to be pre-squashed or be able to be squashed before merging!
However, there is a time and place still for deliberately crafting the PR commits. For instance if you are wanting the reviewer to be able to review chunks of the PR in isolation/stages, or if you want each of the commits to be buildable.. I suppose also for review/testing.
This all depends on the project and other factors like the wider org.
That can be avoided by only staging and committing changes when the change is actually completely implemented. For example, I'll make a change and then I'll stage it as follows:
1. Stage and commit a new method and associated unit tests
2. Stage and commit calls to the new method
3. Stage and commit the version update
Having to go through extensive rebasing is really an issue with treating version control as a back up system rather than a way to record individual units of work with well crafted commit messages that describe what and why the change was made.
This is not always the case. I've worked on codebases where we rarely scan the git history messages for anything useful, it's just as easy for us to dig into the code to figure out what's wrong.
You won't think that when customers report a regression that was introduced some time in the last year.
A complete and utter waste of time. You spend more time messing about with rebase than solving problems.
When you're digging through VCS history due to a bug you often ignore the commit message anyway - if the code did what it seemed to do you wouldn't be there.