erepo: cache all read only external repos by hexsha #3286
Conversation
dvc/scm/git/__init__.py
Outdated
There was a problem hiding this comment.
Hm, when does it happen though?
There was a problem hiding this comment.
For text search, see test_diff.test_directories. There are also dates in @{...} format.
So we have 3 things cached now separately: - clean clones, to not ask for creds repatedly - cache dirs, also shared between erepos with same origin - checked out clones if they are read only, addressed by (url, hexsha) Several additions to `Git` along the way: - Git.is_sha() static method - .pull() and .push() work with multiple returned records correctly - .get_rev() and .resolve_rev() work faster - .resolve_rev() looks for remote branches - .has_rev() Fixes treeverse#3280.
|
Everything should be addressed now. |
It follows `git checkout` logic now - if name can be unambiguously resolved across known remotes then it's done.
skshetry
left a comment
skshetry
left a comment
There was a problem hiding this comment.
Looks good. Just one question. 🙂
|
Oops, forgot to mention. Can we have at least a test that no cloning is done for the same rev? Maybe, just an assert that the cloned directory is same when rev is same? with external_repo("/tmp/repo", "master") as repo1:
pass
with external_repo("/tmp/repo", "master") as repo2: # or, even better, check here with hash?
pass
assert repo1.root_dir == repo2.root_dir |
Not sure we should test that, this is an implementation detail. EDIT. Also, we were not cloning twice before this PR, clones were cached. This PR caches copies from a cached clone |
| for remote in self.repo.remotes | ||
| } - {None} | ||
| if len(shas) > 1: | ||
| raise RevError("ambiguous Git revision '{}'".format(rev)) |
There was a problem hiding this comment.
IIRC git choses the ref-like rev if it can, even in ambigous case. It does print a warning though. Not sure if it matters that much here.
efiop
left a comment
efiop
left a comment
There was a problem hiding this comment.
Looks good. Let's merge and move on. Not happy about the lack of tests, but that is on you 🙂
I'd argue, it's not an implementation detail but a behavior that is reasonable and expected, a subtle but important distinction because if I refactor this, I'd like to keep this behavior intact (without a need to change test code). How'd you prevent this from regressing? |
|
@Suor #3280 (comment) So we should've added tests, right? 😉 |
|
:D just posted #3280 (comment) before reading this |
4 participants
So we have 3 things cached now separately:
Several additions to
Gitalong the way:Fixes #3280.