NodeSupport: Further improve determining VCS info #8333
Conversation
fviernau
force-pushed
the
node-support-handle-textual-node
branch
2 times, most recently
from
22b63e7 to
5669cc1
Compare
fviernau
force-pushed
the
node-support-handle-textual-node
branch
from
5669cc1 to
7c7d700
Compare
fviernau
changed the title
...ns/package-managers/node/src/funTest/assets/projects/synthetic/npm-babel-expected-output.yml
Show resolved
Hide resolved
| revision = head, | ||
| path = path | ||
| revision = head.takeIf { it.isSha1() } ?: vcsInfo.revision.takeIf { it.isSha1() }.orEmpty(), | ||
| path = path.takeIf { it.isNotBlank() } ?: vcsInfo.path |
There was a problem hiding this comment.
I like this line, though. We can simplify to isEmpty() probably.
|
I've slit-out handling textal repository nodes: #8512 |
Improve algorithm for selecting the VCS info properties as follows: 1. Use `VcsHost.parse()` to also obtain the `type`, `revision` and `path` from the URL in case of a textual repository node. 2. Use only revision strings which are SHA1 sums. This avoids having 'master' as revision. In such case using "" as revision to rely on the tag matching seems to be better. 3. Improve the fallback for the VCS path by deriving the path either from the given URL in addition to from the dedicated `directory` property. Signed-off-by: Frank Viernau <[email protected]>
fviernau
force-pushed
the
node-support-handle-textual-node
branch
from
7c7d700 to
a0cf897
Compare
| if (repository.isTextual) { | ||
| val url = expandNpmShortcutUrl(repository.textValue()) | ||
| val vcsInfo = VcsHost.parseUrl(url) | ||
| return vcsInfo.copy(revision = head.takeIf { it.isSha1() } ?: vcsInfo.revision.takeIf { it.isSha1() }.orEmpty()) |
There was a problem hiding this comment.
Maybe something like this instead (similar in line 171):
listOf(head, vcsInfo.revision).firstOrNull { it.isSha1() }.orEmpty()
| return vcsInfo.copy(revision = head.takeIf { it.isSha1() } ?: vcsInfo.revision.takeIf { it.isSha1() }.orEmpty()) | ||
| } | ||
|
|
||
| val type = VcsType.forName(repository["type"].textValueOrEmpty()).takeUnless { it == VcsType.UNKNOWN } |
There was a problem hiding this comment.
At this point, we know that repository is an object node with mandatory type and url nodes, so we could use just textValue() instead of textValueOrEmpty() here any below.
There was a problem hiding this comment.
Maybe move the .takeUnless { it == VcsType.UNKNOWN } part to line 169 to highlight why the ?: fallback is needed there?
|
|
||
| private val SHA1_REGEX = "[0-9a-f]{7,40}".toRegex() | ||
|
|
||
| private fun String.isSha1(): Boolean = SHA1_REGEX.matches(this.lowercase()) |
There was a problem hiding this comment.
Th explicit this. should be dropped here.
|
After looking at it again, the changes take effect in our funTest projects primarily on unprocessed VCS while processed VCS remains the same. So, I've double checked whether the changes are needed for the scans for which I've originally developed the PR for, but it turned out the scans figure out metadata successfully without this PR. So, closing this as not needed / change unprocessed VCS only is not desired. |
Adjust the fallback algorithm in various ways mainly to:
sha1, to avoid branch names asvcs.revisionsuch as "master" / "main".vcs.pathfrom the URLsvcs.typefrom the URLsFor our test data (see diff) this seems to be an improvement in all cases.