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

mih added 2 commits 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 dataladgh-3639
@mih
Copy link
Member Author

@mih 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 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 label Jan 2, 2020
@mih mih added this to Major issues in Windows Jan 2, 2020
kyleam
kyleam approved these changes Jan 2, 2020
Copy link
Contributor

@kyleam kyleam left a comment

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 mih commented Jan 3, 2020

Thx @kyleam

@mih mih merged commit 5f15201 into datalad:master Jan 3, 2020
17 checks passed
@mih mih deleted the bf-3977 branch Jan 3, 2020
@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
Projects
No open projects
Windows
  
Done
Development

Successfully merging this pull request may close these issues.

2 participants