Non-roundtrip push after merging commit with no parent

[lnicola]

Sorry, not a minimal reproduction, but running this in the rust-analyzer repo seems to reproduce it:

$ git branch test-merge 4bf516ee5a960c1e2eee9fedd9b1c9e976a19c86 # last commit before smol_str
$ git checkout --orphan empty
$ git rm -rf .
$ touch xyzzy
$ git add -A xyzzy
$ git ci -am "First"
$ git switch test-merge
$ git merge --allow-unrelated-histories empty
$ rustc-josh-sync push [...]
[...]
    Josh created a non-roundtrip push! Do NOT merge this into rustc!
    Expected 6d7e78d196a4b8482d8537283d2b04dddc1e4fbc, got 7ff61f7cdd2b10c2bcf3379911180cb8a9e3ca9a.

See rust-lang/rust-analyzer#21008 for context.

[RalfJung]

In josh terms, rust-analyzer is a repo created by pulling from josh with a particular filter (configured here) -- similar to miri. What's happening here is that a branch was merged into rust-analyzer that is unrelated to the existing history there, i.e. there's a new no-parent commit in rust-analyzer. The next time we are trying to push this to the main repo, the push to josh seems to succeed, but silently creates a corrupt history in the sense that a subsequent pull does not produce the commit we just pushed. Our josh sync tooling checks for this case since we had such corruption before, and that leads to the above error.

[RalfJung]

Interestingly, we have another josh subtree, compiler-builtins, that also has a merge bringing together two unrelated histories (rust-lang/compiler-builtins@8f8dfb5), and yet that never caused problems. That merge was done before the repo was josh-ified, but josh did the initial push if this history into the rust repo and that commit round-trips just fine. I have no idea what makes the round-trip in RA problematic.

Maybe it is related to the fact that rust-analyzer has a rev in the filter, but compiler-builtins does not? In fact that seems plausible. Whatever rev does on push seems to get confused by having a branch of the history that doesn't contain the revision at all...

[christian-schilling]

You really never run out of edge and corner cases to discover... 😄

So if i understand correctly the repo that is already a josh substree got itself some git subtree(s) added to it..
Is the intention to keep those new sub sub trees also external and in sync?

Anyway there are two parts to this problem:

1. Now there is a hybrid between josh subtree and git subtree..in principle that should be possible but it means having to deal with this wired "move-merge" commits that git subtree creates. And potentially using :rev in different places to make histories match.

2. The addition of unrelated history. We have that documented here: https://josh-project.github.io/josh/guide/importing.html
   So the options are doing the merge of unrelated history in the main repo by hand, creating a merge commit or passing -o merge to git push when pushing via josh.
   -o merge does two things: It instructs the unapply_filter function to keep all orphan commits and also the push code to create a merge. Now im having the idea if it could make sense to offer -o keep-orphans as a dedicated flag but im not sure yet what that means.

So if rewriting / force pushing to the subtree (rust analyzer) is an option I would definetly recomend to do that to get rid of the git subtree "move merges" and have working git blame for that history by importing them as descibed in the above docs. Doing this directy in the main rust repo would work for sure (still check of course.. 😅 ) doing this as sub subtree is not something I tried before and it might require a josh patch.

[christian-schilling]

Ah about having another case that had no problems: Without having looked into it yet, I know does make a difference if the orphan commits are empty vs non empty. Maybe that's the case here.

[lnicola]

Is the intention to keep those new sub sub trees also external and in sync?

No, it was a one-off import into rust-analyzer. I ended up redoing it using git-filter-repo and rebasing the subtrees over our master, which seems to have fixed the roundtrip issue.

[christian-schilling]

So the problem is solved? 🤔

[lnicola]

More like worked around, I guess, but it's good enough for us, assuming we don't find any issues with my branch.

[RalfJung]

Anyway there are two parts to this problem:

I don't think for josh it should matter much how the history was created. The fundamental issue is: if we do an unrelated merge in the "subrepo", we can't push the result. This may be related to the rev filter.

You pointed at the "importing projects" docs, but we're not trying to import a new project. We just have some new history (created with the commands in the OP) in an existing project, and we're trying to push that.

So the options are doing the merge of unrelated history in the main repo by hand, creating a merge commit or passing -o merge to git push when pushing via josh.

We can try setting -o merge. However, syncs are largely automated, and in principle should work with arbitrary new history in the subrepo, so -- should we just always set -o merge?

What does option 2, "creating a merge commit" mean?

[RalfJung]

I did a little experiment with Miri, and the result was the same with and without -o merge. You said -o merge also "instructs the push code to do a merge", what does that mean?

EDIT: I did the experiment wrong. Doing it right, -o merge indeed does something... it makes the round-trip check fail even for Miri's harmless history.^^ Yeah, there's a new merge commit.

[christian-schilling]

Ok, good to know that it did do the merge...I was just looking at the code and it is quite literally if (merge option is set) { create merge commit } so not getting that would be quite strange.

It breaks the round trip 🤔 I suspect the commit coming back is the merge commit, which is a descendant of what was pushed, but not identical.

You are right that pushing any history should work, I do remember however that there is some corner case when orphan commits have a completly empty tree. I'll have to look into it more closely.

I checked that for the compiler-buildins all orphans have a non empty tree, so that's part of the explanation why there was no problem. Now I want to check the orphans in the rust-analyzer case but have not found the right history / sha to look at yet 😅

[christian-schilling]

So I checked: rust-analyzer/smol_str does have a non empty tree in it's initial commit, so that's not it 🤔

[christian-schilling]

I think I found the problem and it does look like a bug 🐛
It comes from the "workpaces" feature: There josh does synthesize merge commits for workspace edits, but needs to treat those "synthetic merges" as non merges during push. The code that is supposed to detect those "synthetic merges" is not smart enough, it assumes "merge with unrelated history" == "synthetic merge".
This behaviour is overwritten when -o merge is specified to enable imports of new "subtrees" via push.
So I'll need to revise that "synthetic merge detection"...

[RalfJung]

We have meanwhile tried -o merge with the RA history and this does indeed export the smol_str history the way we'd expect. We still get the non-roundtrip warning ofc due to the extra merge commit, but the hope is that that's the only extra commit so the situation would stabilize after a sync in both directions. However, we haven't tried the full sync sequence yet to confirm that point.

It breaks the round trip 🤔 I suspect the commit coming back is the merge commit, which is a descendant of what was pushed, but not identical.

Yes, exactly. So that's probably expected.

It comes from the "workpaces" feature: There josh does synthesize merge commits for workspace edits, but needs to treat those "synthetic merges" as non merges during push. The code that is supposed to detect those "synthetic merges" is not smart enough, it assumes "merge with unrelated history" == "synthetic merge".
This behaviour is overwritten when -o merge is specified to enable imports of new "subtrees" via push.

Ah, that does explain all the behavior we have seen.

Thanks for looking into this!

[christian-schilling]

After thinking for a while if the push handling could somehow be smarter about what to do, I decided it's best to leave it up to the user: #1588
It should be rare enough and sometimes even happen by accident, so encouraging a second look is probably a good thing.