Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook
Junio C Hamano <gitster@pobox.com> writes:
Show 12 quoted lines
>> 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.
I briefly looked at upload-pack.c.
As I said, the names of the refs asked is not available but in that file, create_pack_file() should be able to gather transfer statistics (timeing and bytes transferred). It also has access to the want_obj[]/have_obj[] array, so it should be able to keep the rev-list parameters to be fed to the hook if it wanted to (and with that information you could recreate the exact pack data later if necessary).
And want_obj[]/have_obj[] information is much more useful than a crude "fetch/clone" distinction. You can not just tell if the clients have nothing, but how stale they are.
So at a minimum, if this is primarily meant as a statistics hook, I would suggest the hook not to take _any_ argument, but is fed the information from its command line in the following form, one piece of information per line, from the very beginning:
want 40-byte SHA-1 - what were in want_obj[] array
have 40-byte SHA-1 - what were in have_obj[] array
As long as the hook scripts are written to ignore lines they do not understand, in later rounds, we should also be able to feed two more pieces of information with minimum modification to create_pack_file():
time float - number of seconds create_pack_file spent,
size decimal - number of bytes transferred
Here is an illustration patch.
upload-pack.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 66 insertions(+), 2 deletions(-)
Show changes to upload-pack.c +66 −2
diff --git a/upload-pack.c b/upload-pack.c
index 4d8be83..69a6f46 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -141,8 +141,60 @@ static int do_rev_list(int fd, void *create_full_pack)
return 0;
}
+static int feed_obj_to_hook(const char *label, struct object_array *oa, int i, int fd)
+{
+ int cnt;
+ char buf[512];
+
+ cnt = sprintf(buf, "%s %s\n", label,
+ sha1_to_hex(oa->objects[i].item->sha1));
+ return write_in_full(fd, buf, cnt) != cnt;
+}
+
+static int run_post_upload_pack_hook(size_t total, struct timeval *tv)
+{
+ const char *argv[2];
+ struct child_process proc;
+ int err, i;
+ int cnt;
+ char buf[512];
+
+ argv[0] = "hooks/post-upload-pack";
+ argv[1] = NULL;
+
+ if (access(argv[0], X_OK) < 0)
+ return 0;
+
+ memset(&proc, 0, sizeof(proc));
+ proc.argv = argv;
+ proc.in = -1;
+ proc.stdout_to_stderr = 1;
+ err = start_command(&proc);
+ if (err)
+ return err;
+ for (i = 0; !err && i < want_obj.nr; i++)
+ err |= feed_obj_to_hook("want", &want_obj, i, proc.in);
+ for (i = 0; !err && i < have_obj.nr; i++)
+ err |= feed_obj_to_hook("have", &have_obj, i, proc.in);
+ if (!err) {
+ cnt = sprintf(buf, "time %ld.%06ld\n",
+ (long)tv->tv_sec, (long)tv->tv_usec);
+ err |= (write_in_full(proc.in, buf, cnt) != cnt);
+ }
+ if (!err) {
+ cnt = sprintf(buf, "size %ld\n", (long)total);
+ err |= (write_in_full(proc.in, buf, cnt) != cnt);
+ }
+ if (close(proc.in))
+ err = 1;
+ if (finish_command(&proc))
+ err = 1;
+ return err;
+}
+
static void create_pack_file(void)
{
+ struct timeval start_tv, tv;
struct async rev_list;
struct child_process pack_objects;
int create_full_pack = (nr_our_refs == want_obj.nr && !have_obj.nr);
@@ -150,10 +202,12 @@ static void create_pack_file(void)
char abort_msg[] = "aborting due to possible repository "
"corruption on the remote side.";
int buffered = -1;
- ssize_t sz;
+ ssize_t sz, total_sz;
const char *argv[10];
int arg = 0;
+ gettimeofday(&start_tv, NULL);
+ total_sz = 0;
if (shallow_nr) {
rev_list.proc = do_rev_list;
rev_list.data = 0;
@@ -262,7 +316,7 @@ static void create_pack_file(void)
sz = xread(pack_objects.out, cp,
sizeof(data) - outsz);
if (0 < sz)
- ;
+ total_sz += sz;
else if (sz == 0) {
close(pack_objects.out);
pack_objects.out = -1;
@@ -314,6 +368,16 @@ static void create_pack_file(void)
}
if (use_sideband)
packet_flush(1);
+
+ gettimeofday(&tv, NULL);
+ tv.tv_sec -= start_tv.tv_sec;
+ if (tv.tv_usec < start_tv.tv_usec) {
+ tv.tv_sec--;
+ tv.tv_usec += 1000000;
+ }
+ tv.tv_usec -= start_tv.tv_usec;
+ if (run_post_upload_pack_hook(total_sz, &tv))
+ warning("post-upload-hook failed");
return;
fail:
