[HN Gopher] In Praise of Stacked PRs
___________________________________________________________________
In Praise of Stacked PRs
Author : kiyanwang
Score : 151 points
Date : 2022-07-24 16:12 UTC (6 hours ago)
(HTM) web link (benjamincongdon.me)
(TXT) w3m dump (benjamincongdon.me)
| somehnacct3757 wrote:
| I like to figure out what all my PRs will be during the planning
| phase, optimizing for reviewer cognitive load and incremental
| development of shippable features.
|
| I never need to stack PRs cuz I can work on the next one while
| the first one is under review and rebase once it's merged. I can
| see if the review phase is long at your company that this
| wouldn't work. But I prefer it if possible.
|
| One thing I hate about stacked PR delivery is ppl go dark for a
| month building this whole new world and if you have architecture
| concerns in the root PR they will resist them because it means
| rebasing and changing all the downstream PRs as well. Bad
| incentives all around.
| arxanas wrote:
| You might be interested in https://github.com/gitext-rs/git-
| stack, which perfectly implements your workflow where you only
| put up one PR but continue to work locally on the next commits.
|
| > ppl go dark for a month building this whole new world
|
| I think this isn't specific to stacking PRs. You can put 100
| commits in a PR and submit it at once, or you can make 100 PRs
| and submit them all at once, and have the same problems either
| way. Ideally, you put up your first few commits or PRs early so
| that they can get reviewed.
| TazeTSchnitzel wrote:
| Stacked reviews is a very natural pattern if you're using a
| system like Gerrit, where the unit of review is not a whole
| branch, but individual commits. Since they are commits, the
| reviews can have any kind of relationship that commits can have.
| I think it's a great system, including for some other reasons (it
| encourages amending and rebasing commits during the review
| process, which results in a very clean git history). After using
| Gerrit professionally for several years I wonder how I ever lived
| with GitHub-style pull requests.
| baq wrote:
| github pull request review process, UI and the whole experience
| is very much the worst thing about github for me. there's
| literally one feature that I like (C-g, suggest a one-click
| committable change in a comment) and everything else is better
| in gerrit.
| rsstack wrote:
| We switched to using https://graphite.dev/ after yet-another huge
| epic that caused a mess of PRs and one PR that touched 100+ files
| and included dozens of semi-related changed. It's been a blessing
| so far.
| zeroonetwothree wrote:
| Graphite is amazing! Would definitely recommend everyone try it
| at least :)
| rco8786 wrote:
| Hope you like merge conflicts and rebase hell as people start to
| comment and ask for changes
| nateroling wrote:
| I've played with this idea a bit. In my experience it felt like a
| hack to work around the fact that individual PRs took a while to
| get merged.
|
| An alternative solve is to work in a way that allows PRs to be
| merged more quickly, ie pairing, mobbing, or prioritizing getting
| reviews done asap.
| avl999 wrote:
| It is more about being unblocked than however long it takes for
| the PR to be merged in even if it is really fast. It is not
| always possible to pair/mod on a change.
|
| Getting folks to quickly review PRs is difficult as doing so
| breaks the flow state of other people and results in constant
| thrashing and context switching between the work they are doing
| are reviewing PRs.
|
| Even if the team is committed to quickly reviewing PRs, you are
| always going to have times when a bunch of people are in a
| meeting, while a couple are at lunch and a couple are knee deep
| in another issue and no one can look at your PRs for hours and
| you need to keep yourself unblocked.
| tennis_80 wrote:
| Yeah I agree. If you can merge things into a central branch
| quickly, it's much better.
|
| In my experience with this sort of process you spend quite a
| lot of time managing your different branches, especially once
| you start getting feedback and requests for changes. Then
| keeping everything in sync. Git helps make this quicker but
| it's still effort & cognitive load.
| knoebber wrote:
| I like to do this to address PR feedback, mostly because we use
| bitbucket, and it's UI for reviewing individual commits sucks.
| agentultra wrote:
| It depends on the project being worked on. If the main branch
| changes often enough that you need to merge master before you
| merge the branch you'll spend a lot of time propagating changes.
|
| And then there's the problem of _large codebases_. Without a
| really good cache strategy for builds all of the branch switching
| and merging will cause a lot of rebuilds. For some languages and
| large projects that can be 20+ minutes for a fresh build on each
| switch. That's enough time to get distracted by emails or hacker
| news.
|
| I'm a big fan of small commits, often and breaking down changes
| but it does require a large amount of automation and tooling to
| support well.
| jackblemming wrote:
| There's so many articles on trivial things like this and using
| pretty syntax and less articles on things that actual matter:
| architecture and design. It's a bit disheartening to hear
| software engineers argue of if you should put periods in commit
| titles or if so and so syntax is more readable.
| tunesmith wrote:
| How would you align this with Jira stories? Our team tries to
| maintain a one-PR-per-story flow. Partly for QA purposes, we
| don't want multiple QA cycles per story.
|
| Of course, we still have giant PRs that touch 50-100 files and
| take forever to code review.
|
| And yet, for those stories in question, they do make it to prod
| faster than if they were broken apart into multiple 1-2 point
| stories.
|
| I imagine as always the answer is "invest in better tooling".
| More robust automated tests, push-button millisecond-deployments,
| etc...
| arxanas wrote:
| Basically: you don't :)
|
| I've only ever found this workflow to work if you can attach
| multiple commits/code reviews to single higher-level work items
| in your project management software. There's simply too much
| overhead if you try to correlate project management items one-
| to-one with commits/code reviews.
| krooj wrote:
| I've tried this method of taking a large change and breaking it
| up into smaller, more easily digestible PRs, and it didn't work.
| Unless you're working with folks that are directly invested in
| the change, you'll very quickly discover that people have
| memories worse than goldfish. So, it's a lose-lose situation:
| can't really do stacked PRs and large PRs will let bugs and other
| defects through.
| avl999 wrote:
| There is a tradeoff... smaller, digestible PRs result in
| thorougher and faster reviews but it gives up the "1000 feet
| view" of the larger feature as far as the reviewer is
| concerned.
|
| Large PRs preserve the larger "1000 feet view" of what you are
| working on but are likely to be slower to get responses on and
| most likely less thorough thus a larger chance of things being
| missed.
|
| Almost everyone I've worked with prefers smaller review so I
| just accept that trade-off of 1
| culi wrote:
| Can't this easily be resolved by just using feature branches?
| Make your small PRs into the feature branch but then you still
| have the ability to compare the branch to main as a whole and
| get that wider context
| Sol- wrote:
| We have some people in our company that use tools like Graphite
| to implement this pattern on top of Github and I don't know if
| I'm a big fan.
|
| Maybe just a matter of developer discipline, but in my experience
| people tend to create large stacks of 3+ PRs that then take a
| while to resolve. Yeah sure, without these tools the code would
| also exist somewhere, but at least you don't have your pull
| request list full with PRs that aren't yet ready because the
| upstream PRs haven't been approved yet. You anyways have to (or
| should) review things stricly serially, so the additional PRs
| existing are not super useful either. Maybe as context for the
| reviewer to see the future work.
|
| Also for some reason these tools seem to encourage people to put
| unrelated changes in the stack that could be based directly on
| master. Probably because they are anyways working on their stack
| and integrating some unrelated fix they just did into the current
| stack is easier than rebasing/changing your current context. But
| that's just a minor pet peeve.
|
| That said, I would still like a tool that lets me manage stacked
| branches just for myself. Not exposing them as Github PRs or so,
| but to organize my work into different branches. Executing a
| chain of rebases of branch N -> (N-1) can get quite annoying
| manually. Probably some arcane git magic to (interactively)
| rebase branch N->(N-1)->...1 in one command exists already.
| arxanas wrote:
| > Probably some arcane git magic to (interactively) rebase
| branch
|
| There is not really a command for that yet, short of adding a
| bunch of `exec` steps to your interactive rebase manually. See
| https://news.ycombinator.com/item?id=32217204 for an upcoming
| command.
|
| You might enjoy using https://github.com/gitext-rs/git-stack,
| which specifically tries to let you manage stacked branches
| locally while not exposing tons of PRs to your coworkers.
|
| git-branchless itself also lets you manage stacked branches in
| various ways. For example, you can do `git checkout <branch>`,
| `git commit --amend`, and then `git restack` to rebase all the
| descendant branches sensibly. You can use it on the local side
| of things only and then use Github PRs as normal.
| wakeupcall wrote:
| Also a big fan of https://gitlab.com/wavexx/git-assembler
| (and previously topgit). Works with the basic idea that you
| can rebuild branches by combining, merging or rebasing
| automatically on top of others. I frequently use this to
| build a local branch which is an amalgamation of the main
| branch + required patches so that I can work (and later
| submit) on a clean branch without being blocked.
| doktorhladnjak wrote:
| We use GitHub Enterprise where I work now. I do sometimes do
| stacked PRs but GHE does not make it easy. The rebasing and merge
| conflict resolution can be a headache although git rebase --onto
| helps a lot.
|
| I previously worked somewhere that used Phabricator. Its "stacked
| diffs" worked great. I'd use it all the time when working on
| complex, multipart changes.
| arxanas wrote:
| Try out the linked tool git-branchless
| (https://github.com/arxanas/git-branchless, I'm the author). It
| should help you restore Phabricator-like workflows. In
| particular, check out the `git sync`, `git restack`, and `git
| move` commands for handling rebasing and conflict resolution.
| davvid wrote:
| There is active work related to teaching "git rebase" to natively
| support stacked branches in the Git core currently being worked
| on by Derrick Stolee [1].
|
| If you "stack" your changes across multiple inter-dependent
| branches it looks like "git rebase" is going to learn how to
| update related branches using a new "update-ref" command
| (alongside "squash", "fixup", "exec", etc) that gets activated
| automatically through a "git rebase --update-refs" command-line
| flag and config option.
|
| (This is from Derrick, not me)
|
| """ This is a feature I've wanted for quite a while. When working
| on the sparse index topic, I created a long RFC that actually
| broke into three topics for full review upstream. These topics
| were sequential, so any feedback on an earlier one required
| updates to the later ones. I would work on the full feature and
| use interactive rebase to update the full list of commits.
| However, I would need to update the branches pointing to those
| sub-topics.
|
| This series adds a new --update-refs option to 'git rebase'
| (along with a rebase.updateRefs config option) that adds 'git
| update-ref' commands into the TODO list. This is powered by the
| commit decoration machinery.
|
| """
|
| This is under active development (I don't believe the topic has
| been merged yet) so it's still open for feedback and refinement
| on the git development mailing list. Thanks Derrick!
|
| [1] https://public-
| inbox.org/git/pull.1247.git.1654263472.gitgit...
| timhh wrote:
| That sounds great! I have partly solved this issue in my
| autorebase tool (https://github.com/Timmmm/autorebase) - it
| basically rebases every branch, and fixes the commit time so
| that stacked branches get preserved even after a rebase just
| because the hashes all match properly.
|
| That obviously doesn't work if you modify or drop any of the
| commits, so this option is very welcome!
| dcow wrote:
| I'm going to be a little crass for a moment: people don't know
| how to use their tools! Why are we even talking about "stacked
| PRs" and "branchless" workflows?! It's clear the author is
| documenting their git learning process and that's great, really.
| _But I'm just surprised how pedantic people get about forcing
| others to use a tool in a way that caters to their own limited
| understanding of it._ Or, even worse, caters to limitations in
| the 3rd party UI being used to interface with it (ye ol' git vs
| GitHub problem). Learn your tool and then you can worry about the
| higher order things that are supported by it.
| dcow wrote:
| I feel similar about languages and style-guides, but I didn't
| want to conflate the two in my previous comment. Unless you are
| a royal newb and just learning to write code (which is fine,
| everybody has to learn), you're probably writing code in a way
| that best expresses your intent or goal given your
| understanding of the language. Let people do that! If you know
| the language you can handle encountering different styles and
| structures. Otherwise you're just forcing your preferred subset
| of hoe the language can be used to express things. I just don't
| like regression to the mean.
| kqr wrote:
| This is something I've come to realise as I've matured as a
| developer. I just don't care what style you write in, if you
| don't use all the syntactic sugar, etc. I care where you draw
| your module boundaries, which units depend on which, and if
| you've handled edge cases, and so on. That's what's going to
| matter in the end. That's where I'll have to spend a lot of
| time figuring stuff out. Not syntax.
| dcow wrote:
| Exactly. I've seen style-perfect lint-free "write once"
| spaghetti code that drops edge cases and error conditions
| left and right, and I've seen "ugly" but clean code that
| needs no introduction and works perfectly to boot. I worry
| that auto-formatters (not to discredit their upsides) do
| sometimes give people a false sense that they're writing
| good code.
| seoaeu wrote:
| This comment would be more compelling if it gave any hint about
| what aspect of the tooling the author didn't understand or what
| they should be doing differently.
| dcow wrote:
| I'm not talking about the author, specifically or criticizing
| them directly. I'm lamenting how often I encounter people
| putting intense amounts of effort into arguing about how git
| _should_ be used. My hypothesis is that if people understood
| their tools better before getting all evangelical about
| forcing their entire eng team to work in their style, others
| wouldn 't have to spend so much time trying to convince them
| that other styles are exist and are valid.
|
| Compare with a construction site: there are obvious uses for
| tools and there are less obvious uses and I haven't been on a
| single one where tools are only used specifically in one way
| for the entirety of the project. They're used dynamically by
| different people with different experiences in order to
| complete the project. Nobody forces other workers to use a
| specific grip when holding their hammer...
| johnfn wrote:
| I'm not understanding how stacking PRs is inconsistent with
| knowing git well.
| dcow wrote:
| That's not what I'm saying at all. My comment is a meta
| comment about why we even need to write grand defenses of
| these practices in the first place. I've been "stacking
| PRs" forever... when it makes sense to do so. There's no
| golden git usage pattern. I've seen the industry all the
| rave about gitflow. And then it wasn't cool anymore with
| tags being the preference. Now it's cool to just forgo
| branch names entirely? I've watched teams fight over
| whether every PR should be exactly one commit because
| they don't realize that you don't always have to squash
| merge if you want two unrelated changes pulled in at the
| same time--just keep the commits clean and revert doesn't
| care. My point is that if you know how to use the tool
| none of this really matters and all seems quite
| distracting. I no problem with people sharing what works
| for them, that's great (as is this article, abstractly).
| morelisp wrote:
| I can't be certain what dcow is referring to but there
| are a few things that point to confusion, or maybe an
| overambitious attempt at simplification.
|
| For example, _Stacking PRs keeps the author unblocked.
| Authors don't need to wait on a particular change to be
| merged before starting to build something on top of those
| changes._
|
| This will fundamentally be up to the author's git skill
| whether or not they are presenting the PRs to reviewers /
| mergers as stacked. If they're skilled git users there's
| little to no cost presenting them one at a time and
| keeping the not-yet-PRd branches fresh. If they're not
| skilled git users, they have no hope of managing multiple
| PRs effectively only be virtue of presenting them all at
| once.
|
| Or: _Since stacking PRs allows you to create a DAG of
| dependent changes, this natually allows you to manage
| code changes that need to be submitted in a particular
| order._
|
| Assuming this is in a single repository, a fast-forward-
| only commit policy alone ensures this.
|
| Or: _stacked PRs use branches, and can have multiple
| commits in a single atomic change; stacked commits use a
| single commit as the unit of atomic change._
|
| Stacked PRs usually use branches, but stacked commits
| also still use at least one branch and could have more.
| In both the commit is the unit of atomic change, because
| that's what a commit is.
|
| I'll also add I find the entire language around
| "branchless" workflows in git confusing, not just from
| this author. There is no such thing; considering one
| special branch as "not a branch" or your local and remote
| and someone else's remote as the "same branch" just
| because they have the same name is a holdover from
| older/other VC tools. We don't do any new git users a
| pedagogical or practical favor by clinging to that view.
| arxanas wrote:
| git-branchless author here. By "branchless", I mean
| literally using detached HEAD as the primary state of
| development. If you're using tools like Gerrit or
| Phabricator, you never have to explicitly make a branch
| to get your code merged. If using GitHub, then branches
| are unavoidable, but it can be nice to do branchless
| development as part of rapid prototyping (see
| https://github.com/arxanas/git-
| branchless/wiki/Workflow:-div...).
|
| Another term you could use is "anonymous branching". This
| is not technically accurate in the above workflows, but
| it captures the essence pretty well in Git, more so than
| "branchless".
| morelisp wrote:
| But when you call it "anonymous branching" you lay bare
| that the only advantage is that you don't need to name
| your work "branch", and meanwhile you have a workflow
| that's needlessly incompatible with most other git
| tooling.
|
| In particular, since this is the one I see usually called
| out as a benefit of branchless:
|
| _Stock Git does not have good ways of rebasing a
| sequence of branches._
|
| A sequence of branches can be rebased by rebasing (or
| otherwise rewriting) the longest one (the only one you'll
| need locally) then pushing the individual commits in the
| current branch to the remote under any relevant branch
| names. This doesn't take zero time, but with good git UIs
| it will take less time than remembering `git move`, and
| it's not especially hard to do with the stock CLI either.
| arxanas wrote:
| > But when you call it "anonymous branching" you lay bare
| that the only advantage is that you don't need to name
| your work branch
|
| Sure, I'm only responding to what you were saying about
| "There is no such thing; considering one special branch
| as 'not a branch'". There is such a thing in that there
| is no branch involved in the detached HEAD state. It's
| not some kind of Git misunderstanding. I think you might
| be referring to trunk-based development and always
| building off of the remote main branch instead of having
| your own local copy, which is unrelated to being
| "branchless", for the reasons you stated.
|
| For many people (particularly those on Github!), a
| branchless workflow won't help, so you're free to not use
| it. In my opinion, it's a workflow that is better
| compatible than stock Git with code review tools like
| Gerrit and Phabricator.
|
| I personally argue that anonymous branching is useful
| even in some branch-based workflows. Mainly, if Git
| branches are so lightweight to use, why do we also use
| the command `git stash`, instead of just always creating
| a new branch for our temporary work? One benefit of
| anonymous branching is that it consolidates these
| workflows in a convenient way. Some people don't stash
| changes or feel that branching in those cases is
| heavyweight, so anonymous branching doesn't help them at
| all.
|
| > then pushing the individual commits in the current
| branch to the remote under any relevant branch names
|
| If I'm understanding correctly, every time you rebase the
| longest branch, for each commit in the branch, you would
| manually run e.g. `git push <commit> origin:my-branch-
| name`? That seems like it would take a lot of time to me.
| Is the tacit assumption here that you don't have a lot of
| commits in your branch, so this doesn't take a lot of
| time?
| morelisp wrote:
| > There is such a thing in that there is no branch
| involved in the detached HEAD state.
|
| But at the very least there's still an existing remote
| branch, which is the ultimate merge target, for example -
| perhaps even multiple. Since we're talking about
| coordination, there's also the actual state at the remote
| vs. my view of the remote vs. my teammates' views of the
| remote. But we don't think about this too much, because
| we can synchronize easily as long as the remote branch
| has a name. Taking the name off the branch makes it more
| difficult to do anything with someone else's work other
| than merge it, e.g. pass a changeset back and forth or
| hand over a half-complete task.
|
| > why do we also use the command `git stash`, instead of
| just always creating a new branch for our temporary work?
|
| I have no idea actually, because I don't. I haven't run
| git stash manually since I learned about autostash, and
| even before that it wasn't for temporary work but for
| changes I decided I wanted somewhere else only after
| writing them. Temporary work mostly does get a branch
| (usually as a new commit on top of my current local
| branch).
|
| > If I'm understanding correctly, every time you rebase
| the longest branch, for each commit in the branch, you
| would manually run e.g. `git push <commit> origin:my-
| branch-name`?
|
| Close, except I'd scroll down the list of commits and
| type `P o RET TAB branch-name` (or something to that
| effect, it'd be slightly different if I have several
| remotes). This would take perhaps ~0.5 seconds per branch
| and not require me to context switch to a terminal.
|
| It does take longer for people using less powerful
| clients, but virtually all do provide some way to do it
| even it means a few right-clicks on a menu and clicking
| some "OK" buttons, and there's value in everyone using
| analogous operations. And the people using those clients
| usually can't reliably recover from a large class of "git
| broke" mistakes, assistance from git-branchless or not,
| so handing them a CLI and a prayer is out of the
| question. And even the "long version" is pretty
| negligible (like, maybe 10 seconds per branch?) compared
| to everything else you should do to make your changeset
| approachable for review.
| arxanas wrote:
| Which Git client do you use, and also, how do you
| remember all the remote branch names?
|
| > And even the "long version" is pretty negligible (like,
| maybe 10 seconds per branch?)
|
| I notice that mainly the differences in opinion with
| regards to workflow is disagreement about "how long is
| too long" for various operations :)
| morelisp wrote:
| Magit, and the remote branch names are visible in the log
| view and easy to enter via tab completion. (We're still
| talking about stacked PRs right? Median stacked at any
| given time is probably 2 and p90 is maybe 4? Max I've
| ever seen is ~10 and that was quite a fucked up situation
| for plenty of other reasons.)
|
| 10 seconds would certainly be too long for me. It is
| obviously not too long for them, or they'd learn some
| keyboard shortcuts to get the easy 5 out of the way to
| begin with. But regardless, it's all pretty much dwarfed
| by things that need actual brainpower like re-reading my
| commit messages for grammatical errors / broken links /
| etc.
| arxanas wrote:
| I see, thanks for the information. I often stack 10+
| commits, so I don't think your workflow would be
| reasonable for me. (For example, here, I have 11 finished
| commits and 3 in progress:
| https://github.com/arxanas/git-branchless/pull/451 -- but
| I don't actually use stacked PRs on Github, particularly
| if no one is reviewing my code. Most of my stacking is at
| work with Phabricator anyways, which handles this
| better.)
| morelisp wrote:
| It's only necessary to push ten branches, one per commit,
| if you intend ten separate, let's say, "review events"
| though. For that example we would have fewer than that
| nor would we want that many. I do stack 10+ commits
| fairly regularly but that may be only 1-2 reviews.
|
| (I'm not familiar with Phabricator, only Gerrit and
| GH/GL, as well as some more manual Workflows.)
| arxanas wrote:
| Right, in this case, I would prefer 10 separate review
| events. Other than for code review purposes themselves, I
| would want CI to run on each of these commits
| individually, rather than only on the result at the end.
| djur wrote:
| This isn't "how to use a hammer", though, it's more like
| "how to evaluate the completion of work", which is
| definitely something working teams have to agree on. git
| itself is almost irrelevant to the discussion, other than
| its native feature set having some influence on the
| options.
| morelisp wrote:
| If this was just about team review workflow I wouldn't
| expect to find "easier to rollback" and "don't need to
| wait on a particular change to be merged" to be on the
| list of upsides, as these are properties of the
| repository structure and not the review workflow.
| surfmike wrote:
| Graphite (https://graphite.dev/) has been a good tool at our
| startup to manage stacked PRs, I recommend checking them out.
| jasonhansel wrote:
| Why not just break up your change into small components, then
| merge each one into master separately, hiding the change behind a
| feature flag until the whole thing is done?
| travisb wrote:
| Your suggestion comes after the changes are merged. That is,
| you wouldn't normally merge anything to master without having
| it reviewed.
|
| Stacked PRs is _how_ you can break up your changes into small
| components to merge more quickly and block development less.
|
| With stacked PRs the changes are broken into smaller pieces
| because a later PR can depend on an earlier PR. This means it
| can be developed while earlier PRs are still under review. It
| also means each review is smaller and therefore likely to
| complete more quickly.
|
| Stacked PRs can also work like patch series in the Linux Kernel
| development where a large change is broken into smaller
| independent changes done in the logical order, even if their
| order of discovery was probably reverse. For example, if you
| discover you need a refactor or bug fix while halfway through
| developing a feature. In that case you simply insert a PR
| earlier in the stack/graph which contains just that isolated
| fix.
|
| In your feature flag example, a common patten would be to have
| a dozen or so stacked PRs. The early ones do necessary
| refactors and bug fixes. The middle few add the feature flag
| and code. The last make the feature flag the default and delete
| the now obsolete !feature path. Then the prereqequites (which
| you didn't discover until half-way through) can be reviewed and
| merged quickly. The meat of the feature is broken up into
| easily digestible pieces for quick review, but reviewers can
| get a sense of the entire feature by considering the PR series
| as a whole; this is especially useful where one PR adds some
| infrastructure that a later PR uses but both PRs together would
| be too large to effectively review. As PRs are ready to merge,
| they can. Often the last couple of PRs which make the feature
| the default and delete the obsolete code do not merge for quite
| some time, but they were written when everything was still
| fresh in the developers head.
| arxanas wrote:
| I am the author of the linked tool git-branchless. Happy to
| answer any questions about stacked PR workflows here or on GitHub
| discussions https://github.com/arxanas/git-branchless/discussions
| or Discord https://discord.gg/caYQBJ82A4.
| ridiculous_fish wrote:
| Plugging my own tool: if you like to cultivate a stack of
| commits, you'll know how awkward git makes it to edit previous
| commits. With git-prev-next you can run 'git prev 3' and then
| 'git commit --amend'.
|
| https://github.com/ridiculousfish/git-prev-next
| maxekman wrote:
| `git rebase -i origin/main` (or HEAD~N or any other base) with
| the e/edit action on selected commit will do the same with only
| built in functionality.
| ridiculous_fish wrote:
| Yep, and git-prev-next is built on interactive rebase. It's
| just a lot faster to run `git prev`, compared to `git rebase
| -i HEAD^^` and then muck around in a text editor.
| arxanas wrote:
| The mentioned tool `git-branchless` (I am the author) also
| supports `git prev 3` followed by `git commit --amend`. See
| https://github.com/arxanas/git-branchless/wiki/Command:-git-...
| fallingknife wrote:
| This just shows that people will go to any length to
| procrastinate PR reviews. The truth is that to be an effective
| team, reviews need to be highest priority because they are
| blockers. There is just no way around it.
| macintux wrote:
| Any categorical assertion like that feels wrong. Not all
| projects, all teams, all problems are the same.
| loloquwowndueo wrote:
| Bzr has a pipelines plug-in which does basically this. Branches
| can be stacked on top of each other forming a "pipeline", and
| propagating changes from one branch to the dependent / subsequent
| ones is achieved with one command, "bzr pump".
|
| Bzr has no concept of rebasing, so what you get is a simple
| merge, but with very little work on your part.
| is0tope wrote:
| I've usually kept a rule that you should avoid stacking, and if
| you must only one level deep. The fact that you have to stack in
| the first place typically suggests that PRs aren't being merged
| fast enough. Stacking in my personal experience usually leads to
| merge conflict hell as changes and PR suggestions get merged
| underneath you.
| ignormies wrote:
| Something I often use stacked diffs for is deprecation ->
| removal flows.
|
| 1. Deprecate old feature + add opt-in support for replacement
| 2. Make replacement default with opt-out for old pattern 3.
| Completely remove old feature and the opt-out functionality
|
| I can write the entire stack of diffs upfront, have them
| individually reviewed but still linked, and ensure they're
| merged in the correct order.
|
| The bottleneck for merging isn't in the review process, but in
| the deprecation. It wouldn't make sense to land all three of
| these changes as fast as review/merge would allow; that would
| skip the deprecation period.
| wpietri wrote:
| Ooh, good point. I'm suspicious of stacked diffs generally,
| but this seems like a good example where we truly don't
| expect to learn anything after the initial deploy.
| kqr wrote:
| But is it really effective use of your time to make changes
| to the code that you know won't be relevant for weeks or
| months? You could spend the same time on other changes that
| would start earning you money tomorrow, instead.
| tablespoon wrote:
| > But is it really effective use of your time to make
| changes to the code that you know won't be relevant for
| weeks or months?
|
| It totally is, because it doesn't wastefully discard the
| mental context needed to make the follow on changes. Task
| switching unnecessarily incurs significant costs.
|
| > You could spend the same time on other changes that would
| start earning you money tomorrow, instead.
|
| Maybe in some bare-bones startup context that can't afford
| to think beyond next week, but most organizations aren't
| like that.
| ignormies wrote:
| Having the change implemented from start-to-finish
| demonstrates a finished and thorough design. It also allows
| review to happen with the complete change at the forefront
| of the reviewers' minds.
|
| Kicking the completion of the implementation of a change
| you've started landing to an unspecified future date
| indicates poor engineering rigor imo. It's just begging for
| the change to perpetually be half-migrated and never
| finished.
| majormajor wrote:
| Sometimes?
|
| Consider also the stakeholder who gets annoyed whenever the
| dev team wants to work on something that would take longer
| than a week to turn around, and limits the things they'll
| ask for to those estimated at a week. So bigger things can
| never get done at all, and you'll just be looking for a
| local maxima instead of having the chance to make more
| significant changes.
|
| Sometimes it's worth it to prep and clean up as you go.
| Knowing when it's worth it and when it isn't is one thing
| that makes some devs more valuable long-term than others.
| chrisseaton wrote:
| > The fact that you have to stack in the first place typically
| suggests that PRs aren't being merged fast enough.
|
| Unless PRs are merged instantly, I'm always going to be waiting
| after one PR is opened, before I can work on the next, unless I
| stack, aren't I?
|
| Is your definition of 'fast enough' instantly? If not, how does
| this work?
| delecti wrote:
| If your second PR is ready before the first PR is merged,
| then two of the likliest outcomes are that either PR reviews
| are taking too long, or the second PR is small enough that it
| could have just been part of the first. Alternatively, the
| review is taking a long time because the first PR was
| bad/controversial, in which case the assumptions of the
| second PR might need to be reevaluated anyway.
| dairylee wrote:
| Neither of those cases need to be true for a second PR to
| be ready before the first has been merged.
|
| For example you do your first PR, mark it ready for review.
| While doing it you notice there's some refactoring you
| could do to some tangentially related code. It's very
| conceivable that the second refactoring PR could be ready
| pretty quickly.
| wowokay wrote:
| No? You can create a new branch and start working on the next
| thing. Why would you be waiting on your PR to complete unless
| you didn't split your work correctly.
| chrisseaton wrote:
| What if the next thing depends on the previous thing?
| fuzzy2 wrote:
| Then your new branch starts at the tip of the previous
| one.
|
| You can (/will have to) rebase later.
| chrisseaton wrote:
| I thought that was what stacked PRs are - maybe not?
| majormajor wrote:
| Two top-level scenarios here:
|
| * you're working on the same part of the code
|
| * you aren't working on the same part of the code
|
| The second scenario is common but also trivial here since you
| can just have parallel branches going, so I'm gonna assume
| more the first - working on something that's building on top
| of what you just put up for review.
|
| Let's say the review is done in 2 hours. If you're already
| done with the followup, IMO you may be erring too far on the
| side of "small PRs." If you aren't, you just rebase on top of
| whatever changes had to be made, if any, to the first one,
| once it lands on the primary branch.
|
| If, on the other hand, the review isn't done for 2 days...
| then that's a PR turnaround time problem for sure.
|
| But I strongly disagree with the people saying "multiple
| dependent PRs suggests the work wasn't split up properly" -
| there's nothing worse than a mega-PR of thousands of lines
| for the sake of doing a "single feature" all in one shot vs
| having to possibly pause and rebase periodically after
| review. It's _even more painful_ when this mega-PR requires
| fundamental changes that could 've been caught earlier, but
| now will take longer to adjust, and then will stay open for a
| while and likely result in merge conflicts as a result.
| californical wrote:
| You can branch off of a PR, but someone should review and
| merge your first PR before your second is ready to be up in a
| PR again.
|
| Also, trying to make units of work so that they don't need to
| overlap like that can be useful too
| nightpool wrote:
| This is why I think it's really really important that all PR
| reviews be _synchronous_ , so that there's never any time
| spent twiddling your thumbs or context switching onto another
| change. Also it just makes it much easier to review a PR when
| you can sit down and actually talk about it in real time with
| the author, rather than having the ping messages back and
| forth interminably until you reach an agreement
| wizofaus wrote:
| In one of my first jobs, code reviews were done exactly
| like this, with the the reviewer (my boss) and I sitting
| together at one computer going through the changes. It
| definitely has benefits but it's still important to ensure
| recommendations/concerns etc. get written down and doesn't
| necessarily help if there's a need to go away and make
| significant changes based on the outcomes of discussions.
| But while the back & forth discussion that often occurs
| during review can benefit from being synchronous, it might
| still be some time before your coworker has available time
| to participate in such a session, so it's still likely
| you'll need something else to work on in the mean time.
| nightpool wrote:
| > and doesn't necessarily help if there's a need to go
| away and make significant changes based on the outcomes
| of discussions.
|
| See, I disagree, because this is absolutely the place
| where it helps the most--you can now go away and keep
| working on the same task without having to context switch
| to anything else or remember where you were or what you
| were working on. So you're never in a state where you're
| blocked and can't work on your ticket, and you don't have
| anything else to do or have to pick up another ticket and
| start learning about a whole completely separate problem.
| (And yes, definitely everything still needs to be written
| down, it's important to walk away knowing which changes
| you need to make, and why!)
|
| > it might still be some time before your coworker has
| available time to participate in such a session, so it's
| still likely you'll need something else to work on in the
| mean time.
|
| In teams I've worked on, the expectation is that an
| engineers highest priority is always unblocking another
| engineer, so this very rarely happened. Unless they had
| an interview to go to or some kind of meeting--and in
| that case, you could always just ping somebody else.
|
| Obviously it's a style of work that relies really heavily
| on everyone sharing the same timezone and work hours, but
| it works really well to eliminate time lost to context
| switching and minimize the amount of time engineers spend
| blocked waiting on someone else.
| wizofaus wrote:
| But if you're expecting another dev to always be
| available to help you with a review, then _they 're_ the
| ones having to "context-switch", interrupting their own
| work. The reality is context-switching is something that
| comes with the territory if you're working as part of a
| team developing software. Which isn't to say there aren't
| opportunities to minimise the disruption it causes, but
| the idea that you can more or less eliminate it seems
| like wishful thinking. Perhaps there's a case for
| minimising it for more junior devs that aren't as capable
| of dealing with it, though it's equally possible younger
| brains are better at it!
| nightpool wrote:
| I think maybe we're using different definitions of the
| term "context-switch". Certainly it's an _interruption_ ,
| but I don't really think that sitting down with another
| engineer to do a focused code review where they've
| already written out a PR description and thought hard
| about the problem is comparable to starting a brand new
| branch or picking it up after a while away and trying to
| juggle 2, 3, or 4 in-progress tickets.
| wpietri wrote:
| One way this can work is pair programming. Especially if you
| also practice frequent pair rotation, you can get plenty of
| eyeballs on code in a timely fashion. That's my preference.
|
| Another way is that you next work on something different
| enough that it doesn't need to stack. E.g., reviewing pull
| requests.
| perrygeo wrote:
| By pinging people on slack and interrupting them for a review
| asap? Or context switching to an unrelated part of the code
| in parallel while you wait? Or reducing the frequency of
| reviews and make a mega-PR every couple weeks?
|
| I've seen no solutions, only tradeoffs but I'm curious if
| anyone has a tried and true way to avoid this traffic jam
| scenario.
| wowokay wrote:
| Yes, complete your story in a single PR, if the next story
| is related and requires your changes your work wasn't split
| up properly, plus in that scenario there is a good chance
| other devs have to many dependencies on other devs work
| which is a recipe for disaster!
| BerislavLopac wrote:
| My personal rule of thumb is to rebase the working branch as
| soon as the main has been updated; or at least merge main if
| the changes are too complex.
| zmj wrote:
| I stack PRs when I'm working on a piece of new code, and in the
| process discover one or more refactors that simplify the diff
| for the new code. I wouldn't start at the refactors and then
| wait to proceed - there's a chance they are dead ends until I
| know exactly what the new code needs.
| arxanas wrote:
| If you work at asynchronous/remote work company, i.e. your
| coworkers are in different timezones and can't review
| immediately, what else are you going to do? Put out exactly one
| code review per day until your feature is fully merged? Some
| things like refactoring changes can be reviewed and committed
| individually, but lots of feature work is fundamentally
| dependent on the previous work.
|
| Stacking PRs is like pipelining for CPUs. It's efficient under
| the hypothesis that there aren't too many invalidations/stalls.
| The linked tooling `git-branchless` (I'm the author) is aimed
| at reducing the impact of such an invalidation by significantly
| improving the conflict resolution workflows.
| isoos wrote:
| > what else are you going to do?
|
| Depends on the team and the product. My personal approach is
| to have 2-3 larger things to work on, so while I wait for
| reviews on one, I can switch and work on the other. This
| usually means minimum 1-2 weeks of planned work, sometimes
| even more, without being blocked on reviews. If everything is
| blocked, then it is time for some code health cleanup,
| refactoring and fixing those TODOs that are just lingering
| around, and also nudging the reviewers...
| dan-robertson wrote:
| One advantage of 'stacking' is breaking up review into more
| logical units, eg
|
| 1. Introduce new test exhibiting bug
|
| 2. Introduce bug fix and update the test
|
| If 1 and 2 are reviewed together you have less evidence that
| the test actually shows the bug being fixed.
| dtech wrote:
| You can still have them in 2 commits, and configure your CI
| to build both of them, and the first 1 should fail.
|
| We actually have a rule that there must be 1 commit just
| introducing a test that fails on CI for bugfix PRs.
| chrisweekly wrote:
| That's interesting, explicitly to require that bugs be
| reproduced in CI. It makes sense in theory, but in praxis
| (IME) CI systems tend to be overtaxed / underprovisioned -
| meaning this extra burden might be questionable. /$.02
| nemetroid wrote:
| The same burden is present when splitting the two commits
| into two PRs.
| YZF wrote:
| Ideally your CI workflow prevents merging something that
| breaks the tests. Also now if the first review merges but the
| other doesn't for some reason you've broken everyone else
| which is bad.
|
| EDIT: at least in rebase workflows which is what I'm used to.
| I guess in merge workflows this works.
| cornel_io wrote:
| But once you merge 1, your test suite is broken, and
| many/most companies wouldn't allow that.
| dan-robertson wrote:
| Yeah there's two solutions:
|
| 1. Merge #2 into #1 then merge the result, but this can
| obscure your history
|
| 2. Don't make the test fail. Eg expect the incorrect
| behaviour and leave a comment explaining why it is
| incorrect and what the correct thing should be.
| mrkeen wrote:
| > The fact that you have to stack in the first place typically
| suggests that PRs aren't being merged fast enough
|
| Yes. Whenever you inspect a proposed solution, you should
| hopefully find the problem that it solves.
| wpietri wrote:
| For sure. One of the things I learned from the Lean folks was
| to look for inventory; it's one of the 7 Wastes. [1] In
| physical manufacturing, it's pretty obvious, because it's
| physical stuff sitting around on the journey to becoming
| actually useful.
|
| With software it can be harder to notice because you don't have
| to make room for it. But in essence it's the same deal; it's
| anything we have paid to create that isn't yet delivering value
| to the people we serve. Plans, design docs, and any unshipped
| code.
|
| There are a lot of reasons to avoid inventory, but a big one is
| that until something is in use, we haven't closed a feedback
| loop. Software inventory embodies untested hypotheses. E.g., a
| product manager thinks users will use X. A designer thinks Y
| will improve an interface for new users. A developer thinks Z
| will make for cleaner, faster code.
|
| Both large pull requests and stacked pull requests increase
| inventory. In the case of incorrect hypotheses, they also
| increase rework. I could believe that for a well-performing
| team stacked PRs are better than equally-sized single big PRs,
| in that they could reduce inventory and cycle time. But like
| you, I think I'd rather just go for frequent, unstacked, small
| PRs.
|
| [1] e.g. https://kanbanize.com/lean-management/value-
| waste/7-wastes-o...
| vlovich123 wrote:
| I have stacked diffs sometimes when the rework is large. I
| want to make sure that I know the full story sounds the
| change I'm making because I'm forced to think about that
| upfront. What refactoring was needed? Was it actually needed?
| What new path do I carve out in the code or how do features
| interplay? Broken tests with good coverage tell me if I made
| a foundational mistake. Even if I decide to throw away the
| work because it was a dead end (rare) the team will have
| learned something by me explaining what didn't work out. More
| often, I'll need to go back and clean up. But I save
| significant reviewer time by doing that before putting up
| random prs one at a time that are not well understood. With
| the exception of very simple work, stacked reviews generally
| save significant time. You get reviews of non objectionable
| prs. Coworkers can see a bigger picture if that's helpful to
| understand the context of the change that's still coming into
| shape. It actually reduces merge conflicts because, for
| example, you can enable a refactor that everyone agrees needs
| to be done and land that. Then your conflict space is
| smaller.
|
| Small prs don't need it of course but complex features
| benefit from shaking out things earlier. Commit more than 100
| lines are really hard to review (lots of anecdotal and
| empirical research). If you're not reviewing small commit by
| small commit, the reviews are easily missing things. A single
| PR that's 800 lines adds review time to go commit by commit.
| If you can merge the non objectionable stuff, the reviewee
| gets to feel a some of forward progress and fewer merge
| conflicts (eg someone lands a refactor before some simple
| change of yours vs your simple change handed before and you
| made it the person refactoring their problem where it
| belongs)
| ramraj07 wrote:
| It's unavoidable sometimes. I get inspiration and time together
| rarely, I can't wait for small chunks of code to be merged
| before I continue. A lot of times it's an extremely Productive
| Sunday afternoon and I have 2500 new lines of code that builds
| a full new prototype. What am I to do?
| pizza234 wrote:
| I understand that (experienced the same "problem" today), but
| writing "2500 new lines of code" on a Sunday afternoon is
| (hopefully) not representative of regular workplace
| conditions.
| zeroonetwothree wrote:
| Some people enjoy their work. I think it's fine as long as
| it's a choice.
| NateEag wrote:
| It may be fine.
|
| Keep in mind, though, that humans are not perfect and
| some choices are unwise.
| sodapopcan wrote:
| Why would a prototype need a code review?
| computronus wrote:
| First, think about how difficult, and time-consuming, it will
| be for others to digest and review 2500 new lines of code
| that sprung from someone else's mind. So you will end up
| waiting anyway, for even a small part of your work to be
| merged.
|
| The work of breaking up a big, inspired chunk of work into
| small pieces helps you learn more about it, and the
| perspective can reveal improvements that weren't obvious in
| the initial effort. You might notice those yourself, or
| reviewers may. The final outcome will end up overall better
| for it, so spending that time is worthwhile.
| Supermancho wrote:
| > First, think about how difficult, and time-consuming, it
| will be for others to digest and review 2500 new lines of
| code that sprung from someone else's mind.
|
| There's a tradeoff to be made. Have a feature sooner or
| later. Review now quickly and more carefully later, or
| review carefully now. Part of what development teams do, is
| risk assessment.
|
| Put a feature flag on it, do a demo of the branch. If it
| looks good, do a quick once-over to see if it's interactive
| with any limited resources, merge it in, make a ticket for
| a re-review later.
| dreamdeath wrote:
| > First, think about how difficult, and time-consuming,
| it will be for others to digest and review 2500 new lines
| of code that sprung from someone else's mind.
|
| I haven't ever worked at big corp so maybe this kind of
| thinking is actually valuable there. But in most startups
| in my experience this mindset is wrong. You literally
| won't have a job tomorrow (because your company will
| fold) if you don't ship value-generating product
| yesterday. But you're going to worry about how
| _inconvenient_ it will be for some other developer to
| review your PR?
| computronus wrote:
| Well, I'm proceeding from the assumption that thoughtful
| review, which takes time, is desirable. If the situation
| is really that dire, then it's even more important that
| you ship product that works. Code review happens to be
| one of the, if not the most, effective ways to catch bugs
| and prevent disasters. It's a good idea to make the
| review process work for you, even and especially when the
| pressure is on.
|
| It's not about humongous reviews being "inconvenient". A
| drop of thousands of lines of new code takes a long time
| to review thoroughly, whether the company is at risk of
| folding tomorrow or a stodgy enterprise with decades
| ahead of it. If you have to constrain review time - or
| ditch reviews altogether, why not? - you can, but there
| are consequences regardless.
|
| I haven't worked in early startups, but I haven't only
| worked at large corporations either. I hope that most of
| us, most of the time, aren't less than a business day
| away from unemployment, and so there's usually time for
| code reviews. (If not, there's a lot of useless material
| about it!)
|
| Without sarcasm, and potentially getting off-topic, I
| would like to hear stories about how a startup survived
| impending doom with heroic, fast shipping of product that
| set aside a lot of time-consuming processes. What were
| the most crucial steps to keep? How was the technical
| debt repaid?
| [deleted]
| duped wrote:
| I have a couple anecdotes to cover your question.
|
| All of them have something in common: startup needs money
| so you demo to potential customers or investors. Unlike
| in a stable corporate environment where deadlines can
| have flex, you really don't want to cancel or postpone a
| meeting to sell to a client - so those demos dates are
| set in stone, and if things aren't ready you will need to
| pull some heroics.
|
| One memorable one was when three of us spent the hours
| leading up to a demo disabling automation and deploying
| to production by hand. What that meant was disabling a
| bunch of tests and code that checks those tests succeed
| in order to get the code out the door. We spent the next
| week cleaning that up. Sometimes you need to test in
| production.
|
| Another was actually a demo for the same client that set
| up that meeting. This time we had heard from the initial
| introductions they had a specific, very nasty problem and
| we could ostensibly solve it (that was true, we _could_ ,
| but the product at that point had no facility to do it).
| So I spent I think three days hacking together a solution
| that would solve the problem for the contrived demos we
| would show. That solution had atrocious performance
| characteristics so we wound up spending a few story
| points over the next two sprints to optimize and refactor
| the guts.
|
| Yet another demo was an integration. We had scoped out
| the general feature (which was magical when it worked)
| but it required a large amount of effort to coordinate
| with the original software vendors to get the data we
| would need. But those vendors have a tool with a free
| trial install which had a bunch of the data in XML, so I
| spent a day reverse engineering the schema and a second
| day parsing into our internal data model which we demoed
| on the third day to show how our product could solve that
| particular problem. We never wound up getting data from
| the first party company, and the parser got rewritten in
| a different language, and the backend that does the stuff
| is planned to be replaced in the next few sprints once it
| gets priority.
|
| So TL;DR you have a demo scheduled, you hack together a
| thing that can be presented, then spend your time
| reimplementing or refactoring the demo code into
| something maintainable.
|
| It helps to have a stack you can really quickly iterate
| on. And management that understands demos aren't suitable
| for production.
| wizofaus wrote:
| Why are you doing your demos in a production environment
| though? How many live customers did you have that could
| have been seriously negatively affected by what you were
| deploying (assuming it almost certainly contained bugs)?
| majormajor wrote:
| What am I gaining from having a stack of PRs there versus
| just a series of commits that I'm gonna go back and
| polish later, quite probably breaking into various PRs,
| but with some additional time and under less pressure?
|
| When I've hacked together stuff like that I'm often
| making it up on the fly, I rarely have a plan in mind
| that lines up with how I'd want to present it for code
| review later. Hell, a lot of times some of the later PRs
| would simply remove the entirety of a failed earlier
| attempt!
| Supermancho wrote:
| This isn't going to get me internet points, but after a
| few decades, I like to get my experience down.
|
| > Well, I'm proceeding from the assumption that
| thoughtful review, which takes time, is desirable
|
| Ofc it's desirable. Nobody is arguing this.
|
| > If the situation is really that dire, then it's even
| more important that you ship product that works.
|
| That is simply incorrect. Broken features (in innumerable
| shades) are debuted all the time to secure future
| investment in various ways which are not just financial.
| Sometimes it's community confidence, sometimes it's
| leadership clout, etc. Betas are a thing. This fear-
| mongering about 2500 lines of code is pearl clutching
| that hobbles large organizations that either fail to
| deliver or fail to deliver stability, regardless of their
| processes that drags on development for 4x or 10x, which
| they have misplaced confidence in.
|
| > I hope that most of us, most of the time, aren't less
| than a business day away from unemployment, and so
| there's usually time for code reviews. (If not, there's a
| lot of useless material about it!)
|
| There is lots of useless material about it. The number
| one rule is "don't interrupt the flow of money" but that
| rule doesn't extend to every project forever. Large
| organizations get less and less efficient over time (see
| rule 1) because perfect is the enemy of good. It's a good
| tradeoff, when you have an organization that can't keep
| up with how much money they are making to start piling on
| additional process because you don't know where your
| weaknesses are so anything that goes wrong gets an
| additional check. Most orgs are not in this boat.
|
| If you don't look like you're making your money's worth,
| even if you can explain how some side-process adds value,
| someone starts looking for your replacement and
| eventually they will find one and you're gone. That's
| true, regardless if you work at some mom and pop shop or
| Amazon. You're always one day away from unemployment,
| even if you don't know what day the chain of events
| started. So code reviews that block features can hurt you
| and have put a lot of people out of work, I can attest.
|
| The gut-check is when a company has a merger/acquisition
| deal. The team doesn't have a lot of time to vet the
| other company's codebase. You do your best to have your
| info meetings, try to run parts of the code with data,
| make your recommendations, and the escrow eventually
| closes. 2500? How do you think you'll do when looking at
| millions of lines? At some point, you will be forced into
| black box testing and realize that this is what matters
| foremost. Look at the interface, look at the
| expectations, look at the data. After that, the rest is
| gravy that can be addressed later.
| duped wrote:
| Hence why you don't have massive PRs like that, code that
| is easy to review is quick to merge and quick to ship.
|
| I wouldn't want to work at a startup that doesn't value
| that in their engineering culture, at least not again.
| More mature teams (in terms of the staff, not the
| business) I've been on get this and ship quickly.
| jcpst wrote:
| To offer an alternative perspective: I haven't ever
| worked at a startup.
|
| If someone wanted a PR review for 2500 lines, I would
| tell them no. I absolutely expect a developer to think
| twice when trying to make a change that large in one go.
|
| When your corporation's billion lines of code are the
| engine that helps generate $30,000 a minute, you don't
| just say "meh, looks good to me."
|
| Furthermore, this is multiplied on my team, which is the
| framework team. A mistake in our auth middleware, http
| client, etc, could easily turn into a mistake on 50+
| other teams.
| baq wrote:
| If you're the only developer for quite a while...
| otherwise reviewability is a thing PRs must be optimized
| for or your super-duper-important feature won't get in.
|
| Either way you should be looking for a new job, because
| what you're describing is quite unsustainable in a team
| of more than one person working on the same thing.
| majormajor wrote:
| If you're in a go-under-the-next-day scenario who is even
| reviewing your code? How big is your team, realistically?
|
| Once you have significant runway and can hire, you need
| to be able to start delivering faster and faster. One
| person writing 2500 lines of code in bursts once or twice
| a week isn't going to scale. You need a team, and you
| need to be measuring total team throughput.
|
| Need a new service prototype? A 2500-line PR is
| appropriate. But don't be surprised if it needs some big
| revisions - that's the point of a prototype anyway.
|
| Need a feature? One person periodically "when inspiration
| and time line up" dropping 2500 lines (this is a LOT of
| lines in most languages used these days, even in newer,
| more-concise Java versions) on top of what 10 other
| people may be working on is _not_ going to help everyone
| else move quickly at all.
|
| If you're the person who's so busy you rarely have time
| to code, you need to figure out how to turn your
| inspirations into ideas _others_ can execute and be the
| code-level experts on. A team of 10 "1x" developers is
| still more productive than a single "10x" developer who's
| in meetings figuring out the company's plan half the day
| anyway.
| celtain wrote:
| Isn't this exactly why you should use stacked PRs? That's
| what it looks like when I break a big piece of work into
| smaller pieces.
|
| What's the alternative anyway? If you don't want me writing
| 2500 lines of code in one area, would you rather I write 10
| 250 line PRs in 10 different parts of the codebase instead?
| Is that supposed to be easier to review?
|
| Or is the rule just "don't write a lot of code in any short
| period of time"?
| morelisp wrote:
| > don't write a lot of code in any short period of time
|
| Depending on the team this may be a completely reasonable
| policy.
| tomasreimers wrote:
| I am one of the authors of the tool Graphite
| (https://graphite.dev). We built Graphite because we missed the
| stack diff workflow we knew from previous job experiences at
| larger companies, happy to answer any questions (and if you would
| rather not ask in public, feel free to email me at tomas at
| graphite.dev).
| yosito wrote:
| This article uses the term DAG everywhere, but I couldn't find a
| definition of it.
| scruple wrote:
| Directed Acyclic Graph.
|
| [0]: https://en.wikipedia.org/wiki/Directed_acyclic_graph
| compsciphd wrote:
| One thing I wish git did (maybe it does and I don't know how?) is
| to be able to say that a new branch is based off an old branch
| (not a commit that used to be that branches head). so I can
| branch a single pr in progress to start the next. Then if I
| change the base pr in progress (say via rebase or via squashing
| or the like), I can easily rebase my new commits in the new pr on
| top of the current state of the branch.
|
| Currently, one can just do it by never modifying the underlying
| "branch" (i.e. just adding new commits), but in practice that
| doesn't always work, as many times you have to rebase against
| one's master/main branch to pull in changes, which even if no
| conflicts, will reflow your "base" branch" changing all the
| commits.
|
| TLDR: Basically, want to be able to state that branch depends on
| branch (which is currently commit id x) but if I rebase, use
| whatever the current commit id is for that branch, not whatever
| it was when I first made the branch.
|
| possible? stupid idea? thoughts?
| 8K832d7tNmiQ wrote:
| > I wish git did is to be able to say that a new branch is
| based off an old branch (not a commit that used to be that
| branches head). > Then if I change the base pr in progress (say
| via rebase or via squashing or the like), I can easily rebase
| my new commits in the new pr on top of the current state of the
| branch.
|
| You'd still end up fixing all the conflicts your branch made
| either way, and also breaking the flow of commits if you didn't
| update the message. So why bother doing all of that instead of
| stashing your current project, rebase, and force push it? I
| believe that's supposed to be the standard approach to handle
| conflict changes in PRs.
| msbarnett wrote:
| > TLDR: Basically, want to be able to state that branch depends
| on branch (which is currently commit id x) but if I rebase, use
| whatever the current commit id is for that branch, not whatever
| it was when I first made the branch.
|
| As far as I can tell (the "problem" you're describing is a bit
| vague) this is already how git works with the sole exception
| being that git doesn't default to a particular branch for a
| rebase?
|
| eg) all you seem to de describing here is a bog-standard:
| (on feature-branch1)> git checkout -b feature-branch2
|
| <do some work on feature-branch2, return to feature-branch1 and
| make some more changes there, now you want to catch feature-
| branch2 up with feature-branch1> (on
| feature-branch2)> git rebase feature-branch1
|
| <now we're branched off of the new tip of feature-branch1, not
| where we originally branched>
|
| All it seems like you're asking for us to drop "feature-
| branch1" from the rebase?
| epage wrote:
| git-stack [0] tries to do that. I also maintain a list of
| related tools, some of which do similar [1].
|
| [0] https://github.com/gitext-rs/git-stack
|
| [1] https://github.com/gitext-rs/git-
| stack/blob/main/docs/compar...
| bqmjjx0kac wrote:
| It sounds like you're describing what `git branch --set-
| upstream-to=$otherBranch` does?
| zackmorris wrote:
| Came here to say the same thing. I always rebase my own branch
| onto the trunk and force-push before merging. But that's hard
| to explain to other developers, and there's no way to enforce
| it, so the repo inevitably becomes filled with overlapping
| merges, which is an unforced error IMHO. Does anyone know if
| there's a way to only allow rebase-and-merge on GitHub and
| GitLab?
|
| https://docs.github.com/en/pull-requests/collaborating-with-...
|
| https://docs.gitlab.com/ee/topics/git/git_rebase.html#rebase...
| regularjack wrote:
| Isn't that how rebase already works? It will "apply" commits of
| the current branch onto commits of the base branch, including
| new commits that were created in the meantime.
| theptip wrote:
| This is one of the big features from git-branchless
| (https://github.com/arxanas/git-
| branchless/wiki/Command:-git-...) which is recommended in the
| article; moving a stack (sub-tree) of commits instead of just
| one branch.
|
| Might be useful for you if you find the rebase flow to be
| painful. In particular this can be good for local prototyping
| where you have a bunch of functional candidates on top of some
| foundational refactors, and you may be rebasing frequently to
| refine those foundational commits.
| arxanas wrote:
| Also worth pointing out these commands from git-branchless
| (I'm the author):
|
| - git sync: rebase all commit stacks on top of the main
| branch. - git restack: run after making a change to a
| foundational commit to automatically restack dependent
| commits.
| dan-robertson wrote:
| It feels like roughly the issue is that git thinks in terms of
| snapshots whereas people often think in terms of patches. Darcs
| and pijul are version control systems that try to have patches
| as the fundamental object you operate on instead of snapshots.
| In particular, there isn't a rerere operation to change the
| base of a commit because patches don't have bases in the same
| way. However if you're reviewing code you probably do care
| about the base because changing the base may change the meaning
| of the branch and if eg some library function is renamed
| between when you write/approve a patch and when you merge it,
| the patch may no-longer be valid.
| avl999 wrote:
| I do stacked PRs at work, they work great until someone suggests
| an invasive change in a PR lower down in the stack. Does anyone
| have ideas on how to deal with merge conflicts in this type of
| flow? For example let's say I have the following stack of PRs
|
| PR1 -> PR2 -> PR3 -> PR4
|
| The reviewer reviews and suggests a change in PR1, this change
| causes a merge conflict in PR2 and therefore in PR3 and PR4 as
| well. And then you have to go in manually resolve the exact same
| merge conflict all the way through you PR stack in each of the
| PRs. This gets annoying and hard to work with.
|
| Does anyone have a better way of dealing with this pattern of
| merge conflict? I've tried using git rerere which in theory
| sounds helpful but doesn't seem to do anything when I strike this
| issue.
| fire wrote:
| huh, shouldn't solving the conflict to pr2 create a merge
| commit that then also solves the conflict in 3 and 4?
| travisb wrote:
| It sounds like the OP might be using a rebase workflow. Such
| a workflow creates many repetitive conflicts because there is
| no merge commit to record the resolution point.
|
| Using a rebase workflow with stacked/cascaded PRs is an anti-
| pattern that trips up people used to other git workflows
| where the history depth is effectively only one level deep
| instead of arbitrarily deep as in stacked/cascaded PRs.
| xerxes901 wrote:
| I think this is something git rerere is supposed to help
| with
| anderskaseorg wrote:
| You can tell git what the resolution point is using `git
| rebase --onto`. This can help avoid situations where Git
| gives extra conflicts form trying to reapply an old version
| of your change on top of a new version of the same change.
| You can also use `git rebase --skip` if you recognize that
| you've ended up in this situation.
| kqr wrote:
| I wish I had a better answer for you but my go-to solution as
| of late has been the pragmatic "I agree. Do you mind if I fix
| that in a later PR to avoid spending too much time on it?"
|
| If you've built up some trust with your team, they shouldn't
| have a problem with it.
| arxanas wrote:
| The linked tool git-branchless handles this pretty well
| (https://github.com/arxanas/git-branchless, I'm the author).
| You basically run `git checkout <branch>`, `git commit
| --amend`, and then `git restack`. This will rebase all
| dependent branches. As long as you're not using merge commits,
| you won't have to resolve the same conflict more than once. (It
| will also warn you up front whether or not merge conflicts will
| need to be resolved; you can pass `--merge` to start merge
| conflict resolution.)
|
| To rebase your commit stack on top of the main branch, use `git
| sync` instead of `git merge`. Merge commits often make it so
| that you have to resolve conflicts multiple times.
| bvrmn wrote:
| Mentioned tools seem highly underfeatured comparing with stacked
| git[1].
|
| [1] https://stacked-git.github.io/
| arxanas wrote:
| Hi bvrmn, I've been meaning to write a comparison of git-
| branchless with Stacked Git, as they occupy similar spaces. Can
| you list some of the features which Stacked Git has which git-
| branchless might not?
| moffkalast wrote:
| Well this would be impossible to explain and coordinate with the
| team
| __derek__ wrote:
| I'm guilty of doing this in the past, but it seems like an anti-
| pattern because it attempts to create a local optimum, a big no-
| no in the Theory of Constraints[1]. Better to find ways to
| sustainably ease the constraint (code reviewers' time/attention)
| than to find ways to create more WIP at the constraint.
|
| [1]: https://en.wikipedia.org/wiki/Theory_of_constraints
| morelisp wrote:
| As someone coming down from a nearly 2 year fight with an
| engineering manager whose response to every bug was enforcing
| more code review rules, and every code review backlog was
| demanding more time allocated for code review, thank you for
| this comment.
| d0mine wrote:
| Could you suggest some specific alternatives?
| baq wrote:
| i got a major cognitive dissonance from your comment because
| stacking PRs is for me the way to get more time and attention
| from potential reviewers by making their units of work smaller;
| hopefully small enough to be easily mergable. this is a case of
| 'more is less' (within reason).
| ramraj07 wrote:
| Any UI suggestions to make stacked PRs easier? Other than
| graphite.dev - that needs org access on GitHub.
| stitched2gethr wrote:
| I believe this goes a bit further and decouples the submission
| from the review. My team reviews PRs very fast, but why should
| I wait 15 minutes between submitting and continuing my work. Or
| what about when I'm working late when no one else is online.
___________________________________________________________________
(page generated 2022-07-24 23:00 UTC)