tests/ui: A New Order [15/N]

[Kivooeo]

Note

Intermediate commits are intended to help review, but will be squashed prior to merge.

Some tests/ui/ housekeeping, to trim down number of tests directly under tests/ui/. Part of #133895.

r? @jieyouxu

[rustbot]

jieyouxu is not on the review rotation at the moment.
They may take a while to respond.

[tgross35]

Could you please move/rename files in a separate commit from any other updates? Otherwise the git blame+history gets completely wiped out :(

[Kivooeo]

Hm, honestly this is the first time I’m hearing about this, we should probably discuss it

Because I’m working off the guidelines I was given by @jieyouxu

(important addition 1: they weren’t exactly official guidelines, more like suggestions on how to make my PRs easier to review)

(important addition 2 following up on addition 1: since I’m someone who cares a lot about making reviews easier, I make it a priority to follow that suggestions)

I remember that in some of very first my PRs I was told to keep changes for each test in its own dedicated commit

[tgross35]

If the changes are "small" then it can be okay, e.g. a612e73 has a rename and some updates and it's okay. But for anything "big" where much of the file is getting touched, renaming needs to be separate from the changes. E.g. 3597677 it thinks you deleted a file and added an unrelated one, so history looks like this https://github.com/rust-lang/rust/commits/3597677049ee5da6c48e63f9ee4279fe7f85e3f2/tests/ui/derives/derive-Debug-enum-variants.rs. Compare this to the history for that file in master https://github.com/rust-lang/rust/commits/master/tests/ui/log-knows-the-names-of-variants.rs, it was renamed a few times but you can follow how it changed all the way back to when it was added in 2012.

"small" vs. "big" is tough to quantify since it depends on git's similarity index, so it is advisable to play it safe and always rename files in a separate commit. (Not just for this PR series, it's always a git thing.)

Fwiw I did mention this at https://rust-lang.zulipchat.com/#narrow/channel/131828-t-compiler/topic/Discussion.20for.20ui.20test.20suite.20improvements/near/523696309, no worries if you don't check Zulip too often

[Kivooeo]

Oh, you've mentioned it about like a month ago, I really didn't use Zullip that much, most of the time I even forget it exist honestly

And It's pretty relatively late because I already made about 15 PRs, but, I can start use this rule from next PR if this suits to you (I also do want to care about history, really didn't knew about this before) because rebasing this now will be a tough task

In this PR I putted links to related PRs or issues if there were interesting ones

So yeah, sorry again 😅 I Don't use zullip at all, but seems like it's time to start to

[tgross35]

No worries at all! Git can be subtle sometimes, better late than never 😆

because rebasing this now will be a tough task

Not sure how familiar you are with git, but it should let you do this reasonably elegantly if you are up for trying:

# Start a rebase
git rebase -i master --keep-base

    b # add this line so you get a chance to add a commit
    pick a612e7349d4 moved renamed docs formatted | lexical-scoping.rs
    pick 3ca5b1a7b04 moved renamed docs formatted | link-section.rs
    pick cbbc9d4eb82 deleted | log-err-phi.rs
    pick 3597677049e moved renamed docs formatted | log-knows-the-names-of-variants.rs
    pick f35f79be665 moved renamed docs formatted | logging-only-prints-once.rs
    pick cc044b603f1 moved renamed docs | loud_ui.rs
    pick 4e43f317868 moved renamed docs | max-min-classes.rs

    # save + exit

# For convenience, print the diff so you see which files changed in which commits
git log --oneline --stat $(git merge-base tf15 master)..tf15

# Make a commit with just the renames
# You don't _have_ to do all of them here, but at least the ones that conflict
git mv tests/ui/lexical-scoping.rs tests/ui/resolve/type-param-local-var-shadowing.rs
git mv tests/ui/link-section.rs tests/ui/linkage-attr/link-section-placement.rs
# ...
git commit -m "Move some UI tests to more applicable directories"

# Continue
git rebase --continue

# Now most _should_ just work. But each time it stops, just do:
git add path/to/file/after/rename/for/this/commit
git rebase --continue

# Make sure everything looks the same...
git diff origin/tf15

# ... and update!
git push --force-with-lease
git branch -d tmp-rename

[Kivooeo]

I'm definitely up to trying but little bit later today since it's a deep night for me at the moment (5 am)

Also if you have r+ rights you can review this and next PRs if you have vibe to, because jieyouxu is not mentoring this project at the moment and said

EDIT: I'm currently working on compiletest, and won't have bandwidth to mentor this for some time; if someone else is able to take up the mentoring, by all means go for it.

So I'm looking for someone until jieyouxu return

(Also, I would rate my git skills on 4-5/10)

[tgross35]

Lgtm with the history fix then. Feel free to just squash the edits if that's what you usually do, as long as the move is still a separate commit.

If Jieyou isn't able to review these for a little while then you probably don't need to r? anyone in particular, any reviewer that gets assigned should be able to okay them.

[Kivooeo]

Yeah, I saw this edit after this PR

This will be a tricky but I will try it today and ping you after

Hm, also wondering about squashing commits

If I had two commits
First Commit: move and rename test
Second Commit: all other changes (doc comment, format, etc.)

(if i get you right this is how better keep commits to save git blame)

And then before merge I squash this commits into one

How will git handle this and will it consider the file deleted?

[tgross35]

If I had two commits First Commit: move and rename test Second Commit: all other changes (doc comment, format, etc.)

Sgtm 👍 just fyi, when files move you do need to pass --follow to git log / git blame to get the whole thing (but at least it's there!)

And then before merge I squash this commits into one

If you mean squashing those two commits (edit+move together), no don't do that. It's fine to have >1 commit in a PR if it makes sense, we just don't want a bunch of super tiny commits that don't really have any reason to be separated.

I'm definitely up to trying but little bit later today since it's a deep night for me at the moment (5 am)

Please feel free to come back to this tomorrow, I don't mean to keep you up 😆

[jieyouxu]

FWIW I don't mind specifically how the commits are organized that much, if splitting into {plain move, edits} commits make git history better then great.

---

Since you're looking at it already
r? @tgross35

[Kivooeo]

@tgross35 I gave a try and it's harder than it's looks like, it's better to try this from next PRs, this hacks with adding b on top is beyond my mind

But, I still have one unclosed important question to this

If you mean squashing those two commits (edit+move together), no don't do that. It's fine to have >1 commit in a PR if it makes sense

And I need @jieyouxu to discuss it, because, it means we will have about 12-18 commits in each PR, which personally for me not sounds good

[jieyouxu]

No, I believe Trevor specifically means two commits {move-only, edits}, which is >1 commits, not 12 commits.

It's the separate move commit that helps with git history.

[Kivooeo]

Hm, so you mean something like:

Imagine we have tests tests/ui/foo.rs and tests/ui/bar.rs

In first commit I do

git mv tests/ui/foo.rs tests/ui/newplace/newname.rs
git mv tests/ui/bar.rs tests/ui/newplace/newnametwo.rs

git commit -m "moved ... files"

Then in separate commits I do

/* some changes in newname.rs /*
git commit -m "changed newname.rs"

/* some changes in newnametwo.rs /*
git commit -m "changed newnametwo.rs"

Right?

[Kivooeo]

Here's draft of next PR, it still shows some files like deleted even tho I made moves in separate commit?

https://github.com/rust-lang/rust/compare/master...Kivooeo:tf16?expand=1

[tgross35]

Here's draft of next PR, it still shows some files like deleted even tho I made moves in separate commit?

The overall diff will still show as deleted and re-added, that part is okay. You need to look at the history for a specific file; e.g. https://github.com/rust-lang/rust/commits/db989fc3ce6e618f4f835ac9e8f8f52cbb2ecbd8/tests/ui/codegen/maximal-hir-to-mir-coverage-flag.rs, notice how it has "Renamed from tests/ui/maximal_mir_to_hir_coverage.rs" at the bottom. That's all correct!

@tgross35 I gave a try and it's harder than it's looks like, it's better to try this from next PRs, this hacks with adding b on top is beyond my mind

If you are having rebase problems, I'm happy to do the fix and push your branch - just ask :) but where did you get stuck?

---

Purely for the mental model, don't worry if it doesn't make much too much sense, but this is basically what happens when you do a rebase:

1. Git figures out the list of commits to be rebased. When you do git rebase -i ..., this is the list it's showing you (aka the "rebase todo")
2. Git figures out what the "new" base is, i.e. what the new parent of the first commit in that list is. It can be the same one! "rebase" doesn't necessarily have to change the base.
3. It checks out that "new" base
4. It goes through each commit and tries to apply the diff. This is what p/pick in the rebase todo means.
   • If it applies cleanly (no conflicts), it creates a new commit with the same message
   • If it can't apply cleanly (conflicts with the new parent), it stops with "error: could not apply abcd1234 ...". This means you need to manually resolve conflicts, git add the new changes, and git rebase --continue to tell it to keep on rebasing
5. b just means to pause rebasing wherever it is at so you can do whatever. You can put bs/breaks anywhere in the rebase todo, e.g. after each commit to make sure things build after each commit (a requirement for e.g. the Linux kernel). You can also do something like create a new commit which the rest of the rebase will continue on top of (that's what I was going for)

^ looking at git log ("done" things) and git rebase --edit todo (remaining things) during a rebase can kind of help you wrap your head around it all

[Kivooeo]

(I will read the explanation after I send this message so I'm just trying to explain what goes wrong when I tried to make it)

If you are having rebase problems, I'm happy to do the fix and push your branch - just ask :) but where did you get stuck?

I stucked on a very first step

git rebase -i master --keep-base

It started rebasing 5 thousand files

I thought I was a mistake and did instead

git rebase -i head~7

Which worked better

then it open me a this windows you showed in guide (with commits and pick word, I don't really know how this windows called, I guess rebase editor maybe?)

I added a just b on first line

And it moved head on commit that was before my first commit, which is correct as far as I understand...

But then I don’t know how to rewrite the history so that the file move ends up in a single initial commit, keeping only the content changes in the subsequent commits (hopefully I explained this part clealy because my english is not that good)

Also I’m little confused about adding b on top — doesn’t that mean we’d include the base commit in our PR as well?

[Kivooeo]

I'm doing git rebase -i head~N pretty often to be clear, for exapmple for squash all commits, just edit one single commit to fix some nits, or even just rename commit

But this one sounds more complex than such small fixes, if for a person who knows git well it might be an easy task, but for me it's more like a stressfull adventure in dark forest 😅

[tgross35]

For any git thing there are 1000 ways to get the same results 😆 I'm happy to help you get there, but I'm also happy to do it for you because I know this kind of thing is tricky and we certainly don't expect all contributors to know git like the back of their hand.

I stucked on a very first step

git rebase -i master --keep-base

It started rebasing 5 thousand files

I thought I was a mistake and did instead

git rebase -i head~7

Yeah you want the same results as head~7. ... master --keep-base should give you the same thing without manually counting; it's just a shortcut to make the base the point where you branched from master. But if it's giving you thousands of commits, your local master probably isn't up to date so it's a good thing you stopped!

So maybe you need to replace master with upstream/master, or origin/master, or whatever you usually branch off from and keep ~reasonably up to date (personally origin is my fork, upstream is rust-lang/rust). Or just keep doing HEAD~7, but nobody else is counting commits like that 😆

then it open me a this windows you showed in guide (with commits and pick word, I don't really know how this windows called, I guess rebase editor maybe?)

I added a just b on first line

And it moved head on commit that was before my first commit, which is correct as far as I understand...

All sounds correct!

But then I don’t know how to rewrite the history so that the file move ends up in a single initial commit, keeping only the content changes in the subsequent commits

Once you're at this point, you want to create a new commit that never existed before, which just moves all the files. You can do anything at a break; create commits, delete/revert commits, run tests, merge things, cherry picks, etc. Once you run git rebase --continue, it will try to apply the rest of the commits on top of the current state.

So do git mv old/path new/path, add all these files, and commit. git rebase --continue will try to apply each remaining commit on top of these moved files. Some will get stuck (the ones where git doesn't realize it's the same file); that's expected. You can do something like git checkout tf15 -- path/to/new/file to just get the latest version of that file. Add it, then git rebase --continue after you resolve the conflict.

(as a note, it might make things slightly easier if you squash before doing this since there will be a single conflict commit then, and you can git checkout tf15 -- . to get everything rather than going one-by-one)

(hopefully I explained this part clealy because my english is not that good)

Never apologize, it's quite alright :)

Also I’m little confused about adding b on top — doesn’t that mean we’d include the base commit in our PR as well?

break doesn't do anything with the history on its own, you could add 20 b lines anywhere and the result would be identical. GH more or less looks for the first commit on a branch that doesn't exist on master to figure out what to show in a PR, so there's no way the base commit would be included (since the base is a point in master).

[Kivooeo]

The reason I'd think that this b thing adds a commit before to my PR is because when we do git rebase -i head~N (and yes I use lowercase for head, I just noticed every example is using uppercase) where N is more commits than I've made then It also includes them into my PR, and I have to do git reflog to find a commit hash before this rebase command to revert it

And yeah, also if you could just push this changes in this branch it will make all easier and I could continue working on it with an easy heart doing separate commit with plain moves from start

[tgross35]

Pushed! Basically exactly #143118 (comment) for reference.

(I'll help with the rest tomorrow, going offline for the day)

[Kivooeo]

Thanks!

[tgross35]

Mind squashing the rest? The actual changes lgtm #143118 (review), I just didn't want to lose the history

[Kivooeo]

Sure I squash commits that don't move anything

[Kivooeo]

Also I should clarify one thing, you made moves in first commit so I can safely squash rest of them? So we will have just two commits, first with moves you made and second commit with changes (and moves that dont recognize by git as deleted file)

(asking to be giga very super sure because unsquashing will not be most fun task)

[Kivooeo]

Since our working hours are so different, I think the best solution to this situation is this:

If possible and if you have free time and vibe to review it (because it's not most fun task to do), you do a review to PRs and if everything is fine or needs to be fixed, you write about it and delegate the approval rights to me.

Then, in my free time, I look and fix if there is anything and then squash commits and then approve it myself

In my opinion this is very smooth workflow, something like this were done here #142429 (comment) successfully

Also I'd love to hear your rate of ten to this solution

[tgross35]

Also I should clarify one thing, you made moves in first commit so I can safely squash rest of them? So we will have just two commits, first with moves you made and second commit with changes (and moves that dont recognize by git as deleted file)

Yes, squashing everything except the first commit together is good :)

[Kivooeo]

@rustbot ready

[tgross35]

Thanks!
@bors r+ rollup

[bors]

📌 Commit 580bc12 has been approved by tgross35

It is now in the queue for this repository.