Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Ship Small Diffs

blog.skyliner.io

Post reply on HN
Print view View original

51–60 of 76 posts

Re: Ship Small Diffs

#51

Post by Groxx »

rtpg wrote: post #45
Groxx wrote: post #38

Ehh... nice in principle (and I do small deploys all the time for work), but too many artificially-small changes can easily cause you to "miss the forest for the trees". Each change is small and LGTM-able, but they can add up to a misbehaving system unless you have full context (which, because they're small, does not exist in the diffs). If it's conceptually a single unit, keep it a single unit. Pushing dead code in…

The problem is that review complexity is quadratic in size, and quadratic in number of interacting parts. Each thing can interact with each other thing (in theory), so if you have double the diff, you might need 4 times the amount of work. And the increased workload increases the risk of a mistake. But splitting up the diff in two doesn't change the result, right? The reality is that everything doesn't interact with…

>Secondly, it will let _you_ confirm you know the divisions.

Absolutely. Which is why I draw the line at "conceptual units". If there are rational sub-divisions that don't require context, go ahead, divide! If you're baking in cleanup + feature + bugfix + convert tabs to spaces, you're mixing things in the same way that you would usually avoid when coding. Same thing with too-large commits - we avoid 10,000 line files and functions, do the same with commits.

I caution only against artificially small diffs. Small is a fine target, but there's a lot of dogma around it everywhere I've been, which is why I think it's worth calling out. Breaking things up too much can remove context, which can be dangerous.

---

If you're introducing a large thing in 1 vs N units (say a full feature in one go, which is unusable until complete), the line is of course fuzzier. In some ways, sub-units may be more reviewable - that's a solid benefit, though they're not likely a dozen or two lines of code (if they are, and it's a large feature, you probably now have dozens of reviews). But you aren't getting any additional safety, because it's all either on or off - which is half of the post.

For the other half of the post, if you're not getting the same reviewer(s) for all the pieces, unless you've had a detailed architectural review[1], has anybody but you read and understood the whole system? When it's turned on and it breaks in a subtly-interacting way and you're on vacation, does someone else know where to look? Doing it all in one go, as it will be when turned on, forces an architectural review of the system implemented, not just the plans. Say it takes a day or two to thoroughly review - is that bad?

---

Always tradeoffs. For the situation described in the blog post, where large reviews get insufficient attention, I don't think small diffs will solve the problem. The problem is the lack of attention given to reviews - fix that. It's critical regardless of diff sizes.

[1]: No place I've worked for has truly done this, which I would claim is the (vast) majority, but I wholly believe some places do. I would probably even like it! But it's perceived as slowing things down, so it hasn't happened for me yet.

Re: Ship Small Diffs

#52

Post by FLGMwt »

toddsiegel wrote: post #11

Generally, I like the middle ground of reasonably small branch sizes and small, well-organized, well-written commits. Walking through a half dozen or so commits in a PR is still manageable. Too many small things deployed over time can be a pain to roll back too. On the flip side, huge change-the-world deploys are bad too. It's a balancing act of managing the initial feature size, getting it out, layering on more in s…

I think I still prefer the logical diffs to be at a PR level. I know at least personally that I commit a lot of experimental code that I back out of for the eventual implementation.

Doing git fu to reverse that is not worth the time imho. I'd rather invest time in factoring (and testing) tiny deliverables.

At least then, you still have the diff as a travelogue.

Re: Ship Small Diffs

#53

Post by FLGMwt »

Animats wrote: post #18

Birth defects are forever. If the design has a problem, "small diffs" won't fix it. That's the trouble with the "continuous hacking and integration" approach.

I think that's a bit too fatalist. I'd hate to have an org fall into the trap of "oh, well I guess since we didn't plan for changes since the beginning, we're stuck with 21 point stories and appropriately long 4 week sprints". I want to hope that the outcome of this post is "hey, I wonder if I can ship smaller parts of this to make the whole less risky"

Re: Ship Small Diffs

#55

Post by Groxx »

MaulingMonkey wrote: post #30
notacoward wrote: post #5

Um, no. Keeping diff sizes smaller is nice, but asking for "a few dozen lines" most of the time is too doctrinaire. In many codebases, especially those that are older and larger, even a fairly straightforward enhancement can require a few dozen lines of new code plus even more modifying callers or hooking things together in other ways. Oops, already over the limit. Breaking up patches can even make them less coherent…

> Breaking up patches can even make them less coherent as context for each one is lost A potential solution: Non-fast-forward merge commits. Best of both worlds: Small diffs and large diffs. (I'll also note that I haven't found this to be a problem in practice.) > and can slow things down if all the pieces have to be pushed separately through a slow CI pipeline. And can speed things up if the CI pipeline can pinpoint…

I like the merging pattern in principle for that reason. In practice I've had too many tooling issues (git bisect being a PITA to use due to infinite decisions, history-browsing difficulty, people getting confused about which commit to revert and how).

None of which are intrinsic. And maybe they've been fixed since my last attempt? But noticeably painful.

Re: Ship Small Diffs

#56

Post by cortesoft »

carterehsmith wrote: post #42
farnsworth wrote: post #37

Earlier quoted context omitted.

And how many different tech stacks have we been through in that time? I build web apps using very different tools than I used 4 years ago. I read that sentence as comparing web dev on a timeframe to, say, civil engineering.

Yeah I can see how that is problematic. What I do is -- look at what very big players are doing (Salesforce, Facebook, etc) and follow that. Also, look at what all random minor players are doing, and absolutely ignore that.

I have worked at both large companies and small startups, and 'doing what the very big players are doing' is not always the best thing to do. There are just fundamentally different challenges depending on your size.

Re: Ship Small Diffs

#57

Post by charleslmunger »

mcfunley wrote: post #3
pavel_lishin wrote: post #2

> You don’t need elaborate Git release rituals. Ceremony such as tagging releases gets to feel like a waste of time once you are releasing many times per day. What happens when you ship bugged code and need to roll back?

The general advice is to roll forward, i.e. remediate the broken thing and deploy. Ideally, the broken deploy was ramping up a config flag and the fix is to turn it back off. (But that's another post.) Rolling back mechanically is sometimes safe but not always safe, because you have data.

In a large system, every change that comes with stored data needs to have a plan for rolling back - usually it's as simple as "ignore but save" for a new column in the DB, or "write to both, read from old" for a backend migration.

Re: Ship Small Diffs

#58

Post by Groxx »

Vendan wrote: post #17
tonyhb wrote: post #13

Earlier quoted context omitted.

The expressiveness of a language directly impacts the amount of code you need to write for a feature. Go is a concise language but it's not expressive - you'll write 4 lines to call a func and check an error. This means that PRs are inevitably going to be larger than a language like Ruby or other languages with metaprogramming.

I think you have those backwards. Concise is expressing an idea in less words, expressiveness is the breadth of ideas that can be communicated. Go is not concise and does not try to be.

Just to play devil's advocate (in case this is what they were thinking), go's language definition is fairly concise, by design. Few keywords, little magic, leading to lower expressiveness per LoC.

Re: Ship Small Diffs

#59

Post by scott_karana »

pavel_lishin wrote: post #34
scott_karana wrote: post #4

Earlier quoted context omitted.

`git revert` is perfectly suited for this, since it makes a new "inverse" commit of the changes. Your production deploys continue monotonically into the future as usual, and you now have version-controlled documentation of the rollback, instead of needing to maintain a separate mapping of production code state.

Doesn't this assume you only ever deploy one commit at a time?

Cherry picks and merges are single commits... and fit well for a lot of dev->staging->release branch workflows.

Re: Ship Small Diffs

#60

Post by iamleppert »

This is nonsense. The number of lines of change of code has absolutely nothing to do with the amount of risk.

A single line change can easily break an entire system. Applying this size fallacy, however is just as dangerous. Certain classes of changes should be grouped together so they can be reviewed with the context required.

I've had the unfortunate task of working in environments where I've been forced to artificially break up my PR's to suit some arbitrary rule, no doubt because an article like this got read at some point.

What ends up happening is instead of having everything related to a specific changeset or feature in a single PR/commit, you now have it strewn across multiple PR's/commits, non-nonsensically, and with artificial borders most of the time. This makes review more difficult and creates additional work to support the partial implementation working at each slice.

Process should not replace thinking. If a feature warrants it, who cares if the PR is 2,000 lines or 12. Not every task is small and not every task is large. As long as the scope is well defined, many times it does make sense to do tasks in whole rather than as a sum of parts.

Post reply on HN