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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 19, 2020, 16:55 UTC
Message-ID: <xmqq7e0ih5zr.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <nycvar.QRO.7.76.6.2002191515310.46@tvgsbejvaqbjf.bet>

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 20 quoted lines

>> +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:

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

 - 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).

 - 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).

Use of "return !!fn();" that is not to give to exit(3) is probably more common in our codebase. See apply.c::try_create_file() and the comment before the function about its possible return values for example.