Fixed issue #281 - added backslashing of special characters

[tradzik]

Added code, which is backslashing special characters. Tested on very various collection of names...

My mentor: @eMBee ping

[eMBee]

i am unfamiliar with this issue. can anyone else take a look at this please?

[terotil]

@ty221 You are fixing the true cause of a problem reported in #281, as I see it. Escaping of fn is not done properly.

Is escaping spaces and quotes really necessary at all? Dir[fn] treats fn as File::fnmatch pattern, so I'd expect you'd need to escape \*?[ (backslash, asterisk, question mark and opening bracket). Also this fix will break check-attachment hooks, which will (and IMO should) expect non-escaped filename. Escape only the name you provide Dir[] with.

It would be very, very well appreciated if you first wrote a failing test that would pinpoint the cause of the bug reported in #281 and then greened it with the fix. If you don't, please provide us with your core test cases (filenames that failed before fixing and work after fixing).

[tradzik]

Hi,

I suggest you to check these 13 names:

abcd \24
\file " this is file "
anna\john "Word"\document \sss
\edited
file \ january "report\today" file123\43 [I don't like backslashes]

Sun Jan 04 2015 at 09:51:52 użytkownik Tero Tilus [email protected] napisał:

@ty221 https://github.com/ty221 You are fixing the true cause of a problem reported in #281 https://github.com/sup-heliotrope/sup/issues/281, as I see it. Escaping of fn is not done properly.

Is escaping spaces and quotes really necessary at all? Dir[fn] treats fn as File::fnmatch http://apidock.com/ruby/File/fnmatch/class pattern, so I'd expect you'd need to escape *?[ (backslash, asterisk, question mark and opening bracket). Also this fix will break check-attachment hooks, which will (and IMO should) expect non-escaped filename. Escape only the name you provide Dir[] with.

It would be very, very well appreciated if you first wrote a failing test that would pinpoint the cause of the bug reported in #281 https://github.com/sup-heliotrope/sup/issues/281 and then greened it with the fix. If you don't, please provide us with your core test cases (filenames that failed before fixing and work after fixing).

— Reply to this email directly or view it on GitHub https://github.com/sup-heliotrope/sup/pull/367#issuecomment-68625779.

[terotil]

Tymon Radzik, 2015-01-04 11:03:

I suggest you to check these ten names:

abcd \24
\file " this is file "
anna\john "Word"\document \sss
\edited
file \ january "report\today" file123\43

In all those cases, the problem is backslash. To fix those you only need to quote backslash (no quotes or space escaping necessary).

Have you tried

odd*name.txt weird?name.txt document[1].ods

Those incorporate the other File::fnmatch meta characters (asterisk, question mark and opening bracket) besides backslash.

Tero Tilus ## 050 3635 235 ## http://tero.tilus.net/

[terotil]

How about my main points

• unnecessary escapes (of double and single quotes and space) and
• preserving hook interface (i.e. not breaking check-attachment hooks people already have)

[tradzik]

Hmm... as you can see I moved my code down.

Sun Jan 04 2015 at 10:28:01 użytkownik Tero Tilus [email protected] napisał:

How about my main points

• unnecessary escapes (of double and single quotes and space) and
• preserving hook interface (i.e. not breaking check-attachment hooks people already have)

— Reply to this email directly or view it on GitHub https://github.com/sup-heliotrope/sup/pull/367#issuecomment-68626634.

[terotil]

Sorry, ill-timed reload or something. Code seems legit now. But...

It is only now that I start thinking of why that Dir[fn] globbing is there in the first place. And it is apparetly there so that you could actually enter a globbing pattern and that way attach multiple files at once. And I even remember using that feature myself. :smile:

It is pretty apparent that this fix now breaks that functionality. :frowning:

I can't really say if #281 is fixable without some major changes. Problem is the return value of BufferManager.ask_for_filename. Interface is vague. You can't tell if the return value is a plain filename or a globbing pattern that should be expanded, possibly to list of file names. Possibilities that come into my mind now, are

• don't support attaching files with globbing meta characters in their names (IMO bad and would make #281 a wontfix)
• don't support globbing when attaching files
• expand globbing pattern already inside BufferManager.ask_for_filename, change the return value to be a filename or a list of filenames and change all usages accordingly (may require substantial changes in places where BufferManager.ask_for_filename is called)
• some other change to BufferManager.ask_for_filename interface to make globbing support and #281 fix possible

Appears this wasn't trivial after all...

[eMBee]

task closed: http://www.google-melange.com/gci/task/view/google/gci2014/5882243478192128