Re: [PATCH v3] refs: implement reference transaction hook

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 18, 2020, 22:23 UTC
Message-ID: <xmqqo8pgko8k.fsf@gitster.c.googlers.com>
In-Reply-To: <1de96b96e3448c8f7e7974f7c082fd08d2d14e96.1592475610.git.ps@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

> +static const char hook_not_found;
> +static const char *hook;

;-)  Nice.

Show 33 quoted lines

> +static int run_transaction_hook(struct ref_transaction *transaction,
> +				const char *state)
> +{
> +	struct child_process proc = CHILD_PROCESS_INIT;
> +	struct strbuf buf = STRBUF_INIT;
> +	int saved_errno = 0, ret, i;
> +...
> +	ret = start_command(&proc);
> +	if (ret)
> +		return ret;
> +
> +	sigchain_push(SIGPIPE, SIG_IGN);
> +
> +	for (i = 0; i < transaction->nr; i++) {
> +		struct ref_update *update = transaction->updates[i];
> + ...
> +		if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
> +			if (errno != EPIPE)
> +				saved_errno = errno;
> +			break;
> +		}
> +	}
> +
> +	close(proc.in);
> +	sigchain_pop(SIGPIPE);
> +	strbuf_release(&buf);
> +
> +	ret = finish_command(&proc);
> +	if (ret)
> +		return ret;
> +
> +	return saved_errno;
> +}

OK, the only thing that looked a bit tricky was the "saved_errno" that is used in an unusual (relative to its name) way. The use isn't incorrect per-se, but it rubs readers' expectation the wrong way to use the variable named saved_errno for any purpose other than the established pattern:

	saved_errno = errno;
	if (some_libcall_that_may_update_errno()) {
		... deal with an error and perform
		... some fallback action
	}
	errno = saved_errno;

that allows the caller to be oblivious to the library call that is made as a mere implementation detail whose failure does not matter to the caller.

In any case, the idea of the code in the patch is to make sure we remember the fact that we failed to write (or caught any other error, if we added more calls in the future) in a variable, and make sure we return an error even if we manage to cleanly call "finish_command()". For that purpose, in order to avoid overwriting the "ret" variable with the returned value from finish_command(), a separate variable is needed, and "saved_errno" was picked for the name of the variable.

But I do not think it is a good idea to return the errno in one codepath when the function can return an error status that is not an errno that is received from start_command() and finish_command(). The caller of this function cannot (and probably do not want to) tell what the failed syscall was and would be checking if the return value was success (=0) or failure (<0).

So I'd rather simplify the error handling to

 - Remove "saved_errno"; instead initialize ret to 0 at the beginning;

 - Return "ret" even if we return hardcoded 0 in the earlier part
   for consistency;

 - Update "ret" in the loop to -1 (the same error return status that
   is returned by start_command() and finish_command()) when we
   found a write error that we are not ignoring before breaking out
   of the loop.

 - We need to call finish_command() even if we earlier saw an error
   once we successfully called start_command().  So do something
   like this:

	ret |= finish_command(&proc);
	return ret;

   to make sure we retain an earlier error in "ret", we
   unconditionally call finish_command() when the control reaches
   there, and we mark the result a failure when finish_command()
   fails.

if I were writing this function.

Thanks.