Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mega-sync from rust-lang/rust #13676

Merged
merged 56 commits into from
Nov 25, 2022
Merged

Conversation

fasterthanlime
Copy link
Contributor

@fasterthanlime fasterthanlime commented Nov 25, 2022

This essentially implements @oli-obk's suggestion here #13459 (comment), with @eddyb's help.

This PR is equivalent to 14 syncs (back and forth) between rust-lang/rust and rust-lang/rust-analyzer.

Working from this list (from bottom to top):

(x) a2a1d9954 :arrow_up: rust-analyzer
(x) 79923c382 :arrow_up: rust-analyzer
(x) c60b1f641 :arrow_up: rust-analyzer
(x) 8807fc4cc :arrow_up: rust-analyzer
(x) a99a48e78 :arrow_up: rust-analyzer
(x) 4f55ebbd4 :arrow_up: rust-analyzer
(x) f5fde4df4 :arrow_up: rust-analyzer
(x) 459bbb422 :arrow_up: rust-analyzer
(x) 65e1dc4d9 :arrow_up: rust-analyzer
(x) 3e358a682 :arrow_up: rust-analyzer
(x) 31519bb39 :arrow_up: rust-analyzer
(x) 8231fee46 :arrow_up: rust-analyzer
(x) 22c8c9c40 :arrow_up: rust-analyzer
(x) 9d2cb42a4 :arrow_up: rust-analyzer

(This listed was assembled by doing a git subtree push, which made a branch, and looking at the new commits in that branch, picking only those that were :arrow_up: rust-analyzer commits)

We used the following commands to simulate merges in both directions:

TO_MERGE=22c8c9c40 # taken from the list above, bottom to top
git merge --no-edit --no-ff $TO_MERGE
git merge --no-edit --no-ff $(git -C ../rust log --pretty=format:'%cN | %s | %ad => %P' | rg -m1 -F "$(git show --no-patch --pretty=format:%ad $TO_MERGE)" | tee /dev/stderr | rg '.* => \S+ (\S+)$' --replace '$1')

We encountered no merge conflicts that Git wasn't able to solve by doing it this way.

Here's what the commit graph looks like (as shown in the Git Lens VSCode extension):

image

This PR closes #13459

Does this unbreak rust->ra syncs?

Yes, here's how we tried:

In rust-analyzer:

  • check out subtree-fix (this PR's branch)
  • make a new branch off of it: git checkout -b subtree-fix-merge-test
  • simulate this PR getting merged with git merge master

In rust:

  • pull latest master
  • make a new branch: git checkout -b test-change
  • mess with rust-analyzer (I added a comment to src/tools/rust-analyzer/Cargo.toml)
  • commit
  • run git subtree push -P src/tools/rust-analyzer ra-local final-sync (this follows the Clippy sync guide)

This created a final-sync branch in rust-analyzer.

In rust-analyzer:

Now git log in rust-analyzer shows this:

commit 460128387e46ddfc2b95921b2d7f6e913a3d2b9f (HEAD -> subtree-fix-merge-test)
Merge: 0513fc02a 9ce6a734f
Author: Amos Wenger <[email protected]>
Date:   Fri Nov 25 13:28:24 2022 +0100

    Merge branch 'final-sync' into subtree-fix-merge-test

commit 0513fc02a08ea9de952983624bd0a00e98044b36
Merge: 38c98d1ff 6918009fe
Author: Amos Wenger <[email protected]>
Date:   Fri Nov 25 13:28:02 2022 +0100

    Merge branch 'master' into subtree-fix-merge-test

commit 9ce6a734f37ef8e53689f1c6f427a9efafe846bd (final-sync)
Author: Amos Wenger <[email protected]>
Date:   Fri Nov 25 13:26:26 2022 +0100

    Mess with rust-analyzer just for fun

And git diff 0513fc02a08ea9de952983624bd0a00e98044b36 shows this:

diff --git a/Cargo.toml b/Cargo.toml
index 286ef1e7d..c9e24cd19 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -32,3 +32,5 @@ debug = 0
 # ungrammar = { path = "../ungrammar" }
 
 # salsa = { path = "../salsa" }
+
+# lol, hi

Does this unbreak ra->rust syncs?

Yes, here's how we tried.

From rust:

  • git checkout -b sync-from-ra
  • git subtree pull -P src/tools/rust-analyzer ra-local subtree-fix-merge-test (this is adapted from the Clippy sync guide, you would normally use ra-upstream master but we're simulating things here)

A commit editor pops up, there was no merge conflicts.

How do we prevent this from happening again?

Like @bjorn3 said in #13459 (comment)

Whenever syncing from rust-analyzer -> rust you have to immediately sync the merge commit from rust -> rust-analyzer to prevent merge conflicts in the future.

But if we get it wrong again, at least now we have a not-so-painful way to fix it.

fasterthanlime and others added 30 commits July 26, 2022 11:53
This removes some RPC when creating and emitting diagnostics, and
simplifies the bridge slightly.

After this change, there are no remaining methods which take advantage
of the support for `&mut` references to objects in the store as
arguments, meaning that support for them could technically be removed if
we wanted. The only remaining uses of immutable references into the
store are `TokenStream` and `SourceFile`.
…nother-type, r=estebank

Point at a type parameter shadowing another type

This patch fixes a part of #97459.
proc_macro/bridge: send diagnostics over the bridge as a struct

This removes some RPC when creating and emitting diagnostics, and
simplifies the bridge slightly.

After this change, there are no remaining methods which take advantage
of the support for `&mut` references to objects in the store as
arguments, meaning that support for them could technically be removed if
we wanted. The only remaining uses of immutable references into the
store are `TokenStream` and `SourceFile`.

r? `@eddyb`
This fixes a typo first appearing in #94624
in which test-macro diagnostic uses "a" article twice.

Since I searched sources for " a a " sequences,
I also fixed the same issue in a few source files where I found it.

Signed-off-by: Petr Portnov <[email protected]>
initial josh subtree sync

This demonstrates what a josh-based rustup would look like with my patched josh. To create it I did
```
git fetch http://localhost:8000/rust-lang/rust.git:start=75dd959a3a40eb5b4574f8d2e23aa6efbeb33573[:prefix=src/tools/miri]:/src/tools/miri.git master
git merge FETCH_HEAD
./rustup-toolchain HEAD && ./miri fmt
git commit -am rustup
```
Unlike the [previous attempt](rust-lang/miri#2554), this does not add a new root commit to the repo.

Once we merge this, we committed to using josh for subtree syncing, and in particular a version of josh that includes josh-project/josh#961 (or something compatible).
It's easy to just use `unescape_literal` + `byte_from_char`.
Unescaping cleanups

Some code improvements, and some error message improvements.

Best reviewed one commit at a time.

r? ````@matklad````
@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 25, 2022
@Veykril
Copy link
Member

Veykril commented Nov 25, 2022

Thank you very much for spending the time looking into this! (both of you :) )

r? @lnicola

@lnicola
Copy link
Member

lnicola commented Nov 25, 2022

One issue I have with this is not being able to exclude commits. For example, I don't know if 3a1aa37 is something we want, but I understand how it happened downstream.

There's one more thing I want to try, i guess I'll be able to say more afterwards. But thanks for spending time on this.

@fasterthanlime
Copy link
Contributor Author

One issue I have with this is not being able to exclude commits. For example, I don't know if 3a1aa37 is something we want, but I understand how it happened downstream.

Looks like a drive-by fix that happened in rust-lang/rust#100643

I think the right time to review these changes is before they get merge into rust-lang/rust — I'm sure they have automated rules to notify a group of people if a certain path is touched, that seems like a good idea.

I would advise simply reverting that change in rust-lang/rust-analyzer after the "maintainers notification" is set up.

I don't think we can get both of "pick which commits we like" and "sync seamlessly back and forth". The former seems like an organizational problem the rust project has tools for.

@lcnr
Copy link
Contributor

lcnr commented Nov 25, 2022

to set up maintainer notifications you can add yourself to the triagebot.toml file in the rust repo, e.g. rust-lang/rust#104882

@eddyb
Copy link
Member

eddyb commented Nov 25, 2022

One issue I have with this is not being able to exclude commits. For example, I don't know if 3a1aa37 is something we want, but I understand how it happened downstream.

Please do not exclude commits, it's very important for the sync continuity that you never edit/rebase/remove commits generated by git subtree.

You can always do git revert 3a1aa376c55cd070474a8488a2cf30a600beeb2f, but not remove 3a1aa37 from the commit history, it's deterministically generated by git subtree every time you run git subtree push on the Rust side, and if you remove it when all the commits after it will change identity and make a horrible mess.

I would suggest swiftly merging this PR (after reverting whatever unwanted changes, tho if tests pass that can be done later too) to bring the two repos in sync once more, after which weekly syncs can resume as normal.

(The current state of the PR is non-negotiable for allowing the subtree workflow, but we're talking about the commit graph here not the changes, which can be adjusted as-needed - e.g. adding revert commits to PR would result in the next RA -> rust-lang/rust sync to also include those new revert commits)

@Veykril
Copy link
Member

Veykril commented Nov 25, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Nov 25, 2022

📌 Commit 38c98d1 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 25, 2022

⌛ Testing commit 38c98d1 with merge b651646...

@bors
Copy link
Contributor

bors commented Nov 25, 2022

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing b651646 to master...

@bors bors merged commit b651646 into rust-lang:master Nov 25, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 28, 2022
notify the rust-analyzer team on changes to the rust-analyzer subtree

As proposed in rust-lang/rust-analyzer#13676 (comment)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Nov 29, 2022
notify the rust-analyzer team on changes to the rust-analyzer subtree

As proposed in rust-lang/rust-analyzer#13676 (comment)
@lnicola lnicola mentioned this pull request May 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.