{"thread":{"id":"29268","subject":"extended hook api and tweak-fetch hook","startedAt":"2011-12-30T01:07:17Z","lastAt":"2012-01-03T21:44:45Z","messageCount":10,"participants":["Joey Hess","Johannes Sixt","Junio C Hamano","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"181793","messageId":"1325207240-22622-1-git-send-email-joey@kitenet.net","threadId":"29268","inReplyTo":null,"subject":"extended hook api and tweak-fetch hook","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2011-12-30T01:07:17Z","receivedAt":"2011-12-30T01:07:17Z","isPatch":false,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"This patch series adds an extended hook API, and uses it to implement\nthe new tweak-fetch hook.\n\nThe remaining hooks (that do not already use run_hook()) could be\nrefactored later to use this new API.\n\nAlso, the API has been designed to allow several programs to be run\nfor a single hook, when someone wants to add that into git.\n"},{"id":"181794","messageId":"1325207240-22622-2-git-send-email-joey@kitenet.net","threadId":"29268","inReplyTo":"1325207240-22622-1-git-send-email-joey@kitenet.net","subject":"[PATCH 1/3] expanded hook api with stdio support","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2011-12-30T01:07:18Z","receivedAt":"2011-12-30T01:07:18Z","isPatch":true,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Adds run_hook_complex() and the struct hook, and implements\nrun_hook() using it.\n\nThis new API for hooks will allow for a later refactoring of the existing\nad-hoc hook code for hooks that are fed input on stdin, such as pre-receive,\npost-receive, and post-rewrite.\n\nIt also adds support for a new class of hooks whose stdout is processed by\ngit, as well as hooks that both receive stdin and have their stdout\nprocessed. This is controlled by setting \"generator\" and \"reader\" members\nof the struct hook.\n\nThe API provides control over whether a hook must consume all its stdin,\nor is free to ignore some of it; this can be specified by using either\nfeed_hook_in_full() or feed_hook_incomplete() as the \"feeder\" member of\nthe struct hook. The stdin feeder runs asynchronously, to avoid blocking\nwhen the hook's stdin is also being read. So the API design limits the\ncode that needs to run asynchronously, to make it easy to implement\nthread-safe feeders.\n\nFinally, the API is designed to be extended in the future, to support\nrunning multiple programs for a single hook action (such as the contents\nof a .git/hooks/hook.d/ , or a system-wide hook). This design goal led\nto the \"generator\" and \"reader\" members of the struct hook, which are\nspecified such that they can be called repeatedly, with data flowing\nbetween them (via the \"data\" member), like this:\n    generator | hook_prog_1 | reader | generator | hook_prog_2 | reader\n\nSigned-off-by: Joey Hess <joey@kitenet.net>\n---\n Documentation/technical/api-run-command.txt |   53 +++++++++++\n run-command.c                               |  132 ++++++++++++++++++++++++---\n run-command.h                               |   58 +++++++++++-\n 3 files changed, 226 insertions(+), 17 deletions(-)\n\ndiff --git a/Documentation/technical/api-run-command.txt b/Documentation/technical/api-run-command.txt\nindex f18b4f4..ff50d2f 100644\n--- a/Documentation/technical/api-run-command.txt\n+++ b/Documentation/technical/api-run-command.txt\n@@ -87,6 +87,17 @@ The functions above do the following:\n \tOn execution, .stdout_to_stderr and .no_stdin will be set.\n \t(See below.)\n \n+`run_hook_complex`::\n+\n+\tRun a hook, with the caller providing its stdin and/or parsing its\n+\tstdout.\n+\tTakes a pointer to a `struct hook` that specifies the details,\n+\tincluding the name of the hook, any parameters to pass to it,\n+\tand how to handle the stdin and stdout. (See below.)\n+\tIf the hook does not exist or is not executable, the return value\n+\twill be zero.\n+\tIf it is executable, the hook will be executed and the exit\n+\tstatus of the hook is returned.\n \n Data structures\n ---------------\n@@ -241,3 +252,45 @@ a forked process otherwise:\n \n . It must not change the program's state that the caller of the\n   facility also uses.\n+\n+* `struct hook`\n+\n+This describes a hook to run.\n+\n+The caller:\n+\n+1. allocates and clears (memset(&hook, 0, sizeof(hook));) a\n+   struct hook variable;\n+2. initializes the members;\n+3. calls hook();\n+4. if necessary, accesses data read from the hook in .data\n+5. frees the struct hook.\n+\n+The `struct hook` has three function pointers that may be set to\n+control the stdin that is sent to the hook, and to collect its stdout.\n+\n+The `generator` generates the stdin for the hook, returning it in a strbuf.\n+It is passed a pointer to the `struct hook`.\n+\n+The `feeder` is run asynchronously to feed the generated stdin into the hook.\n+It is passed the handle to write to, the strbuf containing the stdin, and \n+a pointer to the `struct hook`, and should return non-zero on failure.\n+Typically it is set to either `feed_hook_in_full`, or `feed_hook_incomplete`.\n+\n+The `reader` reads and processes the hook's stdout. It is passed \n+a handle to read from and a pointer to the `struct hook`, and should return\n+non-zero on failure.\n+\n+If the generator or feeder are NULL, the hook is not fed anything\n+on stdin; if the `reader` is NULL, the hook's stdout is sent to\n+stderr instead.\n+\n+Note that in the future, the generator, feeder, and reader might be run\n+more than once, if multiple programs are run as part of a single hook.\n+Therefore, all three should avoid taking any actions except for generating\n+the stdin, writing it to the hook, reading/parsing the hook's stdout,\n+and printing any necessary warning messages.\n+\n+The `struct hook` also contains a `data` pointer, which can be used\n+to communicate between the generator, feeder, reader, and the\n+caller of the hook.\ndiff --git a/run-command.c b/run-command.c\nindex 1c51043..42a9b06 100644\n--- a/run-command.c\n+++ b/run-command.c\n@@ -605,34 +605,136 @@ int finish_async(struct async *async)\n \n int run_hook(const char *index_file, const char *name, ...)\n {\n-\tstruct child_process hook;\n+\tstruct hook hook;\n \tstruct argv_array argv = ARGV_ARRAY_INIT;\n-\tconst char *p, *env[2];\n-\tchar index[PATH_MAX];\n+\tconst char *p;\n \tva_list args;\n \tint ret;\n \n-\tif (access(git_path(\"hooks/%s\", name), X_OK) < 0)\n-\t\treturn 0;\n-\n \tva_start(args, name);\n-\targv_array_push(&argv, git_path(\"hooks/%s\", name));\n \twhile ((p = va_arg(args, const char *)))\n \t\targv_array_push(&argv, p);\n \tva_end(args);\n \n \tmemset(&hook, 0, sizeof(hook));\n-\thook.argv = argv.argv;\n-\thook.no_stdin = 1;\n-\thook.stdout_to_stderr = 1;\n-\tif (index_file) {\n-\t\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\", index_file);\n+\thook.name = name;\n+\thook.index_file = index_file;\n+\thook.argv_array = &argv;\n+\tret = run_hook_complex(&hook);\n+\n+\targv_array_clear(&argv);\n+\treturn ret;\n+}\n+\n+struct feed_hook_with {\n+\tstruct strbuf *buf;\n+\tstruct hook *hook;\n+};\n+\n+/*\n+ * An async process is used to feed the hook its stdin.\n+ * This allows the hook to read and write at its own pace without blocking.\n+ */\n+static int feed_hook_async(int in, int out, void *data)\n+{\n+\tstruct feed_hook_with *feedwith = data;\n+\tint ret = feedwith->hook->feeder(out, feedwith->buf, feedwith->hook);\n+\tclose(out);\n+\treturn ret;\n+}\n+\n+/*\n+ * Runs a hook, if it exists. Returns non-zero if the hook fails to run\n+ * correctly.\n+ */\n+int run_hook_complex(struct hook *hook)\n+{\n+\tchar *command;\n+\tstruct child_process child;\n+\tstruct async async;\n+\tstruct feed_hook_with feedwith = { 0, hook };\n+\tstruct argv_array argv = ARGV_ARRAY_INIT;\n+\tconst char *env[2];\n+\tchar index[PATH_MAX];\n+\tint res = 0;\n+\tint i;\n+\n+\tcommand = git_path(\"hooks/%s\", hook->name);\n+\tif (access(command, X_OK) < 0)\n+\t\treturn 0;\n+\n+\tmemset(&child, 0, sizeof(child));\n+\targv_array_push(&argv, command);\n+\tif (hook->argv_array)\n+\t\tfor (i = 0; i < hook->argv_array->argc; i++)\n+\t\t\targv_array_push(&argv, hook->argv_array->argv[i]);\n+\tchild.argv = argv.argv;\n+\tif (hook->generator && hook->feeder)\n+\t\tchild.in = -1;\n+\telse\n+\t\tchild.no_stdin = 1;\n+\tif (hook->reader)\n+\t\tchild.out = -1;\n+\telse\n+\t\tchild.stdout_to_stderr = 1;\n+\tif (hook->index_file) {\n+\t\tsnprintf(index, sizeof(index), \"GIT_INDEX_FILE=%s\",\n+\t\t\t\thook->index_file);\n \t\tenv[0] = index;\n \t\tenv[1] = NULL;\n-\t\thook.env = env;\n+\t\tchild.env = env;\n+\t}\n+\tres |= start_command(&child);\n+\tif (res) goto hook_out;\n+\n+\tif (hook->generator && hook->feeder) {\n+\t\tfeedwith.buf = hook->generator(hook);\n+\t\tif (! feedwith.buf) {\n+\t\t\tres = 1;\n+\t\t\tgoto hook_out;\n+\t\t}\n+\n+\t\tmemset(&async, 0, sizeof(async));\n+\t\tasync.proc = feed_hook_async;\n+\t\tasync.data = &feedwith;\n+\t\tasync.out = child.in;\n+\t\tres |= start_async(&async);\n+\t\tif (res) {\n+\t\t\tclose(child.in);\n+\t\t\tclose(child.out);\n+\t\t\tfinish_command(&child);\n+\t\t\tgoto hook_out;\n+\t\t}\n \t}\n \n-\tret = run_command(&hook);\n+\tif (hook->reader)\n+\t\tres |= hook->reader(child.out, hook);\n+\tif (hook->generator && hook->feeder)\n+\t       res |= finish_async(&async);\n+\tres |= finish_command(&child);\n+\n+ hook_out:\n+\tif (feedwith.buf)\n+\t\tstrbuf_release(feedwith.buf);\n \targv_array_clear(&argv);\n-\treturn ret;\n+\n+\treturn res;\n+}\n+\n+/* A feeder that ensures the hook consumes all its stdin. */\n+int feed_hook_in_full(int handle, struct strbuf *buf, struct hook *hook)\n+{\n+\tif (write_in_full(handle, buf->buf, buf->len) != buf->len) {\n+\t\twarning(\"%s hook failed to consume all its input\", hook->name);\n+\t\treturn 1;\n+\t}\n+\telse\n+\t\treturn 0;\n+}\n+\n+/* A feeder that does not require the hook consume all its stdin. */\n+int feed_hook_incomplete(int handle, struct strbuf *buf, struct hook *hook)\n+{\n+\twrite_in_full(handle, buf->buf, buf->len);\n+\treturn 0;\n }\ndiff --git a/run-command.h b/run-command.h\nindex 56491b9..54c9b83 100644\n--- a/run-command.h\n+++ b/run-command.h\n@@ -45,8 +45,6 @@ int start_command(struct child_process *);\n int finish_command(struct child_process *);\n int run_command(struct child_process *);\n \n-extern int run_hook(const char *index_file, const char *name, ...);\n-\n #define RUN_COMMAND_NO_STDIN 1\n #define RUN_GIT_CMD\t     2\t/*If this is to be git sub-command */\n #define RUN_COMMAND_STDOUT_TO_STDERR 4\n@@ -90,4 +88,60 @@ struct async {\n int start_async(struct async *async);\n int finish_async(struct async *async);\n \n+/*\n+ * This data structure controls how a hook is run.\n+ */\n+struct hook {\n+\t/* The name of the hook being run. */\n+\tconst char *name;\n+\t/*\n+\t * Parameters to pass to the hook program, not including the name\n+\t * of the hook. May be NULL.\n+\t */\n+\tstruct argv_array *argv_array;\n+\t/*\n+\t * Pathname to an index file to use, or NULL if the hook\n+\t * uses the default index file or no index is needed.\n+\t */\n+\tconst char *index_file;\n+\t/*\n+\t * An arbitrary data structure, can be used to communicate between\n+\t * the generator, feeder, reader, and the caller of the hook.\n+\t */\n+\tvoid *data;\n+\t/*\n+\t * Populates a strbuf with the content to send to the\n+\t * hook on its standard input.\n+\t *\n+\t * May be NULL, if the hook does not consume standard input.\n+\t */\n+\tstruct strbuf *(*generator)(struct hook *hook);\n+\t/*\n+\t * Feeds generated content to the hook on its standard input\n+\t * via the handle. Returns non-zero on failure.\n+\t *\n+\t * May be NULL, if the hook does not consume standard input.\n+\t *\n+\t * Note that the feeder is run as an async process, and so should\n+\t * avoid modifying any global state, and only use thread-safe\n+\t * operations. It may be run more than once.\n+\t */\n+\tint (*feeder)(int handle, struct strbuf *data, struct hook *hook);\n+\t/*\n+\t * Processes the hook's standard output from the handle,\n+\t * returning non-zero on failure.\n+\t *\n+\t * May be NULL, if the hook's stdin is not processed. (It will\n+\t * instead be redirected to stderr.)\n+\t */\n+\tint (*reader)(int handle, struct hook *hook);\n+};\n+\n+extern int run_hook(const char *index_file, const char *name, ...);\n+\n+extern int run_hook_complex(struct hook *hook);\n+\n+extern int feed_hook_in_full(int handle, struct strbuf *buf, struct hook *hook);\n+extern int feed_hook_incomplete(int handle, struct strbuf *buf, struct hook *hook);\n+\n #endif\n-- \n1.7.7.3\n"},{"id":"181796","messageId":"1325207240-22622-3-git-send-email-joey@kitenet.net","threadId":"29268","inReplyTo":"1325207240-22622-1-git-send-email-joey@kitenet.net","subject":"[PATCH 2/3] preparations for tweak-fetch hook","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2011-12-30T01:07:19Z","receivedAt":"2011-12-30T01:07:19Z","isPatch":true,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"No behavior changes yet, only some groundwork for the next\nchange.\n\nThe refs_result structure combines a status code with a ref map,\nwhich can be NULL even on success. This will be needed when\nthere's a tweak-fetch hook, because it can filter out all refs,\nwhile still succeeding.\n\nfetch_refs returns a refs_result, so that it can modify the ref_map.\n\nSigned-off-by: Joey Hess <joey@kitenet.net>\n---\n builtin/fetch.c |   55 ++++++++++++++++++++++++++++++++++++-------------------\n 1 files changed, 36 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 33ad3aa..a48358a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -29,6 +29,11 @@ enum {\n \tTAGS_SET = 2\n };\n \n+struct refs_result {\n+\tstruct ref *new_refs;\n+\tint status;\n+};\n+\n static int all, append, dry_run, force, keep, multiple, prune, update_head_ok, verbosity;\n static int progress, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n static int tags = TAGS_DEFAULT;\n@@ -89,6 +94,15 @@ static struct option builtin_fetch_options[] = {\n \tOPT_END()\n };\n \n+static int add_existing(const char *refname, const unsigned char *sha1,\n+\t\t\tint flag, void *cbdata)\n+{\n+\tstruct string_list *list = (struct string_list *)cbdata;\n+\tstruct string_list_item *item = string_list_insert(list, refname);\n+\titem->util = (void *)sha1;\n+\treturn 0;\n+}\n+\n static void unlock_pack(void)\n {\n \tif (transport)\n@@ -507,17 +521,25 @@ static int quickfetch(struct ref *ref_map)\n \treturn check_everything_connected(iterate_ref_map, 1, &rm);\n }\n \n-static int fetch_refs(struct transport *transport, struct ref *ref_map)\n+static struct refs_result fetch_refs(struct transport *transport,\n+\t\tstruct ref *ref_map)\n {\n-\tint ret = quickfetch(ref_map);\n-\tif (ret)\n-\t\tret = transport_fetch_refs(transport, ref_map);\n-\tif (!ret)\n-\t\tret |= store_updated_refs(transport->url,\n+\tstruct refs_result res;\n+\tres.status = quickfetch(ref_map);\n+\tif (res.status)\n+\t\tres.status = transport_fetch_refs(transport, ref_map);\n+\tif (!res.status) {\n+\t\tres.new_refs = ref_map;\n+\n+\t\tres.status |= store_updated_refs(transport->url,\n \t\t\t\ttransport->remote->name,\n-\t\t\t\tref_map);\n+\t\t\t\tres.new_refs);\n+\t}\n+\telse {\n+\t\tres.new_refs = ref_map;\n+\t}\n \ttransport_unlock_pack(transport);\n-\treturn ret;\n+\treturn res;\n }\n \n static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)\n@@ -542,15 +564,6 @@ static int prune_refs(struct refspec *refs, int ref_count, struct ref *ref_map)\n \treturn result;\n }\n \n-static int add_existing(const char *refname, const unsigned char *sha1,\n-\t\t\tint flag, void *cbdata)\n-{\n-\tstruct string_list *list = (struct string_list *)cbdata;\n-\tstruct string_list_item *item = string_list_insert(list, refname);\n-\titem->util = (void *)sha1;\n-\treturn 0;\n-}\n-\n static int will_fetch(struct ref **head, const unsigned char *sha1)\n {\n \tstruct ref *rm = *head;\n@@ -673,6 +686,7 @@ static int do_fetch(struct transport *transport,\n \tstruct string_list_item *peer_item = NULL;\n \tstruct ref *ref_map;\n \tstruct ref *rm;\n+\tstruct refs_result res;\n \tint autotags = (transport->remote->fetch_tags == 1);\n \n \tfor_each_ref(add_existing, &existing_refs);\n@@ -710,7 +724,9 @@ static int do_fetch(struct transport *transport,\n \n \tif (tags == TAGS_DEFAULT && autotags)\n \t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, \"1\");\n-\tif (fetch_refs(transport, ref_map)) {\n+\tres = fetch_refs(transport, ref_map);\n+\tref_map = res.new_refs;\n+\tif (res.status) {\n \t\tfree_refs(ref_map);\n \t\treturn 1;\n \t}\n@@ -750,7 +766,8 @@ static int do_fetch(struct transport *transport,\n \t\tif (ref_map) {\n \t\t\ttransport_set_option(transport, TRANS_OPT_FOLLOWTAGS, NULL);\n \t\t\ttransport_set_option(transport, TRANS_OPT_DEPTH, \"0\");\n-\t\t\tfetch_refs(transport, ref_map);\n+\t\t\tres = fetch_refs(transport, ref_map);\n+\t\t\tref_map = res.new_refs;\n \t\t}\n \t\tfree_refs(ref_map);\n \t}\n-- \n1.7.7.3\n"},{"id":"181795","messageId":"1325207240-22622-4-git-send-email-joey@kitenet.net","threadId":"29268","inReplyTo":"1325207240-22622-1-git-send-email-joey@kitenet.net","subject":"[PATCH 3/3] add tweak-fetch hook","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2011-12-30T01:07:20Z","receivedAt":"2011-12-30T01:07:20Z","isPatch":true,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"The tweak-fetch hook is fed lines on stdin for all refs that were fetched,\nand outputs on stdout possibly modified lines. Its output is parsed and\nused when git fetch updates the remote tracking refs, records the entries\nin FETCH_HEAD, and produces its report.\n\nThis is implemented using the new run_hook_complex API.\n\nSigned-off-by: Joey Hess <joey@kitenet.net>\n---\n Documentation/githooks.txt |   29 +++++++++\n builtin/fetch.c            |  144 +++++++++++++++++++++++++++++++++++++++++++-\n 2 files changed, 172 insertions(+), 1 deletions(-)\n\ndiff --git a/Documentation/githooks.txt b/Documentation/githooks.txt\nindex 28edefa..bea450a 100644\n--- a/Documentation/githooks.txt\n+++ b/Documentation/githooks.txt\n@@ -162,6 +162,35 @@ This hook can be used to perform repository validity checks, auto-display\n differences from the previous HEAD if different, or set working dir metadata\n properties.\n \n+tweak-fetch\n+~~~~~~~~~~~\n+\n+This hook is invoked by 'git fetch' (commonly called by 'git pull'), after\n+refs have been fetched from the remote repository. It is not executed, if\n+nothing was fetched.\n+\n+The output of the hook is used to update the remote-tracking branches, and\n+`.git/FETCH_HEAD`, in preparation for for a later merge operation done by\n+'git merge'.\n+\n+It takes no arguments, but is fed a line of the following format on\n+its standard input for each ref that was fetched.\n+\n+  <sha1> SP not-for-merge|merge SP <remote-refname> SP <local-refname> LF\n+\n+Where the \"not-for-merge\" flag indicates the ref is not to be merged into the\n+current branch, and the \"merge\" flag indicates that 'git merge' should\n+later merge it. The `<remote-refname>` is the remote's name for the ref\n+that was fetched, and `<local-refname>` is a name of a remote-tracking branch,\n+like \"refs/remotes/origin/master\", or can be empty if the fetched ref is not\n+being stored in a local refname.\n+\n+The hook must consume all of its standard input, and output back lines\n+of the same format. It can modify its input as desired, including\n+adding or removing lines, updating the sha1 (i.e. re-point the\n+remote-tracking branch), changing the merge flag, and changing the\n+`<local-refname>` (i.e. use different remote-tracking branch).\n+\n post-merge\n ~~~~~~~~~~\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex a48358a..80178d0 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -103,6 +103,148 @@ static int add_existing(const char *refname, const unsigned char *sha1,\n \treturn 0;\n }\n \n+struct strbuf *tweak_fetch_hook_generator(struct hook *hook)\n+{\n+\tstruct ref *ref;\n+\tstruct strbuf *buf = xmalloc(sizeof(struct strbuf));\n+\n+\tstrbuf_init(buf, 128);\n+\tfor (ref = hook->data; ref; ref = ref->next)\n+\t\tstrbuf_addf(buf, \"%s %s %s %s\\n\",\n+\t\t\tsha1_to_hex(ref->old_sha1),\n+\t\t\tref->merge ? \"merge\" : \"not-for-merge\",\n+\t\t\tref->name ? ref->name : \"\",\n+\t\t\tref->peer_ref && ref->peer_ref->name ?\n+\t\t\t\tref->peer_ref->name : \"\");\n+\treturn buf;\n+}\n+\n+static struct ref *parse_tweak_fetch_hook_line(char *l, \n+\t\tstruct string_list *existing_refs, struct hook *hook)\n+{\n+\tstruct ref *ref = NULL, *peer_ref = NULL;\n+\tstruct string_list_item *peer_item = NULL;\n+\tchar *p, *words[4];\n+\tchar *problem;\n+\tint i;\n+\n+\tfor (i = 0 ; i <= ARRAY_SIZE(words) - 1; i++) {\n+\t\tp = strchr(l, ' ');\n+\t\twords[i] = l;\n+\t\tif (!p)\n+\t\t\tbreak;\n+\t\tp[0] = '\\0';\n+\t\tl = p+1;\n+\t}\n+\tif (i != ARRAY_SIZE(words) - 1) {\n+\t\tproblem = \"wrong number of words\";\n+\t\tgoto unparsable;\n+\t}\n+\n+\tref = alloc_ref(words[2]);\n+\tpeer_ref = ref->peer_ref = alloc_ref(words[3]);\n+\tref->peer_ref->force = 1;\n+\n+\tif (get_sha1_hex(words[0], ref->old_sha1)) {\n+\t\tproblem = \"bad sha1\";\n+\t\tgoto unparsable;\n+\t}\n+\n+\tif (!strcmp(words[1], \"merge\")) {\n+\t\tref->merge = 1;\n+\t}\n+\telse if (strcmp(words[1], \"not-for-merge\")) {\n+\t\tproblem = \"bad merge flag\";\n+\t\tgoto unparsable;\n+\t}\n+\n+\tpeer_item = string_list_lookup(existing_refs, peer_ref->name);\n+\tif (peer_item)\n+\t\thashcpy(peer_ref->old_sha1, peer_item->util);\n+\n+\treturn ref;\n+\n+ unparsable:\n+\twarning(\"%s hook output a wrongly formed line: %s\",\n+\t\t\thook->name, problem);\n+\tfree(ref);\n+\tfree(peer_ref);\n+\treturn NULL;\n+}\n+\n+int tweak_fetch_hook_reader(int handle, struct hook *hook)\n+{\n+\tFILE *f;\n+\tstruct strbuf buf;\n+\tstruct string_list existing_refs = STRING_LIST_INIT_NODUP;\n+\tstruct ref *old_refs, *ref, *prevref = NULL;\n+\tint res = 0;\n+\n+\tf = fdopen(handle, \"r\");\n+\tif (f == NULL)\n+\t\treturn 1;\n+\n+\told_refs = hook->data;\n+\thook->data = NULL;\n+\n+\tstrbuf_init(&buf, 128);\n+\tfor_each_ref(add_existing, &existing_refs);\n+\n+\twhile (strbuf_getline(&buf, f, '\\n') != EOF) {\n+\t\tchar *l = strbuf_detach(&buf, NULL);\n+\t\tref = parse_tweak_fetch_hook_line(l, &existing_refs, hook);\n+\t\tif (ref) {\n+\t\t\tif (prevref) {\n+\t\t\t\tprevref->next = ref;\n+\t\t\t\tprevref = ref;\n+\t\t\t} else {\n+\t\t\t\thook->data = prevref = ref;\n+\t\t\t}\n+\t\t} else {\n+\t\t\tres = 1;\n+\t\t}\n+\t\tfree(l);\n+\t}\n+\n+\tstring_list_clear(&existing_refs, 0);\n+\tstrbuf_release(&buf);\n+\tfclose(f);\n+\n+\tif (res)\n+\t\thook->data = old_refs;\n+\telse\n+\t\tfree_refs(old_refs);\n+\n+\treturn res;\n+}\n+\n+/*\n+ * The tweak-fetch hook is fed lines of the form:\n+ * <sha1> SP <not-for-merge|merge> SP <remote-refname> SP <local-refname> LF\n+ * And should output rewritten lines of the same form, which are used\n+ * to generate a tweaked set of fetched_refs.\n+ */\n+static struct ref *run_tweak_fetch_hook(struct ref *fetched_refs)\n+{\n+\tstruct hook hook;\n+\n+\tif (! fetched_refs)\n+\t\treturn fetched_refs;\n+\n+\tmemset(&hook, 0, sizeof(hook));\n+\thook.name = \"tweak-fetch\";\n+\thook.generator = tweak_fetch_hook_generator;\n+\thook.feeder = feed_hook_in_full;\n+\thook.reader = tweak_fetch_hook_reader;\n+\thook.data = fetched_refs;\n+\n+\tif (run_hook_complex(&hook)) {\n+\t\twarning(\"%s hook failed, ignoring its output\", hook.name);\n+\t}\n+\n+\treturn hook.data;\n+}\n+\n static void unlock_pack(void)\n {\n \tif (transport)\n@@ -529,7 +671,7 @@ static struct refs_result fetch_refs(struct transport *transport,\n \tif (res.status)\n \t\tres.status = transport_fetch_refs(transport, ref_map);\n \tif (!res.status) {\n-\t\tres.new_refs = ref_map;\n+\t\tres.new_refs = run_tweak_fetch_hook(ref_map);\n \n \t\tres.status |= store_updated_refs(transport->url,\n \t\t\t\ttransport->remote->name,\n-- \n1.7.7.3\n"},{"id":"181798","messageId":"4EFD88CB.3050403@kdbg.org","threadId":"29268","inReplyTo":"1325207240-22622-2-git-send-email-joey@kitenet.net","subject":"Re: [PATCH 1/3] expanded hook api with stdio support","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-12-30T09:47:55Z","receivedAt":"2011-12-30T09:47:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.12.2011 02:07, schrieb Joey Hess:\n> Finally, the API is designed to be extended in the future, to support\n> running multiple programs for a single hook action (such as the contents\n> of a .git/hooks/hook.d/ , or a system-wide hook). This design goal led\n> to the \"generator\" and \"reader\" members of the struct hook, which are\n> specified such that they can be called repeatedly, with data flowing\n> between them (via the \"data\" member), like this:\n>     generator | hook_prog_1 | reader | generator | hook_prog_2 | reader\n\nIMHO, this is overengineered. I don't think that we need something like\nthis in the foreseeable future, particularly because such a pipeline or\nmulti-hook infrastructure can easily be constructed by the (single) hook\nscript itself.\n\nIOW, none of the three function pointers should be needed (not even the\nfeeder, see below), at least not in a first step.\n\nIMO, as the first step, the user of this infrastructure should only be\nrequired to construct the hook input as a strbuf, and receive the hook\noutput, if needed, also as a strbuf.\n\n> +`run_hook_complex`::\n> +\n> +\tRun a hook, with the caller providing its stdin and/or parsing its\n> +\tstdout.\n> +\tTakes a pointer to a `struct hook` that specifies the details,\n> +\tincluding the name of the hook, any parameters to pass to it,\n> +\tand how to handle the stdin and stdout. (See below.)\n> +\tIf the hook does not exist or is not executable, the return value\n> +\twill be zero.\n> +\tIf it is executable, the hook will be executed and the exit\n> +\tstatus of the hook is returned.\n\nWhat is the rationale for these error modes? It is as if a non-existent\nor non-executable hook counts as 'success'. (I'm not saying that this\nwould be wrong, I'm just asking.)\n\n>  \n>  Data structures\n>  ---------------\n> @@ -241,3 +252,45 @@ a forked process otherwise:\n>  \n>  . It must not change the program's state that the caller of the\n>    facility also uses.\n> +\n> +* `struct hook`\n> +\n> +This describes a hook to run.\n> +\n> +The caller:\n> +\n> +1. allocates and clears (memset(&hook, 0, sizeof(hook));) a\n> +   struct hook variable;\n> +2. initializes the members;\n> +3. calls hook();\n\nrun_hook_complex()?\n\n> +4. if necessary, accesses data read from the hook in .data\n> +5. frees the struct hook.\n\n> +The `feeder` is run asynchronously to feed the generated stdin into the hook.\n> +It is passed the handle to write to, the strbuf containing the stdin, and \n> +a pointer to the `struct hook`, and should return non-zero on failure.\n> +Typically it is set to either `feed_hook_in_full`, or `feed_hook_incomplete`.\n\nIMO, this is overengineered. See below.\n\n> +\tres |= start_command(&child);\n> +\tif (res) goto hook_out;\n\nPlease write this conditional in two lines.\n\n> +/* A feeder that ensures the hook consumes all its stdin. */\n> +int feed_hook_in_full(int handle, struct strbuf *buf, struct hook *hook)\n> +{\n> +\tif (write_in_full(handle, buf->buf, buf->len) != buf->len) {\n> +\t\twarning(\"%s hook failed to consume all its input\", hook->name);\n\nReally? Could there not be any other error condition? The warning would\nbe correct only if errno == EPIPE, and this error will be returned only\nif SIGPIPE is ignored. Does this happen anywhere?\n\nFuthermore, if all data was written to the pipe successfully, there is\nno way to know whether the reader consumed everything.\n\nIOW, I don't it is worth to make the distinction. However, I do think\nthat the parent process must be protected against SIGPIPE.\n\n> +\t\treturn 1;\n> +\t}\n> +\telse\n> +\t\treturn 0;\n> +}\n\n-- Hannes\n"},{"id":"181807","messageId":"20111230171344.GA9667@gnu.kitenet.net","threadId":"29268","inReplyTo":"4EFD88CB.3050403@kdbg.org","subject":"Re: [PATCH 1/3] expanded hook api with stdio support","fromName":"Joey Hess","fromEmail":"joey@kitenet.net","sentAt":"2011-12-30T17:13:44Z","receivedAt":"2011-12-30T17:13:44Z","isPatch":true,"sender":{"key":"joey@kitenet.net","avatar":"https://avatars.githubusercontent.com/u/16392?v=4"},"body":"Johannes Sixt wrote:\n> IMHO, this is overengineered. I don't think that we need something like\n> this in the foreseeable future, particularly because such a pipeline or\n> multi-hook infrastructure can easily be constructed by the (single) hook\n> script itself.\n\nJunio seemed to think this was a good direction to move in and gave some\nexamples in <7vlipz930t.fsf@alter.siamese.dyndns.org>\n\nAnyway, the minimum cases for run_hook_complex() to support are:\n\n* no stdin, no stdout\n* only stdin\n* stdin and stdout (needed for tweak-fetch)\n* only stdout (perhaps)\n\nThe generator and reader members of struct hook allow the caller to\neasily specify which of these cases applies to a hook, and also provides\na natural separation of the caller's stdin generation and stdout parsing\ncode.\n\nThat leaves the feeder and data members of struct hook as possible\noverengineering. See below regarding the feeder. The data member could\nbe eliminated and global variables used by callers that need that,\nbut I prefer designs that don't require global variables.\n\n> > +`run_hook_complex`::\n> > +\n> > +\tRun a hook, with the caller providing its stdin and/or parsing its\n> > +\tstdout.\n> > +\tTakes a pointer to a `struct hook` that specifies the details,\n> > +\tincluding the name of the hook, any parameters to pass to it,\n> > +\tand how to handle the stdin and stdout. (See below.)\n> > +\tIf the hook does not exist or is not executable, the return value\n> > +\twill be zero.\n> > +\tIf it is executable, the hook will be executed and the exit\n> > +\tstatus of the hook is returned.\n> \n> What is the rationale for these error modes? It is as if a non-existent\n> or non-executable hook counts as 'success'. (I'm not saying that this\n> would be wrong, I'm just asking.)\n\nThey are identical to how run_hook already works.\nA non-existant/non-executable hook *is* a valid configuration,\nindeed it's the most likely configuration.\n\n> > +/* A feeder that ensures the hook consumes all its stdin. */\n> > +int feed_hook_in_full(int handle, struct strbuf *buf, struct hook *hook)\n> > +{\n> > +\tif (write_in_full(handle, buf->buf, buf->len) != buf->len) {\n> > +\t\twarning(\"%s hook failed to consume all its input\", hook->name);\n> \n> Really? Could there not be any other error condition? The warning would\n> be correct only if errno == EPIPE, and this error will be returned only\n> if SIGPIPE is ignored. Does this happen anywhere?\n> \n> Futhermore, if all data was written to the pipe successfully, there is\n> no way to know whether the reader consumed everything.\n> \n> IOW, I don't it is worth to make the distinction. However, I do think\n> that the parent process must be protected against SIGPIPE.\n\nYes, this was not thought through, I missed that a write to a pipe can\nsucceed (due to buffering) despite not being fully consumed.\n\nDealing with the hook SIGPIPE issue is complicated as Jeff explains in\n<20111205214351.GA15029@sigill.intra.peff.net>, and I don't feel I'm the\none to do it. I'm feeling like ripping the \"feeder\" stuff out of my\npatch, and not having my patch change the status quo on this issue.\n\n-- \nsee shy jo\n"},{"id":"181808","messageId":"4EFDFD47.2060700@kdbg.org","threadId":"29268","inReplyTo":"20111230171344.GA9667@gnu.kitenet.net","subject":"Re: [PATCH 1/3] expanded hook api with stdio support","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2011-12-30T18:04:55Z","receivedAt":"2011-12-30T18:04:55Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 30.12.2011 18:13, schrieb Joey Hess:\n> Johannes Sixt wrote:\n>> IMHO, this is overengineered. I don't think that we need something like\n>> this in the foreseeable future, particularly because such a pipeline or\n>> multi-hook infrastructure can easily be constructed by the (single) hook\n>> script itself.\n> \n> Junio seemed to think this was a good direction to move in and gave some\n> examples in <7vlipz930t.fsf@alter.siamese.dyndns.org>\n> \n> Anyway, the minimum cases for run_hook_complex() to support are:\n> \n> * no stdin, no stdout\n> * only stdin\n> * stdin and stdout (needed for tweak-fetch)\n> * only stdout (perhaps)\n> \n> The generator and reader members of struct hook allow the caller to\n> easily specify which of these cases applies to a hook, and also provides\n> a natural separation of the caller's stdin generation and stdout parsing\n> code.\n\nBut as long as the generator only needs to generate a strbuf *and* only\none hook is run, there is no value to have it as a callback; the caller\ncan just specify the strbuf itself, run_hook_* does not need to care how\nit was generated.\n\nI can see some value in a reader callback to avoid allocating yet\nanother strbuf.\n\n> ... The data member could\n> be eliminated and global variables used by callers that need that,\n> but I prefer designs that don't require global variables.\n\nAbsolutely.\n\n>>> +\tIf the hook does not exist or is not executable, the return value\n>>> +\twill be zero.\n>>> +\tIf it is executable, the hook will be executed and the exit\n>>> +\tstatus of the hook is returned.\n>>\n>> What is the rationale for these error modes? It is as if a non-existent\n>> or non-executable hook counts as 'success'. (I'm not saying that this\n>> would be wrong, I'm just asking.)\n> \n> They are identical to how run_hook already works.\n> A non-existant/non-executable hook *is* a valid configuration,\n> indeed it's the most likely configuration.\n\nSo, it is so that the caller does not itself have to check whether a\nhook exists. That may be worth a word in the API documentation.\n\n-- Hannes\n"},{"id":"181882","messageId":"7vsjjwtvf1.fsf@alter.siamese.dyndns.org","threadId":"29268","inReplyTo":"4EFD88CB.3050403@kdbg.org","subject":"Re: [PATCH 1/3] expanded hook api with stdio support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-03T19:53:22Z","receivedAt":"2012-01-03T19:53:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt <j6t@kdbg.org> writes:\n\n> IMO, as the first step, the user of this infrastructure should only be\n> required to construct the hook input as a strbuf, and receive the hook\n> output, if needed, also as a strbuf.\n\nNow you brought it up, I think I would agree. The only reason I suggested\na callback feeder approach was because I somehow was hoping that it may be\npossible to share more code with the codepath for textconv that may not\nwant to hold too much buffer in core when we know the data is only used\nsequencially and I wanted to see more things to go through streaming API\nin the future.\n\n>> +`run_hook_complex`::\n\nAlso, I think the updated interface should become the \"run_hook\" function;\nnothing \"complex\" about it. The name \"run_hook()\" was a perfectly fine\nabstraction for what it did when it used to be a static helper function\nwithin builtin-commit.c, but its special-casing of GIT_INDEX_FILE\nenvironment is _not_ general enough to deserve it to be called the\n\"run_hook\" in the global scope.\n\nIOW, I am saying that we screwed up at ae98a00 (Move run_hook() from\nbuiltin-commit.c into run-command.c (libgit), 2009-01-16.\n\nThe environment tweaking should not take a \"index_file\" field in the\nstructure, but an array \"environ\" that is used to tweak the environment\nvariables for the hook process.\n"},{"id":"181886","messageId":"20120103200642.GH20926@sigill.intra.peff.net","threadId":"29268","inReplyTo":"7vsjjwtvf1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/3] expanded hook api with stdio support","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-01-03T20:06:42Z","receivedAt":"2012-01-03T20:06:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 03, 2012 at 11:53:22AM -0800, Junio C Hamano wrote:\n\n> Johannes Sixt <j6t@kdbg.org> writes:\n> \n> > IMO, as the first step, the user of this infrastructure should only be\n> > required to construct the hook input as a strbuf, and receive the hook\n> > output, if needed, also as a strbuf.\n> \n> Now you brought it up, I think I would agree. The only reason I suggested\n> a callback feeder approach was because I somehow was hoping that it may be\n> possible to share more code with the codepath for textconv that may not\n> want to hold too much buffer in core when we know the data is only used\n> sequencially and I wanted to see more things to go through streaming API\n> in the future.\n\nEven if we don't make the input streaming, it would be nice to factor\nthe concept of \"feed input to program and read its output without\ndeadlocking\" into something independent of hooks.\n\nThe credential helper code could potentially have the same deadlock.\nPossibly also clean/smudge filters.\n\nMaybe it could even be part of the run-command interface?\n\n-Peff\n"},{"id":"181896","messageId":"7vliposboy.fsf@alter.siamese.dyndns.org","threadId":"29268","inReplyTo":"20120103200642.GH20926@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] expanded hook api with stdio support","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-01-03T21:44:45Z","receivedAt":"2012-01-03T21:44:45Z","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> The credential helper code could potentially have the same deadlock.\n> Possibly also clean/smudge filters.\n>\n> Maybe it could even be part of the run-command interface?\n\nHmm, that smells like the right thing to do.\n"}]}