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

scm: implement pygit2 resolve rev and native checkout #5270

Merged
merged 12 commits into from
Jan 18, 2021

Conversation

pmrowla
Copy link
Contributor

@pmrowla pmrowla commented Jan 14, 2021

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

Implements required functionality for workspace git checkout and git checkout -b (get_ref/set_ref/iter_refs, resolve_rev, reset) in pygit2 backend.
Related to #2215

@pmrowla pmrowla self-assigned this Jan 14, 2021
@pmrowla pmrowla changed the title scm: implement pygit2 resolve rev and native checkout [WIP] scm: implement pygit2 resolve rev and native checkout Jan 14, 2021
@pmrowla
Copy link
Contributor Author

pmrowla commented Jan 14, 2021

Investigating why checkpoint resume breaks with the pygit2 checkout, everything else in DVC appears to work properly with it.

@pmrowla pmrowla changed the title [WIP] scm: implement pygit2 resolve rev and native checkout scm: implement pygit2 resolve rev and native checkout Jan 15, 2021
Comment on lines +103 to +104
if branch == "-":
branch = "@{-1}"
Copy link
Contributor Author

@pmrowla pmrowla Jan 15, 2021

Choose a reason for hiding this comment

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

For whatever reason, libgit2's resolve_refish doesn't handle - shorthand, probably because it's specific to git checkout/switch and not a general git ref shorthand symbol

@pmrowla pmrowla force-pushed the pygit2-resolve-rev branch from d83f61f to 9affacc Compare January 15, 2021 08:12
@@ -50,7 +50,7 @@ def test_post_checkout(self, tmp_dir, scm, dvc):
os.unlink("file")
scm.install()

scm.checkout("new_branch", create_new=True)
scm.gitpython.git.checkout("-b", "new_branch")
Copy link
Contributor Author

@pmrowla pmrowla Jan 15, 2021

Choose a reason for hiding this comment

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

libgit2 does not run git hooks on any command/function call, and internally we don't need to run this hook.

For this test we want to make sure that the installed DVC post-checkout hook is run when a user does command-line git checkout, not that it is run on DVC scm.checkout.

@pmrowla
Copy link
Contributor Author

pmrowla commented Jan 15, 2021

experiments/test_gc still failing because something in the changes is causing carriage returns to get committed on windows even with core.autocrlf is set to true.

@pmrowla pmrowla force-pushed the pygit2-resolve-rev branch from 143165d to 2b7d94d Compare January 18, 2021 07:34
Comment on lines +380 to +381
else:
return f"refs/heads/{self.repo.active_branch}"
Copy link
Contributor

@efiop efiop Jan 18, 2021

Choose a reason for hiding this comment

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

A bit offtopic and not important in such a small else statement, but what do you guys think about enabling no-else-after-return check in our linters? I seem to stumble upon it regularly.

Suggested change
else:
return f"refs/heads/{self.repo.active_branch}"
return f"refs/heads/{self.repo.active_branch}"

EDIT: add to our retro to discuss πŸ™‚

Copy link
Contributor

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Great stuff πŸš€

@efiop efiop merged commit 407f560 into iterative:master Jan 18, 2021
@pmrowla pmrowla deleted the pygit2-resolve-rev branch January 19, 2021 01:55
efiop added a commit to efiop/dvc that referenced this pull request Jan 19, 2021
@efiop efiop mentioned this pull request Jan 19, 2021
2 tasks
efiop added a commit that referenced this pull request Jan 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants