fxstein / GoProX

The missing GoPro data and workflow manager for macOS
MIT License
25 stars 3 forks source link

Bug: Check for errors when importing files #18

Closed jip149 closed 2 years ago

jip149 commented 2 years ago

Exit status of exiftool in _import_media function is not checked. This is dangerous especially since cleanup can now be used with import.

fxstein commented 2 years ago

Now I remember why I originally did not allow clean with import only. Had some issues with the exit status of exiftool. Need to investigate and do some testing. Hence the exit status of exiftool is currently unchecked in all parts of goprox. Definitely needs to be addressed.

fxstein commented 2 years ago

So here is the issue:

I need the ability to re-import and re-process files, sometimes all, sometimes sparse - e.g. just the missing files. When an output file already exists, exiftool throws an error and moves to the next files. These are acceptable errors as the default behavior of goprox is based on skipping existing files. However it means the error status of exiftool is no longer 0.

Other errors obviously need to get caught. Need to see if I can tell exiftool that existing target files errors are to be ignored while others are not.

jip149 commented 2 years ago

I think it would still be dangerous to clean files if some existing files were skipped.

Imagine an import is stopped in the middle for whatever reason (power outage, disconnected drive…), one of the file may be existing but incomplete. If the user does not pay attention and blindly runs the command again, a file will be lost.

It would not seem a problem to me if cleanup were allowed only when exiftool return no error. If exiftool fails because of existing files, it would be the responsibility of the user to solve the problem (use archive and cleanup, manually cleanup, or remove existing files and run the command again).

What do you think?

fxstein commented 2 years ago

That is indeed a potentially simple solution. Also still looking at some exiftool options. Will get back asap.

fxstein commented 2 years ago

FYI This is why I personally always make archives that I store separately, even at the cost of a lot of extra storage, because it is the one guarantee I have to be able to re-run from scratch. Especially since developing goprox is still missing key features that I would like to apply to parts of my historic library.

But this issue needs to be addressed no matter what.

fxstein commented 2 years ago

@giga Pushed the initial error handling logic as discussed. Can you test before I cut this into a new release? Thanks!

Example output when encountering one or more exiftool errors:

[2022-10-22 12:52:31] Info: Starting media import... 
[2022-10-22 12:52:31] Info: Source: /Users/xxxxx/Desktop/20221021120343_GoPro_Hero10_2826 
[2022-10-22 12:52:31] Info: Library: /Users/xxxxx/goprox-test/imported 
[2022-10-22 12:52:31] ======== /Users/xxxxx/Desktop/20221021120343_GoPro_Hero10_2826/DCIM/100GOPRO/G0030790.JPG [1/2]
[2022-10-22 12:52:31] Error: '/Users/xxxxx/goprox-test/imported/2022/20221001/20221001122745_GoPro_Hero10_2826_G0030790.JPG' already exists - /Users/xxxxx/Desktop/20221021120343_GoPro_Hero10_2826/DCIM/100GOPRO/G0030790.JPG
[2022-10-22 12:52:31] ======== /Users/xxxxx/Desktop/20221021120343_GoPro_Hero10_2826/DCIM/100GOPRO/G0030789.JPG [2/2]
[2022-10-22 12:52:31] Error: '/Users/xxxxx/goprox-test/imported/2022/20221001/20221001122735_GoPro_Hero10_2826_G0030789.JPG' already exists - /Users/xxxxx/Desktop/20221021120343_GoPro_Hero10_2826/DCIM/100GOPRO/G0030789.JPG
[2022-10-22 12:52:31] Warning: exiftool reported one or more errors. Please check output. 
[2022-10-22 12:52:31] Info: Finished media import 
[2022-10-22 12:52:31] Info: Cleaning Source Media... 
[2022-10-22 12:52:31] Info: Source: /Users/xxxxx/Desktop/20221021120343_GoPro_Hero10_2826 
[2022-10-22 12:52:31] Error: Will not clean /Users/xxxxx/Desktop/20221021120343_GoPro_Hero10_2826 due to exiftool error status: 1 
[2022-10-22 12:52:31] Error: Please check output. 
jip149 commented 2 years ago

Hello,

Many thanks for the reactivity. I have this error with latest version: _clean_media:[:8: ']' expected

fxstein commented 2 years ago

Could you please open a new issue and share more details?

Would be great if you could run your command with --debug and copy the output there.

Thanks!