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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 30, 2021, 22:06 UTC
Message-ID: <xmqq7dexafch.fsf@gitster.g>
In-Reply-To: <20210930170146.61489-3-davvid@gmail.com>

David Aguilar <davvid@gmail.com> writes:

Show 6 quoted lines

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