bugGNU Octave - Bugs: bug #54456, Implementation of isfolder

 
 

bug #54456: Implementation of isfolder

Submitter:  Guillaume <gyom>
Submitted:  Tue 07 Aug 2018 11:27:26 AM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Matlab Compatibility
Status:  Fixed Assigned to:  None
Originator Name:  Guillaume Open/Closed:  * Closed
Release:  * dev Operating System:  * Any
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Mon 13 Aug 2018 06:45:35 PM UTC, comment #8: 

I added isfolder to Octave in this cset (https://hg.savannah.gnu.org/hgweb/octave/rev/7c5956c45a29).  I made a small change or two.  First, I made a separate check for a string or cellstr input and produce an error if the input F is not one of these types.  Second, I used the second output of stat() to see if there is an error rather than ! isempty (info) which was ~10% faster.

I checked things in under Guillaume's name so he gets the credit for this patch.  Marking as fixed and closing report.

Rik <rik5>
Group administrator
Mon 13 Aug 2018 11:59:48 AM UTC, comment #7: 

Thanks again Mike. I made the requested changes in the new attachment.

Concerning tab completion, which file has to be modified?

(file #44773)

Guillaume <gyom>
Fri 10 Aug 2018 11:26:56 PM UTC, comment #6: 

Thanks for the update. I've tested both implementations, works for me. I still like the version that doesn't call the 'exist' function at all and is consistent with other functions like 'fileattrib' that also use 'stat'.

One edge case example that doesn't work with the 'make_absolute_filename' implementation:


cd /
isfolder ..



The copyright header is not correct, left over from isdir.m.

Should 'isfolder' be handled so that tab completion only shows directory and file names? I think so.

Mike Miller <mtmiller>
Group Member
Fri 10 Aug 2018 01:41:22 PM UTC, comment #5: 

Thanks Mike. I attach a new version that adds a call to make_absolute_filename() and there is a commented piece of code to use stat() instead if preferable. I also added a couple of unit tests.

(file #44760)

Guillaume <gyom>
Fri 10 Aug 2018 01:40:43 AM UTC, comment #4: 

My point is that Octave's isfolder implementation should not rely on exist(..., "dir").

I would recommend that you try rewriting your isfolder.m to use stat() and S_ISDIR(). These functions are guaranteed to match standard POSIX semantics and not depend on the Octave load path. In case exist and/or isdir ever change to use the load path, isfolder must not change in the same way.

I think the following should happen in this order

1. isfolder is added to Octave, hopefully Octave 5, preferably based on stat(), not relying on exist()
2. isdir is moved to the new scripts/legacy directory, just for organization purposes
3. all other Octave m-file functions that call isdir should be changed to use isfolder instead, and the same with Octave Forge packages (that part may take some time)
4. at some point in the future, exist and isdir may search for directories on the load path for Matlab compatibility

Mike Miller <mtmiller>
Group Member
Thu 09 Aug 2018 04:22:18 PM UTC, comment #3: 

This is true that isdir and isfolder are not identical but the proposed isfolder is compatible with Matlab. What is left is a bug with isdir. It probably has been there for a long time so it might not be worth fixing now that this function is not recommended. The behavior of searching through the search path doesn't seem desirable to me anyway. fopen/load do so and it can be the source of 'funny' bugs. In that respect, it's great to have Octave displaying a warning "... found in searching load path".

Guillaume <gyom>
Wed 08 Aug 2018 05:43:53 PM UTC, comment #2: 

I guess that's another bug in Octave, then, exist(..., 'dir') does not find directories by name on the load path.


>> mkdir ('/tmp/foo');
>> exist ('foo', 'dir')
ans = 0
>> addpath ('/tmp');
>> exist ('foo', 'dir')
ans = 0
>> cd /tmp
>> exist ('foo', 'dir')
ans =  7


But we should not change that until we work out the differences between isdir and isfolder, since other functions use isdir, and the logic may break if the behavior of isdir changes unexpectedly.

Mike Miller <mtmiller>
Group Member
Wed 08 Aug 2018 03:50:53 PM UTC, comment #1: 

I think the logic will need to be modified a little bit: `isdir()` considers the Matlab search PATH the way `exist(..., 'dir')` does. The new Matlab `isfolder()` is stricter, and just evaluates absolute paths, or relative paths relative to pwd.

Andrew Janke <apjanke>
Tue 07 Aug 2018 11:27:26 AM UTC, original submission:  

Octave has isdir(), which is not recommended anymore in Matlab:
https://www.mathworks.com/help/matlab/ref/isdir.html
and isfolder() has been introduced instead:
https://www.mathworks.com/help/matlab/ref/isfolder.html

I guess isdir.m should be moved in to scripts/legacy and I attach a version of isfolder.m (it's just a copy of isdir.m with the extra option that the input value can be a cellstr).

Guillaume <gyom>

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #44773:  isfolder.m added by gyom (2KiB - text/x-matlab)
file #44760:  isfolder.m added by gyom (2KiB - text/x-objcsrc)
file #44734:  isfolder.m added by gyom (2KiB - text/x-objcsrc)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by rik5 (Posted a comment)
  • -email is unavailable- added by mtmiller (Posted a comment)
  • -email is unavailable- added by apjanke (Posted a comment)
  • -email is unavailable- added by gyom (Submitted the item)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 5 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2018-08-13 rik5 StatusNone Fixed
        Open/ClosedOpen Closed
    2018-08-13 gyom Attached File- Added isfolder.m, #44773
    2018-08-10 gyom Attached File- Added isfolder.m, #44760
    2018-08-07 gyom Attached File- Added isfolder.m, #44734

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code