Crate marked as yanked when both yanked and unyanked in one index-update

[Nemo157]

It appears we process the yank and unyank events in the wrong order when they come together in one update:

2023-10-30T01:43:24.525568Z DEBUG docs_rs::build_queue: queueing changes from 0e09b092780cfd28f4d9557f7e06e9fbc1c248c7 to 3f9de8e33bc8be081b6a480a75464b908b439bfc
2023-10-30T01:43:24.531502Z DEBUG docs_rs::build_queue: dylink-0.8.0 unyanked
2023-10-30T01:43:24.536981Z DEBUG docs_rs::build_queue: dylink-0.8.0 yanked

We're explicitly reversing the changes:

docs.rs/src/build_queue.rs

Lines 293 to 294 in 4514194

	// I believe this will fix ordering of queue if we get more than one crate from changes
	changes.reverse();

Maybe this order was changed in a crates-index-diff update?

[syphar]

@Byron you have more info here?

[syphar]

Also, related discussion on zulip: https://rust-lang.zulipchat.com/#narrow/stream/356853-t-docs-rs/topic/.E2.9C.94.20unyank.20docs.2Fdoc.28cfg.29.20ignored

[Byron]

I will take a look ASAP, but vaguely remember that there is a test for this already so I doubt something regressed. But who knows, maybe it's something new that never worked and we just weren't aware. As long as I can still find the respective operations/commits in the crates-index repo I should be able to make it reproducible.

[syphar]

Thanks for checking!

For me it would be good enough to know in which order the updates will be emitted by crates-index-diff, then we just adapt our logic

[Byron]

After taking a closer look I am quite sure that this can't be adapted to, not in its current form. The documentation states that multiple changes for a single crate are emitted in any order. This was alright in the past I suppose but I also wonder how the baseline test can work as perfectly as it does if yanks and unyanks are out of order - most changes are actually ordered nonetheless as it's additions or single-line modifications though.

Tests also deal with the problem by sorting beforehand to get a deterministic order, but now this needs to change to be deterministic. Let's see if I can do that without hurting performance too much (and even if so, performance is clearly secondary here).

[Byron]

That's strange: peek_changes_ordered() already single-steps through the history, and in casae of dylink there is only one change per commit. It first gets yanked, then it gets unyanked, and that order will definitely be reproduced then.

The changes.reverse() will reverse the order, which explains the problem here.

However, this line seems to also have solved a problem, but it might have been coincidence.

The problem remains: should there be deterministic ordering between line-based changes? The answer here is most certainly: Yes, so I will see to it, which should allow the line 294 to be removed without side-effects.

[Byron]

Alright, it took me a while to realise my naiveté in trying to impose any ordering of changes within a diff and assume it's more than 'ordered by line number', but at least it's deterministic then.

I also don't know what line 294 is trying to solve, maybe figuring that out helps to solve the true underlying problem.

My adjustment right now makes the order of multiple changes to a crate deterministic, as it will order them by line number, ascending. And some determinism is certainly an improvement as to not trick anybody into imposing their own ordering for that reason alone.

[Byron]

I have released a new version which uses the latest gix just like docs.rs and makes per-crate changes deterministic in order.

There is definitely no need to reverse the order as done in line 294 as crates-index-diff provides the order created by stepping through commits, and now additionally orders per-crate changes by line number, ascending so it is deterministic as well.

I hope that helps and that soon there will be no issues whatsoever related to the tracking of changes to crates 😅.