Jiang Xin <worldhello.net@xxxxxxxxx> writes:
> builtin/receive-pack.c | 93 +++++-
> t/t5411-proc-receive-hook.sh | 547 +++++++++++++++++++++++++++++++++++
> 2 files changed, 628 insertions(+), 12 deletions(-)
> create mode 100755 t/t5411-proc-receive-hook.sh
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 2cc18bbffd..23d0c224d2 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -312,7 +312,8 @@ struct command {
> struct command *next;
> const char *error_string;
> unsigned int skip_update:1,
> - did_not_exist:1;
> + did_not_exist:1,
> + have_special_ref:1;
> int index;
> struct object_id old_oid;
> struct object_id new_oid;
> @@ -669,6 +670,9 @@ static void prepare_push_cert_sha1(struct child_process *proc)
>
> struct receive_hook_feed_state {
> struct command *cmd;
> + int feed_normal_ref;
> + int feed_special_ref;
> + int hook_must_exist;
> int skip_broken;
> struct strbuf buf;
> const struct string_list *push_options;
> @@ -684,8 +688,14 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
> int code;
>
> argv[0] = find_hook(hook_name);
> - if (!argv[0])
> - return 0;
> + if (!argv[0]) {
> + if (feed_state->hook_must_exist) {
> + rp_error("cannot to find hook '%s'", hook_name);
> + return 1;
> + } else {
> + return 0;
> + }
> + }
>
> argv[1] = NULL;
Why do you even need "must-exist" in the first place? In other
parts of Git, when a hook does not exist, processing by that missing
hook is simply not done. Why does it have to cause an error?
If there is a reasonable answer to the above question, then the
concept that hooks can be marked as "must exist -- otherwise it is
an error" would be independently useful and the support for that
concept should be split out as a separate step before this one, I
would think.
Also, I am not sure why you have to have one single "special" ref
hierarchy to be handled by the hook. Why not feed everything and
let the hook decide?
I would have expected a design more along the lines of "if the hook
exists, then feed all the refs to the hook, let the hook do whatever
it wants and report what it did back to the receive-pack process, so
that the client side can learn what got accepted and what got
rejected (and possibly other things that may require protocol
extension later)". One of the possible things the hook can report
may be "I did not do anything for this ref, please do whatever you
normally do". That way, you do not need to introduce "there are two
classes of refs, normal and special"---that becomes an unused and
unnecessary concept. For whichever ref the hook considers "normal",
it can use the mechanism to say "I want you to do the normal thing".
And when it sees refs/for/ prefix (or whatever is considered
"special" at the hook's discretion), it can do whatever it wants,
and report "OK, I ate it" or "No, I reject it" (which are only the
two things that can be returned back to the client within the
current protocol, IIUC). We may later want to extend the mechanism
to allow richer response by the hook at the same time to extend the
on-the-wire protocol and then it may become possible for the hook to
take a push to refs/for/master, update refs/heads/master to the
commit (or to some other commit), and report "OK, I accepted it and
also updated refs/heads/master to this other commit".
