push: base new branch on latest upstream main

[RalfJung]

Fixes #46

I tested this by doing a Miri-push.
Cc @lnicola

[RalfJung]

Here we rely on nothing else working on the same git repo concurrently as that could clobber FETCH_HEAD...

[tgross35]

Not that it's the biggest concern, but you can be robust against this by replacing base_upstream_sha with the equivalent of the below rather than relying on FETCH_HEAD:

$ git ls-remote https://github.com/rust-lang/rust refs/heads/main
f676c20edd32321e9fa2781543d8970109707e30        refs/heads/main

(this is a fast operation unlike the fetch itself)

[tgross35]

Maybe it's worth having a CLI argument to override the base? I think that with this change, running twice could get you two different results if r-l/r has an update in the meantime.

[RalfJung]

I think that with this change, running twice could get you two different results if r-l/r has an update in the meantime.

The entire reason why we're doing this is that this is already the case. That's what caused this problem. That's what I tried to explain in #46.

[tgross35]

Hm - I meant for the purpose of being able to rerun locally and use the same base within a small timescale to pull in a few new commits on top of the existing tree, without changing the history that I've already looked through and verified. Not typically long enough to run into the problem. But that's not overly useful so 🤷

Nothing is going to be completely robust against the race here if that operation isn't durable, unless we do something like fail the merge in CI if new changes happened while the subtree sync was in the queue.

[RalfJung]

Yeah this assumes that pushes are totally ordered. Which seems like a reasonable requirement, subtrees should coordinate about who's doing syncs (or maybe we can even mostly automate them).

[RalfJung]

I just tied to do another miri push sync with this and got an error:

Preparing https://github.com/RalfJung/rust...
+ cd "../rustc" && "git" "fetch" "https://github.com/RalfJung/rust" "miri"
+ cd "../rustc" && "git" "fetch" "https://github.com/rust-lang/rust"
+ cd "../rustc" && "git" "push" "https://github.com/RalfJung/rust" "FETCH_HEAD:refs/heads/miri"

Pushing changes...
+ cd "/home/r/src/rust/miri" && "git" "push" "http://localhost:42042/RalfJung/rust.git:rev(75dd959a3a40eb5b4574f8d2e23aa6efbeb33573:prefix=src/tools/miri):/src/tools/miri.git" "HEAD:miri"
Error: cannot perform push

Caused by:
    Command `cd "/home/r/src/rust/miri" && "git" "push" "http://localhost:42042/RalfJung/rust.git:rev(75dd959a3a40eb5b4574f8d2e23aa6efbeb33573:prefix=src/tools/miri):/src/tools/miri.git" "HEAD:miri"` failed with exit code Some(1). STDOUT:
    
    STDERR:
    To http://localhost:42042/RalfJung/rust.git:rev(75dd959a3a40eb5b4574f8d2e23aa6efbeb33573:prefix=src/tools/miri):/src/tools/miri.git
     ! [rejected]            HEAD -> miri (fetch first)
    error: failed to push some refs to 'http://localhost:42042/RalfJung/rust.git:rev(75dd959a3a40eb5b4574f8d2e23aa6efbeb33573:prefix=src/tools/miri):/src/tools/miri.git'
    hint: Updates were rejected because the remote contains work that you do not
    hint: have locally. This is usually caused by another repository pushing to
    hint: the same ref. If you want to integrate the remote changes, use
    hint: 'git pull' before pushing again.
    hint: See the 'Note about fast-forwards' in 'git push --help' for details.

@christian-schilling any idea what is happening here?

[christian-schilling]

Yes. Josh will choose the "oldest" (by topology, not date of course) commit in the target which filters to a commit in the branch being pushed to serve as a common ancestor. That means the result will in general not be a descendant of the target tip. We never really used it that way (fast forwarding exsting branches from projections), but rather used the -o base option to create a new branch based on on existing branch.
This helped making the pushes mostly repeatable (not a guarantee, but normally they are) causing far less noise when pushing the same branch repeatedaly as opposed to change the result every time the HEAD changes.
This is because we have been using either gerrit (which natively assumes stacked changes and force push like behaviour) or our own implementation of stacked changes with github (see latestest josh release) which kind of emulates gerrit like worklow.
But also in the "normal" PR workflow it should also work by letting josh create the PR branch, using -o base to point to latest HEAD to find a suitable branch of point.
I thinks it's not required for bors to have the PR tip be a descendant of the target HEAD (might be wrong about that though?) as long as it can be merged without conflicts.
There was once a patch josh-project/josh#1303 to allow using "most recent" as the branch of point but so far all users have been fine without it so it never got merged.
There is also the "-p merge" option that will create a merge commit that is a descendant of the target. But that's not really how it is normaly done with a PR. I mean the merge commit should be created by the merge button/merge queue/bors, not already be in the PR branch. (not a fan of that concept, but it is how github is used normally...)

[RalfJung]

Ah, I didn't realize that this is different from -o base. The problem with -o base is that we need two branches to set up a sync PR. I guess it is reasonable to assume that a user won't mind their main branch being synced with upstream when they do a josh-push, but it is a bit of a surprising interaction.

[christian-schilling]

Ah. Well the users main does not need to be synced with upstream. It just needs contain the the common ancestor. This should be the case if the users fork of the subtree is also based on the users fork of upstream rust.
But when the users fork of the subtree pulls from r-l/r put then pushes with the users outdated own rust as base, then yes it will be inconsistent. So then the users fork needs to be updated.
We will probably add "use base from another remote" to the josh cli in the not to far distant future as this is common with github workflow.

[christian-schilling]

Instead of the surprising interaction of updating the forks main, it could also use some refs/heads/upstream/main.

[RalfJung]

I updated the PR to use the -o base strategy. (That's also what we used in Miri before switching to "prepare a base branch with the old fixed commit".) That seems to work right now but we'll only really see if it works over time -- the old strategy also worked the first time I tried it.

[RalfJung]

Though strangely this push took multiple minutes despite being tiny. It's almost as if somehow it couldn't reuse the cache as well as it usually does.

[christian-schilling]

Though strangely this push took multiple minutes despite being tiny. It's almost as if somehow it couldn't reuse the cache as well as it usually does.

Most likely you're seeing the perf issue that was addressed in josh-project/josh#1665

So this will require updating. Which will require setting the backwards compatibility options on the filter.
There is a description https://josh-project.github.io/josh/guide/migration.html

Miri needs both the crlf and the keep trivial option.
And of course the new rev filter syntax.
I already tested that with Miri specifically the shas will be the same as with old Josh.
In fact I found the crlf issue while testing with Miri before tagging the release.

[RalfJung]

So this will require updating.

Well, it works, it's just slow. ;)

But yeah, updating would be good now that finally 🎉 there is a new release. But that's #42.

And of course the new rev filter syntax.

Would be nice to also include that in the migration docs. :)