dalehenrich / filetree

Monticello repository for directory-based Monticello packages enabling the use of git, svn, etc. for managing Smalltalk source code.
https://github.com/CampSmalltalk/Cypress
MIT License
133 stars 26 forks source link

MonticelloFileTree-Git doesn't handle package-names that contain a space #139

Closed eMBee closed 9 years ago

eMBee commented 9 years ago

apparently package-names may contain spaces. i just filed-in such a package and when saving to git, it simply got ignored because 'git commit -a' didn't find anything to match. then i "fixed" the problem by committing that package manually, and got an error trying to see changes.

the solution is to quote the packageDirectoryString in MCFileTreeGitRepository>>basicStoreVersion: and the packageDirectory in MCFileTreeGitStReader>>zip

i searched through all uses of PipeableOSProcess and only found those two places.

i have a fix in my image and i am working on the pull-request

ThierryGoubier commented 9 years ago

Hi @eMBee, thanks for catching that.

dalehenrich commented 9 years ago

fixed with PR #140

eMBee commented 9 years ago

btw: i suppose this needs to be backported to all branches, even squeak, because the package that lead to discover this is old squeak code..

dalehenrich commented 9 years ago

reopening for @ThierryGoubier comment ... AFAIK this is only a problem in the git filetree code? At least the patch only affected the git filetree code ...

ThierryGoubier commented 9 years ago

Yes, this is only an issue in the git filetree code, and I have never tried to use it (or test it) in Squeak (is it active in one of the squeak branches?)

eMBee commented 9 years ago

Excerpts from Dale Henrichs's message of 2015-02-02 01:30:09 +0100:

reopening for @ThierryGoubier comment ... AFAIK this is only a problem in the git filetree code? At least the patch only affected the git filetree code ...

since it is a quoting issue, the problem should be limited to places where shell commands are used. if filetree itself doesn't use shell commands, it should be fine.

i only searched for uses of PipeableOSProcess though. if there are other ways to call the shell, i didn't see them.

during the commit, the package in question got written to disk properly, it just didn't get added to git, so if filetree and git filetree use the same code to write to the disk, it should be fine.

loading would have to be looked at separately because here the git code works differently from regular filetree.

ThierryGoubier commented 9 years ago

@eMBee , for reading, the code in git filetree is modified to read from a zip (which is extracted with git archive). Since you have found and corrected the bug there, it should be fine.

To make things safer, I'll have a look into adding a test case for that.

dalehenrich commented 9 years ago

@ThierryGoubier once you've added a test case, I'll adopt it for testing the standard FileTree code ... I agree with @eMBee that is probably isn't a problem ... no os calls are harmed by the standard FileTree code, but I cannot be sure that the package names are handled correctly without a test case:)

ThierryGoubier commented 9 years ago

Done! There is now a test case in repository 'issue 139', with a package named 'Issue 139', at commit 5a5b1e46292cfcced5edb67af18eb0b37d6a65da

dalehenrich commented 9 years ago

Excellent! Thanks!