Land multiple diffs? (eg `spr land --all`)

[leebyron]

First off, thank you for building spr! This is already proven to be a really helpful tool.

One workflow I wish could be improved is landing a whole approved stack. I'll often send out a few diffs in a stack (hurray for diff --all) and come back to find them all approved. Right now I'm landing them each with interactive rebase, but this feels cumbersome. I find myself wishing I could land --all to do this programmatically.

Better yet, it would be amazing if this could land those in a landable state (merge-ready and approved) and skip over and report the ones still open. So if I stack A, B and C onto main, find A and B approved, but not yet C and land --all then result in A and B successfully landed and C rebased on the result

[sven-of-cord]

Hi Lee!

Thanks for the idea. It's not super trivial to do, in a useful way, because landing a diff often means that the next diff in the stack needs to be updated after the rebase. I guess that's part of what makes this workflow so clunky. The problem is that I don't want that spr land smuggles in local changes to your commit that were done since the last spr diff. (This is a deviation from Phabricator's behaviour, by the way.) So spr land refuses to land the diff if the contents of the local commit (if applied (cherry-picked) onto origin/master) are any different to what merging the PR would produce. In reality that however often means that a diff that was stacked on one that is now landed, needs updating with spr diff before landing it. Unless you want an automatic "just update the PR and then immediately land it", landing a whole stack in one doesn't really work. The downside of "just update the PRs..." is that you would land code that is potentially different from what is in the pull requests, which could mean unreviewed changes are included, but also any CI runs that happen on pull request don't complete before you land.

I agree this is a bit clunky. Landing a stack of N diffs means landing the first, updating the next one, maybe quickly checking whether you introduced changes with this update (just in case you had local changes that you thought had been part of the PR already but weren't), waiting for CI to finish, then landing that diff and rinse and repeat until you're done.

Maybe not "a bit clunky" but "super clunky". GitHub wasn't really made with stacking Pull Requests in mind. The tricks that spr uses let you stack diffs, and actually, it works pretty well in production use for us, but it's still a bit of a hack.

Anyway, your feedback is welcome and your point is taken. I will give it a think, I'm sure the experience can be improved somehow.

[leebyron]

Very real potential pitfall territory, yes. My thought was that a fairly large majority of the time I don't actually encounter any merge or rebase issues that require manual resolution. Considering spr already goes to great lengths to ensure a landed merge will be exactly what is expected by checking behavior in advance, my thought was that perhaps spr could also be able to check that the rebased commits would not have any additional changes. If it could do this, then for the minority of cases where manual intervention for rebasing is required, spr could fail and explain that an interactive rebase is required, but otherwise would just do the right thing automatically.

Another way to think about this feature request is that really there are two missing features, one which composes the other:

1. "Land first"

spr land --first - automatically land the first diff in the stack.

According to the stack diffs documentation, to land a diff you must use git rebase --interactive to insert exec spr land after the first commit in the stack. This is great when rebasing and landing would require fixing things manually first, but in the majority of cases this runs cleanly, so it's just a few too many manual steps and things you need to remember. Ideally spr could check to ensure the interactive rebase would happen cleanly, and if so automatically place the x spr land in the right place, if it would not happen cleanly it should stop and explain how to manually resolve and land before entering the git rebase mode.

This would simplify landing the first diff in a stack down to just running spr land --first as a single step.

2. "Land all"

spr land --all - given the "--first" behavior, this applies it recursively until there are no more commits to land, the next commit is not landable (not approved yet?), or it is not possible to land automatically without manual resolution.

This would cover the simple majority case where a full stack of diffs has been approved, and a single spr land --all command could see them all land in the correct order. It would also cover my "better yet" case where if a subset are approved, those landable are landed, leaving the new first in the stack as the next to resolve (either get it approved or resolve a conflict)

---

Hopefully that's a helpful reframing! Thanks again for this stellar tool

[rockwotj]

I've been using this (admittedly hacky) python script to do this, a couple improvements that it could do: automatically rebase you (interactively) with the git remote branch and running spr diff --all before actually landing things.

#!/usr/bin/env python3

import argparse
import subprocess
import os

parser = argparse.ArgumentParser()
parser.add_argument("file", nargs="?")
parser.add_argument("--debug", action="store_true")
args = parser.parse_args()

def git(cmd):
    if args.debug:
        print("$ git " + " ".join(cmd))
    return subprocess.check_output(["git", *cmd], text=True).strip()

if args.file:
    with open(args.file, "r+") as f:
        lines = f.read().splitlines(keepends=True)
        lines.insert(1, "exec spr land\n")
        f.seek(0)
        f.writelines(lines)
else:
    master_branch = git(["config", "spr.githubmasterbranch"])
    merge_base = git(["merge-base", f"origin/{master_branch}", "HEAD"])
    num_to_land = git(["rev-list", "--count", f"{merge_base}...HEAD"])
    if args.debug:
        print(f"number of changes to land: {num_to_land}")
    for _ in range(0, int(num_to_land)):
        subprocess.check_call(
            [
                "git",
                "rebase",
                "--interactive",
            ],
            env={"GIT_EDITOR": os.path.abspath(__file__), **os.environ},
        )