Skip to content

Conversation

@karajan1001
Copy link
Contributor

@karajan1001 karajan1001 commented Feb 7, 2022

fix: #30
Current pygit2 backend's resolve_rev didn't raise a proper exception when dealing with
HEAD~n like references.

  1. Catch the exception in resolve_rev and reraise it into a proper format.
  2. Add a test for it.

In resolve_refish both HEAD~n like references and non-exist references raise a ValueError, can't distinguish them. And I don't think we should let Inner exception InvalidSpecError be thrown outside of scmrepo. But this method is a relatively slow one, because we need to iter over all of the remotes.

Closes treeverse/dvc#7434

@pmrowla
Copy link
Contributor

pmrowla commented Feb 8, 2022

I'm still confused as to why this PR is needed, since exp show -n ... and pygit resolve_rev(f"HEAD~{n}") worked prior to the recent refactors. It seems like this might need some more investigation into why resolve_rev started failing here (since the exception was not raised at all before)

Is the issue that the exception is raised when n is invalid (and that relative commit does not actually exist)? Or that it is still raised when HEAD~n points to a valid commit (and that the problem is with using ~ at all)?

@karajan1001
Copy link
Contributor Author

I'm still confused as to why this PR is needed, since exp show -n ... and pygit resolve_rev(f"HEAD~{n}") worked prior to the recent refactors. It seems like this might need some more investigation into why resolve_rev started failing here (since the exception was not raised at all before)

Is the issue that the exception is raised when n is invalid (and that relative commit does not actually exist)? Or that it is still raised when HEAD~n points to a valid commit (and that the problem is with using ~ at all)?

It raises when n is invalid, I add two test cases here.

    assert git.resolve_rev("HEAD~1") == init_rev # this is valid , and works pretty fine in previous.

    with pytest.raises(RevError):
        git.resolve_rev("HEAD~3") # in this case, the previous version will raise `InvalidSpecError` instead of `SCMError`

In the previous version of DVC, if we input an invalid number of n, it will raise an unexcepted error.

$ dvc exp show -n 100 --no-pager
ERROR: unexpected error - refs/remotes/origin/HEAD~11: the given reference name 'refs/remotes/origin/HEAD~11' is not valid

Having any troubles? Hit us up at https://dvc.org/support, we are always happy to help!

@pmrowla
Copy link
Contributor

pmrowla commented Feb 8, 2022

@karajan1001 this needs a rebase for the CI fixes

…ces.

fix: treeverse#30
Current pygit2 backend's resolve_rev didn't raise a proper exception when dealing with
`HEAD~n` like reference.

1. Catch the exception in resolve_rev and reraise it into a proper format.
2. Add a test for it.
@pmrowla pmrowla merged commit bd923a7 into treeverse:main Feb 16, 2022
@karajan1001 karajan1001 deleted the fix30 branch February 16, 2022 07:50
karajan1001 added a commit that referenced this pull request Mar 2, 2022
skshetry pushed a commit that referenced this pull request Mar 4, 2022
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.

exp show: throws an error with -n -1 pygit2 backend didn't raise a proper exception in resolve_rev.

2 participants