Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

dvc: Repo(..., rev=smth) is counter-intuitive or broken #2201

Closed
2 tasks
Suor opened this issue Jun 27, 2019 · 6 comments
Closed
2 tasks

dvc: Repo(..., rev=smth) is counter-intuitive or broken #2201

Suor opened this issue Jun 27, 2019 · 6 comments
Assignees
Labels
bug Did we break something? p2-medium Medium priority, should be done, but less important

Comments

@Suor
Copy link
Contributor

Suor commented Jun 27, 2019

For example repo.add() will browse using currently existing .dvcignore files. repo.pull/push() will work against master not rev.

Should we hide it? Or handle all of that? Or implement everything not using non-default rev Repo objects?

  • either fix or drop rev
  • remove assert rev is None in dvc.api
@Suor
Copy link
Contributor Author

Suor commented Jun 27, 2019

P.S. the examples above are not exhaustive, e.g. Config also ignores rev and who knows what else.

@efiop efiop added bug Did we break something? p0-critical Critical issue. Needs to be fixed ASAP. labels Jun 27, 2019
@efiop
Copy link
Contributor

efiop commented Jun 27, 2019

@Suor Are you sure? pull/push will use self.tree(which is set to rev), so they should do the right thing. Config will indeed not work. Either way, need to check it.

@Suor
Copy link
Contributor Author

Suor commented Jun 28, 2019

@efiop .push()/pull() use brancher and brancher only uses self.tree, if no kwargs are passed. So it was not as I described, but still fishy.

Still the point stands - do we really need repos with non-default rev?

@efiop
Copy link
Contributor

efiop commented Jun 28, 2019

We do for erepos, to be able to collect and pull stuff. But there might be a nicer way to do that.

@efiop efiop added p2-medium Medium priority, should be done, but less important and removed p0-critical Critical issue. Needs to be fixed ASAP. labels Jun 28, 2019
@Suor
Copy link
Contributor Author

Suor commented Jun 28, 2019

I would say that we may break someone workflow if make Repo reread config for each branch :) Some older branch may have no remote config. And when it will have a different one it could be quite surprising for a user that his files are pushed to two different remotes.

@efiop
Copy link
Contributor

efiop commented Jun 28, 2019

I would say that we may break someone workflow if make Repo reread config for each branch :) Some older branch may have no remote config. And when it will have a different one it could be quite surprising for a user that his files are pushed to two different remotes.

Yep, need to give that one a thought. On one hand, it seems like it would be a correct approach none the less, but on the other maybe the current behavior is actually what we want (at least with some explicit option). The least we could do is to check if other branches have different remotes to the one in the current workspace and prompt or raise a warning/error.

For the record: Agreed in a private discussion to remove rev from Repo.__init__ and use self.tree with brancher in a more explicit way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Did we break something? p2-medium Medium priority, should be done, but less important
Projects
None yet
Development

No branches or pull requests

2 participants