ATM errors are output into stderr while json record lacks any hints on what went wrong
$> git annex add --json longfilenametogetlotsoferrorsandevenlonger4
longfilenametogetlotsoferrorsandevenlonger4: setFileMode: permission denied (Operation not permitted)
{"command":"add","success":false,"file":"longfilenametogetlotsoferrorsandevenlonger4"}
would be nice(r) to have
$> git annex add --json longfilenametogetlotsoferrorsandevenlonger4
{"command":"add","success":false,"file":"longfilenametogetlotsoferrorsandevenlonger4", "msg": "setFileMode: permission denied (Operation not permitted)"}
or may be even an explicit "errormsg" or alike instead of just a generic "msg"
Is the benefit of doing this being able to correlate the error message with the filename whose processing caused the error?
For that benefit to be realized, the error message would have to be included in the same json object with the file being processed. If a separate json object were emitted containing only the error message it would not be clear what file it was related to (especially when git-annex is running concurrent jobs), and so that does not seem any better than output to stderr.
Looks like implementation would just involve making outputError check if there is a jsonBuffer, and add the error message to it, probably using an array since there could be multiple warning messages.
I kind of feel like they should still go to stderr in addition to any json output, because any existing json consumers may rely on the current stderr behavior to let the user know what went wrong.
yes -- I could have been clearer that association to the file (if present, may be error is generic and still would be worth to communicate via a simple json message, alike recently added INFO passed from custom special remotes) would be valuable! As for stderr output -- would lead to duplication (we just changed to output at least a "tip" of stderr output which might be heavy). If it is a matter of not breaking tools which would not support this new behavior, having a config or --option-hate-to-suggest-to-addone, would be valuable so we could disable "double-casting" the error messages
A problem with this idea is that error messages are relatively uncommon, and if they're hidden away in an extra field of an existing json message, it would be easy for the consumer to forget to handle that uncommon case.
Note that git-annex currently doesn't document the actual json structure it uses, which is more or less ok because any given git-annex subcommand will always output the same json structure (except some note fields that only sometimes appear, whose data is targeted at humans). It's self-documenting. Using json for errors breaks that pattern.
Currently when there's an exception in processing a file, the error is thrown and then caught and output to stderr, and this prevents the accumulated json buffer for the file from being output, since the processing never finishes. So, you get some error message on stderr, and no indication in the json what file it belongs to.
So, perhaps an easier fix would be to emit the json buffer in this case after the error message. Then stderr output, whether error or warning, would always precede the json for the same file.
The consumer would need to use select() over stdin+stderr to observe the order they were interleaved. May be too much to expect consumers to get that right. Oh, and still would not help untangle the stream when run with -J.
I think I'm tending toward adding a new --json-error-messages option, and making it add "error-messages:[]" to the json when there are no errors, thus keeping the json self-documenting.
The json buffer will need to be flushed in the exception handler, as noted in my previous comment.
FWIW
--json-error-messageswould be great to have.The consumer would need to use select() over stdin+stderr to observe the order they were interleaved.would not work for us atm without major refactoring (if possible at all without significant pains)Basic --json-error-messages implemented after 4 hours work.
Still needing to be done:
git annex infohave a custom json outputter, and may not output the error-messages field by default, or may not make sense to support the option.Testing, both of the cases in my previous message were actually already working as desired. I think it's done!