Bug #565 xASL_adm_DeleteFileList symlink by HenkMutsaerts · Pull Request #667 · ExploreASL/ExploreASL

HenkMutsaerts · 2021-06-19T16:03:46Z

Linked issue

Closes #565

Notes for reviewing

Please check the issue, thanks!

jan-petr

Why do we use the delete function instead of calling xASL_delete one by one in the first place? Is that significantly faster?

HenkMutsaerts · 2021-07-15T11:09:54Z

Why do we use the delete function instead of calling xASL_delete one by one in the first place? Is that significantly faster?

If you call delete, it will delete in one batch, which crashes if you have symbolic links inside the list of which the directory above it was also in the list but is already deleted. So delete crashes if you have a list with files that are also linked within a symbolic link folder. This is the case for BASILs output. BASIL outputs each fit iteration in a separate subfolder, and creates a symbolic link to the latest folder, assuming this is the most optimal output.

MichaelStritt · 2021-07-19T09:46:57Z

@HenkMutsaerts: Should I just check if I can delete a list of files and if that works it's okay? Or does this require more extensive testing?

MichaelStritt

Removing files seems to work and is fast, but the file list that is returned is empty?

>> pathTest = '.\testFolderToRemove';
>> testFiles = xASL_adm_GetFsList(pathTest, '^.+\.exe$');
>> testFiles

testFiles =

  1×8 cell array

    {'myFile (1).exe'}    {'myFile (2).exe'}    {'myFile (3).exe'}    {'myFile (4).exe'}    
    {'myFile (5).exe'}    {'myFile (6).exe'}    {'myFile (7).exe'}    {'myFile (8).exe'}

>> removedFiles = xASL_adm_DeleteFileList(pathTest, '^.+\.exe$');
>> removedFiles

removedFiles =

  0×0 empty char array

jan-petr · 2021-07-19T19:17:26Z

Why do we use the delete function instead of calling xASL_delete one by one in the first place? Is that significantly faster?

If you call delete, it will delete in one batch, which crashes if you have symbolic links inside the list of which the directory above it was also in the list but is already deleted. So delete crashes if you have a list with files that are also linked within a symbolic link folder. This is the case for BASILs output. BASIL outputs each fit iteration in a separate subfolder, and creates a symbolic link to the latest folder, assuming this is the most optimal output.

Yes. I understand why delete crashes. But I don't understand why we have the try-catch statement with catch calling xASL_delete one by one - why don't we skip the use of delete and delete only using the xASL_delete in the catch branch - would that be so slower?

I am simply looking for the motivation to try lines 66-67 and not going right away for solution on line 68-79? Is that faster?

HenkMutsaerts · 2021-07-19T20:32:38Z

Yes, delete is faster, that was the only motivation for the try

jan-petr · 2021-07-22T19:12:01Z

Yes, delete is faster, that was the only motivation for the try

OK. That's a satisfactory answer :)

jan-petr

OK.

MichaelStritt · 2021-07-22T19:53:52Z

Develop

Test without space in path

>> pathTest = '.\Server_xASL\sdfsdf';
>> filepaths = xASL_adm_DeleteFileList(pathTest, '^.+$');
>> filepaths
filepaths =
  4×1 cell array
    {'.\Server_xASL\sdfsdf\Git-2.32.0-64-bit - Kopie (2).exe'    }
    {'.\Server_xASL\sdfsdf\Git-2.32.0-64-bit - Kopie - Kopie.exe'}
    {'.\Server_xASL\sdfsdf\Git-2.32.0-64-bit - Kopie.exe'        }
    {'.\Server_xASL\sdfsdf\Git-2.32.0-64-bit.exe'                }

Test with space in path

>> pathTest = '.\Server_xASL\sdf sdf';
>> filepaths = xASL_adm_DeleteFileList(pathTest, '^.+$');
>> filepaths
filepaths =
  4×1 cell array
    {'.\Server_xASL\sdf sdf\Git-2.32.0-64-bit - Kopie (2).exe'    }
    {'.\Server_xASL\sdf sdf\Git-2.32.0-64-bit - Kopie - Kopie.exe'}
    {'.\Server_xASL\sdf sdf\Git-2.32.0-64-bit - Kopie.exe'        }
    {'.\Server_xASL\sdf sdf\Git-2.32.0-64-bit.exe'                }

Feature branch

Test without space in path

>> pathTest = '.\Server_xASL\sdfsdf';
>> filepaths = xASL_adm_DeleteFileList(pathTest, '^.+$');
>> filepaths
filepaths =
  0×0 empty char array

Test with space in path

>> pathTest = '.\Server_xASL\sdf sdf';
>> filepaths = xASL_adm_DeleteFileList(pathTest, '^.+$');
>> filepaths
filepaths =
  0×0 empty char array

Conclusion

@HenkMutsaerts: Seems like this bug is not in develop and is not related to spaces in file names.

HenkMutsaerts · 2021-07-22T20:32:10Z

@MichaelStritt can you retest? I indeed reinitialized the output value filepaths through the new code, which would then find any residual files to be deleted but this would also render this list empty if the requested files were correctly deleted. Now this behavior is corrected, and another output parameters pathsNotDeleted list added for clarity. Is it now working correctly at your end?

MichaelStritt

Seems to work now 👍

>> pathWithoutSpaces = '.\Server_xASL\testtest';
>> [deletedFiles,NotDeletedFiles] = xASL_adm_DeleteFileList(pathWithoutSpaces, '^.+$')

deletedFiles =

  4×1 cell array

    {'.\Server_xASL\testtest\Git-2.32.0-64-bit - Kopie (2).exe'    }
    {'.\Server_xASL\testtest\Git-2.32.0-64-bit - Kopie - Kopie.exe'}
    {'.\Server_xASL\testtest\Git-2.32.0-64-bit - Kopie.exe'        }
    {'.\Server_xASL\testtest\Git-2.32.0-64-bit.exe'                }

NotDeletedFiles =
  0×0 empty char array

>> pathWithSpaces = '.\Server_xASL\test test';
>> [deletedFiles,NotDeletedFiles] = xASL_adm_DeleteFileList(pathWithSpaces, '^.+$')

deletedFiles =
  4×1 cell array

    {'.\Server_xASL\test test\Git-2.32.0-64-bit - Kopie (2).exe'    }
    {'.\Server_xASL\test test\Git-2.32.0-64-bit - Kopie - Kopie.exe'}
    {'.\Server_xASL\test test\Git-2.32.0-64-bit - Kopie.exe'        }
    {'.\Server_xASL\test test\Git-2.32.0-64-bit.exe'                }


NotDeletedFiles =
  0×0 empty char array

MichaelStritt

So the deleted files list seems to work, the other one seems to be bugged though. Check out my UnitTest. You should be able to run it pretty easily.

Testing/UnitTests/xASL_ut_function_xASL_adm_DeleteFileList.m

MichaelStritt

Sorry, I should have read the header more carefully. I thought pathsNotDeleted would be the paths in the directory that weren't deleted. You describe it correctly though 👍

Filelists are now also deleted file-by-file, checked again if all files are deleted, and warnings about deleting a non-existing file are suppressed. This helps e.g., with a repetition of the identical file in the same list, e.g., in the case of symlinks

HenkMutsaerts requested review from MichaelStritt and jan-petr June 19, 2021 16:03

HenkMutsaerts assigned MichaelStritt Jun 19, 2021

HenkMutsaerts changed the base branch from main to develop June 19, 2021 16:04

MichaelStritt added the bug Something isn't working label Jun 21, 2021

jan-petr reviewed Jul 14, 2021

View reviewed changes

MichaelStritt requested a review from jan-petr July 19, 2021 12:06

MichaelStritt suggested changes Jul 19, 2021

View reviewed changes

jan-petr approved these changes Jul 22, 2021

View reviewed changes

MichaelStritt self-requested a review July 23, 2021 08:28

MichaelStritt approved these changes Jul 23, 2021

View reviewed changes

MichaelStritt self-requested a review July 23, 2021 08:47

MichaelStritt suggested changes Jul 23, 2021

View reviewed changes

HenkMutsaerts commented Jul 26, 2021

View reviewed changes

MichaelStritt approved these changes Jul 26, 2021

View reviewed changes

HenkMutsaerts and others added 5 commits July 26, 2021 10:32

#565 xASL_quant_Basil: Add output symbolic link documentation

341b68a

#565 xASL_adm_DeleteFileList: Add output PathsNotDeleted

d9d629b

#565 xASL_ut_function_...: Add unit test

6f9713d

#565 xASL_adm_DeleteFileList: Improve unit test

464d43e

MichaelStritt force-pushed the bug-#565_xASL_adm_DeleteFileList_Symlink branch from b87f04a to 464d43e Compare July 26, 2021 08:32

MichaelStritt merged commit 464d43e into develop Jul 26, 2021

MichaelStritt deleted the bug-#565_xASL_adm_DeleteFileList_Symlink branch July 26, 2021 08:33

Conversation

HenkMutsaerts commented Jun 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Linked issue

Notes for reviewing

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

HenkMutsaerts commented Jul 15, 2021

Uh oh!

MichaelStritt commented Jul 19, 2021

Uh oh!

MichaelStritt left a comment

Choose a reason for hiding this comment

Uh oh!

jan-petr commented Jul 19, 2021

Uh oh!

HenkMutsaerts commented Jul 19, 2021

Uh oh!

jan-petr commented Jul 22, 2021

Uh oh!

jan-petr left a comment

Choose a reason for hiding this comment

Uh oh!

MichaelStritt commented Jul 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Develop

Test without space in path

Test with space in path

Feature branch

Test without space in path

Test with space in path

Conclusion

Uh oh!

HenkMutsaerts commented Jul 22, 2021

Uh oh!

MichaelStritt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichaelStritt left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

MichaelStritt left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HenkMutsaerts commented Jun 19, 2021 •

edited

Loading

MichaelStritt commented Jul 22, 2021 •

edited

Loading

MichaelStritt left a comment •

edited

Loading