Re: [PATCH v6 3/5] difftool: avoid returning -1 to cmd_main() from run_dir_diff()

[Junio C Hamano <gitster@pobox.com>]

David Aguilar <davvid@gmail.com> writes:

> difftool was forwarding the -1 result from error() to cmd_main(), which
> is implementation-defined since it is outside of the 0-255 range
> specified by POSIX for program exit codes.
>
> Stop assigning the result of error() to `ret`. Assign a value of 1
> whenever internal errors are detected instead.

Many existing codepaths take advantage of error() returning -1 and I
do not see anything is wrong to keep that "negative is error"
convention for the value of "ret" variable.  Most lines in this
patch however split that "ret = error(...)" pattern into two
statements and I do not see a very good reason for it.

Worse yet, after applying this patch, there still are at least three
assignments to "ret" that can give it a negative value:

	if (!mkdtemp(tmpdir.buf)) {
		ret = error("could not create '%s'", tmpdir.buf);
		goto finish;
	}

	ret = run_command_v_opt(helper_argv, flags);

	strbuf_addf(&buf, "%s/wtindex", tmpdir.buf);
	if (hold_lock_file_for_update(&lock, buf.buf, 0) < 0 ||
	    write_locked_index(&wtindex, &lock, COMMIT_LOCK)) {
		ret = error("could not write %s", buf.buf);
		goto finish;
	}

Among them, the return value from run_command_v_opt() eventually
come from wait_or_whine(), I think, so it may be generic -1 or
it may be WEXITSTATUS() of the child process.

But I am not sure if this partcular caller cares.  It is not
prepared to handle -1 and positive return from run_command_v_opt()
any differently.  So I think a single

-	return ret;
+	return !!ret;

at the end would be much easier to reason about and maintain.