catharsis / spotifile

FUSE file system for Spotify
BSD 3-Clause "New" or "Revised" License
149 stars 7 forks source link

Correctly escape ampersands in xspf files #47

Closed gsora closed 8 years ago

gsora commented 8 years ago

As the title says :)

My C is not really good nowadays so feel free to make any correction or simply reject this PR.

This PR correctly escapes "&" with "&": I've tested with my "Discover Weekly" which this week contains a track with an "&", without this patch, the playlist file is considered as malformed and not all the tracks are being recognized by the player's parser.

Also my english is not that good, so please forgive me for any error :)

catharsis commented 8 years ago

Good catch, thanks for the PR!

Overall, the approach seems valid - a few things:

Of course, if you don't feel comfortable or don't have the time to do either or any of these, just say so and I'll make the changes myself. Alternatively, if I can help you out with some guidance, I'd be happy to do so - just let me know!

gsora commented 8 years ago

I'll look into this as soon as I get home :)

Regarding the Travis build failing, on my system nothing breaks even if cloning the repo from scratch

I'll include sys/types.h for completeness, though.

On 12 Mar 2016 11:09 +0100, Anton Löfgrennotifications@github.com, wrote:

Good catch, thanks for the PR!

Overall, the approach seems valid - a few things:

The build fails:https://travis-ci.org/catharsis/spotifile/builds/115293115- presumably because you forgot to#include<sys/types>in string_utils.c. Would you care to also escape",',and perhaps do so in a generalxspf_sanitizefunction? (I think that's all of the special characters) For extra points, also write a test case (tests/xspf_test.c) that exercises the sanitation code with some special characters. Your english is excellent, don't be silly ;)

Of course, if you don't feel comfortable or don't have the time to do either or any of these, just say so and I'll make the changes myself. Alternatively, if I can help you out with some guidance, I'd be happy to do so - just let me know!

— Reply to this email directly orview it on GitHub(https://github.com/catharsis/spotifile/pull/47#issuecomment-195709348).

gsora commented 8 years ago

Travis is now happy :+1:

Regarding the test case, I'd like to write one but I've never used any C testing framework.

catharsis commented 8 years ago

Great job! We use the "Check" framework to write unit tests (http://libcheck.github.io/check/). It's got some boilerplate, but if you just add to an existing suite (tests/xspf_test.c, for example), I doubt you'll have any problems. Have a look at the tests in tests/ for some example tests.

gsora commented 8 years ago

I've added a little test case for the sanitization process, sorry for the delay but I've been buried in my work :+1:

catharsis commented 8 years ago

Awesome, thank you!