Refactor GitHub source

[rgmz]

Description:

This PR refactors the code for the GitHub source. The main changes are:

• Consolidate information like size and visibility into a single repoInfo cache.
• Remove the unnecessary API request in visibilityOf by storing visibility information during initial enumeration (as mentioned in Assume unauthenticated github scans have public visibility #2308 (comment)).
• Move the git.CmdCheck to the top of the Init method, as there's no point in doing additional work if Git isn't installed correctly.
• Remove unnecessary logic under scanComments, such as passing in the repoUrl (string), the repoUrl (url.URL), and the repo URL in segments.
• Move hasWiki check to happen during repo scanning, not at startup
• Refactor scanErrs.String() to produce distinct error messages rather than one long string.

Checklist:

• Tests passing (make test-community)?
• Lint passing (make lint this requires golangci-lint)?

[rosecodym]

I'm guessing that this should probably wait for #2385 and possibly the resolution to #2371 in order to avoid merge conflicts.

[rgmz]

I'm guessing that this should probably wait for #2385 and possibly the resolution to #2371 in order to avoid merge conflicts.

I've moved the wiki logic from repo.go -> github.go to reduce the impact.

[mcastorina]

Thanks for taking this on, it's looking really good!

I'll take another pass once the --repo flag works.

[rosecodym]

@rgmz we've traced a number of regressions to this PR, not all of which we've fully resolved yet, and we're considering just rolling it back and re-doing it as multiple separate changes instead. Do you think there would be any unexpected negative consequences to doing this? The description doesn't read like it has anything world-saving in it, but I don't want to miss anything.

[rgmz]

Apologies, the GitHub source logic is perhaps more of a complex beast than I realized. I recently tried to unify the enumerate logic to fix behavioural inconsistencies between auth types (e.g., unauthenticated scans users passed to --org, but the other methods don't seem to.)

@rgmz we've traced a number of regressions to this PR, not all of which we've fully resolved yet, and we're considering just rolling it back and re-doing it as multiple separate changes instead. Do you think there would be any unexpected negative consequences to doing this? The description doesn't read like it has anything world-saving in it, but I don't want to miss anything.

I'm not sure. Reverting this would likely also require reverting or fixing up a few other changes.

Most of this (063b44b) would be difficult to break into smaller changes, as structural changes like adding s.repoInfoCache to reduce duplicated API calls necessitates moving a lot around. Plus, I suspect that some of the issues, such as #2625 (comment), are not net-new bugs but rather surfacing of latent ones.

I'm happy to continue to help diagnose and push fixes, but it's your call.

[rgmz]

Do you think there would be any unexpected negative consequences to doing this? The description doesn't read like it has anything world-saving in it, but I don't want to miss anything.

Most of this (063b44b) would be difficult to break into smaller changes, as structural changes like adding s.repoInfoCache to reduce duplicated API calls necessitates moving a lot around.

I may have buried the lede a bit. The primary motivation for this PR was reducing unnecessary API calls; most, but not all, of the subsequent refactoring was a consequence of that.

A negative consequence would quicker exhaustion of the GitHub rate limit.

Remove the unnecessary API request in visibilityOf by storing visibility information during initial enumeration (as mentioned in #2308 (comment)).