Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook
- From
Jeff King <peff@peff.net>
- Date
- Aug 25, 2009, 18:45 UTC
- Message-ID
- <20090825184525.GC23731@coredump.intra.peff.net>
- In-Reply-To
- <12c267e40908251043g4f3e36aya05d9c705f5afee2@mail.gmail.com>
On Tue, Aug 25, 2009 at 10:43:57AM -0700, Tom Werner wrote:
Show 14 quoted lines
> 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.
Show 6 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.
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