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

Comply to Posix-like clone URL formats on Windows #7181

Merged
merged 7 commits into from
Nov 22, 2022

Conversation

adswa
Copy link
Member

@adswa adswa commented Nov 16, 2022

Fixes #7180.

Git clone's url format does not include backslashes. Therefore, this commits ensures that paths provided to clone on Windows are compliying to posix. When platform specific windows paths were passed on by clone as git urls, cloning succeeded, but resulting remote urls can became disfunctional. Specifically, in the case of cloning a subdataset from a local relative path, git will append the relative Windows path to an otherwise Posix-confirming absolute path in the remote url in the subdatasets .git/config. (see #7180). This patch should fix this, and add a test.

Git clone's url format does not include backslashes (see man git clone).
Therefore, this commits ensures that paths provided to clone on
Windows are compliying to posix.
When platform specific windows paths were passed on by clone as git urls,
cloning succeeded, but resulting remote urls can became disfunctional.
Specifically, in the case of cloning a subdataset from a local
relative path, git will append the relative Windows path to
an otherwise Posix-confirming absolute path in the remote url
in the subdatasets .git/config:

'C:/Users/adina/AppData/Local/Temp/datalad_temp_frvczceh/ds/..\\origin'

Fixes datalad#7180
@adswa adswa added semver-patch Increment the patch version when merged CHANGELOG-missing When a PR's description does not contain a changelog item, yet. labels Nov 16, 2022
@github-actions github-actions bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Nov 16, 2022
@codecov
Copy link

codecov bot commented Nov 17, 2022

Codecov Report

Base: 88.28% // Head: 90.92% // Increases project coverage by +2.63% 🎉

Coverage data is based on head (305a256) compared to base (19404cb).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##            maint    #7181      +/-   ##
==========================================
+ Coverage   88.28%   90.92%   +2.63%     
==========================================
  Files         355      355              
  Lines       46524    46532       +8     
  Branches     6331     6332       +1     
==========================================
+ Hits        41076    42309    +1233     
+ Misses       5433     4208    -1225     
  Partials       15       15              
Impacted Files Coverage Δ
datalad/tests/test_utils_cached_dataset.py 100.00% <ø> (ø)
datalad/core/distributed/tests/test_clone.py 97.61% <100.00%> (+0.37%) ⬆️
datalad/support/gitrepo.py 92.75% <100.00%> (+0.17%) ⬆️
datalad/distribution/tests/test_install.py 99.81% <0.00%> (+0.18%) ⬆️
datalad/support/tests/test_annexrepo.py 97.97% <0.00%> (+0.20%) ⬆️
datalad/distribution/create_sibling.py 69.97% <0.00%> (+0.28%) ⬆️
datalad/distribution/tests/test_dataset.py 99.70% <0.00%> (+0.29%) ⬆️
datalad/support/network.py 90.17% <0.00%> (+0.43%) ⬆️
... and 47 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

adswa added a commit to adswa/datalad that referenced this pull request Nov 17, 2022
This change reincarnates parts of a changeset originally proposed in
never saw (CI logs weren't preserved), but that I suspect to originate
in datalad#7180 (Git stitching together posix and windows paths into a
non-functional URL). Thus, this change sits on top of datalad#7181, which fixes
these URLs to be fully posix.
Based on those fixes, this change wrangles absolute URLs originating
from relative paths back into relative paths, to keep them functional.
This change also enables an old test for this problem, marked as a known
failure.

Fixes datalad#3538
adswa added a commit to adswa/datalad that referenced this pull request Nov 17, 2022
This change reincarnates parts of a changeset originally proposed in
never saw (CI logs weren't preserved), but that I suspect to originate
in datalad#7180 (Git stitching together posix and windows paths into a
non-functional URL). Thus, this change sits on top of datalad#7181, which fixes
these URLs to be fully posix.
Based on those fixes, this change wrangles absolute URLs originating
from relative paths back into relative paths, to keep them functional.
This change also enables an old test for this problem, marked as a known
failure.

Fixes datalad#3538
adswa added a commit to adswa/datalad that referenced this pull request Nov 17, 2022
This change reincarnates parts of a changeset originally proposed in
never saw (CI logs weren't preserved), but that I suspect to originate
in datalad#7180 (Git stitching together posix and windows paths into a
non-functional URL). Thus, this change sits on top of datalad#7181, which fixes
these URLs to be fully posix.
Based on those fixes, this change wrangles absolute URLs originating
from relative paths back into relative paths, to keep them functional.
This change also enables an old test for this problem, marked as a known
failure.

Fixes datalad#3538
adswa added a commit to adswa/datalad that referenced this pull request Nov 17, 2022
This change reincarnates parts of a changeset originally proposed in
never saw (CI logs weren't preserved), but that I suspect to originate
in datalad#7180 (Git stitching together posix and windows paths into a
non-functional URL). Thus, this change sits on top of datalad#7181, which fixes
these URLs to be fully posix.
Based on those fixes, this change wrangles absolute URLs originating
from relative paths back into relative paths, to keep them functional.
This change also enables an old test for this problem, marked as a known
failure.

Fixes datalad#3538
adswa added a commit to adswa/datalad that referenced this pull request Nov 17, 2022
This change reincarnates parts of a changeset originally proposed in
never saw (CI logs weren't preserved), but that I suspect to originate
in datalad#7180 (Git stitching together posix and windows paths into a
non-functional URL). Thus, this change sits on top of datalad#7181, which fixes
these URLs to be fully posix.
Based on those fixes, this change wrangles absolute URLs originating
from relative paths back into relative paths, to keep them functional.
This change also enables an old test for this problem, marked as a known
failure.

Fixes datalad#3538
adswa added a commit to adswa/datalad that referenced this pull request Nov 17, 2022
This change reincarnates parts of a changeset originally proposed in
datalad#4026, that didn't make it in due
to a failing Windows test that Ivnever saw (CI logs weren't preserved),
but that I suspect to originate
in datalad#7180 (Git stitching together posix and windows paths into a
non-functional URL). Thus, this change sits on top of datalad#7181, which fixes
these URLs to be fully posix.
Based on those fixes, this change wrangles absolute URLs originating
from relative paths back into relative paths, to keep them functional.
This change also enables an old test for this problem, marked as a known
failure.

Fixes datalad#3538
Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

The as_posix approach is certainly right. I suppose the change in gitrepo.clone could be in the RI class(es) instead, but I'm fine with it at as. Thank you, @adswa!

Just one minor thing to adjust please (see below).

datalad/core/distributed/tests/test_clone.py Outdated Show resolved Hide resolved
@adswa adswa force-pushed the bf-7180 branch 2 times, most recently from 570748b to 305a256 Compare November 21, 2022 10:39
Copy link
Member

@bpoldrack bpoldrack left a comment

Choose a reason for hiding this comment

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

Thank you, @adswa. Reran failing Travis build - all good, let's proceed.

@bpoldrack bpoldrack merged commit 653438e into datalad:maint Nov 22, 2022
@adswa adswa deleted the bf-7180 branch November 22, 2022 09:16
@yarikoptic-gitmate
Copy link
Collaborator

PR released in 0.17.10

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Increment the patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Erroneous git url construction with relative Windows paths for subdatasets
3 participants