Skip to content

Conversation

@pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Jun 9, 2020

  • ❗ I have followed the Contributing to DVC checklist.

  • πŸ“– If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here. If the CLI API is changed, I have updated tab completion scripts.

  • ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Related to #3882

  • Local cache/remote always uses its own working tree
    • local cache no longer affected by whether repo.tree is working or git tree
  • State is tied to the local cache working tree rather than repo.tree
  • CleanTree now ignores paths outside of the DVC tree/repo root
    • previously it was used interchangeably with working tree, and was expected to return true for paths outside of the DVC repo root in cases where local cache directory was moved or on a different logical drive
    • local cache working tree should now be used for handling all cache paths, not the DVC repo cleantree instance

@pmrowla pmrowla self-assigned this Jun 9, 2020
@efiop efiop mentioned this pull request Jun 10, 2020
3 tasks
dvc/state.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After remote/cache/tree separation, the only default tree (aka cache.tree) that local cache is working on will be the work tree, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's correct

pmrowla added 4 commits June 12, 2020 16:12
* repo tree (CleanTree) instance should not be relevant to local cache
  operations. When the dvcignore CleanTree is relevant, repo.tree should
  be used. For local remote/cache operations, the local remote work_tree
  should be used.
@pmrowla pmrowla force-pushed the local-remote-tree branch from f58bf4a to 9c59e62 Compare June 12, 2020 08:02
@pmrowla pmrowla marked this pull request as ready for review June 12, 2020 08:57
@pmrowla pmrowla changed the title [WIP] remote: use separate working tree instance for local cache remote: use separate working tree instance for local cache Jun 12, 2020
@pmrowla pmrowla requested review from efiop, pared and skshetry June 12, 2020 08:58
Copy link
Collaborator

@skshetry skshetry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment on lines +92 to +95
repo = local_cache.repo
self.repo = repo
self.root_dir = repo.root_dir
self.tree = WorkingTree(self.root_dir)
self.tree = local_cache.tree.work_tree
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pmrowla State itself has nothing to do with the cache, but rather with the repos working tree, right? So maybe we should have work_tree(or wtree) in repo itself and make local cache and state use it? Just not quite obvious why state depends on local cache here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed to me that since state is only applicable whenever there is a local cache, and used as a cache for calculating md5s which will go into the local cache, it would make more sense to have state associated with the local cache's working tree rather than with a repo tree instance. The repo doesn't actually need a working tree to function (since we can use git trees), but local cache and state only use a working tree.

@efiop efiop merged commit 652f5ab into iterative:master Jun 12, 2020
@pmrowla pmrowla deleted the local-remote-tree branch June 12, 2020 09:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants