Export git.handleBinary and getSafeRemoteURL #3921
Conversation
rosecodym
changed the title
| commitHash := plumbing.NewHash(fullHash) | ||
|
|
||
| if s.skipBinaries || feature.ForceSkipBinaries.Load() { | ||
| logger.V(5).Info("skipping binary file", | ||
| "commit", commitHash.String()[:7], | ||
| "path", path) | ||
| continue | ||
| } |
There was a problem hiding this comment.
If the use of this chunk of code grew to more than two places, would you write a separate function for it?
There was a problem hiding this comment.
Great question! I would certainly consider it - three is a good threshold for that. In this particular case I might not, though, because the repetitive code is so small and simple. I would still need to invoke the new function from each of these call sites, and doing that wouldn't be more much code than just inlining the logic the way it's done here. And since the extracted function itself wouldn't need much updating, there wouldn't be much of a maintainability benefit to extracting it.
Zooming out, I think the fact that we have to consider these questions is itself a bit of a code smell that suggests that some re-architecture might be warranted. (However, doing that would be substantial, risky effort, so it might not be warranted!)
Description:
This PR exports some functionality from the git source so that it's usable in a (new, under development) git-flavored source that does not need to wrap an entire git source in order to operate.
One is
getSafeRemoteURL. This is a straightforward change.The other is
handleBinary. Right now, all git binary file handling is invoked fromGit.ScanRepo, which is itself invoked by our various git-flavored sources.Since the actual binary file handling function
handleBinaryonly used two pieces of information from the "git" source, I just removed its receiver. One of the pieces of information was a flag that caused the function to be skipped entirely; I moved this one to the (two) call sites. The other I just forwarded as an argument.Checklist:
make test-community)?make lintthis requires golangci-lint)?