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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 25, 2009, 23:50 UTC
Message-ID: <7vprajmp16.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20090825184525.GC23731@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 11 quoted lines

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

Very good point; a bare repository does not have to have (and typically shouldn't have) the index, and a bare repository is what upload-pack typically serves.

A short-and-sweet:

	run_hook(NULL, "post-upload-pack",
        	 create_full_pack ? "clone" : "fetch,
                 NULL);

would be sufficient. Notice that run_hook() is variadic and its argument list needs to be terminated with NULL (iow, the original patch is buggy and risks reading random places on the stack---I would recommend against using it on your production site yet).

> 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 do not think that information is available. "want" will tell you what object they want, but that does not necessarily uniquey translate to a ref.

If we are allowed to talk about asking for the moon, and if one of the primary reason for this new hook is statistics, it would be useful to see the number of bytes given, where the fetch-pack came from, and if we are using git-daemon virtual hosting which of our domain served the request.