Re: [PATCH] Added hook in git-receive-pack

[Junio C Hamano <junkio@cox.net>]

Josef Weidendorfer <Josef.Weidendorfer@gmx.de> writes:

> +It is assured that sha1-old is an ancestor of sha1-new (otherwise,
> +the update would have not been allowed). refname is relative to
> +$GIT_DIR; e.g. for the master head this is "refs/heads/master".

I think this description is inaccurate; the send-pack can be run
with the --force flag and it is my understanding that receiver
would happily rewind the branch.  One possibility, if we wanted
to enforce it on the receiver end, would be to add another hook
that is called before the rename happens and tell the
receive-pack to refuse that update, but that should be done with
a separate patch, I suppose.

> +Using this hook, it is easy to generate mails on updates to
> +the local repository. This example script sends a mail with
> +the commits pushed to the repository:
> +
> +       #!/bin/sh
> +       git-rev-list --pretty "$3" "^$2" |
> +        mail -r $USER -s "New commits on $1" commit-list@mydomain

What is the environment the hook runs in?  For example, who
defines $USER used here?

We might want to describe the environment a bit more tightly
than the current patch does.  This includes not just the
environment variables, but $cwd and the set of open file
descriptors among other things.

I am not saying this from the security standpoint (the fact that
you can invoke receive-pack and that you can write into the
update hooks means you already have control over that
repository), but to help hook writers to avoid making mistakes.
For example, I offhand cannot tell what happens if the hook
tries to read from its standard input.  Also what happens if the
hook does not return but sleeps forever in a loop?  Do we want
to somehow time it out?  I think "It is hooks' responsibility to
time itself out" is an acceptable answer here, but if that is
the case it had better be documented.

> +static void updatehook(const char *name, unsigned char *old_sha1, unsigned char *new_sha1)
> +{
> +        if (access(update_hook, X_OK) < 0) return;
> +       fprintf(stderr, "executing update hook for %s\n", name);
> +...
> +}

I think I've seen this "fork -- exec -- for loop with waitpid"
pattern repeated number of times in the code.  Would it be
feasible to make them into a single library-ish function and
call it from here and other existing places?

Another thing you may want to consider is to do this hook
processing before and/or after processing all the refs.  A hook
might want to know what the entire set of refs are that are
being updated, and may not have enough information if it is
invoked once per ref.

Thanks for the patch; I agree with what the patch tries to
achieve in general.

-jc