fix: Fix process-changes not deduplicating changes correctly #14025
Conversation
rustbot
added
the
S-waiting-on-review
Veykril
force-pushed
the
changekind-fix
branch
from
00456af to
d712e52
Compare
|
This seems more correct than before, yeah. @bors r+ |
|
☀️ Test successful - checks-actions |
| true | ||
| } | ||
| // can't really occur | ||
| (Modify, Create) => false, | ||
| (Delete, Modify) => false, | ||
| } | ||
| }); | ||
|
|
||
| if collapsed_create_delete { | ||
| changed_files.pop(); |
There was a problem hiding this comment.
I've only skimmed the changes, but does this work for [Create(f1), Delete(f1), Create(f2), Delete(f2)]?
And generally, I don't think dedup_by guarantees anything about the order of calls. Maybe we should "just" keep a map from FileId to ChangeKind, and go over the input once and update the map, without sorting it first? It might even be (asymptotically) faster. We only keep one ChangeKind per file, except for the "can't really occur" cases.
There was a problem hiding this comment.
It should work, because the flag is only true at this point if it was set to true during the last call to the closure (the beginning of the closure resets it to false).
I agree that the code is hard to follow though, and dedup_by is probably not helping with that.
There was a problem hiding this comment.
But on my example, this results in [Delete(f1)] instead of [], which is probably not desirable. collapsed_create_delete will only cause the last Delete to be dropped.
There was a problem hiding this comment.
A simple map might be a better choice here indeed
internal: Tweak change collapsing CC #14025 (comment).
probably fixes #12873 (will close it with this nevertheless as its not really reproducible)