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

BF: Implement workaround for file:// URL compatibility on Windows #3979

Merged
merged 3 commits into from Jan 3, 2020

Conversation

mih
Copy link
Member

@mih mih commented Jan 2, 2020

The new 'compatibility' argument of get_local_file_url() is relatively
painless and the workaround implementation can be removed (and the
argument rendered into a no-op) if there should be a compatibility
fix at the git-annex end.

Fixes gh-3639

The new 'compatibility' argument of get_local_file_url() is relatively
painless and the workaround implementation can be removed (and the
argument rendered into a no-op) if there should be a compatibility
fix at the git-annex end.

Fixes dataladgh-3639
@mih
Copy link
Member Author

mih commented Jan 2, 2020

Ha, so even if we generate proper Git-compatible file:// URLs on Windows this doesn't work, because GitRepo.clone() throws them away and takes a wrong localpath. Here is the relevant code:

       # try to get a local path from `url`:
        try:
            url = url_ri.localpath
            url_ri = RI(url)
        except ValueError:
            pass

It is unclear to me why a file:// URL cannot be passed as-is (but see below for a clue). In any case, with this code gone, things start working.

Tracing the evolution of a URL through the code is a bit funny. There is also this step in clone.py:

        # Possibly do conversion from source into a git-friendly url
        # luckily GitRepo will undo any fancy file:/// url to make use of Git's
        # optimization for local clones....
        source_url = source
        source_ = _get_git_url_from_source(source)

which converts from str to RI to str, which is later converted back into RI.

I will disable this optimization step on windows to get basic functionality with file:// URLs.

It falsely turns a file:// URL into an invalid POSIX path on Windows.
@codecov
Copy link

codecov bot commented Jan 2, 2020

Codecov Report

Merging #3979 into master will decrease coverage by 0.18%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3979      +/-   ##
==========================================
- Coverage   89.67%   89.49%   -0.19%     
==========================================
  Files         271      271              
  Lines       36351    36339      -12     
==========================================
- Hits        32599    32521      -78     
- Misses       3752     3818      +66
Impacted Files Coverage Δ
datalad/distribution/clone.py 90.41% <ø> (ø) ⬆️
datalad/tests/utils_testrepos.py 92.92% <100%> (-0.89%) ⬇️
datalad/support/gitrepo.py 89.15% <100%> (-0.31%) ⬇️
datalad/support/network.py 86.63% <100%> (ø) ⬆️
datalad/support/tests/test_gitrepo.py 99.78% <100%> (ø) ⬆️
datalad/support/tests/test_network.py 100% <100%> (ø) ⬆️
datalad/customremotes/base.py 69.25% <0%> (-14.89%) ⬇️
datalad/customremotes/tests/__init__.py 91.66% <0%> (-8.34%) ⬇️
datalad/support/keyring_.py 84.44% <0%> (-6.67%) ⬇️
datalad/tests/test_base.py 96.55% <0%> (-3.45%) ⬇️
... and 9 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d4f392f...6424750. Read the comment docs.

@mih mih added the merge-if-ok OP considers this work done, and requests review/merge label Jan 2, 2020
@mih mih added this to Major issues in Windows Jan 2, 2020
Copy link
Contributor

@kyleam kyleam left a comment

Choose a reason for hiding this comment

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

The new 'compatibility' argument of get_local_file_url() is relatively
painless and the workaround implementation can be removed (and the
argument rendered into a no-op) if there should be a compatibility
fix at the git-annex end.

Yes, painless, as well as non-intrusive for non-Windows platforms, so it sounds fine to me.

@mih
Copy link
Member Author

mih commented Jan 3, 2020

Thx @kyleam

@mih mih merged commit 5f15201 into datalad:master Jan 3, 2020
@mih mih deleted the bf-3977 branch January 3, 2020 07:14
@mih mih moved this from Major issues to Done in Windows Mar 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge-if-ok OP considers this work done, and requests review/merge
Projects
No open projects
Windows
  
Done
Development

Successfully merging this pull request may close these issues.

Optimized gitrepo cloning broken on win10
2 participants