Fossil Forum

fossil error status, is there a policy?
Login

fossil error status, is there a policy?

(1) By Larry Brasfield (larrybr) on 2021-02-22 21:22:18 [link] [source]

I recently had reason to use Fossil's sha3sum subcommand and was surprised to find that its error reporting is less useful than is normal for utilities inspired by the Unix way and Fossil itself with other subcommands.

In particular, instead of returning a non-zero exit status when it cannot do what was commanded, it just returns the "success" status, 0. And instead of emitting complaints to stderr, it just mixes them with normal output to stdout. This defeats the main purpose of having a stderr stream, which is to allow exceptional and/or error output to show up on a terminal (or in separately collected file) even when stdout is being piped or otherwise redirected. And of course, non-zero exit status is important for error detection and helps avoid the GIGO problem for subsequent output consumers (who are not humans gazing at a screen.)

This makes me wonder if Fossil's authors are of one mind with respect to the virtue of following the well accepted error reporting conventions. Below is a screen scrape showing how the changes subcommand does follow them while the sha3sum subcommand ignores them.

Ignoring: > fo sha3sum lcb_path.txt && echo Success e7e4ea6b7a4f8e026f61553da8aebcfbb133074ef873b797ef2ab21735c2a356 lcb_path.txt Success

> fo sha3sum lcb_path.huh && echo Success ************** not found *************** lcb_path.huh Success

> fo sha3sum lcb_path.huh && echo Success 2> nul ************** not found *************** lcb_path.huh Success

> fo sha3sum lcb_path.txt && echo Success 2> nul e7e4ea6b7a4f8e026f61553da8aebcfbb133074ef873b797ef2ab21735c2a356 lcb_path.txt Success

Fossil as the good citizen: > fo changes current directory is not within an open checkout

> fo changes && echo Success current directory is not within an open checkout

> fo changes 2> nul && echo Success

> pushd C:WorkProjectsSqliteSources && (fo changes && echo Success) popd

EDITED tool/lemon.c ... Success

>

I would call that former non-script-friendly reporting a bug if Fossil had a policy of following the usual conventions.

Yes, I realize that I could pattern match that "...** not found **..." complaint. That's not the point.

(2) By Stephan Beal (stephan) on 2021-02-23 00:04:42 in reply to 1 [link] [source]

This makes me wonder if Fossil's authors are of one mind with respect to the virtue of following the well accepted error reporting conventions.

We're not a highly-organized team with tight coordination and a feature roadmap, but a ragtag collective of volunteers organically scratching their own itches and, sometimes, the itches of others. (That's not nearly as unhygienic as it may sound.)

IIRC, fossil doesn't currently have APIs for emitting non-fatal messages to anywhere other than stdout, and we've never had any pressing need for it. (Whether fatal errors go to stdout or stderr i can't say off hand - it's never been relevant for my purposes.) Fossil is, IMO, generally considered by those who work on it to be an interactive tool, not a scripting tool, and we've warned countless times about relying on any given output for scripting purposes (possibly in part because we are not of one mind when it comes to structuring output and the next person working on it might do it differently).

The exit codes could certainly be improved but that would have no real benefit for the primary use - interactive. Patches are happily considered.

(3) By Larry Brasfield (larrybr) on 2021-02-23 00:13:17 in reply to 2 [link] [source]

Hi, Stephan.

I hope there was no disdain taken from my comment about Fossil's authors.

My use case is specifically one where Fossil is used from a script. There are others from time to time. I did notice that the 'changes' subcommand does return proper exit status and does make appropriate use of stderr. So that leaves me with hope that this might be easily fixed.

Expect to see a patch, or a branch push suggestion, soon. If Fossil was not such a great tool (set) already, I would be more sanguine about its foibles.

(4) By Stephan Beal (stephan) on 2021-02-23 00:21:29 in reply to 2 [link] [source]

IIRC, fossil doesn't currently have APIs for emitting non-fatal messages to anywhere other than stdout,

After checking: that's not entirely true, but fossil_warning() comes with this (as it were) warning:

/* Print a warning message.
**
** Unlike fossil_fatal() and fossil_panic(), this routine does return
** and processing attempts to continue.  A message is written to the
** error log, however, so this routine should only be used for situations
** that require administrator or developer attention.  Minor problems
** in user inputs should not use this routine.
*/

(5) By Stephan Beal (stephan) on 2021-02-23 00:25:32 in reply to 3 [link] [source]

I hope there was no disdain taken from my comment about Fossil's authors

Not from me, i was just stressing out that we're not an organized troupe and certainly has side effects in how certain things get implemented. Any given command works (at least initially) the way it does because the one who initially implemented it wrote it that way. The most-used commands tend to get refined via group feedback, but seldom-used ones often stay in their original form (sha3sum being an apparent example).

Expect to see a patch, or a branch push suggestion, soon.

That's probably our second-favorite response, right behind "the patch/branch is here..." :-D.

(6.1) By Larry Brasfield (larrybr) on 2021-02-23 02:47:15 edited from 6.0 in reply to 5 [source]

Here is the patch. I've tested modestly on Windows, to be sure it still does what it used to for found (and only found) files. This change causes the sha3sum subcommand to exit with exit status 1 and issue a complaint to stderr when a specified file cannot be read. This conceivably breaks consumers who depend on 0 exit status always (sad) or feed filenames to "fossil sha3sum" expecting to filter "...*** not found ***..." on stdout to determine which "hashes" to ignore. (yuk)

If "good" filenames precede the non-filename which produces a croak, their hash outputs are as usual. It might be an enhancement to just emit complaints (to stderr) and keep going through the file list, then return error exit status at the end, easily done with fossil_print_error(rc, z) and fossil_exit(rc).

It would be good, perhaps, to make the blob_init call with an empty string instead of all those wasted splats. I went for a minimal change here.

Index: src/sha3.c ================================================================== --- src/sha3.c +++ src/sha3.c @@ -670,11 +670,13 @@ blob_init(&cksum, "************** not found ***************", -1); if( g.argv[i][0]=='-' && g.argv[i][1]==0 ){ blob_read_from_channel(&in, stdin, -1); sha3sum_blob(&in, iSize, &cksum); }else{ - sha3sum_file(g.argv[i], eFType, iSize, &cksum); + if (sha3sum_file(g.argv[i], eFType, iSize, &cksum) > 0){ + fossil_fatal("Cannot open and read file %s", g.argv[i]); + } } fossil_print("%s %sn", blob_str(&cksum), g.argv[i]); blob_reset(&cksum); } }

Below is a patch with a more consistent treatment of found files (emitting hashes for them all.) It took another 12 lines of code. It complains about all the missing files. This is more what I prefer rather than finding errors one at a time.

Index: src/sha3.c ================================================================== --- src/sha3.c +++ src/sha3.c @@ -645,11 +645,13 @@ void sha3sum_test(void){ int i; Blob in; Blob cksum; int iSize = 256; + int iErrors = 0; int eFType = SymFILE; + const char * mlead = "Error: sha3sum cannot read: ";

if( find_option("dereference","h",0) ) eFType = ExtFILE; if( find_option("224",0,0)!=0 ) iSize = 224; else if( find_option("256",0,0)!=0 ) iSize = 256; else if( find_option("384",0,0)!=0 ) iSize = 384; @@ -665,16 +667,29 @@ } } verify_all_options();

for(i=2; i<g.argc; i++){ - blob_init(&cksum, "************** not found ***************", -1); + int iBad = 0; + blob_init(&cksum, "", -1); if( g.argv[i][0]=='-' && g.argv[i][1]==0 ){ blob_read_from_channel(&in, stdin, -1); sha3sum_blob(&in, iSize, &cksum); }else{ - sha3sum_file(g.argv[i], eFType, iSize, &cksum); + if (sha3sum_file(g.argv[i], eFType, iSize, &cksum) > 0){ + fossil_puts(mlead, 1); + mlead = ", "; + fossil_puts(g.argv[i], 1); + ++iErrors; + iBad = 1; + } } - fossil_print("%s %sn", blob_str(&cksum), g.argv[i]); + if (!iBad){ + fossil_print("%s %sn", blob_str(&cksum), g.argv[i]); + } blob_reset(&cksum); } + + if (iErrors > 0){ + fossil_exit(1); + } }

(7) By Stephan Beal (stephan) on 2021-02-23 02:49:36 in reply to 6.0 [link] [source]

It would be good, perhaps, to make the blob_init call with an empty string instead of all those wasted splats. I went for a minimal change here.

A very slight modification of your patch, with the "not found" bit removed, is now committed: fossil:bda90774eea87f7c.

Let me know if a more consistent treatment of found files (emitting hashes for them all) would be preferable.

That's a question of taste which i'll defer to someone who has some :).