Skip to content

git: quote filenames at remind_to_track #3315

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.

Sign up for GitHub

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

Merged
efiop merged 1 commit into from Mar 23, 2020
+10 −1
Merged

git: quote filenames at remind_to_track #3315

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion dvc/scm/git/__init__.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import logging
import os
import yaml
import shlex

from funcy import cached_property
from pathspec.patterns import GitWildMatchPattern
Expand Down Expand Up @@ -321,11 +322,13 @@ def remind_to_track(self):
if not self.files_to_track:
return

files = " ".join(shlex.quote(path) for path in self.files_to_track)

logger.info(
"\n"
"To track the changes with git, run:\n"
"\n"
"\tgit add {files}".format(files=" ".join(self.files_to_track))
"\tgit add {files}".format(files=files)
)

def track_file(self, path):
Expand Down
6 changes: 6 additions & 0 deletions tests/unit/scm/test_scm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,3 +37,9 @@ def test_should_throw_and_cleanup(self):

self.assertEqual(0, self.scm_mock.reset_ignores.call_count)
self.assertEqual(0, self.scm_mock.remind_to_track.call_count)


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
Copy link
Copy Markdown
Contributor

@efiop efiop Feb 13, 2020

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.

Copy link
Copy Markdown
Contributor

@casperdcl casperdcl Feb 22, 2020

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

Copy link
Copy Markdown
Contributor

@casperdcl casperdcl Feb 22, 2020

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

Copy link
Copy Markdown
Contributor

@efiop efiop Feb 22, 2020

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.

Copy link
Copy Markdown
Contributor

@casperdcl casperdcl Feb 22, 2020

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