What's the difference with ejoffe's spr?

[dko-slapdash]

Hi.

What's the story behind having the tool with the similar purpose and same name, but written in Go? https://github.com/ejoffe/spr/

That repo doesn't mention you, and also you don't mention them anywhere.

P.S. BTW a super-nice feature you have in comparison to them is that Pull Request: https://github.com/xyz/abc/... block which is added to every single commit in the local repo (like in old FB times). It helps the reader of commit history to instantly find the PR and discussions in it.

[sven-of-cord]

Hi @dko-slapdash ! Thanks for asking!

The tl;dr is: @ejoffe's spr uses force pushes, this spr does not.

This spr tries to emulate Phabricator's workflow as closely as possible, with very few exceptions.

Technically speaking: I'm no expert for ejoffe's spr, but I did play with it. Maybe some of things I'm about to say are not entirely correct (e.g. if some behaviour can be configured a different way, I wouldn't be aware of that and am assuming that you can't do it). Anyway, as far as I can see, ejoffe's spr creates a remote branches pointing at each of the commits of your local branch, pushes them to GitHub and creates PRs for them. On update, it force-pushes your updated commit to the already created remote branch, which updates the PR.

GitHub is not fantastic when dealing with force pushes. GitHub may improve this in the future, but my experience has been that e.g. when you receive inline comments on your code in a PR, force-pushing a new version replaces the old commit with the new one, and the comments on the old version are not super easy to access now. In the PR timeline, GitHub will note that a force push happened, and will even give you button to compare the commits before and after force push. The experience is a lot worse than if you use GitHub the GitHub way, i.e. have a branch for a PR to which you keep adding commits. In that case, the timeline is much clearer.

This spr does not force push ever. It also creates one (or two - see below) branches per PR, but these branches never point at your local commits. Instead, this spr creates synthetic commits that get added to the PR branches and replicate the history of your local commit. If you amend your commit, spr will add a new commit to the PR branch representing the changes. If you rebase your commit locally, spr will add a merge commit to the PR branch that merges in the new base of your commit. When GitHub displays the PR, and thus the PR branch, it shows really nicely the history of your local commit. If someone reviews your PR, and then you update it a few times, once they come back GitHub will show them a button to see the changes in this PR since their last review. This rolls together all the commits that spr added since the review (which is all the changes you did). You don't get that with force pushing.

When you stack PRs, i.e. you have a local branch with multiple commits, spr creates a PR branch for each commmit, but also a base branch for each but the first commit. All other tools I've seen (including ejoffe's spr) use the PR branch of PR1 as the base branch of PR2. This spr has dedicated base branches. I didn't do this because I like having many branches, but to emulate the Phabricator experience better. For one, in Phabricator you could have a branch with multiple commits and not submit all of them for review, but just whichever one you want. As you make changes, you could also choose which ones you want to update. And on each diff, CI would run on the contents of the commit you submitted, including whatever changes were in the base of the commit, whether they had been landed on master already or not. This spr emulates all that. And it also produces a reasonable timeline for each PR on GitHub after landing. If you force push you don't have the luxury of a proper history and timeline anyway, but even other tools that don't force push have a problem with the GitHub display of a stacked PR after landing: if you stack C onto B onto A, and you land all three, then the PR for commit C on GitHub now also shows all the commits (that is all the intermediate versions) of the PRs of A and B as well. That's because C's base branch was B's PR branch, and after landing (if you squash-merge, but let's assume you do, because you want the Phabricator-style linear history) the PR page for C shows all off-master commits in the C branch. This spr crafts dedicated base branches. That doesn't get rid of the fact that GitHub will display the commits in the base branch after landing, but this way we can have the minimum number of commits in the base branch and give them descriptive titles, like "changes introduces through rebase". You can look at #122 as an example, which was stacked on #120 during development. When I updated #122, you might now see two entries in the timeline: first a commit titled "[𝘀𝗽𝗿] changes introduced through rebase]" which contains all the changes to the parent of my local commit (because I made changes up the stack, or rebased onto a newer master), and one with a handwritten title (spr asks you for a title when updating a PR) which contains the changes I made to this commit.

I hope this makes things a little clearer. Anyway, the point being, these tools use different approaches. You can pick your favourite!

I can also recommend the title page of https://getcord.github.io/spr/ for a good intro into the concepts of (this) spr.

PS: feel free to add follow-up questions, or close this issue if you don't have any.

[dko-slapdash]

Oh wow Sven, thank you so much for such a detailed explanation!

I have several follow-up questions here if you allow.

1/ Imagine I have a stack of A-B-C, and there are heavy GitHub actions associated with this GitHub repo which take 10 minutes to finish. All A-B-C are approved and are green in CI. Now I land A. It triggers B and C's actions to rerun, so basically I need to wait 10 more minutes until I can land B, then 10 minutes to land C. Is it unavoidable, or do I miss something?

1a/ The alternative force-push approach of having branches brA-brB-brC for commits A-B-C correspondingly and having PRs prA-prB-prC for master-brA/brA-brB/brB-brC doesn't trigger CI reruns after landing prA. After landing prA, the base of prB starts pointing to master, and GitHub doesn't re-trigger actions, so I can land the entire A-B-C chain super-quickly (considering the entire stack was updated recently and sits on top of master which likely took 10 minutes, not 30). It has the downsides you mentioned above of course, but the ability to land quickly in case of heavy GitHub Actions matters.

1b/ In case a strict protected branch rule is used (Require a pull request before merging, Require linear history), spr land stops working according to my experiments: this is because the base of each PR often points to a synthetic branch, and it rejects the merge. Is there a work-around?

2/ Imagine I have a stack A-B-C, and only C is approved. What I did in this case super-often (in FB) is following: I reordered the commits in the stack (with rebase -i in Git world), so it looked like C-B-A, and then landed C once it passed in CI. The problem is that with spr, after I put C in the beginning of the stack, the base of prC won't point to master (as the base of prA did), i.e. reordering broke the invariant where the 1st commit in the chain is based off master. So I can't land it from GitHub UI for instance and can't satisfy protected branches rules (I suspect this is where push -f for branches would be unavoidable). Do I miss something here, is it an architecture limitation, or something can still be done here, to make prC have the base pointing to master after reordering?

[sven-of-cord]

Hi. Sorry again for taking so much time to respond.

1/ I don't think you miss something. What triggers the re-runs of your CI on B and C? I don't know what your CI setup is like. Is it landing A itself (in which case I guess it can't be avoided), or is it running spr diff on B and C after landing A? If it's the latter, you can sometimes land B without doing spr diff first. It depends on the specific Git topology, e.g. if and how the PRs for A and B were amended and updated, or the first version got landed. Sometimes GitHub considers the PR for B as "mergeable" after landing A, sometimes it doesn't. There may be room for spr land to add a commit to the PR branch just before merging to help GitHub perform the merge if spr can make sure that there are no local unreviewed changes. (Diverging a little from Phabricator behaviour, spr land is meant to never introduce changes to the PR before landing.)

1a/ re-using existing branches as base branches is tricky. Not only does it lead to confusing PR timelines after landing, it also means that spr wouldn't be able to submit a PR if its parent in its current form isn't either on master or submitted as a PR. Like Phabricator, spr allows you to only submit B for code review (on an A-B local branch), without having submitted A. That may sound strange at first, but it's a thing you might want to do. E.g. A-B is a bit of an experiment, and you want to get feedback on what B looks like, before even submitting A for review. Phabricator allows you to do that, and so does spr.

1b/ I don't understand where the problem is. For our closed-source company monorepo, we have the linear history requirement and also a branch protection rule that requires code review. That's the set up spr was designed for. Maybe your config is different. Would changing the base branch of an existing PR mean that it needs to be re-reviewed?

2/ Hmmm, again the scenario you describe works nicely for me. I do that all the time. After you reorder your stack to be C-B-A, prC still has a base branch, but its contents are now identical to the master commit C is based on. spr land switches the base branch to master and merges, which produces the correct new commit on master. I wonder if your GitHub config has a problem with that switch of the base in-between accepting the PR and merging it. If so, it would be possible that spr diff on C after the rebase could already change the base branch to master, not just spr land. The disadvantage is that this will make additional auto-generated commits (prefixed with [spr]) appear in the PR timeline. (Normally these only appear when landing, because only then spr land makes the switch to master as the base branch.)

Oh.... I just see now that you're talking about GitHub UI. Yes... landing has to be done with spr land really.

Anyway... the things you describe are things that I and my colleagues do on a daily basis using spr. We don't ever merge from GitHub UI. And we might have a different GitHub repo configurations.

I'm happy to jump on a video call if you'd like to discuss synchronously.