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

Jeff King [off-list ref] writes:

quoted

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

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

Johannes Schindelin [off-list ref] writes:

Hi,

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

quoted

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?

quoted

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.

Hi,

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

Johannes Schindelin [off-list ref] writes:

quoted

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

quoted

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

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

quoted

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