scm: fix clone

[dtrifiro]

fetch_all_exp() in clone() was called with url="origin", which resulted in the operation being performed with the remote URL defined in the cloned repo's config, which did not include any credentials initially provided to clone.

Fixes #7670

[dtrifiro]

Seems windows tests are failing. Will look at this in a bit, possibly adding some tests for this scenario

[pmrowla]

fetch should work with git remote names, so I would expect origin to work after clone (origin should be a git remote name pointing to this same URL). Maybe there's an underlying issue with the HTTP token/credentials from the URL string not getting written to the git config for origin when we clone?

I'd be willing to merge this as a temporary workaround, but when we get into actually caching the erepo clones, we will need to make sure that we are properly handing URL string credentials on clone.

[karajan1001]

dvc.scm.InvalidRemoteSCMRepo: 'file://C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\popen-gw0\erepo3' is not a valid Git remote or URL

Looks like under the Windows platform, The prefix file:// can not work properly before C:\

[dtrifiro]

Maybe there's an underlying issue with the HTTP token/credentials from the URL string not getting written to the git config for origin when we clone?

Indeed. The issue is that when the repo is cloned, the token is stripped from the URL saved in .git/config, so when trying to fetch from origin, the token is missing, thus making fetch fail.

I'd be willing to merge this as a temporary workaround, but when we get into actually caching the erepo clones, we will need to make sure that we are properly handing URL string credentials on clone.

Do you have anything in mind? Storing tokens in credential.helper, if available, maybe?

[guyrosin]

Sorry to jump in but it seems Poetry has just dealt with a similar issue a few days ago (python-poetry/poetry#5428), maybe you can use a similar approach?
They provide a fallback to system-provided git client in case of an HTTPUnauthorized error - until Dulwich will start supporting git credentials.

[pmrowla]

Do you have anything in mind? Storing tokens in credential.helper, if available, maybe?

Ideally we would implement support for credential helpers in either dulwich (jelmer/dulwich#873) or libgit2/pygit (libgit2/libgit2#4873)

Sorry to jump in but it seems Poetry has just dealt with a similar issue a few days ago (python-poetry/poetry#5428), maybe you can use a similar approach?

Yes, this would also be an option. If we raise NotImplementedError in the underlying scmrepo dulwich/pygit fetch_refspecs implementations upon getting a credentials related exception, it would force falling back to gitpython/CLI git. (fetch_refpecs is not actually implemented in our gitpython backend right now, but would be simple to add)

But IMO we should be avoiding re-introducing the use of gitpython in DVC if at all possible, and personally I would prefer the workaround from this PR as a temporary fix until we implement credentials support in dulwich over using gitpython as a fallback for fetch_refspecs.

[dtrifiro]

dvc.scm.InvalidRemoteSCMRepo: 'file://C:\Users\runneradmin\AppData\Local\Temp\pytest-of-runneradmin\pytest-0\popen-gw0\erepo3' is not a valid Git remote or URL

Finally had a chance to look at this. It seems that the issue is in the tests: we use a file:// prefix for the local directory so that the local directory is treated as an url and an erepo clone is forced.

This works for Git.clone because scmrepo has a dulwich clone-specific workaround for file urls on windows but, since dulwich does not support windows file:// urls, this fails for fetch_all_exps.

dvc/dvc/scm.py

Lines 126 to 129 in dc1d3e8

	git = Git.clone(url, to_path, progress=pbar.update_git, **kwargs)
	if "shallow_branch" not in kwargs:
	fetch_all_exps(git, "origin", progress=pbar.update_git)
	return git

It seems to me that the cleanest solution is to add support for file:// urls on windows to dulwich, which is not major (patch).

[dtrifiro]

Windows tests should succeed once scmrepo 0.0.23 iterative/scmrepo#72 is released and we bump its version in dvc.

[dtrifiro]

merge after scmrepo==0.0.23 version bump #7831

[pmrowla]

@dtrifiro looks like theres a linter issue?

[dtrifiro]

@dtrifiro looks like theres a linter issue?

Indeed, but it was not related to this PR and was fixed in #7836. Just rebased.

[efiop]

@pmrowla Could you take a look, please?

[pmrowla]

approved w/the note that this is a temporary workaround until #7674 is supported