exp show: throws an error with -n -1

[karajan1001]

fix: #7434
exp show -n -1 will throw error on any repo with a remote repo setted.

1. Update new version of scmrepo
2. Update the unit test add a remote repo to the test case.

• ❗ 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.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

[karajan1001]

Looks like scmrepo is not compatible with current version of DVC. After PR

commit 29d834163ca6c5aa60be6f0f68785e3c2f6f2f87 (HEAD)
Author: Ruslan Kuprieiev <kupruser@gmail.com>
Date:   Tue Dec 28 02:49:14 2021 +0200

    fs: detach from local filesystem (#24)

    This PR makes our fs accept paths relative to the root of the repo and using / as a separator.
    Previous behaviour was a legacy from old dvc.

    Pre-requisite for making dvcfs/repofs/etc switch to the same relative repo paths and / separators.

Guess it arises from the relative path and abs path change in recent DVC project.

[skshetry]

See #7359 (comment).

[skshetry]

@karajan1001, I don't understand the change here. Do we need this? Tests pass fine on master with scmrepo==0.0.10.

[karajan1001]

@karajan1001, I don't understand the change here. Do we need this? Tests pass fine on master with scmrepo==0.0.10.

I used to update scmrepo to 0.0.9 and change the test environment in it. But for now the scmrepo had already been updated to a newer version. The remained line of test environment changing will ensure #7434 would be passed. I'm not sure If it is still been needed.

[karajan1001]

@dberenbaum so I guess for now, after updating the scmrepo to 0.0.10 the original #7434 had already been solved, do we still this test or maybe we could close #7434 directly.

[pmrowla]

If the underlying issue was in scmrepo, #7434 should just be closed with a link to the scmrepo fix/PR (and the appropriate test should have been added in scmrepo)

[skshetry]

The fix in treeverse/scmrepo#32 was about supporting ~n references from the title and is tested for that. The description and the change here points to some issue with git repos having remotes set, which is not tested.

[karajan1001]

The fix in iterative/scmrepo#32 was about supporting ~n references from the title and is tested for that. The description and the change here points to some issue with git repos having remotes set, which is not tested.

The test here influence the line below

gen = iter_revs(scm, ["other"], -1)

which will test 1,2,3,...n util receive a ScmError. And going back to #7204 (comment),
This problem only arises in a repo with a remote repo being set.

[pmrowla]

Ok, so the remote being set causes us to manually try refs/remotes/<name> on the DVC side. This would lead to us trying to resolve relative commits that may not exist in refs/remotes (like refs/remotes/origin/HEAD~6).

The actual underlying problem was that trying to resolve relative commits that did not exist would lead to the unhandled pygit exception in scmrepo. This is not actually related to whether or not remotes are set at all, the same behavior would occur if we tried a non-remote ref like refs/HEAD~999999 where that relative commit does not exist. It's just coincidental that the bug was identified by setting a remote.

So we probably do not actually need this test/change on the DVC side, since we now have an actual test for resolving non-existent relative commits on the scmrepo side.

https://github.com/iterative/scmrepo/blob/330bce52c922009e608c6e3dcc16f23e5f09ff18/tests/test_git.py#L582-L583

[pmrowla]

The test here influence the line below

gen = iter_revs(scm, ["other"], -1)

For future reference, when we see cases like this, tests should be for the actual bug behavior. So if there is a case where some higher level function call in DVC leads to a bug in iter_revs(), we would want a test for that iter_revs call, and not the higher level API call. (Or in this particular case, we would want to test in the underlying scmrepo call, since that's where the actual bug occurs)

Adding a test for the higher level API call generally just leads to test code/behavior that is not obvious at all (like randomly setting a git remote). And it's basically impossible for someone else to look at the test function and understand what we are even trying to test in the first place