Sat 12 Dec 2015 11:53:15 AM UTC, comment #26:
Backporting to stable would be fine with me, but see comment #17.
I can imagine that users perceive this as a bug fix rather than a new feature or some added functionality.
([OT] reading back the whole thread after I forgot all about it I find itremarkable that such an apparently tiny issue required so much attention 8-) )
|
Fri 11 Dec 2015 11:13:45 PM UTC, comment #25:
This bug was fixed on the default branch and not on the stable.
Consider fix it in 4.0.1 version
|
Sun 31 May 2015 08:04:27 PM UTC, comment #24:
I changed the cset to use strncmp to find the '\\' prefix since it was 10X faster. I checked in the overall cset on the development branch here (http://hg.savannah.gnu.org/hgweb/octave/rev/8b1884214798).
|
Mon 25 May 2015 06:09:52 PM UTC, comment #23:
Sorry here's the patch
(file #34092)
|
Mon 25 May 2015 06:09:14 PM UTC, comment #22:
OK new changeset attached.
I've tried to keep the elegancy of the OP's solution. Tested on Windows and Linux, all 26 tests pass.
@Rik, will you review the patch? If OK, I can push, or you can if you want.
Thanks
release => development cf. comment #17
|
Mon 25 May 2015 04:21:52 PM UTC, comment #21:
I'm fine with the simpler solution, but then I'll add a stanza in the texinfo header that fullfile.m does not do any validation whatsoever.
Anyway the proposed (by the OP) solution fails several tests, due to Carnë's (fullfile's author) trick of strjoin-ing file parts with fileseps and afterward removing all double fileseps. That trick, ingenious in itself, introduces double fileseps all over the place, incl. in pos. 1 and 2.
So I'm afraid the final fix is going to be a little uglier :-(
I'll take some more time to think up a good solution. Maybe a preliminary count of leading fileseps (before the strjoin) plus an "if (nr_of_leading_fileseps >= 2)" would do the trick.
|
Mon 25 May 2015 03:55:00 PM UTC, comment #20:
I think it is okay to be simpler since it doesn't appear that Matlab does any better in this regard.
If the path needs to be verified afterwards then canonicalize_path_name() or exist() might be used.
|
Mon 25 May 2015 09:51:49 AM UTC, comment #19:
I think that it would be ok to go with simpler solution for few reasons:
- fullfile () in general is not validating paths, it expects that path parts are already valid.
- In MATLAB, fullfile('\\server') will return '\\server' which is maybe ok for someone as intermediate full path result.
|
Mon 25 May 2015 08:36:13 AM UTC, comment #18:
(picking up again)
@Rik:
The OP's suggestion is a bit too simple as even on Windows, UNC file path constructs should have a first part of the form '\\server\sharename', at the very least ~ '\\a\b'. Also some tests have to be adapted.
So, should we include a check for valid UNC full filenames, incl. a warning/error message, or let valid filenames be the responsibility of the user?
In case of the former a fairly simple regexp would do but then I'd also check for valid characters in at least 'server' and 'share', something like:
In the latter case even '\\' would be a possible result of fullfile.m.
|
Thu 21 May 2015 08:38:43 PM UTC, comment #17:
It's simple enough, but I would put in on the development branch. It's more along the lines of an improvement, and closer conformity to Matlab, than a bug fix where 2 + 2 = 5.
|
Thu 21 May 2015 08:33:15 PM UTC, comment #16:
Right, I'll prepare a changeset according to the OP's suggestion as it the fix is only required for Windows.
My original cs is a bit overdone and I think invalid or incomplete anyway for *nix.
Rik,
1. - You want the fix on stable?
2. - Shall I also add a check for the max file/path name length on Windows? (260 chars incl. drive letter IIRC, I'll look it up) ?
I'm not sure I can fix this the coming Easter weekend, next week is stuffed as well, but this looks to be a minor fix.
|
Thu 21 May 2015 12:05:30 AM UTC, comment #15:
@Philip: The promotion of the logical returned from ispc to numeric is automatic in Octave and is fine. Did you want to re-work your patch, or is it good already?
|
Thu 30 Apr 2015 09:15:49 PM UTC, comment #14:
AFAICS now, on Linux //server/share constructs aren't valid. Samba seems to require
smb://server/share
syntax, which that can be shared in the URL syntaxes like file:///some/file, ftp://server, etc. that aren't supported by Matlab anyway
That means the fix by the OP would in principle be fine.
My only (minor) concern is that ispc() returns a logical but is supposed to be interpreted as integer 1 or 0 on Windows. If that is fine with the core devs I'll prepare a changeset.
|
Tue 07 Apr 2015 09:29:14 AM UTC, comment #13:
Rik
@comment #11:
IMO responsibility for valid paths in case of ('/', '/abc')-constructs should be the responsibility of the user, not of fullfile.m; after all the form of the proposed fullfile.m result represents valid path name (UNC) syntax on Windows.
I foresee that such constructs could be cooked up by scripts, so your question is quite right.
@comment #12:
What usually happens is that on at least Windows network paths, drive letters (and partial paths from there like "f:\path\to\shared\folder") are replaced by share names that appear right after the server name:
\\server\sharename\path\to\subdir\relative\to\network\share
\\serverc:\tmp\dir1\dir2 is syntax I've never seen on Windows.
I'm familiar with the other two \\?\ UNC forms, but they're rarely if ever used by ordinary users, I've only used them when poking around in the mountmgr section of the Windows registry.
And then we have the URL forms I mentioned in a previous post:
I think those \\?\ and file:/// and URL path forms had better be left for a future fullfile.m upgrade, as soon as Matlab supports it as well.
Right now the relevant thing is whether //server/share/file is valid on Linux (e.g., samba etc when referring to shares on Windows hosts).
If not, the suggestion by the OP will work fine and is the simplest; yet I do not like adding integers and logicals (1 + ispc()).
BTW the OP's mod would also morph ('\', \ abc') into '\\abc'.
|
Mon 06 Apr 2015 11:59:14 PM UTC, comment #12:
UNC looks more cumbersome than just '\\'. See http://en.wikipedia.org/wiki/UNC_path.
If this is valid syntax, then there is a problem as the '\' between 'server' and 'c:' is eaten.
It also appears that these are valid UNC forms
|
Mon 06 Apr 2015 11:46:04 PM UTC, comment #11:
What should the behavior of the following be:
THe usual UNIX behavior is that excess slashes are collapsed to one. The old function did this, the new one does not.
|
Mon 06 Apr 2015 04:19:54 PM UTC, comment #10:
@Rik:
See dicussion below.
Matlab r2012a's fullfile() accepts & returns '\\server\path' (literally) so I surmise that is a valid outcome.
That would be in favor of Matlab compatibility.
@Mike:
On the page you mention it is stated that ML accepts "full path names including ".... UNC path '\\' string" which includes a server name - please correct me if I'm wrong but a UNC path is a network path.
It may be (I just do not know) that on *nix such forms are not legal - is that so? Not even with Samba accessing Windows shares?
I cannot test neither at work, as there are no network "shares" (at least not accessible in the form '\\svr\path' by me as ordinary user), nor at home as Matlab r2015a prerelease has timed out (just a few days ago :-( ).
BTW it just occurred to me that fullfile.m doesn't check for the 260 char length limit on UNC paths. Maybe something for another bug report & -fix.
|
Mon 06 Apr 2015 03:11:43 PM UTC, comment #9:
I can't test, but according to:
https://www.mathworks.com/help/matlab/matlab_env/specify-file-names.html
Matlab treats paths beginning with a double-backslash as absolute directories, just as if it were a path beginning with a drive letter.
|
Mon 06 Apr 2015 03:07:51 PM UTC, comment #8:
Is this a Matlab compatibility issue, or is this an Octave extension beyond Matlab to support network shares?
|
Mon 06 Apr 2015 08:56:56 AM UTC, comment #7:
Here's a patch. I'll cc Rik for review
(BTW, title adapted as this bug shows on any platform)
(file #33562)
|
Sun 05 Apr 2015 09:17:53 PM UTC, comment #6:
I have a fix; need to test on Linux.
It's not going to look pretty; that is unavoidable because of the way fullfile.m has been written:
Using strjoin(), introducing intermediate consecutive fileseps when input partial file paths have leading or trailing fileseps. Those intermediate consecutive fileseps have to be distinguished from leading fileseps for network paths, but '//' isn't acceptable; etc.
|
Wed 01 Apr 2015 05:32:44 PM UTC, comment #5:
I'd swear I've seen or maybe even fixed a similar bug a while ago.....
Yes: http://hg.savannah.gnu.org/hgweb/octave/rev/c304687571c8
OK I'll have a look later on, if nobody beats me to it.
This one is different as it also fails on *nix; it looks nastier as there's no reliable way to distinguish an erroneous path ('\\wrong\path\name') from a valid network path ('\\server\d\rive\path'). fullfile.m simply strips multiple leading fileseps away.
Maybe just assuming that two consecutive fileseps always constitute a network path would do the trick? (with a check on Windows that there's no <driveletter><colon> sequence> ?)
In addition fullfile.m doesn't know about e.g.,
file:///rootdir/path
ftp://server/rootdir/path
www://server/rootdir/path
http://server/rootdir/path
...and other path forms. But that's a separate issue.
OS -> Any; Status -> confirmed
|
Wed 01 Apr 2015 12:21:06 PM UTC, comment #4:
fullfile('//server', 'file.txt') returns string back with the / changed to a backslash, and with only one of them before the server name.
|
Wed 01 Apr 2015 06:20:01 AM UTC, comment #3:
You could also try if it works with forward slashes.
From the Octave prompt, forward slashes generally work fine as file separator on Windows systems.
|
Tue 31 Mar 2015 09:48:50 PM UTC, comment #2:
Oughh, now backspash is not escape character anymore..
I hope that you can understand what was an issue.
|
Tue 31 Mar 2015 09:46:48 PM UTC, comment #1:
Backslashes have been removed since they were parsed as escape characters.
Sample code should look like:
|
Tue 31 Mar 2015 09:40:43 PM UTC, original submission:
Function fullfile should allow double backslash to apear at the begining of the path to support network drive paths on Windows platform.
e.g.
shoud return:
not
Simplest way to achive that functionality would be by changing fullfile.m:62 from:
to
|