-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
git: quote filenames at remind_to_track #3315
Conversation
def test_remind_to_track(scm, caplog): | ||
scm.files_to_track = ["fname with spaces.txt", "тест", "foo"] | ||
scm.remind_to_track() | ||
assert "git add 'fname with spaces.txt' 'тест' foo" in caplog.text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember windows cmdline being weird about single-quotes and having to use double-quotes. Also IIRC windows cmd always does completion with double-quotes. Searching around (e.g. https://ss64.com/nt/syntax-esc.html) I also see double quotes used and not single-quotes. Let's switch to double-quotes too, it will be compatible with both win and *nix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well *nix will shell-expand things in double quotes:
$ export foo=bar
$ echo spam > \$foo
$ cat "$foo" # double quotes :(
cat: bar: No such file or directory
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also @efiop this PR uses shlex.quote
which avoids this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@casperdcl Yep, there is also mslex
which properly quotes things for windows, but it doesn't have a conda package. I'm wondering if it is easier to just make dvc automatically git add
things at this point. Easier for both us on implementation and for users in terms of UX.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes not sure if I've mentioned this somewhere before but simply adding --git
to any dvc
command to auto-exec the obvious would be nice
Merging as is, we'll see about mslex in the future, for now this will cover 90% (or more) of use cases. |
Fix #3303