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

After upload-pack successfully finishes its operation, post-upload-pack
hook can be called for logging purposes.

The hook is passed various pieces of information, one per line, from its
standard input.  Currently the following items can be fed to the hook, but
more types of information may be added in the future:

    want SHA-1::
        40-byte hexadecimal object name the client asked to include in the
        resulting pack.  Can occur one or more times in the input.

    have SHA-1::
        40-byte hexadecimal object name the client asked to exclude from
        the resulting pack, claiming to have them already.  Can occur zero
        or more times in the input.

    time float::
        Number of seconds spent for creating the packfile.

    size decimal::
        Size of the resulting packfile in bytes.

Signed-off-by: Junio C Hamano <redacted>
---

 > Here is an illustration patch.

 And here is a bit more polished one with necessary supporting material.

 Documentation/git-upload-pack.txt |    2 +
 Documentation/githooks.txt        |   25 +++++++++++++
 t/t5501-post-upload-pack.sh       |   49 ++++++++++++++++++++++++++
 upload-pack.c                     |   68 +++++++++++++++++++++++++++++++++++-
 4 files changed, 142 insertions(+), 2 deletions(-)
 create mode 100755 t/t5501-post-upload-pack.sh

diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt
index b8e49dc..63f3b5c 100644
--- a/Documentation/git-upload-pack.txt
+++ b/Documentation/git-upload-pack.txt

@@ -20,6 +20,8 @@ The UI for the protocol is on the 'git-fetch-pack' side, and the
 program pair is meant to be used to pull updates from a remote
 repository.  For push operations, see 'git-send-pack'.
 
+After finishing the operation successfully, `post-upload-pack`
+hook is called (see linkgit:githooks[5]).
 
 OPTIONS
 -------

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 1c73673..036f6c7 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt

@@ -307,6 +307,31 @@ Both standard output and standard error output are forwarded to
 'git-send-pack' on the other end, so you can simply `echo` messages
 for the user.
 
+post-upload-pack
+----------------
+
+After upload-pack successfully finishes its operation, this hook is called
+for logging purposes.
+
+The hook is passed various pieces of information, one per line, from its
+standard input.  Currently the following items can be fed to the hook, but
+more types of information may be added in the future:
+
+want SHA-1::
+    40-byte hexadecimal object name the client asked to include in the
+    resulting pack.  Can occur one or more times in the input.
+
+have SHA-1::
+    40-byte hexadecimal object name the client asked to exclude from
+    the resulting pack, claiming to have them already.  Can occur zero
+    or more times in the input.
+
+time float::
+    Number of seconds spent for creating the packfile.
+
+size decimal::
+    Size of the resulting packfile in bytes.
+
 pre-auto-gc
 -----------
 

diff --git a/t/t5501-post-upload-pack.sh b/t/t5501-post-upload-pack.sh
new file mode 100755
index 0000000..2cb63f8
--- /dev/null
+++ b/t/t5501-post-upload-pack.sh

@@ -0,0 +1,49 @@
+#!/bin/sh
+
+test_description='post upload-hook'
+
+. ./test-lib.sh
+
+LOGFILE=".git/post-upload-pack-log"
+
+test_expect_success setup '
+	test_commit A &&
+	test_commit B &&
+	git reset --hard A &&
+	test_commit C &&
+	git branch prev B &&
+	mkdir -p .git/hooks &&
+	{
+		echo "#!$SHELL_PATH" &&
+		echo "cat >post-upload-pack-log"
+	} >".git/hooks/post-upload-pack" &&
+	chmod +x .git/hooks/post-upload-pack
+'
+
+: test_expect_success initial '
+	rm -fr sub &&
+	git init sub &&
+	(
+		cd sub &&
+		git fetch --no-tags .. prev
+	) &&
+	want=$(sed -n "s/^want //p" "$LOGFILE") &&
+	test "$want" = "$(git rev-parse --verify B)" &&
+	! grep "^have " "$LOGFILE"
+'
+
+test_expect_success second '
+	rm -fr sub &&
+	git init sub &&
+	(
+		cd sub &&
+		git fetch --no-tags .. prev:refs/remotes/prev &&
+		git fetch --no-tags .. master
+	) &&
+	want=$(sed -n "s/^want //p" "$LOGFILE") &&
+	test "$want" = "$(git rev-parse --verify C)" &&
+	have=$(sed -n "s/^have //p" "$LOGFILE") &&
+	test "$have" = "$(git rev-parse --verify B)"
+'
+
+test_done

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:

-- 
1.6.4.1.288.g10d22

On Thu, Aug 27, 2009 at 2:47 AM, Junio C Hamano[off-list ref] wrote:

After upload-pack successfully finishes its operation, post-upload-pack
hook can be called for logging purposes.

The hook is passed various pieces of information, one per line, from its
standard input.  Currently the following items can be fed to the hook, but
more types of information may be added in the future:

   want SHA-1::
       40-byte hexadecimal object name the client asked to include in the
       resulting pack.  Can occur one or more times in the input.

   have SHA-1::
       40-byte hexadecimal object name the client asked to exclude from
       the resulting pack, claiming to have them already.  Can occur zero
       or more times in the input.

   time float::
       Number of seconds spent for creating the packfile.

   size decimal::
       Size of the resulting packfile in bytes.

Neat. And feeding it lines gives more room for future additions.

I'd like to suggest the following line from the original patch:

   full-pack integer::
        1 if the request was considered a full clone, 0 if it was a
partial update (fetch)


Also, on a similar note; in the little git-daemon (a tiny fork+exec
server in ruby) included with Gitorious there's a geo-ip lookup based
on the client addr. It would be fun if the client ip could be passed
along to this hook as well, but that would require passing it along
all the way from before fetch-pack is invoked as far as I can see..?

JS

Johan Sorensen [off-list ref] writes:

On Thu, Aug 27, 2009 at 2:47 AM, Junio C Hamano [off-list ref] wrote:

quoted

After upload-pack successfully finishes its operation, post-upload-pack
hook can be called for logging purposes.

The hook is passed various pieces of information, one per line, from its
standard input.  Currently the following items can be fed to the hook, but
more types of information may be added in the future:

   want SHA-1::
       40-byte hexadecimal object name the client asked to include in the
       resulting pack.  Can occur one or more times in the input.

   have SHA-1::
       40-byte hexadecimal object name the client asked to exclude from
       the resulting pack, claiming to have them already.  Can occur zero
       or more times in the input.

   time float::
       Number of seconds spent for creating the packfile.

   size decimal::
       Size of the resulting packfile in bytes.

Neat. And feeding it lines gives more room for future additions.

I'd like to suggest the following line from the original patch:

   full-pack integer::
        1 if the request was considered a full clone, 0 if it was a
partial update (fetch)

 
If it is all "want" and no "have", it is clone or fetch into empty
repository.  If additionaly "want"s cover all refs, it is a clone.
No need to pass this information: it can be derived.

Also, on a similar note; in the little git-daemon (a tiny fork+exec
server in ruby) included with Gitorious there's a geo-ip lookup based
on the client addr. It would be fun if the client ip could be passed
along to this hook as well, but that would require passing it along
all the way from before fetch-pack is invoked as far as I can see..?

Well, we can pass at least `client-ip`...

[please don't quote what is not needed]
-- 
Jakub Narebski
Poland
ShadeHawk on #git

On Wed, Aug 26, 2009 at 05:47:41PM -0700, Junio C Hamano wrote:

After upload-pack successfully finishes its operation, post-upload-pack
hook can be called for logging purposes.

+1 from me.

I'm actually going to scrap my previous pre-upload-pack hook and rebase
it off this, because I see a lot of commonality (I was passing in
have/want via stdin too).

-- 
Robin Hugh Johnson
Gentoo Linux: Developer, Trustee & Infrastructure Lead
E-Mail     : robbat2@gentoo.org
GnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85

Jakub Narebski [off-list ref] writes:

quoted

I'd like to suggest the following line from the original patch:

   full-pack integer::
        1 if the request was considered a full clone, 0 if it was a
partial update (fetch)

 
If it is all "want" and no "have", it is clone or fetch into empty
repository.  If additionaly "want"s cover all refs, it is a clone.
No need to pass this information: it can be derived.

Well, not exactly.

Here is an iffy RFC patch.  Iffy not in the sense that its implementation
is questionable, but in the sense that I am not really convinced if the
distinction between fetching some (or in the worst case, most) but not all
refs, and fetching full set of refs, into an empty repository is something
worth making.

Does anybody from GitHub have any input?  Is there something that can
still improved to suit GitHub's needs?

-- >8 --
Subject: [PATCH] upload-pack: feed "kind [clone|fetch]" to post-upload-pack hook

A request to clone the repository does not give any "have" but asks for
all the refs we offer with "want".  When a request does not ask to clone
the repository fully, but asks to fetch some refs into an empty
repository, it will not give any "have" but its "want" won't ask for all
the refs we offer.

If we suppose (and I would say this is a rather big if) that it makes
sense to distinguish these two cases, a hook cannot reliably do this
alone.  The hook can detect lack of "have" and bunch of "want", but there
is no direct way to tell if the other end asked for all refs we offered,
or merely most of them.

Between the time we talked with the other end and the time the hook got
called, we may have acquired more refs or lost some refs in the repository
by concurrent operations.  Given that we plan to introduce selective
advertisement of refs with a protocol extension, it would become even more
difficult for hooks to guess between these two cases.

This adds "kind [clone|fetch]" to hook's input, as a stable interface to
allow the hooks to tell these cases apart.

Signed-off-by: Junio C Hamano <redacted>
---
 Documentation/githooks.txt  |    4 ++++
 t/t5501-post-upload-pack.sh |   24 ++++++++++++++++++++++--
 upload-pack.c               |    4 ++++
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt
index 036f6c7..c308d29 100644
--- a/Documentation/githooks.txt
+++ b/Documentation/githooks.txt

@@ -332,6 +332,10 @@ time float::
 size decimal::
     Size of the resulting packfile in bytes.
 
+kind string:
+    Either "clone" (when the client did not give us any "have", and asked
+    for all our refs with "want"), or "fetch" (otherwise).
+
 pre-auto-gc
 -----------
 

diff --git a/t/t5501-post-upload-pack.sh b/t/t5501-post-upload-pack.sh
index 47ee7b5..d89fb51 100755
--- a/t/t5501-post-upload-pack.sh
+++ b/t/t5501-post-upload-pack.sh

@@ -29,7 +29,9 @@ test_expect_success initial '
 	) &&
 	want=$(sed -n "s/^want //p" "$LOGFILE") &&
 	test "$want" = "$(git rev-parse --verify B)" &&
-	! grep "^have " "$LOGFILE"
+	! grep "^have " "$LOGFILE" &&
+	kind=$(sed -n "s/^kind //p" "$LOGFILE") &&
+	test "$kind" = fetch
 '
 
 test_expect_success second '

@@ -43,7 +45,25 @@ test_expect_success second '
 	want=$(sed -n "s/^want //p" "$LOGFILE") &&
 	test "$want" = "$(git rev-parse --verify C)" &&
 	have=$(sed -n "s/^have //p" "$LOGFILE") &&
-	test "$have" = "$(git rev-parse --verify B)"
+	test "$have" = "$(git rev-parse --verify B)" &&
+	kind=$(sed -n "s/^kind //p" "$LOGFILE") &&
+	test "$kind" = fetch
+'
+
+test_expect_success all '
+	rm -fr sub &&
+	HERE=$(pwd) &&
+	git init sub &&
+	(
+		cd sub &&
+		git clone "file://$HERE/.git" new
+	) &&
+	sed -n "s/^want //p" "$LOGFILE" | sort >actual &&
+	git rev-parse A B C | sort >expect &&
+	test_cmp expect actual &&
+	! grep "^have " "$LOGFILE" &&
+	kind=$(sed -n "s/^kind //p" "$LOGFILE") &&
+	test "$kind" = clone
 '
 
 test_done

diff --git a/upload-pack.c b/upload-pack.c
index 857440d..8e82179 100644
--- a/upload-pack.c
+++ b/upload-pack.c

@@ -187,6 +187,10 @@ static int run_post_upload_pack_hook(size_t total, struct timeval *tv)
 					(long)tv->tv_sec, (long)tv->tv_usec);
 	if (!err)
 		err |= feed_msg_to_hook(proc.in, "size %ld\n", (long)total);
+	if (!err)
+		err |= feed_msg_to_hook(proc.in, "kind %s\n",
+					(nr_our_refs == want_obj.nr && !have_obj.nr)
+					? "clone" : "fetch");
 	if (close(proc.in))
 		err = 1;
 	if (finish_command(&proc))

-- 
1.6.4.1.307.g70e9f

On Fri, Aug 28, 2009 at 11:17 PM, Junio C Hamano[off-list ref] wrote:

Jakub Narebski [off-list ref] writes:

quoted

quoted

I'd like to suggest the following line from the original patch:

   full-pack integer::
        1 if the request was considered a full clone, 0 if it was a
partial update (fetch)

If it is all "want" and no "have", it is clone or fetch into empty
repository.  If additionaly "want"s cover all refs, it is a clone.
No need to pass this information: it can be derived.

Well, not exactly.

Here is an iffy RFC patch.  Iffy not in the sense that its implementation
is questionable, but in the sense that I am not really convinced if the
distinction between fetching some (or in the worst case, most) but not all
refs, and fetching full set of refs, into an empty repository is something
worth making.

Does anybody from GitHub have any input?  Is there something that can
still improved to suit GitHub's needs?

From GitHub's perspective, we'd treat any clone or fetch into an empty
repo as a clone operation, whether or not that included all of the
refs that were available. For us, the distinction between full and
partial clones is too nuanced to warrant additional code. I'd be happy
with the previous incarnation of the post-upload-pack that simply
sends the HAVEs and WANTs.

Tom

Tom Werner [off-list ref] writes:

... I'd be happy
with the previous incarnation of the post-upload-pack that simply
sends the HAVEs and WANTs.

Thanks.  This settles the biggest worry I had.

The worry was not about "do we feed clone/fetch info?", but was about
getting a complaint "Now, these changes to feed info from standard input
does not help anybody but forces us to update our hooks for no good
reason" from you guys ;-).