{"thread":{"id":"44193","subject":"[PATCH 0/6] receive-pack: quarantine pushed objects","startedAt":"2016-09-30T19:35:40Z","lastAt":"2017-04-10T21:14:37Z","messageCount":32,"participants":["Jeff King","Junio C Hamano","David Turner","René Scharfe","Christian Couder","Jakub Narębski","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"303006","messageId":"20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net","threadId":"44193","inReplyTo":null,"subject":"[PATCH 0/6] receive-pack: quarantine pushed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-30T19:35:34Z","receivedAt":"2016-09-30T19:35:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I've mentioned before on the list that GitHub \"quarantines\" objects\nwhile the pre-receive hook runs. Here are the patches to implement\nthat.\n\nThe basic problem is that as-is, index-pack admits pushed objects into\nthe main object database immediately, before the pre-receive hook runs.\nIt _has_ to, since the hook needs to be able to actually look at the\nobjects. However, this means that if the pre-receive hook rejects the\npush, we still end up with the objects in the repository. We can't just\ndelete them as temporary files, because we don't know what other\nprocesses might have started referencing them.\n\nThe solution here is to push into a \"quarantine\" directory that is\naccessible only to pre-receive, check_connected(), etc, and only\nmove the objects into the main object database after we've finished\nthose basic checks.\n\nOne of the things we use it for at GitHub is object-size policy, which\nwe implement via a pre-receive hook (sort of; see below). This scheme\nhas been in use for about 2 years, though I did do a fair bit of\ntweaking to make it ready for upstream (squashing bugfixes and merges\nfrom upstream that came later, along with polishing a few rough edges I\nsaw while doing so). So I may have introduced new bugs. :)\n\nThe patches are:\n\n  [1/6]: check_connected: accept an env argument\n  [2/6]: sha1_file: always allow relative paths to alternates\n\n    These two are preparatory.\n\n  [3/6]: tmp-objdir: introduce API for temporary object directories\n  [4/6]: receive-pack: quarantine objects until pre-receive accepts\n\n    This is the interesting part.\n\n  [5/6]: tmp-objdir: put quarantine information in the environment\n  [6/6]: tmp-objdir: do not migrate files starting with '.'\n\n    These are two changes that I ended up doing later to support another\n    series. They're not strictly necessary here, but I think they're\n    worth including now, as they change the visible behavior in minor\n    ways. It seems like a good idea to start with what I think should be\n    the final behavior.\n\n    The other series is basically an optimization for the object-size\n    policy. Without it, you are stuck walking the graph again in the\n    pre-receive hook to find the new objects and check their sizes.\n\n    But index-pack can do that for you very cheaply; it has the size of\n    each object already. But it _doesn't_ produce nice error messages;\n    it has no idea at what path the objects are found, and it doesn't\n    know what kind of advice it should give the user.\n\n    So what we can do is ask index-pack to make a note of any objects\n    larger than N bytes, and write their sha1 and size into a file in\n    the quarantine path. Then the pre-receive hook can look in that log\n    and generate any nice message it wants. In the common case, the log\n    is empty, and it does not have to do any work at all.\n\n    These two patches set that up by letting index-pack and pre-receive\n    know that quarantine path and use it to store arbitrary files that\n    _don't_ get migrated to the main object database (i.e., the log file\n    mentioned above).\n\n-Peff\n"},{"id":"303007","messageId":"20160930193557.q6dtz3wcnui4ndrr@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net","subject":"[PATCH 1/6] check_connected: accept an env argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-30T19:35:57Z","receivedAt":"2016-09-30T19:36:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This lets callers influence the environment seen by\nrev-list, which will be useful when we start providing\nquarantined objects.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n connected.c | 1 +\n connected.h | 5 +++++\n 2 files changed, 6 insertions(+)\n\ndiff --git a/connected.c b/connected.c\nindex 8e3e4b1..136c2ac 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -63,6 +63,7 @@ int check_connected(sha1_iterate_fn fn, void *cb_data,\n \t\t\t\t _(\"Checking connectivity\"));\n \n \trev_list.git_cmd = 1;\n+\trev_list.env = opt->env;\n \trev_list.in = -1;\n \trev_list.no_stdout = 1;\n \tif (opt->err_fd)\ndiff --git a/connected.h b/connected.h\nindex afa48cc..4ca325f 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -33,6 +33,11 @@ struct check_connected_options {\n \n \t/* If non-zero, show progress as we traverse the objects. */\n \tint progress;\n+\n+\t/*\n+\t * Insert these variables into the environment of the child process.\n+\t */\n+\tconst char **env;\n };\n \n #define CHECK_CONNECTED_INIT { 0 }\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303008","messageId":"20160930193604.hqspizasfpqa6bez@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net","subject":"[PATCH 2/6] sha1_file: always allow relative paths to alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-30T19:36:05Z","receivedAt":"2016-09-30T19:36:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We recursively expand alternates repositories, so that if A\nborrows from B which borrows from C, A can see all objects.\n\nFor the root object database, we allow relative paths, so A\ncan point to B as \"../B/objects\". However, we currently do\nnot allow relative paths when recursing, so B must use an\nabsolute path to reach C.\n\nThat is an ancient protection from c2f493a (Transitively\nread alternatives, 2006-05-07) that tries to avoid adding\nthe same alternate through two different paths. But since\n5bdf0a8 (sha1_file: normalize alt_odb path before comparing\nand storing, 2011-09-07), we use a normalized absolute path\nfor each alt_odb entry.\n\nSo this protection is no longer necessary; we will detect\nthe duplicate no matter how we got there.  And it's a good\nidea to get rid of it, as it creates an unnecessary\ncomplication when setting up recursive alternates (B has to\nknow that A is going to borrow from it and make sure to use\nan absolute path).\n\nWe adjust the test script here to demonstrate that this now\nworks. Unfortunately, we can't demonstrate that the\nduplicate is suppressed, since it has no user-visible\nbehavior (it's just one less place for our object lookups to\ngo). But you can verify it manually via gdb, with something\nlike:\n\n    for i in a b c; do\n            git init --bare $i\n            blob=$(echo $i | git -C $i hash-object -w --stdin)\n    done\n    echo \"../../b/objects\" >a/objects/info/alternates\n    echo \"../../c/objects\" >>a/objects/info/alternates\n    echo \"../../c/objects\" >b/objects/info/alternates\n    gdb --args git cat-file -e $blob\n\nAfter prepare_alt_odb() runs, we have only a single copy of\n\"/path/to/c/objects/\" in the alt_odb list.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n sha1_file.c               | 7 +------\n t/t5613-info-alternate.sh | 4 ++--\n 2 files changed, 3 insertions(+), 8 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex b9c1fa3..9a79c19 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -343,12 +343,7 @@ static void link_alt_odb_entries(const char *alt, int len, int sep,\n \t\tconst char *entry = entries.items[i].string;\n \t\tif (entry[0] == '\\0' || entry[0] == '#')\n \t\t\tcontinue;\n-\t\tif (!is_absolute_path(entry) && depth) {\n-\t\t\terror(\"%s: ignoring relative alternate object store %s\",\n-\t\t\t\t\trelative_base, entry);\n-\t\t} else {\n-\t\t\tlink_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);\n-\t\t}\n+\t\tlink_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);\n \t}\n \tstring_list_clear(&entries, 0);\n \tfree(alt_copy);\ndiff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh\nindex 9cd2626..b429707 100755\n--- a/t/t5613-info-alternate.sh\n+++ b/t/t5613-info-alternate.sh\n@@ -102,9 +102,9 @@ test_valid_repo'\n cd \"$base_dir\"\n \n test_expect_success \\\n-    'that relative alternate is only possible for current dir' '\n+    'that relative alternate is recursive' '\n     cd D &&\n-    ! (test_valid_repo)\n+    test_valid_repo\n '\n \n cd \"$base_dir\"\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303009","messageId":"20160930193613.dwpjiw5xps6a3wgj@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net","subject":"[PATCH 3/6] tmp-objdir: introduce API for temporary object directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-30T19:36:13Z","receivedAt":"2016-09-30T19:36:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once objects are added to the object database by a process,\nthey cannot easily be deleted, as we don't know what other\nprocesses may have started referencing them. We have to\nclean them up with git-gc, which will apply the usual\nreachability and grace-period checks.\n\nThis patch provides an alternative: it helps callers create\na temporary directory inside the object directory, and a\ntemporary environment which can be passed to sub-programs to\nask them to write there (the original object directory\nremains accessible as an alternate of the temporary one).\n\nSee tmp-objdir.h for details on the API.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Makefile     |   1 +\n cache.h      |   1 +\n sha1_file.c  |   6 ++\n tmp-objdir.c | 266 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n tmp-objdir.h |  53 ++++++++++++\n 5 files changed, 327 insertions(+)\n create mode 100644 tmp-objdir.c\n create mode 100644 tmp-objdir.h\n\ndiff --git a/Makefile b/Makefile\nindex 1aad150..4e3becb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -831,6 +831,7 @@ LIB_OBJS += submodule-config.o\n LIB_OBJS += symlinks.o\n LIB_OBJS += tag.o\n LIB_OBJS += tempfile.o\n+LIB_OBJS += tmp-objdir.o\n LIB_OBJS += trace.o\n LIB_OBJS += trailer.o\n LIB_OBJS += transport.o\ndiff --git a/cache.h b/cache.h\nindex ed3d5df..607c9b5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1389,6 +1389,7 @@ extern void prepare_alt_odb(void);\n extern void read_info_alternates(const char * relative_base, int depth);\n extern char *compute_alternate_path(const char *path, struct strbuf *err);\n extern void add_to_alternates_file(const char *reference);\n+extern void add_to_alternates_internal(const char *reference);\n typedef int alt_odb_fn(struct alternate_object_database *, void *);\n extern int foreach_alt_odb(alt_odb_fn, void*);\n \ndiff --git a/sha1_file.c b/sha1_file.c\nindex 9a79c19..65deaf9 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -421,6 +421,12 @@ void add_to_alternates_file(const char *reference)\n \tfree(alts);\n }\n \n+void add_to_alternates_internal(const char *reference)\n+{\n+\tprepare_alt_odb();\n+\tlink_alt_odb_entries(reference, strlen(reference), '\\n', NULL, 0);\n+}\n+\n /*\n  * Compute the exact path an alternate is at and returns it. In case of\n  * error NULL is returned and the human readable error is added to `err`\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nnew file mode 100644\nindex 0000000..c92e6cc\n--- /dev/null\n+++ b/tmp-objdir.c\n@@ -0,0 +1,266 @@\n+#include \"cache.h\"\n+#include \"tmp-objdir.h\"\n+#include \"dir.h\"\n+#include \"sigchain.h\"\n+#include \"string-list.h\"\n+#include \"strbuf.h\"\n+#include \"argv-array.h\"\n+\n+struct tmp_objdir {\n+\tstruct strbuf path;\n+\tstruct argv_array env;\n+};\n+\n+/*\n+ * Allow only one tmp_objdir at a time in a running process, which simplifies\n+ * our signal/atexit cleanup routines.  It's doubtful callers will ever need\n+ * more than one, and we can expand later if so.  You can have many such\n+ * tmp_objdirs simultaneously in many processes, of course.\n+ */\n+struct tmp_objdir *the_tmp_objdir;\n+\n+static void tmp_objdir_free(struct tmp_objdir *t)\n+{\n+\tstrbuf_release(&t->path);\n+\targv_array_clear(&t->env);\n+\tfree(t);\n+}\n+\n+static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n+{\n+\tint err;\n+\n+\tif (!t)\n+\t\treturn 0;\n+\n+\tif (t == the_tmp_objdir)\n+\t\tthe_tmp_objdir = NULL;\n+\n+\t/*\n+\t * This may use malloc via strbuf_grow(), but we should\n+\t * have pre-grown t->path sufficiently so that this\n+\t * doesn't happen in practice.\n+\t */\n+\terr = remove_dir_recursively(&t->path, 0);\n+\n+\t/*\n+\t * When we are cleaning up due to a signal, we won't bother\n+\t * freeing memory; it may cause a deadlock if the signal\n+\t * arrived while libc's allocator lock is held.\n+\t */\n+\tif (!on_signal)\n+\t\ttmp_objdir_free(t);\n+\treturn err;\n+}\n+\n+int tmp_objdir_destroy(struct tmp_objdir *t)\n+{\n+\treturn tmp_objdir_destroy_1(t, 0);\n+}\n+\n+static void remove_tmp_objdir(void)\n+{\n+\ttmp_objdir_destroy(the_tmp_objdir);\n+}\n+\n+static void remove_tmp_objdir_on_signal(int signo)\n+{\n+\ttmp_objdir_destroy_1(the_tmp_objdir, 1);\n+\tsigchain_pop(signo);\n+\traise(signo);\n+}\n+\n+static void env_append(struct argv_array *env, const char *key, const char *val)\n+{\n+\tconst char *old = getenv(key);\n+\n+\tif (!old)\n+\t\targv_array_pushf(env, \"%s=%s\", key, val);\n+\telse\n+\t\targv_array_pushf(env, \"%s=%s%c%s\", key, old, PATH_SEP, val);\n+}\n+\n+static void env_replace(struct argv_array *env, const char *key, const char *val)\n+{\n+\targv_array_pushf(env, \"%s=%s\", key, val);\n+}\n+\n+static int setup_tmp_objdir(const char *root)\n+{\n+\tchar *path;\n+\tint ret = 0;\n+\n+\tpath = xstrfmt(\"%s/pack\", root);\n+\tret = mkdir(path, 0777);\n+\tfree(path);\n+\n+\treturn ret;\n+}\n+\n+struct tmp_objdir *tmp_objdir_create(void)\n+{\n+\tstatic int installed_handlers;\n+\tstruct tmp_objdir *t;\n+\n+\tif (the_tmp_objdir)\n+\t\tdie(\"BUG: only one tmp_objdir can be used at a time\");\n+\n+\tt = xmalloc(sizeof(*t));\n+\tstrbuf_init(&t->path, 0);\n+\targv_array_init(&t->env);\n+\n+\tstrbuf_addf(&t->path, \"%s/incoming-XXXXXX\", get_object_directory());\n+\n+\t/*\n+\t * Grow the strbuf beyond any filename we expect to be placed in it.\n+\t * If tmp_objdir_destroy() is called by a signal handler, then\n+\t * we should be able to use the strbuf to remove files without\n+\t * having to call malloc.\n+\t */\n+\tstrbuf_grow(&t->path, 1024);\n+\n+\tif (!mkdtemp(t->path.buf)) {\n+\t\t/* free, not destroy, as we never touched the filesystem */\n+\t\ttmp_objdir_free(t);\n+\t\treturn NULL;\n+\t}\n+\n+\tthe_tmp_objdir = t;\n+\tif (!installed_handlers) {\n+\t\tatexit(remove_tmp_objdir);\n+\t\tsigchain_push_common(remove_tmp_objdir_on_signal);\n+\t\tinstalled_handlers++;\n+\t}\n+\n+\tif (setup_tmp_objdir(t->path.buf)) {\n+\t\ttmp_objdir_destroy(t);\n+\t\treturn NULL;\n+\t}\n+\n+\tenv_append(&t->env, ALTERNATE_DB_ENVIRONMENT,\n+\t\t   absolute_path(get_object_directory()));\n+\tenv_replace(&t->env, DB_ENVIRONMENT, absolute_path(t->path.buf));\n+\n+\treturn t;\n+}\n+\n+/*\n+ * Make sure we copy packfiles and their associated metafiles in the correct\n+ * order. All of these ends_with checks are slightly expensive to do in\n+ * the midst of a sorting routine, but in practice it shouldn't matter.\n+ * We will have a relatively small number of packfiles to order, and loose\n+ * objects exit early in the first line.\n+ */\n+static int pack_copy_priority(const char *name)\n+{\n+\tif (!starts_with(name, \"pack\"))\n+\t\treturn 0;\n+\tif (ends_with(name, \".keep\"))\n+\t\treturn 1;\n+\tif (ends_with(name, \".pack\"))\n+\t\treturn 2;\n+\tif (ends_with(name, \".idx\"))\n+\t\treturn 3;\n+\treturn 4;\n+}\n+\n+static int pack_copy_cmp(const char *a, const char *b)\n+{\n+\treturn pack_copy_priority(a) - pack_copy_priority(b);\n+}\n+\n+static int read_dir_paths(struct string_list *out, const char *path)\n+{\n+\tDIR *dh;\n+\tstruct dirent *de;\n+\n+\tdh = opendir(path);\n+\tif (!dh)\n+\t\treturn -1;\n+\n+\twhile ((de = readdir(dh)))\n+\t\tif (!is_dot_or_dotdot(de->d_name))\n+\t\t\tstring_list_append(out, de->d_name);\n+\n+\tclosedir(dh);\n+\treturn 0;\n+}\n+\n+static int migrate_paths(struct strbuf *src, struct strbuf *dst);\n+\n+static int migrate_one(struct strbuf *src, struct strbuf *dst)\n+{\n+\tstruct stat st;\n+\n+\tif (stat(src->buf, &st) < 0)\n+\t\treturn -1;\n+\tif (S_ISDIR(st.st_mode)) {\n+\t\tif (!mkdir(dst->buf, 0777)) {\n+\t\t\tif (adjust_shared_perm(dst->buf))\n+\t\t\t\treturn -1;\n+\t\t} else if (errno != EEXIST)\n+\t\t\treturn -1;\n+\t\treturn migrate_paths(src, dst);\n+\t}\n+\treturn finalize_object_file(src->buf, dst->buf);\n+}\n+\n+static int migrate_paths(struct strbuf *src, struct strbuf *dst)\n+{\n+\tsize_t src_len = src->len, dst_len = dst->len;\n+\tstruct string_list paths = STRING_LIST_INIT_DUP;\n+\tint i;\n+\tint ret = 0;\n+\n+\tif (read_dir_paths(&paths, src->buf) < 0)\n+\t\treturn -1;\n+\tpaths.cmp = pack_copy_cmp;\n+\tstring_list_sort(&paths);\n+\n+\tfor (i = 0; i < paths.nr; i++) {\n+\t\tconst char *name = paths.items[i].string;\n+\n+\t\tstrbuf_addf(src, \"/%s\", name);\n+\t\tstrbuf_addf(dst, \"/%s\", name);\n+\n+\t\tret |= migrate_one(src, dst);\n+\n+\t\tstrbuf_setlen(src, src_len);\n+\t\tstrbuf_setlen(dst, dst_len);\n+\t}\n+\n+\tstring_list_clear(&paths, 0);\n+\treturn ret;\n+}\n+\n+int tmp_objdir_migrate(struct tmp_objdir *t)\n+{\n+\tstruct strbuf src = STRBUF_INIT, dst = STRBUF_INIT;\n+\tint ret;\n+\n+\tif (!t)\n+\t\treturn 0;\n+\n+\tstrbuf_addbuf(&src, &t->path);\n+\tstrbuf_addstr(&dst, get_object_directory());\n+\n+\tret = migrate_paths(&src, &dst);\n+\n+\tstrbuf_release(&src);\n+\tstrbuf_release(&dst);\n+\n+\ttmp_objdir_destroy(t);\n+\treturn ret;\n+}\n+\n+const char **tmp_objdir_env(const struct tmp_objdir *t)\n+{\n+\tif (!t)\n+\t\treturn NULL;\n+\treturn t->env.argv;\n+}\n+\n+void tmp_objdir_add_as_alternate(const struct tmp_objdir *t)\n+{\n+\tadd_to_alternates_internal(t->path.buf);\n+}\ndiff --git a/tmp-objdir.h b/tmp-objdir.h\nnew file mode 100644\nindex 0000000..aa47aa9\n--- /dev/null\n+++ b/tmp-objdir.h\n@@ -0,0 +1,53 @@\n+#ifndef TMP_OBJDIR_H\n+#define TMP_OBJDIR_H\n+\n+/*\n+ * This API allows you to create a temporary object directory, advertise it to\n+ * sub-processes via GIT_OBJECT_DIRECTORY and GIT_ALTERNATE_OBJECT_DIRECTORIES,\n+ * and then either migrate its object into the main object directory, or remove\n+ * it. The library handles unexpected signal/exit death by cleaning up the\n+ * temporary directory.\n+ *\n+ * Example:\n+ *\n+ *\tstruct tmp_objdir *t = tmp_objdir_create();\n+ *\tif (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&\n+ *\t    !tmp_objdir_migrate(t))\n+ *\t\tprintf(\"success!\\n\");\n+ *\telse\n+ *\t\tdie(\"failed...tmp_objdir will clean up for us\");\n+ *\n+ */\n+\n+struct tmp_objdir;\n+\n+/*\n+ * Create a new temporary object directory; returns NULL on failure.\n+ */\n+struct tmp_objdir *tmp_objdir_create(void);\n+\n+/*\n+ * Return a list of environment strings, suitable for use with\n+ * child_process.env, that can be passed to child programs to make use of the\n+ * temporary object directory.\n+ */\n+const char **tmp_objdir_env(const struct tmp_objdir *);\n+\n+/*\n+ * Finalize a temporary object directory by migrating its objects into the main\n+ * object database.\n+ */\n+int tmp_objdir_migrate(struct tmp_objdir *);\n+\n+/*\n+ * Destroy a temporary object directory, discarding any objects it contains.\n+ */\n+int tmp_objdir_destroy(struct tmp_objdir *);\n+\n+/*\n+ * Add the temporary object directory as an alternate object store in the\n+ * current process.\n+ */\n+void tmp_objdir_add_as_alternate(const struct tmp_objdir *);\n+\n+#endif /* TMP_OBJDIR_H */\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303010","messageId":"20160930193630.vwq6n5hjbthrz6fj@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net","subject":"[PATCH 4/6] receive-pack: quarantine objects until pre-receive accepts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-30T19:36:31Z","receivedAt":"2016-09-30T19:36:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a client pushes objects to us, index-pack checks the\nobjects themselves and then installs them into place. If we\nthen reject the push due to a pre-receive hook, we cannot\njust delete the packfile; other processes may be depending\non it. We have to do a normal reachability check at this\npoint via `git gc`.\n\nBut such objects may hang around for weeks due to the\ngc.pruneExpire grace period. And worse, during that time\nthey may be exploded from the pack into inefficient loose\nobjects.\n\nInstead, this patch teaches receive-pack to put the new\nobjects into a \"quarantine\" temporary directory. We make\nthese objects available to the connectivity check and to the\npre-receive hook, and then install them into place only if\nit is successful (and otherwise remove them as tempfiles).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/receive-pack.c     | 41 ++++++++++++++++++++++++++++++++++++++++-\n t/t5547-push-quarantine.sh | 36 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 76 insertions(+), 1 deletion(-)\n create mode 100755 t/t5547-push-quarantine.sh\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 896b16f..04ad909 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -20,6 +20,7 @@\n #include \"gpg-interface.h\"\n #include \"sigchain.h\"\n #include \"fsck.h\"\n+#include \"tmp-objdir.h\"\n \n static const char * const receive_pack_usage[] = {\n \tN_(\"git receive-pack <git-dir>\"),\n@@ -86,6 +87,8 @@ static enum {\n } use_keepalive;\n static int keepalive_in_sec = 5;\n \n+struct tmp_objdir *tmp_objdir;\n+\n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n \tif (value) {\n@@ -663,6 +666,9 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \t} else\n \t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT\");\n \n+\tif (tmp_objdir)\n+\t\targv_array_pushv(&proc.env_array, tmp_objdir_env(tmp_objdir));\n+\n \tif (use_sideband) {\n \t\tmemset(&muxer, 0, sizeof(muxer));\n \t\tmuxer.proc = copy_to_sideband;\n@@ -762,6 +768,7 @@ static int run_update_hook(struct command *cmd)\n \tproc.stdout_to_stderr = 1;\n \tproc.err = use_sideband ? -1 : 0;\n \tproc.argv = argv;\n+\tproc.env = tmp_objdir_env(tmp_objdir);\n \n \tcode = start_command(&proc);\n \tif (code)\n@@ -833,6 +840,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \t\t    !delayed_reachability_test(si, i))\n \t\t\tsha1_array_append(&extra, si->shallow->sha1[i]);\n \n+\topt.env = tmp_objdir_env(tmp_objdir);\n \tsetup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);\n \tif (check_connected(command_singleton_iterator, cmd, &opt)) {\n \t\trollback_lock_file(&shallow_lock);\n@@ -1240,12 +1248,17 @@ static void set_connectivity_errors(struct command *commands,\n \n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tstruct command *singleton = cmd;\n+\t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n+\n \t\tif (shallow_update && si->shallow_ref[cmd->index])\n \t\t\t/* to be checked in update_shallow_ref() */\n \t\t\tcontinue;\n+\n+\t\topt.env = tmp_objdir_env(tmp_objdir);\n \t\tif (!check_connected(command_singleton_iterator, &singleton,\n-\t\t\t\t     NULL))\n+\t\t\t\t     &opt))\n \t\t\tcontinue;\n+\n \t\tcmd->error_string = \"missing necessary objects\";\n \t}\n }\n@@ -1428,6 +1441,7 @@ static void execute_commands(struct command *commands,\n \tdata.si = si;\n \topt.err_fd = err_fd;\n \topt.progress = err_fd && !quiet;\n+\topt.env = tmp_objdir_env(tmp_objdir);\n \tif (check_connected(iterate_receive_command_list, &data, &opt))\n \t\tset_connectivity_errors(commands, si);\n \n@@ -1444,6 +1458,19 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\t/*\n+\t * Now we'll start writing out refs, which means the objects need\n+\t * to be in their final positions so that other processes can see them.\n+\t */\n+\tif (tmp_objdir_migrate(tmp_objdir) < 0) {\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (!cmd->error_string)\n+\t\t\t\tcmd->error_string = \"unable to migrate objects to permanent storage\";\n+\t\t}\n+\t\treturn;\n+\t}\n+\ttmp_objdir = NULL;\n+\n \tcheck_aliased_updates(commands);\n \n \tfree(head_name_to_free);\n@@ -1639,6 +1666,18 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\targv_array_push(&child.args, alt_shallow_file);\n \t}\n \n+\ttmp_objdir = tmp_objdir_create();\n+\tif (!tmp_objdir)\n+\t\treturn \"unable to create temporary object directory\";\n+\tchild.env = tmp_objdir_env(tmp_objdir);\n+\n+\t/*\n+\t * Normally we just pass the tmp_objdir environment to the child\n+\t * processes that do the heavy lifting, but we may need to see these\n+\t * objects ourselves to set up shallow information.\n+\t */\n+\ttmp_objdir_add_as_alternate(tmp_objdir);\n+\n \tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n \t\targv_array_pushl(&child.args, \"unpack-objects\", hdr_arg, NULL);\n \t\tif (quiet)\ndiff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh\nnew file mode 100755\nindex 0000000..1e5d32d\n--- /dev/null\n+++ b/t/t5547-push-quarantine.sh\n@@ -0,0 +1,36 @@\n+#!/bin/sh\n+\n+test_description='check quarantine of objects during push'\n+. ./test-lib.sh\n+\n+test_expect_success 'create picky dest repo' '\n+\tgit init --bare dest.git &&\n+\twrite_script dest.git/hooks/pre-receive <<-\\EOF\n+\twhile read old new ref; do\n+\t\ttest \"$(git log -1 --format=%s $new)\" = reject && exit 1\n+\tdone\n+\texit 0\n+\tEOF\n+'\n+\n+test_expect_success 'accepted objects work' '\n+\ttest_commit ok &&\n+\tgit push dest.git HEAD &&\n+\tcommit=$(git rev-parse HEAD) &&\n+\tgit --git-dir=dest.git cat-file commit $commit\n+'\n+\n+test_expect_success 'rejected objects are not installed' '\n+\ttest_commit reject &&\n+\tcommit=$(git rev-parse HEAD) &&\n+\ttest_must_fail git push dest.git reject &&\n+\ttest_must_fail git --git-dir=dest.git cat-file commit $commit\n+'\n+\n+test_expect_success 'rejected objects are removed' '\n+\techo \"incoming-*\" >expect &&\n+\t(cd dest.git/objects && echo incoming-*) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303011","messageId":"20160930193638.lybw4idzwersile7@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net","subject":"[PATCH 5/6] tmp-objdir: put quarantine information in the environment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-30T19:36:38Z","receivedAt":"2016-09-30T19:36:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The presence of the GIT_QUARANTINE_PATH variable lets any\ncalled programs know that they're operating in a temporary\nobject directory (and where that directory is).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h      | 1 +\n tmp-objdir.c | 2 ++\n 2 files changed, 3 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 607c9b5..fd81a6c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -433,6 +433,7 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_GLOB_PATHSPECS_ENVIRONMENT \"GIT_GLOB_PATHSPECS\"\n #define GIT_NOGLOB_PATHSPECS_ENVIRONMENT \"GIT_NOGLOB_PATHSPECS\"\n #define GIT_ICASE_PATHSPECS_ENVIRONMENT \"GIT_ICASE_PATHSPECS\"\n+#define GIT_QUARANTINE_ENVIRONMENT \"GIT_QUARANTINE_PATH\"\n \n /*\n  * This environment variable is expected to contain a boolean indicating\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex c92e6cc..9f53238 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -140,6 +140,8 @@ struct tmp_objdir *tmp_objdir_create(void)\n \tenv_append(&t->env, ALTERNATE_DB_ENVIRONMENT,\n \t\t   absolute_path(get_object_directory()));\n \tenv_replace(&t->env, DB_ENVIRONMENT, absolute_path(t->path.buf));\n+\tenv_replace(&t->env, GIT_QUARANTINE_ENVIRONMENT,\n+\t\t    absolute_path(t->path.buf));\n \n \treturn t;\n }\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303012","messageId":"20160930193641.gkpcuxbbc7bei67h@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net","subject":"[PATCH 6/6] tmp-objdir: do not migrate files starting with '.'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-30T19:36:42Z","receivedAt":"2016-09-30T19:36:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This avoids \".\" and \"..\", as we already do, but also leaves\nroom for index-pack to store extra data in the quarantine\narea (e.g., for passing back any analysis to be read by the\npre-receive hook).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n tmp-objdir.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex 9f53238..2181a42 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -181,7 +181,7 @@ static int read_dir_paths(struct string_list *out, const char *path)\n \t\treturn -1;\n \n \twhile ((de = readdir(dh)))\n-\t\tif (!is_dot_or_dotdot(de->d_name))\n+\t\tif (de->d_name[0] != '.')\n \t\t\tstring_list_append(out, de->d_name);\n \n \tclosedir(dh);\n-- \n2.10.0.618.g82cc264\n"},{"id":"303018","messageId":"xmqqponl84h4.fsf@gitster.mtv.corp.google.com","threadId":"44193","inReplyTo":"20160930193613.dwpjiw5xps6a3wgj@sigill.intra.peff.net","subject":"Re: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-09-30T21:25:43Z","receivedAt":"2016-09-30T21:25:51Z","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> diff --git a/sha1_file.c b/sha1_file.c\n> index 9a79c19..65deaf9 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -421,6 +421,12 @@ void add_to_alternates_file(const char *reference)\n>  \tfree(alts);\n>  }\n>  \n> +void add_to_alternates_internal(const char *reference)\n> +{\n> +\tprepare_alt_odb();\n> +\tlink_alt_odb_entries(reference, strlen(reference), '\\n', NULL, 0);\n> +}\n> +\n\nA function _internal being extern felt a bit funny.  We are only\nappending so the first one does not have to be reprepare.\n\n> +static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n> +{\n> +\tint err;\n> +\n> +\tif (!t)\n> +\t\treturn 0;\n> +\n> +\tif (t == the_tmp_objdir)\n> +\t\tthe_tmp_objdir = NULL;\n> +\n> +\t/*\n> +\t * This may use malloc via strbuf_grow(), but we should\n> +\t * have pre-grown t->path sufficiently so that this\n> +\t * doesn't happen in practice.\n> +\t */\n> +\terr = remove_dir_recursively(&t->path, 0);\n> +\n> +\t/*\n> +\t * When we are cleaning up due to a signal, we won't bother\n> +\t * freeing memory; it may cause a deadlock if the signal\n> +\t * arrived while libc's allocator lock is held.\n> +\t */\n> +\tif (!on_signal)\n> +\t\ttmp_objdir_free(t);\n> +\treturn err;\n> +}\n> +\n> +int tmp_objdir_destroy(struct tmp_objdir *t)\n> +{\n> +\treturn tmp_objdir_destroy_1(t, 0);\n> +}\n\nLooks sensible.\n\n> +\tt = xmalloc(sizeof(*t));\n> +\tstrbuf_init(&t->path, 0);\n> +\targv_array_init(&t->env);\n> +\n> +\tstrbuf_addf(&t->path, \"%s/incoming-XXXXXX\", get_object_directory());\n\nI was wondering where you would put this in.  Inside .git/objects/\nsounds good.\n\n> +/*\n> + * Make sure we copy packfiles and their associated metafiles in the correct\n> + * order. All of these ends_with checks are slightly expensive to do in\n> + * the midst of a sorting routine, but in practice it shouldn't matter.\n> + * We will have a relatively small number of packfiles to order, and loose\n> + * objects exit early in the first line.\n> + */\n> +static int pack_copy_priority(const char *name)\n> +{\n> +\tif (!starts_with(name, \"pack\"))\n> +\t\treturn 0;\n> +\tif (ends_with(name, \".keep\"))\n> +\t\treturn 1;\n> +\tif (ends_with(name, \".pack\"))\n> +\t\treturn 2;\n> +\tif (ends_with(name, \".idx\"))\n> +\t\treturn 3;\n> +\treturn 4;\n> +}\n\nThanks for being careful.  A blind \"cp -r\" would have ruined the\nday.\n\nWe do not do bitmaps upon receiving, I guess.\n\n> + *\tstruct tmp_objdir *t = tmp_objdir_create();\n> + *\tif (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&\n> + *\t    !tmp_objdir_migrate(t))\n> + *\t\tprintf(\"success!\\n\");\n> + *\telse\n> + *\t\tdie(\"failed...tmp_objdir will clean up for us\");\n\nMade me briefly wonder if a caller might want to use appropriate\nenvironment to use the tmp-objdir given by the API in addition to\nits own, but then such a caller just needs to prepare its own argv-array\nand concatenate tmp_objdir_env() before making the opt_cd_env call,\nso this is perfectly fine.\n\n"},{"id":"303019","messageId":"b7136f0690244aee94ff2db726a7edce@exmbdft7.ad.twosigma.com","threadId":"44193","inReplyTo":"20160930193613.dwpjiw5xps6a3wgj@sigill.intra.peff.net","subject":"RE: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-09-30T21:32:04Z","receivedAt":"2016-09-30T21:38:22Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"> +static void env_append(struct argv_array *env, const char *key, const\n> +char *val) {\n> +\tconst char *old = getenv(key);\n> +\n> +\tif (!old)\n> +\t\targv_array_pushf(env, \"%s=%s\", key, val);\n> +\telse\n> +\t\targv_array_pushf(env, \"%s=%s%c%s\", key, old, PATH_SEP,\n> val); \n>+}\n\nI would like a comment explaining this function. \n\n> + * Finalize a temporary object directory by migrating its objects into\n> +the main\n> + * object database.\n> + */\n\nThis should mention that it frees its argument.\n\n"},{"id":"303021","messageId":"20160930221319.a6dwhqarcj4v7vwu@sigill.intra.peff.net","threadId":"44193","inReplyTo":"xmqqponl84h4.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-30T22:13:19Z","receivedAt":"2016-09-30T22:13:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 30, 2016 at 02:25:43PM -0700, Junio C Hamano wrote:\n\n> > +void add_to_alternates_internal(const char *reference)\n> > +{\n> > +\tprepare_alt_odb();\n> > +\tlink_alt_odb_entries(reference, strlen(reference), '\\n', NULL, 0);\n> > +}\n> > +\n> \n> A function _internal being extern felt a bit funny.  We are only\n> appending so the first one does not have to be reprepare.\n\nIt's a match for add_to_alternates_file(). Suggestions for a better word\nare welcome.\n\nWe do need to prepare_alt_odb(), as that is what sets up the\nalt_odb_tail pointer. And also, a later prepare() call would overwrite\nour entry.  We could refactor the alt_odb code, but it seemed simplest\nto just make sure we don't add to an unprepared list.\n\n> > +\tt = xmalloc(sizeof(*t));\n> > +\tstrbuf_init(&t->path, 0);\n> > +\targv_array_init(&t->env);\n> > +\n> > +\tstrbuf_addf(&t->path, \"%s/incoming-XXXXXX\", get_object_directory());\n> \n> I was wondering where you would put this in.  Inside .git/objects/\n> sounds good.\n\nThe name \"incoming\" is kind of arbitrary and related to the fact that\nthis is used for receive-pack (though if we were to use it on the\nfetching side, I think it would be equally correct). I don't think it\nreally matters in practice.\n\n> > +static int pack_copy_priority(const char *name)\n> > +{\n> > +\tif (!starts_with(name, \"pack\"))\n> > +\t\treturn 0;\n> > +\tif (ends_with(name, \".keep\"))\n> > +\t\treturn 1;\n> > +\tif (ends_with(name, \".pack\"))\n> > +\t\treturn 2;\n> > +\tif (ends_with(name, \".idx\"))\n> > +\t\treturn 3;\n> > +\treturn 4;\n> > +}\n> \n> Thanks for being careful.  A blind \"cp -r\" would have ruined the\n> day.\n> \n> We do not do bitmaps upon receiving, I guess.\n\nBut we don't, but they (and anything else) would just sort at the end,\nwhich is OK.\n\n> > + *\tstruct tmp_objdir *t = tmp_objdir_create();\n> > + *\tif (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&\n> > + *\t    !tmp_objdir_migrate(t))\n> > + *\t\tprintf(\"success!\\n\");\n> > + *\telse\n> > + *\t\tdie(\"failed...tmp_objdir will clean up for us\");\n> \n> Made me briefly wonder if a caller might want to use appropriate\n> environment to use the tmp-objdir given by the API in addition to\n> its own, but then such a caller just needs to prepare its own argv-array\n> and concatenate tmp_objdir_env() before making the opt_cd_env call,\n> so this is perfectly fine.\n\nYep, and that's exactly what happens in one spot of the next patch.\nMy original had just open-coded, but I was happy to see we have\nargv_array_pushv() these days, so it's a one-liner.\n\nIn the very original version, the receive-pack process did not need to\naccess the new objects at all (not until ref update time anyway, at\nwhich point they've been migrated). And that's why the environment is\nintentionally kept separate, and the caller can feed it to whichever\nsub-programs it chooses. But a later version of git that handled shallow\npushes required receive-pack to actually look at the objects, and I\nadded the add_to_alternates_internal() call you see here.\n\nAt that point, it does make me wonder if a better interface would be for\ntmp_objdir to just set up the environment variables in the parent\nprocess in the first place, and then restore them upon\ntmp_objdir_destroy(). It makes things a bit more automatic, which makes\nme hesitate, but I think it would be fine for receive-pack.\n\nI dunno. I mostly left it alone because I did it this way long ago, and\nit wasn't broke. Polishing for upstream is an opportunity to fix old\noddities, but I think there is some value in applying a more\nbattle-tested patch.\n\n-Peff\n\n"},{"id":"303029","messageId":"20160930224450.zewgfqelljiwowh6@sigill.intra.peff.net","threadId":"44193","inReplyTo":"b7136f0690244aee94ff2db726a7edce@exmbdft7.ad.twosigma.com","subject":"Re: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-09-30T22:44:50Z","receivedAt":"2016-09-30T22:44:57Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 30, 2016 at 09:32:04PM +0000, David Turner wrote:\n\n> > +static void env_append(struct argv_array *env, const char *key, const\n> > +char *val) {\n> > +\tconst char *old = getenv(key);\n> > +\n> > +\tif (!old)\n> > +\t\targv_array_pushf(env, \"%s=%s\", key, val);\n> > +\telse\n> > +\t\targv_array_pushf(env, \"%s=%s%c%s\", key, old, PATH_SEP,\n> > val); \n> >+}\n> \n> I would like a comment explaining this function. \n\nI'll squash in:\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex c92e6cc..a98c246 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -70,6 +70,13 @@ static void remove_tmp_objdir_on_signal(int signo)\n \traise(signo);\n }\n \n+/*\n+ * These env_* functions are for setting up the child environment; the\n+ * \"replace\" variant overrides the value of any existing variable with that\n+ * \"key\". The \"append\" variant puts our new value at the end of a list,\n+ * separated by PATH_SEP (which is what separate values in\n+ * GIT_ALTERNATE_OBJECT_DIRECTORIES).\n+ */\n static void env_append(struct argv_array *env, const char *key, const char *val)\n {\n \tconst char *old = getenv(key);\n\n> > + * Finalize a temporary object directory by migrating its objects into\n> > +the main\n> > + * object database.\n> > + */\n> \n> This should mention that it frees its argument.\n\nAnd:\n\ndiff --git a/tmp-objdir.h b/tmp-objdir.h\nindex aa47aa9..b1e45b4 100644\n--- a/tmp-objdir.h\n+++ b/tmp-objdir.h\n@@ -35,7 +35,8 @@ const char **tmp_objdir_env(const struct tmp_objdir *);\n \n /*\n  * Finalize a temporary object directory by migrating its objects into the main\n- * object database.\n+ * object database, removing the temporary directory, and freeing any\n+ * associated resources.\n  */\n int tmp_objdir_migrate(struct tmp_objdir *);\n \n\n-Peff\n"},{"id":"303031","messageId":"b22eb24e64ee4026a04459dc02c27ff0@exmbdft7.ad.twosigma.com","threadId":"44193","inReplyTo":"20160930224450.zewgfqelljiwowh6@sigill.intra.peff.net","subject":"RE: [PATCH 3/6] tmp-objdir: introduce API for temporary object directories","fromName":"David Turner","fromEmail":"david.turner@twosigma.com","sentAt":"2016-09-30T23:07:11Z","receivedAt":"2016-09-30T23:13:04Z","isPatch":true,"sender":{"key":"david.turner@twosigma.com","avatar":null},"body":"Thanks.  The rest all look good too.\n\n> -----Original Message-----\n> From: Jeff King [mailto:peff@peff.net]\n> Sent: Friday, September 30, 2016 6:45 PM\n> To: David Turner\n> Cc: git@vger.kernel.org\n> Subject: Re: [PATCH 3/6] tmp-objdir: introduce API for temporary object\n> directories\n> \n> On Fri, Sep 30, 2016 at 09:32:04PM +0000, David Turner wrote:\n> \n> > > +static void env_append(struct argv_array *env, const char *key,\n> > > +const char *val) {\n> > > +\tconst char *old = getenv(key);\n> > > +\n> > > +\tif (!old)\n> > > +\t\targv_array_pushf(env, \"%s=%s\", key, val);\n> > > +\telse\n> > > +\t\targv_array_pushf(env, \"%s=%s%c%s\", key, old, PATH_SEP,\n> > > val);\n> > >+}\n> >\n> > I would like a comment explaining this function.\n> \n> I'll squash in:\n> \n> diff --git a/tmp-objdir.c b/tmp-objdir.c index c92e6cc..a98c246 100644\n> --- a/tmp-objdir.c\n> +++ b/tmp-objdir.c\n> @@ -70,6 +70,13 @@ static void remove_tmp_objdir_on_signal(int signo)\n>  \traise(signo);\n>  }\n> \n> +/*\n> + * These env_* functions are for setting up the child environment; the\n> + * \"replace\" variant overrides the value of any existing variable with\n> +that\n> + * \"key\". The \"append\" variant puts our new value at the end of a list,\n> + * separated by PATH_SEP (which is what separate values in\n> + * GIT_ALTERNATE_OBJECT_DIRECTORIES).\n> + */\n>  static void env_append(struct argv_array *env, const char *key, const char\n> *val)  {\n>  \tconst char *old = getenv(key);\n> \n> > > + * Finalize a temporary object directory by migrating its objects\n> > > +into the main\n> > > + * object database.\n> > > + */\n> >\n> > This should mention that it frees its argument.\n> \n> And:\n> \n> diff --git a/tmp-objdir.h b/tmp-objdir.h index aa47aa9..b1e45b4 100644\n> --- a/tmp-objdir.h\n> +++ b/tmp-objdir.h\n> @@ -35,7 +35,8 @@ const char **tmp_objdir_env(const struct tmp_objdir\n> *);\n> \n>  /*\n>   * Finalize a temporary object directory by migrating its objects into the main\n> - * object database.\n> + * object database, removing the temporary directory, and freeing any\n> + * associated resources.\n>   */\n>  int tmp_objdir_migrate(struct tmp_objdir *);\n> \n> \n> -Peff\n"},{"id":"303040","messageId":"20161001091245.jkju5rlytbcb2r52@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20160930193630.vwq6n5hjbthrz6fj@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] receive-pack: quarantine objects until pre-receive accepts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-01T09:12:45Z","receivedAt":"2016-10-01T09:12:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 30, 2016 at 03:36:30PM -0400, Jeff King wrote:\n\n> @@ -1639,6 +1666,18 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n>  \t\targv_array_push(&child.args, alt_shallow_file);\n>  \t}\n>  \n> +\ttmp_objdir = tmp_objdir_create();\n> +\tif (!tmp_objdir)\n> +\t\treturn \"unable to create temporary object directory\";\n> +\tchild.env = tmp_objdir_env(tmp_objdir);\n\nOne thing to note here: this new code kicks in all the time. My\nreasoning was that there's basically no time you _wouldn't_ want it to,\nand certainly that was the case for us when I wrote it. But I tried to\nthink of user-visible changes. Here's what I came up with:\n\n  - we currently leave the tmp_pack_* for a failed push sitting around\n    (e.g., if the client hangs up halfway through, or index-pack rejects\n    the pack for some reason). But with this series, it would always be\n    cleaned up. That's a very good thing if you're running a git hosting\n    site. It might make things harder if you're debugging.\n\n    I don't think it's a good reason not to enable this by default, but\n    it _could_ be a reason to have a config switch to turn it off\n    temporarily (or just leave the \"incoming-*\" directory in place).\n\n  - the environment that pre-receive pack runs in has access to objects\n    that the rest of the repository doesn't. So if you were to do\n    something silly in your pre-receive like:\n\n      # reject the push, but log a copy of the objects\n      git update-ref refs/rejected/$(date +%s) $new_sha1\n      exit 1\n\n    Then your ref-update would succeed (you have $new_sha1), but the\n    objects would be deleted immediately afterwards. I find this a\n    somewhat questionable pattern, and I have no idea if anybody else\n    has thought of it. But it _does_ work today, and not with this\n    series.\n\nI don't think it would be too hard to put a config conditional around\nthis tmp_objdir_create(). And then all of the tmp_objdir_env() calls\nwould just return NULL, and effectively become noops.\n\n-Peff\n"},{"id":"303058","messageId":"c27dc1a4-3c7a-2866-d9d8-f5d3eb161650@web.de","threadId":"44193","inReplyTo":"20160930193604.hqspizasfpqa6bez@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] sha1_file: always allow relative paths to alternates","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-10-02T09:07:39Z","receivedAt":"2016-10-02T09:08:10Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 30.09.2016 um 21:36 schrieb Jeff King:\n> We adjust the test script here to demonstrate that this now\n> works. Unfortunately, we can't demonstrate that the\n> duplicate is suppressed, since it has no user-visible\n> behavior (it's just one less place for our object lookups to\n> go). But you can verify it manually via gdb, with something\n> like:\n> \n>     for i in a b c; do\n>             git init --bare $i\n>             blob=$(echo $i | git -C $i hash-object -w --stdin)\n>     done\n>     echo \"../../b/objects\" >a/objects/info/alternates\n>     echo \"../../c/objects\" >>a/objects/info/alternates\n>     echo \"../../c/objects\" >b/objects/info/alternates\n>     gdb --args git cat-file -e $blob\n> \n> After prepare_alt_odb() runs, we have only a single copy of\n> \"/path/to/c/objects/\" in the alt_odb list.\n\nA better way would be to provide a UI for that.  We could easily bolt\nit to the side of count-objects like in the patch below.  Feels a bit\nhackish, though.\n\nPer-ODB counting would be nice, but a lot more involved, I guess\n(didn't try).\n\n\ndiff --git a/builtin/count-objects.c b/builtin/count-objects.c\nindex ba92919..b2afe36 100644\n--- a/builtin/count-objects.c\n+++ b/builtin/count-objects.c\n@@ -73,6 +73,12 @@ static int count_cruft(const char *basename, const char *path, void *data)\n \treturn 0;\n }\n \n+static int print_alt_odb(struct alternate_object_database *alt, void *data)\n+{\n+\tputs(alt->base);\n+\treturn 0;\n+}\n+\n static char const * const count_objects_usage[] = {\n \tN_(\"git count-objects [-v] [-H | --human-readable]\"),\n \tNULL\n@@ -81,10 +87,13 @@ static char const * const count_objects_usage[] = {\n int cmd_count_objects(int argc, const char **argv, const char *prefix)\n {\n \tint human_readable = 0;\n+\tint list_alt_odb = 0;\n \tstruct option opts[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n \t\tOPT_BOOL('H', \"human-readable\", &human_readable,\n \t\t\t N_(\"print sizes in human readable format\")),\n+\t\tOPT_BOOL(0, \"list-alternates\", &list_alt_odb,\n+\t\t\t N_(\"print list of alternate object databases\")),\n \t\tOPT_END(),\n \t};\n \n@@ -92,6 +101,12 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix)\n \t/* we do not take arguments other than flags for now */\n \tif (argc)\n \t\tusage_with_options(count_objects_usage, opts);\n+\n+\tif (list_alt_odb) {\n+\t\tforeach_alt_odb(print_alt_odb, NULL);\n+\t\treturn 0;\n+\t}\n+\n \tif (verbose) {\n \t\treport_garbage = real_report_garbage;\n \t\treport_linked_checkout_garbage();\n\n"},{"id":"303059","messageId":"CAP8UFD3Y1du+L9DRgz01wgbsCYiebu2DgyePH41MaDhjV24oHw@mail.gmail.com","threadId":"44193","inReplyTo":"20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] receive-pack: quarantine pushed objects","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-02T09:20:59Z","receivedAt":"2016-10-02T09:21:07Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Sep 30, 2016 at 9:35 PM, Jeff King <peff@peff.net> wrote:\n> I've mentioned before on the list that GitHub \"quarantines\" objects\n> while the pre-receive hook runs. Here are the patches to implement\n> that.\n\nGreat! Thanks for upstreaming these patches!\n\nI wonder if the patch you sent in:\n\nhttps://public-inbox.org/git/20160816144642.5ikkta4l5hyx6act@sigill.intra.peff.net/\n\nis still useful or not.\n\n> The basic problem is that as-is, index-pack admits pushed objects into\n> the main object database immediately, before the pre-receive hook runs.\n> It _has_ to, since the hook needs to be able to actually look at the\n> objects. However, this means that if the pre-receive hook rejects the\n> push, we still end up with the objects in the repository. We can't just\n> delete them as temporary files, because we don't know what other\n> processes might have started referencing them.\n>\n> The solution here is to push into a \"quarantine\" directory that is\n> accessible only to pre-receive, check_connected(), etc, and only\n> move the objects into the main object database after we've finished\n> those basic checks.\n\nI guess if we fail the receive-pack because the pack is bigger than\nreceive.maxInputSize, then the \"quarantine\" directory will also be\nremoved, so the part of the pack that we received before failing the\nreceive-pack will be deleted.\n\n[...]\n\n>     These two patches set that up by letting index-pack and pre-receive\n>     know that quarantine path and use it to store arbitrary files that\n>     _don't_ get migrated to the main object database (i.e., the log file\n>     mentioned above).\n\nIt would be nice to have a diffstat for the whole series.\n\nThanks,\nChristian.\n"},{"id":"303062","messageId":"20161002130216.bburwjkx4dkzxiig@sigill.intra.peff.net","threadId":"44193","inReplyTo":"CAP8UFD3Y1du+L9DRgz01wgbsCYiebu2DgyePH41MaDhjV24oHw@mail.gmail.com","subject":"Re: [PATCH 0/6] receive-pack: quarantine pushed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-02T13:02:16Z","receivedAt":"2016-10-02T13:02:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 02, 2016 at 11:20:59AM +0200, Christian Couder wrote:\n\n> On Fri, Sep 30, 2016 at 9:35 PM, Jeff King <peff@peff.net> wrote:\n> > I've mentioned before on the list that GitHub \"quarantines\" objects\n> > while the pre-receive hook runs. Here are the patches to implement\n> > that.\n> \n> Great! Thanks for upstreaming these patches!\n> \n> I wonder if the patch you sent in:\n> \n> https://public-inbox.org/git/20160816144642.5ikkta4l5hyx6act@sigill.intra.peff.net/\n> \n> is still useful or not.\n\nIt is potentially still useful for other code paths besides\nreceive-pack. But if the main concern is pushes, then yeah, I think it\nis not really doing anything.\n\n> I guess if we fail the receive-pack because the pack is bigger than\n> receive.maxInputSize, then the \"quarantine\" directory will also be\n> removed, so the part of the pack that we received before failing the\n> receive-pack will be deleted.\n\nCorrect. _Any_ failure up to the tmp_objdir_migrate() call will drop the\nobjects. So that includes index-pack failing for any reason.\n\n> >     These two patches set that up by letting index-pack and pre-receive\n> >     know that quarantine path and use it to store arbitrary files that\n> >     _don't_ get migrated to the main object database (i.e., the log file\n> >     mentioned above).\n> \n> It would be nice to have a diffstat for the whole series.\n\nYou mean in the cover letter? I do not mind including it if people find\nthem useful, but I personally have always just found them to be clutter\nat that level.\n\n-Peff\n"},{"id":"303063","messageId":"20161002130346.4ay2d52su3bggh6o@sigill.intra.peff.net","threadId":"44193","inReplyTo":"c27dc1a4-3c7a-2866-d9d8-f5d3eb161650@web.de","subject":"Re: [PATCH 2/6] sha1_file: always allow relative paths to alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-02T13:03:46Z","receivedAt":"2016-10-02T13:03:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 02, 2016 at 11:07:39AM +0200, René Scharfe wrote:\n\n> Am 30.09.2016 um 21:36 schrieb Jeff King:\n> > We adjust the test script here to demonstrate that this now\n> > works. Unfortunately, we can't demonstrate that the\n> > duplicate is suppressed, since it has no user-visible\n> > behavior (it's just one less place for our object lookups to\n> > go). But you can verify it manually via gdb, with something\n> > like:\n> > \n> >     for i in a b c; do\n> >             git init --bare $i\n> >             blob=$(echo $i | git -C $i hash-object -w --stdin)\n> >     done\n> >     echo \"../../b/objects\" >a/objects/info/alternates\n> >     echo \"../../c/objects\" >>a/objects/info/alternates\n> >     echo \"../../c/objects\" >b/objects/info/alternates\n> >     gdb --args git cat-file -e $blob\n> > \n> > After prepare_alt_odb() runs, we have only a single copy of\n> > \"/path/to/c/objects/\" in the alt_odb list.\n> \n> A better way would be to provide a UI for that.  We could easily bolt\n> it to the side of count-objects like in the patch below.  Feels a bit\n> hackish, though.\n\nYeah, I thought about something like that, but I'm a bit hesitant to add\npublic interfaces that exist just for testing. OTOH, \"count-objects\n--list-alternates\" is conceivably a useful debugging tool for \"did I set\nup my alternates correctly?\".\n\n-Peff\n"},{"id":"303064","messageId":"20161002153856.sqdw26mtldhvue2r@sigill.intra.peff.net","threadId":"44193","inReplyTo":"c27dc1a4-3c7a-2866-d9d8-f5d3eb161650@web.de","subject":"Re: [PATCH 2/6] sha1_file: always allow relative paths to alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-02T15:38:57Z","receivedAt":"2016-10-02T15:39:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 02, 2016 at 11:07:39AM +0200, René Scharfe wrote:\n\n> diff --git a/builtin/count-objects.c b/builtin/count-objects.c\n> index ba92919..b2afe36 100644\n> --- a/builtin/count-objects.c\n> +++ b/builtin/count-objects.c\n> @@ -73,6 +73,12 @@ static int count_cruft(const char *basename, const char *path, void *data)\n>  \treturn 0;\n>  }\n>  \n> +static int print_alt_odb(struct alternate_object_database *alt, void *data)\n> +{\n> +\tputs(alt->base);\n> +\treturn 0;\n> +}\n\nThis turns out to be wrong, because alt->base _isn't_ just the base;\nit's also the scratch buffer we write into to form pathnames. So if\nwe've used the buffer to look up an object, we'll get that object name\nhere, not just the base.\n\nIt tends to work for your command because we do nothing except list the\nalternates and exit, but I'm not sure if there are code paths which\nmight access an object.\n\nI think giving a known state to the callback should be the\nresponsibility of foreach_alt_odb(), though.\n\n-Peff\n"},{"id":"303066","messageId":"20161002165913.txqotwtwulphja67@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20161002153856.sqdw26mtldhvue2r@sigill.intra.peff.net","subject":"Re: [PATCH 2/6] sha1_file: always allow relative paths to alternates","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-02T16:59:14Z","receivedAt":"2016-10-02T16:59:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Oct 02, 2016 at 11:38:56AM -0400, Jeff King wrote:\n\n> On Sun, Oct 02, 2016 at 11:07:39AM +0200, René Scharfe wrote:\n> \n> > diff --git a/builtin/count-objects.c b/builtin/count-objects.c\n> > index ba92919..b2afe36 100644\n> > --- a/builtin/count-objects.c\n> > +++ b/builtin/count-objects.c\n> > @@ -73,6 +73,12 @@ static int count_cruft(const char *basename, const char *path, void *data)\n> >  \treturn 0;\n> >  }\n> >  \n> > +static int print_alt_odb(struct alternate_object_database *alt, void *data)\n> > +{\n> > +\tputs(alt->base);\n> > +\treturn 0;\n> > +}\n> \n> This turns out to be wrong, because alt->base _isn't_ just the base;\n> it's also the scratch buffer we write into to form pathnames. So if\n> we've used the buffer to look up an object, we'll get that object name\n> here, not just the base.\n> \n> It tends to work for your command because we do nothing except list the\n> alternates and exit, but I'm not sure if there are code paths which\n> might access an object.\n> \n> I think giving a known state to the callback should be the\n> responsibility of foreach_alt_odb(), though.\n\nActually, it looks like there are a lot of opportunities for cleanups in\nthat area, and maybe some bugfixes (e.g., we should consistently be\nusing fspathcmp and not memcmp to check for duplicate paths). I started\non a series, but don't have anything to show yet (just FYI in case you\nwere thinking about doing the same).\n\n-Peff\n"},{"id":"303072","messageId":"CAP8UFD0FTe7P-0C_aqF3RVMMqVaa=s=Ld=pcoVyYirFw1h9Ogg@mail.gmail.com","threadId":"44193","inReplyTo":"20161002130216.bburwjkx4dkzxiig@sigill.intra.peff.net","subject":"Re: [PATCH 0/6] receive-pack: quarantine pushed objects","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2016-10-03T06:45:18Z","receivedAt":"2016-10-03T06:45:25Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sun, Oct 2, 2016 at 3:02 PM, Jeff King <peff@peff.net> wrote:\n> On Sun, Oct 02, 2016 at 11:20:59AM +0200, Christian Couder wrote:\n>\n>> I wonder if the patch you sent in:\n>>\n>> https://public-inbox.org/git/20160816144642.5ikkta4l5hyx6act@sigill.intra.peff.net/\n>>\n>> is still useful or not.\n>\n> It is potentially still useful for other code paths besides\n> receive-pack. But if the main concern is pushes, then yeah, I think it\n> is not really doing anything.\n>\n>> I guess if we fail the receive-pack because the pack is bigger than\n>> receive.maxInputSize, then the \"quarantine\" directory will also be\n>> removed, so the part of the pack that we received before failing the\n>> receive-pack will be deleted.\n>\n> Correct. _Any_ failure up to the tmp_objdir_migrate() call will drop the\n> objects. So that includes index-pack failing for any reason.\n\nGreat, thanks for explaining!\n\n>> >     These two patches set that up by letting index-pack and pre-receive\n>> >     know that quarantine path and use it to store arbitrary files that\n>> >     _don't_ get migrated to the main object database (i.e., the log file\n>> >     mentioned above).\n>>\n>> It would be nice to have a diffstat for the whole series.\n>\n> You mean in the cover letter? I do not mind including it if people find\n> them useful, but I personally have always just found them to be clutter\n> at that level.\n\nI think it can help to quickly get an idea about what the series\nimpacts, and it would have made it easier for me to see that the\nchanges in the patch you sent previously\n(https://public-inbox.org/git/20160816144642.5ikkta4l5hyx6act@sigill.intra.peff.net/)\nare not part of this series.\n\nThanks anyway,\nChristian.\n"},{"id":"303130","messageId":"20161003204851.klwspo6agykx6s3q@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net","subject":"[PATCH v2 0/5] receive-pack: quarantine pushed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:48:51Z","receivedAt":"2016-10-03T20:49:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Here's a v2; the original was at:\n\n  http://public-inbox.org/git/20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net/\n\nwhich contains the rationale.  The required alternate-objects patches\n(both the \"allow recursive relative\" one, and the helper to add an\ninternal alt-odb) have been pushed into their own series, that I posted\nhere:\n\n  http://public-inbox.org/git/20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net/\n\nThis needs to be applied on top.\n\nOther than that, v2 just fixes the minor issues raised on the list\n(missing \"static\", and some clarifying comments).\n\n  [1/5]: check_connected: accept an env argument\n  [2/5]: tmp-objdir: introduce API for temporary object directories\n  [3/5]: receive-pack: quarantine objects until pre-receive accepts\n  [4/5]: tmp-objdir: put quarantine information in the environment\n  [5/5]: tmp-objdir: do not migrate files starting with '.'\n\n Makefile                   |   1 +\n builtin/receive-pack.c     |  41 ++++++-\n cache.h                    |   1 +\n connected.c                |   1 +\n connected.h                |   5 +\n t/t5547-push-quarantine.sh |  36 ++++++\n tmp-objdir.c               | 275 +++++++++++++++++++++++++++++++++++++++++++++\n tmp-objdir.h               |  54 +++++++++\n 8 files changed, 413 insertions(+), 1 deletion(-)\n create mode 100755 t/t5547-push-quarantine.sh\n create mode 100644 tmp-objdir.c\n create mode 100644 tmp-objdir.h\n\n"},{"id":"303131","messageId":"20161003204908.3nlg6xq2whg2senj@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20161003204851.klwspo6agykx6s3q@sigill.intra.peff.net","subject":"[PATCH v2 1/5] check_connected: accept an env argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:49:08Z","receivedAt":"2016-10-03T20:49:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This lets callers influence the environment seen by\nrev-list, which will be useful when we start providing\nquarantined objects.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n connected.c | 1 +\n connected.h | 5 +++++\n 2 files changed, 6 insertions(+)\n\ndiff --git a/connected.c b/connected.c\nindex 8e3e4b1..136c2ac 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -63,6 +63,7 @@ int check_connected(sha1_iterate_fn fn, void *cb_data,\n \t\t\t\t _(\"Checking connectivity\"));\n \n \trev_list.git_cmd = 1;\n+\trev_list.env = opt->env;\n \trev_list.in = -1;\n \trev_list.no_stdout = 1;\n \tif (opt->err_fd)\ndiff --git a/connected.h b/connected.h\nindex afa48cc..4ca325f 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -33,6 +33,11 @@ struct check_connected_options {\n \n \t/* If non-zero, show progress as we traverse the objects. */\n \tint progress;\n+\n+\t/*\n+\t * Insert these variables into the environment of the child process.\n+\t */\n+\tconst char **env;\n };\n \n #define CHECK_CONNECTED_INIT { 0 }\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303132","messageId":"20161003204911.xrudjamm3mp2xj6q@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20161003204851.klwspo6agykx6s3q@sigill.intra.peff.net","subject":"[PATCH v2 2/5] tmp-objdir: introduce API for temporary object directories","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:49:11Z","receivedAt":"2016-10-03T20:49:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Once objects are added to the object database by a process,\nthey cannot easily be deleted, as we don't know what other\nprocesses may have started referencing them. We have to\nclean them up with git-gc, which will apply the usual\nreachability and grace-period checks.\n\nThis patch provides an alternative: it helps callers create\na temporary directory inside the object directory, and a\ntemporary environment which can be passed to sub-programs to\nask them to write there (the original object directory\nremains accessible as an alternate of the temporary one).\n\nSee tmp-objdir.h for details on the API.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n Makefile     |   1 +\n tmp-objdir.c | 273 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++\n tmp-objdir.h |  54 ++++++++++++\n 3 files changed, 328 insertions(+)\n create mode 100644 tmp-objdir.c\n create mode 100644 tmp-objdir.h\n\ndiff --git a/Makefile b/Makefile\nindex 1aad150..4e3becb 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -831,6 +831,7 @@ LIB_OBJS += submodule-config.o\n LIB_OBJS += symlinks.o\n LIB_OBJS += tag.o\n LIB_OBJS += tempfile.o\n+LIB_OBJS += tmp-objdir.o\n LIB_OBJS += trace.o\n LIB_OBJS += trailer.o\n LIB_OBJS += transport.o\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nnew file mode 100644\nindex 0000000..9443868\n--- /dev/null\n+++ b/tmp-objdir.c\n@@ -0,0 +1,273 @@\n+#include \"cache.h\"\n+#include \"tmp-objdir.h\"\n+#include \"dir.h\"\n+#include \"sigchain.h\"\n+#include \"string-list.h\"\n+#include \"strbuf.h\"\n+#include \"argv-array.h\"\n+\n+struct tmp_objdir {\n+\tstruct strbuf path;\n+\tstruct argv_array env;\n+};\n+\n+/*\n+ * Allow only one tmp_objdir at a time in a running process, which simplifies\n+ * our signal/atexit cleanup routines.  It's doubtful callers will ever need\n+ * more than one, and we can expand later if so.  You can have many such\n+ * tmp_objdirs simultaneously in many processes, of course.\n+ */\n+static struct tmp_objdir *the_tmp_objdir;\n+\n+static void tmp_objdir_free(struct tmp_objdir *t)\n+{\n+\tstrbuf_release(&t->path);\n+\targv_array_clear(&t->env);\n+\tfree(t);\n+}\n+\n+static int tmp_objdir_destroy_1(struct tmp_objdir *t, int on_signal)\n+{\n+\tint err;\n+\n+\tif (!t)\n+\t\treturn 0;\n+\n+\tif (t == the_tmp_objdir)\n+\t\tthe_tmp_objdir = NULL;\n+\n+\t/*\n+\t * This may use malloc via strbuf_grow(), but we should\n+\t * have pre-grown t->path sufficiently so that this\n+\t * doesn't happen in practice.\n+\t */\n+\terr = remove_dir_recursively(&t->path, 0);\n+\n+\t/*\n+\t * When we are cleaning up due to a signal, we won't bother\n+\t * freeing memory; it may cause a deadlock if the signal\n+\t * arrived while libc's allocator lock is held.\n+\t */\n+\tif (!on_signal)\n+\t\ttmp_objdir_free(t);\n+\treturn err;\n+}\n+\n+int tmp_objdir_destroy(struct tmp_objdir *t)\n+{\n+\treturn tmp_objdir_destroy_1(t, 0);\n+}\n+\n+static void remove_tmp_objdir(void)\n+{\n+\ttmp_objdir_destroy(the_tmp_objdir);\n+}\n+\n+static void remove_tmp_objdir_on_signal(int signo)\n+{\n+\ttmp_objdir_destroy_1(the_tmp_objdir, 1);\n+\tsigchain_pop(signo);\n+\traise(signo);\n+}\n+\n+/*\n+ * These env_* functions are for setting up the child environment; the\n+ * \"replace\" variant overrides the value of any existing variable with that\n+ * \"key\". The \"append\" variant puts our new value at the end of a list,\n+ * separated by PATH_SEP (which is what separate values in\n+ * GIT_ALTERNATE_OBJECT_DIRECTORIES).\n+ */\n+static void env_append(struct argv_array *env, const char *key, const char *val)\n+{\n+\tconst char *old = getenv(key);\n+\n+\tif (!old)\n+\t\targv_array_pushf(env, \"%s=%s\", key, val);\n+\telse\n+\t\targv_array_pushf(env, \"%s=%s%c%s\", key, old, PATH_SEP, val);\n+}\n+\n+static void env_replace(struct argv_array *env, const char *key, const char *val)\n+{\n+\targv_array_pushf(env, \"%s=%s\", key, val);\n+}\n+\n+static int setup_tmp_objdir(const char *root)\n+{\n+\tchar *path;\n+\tint ret = 0;\n+\n+\tpath = xstrfmt(\"%s/pack\", root);\n+\tret = mkdir(path, 0777);\n+\tfree(path);\n+\n+\treturn ret;\n+}\n+\n+struct tmp_objdir *tmp_objdir_create(void)\n+{\n+\tstatic int installed_handlers;\n+\tstruct tmp_objdir *t;\n+\n+\tif (the_tmp_objdir)\n+\t\tdie(\"BUG: only one tmp_objdir can be used at a time\");\n+\n+\tt = xmalloc(sizeof(*t));\n+\tstrbuf_init(&t->path, 0);\n+\targv_array_init(&t->env);\n+\n+\tstrbuf_addf(&t->path, \"%s/incoming-XXXXXX\", get_object_directory());\n+\n+\t/*\n+\t * Grow the strbuf beyond any filename we expect to be placed in it.\n+\t * If tmp_objdir_destroy() is called by a signal handler, then\n+\t * we should be able to use the strbuf to remove files without\n+\t * having to call malloc.\n+\t */\n+\tstrbuf_grow(&t->path, 1024);\n+\n+\tif (!mkdtemp(t->path.buf)) {\n+\t\t/* free, not destroy, as we never touched the filesystem */\n+\t\ttmp_objdir_free(t);\n+\t\treturn NULL;\n+\t}\n+\n+\tthe_tmp_objdir = t;\n+\tif (!installed_handlers) {\n+\t\tatexit(remove_tmp_objdir);\n+\t\tsigchain_push_common(remove_tmp_objdir_on_signal);\n+\t\tinstalled_handlers++;\n+\t}\n+\n+\tif (setup_tmp_objdir(t->path.buf)) {\n+\t\ttmp_objdir_destroy(t);\n+\t\treturn NULL;\n+\t}\n+\n+\tenv_append(&t->env, ALTERNATE_DB_ENVIRONMENT,\n+\t\t   absolute_path(get_object_directory()));\n+\tenv_replace(&t->env, DB_ENVIRONMENT, absolute_path(t->path.buf));\n+\n+\treturn t;\n+}\n+\n+/*\n+ * Make sure we copy packfiles and their associated metafiles in the correct\n+ * order. All of these ends_with checks are slightly expensive to do in\n+ * the midst of a sorting routine, but in practice it shouldn't matter.\n+ * We will have a relatively small number of packfiles to order, and loose\n+ * objects exit early in the first line.\n+ */\n+static int pack_copy_priority(const char *name)\n+{\n+\tif (!starts_with(name, \"pack\"))\n+\t\treturn 0;\n+\tif (ends_with(name, \".keep\"))\n+\t\treturn 1;\n+\tif (ends_with(name, \".pack\"))\n+\t\treturn 2;\n+\tif (ends_with(name, \".idx\"))\n+\t\treturn 3;\n+\treturn 4;\n+}\n+\n+static int pack_copy_cmp(const char *a, const char *b)\n+{\n+\treturn pack_copy_priority(a) - pack_copy_priority(b);\n+}\n+\n+static int read_dir_paths(struct string_list *out, const char *path)\n+{\n+\tDIR *dh;\n+\tstruct dirent *de;\n+\n+\tdh = opendir(path);\n+\tif (!dh)\n+\t\treturn -1;\n+\n+\twhile ((de = readdir(dh)))\n+\t\tif (!is_dot_or_dotdot(de->d_name))\n+\t\t\tstring_list_append(out, de->d_name);\n+\n+\tclosedir(dh);\n+\treturn 0;\n+}\n+\n+static int migrate_paths(struct strbuf *src, struct strbuf *dst);\n+\n+static int migrate_one(struct strbuf *src, struct strbuf *dst)\n+{\n+\tstruct stat st;\n+\n+\tif (stat(src->buf, &st) < 0)\n+\t\treturn -1;\n+\tif (S_ISDIR(st.st_mode)) {\n+\t\tif (!mkdir(dst->buf, 0777)) {\n+\t\t\tif (adjust_shared_perm(dst->buf))\n+\t\t\t\treturn -1;\n+\t\t} else if (errno != EEXIST)\n+\t\t\treturn -1;\n+\t\treturn migrate_paths(src, dst);\n+\t}\n+\treturn finalize_object_file(src->buf, dst->buf);\n+}\n+\n+static int migrate_paths(struct strbuf *src, struct strbuf *dst)\n+{\n+\tsize_t src_len = src->len, dst_len = dst->len;\n+\tstruct string_list paths = STRING_LIST_INIT_DUP;\n+\tint i;\n+\tint ret = 0;\n+\n+\tif (read_dir_paths(&paths, src->buf) < 0)\n+\t\treturn -1;\n+\tpaths.cmp = pack_copy_cmp;\n+\tstring_list_sort(&paths);\n+\n+\tfor (i = 0; i < paths.nr; i++) {\n+\t\tconst char *name = paths.items[i].string;\n+\n+\t\tstrbuf_addf(src, \"/%s\", name);\n+\t\tstrbuf_addf(dst, \"/%s\", name);\n+\n+\t\tret |= migrate_one(src, dst);\n+\n+\t\tstrbuf_setlen(src, src_len);\n+\t\tstrbuf_setlen(dst, dst_len);\n+\t}\n+\n+\tstring_list_clear(&paths, 0);\n+\treturn ret;\n+}\n+\n+int tmp_objdir_migrate(struct tmp_objdir *t)\n+{\n+\tstruct strbuf src = STRBUF_INIT, dst = STRBUF_INIT;\n+\tint ret;\n+\n+\tif (!t)\n+\t\treturn 0;\n+\n+\tstrbuf_addbuf(&src, &t->path);\n+\tstrbuf_addstr(&dst, get_object_directory());\n+\n+\tret = migrate_paths(&src, &dst);\n+\n+\tstrbuf_release(&src);\n+\tstrbuf_release(&dst);\n+\n+\ttmp_objdir_destroy(t);\n+\treturn ret;\n+}\n+\n+const char **tmp_objdir_env(const struct tmp_objdir *t)\n+{\n+\tif (!t)\n+\t\treturn NULL;\n+\treturn t->env.argv;\n+}\n+\n+void tmp_objdir_add_as_alternate(const struct tmp_objdir *t)\n+{\n+\tadd_to_alternates_memory(t->path.buf);\n+}\ndiff --git a/tmp-objdir.h b/tmp-objdir.h\nnew file mode 100644\nindex 0000000..b1e45b4\n--- /dev/null\n+++ b/tmp-objdir.h\n@@ -0,0 +1,54 @@\n+#ifndef TMP_OBJDIR_H\n+#define TMP_OBJDIR_H\n+\n+/*\n+ * This API allows you to create a temporary object directory, advertise it to\n+ * sub-processes via GIT_OBJECT_DIRECTORY and GIT_ALTERNATE_OBJECT_DIRECTORIES,\n+ * and then either migrate its object into the main object directory, or remove\n+ * it. The library handles unexpected signal/exit death by cleaning up the\n+ * temporary directory.\n+ *\n+ * Example:\n+ *\n+ *\tstruct tmp_objdir *t = tmp_objdir_create();\n+ *\tif (!run_command_v_opt_cd_env(cmd, 0, NULL, tmp_objdir_env(t)) &&\n+ *\t    !tmp_objdir_migrate(t))\n+ *\t\tprintf(\"success!\\n\");\n+ *\telse\n+ *\t\tdie(\"failed...tmp_objdir will clean up for us\");\n+ *\n+ */\n+\n+struct tmp_objdir;\n+\n+/*\n+ * Create a new temporary object directory; returns NULL on failure.\n+ */\n+struct tmp_objdir *tmp_objdir_create(void);\n+\n+/*\n+ * Return a list of environment strings, suitable for use with\n+ * child_process.env, that can be passed to child programs to make use of the\n+ * temporary object directory.\n+ */\n+const char **tmp_objdir_env(const struct tmp_objdir *);\n+\n+/*\n+ * Finalize a temporary object directory by migrating its objects into the main\n+ * object database, removing the temporary directory, and freeing any\n+ * associated resources.\n+ */\n+int tmp_objdir_migrate(struct tmp_objdir *);\n+\n+/*\n+ * Destroy a temporary object directory, discarding any objects it contains.\n+ */\n+int tmp_objdir_destroy(struct tmp_objdir *);\n+\n+/*\n+ * Add the temporary object directory as an alternate object store in the\n+ * current process.\n+ */\n+void tmp_objdir_add_as_alternate(const struct tmp_objdir *);\n+\n+#endif /* TMP_OBJDIR_H */\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303133","messageId":"20161003204914.2jn2ik252scsusry@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20161003204851.klwspo6agykx6s3q@sigill.intra.peff.net","subject":"[PATCH v2 3/5] receive-pack: quarantine objects until pre-receive accepts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:49:14Z","receivedAt":"2016-10-03T20:49:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When a client pushes objects to us, index-pack checks the\nobjects themselves and then installs them into place. If we\nthen reject the push due to a pre-receive hook, we cannot\njust delete the packfile; other processes may be depending\non it. We have to do a normal reachability check at this\npoint via `git gc`.\n\nBut such objects may hang around for weeks due to the\ngc.pruneExpire grace period. And worse, during that time\nthey may be exploded from the pack into inefficient loose\nobjects.\n\nInstead, this patch teaches receive-pack to put the new\nobjects into a \"quarantine\" temporary directory. We make\nthese objects available to the connectivity check and to the\npre-receive hook, and then install them into place only if\nit is successful (and otherwise remove them as tempfiles).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/receive-pack.c     | 41 ++++++++++++++++++++++++++++++++++++++++-\n t/t5547-push-quarantine.sh | 36 ++++++++++++++++++++++++++++++++++++\n 2 files changed, 76 insertions(+), 1 deletion(-)\n create mode 100755 t/t5547-push-quarantine.sh\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 896b16f..267d320 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -20,6 +20,7 @@\n #include \"gpg-interface.h\"\n #include \"sigchain.h\"\n #include \"fsck.h\"\n+#include \"tmp-objdir.h\"\n \n static const char * const receive_pack_usage[] = {\n \tN_(\"git receive-pack <git-dir>\"),\n@@ -86,6 +87,8 @@ static enum {\n } use_keepalive;\n static int keepalive_in_sec = 5;\n \n+static struct tmp_objdir *tmp_objdir;\n+\n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n \tif (value) {\n@@ -663,6 +666,9 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,\n \t} else\n \t\targv_array_pushf(&proc.env_array, \"GIT_PUSH_OPTION_COUNT\");\n \n+\tif (tmp_objdir)\n+\t\targv_array_pushv(&proc.env_array, tmp_objdir_env(tmp_objdir));\n+\n \tif (use_sideband) {\n \t\tmemset(&muxer, 0, sizeof(muxer));\n \t\tmuxer.proc = copy_to_sideband;\n@@ -762,6 +768,7 @@ static int run_update_hook(struct command *cmd)\n \tproc.stdout_to_stderr = 1;\n \tproc.err = use_sideband ? -1 : 0;\n \tproc.argv = argv;\n+\tproc.env = tmp_objdir_env(tmp_objdir);\n \n \tcode = start_command(&proc);\n \tif (code)\n@@ -833,6 +840,7 @@ static int update_shallow_ref(struct command *cmd, struct shallow_info *si)\n \t\t    !delayed_reachability_test(si, i))\n \t\t\tsha1_array_append(&extra, si->shallow->sha1[i]);\n \n+\topt.env = tmp_objdir_env(tmp_objdir);\n \tsetup_alternate_shallow(&shallow_lock, &opt.shallow_file, &extra);\n \tif (check_connected(command_singleton_iterator, cmd, &opt)) {\n \t\trollback_lock_file(&shallow_lock);\n@@ -1240,12 +1248,17 @@ static void set_connectivity_errors(struct command *commands,\n \n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tstruct command *singleton = cmd;\n+\t\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n+\n \t\tif (shallow_update && si->shallow_ref[cmd->index])\n \t\t\t/* to be checked in update_shallow_ref() */\n \t\t\tcontinue;\n+\n+\t\topt.env = tmp_objdir_env(tmp_objdir);\n \t\tif (!check_connected(command_singleton_iterator, &singleton,\n-\t\t\t\t     NULL))\n+\t\t\t\t     &opt))\n \t\t\tcontinue;\n+\n \t\tcmd->error_string = \"missing necessary objects\";\n \t}\n }\n@@ -1428,6 +1441,7 @@ static void execute_commands(struct command *commands,\n \tdata.si = si;\n \topt.err_fd = err_fd;\n \topt.progress = err_fd && !quiet;\n+\topt.env = tmp_objdir_env(tmp_objdir);\n \tif (check_connected(iterate_receive_command_list, &data, &opt))\n \t\tset_connectivity_errors(commands, si);\n \n@@ -1444,6 +1458,19 @@ static void execute_commands(struct command *commands,\n \t\treturn;\n \t}\n \n+\t/*\n+\t * Now we'll start writing out refs, which means the objects need\n+\t * to be in their final positions so that other processes can see them.\n+\t */\n+\tif (tmp_objdir_migrate(tmp_objdir) < 0) {\n+\t\tfor (cmd = commands; cmd; cmd = cmd->next) {\n+\t\t\tif (!cmd->error_string)\n+\t\t\t\tcmd->error_string = \"unable to migrate objects to permanent storage\";\n+\t\t}\n+\t\treturn;\n+\t}\n+\ttmp_objdir = NULL;\n+\n \tcheck_aliased_updates(commands);\n \n \tfree(head_name_to_free);\n@@ -1639,6 +1666,18 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n \t\targv_array_push(&child.args, alt_shallow_file);\n \t}\n \n+\ttmp_objdir = tmp_objdir_create();\n+\tif (!tmp_objdir)\n+\t\treturn \"unable to create temporary object directory\";\n+\tchild.env = tmp_objdir_env(tmp_objdir);\n+\n+\t/*\n+\t * Normally we just pass the tmp_objdir environment to the child\n+\t * processes that do the heavy lifting, but we may need to see these\n+\t * objects ourselves to set up shallow information.\n+\t */\n+\ttmp_objdir_add_as_alternate(tmp_objdir);\n+\n \tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n \t\targv_array_pushl(&child.args, \"unpack-objects\", hdr_arg, NULL);\n \t\tif (quiet)\ndiff --git a/t/t5547-push-quarantine.sh b/t/t5547-push-quarantine.sh\nnew file mode 100755\nindex 0000000..1e5d32d\n--- /dev/null\n+++ b/t/t5547-push-quarantine.sh\n@@ -0,0 +1,36 @@\n+#!/bin/sh\n+\n+test_description='check quarantine of objects during push'\n+. ./test-lib.sh\n+\n+test_expect_success 'create picky dest repo' '\n+\tgit init --bare dest.git &&\n+\twrite_script dest.git/hooks/pre-receive <<-\\EOF\n+\twhile read old new ref; do\n+\t\ttest \"$(git log -1 --format=%s $new)\" = reject && exit 1\n+\tdone\n+\texit 0\n+\tEOF\n+'\n+\n+test_expect_success 'accepted objects work' '\n+\ttest_commit ok &&\n+\tgit push dest.git HEAD &&\n+\tcommit=$(git rev-parse HEAD) &&\n+\tgit --git-dir=dest.git cat-file commit $commit\n+'\n+\n+test_expect_success 'rejected objects are not installed' '\n+\ttest_commit reject &&\n+\tcommit=$(git rev-parse HEAD) &&\n+\ttest_must_fail git push dest.git reject &&\n+\ttest_must_fail git --git-dir=dest.git cat-file commit $commit\n+'\n+\n+test_expect_success 'rejected objects are removed' '\n+\techo \"incoming-*\" >expect &&\n+\t(cd dest.git/objects && echo incoming-*) >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303134","messageId":"20161003204921.gomizj4qbcwbfbhr@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20161003204851.klwspo6agykx6s3q@sigill.intra.peff.net","subject":"[PATCH v2 5/5] tmp-objdir: do not migrate files starting with '.'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:49:22Z","receivedAt":"2016-10-03T20:49:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This avoids \".\" and \"..\", as we already do, but also leaves\nroom for index-pack to store extra data in the quarantine\narea (e.g., for passing back any analysis to be read by the\npre-receive hook).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n tmp-objdir.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex 780af8e..64435f2 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -188,7 +188,7 @@ static int read_dir_paths(struct string_list *out, const char *path)\n \t\treturn -1;\n \n \twhile ((de = readdir(dh)))\n-\t\tif (!is_dot_or_dotdot(de->d_name))\n+\t\tif (de->d_name[0] != '.')\n \t\t\tstring_list_append(out, de->d_name);\n \n \tclosedir(dh);\n-- \n2.10.0.618.g82cc264\n"},{"id":"303135","messageId":"20161003204918.tfikcjyoss7h6o6n@sigill.intra.peff.net","threadId":"44193","inReplyTo":"20161003204851.klwspo6agykx6s3q@sigill.intra.peff.net","subject":"[PATCH v2 4/5] tmp-objdir: put quarantine information in the environment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T20:49:18Z","receivedAt":"2016-10-03T20:49:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The presence of the GIT_QUARANTINE_PATH variable lets any\ncalled programs know that they're operating in a temporary\nobject directory (and where that directory is).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h      | 1 +\n tmp-objdir.c | 2 ++\n 2 files changed, 3 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 9866e46..fcab658 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -433,6 +433,7 @@ static inline enum object_type object_type(unsigned int mode)\n #define GIT_GLOB_PATHSPECS_ENVIRONMENT \"GIT_GLOB_PATHSPECS\"\n #define GIT_NOGLOB_PATHSPECS_ENVIRONMENT \"GIT_NOGLOB_PATHSPECS\"\n #define GIT_ICASE_PATHSPECS_ENVIRONMENT \"GIT_ICASE_PATHSPECS\"\n+#define GIT_QUARANTINE_ENVIRONMENT \"GIT_QUARANTINE_PATH\"\n \n /*\n  * This environment variable is expected to contain a boolean indicating\ndiff --git a/tmp-objdir.c b/tmp-objdir.c\nindex 9443868..780af8e 100644\n--- a/tmp-objdir.c\n+++ b/tmp-objdir.c\n@@ -147,6 +147,8 @@ struct tmp_objdir *tmp_objdir_create(void)\n \tenv_append(&t->env, ALTERNATE_DB_ENVIRONMENT,\n \t\t   absolute_path(get_object_directory()));\n \tenv_replace(&t->env, DB_ENVIRONMENT, absolute_path(t->path.buf));\n+\tenv_replace(&t->env, GIT_QUARANTINE_ENVIRONMENT,\n+\t\t    absolute_path(t->path.buf));\n \n \treturn t;\n }\n-- \n2.10.0.618.g82cc264\n\n"},{"id":"303142","messageId":"xmqqlgy55dmk.fsf@gitster.mtv.corp.google.com","threadId":"44193","inReplyTo":"20161003204851.klwspo6agykx6s3q@sigill.intra.peff.net","subject":"Re: [PATCH v2 0/5] receive-pack: quarantine pushed objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-10-03T21:25:23Z","receivedAt":"2016-10-03T21:25:31Z","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> Here's a v2; the original was at:\n>\n>   http://public-inbox.org/git/20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net/\n>\n> which contains the rationale.  The required alternate-objects patches\n> (both the \"allow recursive relative\" one, and the helper to add an\n> internal alt-odb) have been pushed into their own series, that I posted\n> here:\n>\n>   http://public-inbox.org/git/20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net/\n>\n> This needs to be applied on top.\n\nSorry, you lost me.  So this 5-patch series comes on top of a brand\nnew 18-patch clean-up series?\n"},{"id":"303144","messageId":"20161003212838.5jpiy5mjxi3caxmd@sigill.intra.peff.net","threadId":"44193","inReplyTo":"xmqqlgy55dmk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 0/5] receive-pack: quarantine pushed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-03T21:28:38Z","receivedAt":"2016-10-03T21:28:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 03, 2016 at 02:25:23PM -0700, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Here's a v2; the original was at:\n> >\n> >   http://public-inbox.org/git/20160930193533.ynbepaago6oycg5t@sigill.intra.peff.net/\n> >\n> > which contains the rationale.  The required alternate-objects patches\n> > (both the \"allow recursive relative\" one, and the helper to add an\n> > internal alt-odb) have been pushed into their own series, that I posted\n> > here:\n> >\n> >   http://public-inbox.org/git/20161003203321.rj5jepviwo57uhqw@sigill.intra.peff.net/\n> >\n> > This needs to be applied on top.\n> \n> Sorry, you lost me.  So this 5-patch series comes on top of a brand\n> new 18-patch clean-up series?\n\nYes, sorry to be unclear. The final sentence should have been:\n\n  This v2 quarantine series need to be applied on top of that cleanup\n  series.\n\n-Peff\n"},{"id":"303423","messageId":"6ed4670d-6835-a45e-2842-cfe65b0e2981@gmail.com","threadId":"44193","inReplyTo":"20161003204908.3nlg6xq2whg2senj@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/5] check_connected: accept an env argument","fromName":"Jakub Narębski","fromEmail":"jnareb@gmail.com","sentAt":"2016-10-05T19:01:57Z","receivedAt":"2016-10-05T19:02:19Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"W dniu 03.10.2016 o 22:49, Jeff King pisze:\n\n> diff --git a/connected.h b/connected.h\n> index afa48cc..4ca325f 100644\n> --- a/connected.h\n> +++ b/connected.h\n> @@ -33,6 +33,11 @@ struct check_connected_options {\n>  \n>  \t/* If non-zero, show progress as we traverse the objects. */\n>  \tint progress;\n> +\n> +\t/*\n> +\t * Insert these variables into the environment of the child process.\n> +\t */\n> +\tconst char **env;\n>  };\n\nJust a nitpick, but I wonder why one comment is in single-line form,\nand the other uses block-form with a single line.\n\n-- \nJakub Narębski, bikeshedding\n\n"},{"id":"303426","messageId":"20161005190630.igp4bhdmpp2porc6@sigill.intra.peff.net","threadId":"44193","inReplyTo":"6ed4670d-6835-a45e-2842-cfe65b0e2981@gmail.com","subject":"Re: [PATCH v2 1/5] check_connected: accept an env argument","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-10-05T19:06:30Z","receivedAt":"2016-10-05T19:06:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Oct 05, 2016 at 09:01:57PM +0200, Jakub Narębski wrote:\n\n> > diff --git a/connected.h b/connected.h\n> > index afa48cc..4ca325f 100644\n> > --- a/connected.h\n> > +++ b/connected.h\n> > @@ -33,6 +33,11 @@ struct check_connected_options {\n> >  \n> >  \t/* If non-zero, show progress as we traverse the objects. */\n> >  \tint progress;\n> > +\n> > +\t/*\n> > +\t * Insert these variables into the environment of the child process.\n> > +\t */\n> > +\tconst char **env;\n> >  };\n> \n> Just a nitpick, but I wonder why one comment is in single-line form,\n> and the other uses block-form with a single line.\n\nI think I wrote something longer originally, and then shortened it\nbefore sending.\n\nI don't generally think it matters much for a case like this (if it were\nin the middle of code, I think the shorter form is worth doing, but here\nit's basically a header for this variable).\n\n-Peff\n"},{"id":"316412","messageId":"CACBZZX7Rt4zLGk=iy_Z9+8M-xUexwYA060R-v0nvseY7g+zA0w@mail.gmail.com","threadId":"44193","inReplyTo":"20161001091245.jkju5rlytbcb2r52@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] receive-pack: quarantine objects until pre-receive accepts","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-04-08T14:53:38Z","receivedAt":"2017-04-08T14:54:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Sat, Oct 1, 2016 at 11:12 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, Sep 30, 2016 at 03:36:30PM -0400, Jeff King wrote:\n>\n>> @@ -1639,6 +1666,18 @@ static const char *unpack(int err_fd, struct shallow_info *si)\n>>               argv_array_push(&child.args, alt_shallow_file);\n>>       }\n>>\n>> +     tmp_objdir = tmp_objdir_create();\n>> +     if (!tmp_objdir)\n>> +             return \"unable to create temporary object directory\";\n>> +     child.env = tmp_objdir_env(tmp_objdir);\n>\n> One thing to note here: this new code kicks in all the time. My\n> reasoning was that there's basically no time you _wouldn't_ want it to,\n> and certainly that was the case for us when I wrote it. But I tried to\n> think of user-visible changes. Here's what I came up with:\n>\n>   - we currently leave the tmp_pack_* for a failed push sitting around\n>     (e.g., if the client hangs up halfway through, or index-pack rejects\n>     the pack for some reason). But with this series, it would always be\n>     cleaned up. That's a very good thing if you're running a git hosting\n>     site. It might make things harder if you're debugging.\n>\n>     I don't think it's a good reason not to enable this by default, but\n>     it _could_ be a reason to have a config switch to turn it off\n>     temporarily (or just leave the \"incoming-*\" directory in place).\n>\n>   - the environment that pre-receive pack runs in has access to objects\n>     that the rest of the repository doesn't. So if you were to do\n>     something silly in your pre-receive like:\n>\n>       # reject the push, but log a copy of the objects\n>       git update-ref refs/rejected/$(date +%s) $new_sha1\n>       exit 1\n>\n>     Then your ref-update would succeed (you have $new_sha1), but the\n>     objects would be deleted immediately afterwards. I find this a\n>     somewhat questionable pattern, and I have no idea if anybody else\n>     has thought of it. But it _does_ work today, and not with this\n>     series.\n>\n> I don't think it would be too hard to put a config conditional around\n> this tmp_objdir_create(). And then all of the tmp_objdir_env() calls\n> would just return NULL, and effectively become noops.\n\nVery late reply, but I have a question about this. Is there anything\nyou can do on the plumbing level to figure out which area an object is\nin (of course that's not mutually exclusive).\n\nThe use-case for that is e.g. having a hook that rejects large\nbinaries, but has an exception for a binary that's been added in the\npast, before your change there's no optimal way to find that out,\nyou'd need to go over the whole history and list all blobs that have\never been added, with your change it would be trivial if something\ncould give me \"this object is in the quarantine area\", but AFAICT\nthere's no API that'll show me that.\n\nAlso, I think this whole thing could really do with some documentation\nin githooks(5). E.g. what hooks does it apply for? The test is just\nfor pre-receive but the patch changes run_update_hook(), does it also\ntake effect for update? Ditto the caveat about update-ref.\n"},{"id":"316523","messageId":"20170410211424.226psdn7ra3sb3mv@sigill.intra.peff.net","threadId":"44193","inReplyTo":"CACBZZX7Rt4zLGk=iy_Z9+8M-xUexwYA060R-v0nvseY7g+zA0w@mail.gmail.com","subject":"Re: [PATCH 4/6] receive-pack: quarantine objects until pre-receive accepts","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-04-10T21:14:24Z","receivedAt":"2017-04-10T21:14:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 08, 2017 at 04:53:38PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> Very late reply, but I have a question about this. Is there anything\n> you can do on the plumbing level to figure out which area an object is\n> in (of course that's not mutually exclusive).\n> \n> The use-case for that is e.g. having a hook that rejects large\n> binaries, but has an exception for a binary that's been added in the\n> past, before your change there's no optimal way to find that out,\n> you'd need to go over the whole history and list all blobs that have\n> ever been added, with your change it would be trivial if something\n> could give me \"this object is in the quarantine area\", but AFAICT\n> there's no API that'll show me that.\n\nNo, I don't think there's a way to ask that. The best API to add it to\nwould probably be \"cat-file --batch-check\", which can tell you similar\nthings about the on-disk storage, like \"is it a delta? against what?\"\nand the on-disk size.\n\n_But_. Like those other things, it would have the big caveat that the\nobject may appear multiple times. So if somebody sent you an object that\nwas a duplicate of an existing one, you may or may not see it as being\nin the quarantine directory.\n\nI think a better rule for hooks is to complain about history which\nintroduces new references to problematic objects. IOW, instead of\nlooking at the object database, actually see what the history\nintroduces:\n\n  while read old new refname; do\n\techo \"$new\"\n\tif test \"$old\" != 0000000000000000000000000000000000000000; then\n\t\techo \"^$old\"\n\tfi\n  done |\n  git rev-list --objects --stdin |\n  while read sha1 fn; do\n    # various object checks against $sha1\n  done\n\nHowever, if large binaries are your interest, I will suggest one other\nthing: it's much cheaper to find them during index-pack, rather than by\ntraversing history. At GitHub we run with a patch to index-pack that\ndumps the sha1s of any large blobs into .large-objects in the quarantine\ndirectory. You can abort there, of course, but it's hard to produce a\nimplement any other policy or produce a nice message. But if you just\nnote them and leave it to the pre-receive hook, then that hook can do\nthe traversal to find, e.g., which pathnames reference the large blobs.\nAnd it only has to run at all when we know there's at least one such\nblob (and it can quit immediately when it finds it).\n\nI can share those patches, but they need some cleanup to be ready for\ninclusion.\n\n> Also, I think this whole thing could really do with some documentation\n> in githooks(5). E.g. what hooks does it apply for? The test is just\n> for pre-receive but the patch changes run_update_hook(), does it also\n> take effect for update? Ditto the caveat about update-ref.\n\nMy thinking was that the cases where the effects were user-visible were\nsufficiently insane that nobody would need to know or care when the\nfeature was in use.\n\nIt isn't in use for the update hook. It can't be, because that is run\nper-ref. So the objects need to be pulled out of the quarantine before\nthe first ref is updated (I think the call in run_update_hook is\nleftover cruft from before I realized that; it should be a noop because\nwe'll always have migrated the objects by then, and tmp_objdir_env()\nwill just return NULL).\n\n-Peff\n"}]}