{"thread":{"id":"20650","subject":"[PATCH] upload-pack: add a trigger for post-upload-pack hook","startedAt":"2009-08-18T07:04:53Z","lastAt":"2009-08-31T23:36:10Z","messageCount":16,"participants":["Tom Preston-Werner","Tom Werner","Jeff King","Junio C Hamano","Johannes Schindelin","Johan Sørensen","Jakub Narebski","Robin H. Johnson"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"121060","messageId":"1250579093-40706-1-git-send-email-tom@mojombo.com","threadId":"20650","inReplyTo":null,"subject":"[PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Tom Preston-Werner","fromEmail":"tom@mojombo.com","sentAt":"2009-08-18T07:04:53Z","receivedAt":"2009-08-18T07:04:53Z","isPatch":true,"sender":{"key":"tom@mojombo.com","avatar":null},"body":"A post-upload-pack hook is desirable for Git hosts that need to\ncollect statistics on how many clones and/or fetches are made\non each repository.\n\nThe hook is called with either \"clone\" or \"fetch\" as the only\nargument, depending on whether a full pack file was sent to the\nclient or not.\n\nSigned-off-by: Tom Preston-Werner <tom@mojombo.com>\n---\n upload-pack.c |    9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex f7d308a..96231dc 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -141,6 +141,13 @@ static int do_rev_list(int fd, void *create_full_pack)\n \treturn 0;\n }\n \n+static void run_post_upload_pack_hook(int create_full_pack)\n+{\n+\tconst char *fetch_type;\n+\tfetch_type = (create_full_pack) ? \"clone\" : \"fetch\";\n+\trun_hook(get_index_file(), \"post-upload-pack\", fetch_type);\n+}\n+\n static void create_pack_file(void)\n {\n \tstruct async rev_list;\n@@ -314,6 +321,8 @@ static void create_pack_file(void)\n \t}\n \tif (use_sideband)\n \t\tpacket_flush(1);\n+\n+\trun_post_upload_pack_hook(create_full_pack);\n \treturn;\n \n  fail:\n-- \n1.6.3.1\n"},{"id":"121738","messageId":"12c267e40908251043g4f3e36aya05d9c705f5afee2@mail.gmail.com","threadId":"20650","inReplyTo":"1250579093-40706-1-git-send-email-tom@mojombo.com","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Tom Werner","fromEmail":"mojombo@gmail.com","sentAt":"2009-08-25T17:43:57Z","receivedAt":"2009-08-25T17:43:57Z","isPatch":true,"sender":{"key":"mojombo@gmail.com","avatar":null},"body":"On Tue, Aug 18, 2009 at 12:04 AM, Tom Preston-Werner<tom@mojombo.com> wrote:\n> A post-upload-pack hook is desirable for Git hosts that need to\n> collect statistics on how many clones and/or fetches are made\n> on each repository.\n>\n> The hook is called with either \"clone\" or \"fetch\" as the only\n> argument, depending on whether a full pack file was sent to the\n> client or not.\n\nI was hoping to get some feedback on this patch, either positive or\nnegative. Since we'll be applying this patch for our use of the Git\nDaemon on GitHub, it would be great to see it in core, so we don't\nhave to maintain custom debian builds forever. I'd imagine that other\nGit hosting sites would find this hook useful as well. Thanks!\n\nTom\n\n--\nTom Preston-Werner\nGitHub Cofounder\nhttp://tom.preston-werner.com\ngithub.com/mojombo\n"},{"id":"121744","messageId":"20090825184525.GC23731@coredump.intra.peff.net","threadId":"20650","inReplyTo":"12c267e40908251043g4f3e36aya05d9c705f5afee2@mail.gmail.com","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-08-25T18:45:25Z","receivedAt":"2009-08-25T18:45:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 25, 2009 at 10:43:57AM -0700, Tom Werner wrote:\n\n> On Tue, Aug 18, 2009 at 12:04 AM, Tom Preston-Werner<tom@mojombo.com> wrote:\n> > A post-upload-pack hook is desirable for Git hosts that need to\n> > collect statistics on how many clones and/or fetches are made\n> > on each repository.\n> >\n> > The hook is called with either \"clone\" or \"fetch\" as the only\n> > argument, depending on whether a full pack file was sent to the\n> > client or not.\n> \n> I was hoping to get some feedback on this patch, either positive or\n> negative. Since we'll be applying this patch for our use of the Git\n> Daemon on GitHub, it would be great to see it in core, so we don't\n> have to maintain custom debian builds forever. I'd imagine that other\n> Git hosting sites would find this hook useful as well. Thanks!\n\nI expect it didn't get any response because nobody here cared one way or\nthe other. Not too surprising, since I think not many people are running\na GitHub-sized hosting site that cares about such statistics. ;) So I\nthink following up as you are doing is the right thing.\n\nAs for the hook itself, the concept certainly seems sane to me. It\npasses the \"hook\" test defined here:\n\n  http://thread.gmane.org/gmane.comp.version-control.git/70781/focus=71069\n\nbecause it is a remote trigger.\n\nBut a few comments on the patch:\n\n> ---\n>  upload-pack.c |    9 +++++++++\n>  1 files changed, 9 insertions(+), 0 deletions(-)\n\nIt needs at least a mention in Documentation/githooks.txt.\n\n> +static void run_post_upload_pack_hook(int create_full_pack)\n> +{\n> +\tconst char *fetch_type;\n> +\tfetch_type = (create_full_pack) ? \"clone\" : \"fetch\";\n> +\trun_hook(get_index_file(), \"post-upload-pack\", fetch_type);\n> +}\n\nDoes it really need an index file? This operation in question seems to\nbe totally disconnected from the index (and indeed, most bare\nrepositories won't even have one). Probably it should pass NULL as the\ninitial argument to run_hook.\n\nIs there any other information that might be useful to other non-GitHub\nusers of the hook? The only thing I can think of is the list of refs\nthat were fetched. I don't want to over-engineer it, but nor do I want\nto be left with the mess of retro-fitting more information onto an\nexisting hook later. Maybe others can comment on whether they would find\nmore information useful.\n\n-Peff\n"},{"id":"121761","messageId":"7vprajmp16.fsf@alter.siamese.dyndns.org","threadId":"20650","inReplyTo":"20090825184525.GC23731@coredump.intra.peff.net","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-25T23:50:29Z","receivedAt":"2009-08-25T23:50:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> +static void run_post_upload_pack_hook(int create_full_pack)\n>> +{\n>> +\tconst char *fetch_type;\n>> +\tfetch_type = (create_full_pack) ? \"clone\" : \"fetch\";\n>> +\trun_hook(get_index_file(), \"post-upload-pack\", fetch_type);\n>> +}\n>\n> Does it really need an index file? This operation in question seems to\n> be totally disconnected from the index (and indeed, most bare\n> repositories won't even have one). Probably it should pass NULL as the\n> initial argument to run_hook.\n\nVery good point; a bare repository does not have to have (and typically\nshouldn't have) the index, and a bare repository is what upload-pack\ntypically serves.\n\nA short-and-sweet:\n\n\trun_hook(NULL, \"post-upload-pack\",\n        \t create_full_pack ? \"clone\" : \"fetch,\n                 NULL);\n\nwould be sufficient.  Notice that run_hook() is variadic and its argument\nlist needs to be terminated with NULL (iow, the original patch is buggy\nand risks reading random places on the stack---I would recommend against\nusing it on your production site yet).\n\n> Is there any other information that might be useful to other non-GitHub\n> users of the hook? The only thing I can think of is the list of refs\n> that were fetched.\n\nI do not think that information is available.  \"want\" will tell you what\nobject they want, but that does not necessarily uniquey translate to a\nref.\n\nIf we are allowed to talk about asking for the moon, and if one of the\nprimary reason for this new hook is statistics, it would be useful to see\nthe number of bytes given, where the fetch-pack came from, and if we are\nusing git-daemon virtual hosting which of our domain served the request.\n"},{"id":"121775","messageId":"alpine.DEB.1.00.0908261043140.4713@intel-tinevez-2-302","threadId":"20650","inReplyTo":"7vprajmp16.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-08-26T08:44:35Z","receivedAt":"2009-08-26T08:44:35Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 25 Aug 2009, Junio C Hamano wrote:\n\n> If we are allowed to talk about asking for the moon,\n\nHow about\n\n \trun_hook(NULL, \"post-upload-pack\",\n         \t create_full_pack ? \"clone\" : \"fetch,\n\t\t \"the moon\",\n                 NULL);\n\nthen?\n\n> and if one of the primary reason for this new hook is statistics, it \n> would be useful to see the number of bytes given, where the fetch-pack \n> came from, and if we are using git-daemon virtual hosting which of our \n> domain served the request.\n\nCertainly those are possible add-on patches, but would you require them to \nbe in the same commit?\n\nCiao,\nDscho\n"},{"id":"121779","messageId":"7vr5uzeyl7.fsf@alter.siamese.dyndns.org","threadId":"20650","inReplyTo":"alpine.DEB.1.00.0908261043140.4713@intel-tinevez-2-302","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-26T09:03:32Z","receivedAt":"2009-08-26T09:03:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> Hi,\n>\n> On Tue, 25 Aug 2009, Junio C Hamano wrote:\n>\n>> If we are allowed to talk about asking for the moon,\n>\n> How about\n>\n>  \trun_hook(NULL, \"post-upload-pack\",\n>          \t create_full_pack ? \"clone\" : \"fetch,\n> \t\t \"the moon\",\n>                  NULL);\n>\n> then?\n>\n>> and if one of the primary reason for this new hook is statistics, it \n>> would be useful to see the number of bytes given, where the fetch-pack \n>> came from, and if we are using git-daemon virtual hosting which of our \n>> domain served the request.\n>\n> Certainly those are possible add-on patches, but would you require them to \n> be in the same commit?\n\nI was merely responding to the \"what else would be useful\" question posed\nby Peff.\n\nDid you get an impression that I was saying \"you must add these otherwise\nI'll reject the patch\"?\n\nI didn't mean to.  I think it is entirely reasonable to queue the patch in\n'pu' (after fixing the NULL termination bug), and start cooking without\nany of the additional information.\n\nHaving said that, this is an external interface, and until we feel\nreasonably sure that we are giving enough information to the hook and\nwe wouldn't need to change the interface, the series must not come near\n'master'.\n\nIt might make sense to define the external interface to be \"information is\ngiven through the standard input of the hook, formatted in YAML, and here\nare the initial set of items that may be fed\", so that we do not have to\nworry about the details too much.\n"},{"id":"121789","messageId":"alpine.DEB.1.00.0908261200160.4713@intel-tinevez-2-302","threadId":"20650","inReplyTo":"7vr5uzeyl7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-08-26T10:06:59Z","receivedAt":"2009-08-26T10:06:59Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 26 Aug 2009, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > On Tue, 25 Aug 2009, Junio C Hamano wrote:\n> >\n> >> and if one of the primary reason for this new hook is statistics, it \n> >> would be useful to see the number of bytes given, where the \n> >> fetch-pack came from, and if we are using git-daemon virtual hosting \n> >> which of our domain served the request.\n> >\n> > Certainly those are possible add-on patches, but would you require \n> > them to be in the same commit?\n> \n> I was merely responding to the \"what else would be useful\" question posed\n> by Peff.\n\nSure.\n\n> Did you get an impression that I was saying \"you must add these \n> otherwise I'll reject the patch\"?\n\nWell, I got the impression that you'd not accept the patch without \nadditional information given by the hook, and I got the impression that \nTom would decide as a consequence to rather live with his eternal fork \ninstead of working on getting this patch included.\n\n> It might make sense to define the external interface to be \"information \n> is given through the standard input of the hook, formatted in YAML, and \n> here are the initial set of items that may be fed\", so that we do not \n> have to worry about the details too much.\n\nHmm.  You bring up YAML a few times recently, it seems, but I think this \nis not what you are meaning.  In this case, you'd need to have a simple \nenquiry system that asks for some information and receives it as a \nresponse.\n\nBut I would find it utterly overengineered if upload-pack would support \nsomething as complicated as that.  IMHO either upload-pack knows already \nabout the information, or the script has to try to discover it using Git \ncommands itself.\n\nMy conclusion: I _think_ it would make sense to pass the name of the pack \nfile(s) created by upload-pack to the hook, or something similar, but \nnothing more.  Well, _maybe_ the byte count of the protocol exchange and \nthe IP.  But nothing that requires calculations.\n\nCiao,\nDscho\n"},{"id":"121803","messageId":"20090826141907.GA3485@sigill.intra.peff.net","threadId":"20650","inReplyTo":"alpine.DEB.1.00.0908261200160.4713@intel-tinevez-2-302","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2009-08-26T14:19:08Z","receivedAt":"2009-08-26T14:19:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 26, 2009 at 12:06:59PM +0200, Johannes Schindelin wrote:\n\n> > Did you get an impression that I was saying \"you must add these \n> > otherwise I'll reject the patch\"?\n> \n> Well, I got the impression that you'd not accept the patch without \n> additional information given by the hook, and I got the impression that \n> Tom would decide as a consequence to rather live with his eternal fork \n> instead of working on getting this patch included.\n\nI don't think any of us wants that. My point in bringing it up at all\nwas just \"is there anything obvious that we should be adding before this\nmakes it into master, because after that we will have to deal with an\ninterface change\". I certainly don't want to delay a useful patch too\nmuch while we wait for the moon.\n\n-Peff\n"},{"id":"121825","messageId":"7v1vmycfhd.fsf@alter.siamese.dyndns.org","threadId":"20650","inReplyTo":"7vprajmp16.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-26T23:39:10Z","receivedAt":"2009-08-26T23:39:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n>> Is there any other information that might be useful to other non-GitHub\n>> users of the hook? The only thing I can think of is the list of refs\n>> that were fetched.\n>\n> I do not think that information is available.  \"want\" will tell you what\n> object they want, but that does not necessarily uniquey translate to a\n> ref.\n>\n> If we are allowed to talk about asking for the moon, and if one of the\n> primary reason for this new hook is statistics, it would be useful to see\n> the number of bytes given, where the fetch-pack came from, and if we are\n> using git-daemon virtual hosting which of our domain served the request.\n\nI briefly looked at upload-pack.c.\n\nAs I said, the names of the refs asked is not available but in that file,\ncreate_pack_file() should be able to gather transfer statistics (timeing\nand bytes transferred).  It also has access to the want_obj[]/have_obj[]\narray, so it should be able to keep the rev-list parameters to be fed to\nthe hook if it wanted to (and with that information you could recreate the\nexact pack data later if necessary).\n\nAnd want_obj[]/have_obj[] information is much more useful than a crude\n\"fetch/clone\" distinction.  You can not just tell if the clients have\nnothing, but how stale they are.\n\nSo at a minimum, if this is primarily meant as a statistics hook, I would\nsuggest the hook not to take _any_ argument, but is fed the information\nfrom its command line in the following form, one piece of information per\nline, from the very beginning:\n\n\twant 40-byte SHA-1\t- what were in want_obj[] array\n\thave 40-byte SHA-1\t- what were in have_obj[] array\n\n\nAs long as the hook scripts are written to ignore lines they do not\nunderstand, in later rounds, we should also be able to feed two more\npieces of information with minimum modification to create_pack_file():\n\n\ttime float\t- number of seconds create_pack_file spent,\n\tsize decimal\t- number of bytes transferred\n\nHere is an illustration patch.\n\n upload-pack.c |   68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-\n 1 files changed, 66 insertions(+), 2 deletions(-)\n\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 4d8be83..69a6f46 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -141,8 +141,60 @@ static int do_rev_list(int fd, void *create_full_pack)\n \treturn 0;\n }\n \n+static int feed_obj_to_hook(const char *label, struct object_array *oa, int i, int fd)\n+{\n+\tint cnt;\n+\tchar buf[512];\n+\n+\tcnt = sprintf(buf, \"%s %s\\n\", label,\n+\t\t      sha1_to_hex(oa->objects[i].item->sha1));\n+\treturn write_in_full(fd, buf, cnt) != cnt;\n+}\n+\n+static int run_post_upload_pack_hook(size_t total, struct timeval *tv)\n+{\n+\tconst char *argv[2];\n+\tstruct child_process proc;\n+\tint err, i;\n+\tint cnt;\n+\tchar buf[512];\n+\n+\targv[0] = \"hooks/post-upload-pack\";\n+\targv[1] = NULL;\n+\n+\tif (access(argv[0], X_OK) < 0)\n+\t\treturn 0;\n+\n+\tmemset(&proc, 0, sizeof(proc));\n+\tproc.argv = argv;\n+\tproc.in = -1;\n+\tproc.stdout_to_stderr = 1;\n+\terr = start_command(&proc);\n+\tif (err)\n+\t\treturn err;\n+\tfor (i = 0; !err && i < want_obj.nr; i++)\n+\t\terr |= feed_obj_to_hook(\"want\", &want_obj, i, proc.in);\n+\tfor (i = 0; !err && i < have_obj.nr; i++)\n+\t\terr |= feed_obj_to_hook(\"have\", &have_obj, i, proc.in);\n+\tif (!err) {\n+\t\tcnt = sprintf(buf, \"time %ld.%06ld\\n\",\n+\t\t\t      (long)tv->tv_sec, (long)tv->tv_usec);\n+\t\terr |= (write_in_full(proc.in, buf, cnt) != cnt);\n+\t}\n+\tif (!err) {\n+\t\tcnt = sprintf(buf, \"size %ld\\n\", (long)total);\n+\t\terr |= (write_in_full(proc.in, buf, cnt) != cnt);\n+\t}\n+\tif (close(proc.in))\n+\t\terr = 1;\n+\tif (finish_command(&proc))\n+\t\terr = 1;\n+\treturn err;\n+}\n+\n static void create_pack_file(void)\n {\n+\tstruct timeval start_tv, tv;\n \tstruct async rev_list;\n \tstruct child_process pack_objects;\n \tint create_full_pack = (nr_our_refs == want_obj.nr && !have_obj.nr);\n@@ -150,10 +202,12 @@ static void create_pack_file(void)\n \tchar abort_msg[] = \"aborting due to possible repository \"\n \t\t\"corruption on the remote side.\";\n \tint buffered = -1;\n-\tssize_t sz;\n+\tssize_t sz, total_sz;\n \tconst char *argv[10];\n \tint arg = 0;\n \n+\tgettimeofday(&start_tv, NULL);\n+\ttotal_sz = 0;\n \tif (shallow_nr) {\n \t\trev_list.proc = do_rev_list;\n \t\trev_list.data = 0;\n@@ -262,7 +316,7 @@ static void create_pack_file(void)\n \t\t\tsz = xread(pack_objects.out, cp,\n \t\t\t\t  sizeof(data) - outsz);\n \t\t\tif (0 < sz)\n-\t\t\t\t\t;\n+\t\t\t\ttotal_sz += sz;\n \t\t\telse if (sz == 0) {\n \t\t\t\tclose(pack_objects.out);\n \t\t\t\tpack_objects.out = -1;\n@@ -314,6 +368,16 @@ static void create_pack_file(void)\n \t}\n \tif (use_sideband)\n \t\tpacket_flush(1);\n+\n+\tgettimeofday(&tv, NULL);\n+\ttv.tv_sec -= start_tv.tv_sec;\n+\tif (tv.tv_usec < start_tv.tv_usec) {\n+\t\ttv.tv_sec--;\n+\t\ttv.tv_usec += 1000000;\n+\t}\n+\ttv.tv_usec -= start_tv.tv_usec;\n+\tif (run_post_upload_pack_hook(total_sz, &tv))\n+\t\twarning(\"post-upload-hook failed\");\n \treturn;\n \n  fail:\n"},{"id":"121831","messageId":"7vy6p69j6a.fsf@alter.siamese.dyndns.org","threadId":"20650","inReplyTo":"7v1vmycfhd.fsf@alter.siamese.dyndns.org","subject":"[PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-27T00:47:41Z","receivedAt":"2009-08-27T00:47:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"After upload-pack successfully finishes its operation, post-upload-pack\nhook can be called for logging purposes.\n\nThe hook is passed various pieces of information, one per line, from its\nstandard input.  Currently the following items can be fed to the hook, but\nmore types of information may be added in the future:\n\n    want SHA-1::\n        40-byte hexadecimal object name the client asked to include in the\n        resulting pack.  Can occur one or more times in the input.\n\n    have SHA-1::\n        40-byte hexadecimal object name the client asked to exclude from\n        the resulting pack, claiming to have them already.  Can occur zero\n        or more times in the input.\n\n    time float::\n        Number of seconds spent for creating the packfile.\n\n    size decimal::\n        Size of the resulting packfile in bytes.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n > Here is an illustration patch.\n\n And here is a bit more polished one with necessary supporting material.\n\n Documentation/git-upload-pack.txt |    2 +\n Documentation/githooks.txt        |   25 +++++++++++++\n t/t5501-post-upload-pack.sh       |   49 ++++++++++++++++++++++++++\n upload-pack.c                     |   68 +++++++++++++++++++++++++++++++++++-\n 4 files changed, 142 insertions(+), 2 deletions(-)\n create mode 100755 t/t5501-post-upload-pack.sh\n\ndiff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt\nindex b8e49dc..63f3b5c 100644\n--- a/Documentation/git-upload-pack.txt\n+++ b/Documentation/git-upload-pack.txt\n@@ -20,6 +20,8 @@ The UI for the protocol is on the 'git-fetch-pack' side, and the\n program pair is meant to be used to pull updates from a remote\n repository.  For push operations, see 'git-send-pack'.\n \n+After finishing the operation successfully, `post-upload-pack`\n+hook is called (see linkgit:githooks[5]).\n \n OPTIONS\n -------\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 1c73673..036f6c7 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -307,6 +307,31 @@ Both standard output and standard error output are forwarded to\n 'git-send-pack' on the other end, so you can simply `echo` messages\n for the user.\n \n+post-upload-pack\n+----------------\n+\n+After upload-pack successfully finishes its operation, this hook is called\n+for logging purposes.\n+\n+The hook is passed various pieces of information, one per line, from its\n+standard input.  Currently the following items can be fed to the hook, but\n+more types of information may be added in the future:\n+\n+want SHA-1::\n+    40-byte hexadecimal object name the client asked to include in the\n+    resulting pack.  Can occur one or more times in the input.\n+\n+have SHA-1::\n+    40-byte hexadecimal object name the client asked to exclude from\n+    the resulting pack, claiming to have them already.  Can occur zero\n+    or more times in the input.\n+\n+time float::\n+    Number of seconds spent for creating the packfile.\n+\n+size decimal::\n+    Size of the resulting packfile in bytes.\n+\n pre-auto-gc\n -----------\n \ndiff --git a/t/t5501-post-upload-pack.sh b/t/t5501-post-upload-pack.sh\nnew file mode 100755\nindex 0000000..2cb63f8\n--- /dev/null\n+++ b/t/t5501-post-upload-pack.sh\n@@ -0,0 +1,49 @@\n+#!/bin/sh\n+\n+test_description='post upload-hook'\n+\n+. ./test-lib.sh\n+\n+LOGFILE=\".git/post-upload-pack-log\"\n+\n+test_expect_success setup '\n+\ttest_commit A &&\n+\ttest_commit B &&\n+\tgit reset --hard A &&\n+\ttest_commit C &&\n+\tgit branch prev B &&\n+\tmkdir -p .git/hooks &&\n+\t{\n+\t\techo \"#!$SHELL_PATH\" &&\n+\t\techo \"cat >post-upload-pack-log\"\n+\t} >\".git/hooks/post-upload-pack\" &&\n+\tchmod +x .git/hooks/post-upload-pack\n+'\n+\n+: test_expect_success initial '\n+\trm -fr sub &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit fetch --no-tags .. prev\n+\t) &&\n+\twant=$(sed -n \"s/^want //p\" \"$LOGFILE\") &&\n+\ttest \"$want\" = \"$(git rev-parse --verify B)\" &&\n+\t! grep \"^have \" \"$LOGFILE\"\n+'\n+\n+test_expect_success second '\n+\trm -fr sub &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit fetch --no-tags .. prev:refs/remotes/prev &&\n+\t\tgit fetch --no-tags .. master\n+\t) &&\n+\twant=$(sed -n \"s/^want //p\" \"$LOGFILE\") &&\n+\ttest \"$want\" = \"$(git rev-parse --verify C)\" &&\n+\thave=$(sed -n \"s/^have //p\" \"$LOGFILE\") &&\n+\ttest \"$have\" = \"$(git rev-parse --verify B)\"\n+'\n+\n+test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 4d8be83..69a6f46 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -141,8 +141,60 @@ static int do_rev_list(int fd, void *create_full_pack)\n \treturn 0;\n }\n \n+static int feed_obj_to_hook(const char *label, struct object_array *oa, int i, int fd)\n+{\n+\tint cnt;\n+\tchar buf[512];\n+\n+\tcnt = sprintf(buf, \"%s %s\\n\", label,\n+\t\t      sha1_to_hex(oa->objects[i].item->sha1));\n+\treturn write_in_full(fd, buf, cnt) != cnt;\n+}\n+\n+static int run_post_upload_pack_hook(size_t total, struct timeval *tv)\n+{\n+\tconst char *argv[2];\n+\tstruct child_process proc;\n+\tint err, i;\n+\tint cnt;\n+\tchar buf[512];\n+\n+\targv[0] = \"hooks/post-upload-pack\";\n+\targv[1] = NULL;\n+\n+\tif (access(argv[0], X_OK) < 0)\n+\t\treturn 0;\n+\n+\tmemset(&proc, 0, sizeof(proc));\n+\tproc.argv = argv;\n+\tproc.in = -1;\n+\tproc.stdout_to_stderr = 1;\n+\terr = start_command(&proc);\n+\tif (err)\n+\t\treturn err;\n+\tfor (i = 0; !err && i < want_obj.nr; i++)\n+\t\terr |= feed_obj_to_hook(\"want\", &want_obj, i, proc.in);\n+\tfor (i = 0; !err && i < have_obj.nr; i++)\n+\t\terr |= feed_obj_to_hook(\"have\", &have_obj, i, proc.in);\n+\tif (!err) {\n+\t\tcnt = sprintf(buf, \"time %ld.%06ld\\n\",\n+\t\t\t      (long)tv->tv_sec, (long)tv->tv_usec);\n+\t\terr |= (write_in_full(proc.in, buf, cnt) != cnt);\n+\t}\n+\tif (!err) {\n+\t\tcnt = sprintf(buf, \"size %ld\\n\", (long)total);\n+\t\terr |= (write_in_full(proc.in, buf, cnt) != cnt);\n+\t}\n+\tif (close(proc.in))\n+\t\terr = 1;\n+\tif (finish_command(&proc))\n+\t\terr = 1;\n+\treturn err;\n+}\n+\n static void create_pack_file(void)\n {\n+\tstruct timeval start_tv, tv;\n \tstruct async rev_list;\n \tstruct child_process pack_objects;\n \tint create_full_pack = (nr_our_refs == want_obj.nr && !have_obj.nr);\n@@ -150,10 +202,12 @@ static void create_pack_file(void)\n \tchar abort_msg[] = \"aborting due to possible repository \"\n \t\t\"corruption on the remote side.\";\n \tint buffered = -1;\n-\tssize_t sz;\n+\tssize_t sz, total_sz;\n \tconst char *argv[10];\n \tint arg = 0;\n \n+\tgettimeofday(&start_tv, NULL);\n+\ttotal_sz = 0;\n \tif (shallow_nr) {\n \t\trev_list.proc = do_rev_list;\n \t\trev_list.data = 0;\n@@ -262,7 +316,7 @@ static void create_pack_file(void)\n \t\t\tsz = xread(pack_objects.out, cp,\n \t\t\t\t  sizeof(data) - outsz);\n \t\t\tif (0 < sz)\n-\t\t\t\t\t;\n+\t\t\t\ttotal_sz += sz;\n \t\t\telse if (sz == 0) {\n \t\t\t\tclose(pack_objects.out);\n \t\t\t\tpack_objects.out = -1;\n@@ -314,6 +368,16 @@ static void create_pack_file(void)\n \t}\n \tif (use_sideband)\n \t\tpacket_flush(1);\n+\n+\tgettimeofday(&tv, NULL);\n+\ttv.tv_sec -= start_tv.tv_sec;\n+\tif (tv.tv_usec < start_tv.tv_usec) {\n+\t\ttv.tv_sec--;\n+\t\ttv.tv_usec += 1000000;\n+\t}\n+\ttv.tv_usec -= start_tv.tv_usec;\n+\tif (run_post_upload_pack_hook(total_sz, &tv))\n+\t\twarning(\"post-upload-hook failed\");\n \treturn;\n \n  fail:\n-- \n1.6.4.1.288.g10d22\n"},{"id":"121862","messageId":"9e0f31700908270509o1031a027y1b49efe7ea9a4fd3@mail.gmail.com","threadId":"20650","inReplyTo":"7vy6p69j6a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Johan Sørensen","fromEmail":"johan@johansorensen.com","sentAt":"2009-08-27T12:09:23Z","receivedAt":"2009-08-27T12:09:23Z","isPatch":true,"sender":{"key":"johan@johansorensen.com","avatar":"https://gravatar.com/avatar/dad7869db9d9711098ad213108d1b09967181bb51b667d5a5088c7634ce46616?d=mp&s=160"},"body":"On Thu, Aug 27, 2009 at 2:47 AM, Junio C Hamano<gitster@pobox.com> wrote:\n> After upload-pack successfully finishes its operation, post-upload-pack\n> hook can be called for logging purposes.\n>\n> The hook is passed various pieces of information, one per line, from its\n> standard input.  Currently the following items can be fed to the hook, but\n> more types of information may be added in the future:\n>\n>    want SHA-1::\n>        40-byte hexadecimal object name the client asked to include in the\n>        resulting pack.  Can occur one or more times in the input.\n>\n>    have SHA-1::\n>        40-byte hexadecimal object name the client asked to exclude from\n>        the resulting pack, claiming to have them already.  Can occur zero\n>        or more times in the input.\n>\n>    time float::\n>        Number of seconds spent for creating the packfile.\n>\n>    size decimal::\n>        Size of the resulting packfile in bytes.\n\nNeat. And feeding it lines gives more room for future additions.\n\nI'd like to suggest the following line from the original patch:\n\n   full-pack integer::\n        1 if the request was considered a full clone, 0 if it was a\npartial update (fetch)\n\n\nAlso, on a similar note; in the little git-daemon (a tiny fork+exec\nserver in ruby) included with Gitorious there's a geo-ip lookup based\non the client addr. It would be fun if the client ip could be passed\nalong to this hook as well, but that would require passing it along\nall the way from before fetch-pack is invoked as far as I can see..?\n\nJS\n\n\n>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>\n>  > Here is an illustration patch.\n>\n>  And here is a bit more polished one with necessary supporting material.\n>\n>  Documentation/git-upload-pack.txt |    2 +\n>  Documentation/githooks.txt        |   25 +++++++++++++\n>  t/t5501-post-upload-pack.sh       |   49 ++++++++++++++++++++++++++\n>  upload-pack.c                     |   68 +++++++++++++++++++++++++++++++++++-\n>  4 files changed, 142 insertions(+), 2 deletions(-)\n>  create mode 100755 t/t5501-post-upload-pack.sh\n>\n> diff --git a/Documentation/git-upload-pack.txt b/Documentation/git-upload-pack.txt\n> index b8e49dc..63f3b5c 100644\n> --- a/Documentation/git-upload-pack.txt\n> +++ b/Documentation/git-upload-pack.txt\n> @@ -20,6 +20,8 @@ The UI for the protocol is on the 'git-fetch-pack' side, and the\n>  program pair is meant to be used to pull updates from a remote\n>  repository.  For push operations, see 'git-send-pack'.\n>\n> +After finishing the operation successfully, `post-upload-pack`\n> +hook is called (see linkgit:githooks[5]).\n>\n>  OPTIONS\n>  -------\n> diff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\n> index 1c73673..036f6c7 100644\n> --- a/Documentation/githooks.txt\n> +++ b/Documentation/githooks.txt\n> @@ -307,6 +307,31 @@ Both standard output and standard error output are forwarded to\n>  'git-send-pack' on the other end, so you can simply `echo` messages\n>  for the user.\n>\n> +post-upload-pack\n> +----------------\n> +\n> +After upload-pack successfully finishes its operation, this hook is called\n> +for logging purposes.\n> +\n> +The hook is passed various pieces of information, one per line, from its\n> +standard input.  Currently the following items can be fed to the hook, but\n> +more types of information may be added in the future:\n> +\n> +want SHA-1::\n> +    40-byte hexadecimal object name the client asked to include in the\n> +    resulting pack.  Can occur one or more times in the input.\n> +\n> +have SHA-1::\n> +    40-byte hexadecimal object name the client asked to exclude from\n> +    the resulting pack, claiming to have them already.  Can occur zero\n> +    or more times in the input.\n> +\n> +time float::\n> +    Number of seconds spent for creating the packfile.\n> +\n> +size decimal::\n> +    Size of the resulting packfile in bytes.\n> +\n>  pre-auto-gc\n>  -----------\n>\n> diff --git a/t/t5501-post-upload-pack.sh b/t/t5501-post-upload-pack.sh\n> new file mode 100755\n> index 0000000..2cb63f8\n> --- /dev/null\n> +++ b/t/t5501-post-upload-pack.sh\n> @@ -0,0 +1,49 @@\n> +#!/bin/sh\n> +\n> +test_description='post upload-hook'\n> +\n> +. ./test-lib.sh\n> +\n> +LOGFILE=\".git/post-upload-pack-log\"\n> +\n> +test_expect_success setup '\n> +       test_commit A &&\n> +       test_commit B &&\n> +       git reset --hard A &&\n> +       test_commit C &&\n> +       git branch prev B &&\n> +       mkdir -p .git/hooks &&\n> +       {\n> +               echo \"#!$SHELL_PATH\" &&\n> +               echo \"cat >post-upload-pack-log\"\n> +       } >\".git/hooks/post-upload-pack\" &&\n> +       chmod +x .git/hooks/post-upload-pack\n> +'\n> +\n> +: test_expect_success initial '\n> +       rm -fr sub &&\n> +       git init sub &&\n> +       (\n> +               cd sub &&\n> +               git fetch --no-tags .. prev\n> +       ) &&\n> +       want=$(sed -n \"s/^want //p\" \"$LOGFILE\") &&\n> +       test \"$want\" = \"$(git rev-parse --verify B)\" &&\n> +       ! grep \"^have \" \"$LOGFILE\"\n> +'\n> +\n> +test_expect_success second '\n> +       rm -fr sub &&\n> +       git init sub &&\n> +       (\n> +               cd sub &&\n> +               git fetch --no-tags .. prev:refs/remotes/prev &&\n> +               git fetch --no-tags .. master\n> +       ) &&\n> +       want=$(sed -n \"s/^want //p\" \"$LOGFILE\") &&\n> +       test \"$want\" = \"$(git rev-parse --verify C)\" &&\n> +       have=$(sed -n \"s/^have //p\" \"$LOGFILE\") &&\n> +       test \"$have\" = \"$(git rev-parse --verify B)\"\n> +'\n> +\n> +test_done\n> diff --git a/upload-pack.c b/upload-pack.c\n> index 4d8be83..69a6f46 100644\n> --- a/upload-pack.c\n> +++ b/upload-pack.c\n> @@ -141,8 +141,60 @@ static int do_rev_list(int fd, void *create_full_pack)\n>        return 0;\n>  }\n>\n> +static int feed_obj_to_hook(const char *label, struct object_array *oa, int i, int fd)\n> +{\n> +       int cnt;\n> +       char buf[512];\n> +\n> +       cnt = sprintf(buf, \"%s %s\\n\", label,\n> +                     sha1_to_hex(oa->objects[i].item->sha1));\n> +       return write_in_full(fd, buf, cnt) != cnt;\n> +}\n> +\n> +static int run_post_upload_pack_hook(size_t total, struct timeval *tv)\n> +{\n> +       const char *argv[2];\n> +       struct child_process proc;\n> +       int err, i;\n> +       int cnt;\n> +       char buf[512];\n> +\n> +       argv[0] = \"hooks/post-upload-pack\";\n> +       argv[1] = NULL;\n> +\n> +       if (access(argv[0], X_OK) < 0)\n> +               return 0;\n> +\n> +       memset(&proc, 0, sizeof(proc));\n> +       proc.argv = argv;\n> +       proc.in = -1;\n> +       proc.stdout_to_stderr = 1;\n> +       err = start_command(&proc);\n> +       if (err)\n> +               return err;\n> +       for (i = 0; !err && i < want_obj.nr; i++)\n> +               err |= feed_obj_to_hook(\"want\", &want_obj, i, proc.in);\n> +       for (i = 0; !err && i < have_obj.nr; i++)\n> +               err |= feed_obj_to_hook(\"have\", &have_obj, i, proc.in);\n> +       if (!err) {\n> +               cnt = sprintf(buf, \"time %ld.%06ld\\n\",\n> +                             (long)tv->tv_sec, (long)tv->tv_usec);\n> +               err |= (write_in_full(proc.in, buf, cnt) != cnt);\n> +       }\n> +       if (!err) {\n> +               cnt = sprintf(buf, \"size %ld\\n\", (long)total);\n> +               err |= (write_in_full(proc.in, buf, cnt) != cnt);\n> +       }\n> +       if (close(proc.in))\n> +               err = 1;\n> +       if (finish_command(&proc))\n> +               err = 1;\n> +       return err;\n> +}\n> +\n>  static void create_pack_file(void)\n>  {\n> +       struct timeval start_tv, tv;\n>        struct async rev_list;\n>        struct child_process pack_objects;\n>        int create_full_pack = (nr_our_refs == want_obj.nr && !have_obj.nr);\n> @@ -150,10 +202,12 @@ static void create_pack_file(void)\n>        char abort_msg[] = \"aborting due to possible repository \"\n>                \"corruption on the remote side.\";\n>        int buffered = -1;\n> -       ssize_t sz;\n> +       ssize_t sz, total_sz;\n>        const char *argv[10];\n>        int arg = 0;\n>\n> +       gettimeofday(&start_tv, NULL);\n> +       total_sz = 0;\n>        if (shallow_nr) {\n>                rev_list.proc = do_rev_list;\n>                rev_list.data = 0;\n> @@ -262,7 +316,7 @@ static void create_pack_file(void)\n>                        sz = xread(pack_objects.out, cp,\n>                                  sizeof(data) - outsz);\n>                        if (0 < sz)\n> -                                       ;\n> +                               total_sz += sz;\n>                        else if (sz == 0) {\n>                                close(pack_objects.out);\n>                                pack_objects.out = -1;\n> @@ -314,6 +368,16 @@ static void create_pack_file(void)\n>        }\n>        if (use_sideband)\n>                packet_flush(1);\n> +\n> +       gettimeofday(&tv, NULL);\n> +       tv.tv_sec -= start_tv.tv_sec;\n> +       if (tv.tv_usec < start_tv.tv_usec) {\n> +               tv.tv_sec--;\n> +               tv.tv_usec += 1000000;\n> +       }\n> +       tv.tv_usec -= start_tv.tv_usec;\n> +       if (run_post_upload_pack_hook(total_sz, &tv))\n> +               warning(\"post-upload-hook failed\");\n>        return;\n>\n>  fail:\n> --\n> 1.6.4.1.288.g10d22\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n>\n"},{"id":"121864","messageId":"m3ws4pl6x7.fsf@localhost.localdomain","threadId":"20650","inReplyTo":"9e0f31700908270509o1031a027y1b49efe7ea9a4fd3@mail.gmail.com","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-08-27T13:33:10Z","receivedAt":"2009-08-27T13:33:10Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Johan Sorensen <johan@johansorensen.com> writes:\n> On Thu, Aug 27, 2009 at 2:47 AM, Junio C Hamano <gitster@pobox.com> wrote:\n\n> > After upload-pack successfully finishes its operation, post-upload-pack\n> > hook can be called for logging purposes.\n> >\n> > The hook is passed various pieces of information, one per line, from its\n> > standard input.  Currently the following items can be fed to the hook, but\n> > more types of information may be added in the future:\n> >\n> >    want SHA-1::\n> >        40-byte hexadecimal object name the client asked to include in the\n> >        resulting pack.  Can occur one or more times in the input.\n> >\n> >    have SHA-1::\n> >        40-byte hexadecimal object name the client asked to exclude from\n> >        the resulting pack, claiming to have them already.  Can occur zero\n> >        or more times in the input.\n> >\n> >    time float::\n> >        Number of seconds spent for creating the packfile.\n> >\n> >    size decimal::\n> >        Size of the resulting packfile in bytes.\n> \n> Neat. And feeding it lines gives more room for future additions.\n> \n> I'd like to suggest the following line from the original patch:\n> \n>    full-pack integer::\n>         1 if the request was considered a full clone, 0 if it was a\n> partial update (fetch)\n \nIf it is all \"want\" and no \"have\", it is clone or fetch into empty\nrepository.  If additionaly \"want\"s cover all refs, it is a clone.\nNo need to pass this information: it can be derived.\n\n> Also, on a similar note; in the little git-daemon (a tiny fork+exec\n> server in ruby) included with Gitorious there's a geo-ip lookup based\n> on the client addr. It would be fun if the client ip could be passed\n> along to this hook as well, but that would require passing it along\n> all the way from before fetch-pack is invoked as far as I can see..?\n\nWell, we can pass at least `client-ip`...\n\n[please don't quote what is not needed]\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"121936","messageId":"robbat2-20090827T225507-944514463Z@orbis-terrarum.net","threadId":"20650","inReplyTo":"7vy6p69j6a.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Robin H. Johnson","fromEmail":"robbat2@gentoo.org","sentAt":"2009-08-27T22:56:05Z","receivedAt":"2009-08-27T22:56:05Z","isPatch":true,"sender":{"key":"robbat2@gentoo.org","avatar":"https://avatars.githubusercontent.com/u/373898?v=4"},"body":"On Wed, Aug 26, 2009 at 05:47:41PM -0700, Junio C Hamano wrote:\n> After upload-pack successfully finishes its operation, post-upload-pack\n> hook can be called for logging purposes.\n+1 from me.\n\nI'm actually going to scrap my previous pre-upload-pack hook and rebase\nit off this, because I see a lot of commonality (I was passing in\nhave/want via stdin too).\n\n-- \nRobin Hugh Johnson\nGentoo Linux: Developer, Trustee & Infrastructure Lead\nE-Mail     : robbat2@gentoo.org\nGnuPG FP   : 11AC BA4F 4778 E3F6 E4ED  F38E B27B 944E 3488 4E85\n"},{"id":"122077","messageId":"7vljl3p2iw.fsf@alter.siamese.dyndns.org","threadId":"20650","inReplyTo":"m3ws4pl6x7.fsf@localhost.localdomain","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-29T06:17:27Z","receivedAt":"2009-08-29T06:17:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n>> I'd like to suggest the following line from the original patch:\n>> \n>>    full-pack integer::\n>>         1 if the request was considered a full clone, 0 if it was a\n>> partial update (fetch)\n>  \n> If it is all \"want\" and no \"have\", it is clone or fetch into empty\n> repository.  If additionaly \"want\"s cover all refs, it is a clone.\n> No need to pass this information: it can be derived.\n\nWell, not exactly.\n\nHere is an iffy RFC patch.  Iffy not in the sense that its implementation\nis questionable, but in the sense that I am not really convinced if the\ndistinction between fetching some (or in the worst case, most) but not all\nrefs, and fetching full set of refs, into an empty repository is something\nworth making.\n\nDoes anybody from GitHub have any input?  Is there something that can\nstill improved to suit GitHub's needs?\n\n-- >8 --\nSubject: [PATCH] upload-pack: feed \"kind [clone|fetch]\" to post-upload-pack hook\n\nA request to clone the repository does not give any \"have\" but asks for\nall the refs we offer with \"want\".  When a request does not ask to clone\nthe repository fully, but asks to fetch some refs into an empty\nrepository, it will not give any \"have\" but its \"want\" won't ask for all\nthe refs we offer.\n\nIf we suppose (and I would say this is a rather big if) that it makes\nsense to distinguish these two cases, a hook cannot reliably do this\nalone.  The hook can detect lack of \"have\" and bunch of \"want\", but there\nis no direct way to tell if the other end asked for all refs we offered,\nor merely most of them.\n\nBetween the time we talked with the other end and the time the hook got\ncalled, we may have acquired more refs or lost some refs in the repository\nby concurrent operations.  Given that we plan to introduce selective\nadvertisement of refs with a protocol extension, it would become even more\ndifficult for hooks to guess between these two cases.\n\nThis adds \"kind [clone|fetch]\" to hook's input, as a stable interface to\nallow the hooks to tell these cases apart.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/githooks.txt  |    4 ++++\n t/t5501-post-upload-pack.sh |   24 ++++++++++++++++++++++--\n upload-pack.c               |    4 ++++\n 3 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 036f6c7..c308d29 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -332,6 +332,10 @@ time float::\n size decimal::\n     Size of the resulting packfile in bytes.\n \n+kind string:\n+    Either \"clone\" (when the client did not give us any \"have\", and asked\n+    for all our refs with \"want\"), or \"fetch\" (otherwise).\n+\n pre-auto-gc\n -----------\n \ndiff --git a/t/t5501-post-upload-pack.sh b/t/t5501-post-upload-pack.sh\nindex 47ee7b5..d89fb51 100755\n--- a/t/t5501-post-upload-pack.sh\n+++ b/t/t5501-post-upload-pack.sh\n@@ -29,7 +29,9 @@ test_expect_success initial '\n \t) &&\n \twant=$(sed -n \"s/^want //p\" \"$LOGFILE\") &&\n \ttest \"$want\" = \"$(git rev-parse --verify B)\" &&\n-\t! grep \"^have \" \"$LOGFILE\"\n+\t! grep \"^have \" \"$LOGFILE\" &&\n+\tkind=$(sed -n \"s/^kind //p\" \"$LOGFILE\") &&\n+\ttest \"$kind\" = fetch\n '\n \n test_expect_success second '\n@@ -43,7 +45,25 @@ test_expect_success second '\n \twant=$(sed -n \"s/^want //p\" \"$LOGFILE\") &&\n \ttest \"$want\" = \"$(git rev-parse --verify C)\" &&\n \thave=$(sed -n \"s/^have //p\" \"$LOGFILE\") &&\n-\ttest \"$have\" = \"$(git rev-parse --verify B)\"\n+\ttest \"$have\" = \"$(git rev-parse --verify B)\" &&\n+\tkind=$(sed -n \"s/^kind //p\" \"$LOGFILE\") &&\n+\ttest \"$kind\" = fetch\n+'\n+\n+test_expect_success all '\n+\trm -fr sub &&\n+\tHERE=$(pwd) &&\n+\tgit init sub &&\n+\t(\n+\t\tcd sub &&\n+\t\tgit clone \"file://$HERE/.git\" new\n+\t) &&\n+\tsed -n \"s/^want //p\" \"$LOGFILE\" | sort >actual &&\n+\tgit rev-parse A B C | sort >expect &&\n+\ttest_cmp expect actual &&\n+\t! grep \"^have \" \"$LOGFILE\" &&\n+\tkind=$(sed -n \"s/^kind //p\" \"$LOGFILE\") &&\n+\ttest \"$kind\" = clone\n '\n \n test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex 857440d..8e82179 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -187,6 +187,10 @@ static int run_post_upload_pack_hook(size_t total, struct timeval *tv)\n \t\t\t\t\t(long)tv->tv_sec, (long)tv->tv_usec);\n \tif (!err)\n \t\terr |= feed_msg_to_hook(proc.in, \"size %ld\\n\", (long)total);\n+\tif (!err)\n+\t\terr |= feed_msg_to_hook(proc.in, \"kind %s\\n\",\n+\t\t\t\t\t(nr_our_refs == want_obj.nr && !have_obj.nr)\n+\t\t\t\t\t? \"clone\" : \"fetch\");\n \tif (close(proc.in))\n \t\terr = 1;\n \tif (finish_command(&proc))\n-- \n1.6.4.1.307.g70e9f\n"},{"id":"122195","messageId":"12c267e40908311150n2aad598aw978c4691c27ac0fa@mail.gmail.com","threadId":"20650","inReplyTo":"7vljl3p2iw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Tom Werner","fromEmail":"mojombo@gmail.com","sentAt":"2009-08-31T18:50:08Z","receivedAt":"2009-08-31T18:50:08Z","isPatch":true,"sender":{"key":"mojombo@gmail.com","avatar":null},"body":"On Fri, Aug 28, 2009 at 11:17 PM, Junio C Hamano<gitster@pobox.com> wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n>\n>>> I'd like to suggest the following line from the original patch:\n>>>\n>>>    full-pack integer::\n>>>         1 if the request was considered a full clone, 0 if it was a\n>>> partial update (fetch)\n>>\n>> If it is all \"want\" and no \"have\", it is clone or fetch into empty\n>> repository.  If additionaly \"want\"s cover all refs, it is a clone.\n>> No need to pass this information: it can be derived.\n>\n> Well, not exactly.\n>\n> Here is an iffy RFC patch.  Iffy not in the sense that its implementation\n> is questionable, but in the sense that I am not really convinced if the\n> distinction between fetching some (or in the worst case, most) but not all\n> refs, and fetching full set of refs, into an empty repository is something\n> worth making.\n>\n> Does anybody from GitHub have any input?  Is there something that can\n> still improved to suit GitHub's needs?\n\nFrom GitHub's perspective, we'd treat any clone or fetch into an empty\nrepo as a clone operation, whether or not that included all of the\nrefs that were available. For us, the distinction between full and\npartial clones is too nuanced to warrant additional code. I'd be happy\nwith the previous incarnation of the post-upload-pack that simply\nsends the HAVEs and WANTs.\n\nTom\n"},{"id":"122211","messageId":"7vab1fzhcl.fsf@alter.siamese.dyndns.org","threadId":"20650","inReplyTo":"12c267e40908311150n2aad598aw978c4691c27ac0fa@mail.gmail.com","subject":"Re: [PATCH] upload-pack: add a trigger for post-upload-pack hook","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-08-31T23:36:10Z","receivedAt":"2009-08-31T23:36:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tom Werner <mojombo@gmail.com> writes:\n\n> ... I'd be happy\n> with the previous incarnation of the post-upload-pack that simply\n> sends the HAVEs and WANTs.\n\nThanks.  This settles the biggest worry I had.\n\nThe worry was not about \"do we feed clone/fetch info?\", but was about\ngetting a complaint \"Now, these changes to feed info from standard input\ndoes not help anybody but forces us to update our hooks for no good\nreason\" from you guys ;-).\n"}]}