Skip to content

Conversation

mmanela
Copy link
Contributor

@mmanela mmanela commented May 14, 2024

Fixes GRAPH-129

This PR resolves a mismatch with how scip-go would generate the version string when not pointing at a tagged module version string. This mismatch would break cross-repo references.

Background

When indexing a repo who's head commit was not tagged, scip-go sets the version as the first 12 characters of the HEAD commit sha. However, when referencing a module at that same non-tagged commit, scip-go would just set the version to the pseudo-version string.

To fix this, we will consistently use the sha1 when pointing at a non-tagged commit. If you point at a tagged commit (e.g. v1.2.3) then we do not change anything and still just use the proper version string.

Result

Below are the generated version strings when pointing at the latest commit on github.com/sourcegraph/conc

Definition

definition scip-go gomod github.com/sourcegraph/conc 5f936abd7ae8 `github.com/sourcegraph/conc/pool`/NewWithResults().

Reference

reference scip-go gomod github.com/sourcegraph/conc 5f936abd7ae8 `github.com/sourcegraph/conc/pool`/NewWithResults().

@mmanela mmanela requested a review from a team May 14, 2024 02:38
Copy link
Contributor

@varungandhi-src varungandhi-src left a comment

Choose a reason for hiding this comment

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

Oh, sorry I forgot to submit the review yesterday. 🤦🏽

@mmanela mmanela marked this pull request as ready for review May 15, 2024 19:43
Copy link
Contributor

@varungandhi-src varungandhi-src left a comment

Choose a reason for hiding this comment

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

I think this looks OK, but I think we should be explicitly stripping out the build metadata suffix, if present. That's technically a different bug, but a bug nonetheless, and it's very much related to the code you're modifying.

@mmanela mmanela merged commit fbb4bd4 into main May 16, 2024
@mmanela mmanela deleted the mmanela/GRAPH-129 branch May 16, 2024 15:56
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