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

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

Patrick Steinhardt <ps@pks.im> writes:

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

;-)  Nice.

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