Hacker News new | ask | show | jobs
by derefr 1318 days ago
So you're the one making me code-review 10000-line PRs because you just dumped your WIP branch — with three PRs' worth of code, plus formatting changes — directly into a PR, rather than factoring apart said WIP branch either during or after the fact.

The designed unit of a (distributed) git workflow is a patch — i.e. a locally rebase-squashed set of cherry-picked commits from development branches, with `git reset --soft` + `git add -p` (or even `git format-patch` + manual editing) used to prune the patch to a minimal size. Everything you do in your local repo should be with the intention of producing readable patches for code review (whether that patch is then done via PR or mailing list.) It's not about creating a pretty history (retroactive); it's about making it easy on the people who will discuss and reformulate your changes, one at a time, before accepting them upstream.

To be clear, you can do whatever you want when your git workflow isn't distributed (i.e. if you're committing to only your own private projects, not proposing changes to other people's projects.) But if your workflow isn't distributed, then why be opinionated about git? You can simplify your life at that point by using something with central-repo-oriented semantics, e.g. Subversion. There's no rebasing in Subversion. :)

2 comments

If you regularly need to review 10000 lines of code per PR your dev workflow is seriously broken. It‘s got nothing to do with git and its implementation‘s complexity.

Sometimes features do require large changes. But usually you can break a feature into different parts (e.g. database, backend, frontend) and merge them in separately.

Not regularly, no. But sometimes a feature change requires a dependent architectural change — a refactoring of the internal library code that the feature will be implemented into. Or sometimes the language of choice doesn't have a pre-commit-hookable CLI auto-formatter, only an IDE auto-formatter, and the dev editing a file triggers formatting changes to be applied that should have been done in a previous change. And sometimes, a dev thinks it's a good idea to change the representation and decoding logic for a data file or embedded data-structure literal at the same time that they're adding an entry to it (usually because they can't represent the added item's additional semantics without said change.)

> But usually you can break a feature into different parts (e.g. database, backend, frontend) and merge them in separately.

So you've already written all that code, because you couldn't get anything to "work" for end-to-end testing until you wrote all parts of it. The patchset as a whole is inherently large.

Now what? How do you "break [the] feature into different parts" when it's already all written and committed on a WIP branch?

That's right: cherry-picking and rebasing.

The GP is arguing against bothering with this process. Presumably because git-rebase(1) is unintuitive to them, and they don't realize that you should start this workflow with a copy of your branch, or a new branch with cherry-picked commits from your WIP branch, to guarantee non-destructive rebasing. Like making a copy of a layer in Photoshop. (Yes, you can always restore your branch from the reflog, so it's technically always non-destructive; but `git checkout -b foo` is something you learn in Chapter 1 of the Git book.)

Nobody should have long running WIP branches with these massive commits full of unrelated stuff. That’s an antipattern.