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

Consistent shell command quoting #690

Merged
merged 3 commits into from
May 17, 2024

Conversation

christian-monch
Copy link
Contributor

@christian-monch christian-monch commented May 17, 2024

Fixes issue #661

This PR automatically quotes all path parameters provided as Path or PurePosixPath-instances to shell.posix-functions via shlex.quote.

Byte-parameter like the command-parameter of ShellCommandExecutor.__call__() are never quoted. The caller has to ensure that file names are properly quoted. The are a number of reasons for this policy:

  1. It is not clear which syntax is used by the shell-program, for example, it could be: bash, PowerShell, Python, etc.
  2. Even if the shell-program was known, e.g. bash, there is no way to generally determine whether a path is contained in the command-argument.

@christian-monch christian-monch force-pushed the consistent_shell_command_quoting branch 9 times, most recently from 3206f05 to c58347c Compare May 17, 2024 13:23
@christian-monch christian-monch marked this pull request as ready for review May 17, 2024 13:23
@christian-monch christian-monch requested a review from mih as a code owner May 17, 2024 13:23
@mih
Copy link
Member

mih commented May 17, 2024

The failure on windows seems to be real.

This commit unifies the quoting of
`PurePosixPath`-instances in
`datalad_next.shell.operations.posix`.
It uses `shlex.quote` to quote stringified
`PurePosixPath`-instances.
@christian-monch christian-monch force-pushed the consistent_shell_command_quoting branch from c58347c to 408fa9b Compare May 17, 2024 14:26
@christian-monch christian-monch force-pushed the consistent_shell_command_quoting branch from 408fa9b to 5d5ee93 Compare May 17, 2024 15:16
Copy link

codecov bot commented May 17, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.42%. Comparing base (37246b1) to head (5d5ee93).

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #690   +/-   ##
=======================================
  Coverage   92.41%   92.42%           
=======================================
  Files         193      193           
  Lines       14108    14121   +13     
  Branches     2126     2128    +2     
=======================================
+ Hits        13038    13051   +13     
  Misses        808      808           
  Partials      262      262           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@christian-monch
Copy link
Contributor Author

The failure on windows seems to be real.

Yes, I overestimated the capabilities of FAT and NTFS. Everything should be fine now

Copy link
Member

@mih mih left a comment

Choose a reason for hiding this comment

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

Thanks!

@mih mih merged commit 3691e63 into datalad:main May 17, 2024
8 checks passed
@mih mih added this to the 1.4 milestone May 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants