Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook

[Jeff King <peff@peff.net>]

On Tue, Aug 25, 2009 at 10:43:57AM -0700, Tom Werner wrote:

> On Tue, Aug 18, 2009 at 12:04 AM, Tom Preston-Werner<tom@mojombo.com> wrote:
> > A post-upload-pack hook is desirable for Git hosts that need to
> > collect statistics on how many clones and/or fetches are made
> > on each repository.
> >
> > The hook is called with either "clone" or "fetch" as the only
> > argument, depending on whether a full pack file was sent to the
> > client or not.
> 
> I was hoping to get some feedback on this patch, either positive or
> negative. Since we'll be applying this patch for our use of the Git
> Daemon on GitHub, it would be great to see it in core, so we don't
> have to maintain custom debian builds forever. I'd imagine that other
> Git hosting sites would find this hook useful as well. Thanks!

I expect it didn't get any response because nobody here cared one way or
the other. Not too surprising, since I think not many people are running
a GitHub-sized hosting site that cares about such statistics. ;) So I
think following up as you are doing is the right thing.

As for the hook itself, the concept certainly seems sane to me. It
passes the "hook" test defined here:

  http://thread.gmane.org/gmane.comp.version-control.git/70781/focus=71069

because it is a remote trigger.

But a few comments on the patch:

> ---
>  upload-pack.c |    9 +++++++++
>  1 files changed, 9 insertions(+), 0 deletions(-)

It needs at least a mention in Documentation/githooks.txt.

> +static void run_post_upload_pack_hook(int create_full_pack)
> +{
> +	const char *fetch_type;
> +	fetch_type = (create_full_pack) ? "clone" : "fetch";
> +	run_hook(get_index_file(), "post-upload-pack", fetch_type);
> +}

Does it really need an index file? This operation in question seems to
be totally disconnected from the index (and indeed, most bare
repositories won't even have one). Probably it should pass NULL as the
initial argument to run_hook.

Is there any other information that might be useful to other non-GitHub
users of the hook? The only thing I can think of is the list of refs
that were fetched. I don't want to over-engineer it, but nor do I want
to be left with the mess of retro-fitting more information onto an
existing hook later. Maybe others can comment on whether they would find
more information useful.

-Peff