Ship / Show / Ask – A modern branching strategy | Hacker News Reader

Ask – A modern branching strategy

martinfowler.com

> Code review, or “Approval”, should not be a requirement for a Pull Request to be merged.

Shouldn‘t it? With all the code I‘ve seen, I think this works only in very specific projects. In all the other projects, this process will lead to many bugs in production that could have been caught by a short code review by a single person.

> People get to merge their own Pull Requests. This way they’re in control of whether their change is a “Show” or an “Ask”, and they can decide when it goes live.

Don‘t like that. It‘s a „I can decide and I can do whatever I think is best“, which will obviously not always work.

Broadly agreed. It's taken me a while to adjust to various different code review practices, styles, and environments, but in the end the opinion I've settled on is that no matter how frustrating it might seem to be to wait for review (especially if it takes a long time), it's always better to have at least one person review any change, even seemingly minor ones (with the possible, albeit not-always-simple, exception of reverts/rollbacks).

Why?

- It reduces the chance that you get into thoughtless fix-merge-fix-merge loops (it can still happen, but knowing that changes may take time for review has an incentive-correction effect)

- It shares knowledge about code changes with other people, so that if-and-when (usually when) bugs occur in that part of the codebase in future, you're not the only person somewhat familiar with them

- It provides training and learning opportunities for reviewer and reviewee alike; discussions, scenarios and opinions can all be shared and worked through, and can shift behaviours and improve system and process reliability

If there's some magical workflow that can allow shipping your own changes without losing any of those benefits, that will be impressive; for now I'm curious to learn more about this approach, although initially skeptical.

This is an interesting way to frame this that I appreciate. It helps me better elucidate my own processes.

One thing that jumps out of me is that I'd rename "Ask" as presented here as "Tell": it tells a story of work already done and implies a binary merge yes/no question, but it doesn't really "Ask" for discussion in my opinion.

It's mentioned in the caveats list too that often such branches get PRed too late for meaningful discussion.

Something that I try to encourage are "very early PRs" (sometimes "as early as possible") and I'd call those "Ask" branches: some PR tools allow PRs as early as an empty branch. Other times it makes sense to open a PR after writing an initial bit of documentation about how it is expected to work, or a BDD style test, or even just adding a new Feature Flag ID to the list of system feature flags. People are often hesitant to do that, but I've seen some great discussions in branches worked on in this way. Most of the PR tools today have a good user flow for it allowing you to review/comment piece at a time over time as updates to the branch are made. Various ways to signal a re-discussion point in an ongoing PR depending on the tool (@-mentions, status changes, …). Some PR tools are better than others at even some "advanced" discussion flows like force pushing a clean branch to wipe existing code-specific reviews but leave past discussion history.

Adding a fourth option to the list Ship/Show/Tell/Ask maybe starts to make it maybe too many options (people love the rule of three), but I think that "open a PR early as a discussion container on approach/everything" is a sometimes very powerful tool.

Personally, I don't allow "Ship" changes, and try to default more to "Show". Not because of trust reasons, but for tooling ones: I like --no-ff merge commits as a strong log encoded directly in the git DAG of every integration, even "small/minor" integrations that look deceptively simple as a fast-forward. git bisect --first-person I can trust to run at the "integration log" level, for instance. git log --first-parent gives a high level integration log as a focused place to read branch histories. (And helps answer questions such as "when did this change make it into this branch?" versus "when was it written?") I think git works really well that way using the power of git's DAG to help, and I don't have to worry about accidental bad integrations/merges hidden away in rebases that I can't find because the tools don't record/log them (outside of deep dive internals such as reflog; I have been able to do some of that work when I needed to in a past life but I'd hope to never again need to).

Reply on news.ycombinator.com