Hacker Newsnew | past | comments | ask | show | jobs | submitlogin

The most productive engineers I work with just stack PRs. No diff is ever more than 200 lines changed. Sure, sometimes it's annoying to fix things in the stack, but I've found that keeping PRs small is an overall velocity (and safety) win in the long run.

Diffs that are > 500 lines either tend to sit in code review for several days or get the lazy accept and break something. If I absolutely cannot stack for some reason, I'll put up the PR, then set up time with some teammates to review it together. That way it gets merged that day and I avoid massive rebase conflicts (or I take their feedback and go back to the drawing board).

More info about PR stacking — I wish there was an easier git/GitHub interface for doing this:

- https://0xc0d1.com/blog/git-stack/

- https://kurtisnusbaum.medium.com/stacked-diffs-keeping-phabr...



I often do this because I move fast when I get momentum. But to be honest, even the best reviewers I’ve worked with find it a pain on their end. It doesn’t necessarily lead to better change sets or better reviews. And if there’s significant rework in a prerequisite, it’s significant cognitive overhead to keep the stack tidy. In a legacy codebase sometimes significant rework isn’t a few easily rebased/edited changes, sometimes it’s an entirely different effort, even if the downstream work still depends on it.

It’s good to do small iterative changes, but it’s not a silver bullet for review challenges.


Totally agree!

Re an easier tool, I'm one of the co-founders of Graphite (https://graphite.dev), we built this tool literally to help you stack PRs on GitHub (former FB'ers who missed a stack-based workflow). It does not require your team to change their workflow. If you're interested in giving it a spin, happy to let anyone here through the waitlist, feel free to just dm me on twitter (@tomasreimers) or email me at tomas@graphite.dev

If you're new to the concept of stacking, you can read our post on it here: https://graphite.dev/blog/post/DThX8ffP1gmxWJChEv0y

And a tutorial on how to do it here: https://graphite.dev/blog/post/N8SOs49y4bYdV1Vjf5qE


Crap! I just switched to a workplace that doesn't use GitHub. This looks amazing. Certainly better than the quirky collections of shellscripts I used before...


Checkout https://github.com/gitext-rs/git-stack/blob/main/docs/compar... (note, the tool hosting this page is not included but as the author).

As the author of git stack, with all relevant biases, I recommend

- git stack for automating what you are already doing

- git branchless for more power at the risk of incombatibilities because its only as good as the data fed to git hooks

- jj if your open to something very different


How is this different than selecting a non-"main" base branch for a PR on Github?


Watched the video, looks cool! Also saw someone else link Git spr - any thoughts on how they compare?


I totally agree that stack PRs are a good idea. It's how Git was meant to be used (see the Linux kernel's development process).

The problem is, they're a pain to work with in GitHub and so only a very small number of people end up learning how to work with them.


I've heard good things from our customers about spr for GitHub (https://github.com/ejoffe/spr). It extends git with useful commands and hooks directly into GitHub PRs. We'll be adding some direct support for the tool in Reviewable shortly.


> The most productive engineers I work with just stack PRs. No diff is ever more than 200 lines changed. Sure, sometimes it's annoying to fix things in the stack, but I've found that keeping PRs small is an overall velocity (and safety) win in the long run.

Sadly, the culture in some places mandates one pull/merge request per Jira issue/feature and therefore this isn't viable.

And even when it's not a top down decision, your colleagues might just say that actually they want to see all of the changes together once they're done because that's what they are used to, which will lead to you getting feedback pretty late in development and will probably make you need to refactor large parts of your code.


On GitHub, you can change the PR target branch to the previous PR in the stack to get an incremental diff. If you merge a PR lower in the stack, a PR pointing to that will auto update its branch.

With that in mind, you can change the top of the stack PR to the default branch to get a complete diff.


I know this, and I suspect the person you replied to also knows this, but I have never in my career as a programmer found anyone whose review I needed, who would have been comfortable doing this without my help.

I haven't always been working with "best and brightest" although that's not a dig, as the people I'm working with now are probably in the top 0.001% of Git users, nobody who does not work on Git as a discipline in and of itself can live up to this standard.


> I know this, and I suspect the person you replied to also knows this, but I have never in my career as a programmer found anyone whose review I needed, who would have been comfortable doing this without my help.

Well, i wouldn't say never, but you are spot on about the rest!

Many don't want to deal with the additional cognitive load. Of course, not everyone uses GitHub either: as far as i know, GitLab doesn't really have good UI for stacking merge requests (their terminology for the equivalent of GitHub's pull requests) and as a consequence you end up having to write comments about the order in which things should be reviewed.

Now, admittedly, i've personally done it in cases where it makes sense (e.g. separate bunches of refactoring or preparatory work from the introduction of new features), but it was definitely the exception, not the normal way of doing things.

I feel like if i started stacking my changes, some of the more established developers would furrow their brows and complain in the stand-up about needing to spend extra time on review, as they have in the past. Essentially, it's not enough for an idea to be good (or at least a good fit, given its drawbacks), but you also need to read the room!


I feel like if you `git rebase —-interactive` before you open a PR, and squash commits together that are a logical PR above (writing a good commit message), you get the same thing. Commits are the atomic unit of change and “stack” implicitly.


I think you are talking of the opposite of what the comment you are responding to are.

I think the comment you responded to is talking about deconstructing a single chunk of work into multiple commits and opening a PR for each one, rather than combining several units of work into one PR.


Its both the opposite and the same, depending on the reviewer. I have two colleagues, one prefers reviewing the full changeset and one prefers reviewing commit by commit.

The first might be pretty happy to see a bundle of smaller patches, and the second would be very upset because his workflow is now X times larger if they want to request changes.


Absolutely right.

I sometimes found myself going, "I could split this PR up into smaller ones but I'll skip it just this once" and it's been a mistake every single time.

Few things are a mistake so reliably as that.

These days, I always split it up until multiple PRs.


I try to manage most large change sets in the same way, never knew it had a name. For me, the main pain point that keeps me from using it all the time is working across forks, but that depends on how your organization handles code and might not be a problem for you. The same approach is also useful when you want to chunk off a part of a feature for review, but keep working on further functionality while the first part is in review.


I really like the idea, but I find that there is a big gap in the tooling to do this (and make it easy to review)

It would be nice to open just one PR with several sets of changes where you could provide context for each set of changes.

With GitHub you could do this by having a commit per each set of changes, but you can't add a description for each commit.

You end up having a PR that points to the previous PR, and then you have to decide if you run CI in that PR or just the last PR, and then having some merge issues. With all this hassle people usually just open a big PR with all the changes or decide against doing that small refactor (because it would have to do it in another PR and the hassle is not worth it), overall this ends up decreasing code quality in the codebase.


We once upon a time had a thing where we would have feature branches, and off of those we'd make task branches that would be merged (or a PR) back into the feature branch, then a final review and validation before the feature branch goes back into main. It kinda worked for us. This was nearly ten years ago.


This is the workflow I set up for my team. It's also what I use in my own projects. Why the past tense? Did the industry move on from this? Did you?


200 lines of python or 200 lines of Java?


Better question:

- 200 lines of statically typed, pure functional code with high test coverage

- 20 lines of code with I/O, parallelism, mutable state and/or interacting with external APIs

I find it way less cognitively draining to review a big chunk of blue ocean code with good patterns, than even small amounts of code which requires me to maintain any kind of state in my head, or try to think about the consequences of how this change interacts with the broader system. I'd say that some kinds of code are easily 10x more cognitively draining than others.


Doesn't matter. People are roughly as good at comprehending languages given the same number of lines.


I'd like to see someone try to review a 200 line regexp. They might take about the same amount of time to write per line, but they definitely don't take the same amount of time to read.


It sounds like you have an illegible, compact regex in mind. If you write regexes like that for others to read, I feel sorry for the other people.

If you write the regex like you would production code, i.e. with whitespace, newlines, and comments[1], I imagine 200 lines of regex would be about the same as 200 lines of any other type of production code, be it APL, Haskell, or conjunctive normal form.

I realise this is a bit of a circular argument: if you write the code such that 200 lines is about n units of review in a specific language, then 200 lines will be about n units of review in any language. But that's also kind of the point. There's a very specific logic density that you can accomplish for production code to be readable, and that results in about that density being reviewed.

[1]: In Python you can do this with the re.VERBOSE flag, in other languages you can do it by breaking the regex apart into separate variables that are then combined together.


Depends if the Java dev is stuck in Java 7 or has catch up with the times.




Guidelines | FAQ | Lists | API | Security | Legal | Apply to YC | Contact

Search: