Skip to content

Do not map (leave as is) trailing / or \ in github URLs. - #7418

Merged
yarikoptic merged 2 commits into
maintfrom
bf-map-url
Jun 9, 2023
Merged

Do not map (leave as is) trailing / or \ in github URLs.#7418
yarikoptic merged 2 commits into
maintfrom
bf-map-url

Conversation

@yarikoptic

Copy link
Copy Markdown
Member

Otherwise we would fail to clone (git would start asking for password since there is no such repo) github repositories while including trailing / which should be perfectly fine and which git handles just fine:

❯ datalad -l debug clone https://github.com/datasets-mila/datasets--robotcar/
[DEBUG  ] Command line args 1st pass for DataLad 0.18.4+21.g560dc910e. Parsed: Namespace() Unparsed: ['clone', 'https://github.com/datasets-mila/datasets--robotcar/']
[DEBUG  ] Building doc for <class 'datalad.core.distributed.clone.Clone'>
[DEBUG  ] Parsing known args among ['/home/yoh/proj/datalad/datalad-maint/venvs/dev3/bin/datalad', '-l', 'debug', 'clone', 'https://github.com/datasets-mila/datasets--robotcar/']
[DEBUG  ] Determined class of decorated function: <class 'datalad.core.distributed.clone.Clone'>
[DEBUG  ] Determined clone target path from source
[DEBUG  ] Resolved clone target path to: '/tmp/datasets--robotcar'
[DEBUG  ] Apply ephemeral patch to clone.py:_pre_annex_init_processing_
[DEBUG  ] Apply ephemeral patch to clone.py:_post_annex_init_processing_
[DEBUG  ] Apply RIA patch to clone.py:_post_git_init_processing_
[DEBUG  ] Apply RIA patch to clone.py:_pre_final_processing_
[DEBUG  ] URL substitution: 'https://github.com/datasets-mila/datasets--robotcar/' -> 'https://github.com/datasets-mila/datasets--robotcar-'
[DEBUG  ] Git clone from https://github.com/datasets-mila/datasets--robotcar- to /tmp/datasets--robotcar
[DEBUG  ] Run ['git', '-c', 'diff.ignoreSubmodules=none', 'clone', '--progress', 'https://github.com/datasets-mila/datasets--robotcar-', '/tmp/datasets--robotcar'] (protocol_class=GitProgress) (cwd=None)
[DEBUG  ] Non-progress stderr: b"Cloning into '/tmp/datasets--robotcar'...\n"
Cloning:  50%|███████████████████████████████████████████████                                               | 1.00/2.00 [00:00<00:00, 534 candidates/s]Username for 'https://github.com':

The fix consists of adding negative lookahead that the final (back)slash is not at the end of the string. I was not sure why (how do we get one) we are to map any backslash so did not introduce any special handling for it, so it just follows the same rule -- the last one would not be mapped.

Otherwise we would fail to clone (git would start asking for password since
there is no such repo) github repositories while including trailing / which
should be perfectly fine and which git handles just fine:

	❯ datalad -l debug clone https://github.com/datasets-mila/datasets--robotcar/
	[DEBUG  ] Command line args 1st pass for DataLad 0.18.4+21.g560dc910e. Parsed: Namespace() Unparsed: ['clone', 'https://github.com/datasets-mila/datasets--robotcar/']
	[DEBUG  ] Building doc for <class 'datalad.core.distributed.clone.Clone'>
	[DEBUG  ] Parsing known args among ['/home/yoh/proj/datalad/datalad-maint/venvs/dev3/bin/datalad', '-l', 'debug', 'clone', 'https://github.com/datasets-mila/datasets--robotcar/']
	[DEBUG  ] Determined class of decorated function: <class 'datalad.core.distributed.clone.Clone'>
	[DEBUG  ] Determined clone target path from source
	[DEBUG  ] Resolved clone target path to: '/tmp/datasets--robotcar'
	[DEBUG  ] Apply ephemeral patch to clone.py:_pre_annex_init_processing_
	[DEBUG  ] Apply ephemeral patch to clone.py:_post_annex_init_processing_
	[DEBUG  ] Apply RIA patch to clone.py:_post_git_init_processing_
	[DEBUG  ] Apply RIA patch to clone.py:_pre_final_processing_
	[DEBUG  ] URL substitution: 'https://github.com/datasets-mila/datasets--robotcar/' -> 'https://github.com/datasets-mila/datasets--robotcar-'
	[DEBUG  ] Git clone from https://github.com/datasets-mila/datasets--robotcar- to /tmp/datasets--robotcar
	[DEBUG  ] Run ['git', '-c', 'diff.ignoreSubmodules=none', 'clone', '--progress', 'https://github.com/datasets-mila/datasets--robotcar-', '/tmp/datasets--robotcar'] (protocol_class=GitProgress) (cwd=None)
	[DEBUG  ] Non-progress stderr: b"Cloning into '/tmp/datasets--robotcar'...\n"
	Cloning:  50%|███████████████████████████████████████████████                                               | 1.00/2.00 [00:00<00:00, 534 candidates/s]Username for 'https://github.com':

The fix consists of adding negative lookahead that the final (back)slash is not
at the end of the string.  I was not sure why (how do we get one) we are to map
any backslash so did not introduce any special handling for it, so it just
follows the same rule -- the last one would not be mapped.
@yarikoptic
yarikoptic requested a review from mih June 7, 2023 15:12
@yarikoptic yarikoptic added semver-patch Increment the patch version when merged CHANGELOG-missing When a PR's description does not contain a changelog item, yet. labels Jun 7, 2023
@github-actions github-actions Bot removed the CHANGELOG-missing When a PR's description does not contain a changelog item, yet. label Jun 7, 2023

@mih mih left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Makes sense. Thx for the detailed commit message, much appreciated!

@yarikoptic

Copy link
Copy Markdown
Member Author

appveyour on mac unrelated afaik

FAILED ../datalad/core/distributed/tests/test_push.py::test_ria_push - datalad.runner.exception.CommandError: CommandError: 'git -c diff.ignoreSubmodules=none annex findref --anything HEAD --json --json-error-messages -c annex.dotfiles=true' failed with exitcode 139 under /Users/appveyor/DLTMP/datalad_temp_test_ria_pushl12zigr9 [info keys: stdout_json] [err: 'error: git-annex died of signal 11']

@yarikoptic
yarikoptic merged commit 8a873f4 into maint Jun 9, 2023
@yarikoptic
yarikoptic deleted the bf-map-url branch June 9, 2023 14:22
@yarikoptic-gitmate

Copy link
Copy Markdown
Collaborator

PR released in 0.18.5

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.

3 participants