Version ordering and version graph fixes

[0x189D7997]

This PR evolved to include multiple fixes for bugs related to incorrect ordering of versions, especially when using non-mojang manifest sources. I decided to include fixes for version graph here too because they are closely related and dependent on ordering fixes.

Previous description

Omniarchive manifest contains versions such as `19w13b+1653` which get matched to `null` by `BaseMetadataProvider.getVersionByVersionID(String versionId)`.

OmniarchiveMetadataProvider inherits getParentVersions(...) from MojangLauncherMetadataProvider and that results in a list containing null values causing a NullPointerException down the line (in MinecraftVersionGraph.isValidEdge(...)).

I have fixed this in MojangLauncherMetadataProvider instead of overriding the method in OmniarchiveMetadataProvider because a list with nulls is usually not a good or intended thing.

This was already accounted for in SkyrisingMetadataProvider by an override doing the same filtering.

[SpaceWalkerRS]

While this fixes the crash, there's another issue here, which is that getParentVersions returns the wrong value for some versions (since the Mojang manifest id differs from the Omniarchive manifest id for some versions). You'll want to add 19w13b+1653 and other cases of this to Omniarchive.getParentVersionIds.

[SpaceWalkerRS]

In this case you just need to figure out the correct version ids. MojangLauncherMetadataProvider.getParentVersionIds has a bunch of special cases. Apparently some of the returned values are valid version ids for the Mojang manifest but invalid version ids for the Omniarchive manifest. You can browse the Omniarchive manifest to find and compare the version ids and add special cases to OmniarchiveMetadataProvier.getParentVersionIds.

[SpaceWalkerRS]

Now I think that I should replace null filter with non-null assertion so something like this doesn't stay uncaught in the future.

I think that's a good idea. The filter in SkyrisingMetadataProvider was added because Alpha and Classic server versions are filtered out altogether, but they are listed in the "details" JSON files that the provider uses to determine the ordering.

I reckon the proper solution is to replace the non-null filter with a general version filter that is also used by the shouldExclude method (the idea is to ensure both filters have the same behavior), and then followed by your idea or a non-null assertion.

[0x189D7997]

@SpaceWalkerRS There is also a problem with mojang_historic.

Trying to run it both with and without my changes results in this:
During validating of the graph, not all vertices were visited. This is most likely caused by an inconsistency, e.g. a cycle in the graph. Last vertex checked: 1.8.3 (1.8.3) (problem may be near that vertex)

So far I have not pinned it but I don't want to open a separate issue yet because this might be related and be fixed in this PR.

[SpaceWalkerRS]

Actually about them: their semver is actually 2.0-<color> and whenever they end up in a TreeSet<OrderedVersion> they end up last. Maybe it is worth adding them minecraftVersionSemVerOverride as 1.5.2-alpha.13.14.<color>?

I think that would be fine. I don't think it will affected the ordering at all since their parent version is set explicitly in getParentVersionIds.

Also there is 1.0.0-tominecon. It gets ordered correctly because alphabetically it comes after an rc and semantically before a clean version. Should it be ordered by getParentVersionIds explicitly?

If it gets sorted fine automatically I wouldn't bother with it.

Trying to run it both with and without my changes results in this:
During validating of the graph, not all vertices were visited. This is most likely caused by an inconsistency, e.g. a cycle in the graph. Last vertex checked: 1.8.3 (1.8.3) (problem may be near that vertex)

That's a nasty issue. And the log message isn't actually that helpful I've found, the last visited version is rarely actually related to the issue. If it happens without your changes too I would just investigate it separately and open a new PR for it. I might have some time to look into it as well.

[0x189D7997]

I think that would be fine. I don't think it will affected the ordering at all since their parent version is set explicitly in getParentVersionIds.

It affects the order when automatically naming a repo, not sure if that actually matters.
For example when given --only-version=1.5.1,2.0-blue,1.21.10 the name will be minecraft-repo-<mappings>-<manifest>-1.5.1-1.21.10-2.0-blue.
Maybe the naming should be based on version graph and not just a TreeSet? Flatten the graph and the order should be correct.

Other then this the PR should be ready. I will try to deal with graph error separately.

Edit: add example

[0x189D7997]

PR should be ready

Or not. Found 2 more small issues worth fixing here:

• 22w13oneblockatatime is a fork of 1.18.2 not 22w13a as specified in parent overrides. Backed by wiki, Fabric's semver and my own inspection of the decompiled code. This causes a commit order error under rare circumstances when it is included together with nearby versions.
• '15w14a' is treated as a regular snapshot and not an april fools one. It should be included in shouldExcludeFromMainBranch(...).

And another suggestion:
As it turns out the wiki is technically wrong about 1.RV-Pre1. When I carefully compared the changes in code it turned out to be not a fork of 1.9.2 but of somewhere inbetween of 1.9.1 and 1.9.2. For better representation I think it will be better to make it branch from 1.9.1.

[SpaceWalkerRS]

this is looking good

[0x189D7997]

this is looking good

There's still a big issue that needs custom logic. Because 1.18 experimental snapshots were merged, their branching point (1.17.1-pre1) has to be included with them if the merging point (21w37a) or anything later is.

This is exactly what --min-version=1.17.1 does and fails when committing to repo.
Can't merge branches with orphan roots.

When filtering ranges we should check for such merges and exclude orphan branches. Only exclude if merging point is included, otherwise this is fine.
When requested by user with --only-version or --exclude-version this should fail with explanation message.

[0x189D7997]

This is getting cluttered, I will be making a review explaining each individual change later.

[0x189D7997]

There's still a big issue that needs custom logic. Because 1.18 experimental snapshots were merged, their branching point (1.17.1-pre1) has to be included with them if the merging point (21w37a) or anything later is.

This needs a 'walk forward to tip' method in MinecraftVersionGraph. I could not implement it with current logic, relying on pathsToTip being stored only for branch points to detect them like walkBackToBranchPoint does just doesn't allow it.

Current branch finding logic also incorrectly marks branches which had main branch merged into them (combat experimental snapshots).
To make it work for me I had to rewrite it completely and make it much much simpler. My take on it can be found here: master...0x189D7997:GitCraft:rewrite-branchfinding

@SpaceWalkerRS I want you to look at this and decide if this should be implemented, and if so I will open a PR. From my testing it works flawlessly.

[0x189D7997]

@SpaceWalkerRS I decided to split this in three parts to make merging easier and avoid clutter:

• Generic fixes for version order
• Fixes for Omniarchive specifically
• A complete rewrite of logic that handles branch structure in MinecraftVersionGraph

I will publish these PRs individually later.