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

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

[Tom Preston-Werner]

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.

Signed-off-by: Tom Preston-Werner <tom@mojombo.com>
---
 upload-pack.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index f7d308a..96231dc 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -141,6 +141,13 @@ static int do_rev_list(int fd, void *create_full_pack)
 	return 0;
 }
 
+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);
+}
+
 static void create_pack_file(void)
 {
 	struct async rev_list;
@@ -314,6 +321,8 @@ static void create_pack_file(void)
 	}
 	if (use_sideband)
 		packet_flush(1);
+
+	run_post_upload_pack_hook(create_full_pack);
 	return;
 
  fail:
-- 
1.6.3.1

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

[Tom Werner]

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!

Tom

--
Tom Preston-Werner
GitHub Cofounder
http://tom.preston-werner.com
github.com/mojombo

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Johannes Schindelin]

Hi,

On Tue, 25 Aug 2009, Junio C Hamano wrote:

> If we are allowed to talk about asking for the moon,

How about

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

then?

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

Certainly those are possible add-on patches, but would you require them to 
be in the same commit?

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hi,
>
> On Tue, 25 Aug 2009, Junio C Hamano wrote:
>
>> If we are allowed to talk about asking for the moon,
>
> How about
>
>  	run_hook(NULL, "post-upload-pack",
>          	 create_full_pack ? "clone" : "fetch,
> 		 "the moon",
>                  NULL);
>
> then?
>
>> 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.
>
> Certainly those are possible add-on patches, but would you require them to 
> be in the same commit?

I was merely responding to the "what else would be useful" question posed
by Peff.

Did you get an impression that I was saying "you must add these otherwise
I'll reject the patch"?

I didn't mean to.  I think it is entirely reasonable to queue the patch in
'pu' (after fixing the NULL termination bug), and start cooking without
any of the additional information.

Having said that, this is an external interface, and until we feel
reasonably sure that we are giving enough information to the hook and
we wouldn't need to change the interface, the series must not come near
'master'.

It might make sense to define the external interface to be "information is
given through the standard input of the hook, formatted in YAML, and here
are the initial set of items that may be fed", so that we do not have to
worry about the details too much.

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

[Johannes Schindelin]

Hi,

On Wed, 26 Aug 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > On Tue, 25 Aug 2009, Junio C Hamano wrote:
> >
> >> 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.
> >
> > Certainly those are possible add-on patches, but would you require 
> > them to be in the same commit?
> 
> I was merely responding to the "what else would be useful" question posed
> by Peff.

Sure.

> Did you get an impression that I was saying "you must add these 
> otherwise I'll reject the patch"?

Well, I got the impression that you'd not accept the patch without 
additional information given by the hook, and I got the impression that 
Tom would decide as a consequence to rather live with his eternal fork 
instead of working on getting this patch included.

> It might make sense to define the external interface to be "information 
> is given through the standard input of the hook, formatted in YAML, and 
> here are the initial set of items that may be fed", so that we do not 
> have to worry about the details too much.

Hmm.  You bring up YAML a few times recently, it seems, but I think this 
is not what you are meaning.  In this case, you'd need to have a simple 
enquiry system that asks for some information and receives it as a 
response.

But I would find it utterly overengineered if upload-pack would support 
something as complicated as that.  IMHO either upload-pack knows already 
about the information, or the script has to try to discover it using Git 
commands itself.

My conclusion: I _think_ it would make sense to pass the name of the pack 
file(s) created by upload-pack to the hook, or something similar, but 
nothing more.  Well, _maybe_ the byte count of the protocol exchange and 
the IP.  But nothing that requires calculations.

Ciao,
Dscho

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

[Jeff King]

On Wed, Aug 26, 2009 at 12:06:59PM +0200, Johannes Schindelin wrote:

> > Did you get an impression that I was saying "you must add these 
> > otherwise I'll reject the patch"?
> 
> Well, I got the impression that you'd not accept the patch without 
> additional information given by the hook, and I got the impression that 
> Tom would decide as a consequence to rather live with his eternal fork 
> instead of working on getting this patch included.

I don't think any of us wants that. My point in bringing it up at all
was just "is there anything obvious that we should be adding before this
makes it into master, because after that we will have to deal with an
interface change". I certainly don't want to delay a useful patch too
much while we wait for the moon.

-Peff

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> 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(-)

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:

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

[Junio C Hamano]

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 <gitster@pobox.com>
---

 > 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

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

[Johan Sørensen]

On Thu, Aug 27, 2009 at 2:47 AM, Junio C Hamano<gitster@pobox.com> 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


>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>
>  > 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
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

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

[Jakub Narebski]

Johan Sorensen <johan@johansorensen.com> writes:
> On Thu, Aug 27, 2009 at 2:47 AM, Junio C Hamano <gitster@pobox.com> 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)
 
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

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

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>> 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 <gitster@pobox.com>
---
 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

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

[Tom Werner]

On Fri, Aug 28, 2009 at 11:17 PM, Junio C Hamano<gitster@pobox.com> wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
>
>>> 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

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

[Junio C Hamano]

Tom Werner <mojombo@gmail.com> 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 ;-).

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

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 547 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 330 bytes --]