Re: [PATCH v7 03/15] bugreport: add tool to generate debugging info

[Emily Shaffer <emilyshaffer@google.com>]

On Wed, Feb 19, 2020 at 08:55:04AM -0800, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> +int cmd_main(int argc, const char **argv)
> >> +{
> >> +...
> >> +	if (report == NULL) {
> >> +		strbuf_release(&report_path);
> >> +		die("couldn't open '%s' for writing", report_path.buf);
> >> +	}
> >> +
> >> +	strbuf_write(&buffer, report);
> >> +	fclose(report);
> >> +
> >> +	fprintf(stderr, _("Created new report at '%s'.\n"), report_path.buf);
> >> +
> >> +	UNLEAK(buffer);
> >> +	UNLEAK(report_path);
> >> +	return -launch_editor(report_path.buf, NULL, NULL);
> >
> > This would be the first time (at least that _I_ know of) that we use `-`
> > in this way. We seem to use `!!` a lot more often. And now I wonder
> > whether there is a reason for that `-` that I missed?
> 
> In general, our preferred way to report an error from API functions
> is to return a negative number and a success is reported by
> returning zero.
> 
> The argument to exit(3), which the return value from the main()
> function essentially is, on the other hand, is expected to be a
> small non-negative integer.  As long as we are in tight control of
> the range of the returned value from launch_editor() (i.e. it must
> return a small non-positive integer whose negation is suitable to be
> fed to exit(3)), the above is fine.
> 
> The idiom "return !!fn();" is to canonicalize the value to 0 or 1
> for the caller who asked "so, did fn() give us 0 or non-zero?",
> which can also be used as the value given to exit(3), and could be
> more appropriate under some conditions:

In editor.c, launch_editor() returns launch_specified_editor() without
altering the return code.

> 
>  - we MUST know launch_editor() returns zero if and only if it is
>    successful.

launch_specified_editor() has a handful of exit points, of three kinds:
 1. return error(something)
 2. raise(sigsomething)
 3. return 0
    a. when the editor process closed happily, but the user supplied
       NULL instead of a buffer. That is, the user didn't want the
       contents of the editor given back to them in a strbuf.
    b. when the editor process closed happily and the user's supplied
       buffer was filled with the file's contents with no issue.

So I think we can check "yes" here.

> 
>  - we do not have to be confident to be in tight control of the
>    range of the returned value from launch_editor() (e.g. it could
>    return a positive upon an error).

According to 'man 3 raise' (POSIX), raise() exits the current thread. If it
can't exit itself(?) it seems it returns "nonzero for failure". We've
also got a mingw_raise() in compat/mingw.h which could be used instead;
this one seems to return -1 or the result of raise(), presumably the one
from the MSC runtime[1], which also returns "a nonzero value" if not
successful.

So it's true that we aren't confident that launch_editor() returns a
positive or negative value.

>  - we MUST NOT care to differenciate different error codes returned
>    from launch_editor().  IOW, we must be fine to give the invoker
>    of the program only 0 (success) or 1 (unspecified failure).

This part is a little sticking point for me; I think I'd rather give the
user some hint of what's broken than no hint at all, especially in this
scenario, where it's conceivable someone could say "I tried to run
'git-bugreport' and it crashed, here is the return code and I have
manually collected some relevant info too".

The uncertainty coming from raise() (POSIX and MSCR both) leads me to
believe if I do want to return the error on exit, I should at least
check the sign before I flip it. Anybody else have opinions?

 - Emily

[1] https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/raise?view=vs-2019