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

Protect filenames #1426

Closed
wants to merge 3 commits into from

Conversation

rpspringuel
Copy link
Contributor

I've created a function for protecting file names (substituting - for a bunch of invalid characters) and used it to protect snippet related files. This prevents projects which have these characters in the main project file's name from not finding the executable (#1416).

I've created a function for protecting file names (substituting `-` for a bunch of invalid characters) and used it to protect snippet related files.  This prevents projects which have these characters in the main project file's name from not finding the executable (gregorio-project#1416).
@rpspringuel
Copy link
Contributor Author

Just realized that I haven't created a test for this issue. I'll work on that later today.

@rpspringuel
Copy link
Contributor Author

Problem with creating a possible test. Adding a single quote to a file name raises the following error from the testing script: gxargs: unmatched single quote; by default quotes are special to xargs unless you use the -0 option

@henryso do you have any suggestions for how we might deal with this in the script?

@rpspringuel
Copy link
Contributor Author

I've been thinking about this and since the problem is in trying to mix this unusual filename with xargs perhaps we should run this test without the benefit of xargs? Perhaps as an optional "extra" at the end?

@henryso
Copy link
Contributor

henryso commented Aug 2, 2018

Perhaps the harness can be rewritten to use the null separator so that -0 can work. I haven't thought about how difficult that would be to do, though.

@henryso
Copy link
Contributor

henryso commented Aug 11, 2018

I think I've figured out a way to make this work. However, I think it will require a change to the viewer settings in gregorio-test.rc. Also, it'll take a bit more doing before I'm done.

@henryso
Copy link
Contributor

henryso commented Aug 12, 2018

I have something that works, but I'd like to wait for #1429 to be merged so that it's easier to make the changes I need to make to harness.sh.

@henryso
Copy link
Contributor

henryso commented Aug 12, 2018

On second thought, looks like I'd need to get this against the ctan branch. I'll try to rework it tomorrow.

@henryso
Copy link
Contributor

henryso commented Aug 12, 2018

I slept on it and decided I could just bring the changes in wholesale. @rpspringuel Please try your tests with this code, and merge gregorio-project/gregorio-test#328 if it is satisfactory or provide feedback for me to fix.

rpspringuel added a commit to rpspringuel/gregorio-test that referenced this pull request Mar 30, 2019
rpspringuel added a commit to rpspringuel/gregorio that referenced this pull request Mar 30, 2019
This fix for problematic filenames seems to work and is simpler than what I was working on in gregorio-project#1426.
It is compatible with gregorio-project/gregorio-test#328 after the changes I proposed on henryso/gregorio-test#1 are pulled in.
The corresponding PR which adds the test for this problem is gregorio-project/gregorio-test#331
@henryso
Copy link
Contributor

henryso commented Mar 31, 2019

This PR is getting a bit difficult for me to follow, but I think as long as the tests work (which they seem to do) and this change works on Windows as well, then we're good to go ahead with it.

@rpspringuel
Copy link
Contributor Author

This PR is the one that would be closed without merging. #1450 is I think a simpler and therefore better fix.

Sent with GitHawk

@henryso
Copy link
Contributor

henryso commented Mar 31, 2019

OK, I'll put my comment there.

@henryso henryso closed this Mar 31, 2019
@rpspringuel rpspringuel deleted the filename_quote branch April 6, 2019 20:18
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.

2 participants