Skip to content

Improve MetadataProvider.getVersions(...) logic#46

Merged
WinPlay02 merged 6 commits intoWinPlay02:masterfrom
0x189D7997:getversionsassumeloaded
Dec 21, 2025
Merged

Improve MetadataProvider.getVersions(...) logic#46
WinPlay02 merged 6 commits intoWinPlay02:masterfrom
0x189D7997:getversionsassumeloaded

Conversation

@0x189D7997
Copy link
Contributor

As discussed in #31, calling MetadataProvider.getVersions(Executor) with null executor when assuming the versions are already loaded can potentially cause a hard-to-trace exception.

This PR adds getVersionsAssumeLoaded() variant specifically for this use case which fails explicitly if called before versions are loaded. The executor in initializeAndLoadVersions(Executor) is also checked for being null and versionsLoaded flag is now set only after all loading steps are complete for thread safety.

0x189D7997 added a commit to 0x189D7997/GitCraft that referenced this pull request Dec 18, 2025
Copy link
Owner

@WinPlay02 WinPlay02 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for taking care of this. Other than the tests that needed adjustment, this looks good

@WinPlay02 WinPlay02 merged commit 4df6c80 into WinPlay02:master Dec 21, 2025
1 check passed
@0x189D7997
Copy link
Contributor Author

0x189D7997 commented Dec 22, 2025

@WinPlay02 I am sorry for not taking care of tests myself, I didn't want to overuse CI and forgot to account for this.

There is still a smaller issue with them, when ran individually, some tests such as versionGraphFilter can still fail with "getVersionsAssumeLoaded() called but the versions are not loaded", same fix is needed for them too.

Edit: I opened #47 for this.

0x189D7997 added a commit to 0x189D7997/GitCraft that referenced this pull request Dec 23, 2025
WinPlay02 added a commit that referenced this pull request Dec 23, 2025
@0x189D7997 0x189D7997 deleted the getversionsassumeloaded branch December 26, 2025 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants