Hacker News Reader

Comment by sethammons | Hacker News Reader

Nice commits can really tell a development story that makes reviews easier. That said, I want all teams to squash merge their feature into master after tests pass. One commit at the end, and one commit to remove in case of an issue affecting customers related to the release.

A very, very large problem at five out of six companies I have worked at is casual code improvement and refactoring. Devs would say, "we will address that minor and unrelated thing in a separate PR" - one that never comes. At one company, a single PR could address unrelated fixes and it was encouraged to "take out the trash" on the code. Unrelated metrics added, logging improvement, or code simplified, or test robustness improved, etc. That company had vastly better code. Easier to read. Easier to maintain. Easier to observe. And easier to test.

View on HN

I'm honestly baffled by this. You're a proponent of dealing with chores as you encounter them during development rather than putting them off til later (great! I love this!), but also when that PR lands you want it all squashed down into a single commit, which presumably will have a message like "Implemented Important Feature, also did a bunch of unrelated work".

That sort of workflow is ideal for making sure you've got a set of isolated commits each looking at a single subject so that when someone is reading through the history later they can quickly see where something was introduced or why, and jj is perfect for doing that because it makes crafting those commits so much easier.

What if you could have the best of both worlds?

Where the developers at that other company could “take out the trash” amidst one PR, jj makes it trivial to carve off each of those fixes into their own separate PR. It’s no more work than making them separate commits.

People don’t do this in git because it’s a hassle. So you either get cleanup that never comes because each branch needs to know in advance what work it’s going to do or you get omnibus PRs that do twenty different things.

There’s a better way, and it lets you have clean history and developers can fix things they run into along the way.

> That company had vastly better code.

It's very frustrating that what IMO is the inferior approach was producing better results in those teams!

It's because what you see as the inferior approach involves less effort and friction for the developers.

When you are told to separate general code improvements to another PR, or worse, to not do them, and create a Jira task for them so they can be adequately prioritized, it just saps your will to do so. You just won't do any improvements that fall outside the scope of the feature, because even just thinking about the hoops you have to jump through to get work done is mentally draining.

I don't know how but we need to get developers out of thinking of PRs as the smallest possible change unit. This is literally what commits are for, you do a chunk of work, and you have a commit which describes that chunk of work. If you've got cleanly isolated commits then when you come to reviewing the PR (or changeset as I'd much rather see them called) and someone questions the wisdom of including that particular change you can either modify the commit to satisfy the questions, or just pull it out of the changeset into it's own for later review without blocking merge of the wider feature.

Coincidentally jj makes this process much easier than it would be with git, it will very happily let you shift commits around between different branches, edit commits in place and cleanly rebase those edits onto subsequent commits, or split a messy commit into two commits that makes sense.

The UI may be cluncky in the PR page, but I just use rebase, edit the commits, and force push the whole branch. The PR is the unit from the business perspective, not from my computer environment. I don’t mind creating two PRs for stacked changes, then once the first is merged, rebase from the main branch and publish the second one. Comments can be used to explain the link.

I think the confusion and angst comes from when someone has multiple unrelated commits, submits it as a single PR, and is then REVIEWED intermingled all at the same time!

If people instead reviewed commit by commit until the PR HEAD, the code itself would tell a story, but best of all - the story would then be obvious!

> It's because what you see as the inferior approach involves less effort and friction for the developers.

I can see that.

From the other side of the PR though, it involves significantly _more_ work from a reviewer.

The "red tape" of separating commits and opening separate PRs should be removed by the team.

The effort of separating commits and opening separate PRs is minimal once you're comfortable with the tools.

I encourage colleagues to be comfortable with these workflows, because a reviewer's time is generally no less valuable than their's.

Reviewers don't want to navigate 33 tiny PRS either.

The best way of getting changes is through is simply sitting down and talking with the reviewer. Most of these small PRS, splitting things, creating elaborate stacking systems are just technology hacks around a social/process problem. I've seen people make more of a mess trying to split pr's up where they are so fine grained its silly and actually had dependencies on commits they didn't realise they had which reviewers then had to resolve. Literally anything to avoid talking and working with people. People are trying to turn a tightly collaborative process and turn it into isolated single work units with no collaboration that just need a rubber stamp.

> Reviewers don't want to navigate 33 tiny PRS either.

As opposed to one 33-change PR? Yes, absolutely yes they do.

I probably don’t have time to review a giant PR like that. If I do, I feel guilty asking for fixed in one part when 31 of the changes are great. Why are we holding up all these improvements for one or two small concerns? We can merge and just fix those later. Except that never happens.

I probably have time to review eight one-liners. My other coworker has time for five. After lunch I can quickly check out another seven. Over the course of the day all 33 get reviewed and merged as time allows.

> As opposed to one 33-change PR? Yes, absolutely yes they do.

100% this.

With remote teams spread across time zones, mega-PRs become even more problematic.

> one-liners

One commit for a single line? I can't imagine developing software this way. Fixing bugs? Sure. Altering this or that in a well-established, mature codebase? Probably. For features, I don't see how it's possible.

Jj makes this effort vastly easier. Nearly frictionless.

Great. As I said elsewhere on this story, I'll suggest jj to colleagues who struggle with git.

Yeah, I don't get it. Maybe I'm just confused but the git mental model has never struck me as complex or confusing.

jujutsu also seems like a poor fit for large monorepos repos (particularly ones that use git hooks). The expense of certain git operations increases quadratically with the size of the repo. A repo doesn't need to be terribly large for `git status` to require 5-6 seconds. (God bless the man who added scalar and sparse checkout to git.)

The absolute last thing I want is a tool that's doing more with git -- creating commits, branches, checking things out, rebasing, or doing anything that requires a git-status check.

The approach didn't drive quality. 1 team cared about their code and felt empowered to improve it, the other didn't.

The majority of people care about the quality of their work when they are starting off. Not caring is a learned behavior. When you repeatedly get reprimanded for it, you learn not to care as a way of protecting your mental health from taking even more damage.

paperwork and red tape results in lower velocity. sometimes this is what you want.

note this is also true in software engineering.

> That said, I want all teams to squash merge their feature into master after tests pass. One commit at the end, and one commit to remove in case of an issue affecting customers related to the release.

This is the one thing I always point to when I say "Mercurial is better" (and now, jj).

Why can't we have our cake and eat it? Mercurial lets you have lots of individual commits, but they're hidden, and you'll see only the squashed commit. Why do we have to lose all those individual commits to get that squashed commit? The answer: You don't need to.

> I want all teams to squash merge their feature into master after tests pass. One commit at the end, and one commit to remove in case of an issue affecting customers related to the release.

Hmm what's the issue with the GitHub default of merging PRs, where there's a merge commit which individually pulls in the PR's commits? You can revert the merge PR as a whole, or the PR commits individually. E.g. with this[1] merge commit, you can `git revert 0a98f570 -m 1` (the merge commit) or `git revert b30950fc` (an individual commit from the PR).

[1] https://github.com/amelioro/ameliorate/commit/0a98f570f63ffd...

Reply on news.ycombinator.com