Re: [PATCH] hook: introduce the report hook for git-receive-pack(1)

Karthik Nayak <[email protected]> writes:

> When running 'git-receive-pack(1)', there is currently no way for the
> server to intercept and modify the status report before it is sent back
> to the client. This is useful for servers with custom logic that need
> to transform or gate the report based on the outcome of external logic
> post reference updates.

One sentence is missing.  The fact that there is no way for the
server to customize the report is not useful, but that is how the
above reads.  Drop "currently", as the introductory observation is
always about the status, explaining what is missing to make
readers realize why they may want the new feature introduced by the
change.

> Introduce a new 'report' hook which receives the pkt-line encoded
> status report on stdin and whose stdout replaces the report sent to the
> client. A non-zero exit status causes `receive-pack` to die and the
> client to treat the push as failed.

After getting asked to accept a push to three refs and receiving
the object transfer, the hook can say "I'll let these two refs be
updated, but refuse to update the other one" and return success by
exiting 0.  What does the other side of the connection see?  Two
successes with one rejection, I guess.  If the hook instead rewrites
the report to say "all three ref updates were rejected" and returns
success, then what does the other side see?  Failures on all three
refs, right?

What should happen when the hook says "all three ref updates were
accepted and they updated to point at objects X, Y, Z", but the hook
itself exits with a non-zero status?  How does the other side tell
if their push succeeded (as described in the returned report) or
failed (as receive-pack(1) noticed the hook's exit status was not
0)?

How is the failure due to the hook's exit status propagated back to
the other side of the connection?  Does receive-pack(1) hold on to
the report until the hook dies, and if it dies with status 0 give
that report back to 'git push'?  And if it dies with a non-zero
status, then what?  Ignore the report and send a failure report
generated on its own?

The observation made in the preceding paragraphs shows that allowing
the exit status of the hook to further affect the outcome is a bit
iffy as a design to define what a "failure" is, unless it is more
tightly described.  The hook can signal failure in its report
output without exiting with a non-zero status at all, and if
receive-pack(1) wants to allow the exit code of the hook to affect
the outcome, it cannot stream the report back to 'git push' as it
receives it from the hook.

> @@ -2547,6 +2582,9 @@ static void report(struct command *commands, const char *unpack_status)
>  	}
>  	packet_buf_flush(&buf);
>  
> +	if (run_report_hook(&buf))
> +		die("report hook failed");
> +
>  	if (use_sideband)
>  		send_sideband(1, 1, buf.buf, buf.len, use_sideband);
>  	else
> @@ -2592,6 +2630,9 @@ static void report_v2(struct command *commands, const char *unpack_status)
>  	}
>  	packet_buf_flush(&buf);
>  
> +	if (run_report_hook(&buf))
> +		die("report hook failed");
> +
>  	if (use_sideband)
>  		send_sideband(1, 1, buf.buf, buf.len, use_sideband);
>  	else

OK.  The other side does not even hear the report if the hook
aborts.  And lack of success report is what the other side
interprets as a failure.

Ugly, but may work OK.  Needs to be documented a bit more clearly,
though.

Thanks.