Modify exiv2/actions.cpp return -3/253 when no metadata has been found.
|Status:||Closed||Start date:||11 Aug 2013|
|Assignee:||Andreas Huggel||% Done:|
|Category:||design||Estimated time:||2.00 hours|
1) we should print the error message when we set the error. We should not require -v (verbose) to be set to get this error message.
2) when -g (grep) has been used, we should not set an error if no tag matches
#917. Changing the condition for reporting an error. We should not report an error for missing metadata when the user uses [-g match]+
#1 Updated by Andreas Huggel about 5 years ago
Both current behaviors 1) and 2) are designed to meet the needs of both, users of the command line program - they can set -v to see some extra messages - and scripts - it is more concise to check the return value in a script than to parse the program's standard error output and it is easier if the program doesn't generate a lot of unneeded output. In addition, the behavior of the -g (grep) option follows that of the grep command, which also returns a non-zero value if it can't find what you're looking for. I guess one could argue what the default behavior should be (i.e., whether the program should output the -v messages by default and have a switch to suppress them instead), but I wouldn't be surprised if you can find an equal number of supporters for each of these options...
#2 Updated by Robin Mills about 5 years ago
Thanks for adding your comments, Andreas.
Are you saying that you think we should not do anything about this?
It's possible to make a case that the existing behavior is correct and requires no change. Having discussed the matter in Topic 1582, Torsten has stated that he can work-around the current behavior.
If you consider that we should do nothing, then please close this issue/feature as "Rejected".
#3 Updated by Robin Mills over 3 years ago
- Assignee changed from Robin Mills to Thomas Schmidt
May I ask you to have a look at this please? This is a "gray one" in which the existing behaviour is neither right nor wrong. I don't remember the details here, however you can see my thoughts when I opened this issue in response to topic 1582.
If you would like a bigger challenge, it would be good to make the -g option respect regular expressions. However do not use boost. The build consequences of boost outweigh the benefits of -g regular-expression. If you know/find decent public domain code for regular-expressions, can you add it to utils.cpp and use it to support the -g option?
#5 Updated by Thomas Schmidt over 3 years ago
- File 0917_returnvaluesanderror.patch added
- Assignee changed from Thomas Schmidt to Robin Mills
Patch attached for the following behavior:
o Return code is only an error (-3 i.e. 253) if none of the requested meta data type is present: when all types are requested (-pa), any type suffices. When a specific type (e.g. IPTC, -pi) is requested, some data of its type must be present.
o If tag filters are given, no error is returned when no key matches.
o Error "No (Exif|IPTC|XMP) data found in the file" is reported without giving -v argument, but only if no data of another requested type is present.
#6 Updated by Robin Mills over 3 years ago
r3566 Thank you very much to Thomas for working on this and providing the patch.
I've modified Thomas code slightly and hope not to have introduced a bug. If I have, I'll take ownership and fix it. I've also updated test files which are impacted. The test file output is modified because it now (correctly in my opinion) reports when no metadata is available to match the request.
#7 Updated by Robin Mills over 3 years ago
- Status changed from Assigned to Resolved
- Assignee changed from Robin Mills to Thomas Schmidt
I'm going to mark this as 'Resolved" as it builds and passes the test suite on the build server. http://exiv2.dyndns.org:8080/job/Exiv2-trunk/1835/
I've also changed the Assignee to Thomas Schmidt to ensure he is credited with this fix when the v0.25 release notes are compiled. Thank You, Thomas for working on this.
#9 Updated by Robin Mills over 3 years ago
- Status changed from Resolved to Assigned
I'm going to reopen this because I'm rather unhappy with a consequence of the fix. The most common report I request with exiv2 is -pa = print all. However, if there is no IPTC metadata, a message is written to stderr, however the return code is set to 0.
581 rmills@rmillsmbp:~/gnu/exiv2/trunk/test $ exiv2 -pa ~/R.jpg | wc /Users/rmills/R.jpg: (No IPTC data found in the file) 177 837 12153 582 rmills@rmillsmbp:~/gnu/exiv2/trunk/test $ echo $? 0 583 rmills@rmillsmbp:~/gnu/exiv2/trunk/test $This is rather inconsistent. If we write to stderr, we should set a return code that is != 0. And equally, if we set a return code != 0, we should always write to stderr.
I am very happy with the return code of 0. I think the semantics of -pa are "give me all you can find" and no message "(No IPTC data found in the file)" should be reported by default. The options -pa --verbose should report "No IPTC data found in the file" and set a return code of -3/253. I'd also like to see us return -3 (and not 253). The man page src/exiv2.1 should be updated with a section "EXIT STATUS" and the return codes documented as recommended here: https://wiki.archlinux.org/index.php/Man_page
There are other related matters here. The man page documents -pa as being the same as -Pkyct and -pt as -PEkyct. There's something inconsistent about this (and may simply be a man page error). We should also add the option -pe to report only Exif data (to correspond to options -pi and -px which report IPTC and XMP respectively.
When we do this, we're going to change return codes and error messages. We'll need to update the reference files in the test file. This usually only involves copying output files from test/tmp/ to test/data. For example, if bugfixes.sh reports something you understand, you can silence him with:
cp tmp/bugfixes-test.out-stripped data/bugfixes-test.outHere's a summary:
- We should set a return code != 0 if we write to stderr
- We should always return a code != 0 if we write to stderr
- Fix odd codes such as 253 to be -3
- Add option -pe to only report Exif metadata
- Change semantics of -pa to be silent when the file does not contain any metadata sections (Exif, IPTC, XMP)
- Change semantics of -pa --verbose to complain when a metadata section is missing (and set return != 0)
- Add a section EXIT STATUS to the man page
- Review if the man page -pa == -Pkyct is true
- Update the man page about -pe (both Command Summary and Options)
- Clean up the reference files in the test suite
I'll leave this assigned to Thomas S. Thomas, if you're too busy to work on this, please assign it to me and I'll take care of it.
#13 Updated by Andreas Huggel almost 3 years ago
- Assignee set to Andreas Huggel
- % Done changed from 0 to 50
1. We should set a return code != 0 if we write to stderr
2. We should always return a code != 0 if we write to stderr
That's difficult to implement and not practical imho. (The library may write to stderr without the utility knowing.)
3. Fix odd codes such as 253 to be -3
I don't know how to do that. It may be better to return small positive values instead.
4. Add option -pe to only report Exif metadata
There are several options to only report Exif metadata: -pt, -pv and -ph. Maybe we should rename -pt to -pe.
5. Change semantics of -pa to be silent when the file does not contain any metadata sections (Exif, IPTC, XMP)
6. Change semantics of -pa --verbose to complain when a metadata section is missing (and set return != 0)
Changed back to original: silent without -v, inform about absence of metadata with -v. In accordance with the "give me all you can find" comment I also see this as just an info message, not a complaint (warning or error), thus it now returns 0 in either case.
7. Add a section EXIT STATUS to the man page
8. Review if the man page -pa == -Pkyct is true
Yes, it is.
9. Update the man page about -pe (both Command Summary and Options)
10. Clean up the reference files in the test suite
Further, with -g or -K, exiv2 now returns 1 if no matching tags were found. That's in-line with UNIX grep.
#14 Updated by Nigel Winterbottom over 2 years ago
This bug just bit me because I'm chaining actions in a find command. Of course the non-zero exit code of exiv2 prevents later actions happening. MY usage case has not been discussed above and can be summarised thus:
$ exiv2 -V exiv2 0.22 001600 (32 bit build) Copyright (C) 2004-2011 Andreas Huggel. [snipped] nigel@D8300:~$ exiv2 -v -Pv -g Exif.Image.Model ~/Pictures/CIMG4496.JPG File 1/1: /home/nigel/Pictures/CIMG4496.JPG EX-Z40 /home/nigel/Pictures/CIMG4496.JPG: No IPTC data found in the file /home/nigel/Pictures/CIMG4496.JPG: No XMP data found in the file nigel@D8300:~$ echo $? 253 nigel@D8300:~$
Note I'm making no specific request to examine IPTC or XMP data.
I'm on Ubuntu 12.04LTS and was quite prepared to build the latest from source but see 0.25 does not include any fix. Please include my usage in any further test cases.
I'm going to try the perl module exiftool now - I hope it supports file renames because I have clashing file names across different Casio models and ultimately that's what I need to do.
#15 Updated by Robin Mills over 2 years ago
Thank you for your feedback on this matter. I see you are using v0.22 which is rather elderly version of exiv2. Perhaps you could try/test our daily build to see if you have a use case that defeats our fix. You can collect it from: http://exiv2.dyndns.org:8080/userContent/builds/Categorized/Platform/linux/
If you do find a flaw in our fix, I'll be happy to both repair it and and add a regression detector to our test suite.
Thank You for using exiv2.
#16 Updated by Robin Mills over 2 years ago
Your use-case appears to be fixed on the trunk:
1071 rmills@rmillsmbp:~/clanmills $ exiv2 -v -Pv -g Exif.Image.Model http://clanmills.com/Stonehenge.jpg File 1/1: http://clanmills.com/Stonehenge.jpg NIKON D5300 1072 rmills@rmillsmbp:~/clanmills $ echo $? 0 1073 rmills@rmillsmbp:~/clanmills $ cd ~/temp ; curl -O http://clanmills.com/Stonehenge.jpg % Total % Received % Xferd Average Speed Time Time Time Current Dload Upload Total Spent Left Speed 100 6599k 100 6599k 0 0 1658k 0 0:00:03 0:00:03 --:--:-- 1934k 1074 rmills@rmillsmbp:~/temp $ exiv2 -di Stonehenge.jpg 1075 rmills@rmillsmbp:~/temp $ exiv2 -dx Stonehenge.jpg 1076 rmills@rmillsmbp:~/temp $ exiv2 -v -Pv -g Exif.Image.Model Stonehenge.jpg File 1/1: Stonehenge.jpg NIKON D5300 Stonehenge.jpg: No IPTC data found in the file Stonehenge.jpg: No XMP data found in the file 1077 rmills@rmillsmbp:~/temp $ echo $? 0 1078 rmills@rmillsmbp:~/temp $