The less you know about code, the more local your changes. In fact this is how the architecture of a software system degrades: lots of local changes sort of make it "melt". Global invariants become temporally or spatially local invariants, code and data flow which had a single purpose acquire multiple purposes, names become semantically overloaded, and so on.
Changes tend to be made higher up in the stack, ultimately the UI, because that has a lower risk of breaking something else. This gets very messy very fast.
Bugs in shared code tend to be worked around rather than fixed because other code might depend on those bugs — which quickly becomes a self-fulfilling prophecy.
Some functions/methods seem to be natural magnets for these overly local changes, and as a result, can wildly grow in size over time. I once analyzed how a 50-line function had grown to 5000 lines over a few years in a series of individually justifiable changes.
All of this is a downwards spiral that’s very hard to stop once it has started. “Refactoring sprints” and other heroic efforts sadly seem to have little impact in such an environment if they don’t also radically address the engineering practices that lead to the situation.
Do you know of any external, authoritative studies demonstrating this? I know it's true from long experience of seeing it happen repeatedly, but I'd like to point at something that can justify a 'refactoring sprint' or something like that to try tackling the impact of this pattern. The other justification is that clean refactored codebases are quicker to change, but it's a difficult sell.
The other side of the coin though is that when you are too close to your code, you can't see some of the refactoring patterns which will simplify it. Fresh eyes bring this to the party. So as ever, a mix of approaches, competencies and knowledge give the best result.
And I disagree about this being what goes wrong with architectures. From my experience this is more to do with entropy than anything else. It takes a continual input of effort to 'tidy up' change to keep things looking neat, and this is often ignored or under-estimated until it's too late. The other reason is when the architecture isn't fit for purpose so has to be bent to fit around the problem it's being used to solve, but that's a different argument.
I think "tidy up" is another phrase for integrating an overly local change.
No more than if you're repairing some damage to a wall, you want to work the repair into the surrounding area rather than just infilling and spot painting the exact area of damage.
Architecture is often fit for purpose when it is initially written, but a change comes along which requires a minor rearchitecture. The overly local change might be to piggy-back some data on an existing structure (which has subtly or not-so-subtly different scope), or add a flag to an existing method. If it's not just routine BAU - adding yet another CRUD handler - it's somewhat unusual for a slight adjustment in design to not be desirable, IMO.
My argument isn't for architecture czars or so-titled architects to be in charge of planning changes. It's that local changes often don't include enough rearchitecture because the person making the local change doesn't have the time to try and see the bigger picture. They just need their feature implemented, their bug fixed, their OKR satisfied.
I didn't opine as to a solution, but if I had to, I'd say (1) ensure modules have clear ownership who is / are invested in long term code health, and (2) ensure modularity is crisp and sized small enough that a person or a very small team could rewrite a module in a few months. This isn't enough - there's higher level complexity in the interaction between modules - but it's a start.
When my own code presents a bug I tend to chase it back to it’s “philosophical roots”.
I tried to work out edge cases from the beginning; to handle invalid data; to swat the bugs before they appeared.
A bug reared it’s little insectoid head.
Now I’m wading my way through every piece of code that remotely affects the issue and making damn sure that my perfect vision is realised.
On the other hand, fixing a bug in someone else’s code doesn’t have all these personal implications. If I fix the bug without causing regressions I’m a hero! After all, it’s not personal to me.
- Someone fixing their own code fixes the codebase. Someone fixing someone else's code, generally only knows enough to make a fix at a single point, to fix an observed bug.
I think the abstract is just making statistical observations and hopefully in time will develop and test hypothesis that could create a theory. Anyway, they can use my hypothesis above and give it a whirl.
I'm always a bit embarassed when I look at my commit history, for personal projects but also for projects at work. The simple commit that just fixes a bug or adds a feature in a few lines is very rare. It's often that a bug could be fixed on a single line, but this "actual" issue is drowned in a sea of read and green in the commit diff. Normally I cannot resist the urge to improve something. Sometimes it's a structural improvement. Often it's only cosmetic changes or moving stuff around that might make things clearer or not, I'm not always sure. At least my mind is working on the problem.
When I look at commits from some other devs that often do these localized fixes, it almost always seems like the codebase deteriotes a little bit on a global scale for example because some knowledge is duplicated from some other location 500 lines away.
I think I lean on the side of focusing on the end product (the tip of the development branch) at the cost of having not the most beautiful history ever. I'm also aware that my style of developing makes it hard to develop in large teams / a product with many release lines. For my small projects that have very little collaboration, my approach seems to work, though.
Maybe there's only two ways to have a nice history: Have a shitty end product, or be a domain expert. The latter seems unrealistic to me, because working on something you already know how to do is not very satisfying.
In fact, a commit should have a single purpose, otherwise it creates dependencies and, for instance, you cannot revert a feature without also reverting a bug fix and it makes cherry-picking a nightmare.
With normal review cycle times, this would slow things very much.
Optimizing your development practices for being able to revert single commits seems very wrong to me. If you produce so many bugs that this is important, I'd focus on making fewer bugs.
At my current job I can't remember us reverting a commit in the three years I've been here.
We probably work in very different environments, and your views may be the pragmatic ones in your environment.
> With normal review cycle times, this would slow things very much.
I don't think it does. Ultimately you need to review everything so if you ensure that each commit is logically coherent it actually makes reviews easier.
Otherwise, in my experience, things can get quite messy and the history very difficult to follow.
It's also impossible to predict when or if you'll need to revert a commit. If you follow good practices and have discipline then you make your life much easier if things go wrong or you need to cherry-pick something, which is quite common with bug fixes if you have several branches (e.g. one branch per release).
I agree that reverting a single commit is less common than we might plan for, but don't smaller commits help review? That's how it is on my team. You can review smaller change batches instead of a whole pr or branch diff
> With normal review cycle times, this would slow things very much.
Compared to how much it would slow things down having to try to revert or correct a commit that both fixes a bug and introduces a feature? Especially if that change involved any sort of DB schema change that had to be reverted.
Anyone who has ever gone to production with a set of changes that involved an important bug fix and introduced a high-value feature, only to find that the whole thing had to be reverted because of a bug in the new feature, re-introducing the bug to customers, knows the importance of small, separable changes.
I agree, but I've also been told off for making many, small commits. I don't know why the complainers don't squash them together if that's the way they feel and leave the moaning out of it.
That's a little strange to me. As a reviewer, I find it substantially easier to read 30 commits that are 20 lines each than one commit that is 200 lines.
Bite sized ideas are really easy to digest. Maybe the real complaint is that the commits aren't sufficiently independent. I know some projects mandate that the full test suite be able to pass on every commit, which is really just a way of enforcing that every commit is "complete".
My view, and I think a best practice, is that for bugs the fix should be in a single, dedicated commit, even is that's just one line.
For features you may split into multiple commits if things are too big but then each commit should be self-contained and complete by splitting the feature in a number of smaller improvements.
So really, commits are what they are based on the work at hand as long as they are kept to manageable sizes. I've seen code reviews for 1000+ lines all over the place. The author may know what they are doing but the reviewers are usually lost...
More commits != Bad, but there are a few bad ways to do it. Committing incomplete changes just to save your current changes, carelessly trying stuff and then piling on fixes to previous mistakes instead of fixing them, etc. But yeah, some people are just threatened by certain things.
> I agree, but I've also been told off for making many, small commits.
We're not paying by the commit, here. As long as the commits make some logical sense, you're doing the right thing and those folks don't know how to use version control effectively.
If it's their project you sort of have to follow their rules. Otherwise I'd say just ignore them!
I'd agree when it's simple to do. Sometimes "fix a bug" leads to rewriting some code and the bug fix comes naturally - or more easily - with that.
For a contrived example, say a bug exists because a sort algorithm sometimes reverses the order of identical values. Fixing the resulting bug locally might involve handling duplicates with some extra logic, but changing the sort might result in fixing the bug as a side effect. I might argue for both because correctness should not be an accident, but we might change one fix to an assert rather than handling a case that doesn't occur any more.
A sort is called "stable" if it doesn't re-order things that compare equal. Unstable sorts are sometimes faster. If you care your code should request a stable sort. If your library only offers one sort and doesn't specify whether it's stable, I agree with your "defence in depth" strategy but I believe the right long term fix is to have the library clear up whether or not the provided sort is stable.
This may be a special case because the question comes up so often, but in general, I don’t think libraries should document all the qualities they don’t have, because that’s inexhaustible. In my opinion, if a sort function isn’t documented to do a stable sort, it must be assumed to be not stable, regardless of current implementation details. It’s often not such a big deal either: in many cases, there’s an artificial key (an id) that you can use as a tie-breaker in the comparison function.
One thing I hate is when people look at the library source code to figure this kind of thing out, since any implementation detail can change with an update. Assume the most hostile implementation allowed by the docs and you’re usually fine.
> One thing I hate is when people look at the library source code to figure this kind of thing out
In case you didn't already know: Hyrum's Law. https://www.hyrumslaw.com/ Even if the source code isn't provided and nothing is documented your users will rely on every observable nuance of your actual implementation anyway.
At Google's scale (internally, for their internal software where they could in principle fire people for doing this) this means if you change how the allocator works so that now putting ten foozles in the diddly triggers a fresh allocation instead of twenty, you blow up real production systems because although this behaviour was never documented somebody had measured, concluded ten doesn't allocate and designed their whole system around this belief and now South East Asia doesn't have working Google search with the new allocator behaviour...
In protocol design Hyrum's Law led to TLS 1.3 having to be spelled TLS 1.2 Resumption. If your client writes "Hi I want to speak TLS 1.3" the middleboxes freak out so nothing works. So instead every single TLS 1.3 connection you make is like, "Don't mind me, just a TLS 1.2 session resumption... also I know FlyCasualThisIsTLS1.3 and here is an ECDH key share for no reason" and the server goes "Yes, resuming your session, I too know FlyCasualThisIsTLS1.3 and here's another ECDH key share I'm just blurting out for no reason. Now, since we're just resuming a TLS 1.2 session everything else is encrypted" and idiot middleboxes go "Huh, TLS resumption, I never did really understand those, nothing to see here, carry on" and it works.
Thanks for reminding me of the term "stable sort". It was a contrived example. I suppose I could have abstracted even more:
Algorithm B sometimes fails on the output of Algorithm A. We can fix the issue by making B deal with that case, or we can change A so it never produces that output. Sometimes changing the algorithm in A just makes the problem go away, and maybe that was a good idea anyway. This seems too abstract, so I picked a slightly more specific (sorting) thing for A.
Mixing multiple purposes into a single commit not only makes the initial review more difficult, it makes later review very taxing when, for example, trying to sort out the “why” of a particular change when troubleshooting. If you have to look through 200 lines of non-functional cosmetic changes to suss out the three lines of functional change, you've had to reinvest basically the whole code review process time again just to get oriented.
yeah, but my single purpose is "make code better". If in fixing bug I can refactor and/or fix/expand/add comments and/or document things to make similar future bug impossible or less likely I'm doing that.
Also the "single purpose" of a commit often includes: fixing or adding issue, documenting what / why of the fix, and mentoring other devs via example or co-review.
When I write some code it has a platonic ideal in my mind, and I never achieve that ideal, due to various constraints (time, skill, etc). I may pepper the code with TODOs but it's hard to convey that ideal to others.
So when I come back, my code is an unfinished oil painting, and my fixes will try to complete the painting.
If I bug fix someone else's code I am not trying to complete the painting - I am at best just doing restoration.
It is a long time before Inhave made enough restorations to believe I truly own the painting.
That did fleetingly cross my mind as I was writing it. And yeah, I suspect I may have done that a couple of times - taking over someone else's codebase is not unlike watching the pilot parachute out of a flying plane with a "it's all yours now". Sometimes you have wings and engines and radio, sometimes the port wing is on upside down but the joystick is jammed starbd so it's all ok. But you really need to know that as the pilot jumps.
All in all, I think understanding someone else's codebase is like taking over writing a novel halfway through. It's why we all like a rewrite.
(And maybe why film scripts sometimes fell like they were written by committee)
One of my direct reports once fixed one of my "simple bugs" with the astonishingly direct commit message:
"Const, motherfucker. Do you speak it??!?"
(which, I hasten to add, would have been unthinkable if it had been a bit of peer-to-peer banter, much less senior-to-junior, but as a way of winding up your boss, it was a absolute masterwork)
Const should be default. Kate Gregory has a C++ talk where she discusses being brought in to clean up code and she's like OK step one, mark everything const. Of course some of it isn't const, but the compiler will point out every place where this made no sense, so next you can remove the consts that the compiler is unhappy about until you get a const-correct program.
Yeah, pretty much agree. The problem of being lazy about constness is that when you finally get your act together and think about it, you'll wind up having these huge "viral" chains of const-fixes to do. The code will be better for it, but you'll wind up changing long chains of things to be const.
He was probably talking to the code and not you. You're right he should be tactful, but as a manager, please don't take it personally. It's kind of your job not to.
Nah, he was talking to me. We had that kind of a relationship where this stuff was always going to be considered funny, not offensive. We did tone it down a lot when other people were around because this kind of banter can be misinterpreted by third parties ("oh my god, what terrible things is he going to say to me if they speak like that to each other").
My bug fixes almost always start out with “why the F did I do that?????” And then a long series of “oh…” before I finally get to the “ah hah!” Which is a moment I still enjoy after all these years. But fixing someone else’s code I’m more like “ok…I need to find the bread crumbs so I don’t get lost in this forest and end up messing something up…let me test this seemingly simple…ok yeah that broke stuff better roll it back.”
I remember that in my first job, one of my first tasks was to fix a bug where the dates were off by 1. I knew nothing about the codebase and nothing about professional software development so I just incremented the date in the UI by 1.
This is so terribly beautiful in providing a great example of exactly what the article is talking about. Even better if your 'fix' passed peer code review.
> when they fix other people's bugs, their commits are smaller and more tightly focused.
While this may be true of junior engineers or people working on a project that they don't actually put into production, I feel it can't be true of most people working a devops roll on a production system.
I only worked several years in devops on a small production system with a few hundred users but I learned very quickly to respect others' work and the thought they had put into their areas and to be very wary of "fixing" anything more than the described bug.
Honestly, not exaggerating, it makes my stomach churn a bit. Regressions are very real and if you don't have decent test coverage with actual integration or functional tests, you are writing regressions and you just don't know it.
GP may be saying that senior devs tend to s---t all over people's code. I've certainly seen it happen. But I think that's more of a personality trait (well, lack of empathy) than anything else.
When I will see the problem, I will fix the problem. What the problem with that? Do I need to create a ticket and assign it to Junior with a book long description and proposed solution?
The problem is that when one fixes a problem by "s---ting all over people's code" like GP said, chances are they're introducing more bugs or restoring old ones.
That's why git-fu, comments and a collective memory is important. Not only we need to know what was done, but also why, when and in which conditions it was done.
No. It's not about satisfying bureaucracy. It's that at a certain point, any complex code system becomes a black box. You cannot fully reason about it. Your tests become your source of validating that box and any change you make is just as likely to break something as it is to fix something.
> This is why I find empirical studies so valuable: in many cases "X" and "not X" are equally easy to rationalize, so we need evidence to help us decide which to believe.
I agree with the author here, but I also see some cause for alarm. When we could see it going either way, maybe it really could. As in, the result was a coincidence. Or maybe not exactly a coincidence, but caused by some very specific details.
This investigation was on java code, but will the same hold for haskell code? Closed source code? Will it be true 20 years from now? Maybe... but I also wouldn't take the answer for granted.
And, most insidious of all, maybe we as a community will look at this and decide it's a problem to be solved, and succeed in solving it.
A less charitable take, and something that came to mind for me faster than what others are presenting (that commits fixing bugs that you caused tend to be larger because you know more about the architecture around the code needing to be fixed), is that including other code changes in your bug fix commit serves as a way of covering your ass. If the reviewer needs to sift through and understand 100 lines of whatever (maybe germane to the fix, maybe not), the few lines that address the issue you caused won’t get as much focus, and therefore the issue (that you caused!) itself won’t stand out in your teammates’ minds.
One of the great challenges in coding is fixing somebody else's code with 100% confidence you didn't break something bigger.
Each function may appear to have a contract, but it's sometimes unclear if other code is relying on the apparent (mis) behavior of the code. Obviously any sort of global state (including config files, database), magic, parallelism, async, abstraction layers make this exponentially more complex.
The author knows all the things that aren't there, it's easy. The bug-fixer just has to check every single thing or risk being bitten by something spooky (e.g. a database trigger).
Tests and compiled languages help reduce risks associated with fixing a bug in a function.
The classic fear that "I don't know what else uses this function" is simple enough to solve for a developer who knows how to perform static analysis.
However, many don't. Even if they do know how to find all areas where a function is being called, it's easier for the "busy" developer to make a copy of the entire function and just change the part they need.
Static analysis of use requires knowing all the calling parties and having access to their code. A lot of systems these days are not static monoliths but dynamic assemblies.
> One of the great challenges in coding is fixing somebody else's code with 100% confidence you didn't break something bigger.
That's the entire premise of the techniques in Working Effectively With Legacy Code: how to ensure the code you write changes the behavior that you want to change but doesn't change behavior that shouldn't. Great book.
Hmm, this agrees with how I have behaved. I have deeper knowledge of the bug in my code so I can make more fundamental changes. With code I'm unfamiliar with I am less certain if I'm bringing down Chesterton's fence.
I wonder if there is also a social aspect to this of 'politeness' in not stepping on others' work. I wonder if there are ways around that.
EDIT: Though, interestingly, reading some of the comments with people misunderstanding the title, it looks like the article is right. If it had said the opposite, I might well have rationalized it the other way.
Interesting. So many times I have patched something in order to get it working and wondered whether the developer would have come up with the same fix, or if they would have done it differently.
From the article:
We also found that bug-fixing commits by authors tended to be larger in size and scope, and address multiple issues, whereas bug-fixing commits by other developers tended to be smaller and more focused on the bug itself.
This makes sense. If I'm fixing a bug in code I'm not super sure about, I want the change to be as focused as possible because even if it looks like it should be made into a bigger change, I don't know what the knock-on effects will be. There's also a matter of not being sure if the desire to rewrite a large chunk just comes down to personal preference.
Yeah, in my case at least "what the hell were they thinking" comes down not to them having solved the problem incorrectly, but a difference in the shapes of our minds.
See also Peter Naur's "Programming as Theory Building". Essentially,the original authors of a system have a theory (what nowadays we would call a model) in their minds of the system, of its architecture. Something that is difficult to convey by code alone. So when a new batch of programmers start working on that codebase, they don't have any knowledge of this theory, so their fixes and new features are of much worse quality than what the original programmers would have proposed.
Just like there is a difference between interface and implementation in OOP languages, here 'theory' is in the heads of the original builders of the software, and the 'implementation' is the code written in particular languages.
It is easy to debug and fix when one has that 'theory' in one's head.
We also found that bug-fixing commits by authors tended to be larger in size and scope, and address multiple issues
Too bad I cannot see examples of what this actually entails - for us one commit which fixes several issues is a complete no-go: that should be separate commits. So now I wonder whether the paper really means single commits which address multiple things, or groups of commits?
I think that largely depends on the team and the code. I've worked in legacy code bases where fixing a bug can cause other bugs to show up. A single PR must address multiple issues to keep the software working.
In such cases I try to split those additional fixes and apply them before your main bug fix that requires them. But yeah, sometimes that isn't possible.
Sure but if the thing you're fixing requires fixing other parts how do you get to 'one commit fixes one thing'. Its simply not possible sometimes. Unless you hack in half baked fixed for the sake of a 'commit to fix X' then have to change that in the next commit to fix Y, etc. That's just a time wasting exercise.
I took it to mean a PR level commit (aka squash/merge commit in git). I work in SVN and git everyday so commit and PR are basically the same unless talking about git specifically.
These are single-statement bug fixes in Java code so it likely would not make sense to try to cut them into multiple commits.
Consider this line (my Java is a little rusty, but let's hope not too rusty as I start a new job next week in Java) in method get_answer of the class Query...
Perhaps the author, upon reading the bug report concludes that the problem is they've calculated a new query but in this scenario it's the similar old query they ought to check in the cache instead and they write:
answer = Query.get_answer("Slightly different query");
Again this is a one line change, and perhaps it cures the symptom, bug closed, but the author was correct, and "Slightly different query" just happens to avoid the case where newQuery is different enough from query for the error to cause trouble.
This type of change that "fixes several issues" is fixing a correctness bug and I don't really believe you that you'd refuse to accept the first one line change because it "fixes several issues". Correctness bugs are going to do that.
> These are single-statement bug fixes in Java code so it likely would not make sense to try to cut them into multiple commits.
But if the "fix" itself were always only a single statement then it couldn't ever be larger in some cases, and then there wouldn't be have any differences in what people submit to describe. I suppose it comes down to whether one interprets the article's "...looks at single-statement bug fixes in Java..." as "[single-statement] [bug fixes]" or "[single-statement bug] [fixes]". It feels to me like you're reading it as the former, while I think it has to be the latter.
So people could well set out to fix a bug (that the authors somehow know was) caused by a single statement, and when the code is someone else's, that's all they touch; but when it's their own, they also, "while I'm at it", fix a bunch of other stuff they notice while looking for the bug. Because they're familiar with the code, so have the confidence, and feel responsible for it, so they have the desire ("Wow, this is ugly, can't leave this in").
So all those changes together could well be several regular working-branch commits. (Maybe then rolled up into a single merge/rebase commit for PR, Idunno, it wasn't quite clear to me either.)
I think it's important to keep in mind the difference between "I changed this one thing to fix problem A, and it happened to also fix B and C" vs "I changed this to fix A, that to fix M, and another thing to fix X".
And I think the former happens exceedingly rarely. :-) Most cases where it does happen are probably due to M and X just being other aspects or consequences of A.
I wasn't really trying to be pedantic, Im genuinely curious about what gets meant here. E.g. as you say, do they mean PR or literally one single commit? And in the latter case, is this something localized to the projects they sampled or is this a more general practice?
Sometimes, a collection of related changes has to go in together. E.g. one fix may uncover another latent bug, so that tests no longer pass, or an API change may be the right solution, but that has consequent effects, and should be done in phases.
Where I'm working now, it's the opposite to your place: It's almost a no-go to use separate commits to fix separate issues in a change that is reviewed as a whole. We have been asked to use fewer, larger commits. In practice, what I see is commits with a description (if you're lucky) that describes the main change, but there are other changes mixed in which aren't mentioned in any description.
I prefer to use separate commits to fix individual issues even when making a bugfix PR that is "larger in size and scope". That way, people can see each part and I can present the fix most clearly, and I have to figure out the best order in which each fix makes sense. I usually do this even with my own private work that nobody else sees, as there is some satisfaction and clarity in dividing up related issues. I usually construct the logical set of changes after I've figured out what they are and satisfied myself they work well together, through use of "git add -p", "git rebase -i" and per-commit testing in a "curate your Git history for others" approach.
This comes from the Linux approach, where a proposed patch series must be, in principle, a sequence of commits designed for review, that could be cherry-picked or have some of them left out.
The unit of PR should be larger than the unit of commit, and commits should each be one reasonably self-contained logical change that could in principle be cherry-picked in isolation, or individually rejected.
But lots of places have a policy which requires PRs to be "squash merged", so all the changes are one commit in the end. The individual commit history is actively destroyed. There is no point putting in the effort to lay out carefully curated commits for a PR, if most of the history will be destroyed soon after, especially if reviews don't go deep.
I've been told this should be done because it "helps with rolling back changes" but I think that's incorrect for my style of commits (because they are curated into functioning logical changes already), and it's a crude approach to rollback. However, it makes sense for those people's PRs which are chronological (commits like "try X", "try Y", "aha, fixed it"), and where part way through the PR, the project doesn't even build.
When I roll back changes that have been merged, it's rare to want to roll back an entire group of changes. By the time a problem is found with a merged PR, it's probably too late to undo it all anyway as other things will have been built on top. That's when I'm most likely to find the individual commits of the PR most useful.
