Fix scan-remote-repo --branch logic #250
Conversation
A fresh-cloned repository does not have branch information for remote branches. We must follow the information in `remotes.origin.refs` instead of `branches` (which will have only the main/master branch). We add a new `is_remote` flag so the common code knows which strategy to employ. A side-effect appears to be that because HEAD appears in the reference list, we'll scan the default branch twice, but I can live with this.
| ) | ||
| else: | ||
| # Everything | ||
| branches = unfiltered_branches |
There was a problem hiding this comment.
Will this truly only be branches, or might this possibly include refs to PRs, tags, etc?
There was a problem hiding this comment.
It might be excessive (I know in simple testing that HEAD showed up in the references list). However, I feel this is "harmlessly excessive":
- If user specifies a branch, we are looking for a reference with the name of the branch, and therefore will scan only the branch the user asked us to scan
- If the user didn't, we'll scan "everything" (which is what is intended). We might end up scanning parts of "everything" multiple times, to the extent multiple references appear -- in which case I hope most of this will be short-circuited by the "I've seen this diff before" lru cache and the impact will be minimized.
The fix for v3.x is much better than this one, so I'm willing to sacrifice some efficiency for correctness in the short term, knowing that when people transition to v3 it won't be a problem any longer.
There was a problem hiding this comment.
Subtle change: In testing, I noted that the fresh-cloned scratch repo had only a main (or presumably master) branch defined, so the old default scan-remote-repo (without a branch) actually was probably only scanning the master branch and not the entire repository -- so people might be surprised to discover things getting scanned now that weren't scanned (at least not since v2.0.2).
There was a problem hiding this comment.
I think it's likely there will be a number of surprises for users as they move to v3.0 and find how much more accurate it is. 😄
|
(Ignore that last part of my comment -- just saw the other PR!) |
To help us get this pull request reviewed and merged quickly, please be sure to include the following items:
PR Type
What kind of change does this PR introduce?
Backward Compatibility
Is this change backward compatible with the most recently released version? Does it introduce changes which might change the user experience in any way? Does it alter the API in any way?
Issue Linking
fixes #247
What's new?
remotes.origin.refsinstead ofbranches(which will have only the main/master branch).is_remoteflag so the common code knows which strategy to employ.HEADappears in the reference list, we'll scan the default branch twice, but I can live with this.