{"thread":{"id":"42377","subject":"[PATCH/RFC 0/6] pack-objects hook for upload-pack","startedAt":"2016-05-18T22:37:12Z","lastAt":"2016-05-27T02:11:34Z","messageCount":23,"participants":["Jeff King","Ævar Arnfjörð Bjarmason","Junio C Hamano","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"286938","messageId":"20160518223712.GA18317@sigill.intra.peff.net","threadId":"42377","inReplyTo":null,"subject":"[PATCH/RFC 0/6] pack-objects hook for upload-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T22:37:12Z","receivedAt":"2016-05-18T22:37:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"I've often wanted to intercept the call from upload-pack to\npack-objects. The final patch in this series goes into more detail, but\nbasically it's good for:\n\n 1. Capturing the output from pack-objects for debugging/inspection.\n\n 2. Capturing the input to pack-objects to replay for debugging or\n performance analysis.\n\n 3. Caching pack-objects output.\n\nIt's pretty trivial to add a hook to run instead of pack-objects (and\nthe hook would just run pack-objects itself). But we don't want to run\nhooks in upload-pack, because we don't necessarily trust the repository\nwe're running in.\n\nSo instead we'd have to learn about the hook from the environment, or\nfrom any of the non-repo config files. And that in turn requires some\nsupport from the config code, which is what patches 1-5 are doing (along\nwith some cleanups along the way).\n\nI think the config refactoring is all pretty sane, and could support\nother \"dangerous\" features besides this particular hook in a way that's\neasy to use for site admins (whether they trust the repos they're\nserving or not). I've marked this \"RFC\" because I'd like input on some\ndetails of the final patch; I'll point out my questions separately\nthere.\n\n [1/6]: git_config_with_options: drop \"found\" counting\n [2/6]: git_config_parse_parameter: refactor cleanup code\n [3/6]: config: set up config_source for command-line config\n [4/6]: config: return configset value for current_config_ functions\n [5/6]: config: add a notion of \"scope\"\n [6/6]: upload-pack: provide a hook for running pack-objects\n\n-Peff\n"},{"id":"286939","messageId":"20160518223902.GA22443@sigill.intra.peff.net","threadId":"42377","inReplyTo":"20160518223712.GA18317@sigill.intra.peff.net","subject":"[PATCH 1/6] git_config_with_options: drop \"found\" counting","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T22:39:02Z","receivedAt":"2016-05-18T22:39:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Prior to 1f2baa7 (config: treat non-existent config files as\nempty, 2010-10-21), we returned an error if any config files\nwere missing. That commit made this a non-error, but\nreturned the number of sources found, in case any caller\nwanted to distinguish this case.\n\nIn the past 5+ years, no caller has; the only two places\nwhich bother to check the return value care only about the\nerror case. Let's drop this code, which complicates the\nfunction. Similarly, let's drop the \"found anything\" return\nfrom git_config_from_parameters, which was present only to\nsupport this (and similarly has never had other callers care\nfor the past 5+ years).\n\nNote that we do need to update a comment in one of the\ncallers, even though the code immediately below it doesn't\ncare about this case.\n\nSigned-off-by: Jeff King \n---\nThis technically changes the function interface without breaking\ncompilation for any topics in flight. But given the history, and that\nthe now-unhandled case is not actually _useful_, I don't think it's\nworth worrying about.\n\n config.c | 34 +++++++++-------------------------\n 1 file changed, 9 insertions(+), 25 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 096fe03..3f8f6aa 100644\n--- a/config.c\n+++ b/config.c\n@@ -230,7 +230,7 @@ int git_config_from_parameters(config_fn_t fn, void *data)\n \n \tfree(argv);\n \tfree(envw);\n-\treturn nr > 0;\n+\treturn 0;\n }\n \n static int get_next_char(void)\n@@ -1201,47 +1201,31 @@ int git_config_system(void)\n \n static int do_git_config_sequence(config_fn_t fn, void *data)\n {\n-\tint ret = 0, found = 0;\n+\tint ret = 0;\n \tchar *xdg_config = xdg_config_home(\"config\");\n \tchar *user_config = expand_user_path(\"~/.gitconfig\");\n \tchar *repo_config = git_pathdup(\"config\");\n \n-\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0)) {\n+\tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))\n \t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n \t\t\t\t\t data);\n-\t\tfound += 1;\n-\t}\n \n-\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK)) {\n+\tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))\n \t\tret += git_config_from_file(fn, xdg_config, data);\n-\t\tfound += 1;\n-\t}\n \n-\tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK)) {\n+\tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))\n \t\tret += git_config_from_file(fn, user_config, data);\n-\t\tfound += 1;\n-\t}\n \n-\tif (repo_config && !access_or_die(repo_config, R_OK, 0)) {\n+\tif (repo_config && !access_or_die(repo_config, R_OK, 0))\n \t\tret += git_config_from_file(fn, repo_config, data);\n-\t\tfound += 1;\n-\t}\n \n-\tswitch (git_config_from_parameters(fn, data)) {\n-\tcase -1: /* error */\n+\tif (git_config_from_parameters(fn, data) < 0)\n \t\tdie(_(\"unable to parse command-line config\"));\n-\t\tbreak;\n-\tcase 0: /* found nothing */\n-\t\tbreak;\n-\tdefault: /* found at least one item */\n-\t\tfound++;\n-\t\tbreak;\n-\t}\n \n \tfree(xdg_config);\n \tfree(user_config);\n \tfree(repo_config);\n-\treturn ret == 0 ? found : ret;\n+\treturn ret;\n }\n \n int git_config_with_options(config_fn_t fn, void *data,\n@@ -1276,7 +1260,7 @@ static void git_config_raw(config_fn_t fn, void *data)\n \tif (git_config_with_options(fn, data, NULL, 1) < 0)\n \t\t/*\n \t\t * git_config_with_options() normally returns only\n-\t\t * positive values, as most errors are fatal, and\n+\t\t * zero, as most errors are fatal, and\n \t\t * non-fatal potential errors are guarded by \"if\"\n \t\t * statements that are entered only when no error is\n \t\t * possible.\n-- \n2.8.2.888.gecb1fe3\n"},{"id":"286940","messageId":"20160518223949.GB22443@sigill.intra.peff.net","threadId":"42377","inReplyTo":"20160518223712.GA18317@sigill.intra.peff.net","subject":"[PATCH 2/6] git_config_parse_parameter: refactor cleanup code","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T22:39:49Z","receivedAt":"2016-05-18T22:39:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We have several exits from the function, each of which has\nto do some cleanup. Let's consolidate these in an \"out\"\nlabel we can jump to. This doesn't save us much now, but it\nwill help as we add more things that need cleanup.\n\nSigned-off-by: Jeff King \n---\n config.c | 13 +++++++------\n 1 file changed, 7 insertions(+), 6 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 3f8f6aa..eb1e268 100644\n--- a/config.c\n+++ b/config.c\n@@ -205,6 +205,7 @@ int git_config_parse_parameter(const char *text,\n int git_config_from_parameters(config_fn_t fn, void *data)\n {\n \tconst char *env = getenv(CONFIG_DATA_ENVIRONMENT);\n+\tint ret = 0;\n \tchar *envw;\n \tconst char **argv = NULL;\n \tint nr = 0, alloc = 0;\n@@ -216,21 +217,21 @@ int git_config_from_parameters(config_fn_t fn, void *data)\n \tenvw = xstrdup(env);\n \n \tif (sq_dequote_to_argv(envw, &argv, &nr, &alloc) < 0) {\n-\t\tfree(envw);\n-\t\treturn error(\"bogus format in \" CONFIG_DATA_ENVIRONMENT);\n+\t\tret = error(\"bogus format in \" CONFIG_DATA_ENVIRONMENT);\n+\t\tgoto out;\n \t}\n \n \tfor (i = 0; i < nr; i++) {\n \t\tif (git_config_parse_parameter(argv[i], fn, data) < 0) {\n-\t\t\tfree(argv);\n-\t\t\tfree(envw);\n-\t\t\treturn -1;\n+\t\t\tret = -1;\n+\t\t\tgoto out;\n \t\t}\n \t}\n \n+out:\n \tfree(argv);\n \tfree(envw);\n-\treturn 0;\n+\treturn ret;\n }\n \n static int get_next_char(void)\n-- \n2.8.2.888.gecb1fe3\n"},{"id":"286942","messageId":"20160518224107.GC22443@sigill.intra.peff.net","threadId":"42377","inReplyTo":"20160518223712.GA18317@sigill.intra.peff.net","subject":"[PATCH 3/6] config: set up config_source for command-line config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T22:41:08Z","receivedAt":"2016-05-18T22:41:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When we parse a config file, we set up the global \"cf\"\nvariable as a pointer to a \"struct config_source\" describing\nthe file we are parsing. This is used for error messages, as\nwell as for lookup functions like current_config_name().\n\nThe \"cf\" variable is NULL in two cases:\n\n 1. When we are parsing command-line config, in which case\n there is no source file.\n\n 2. When we are not parsing any config at all.\n\nCallers like current_config_name() must assume we are in\ncase 1 if they see a NULL \"cf\". However, this means that if\nthey are accidentally used outside of a config parsing\ncallback, they will quietly return a bogus answer.\n\nThis might seem like an unlikely accident (why would you ask\nfor the current config file if you are not parsing config?),\nbut it's actually an easy mistake to make due to the\nconfigset caching. git_config() serves the answers from a\nconfigset cache, and any calls to current_config_name() will\nclaim that we are parsing command-line config, no matter\nwhat the original source.\n\nSo let's distinguish these cases by having the command-line\nconfig parser set up a config_source with a NULL name (which\ncallers already handle properly). We can use this to catch\nprogramming errors in some cases, and to give better\nmessages to the user in others.\n\nSigned-off-by: Jeff King \n---\n config.c | 23 +++++++++++++++++++----\n 1 file changed, 19 insertions(+), 4 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex eb1e268..4beba52 100644\n--- a/config.c\n+++ b/config.c\n@@ -131,7 +131,9 @@ static int handle_path_include(const char *path, struct config_include_data *inc\n \tif (!access_or_die(path, R_OK, 0)) {\n \t\tif (++inc->depth > MAX_INCLUDE_DEPTH)\n \t\t\tdie(include_depth_advice, MAX_INCLUDE_DEPTH, path,\n-\t\t\t cf && cf->name ? cf->name : \"the command line\");\n+\t\t\t !cf ? \"\" :\n+\t\t\t cf->name ? cf->name :\n+\t\t\t \"the command line\");\n \t\tret = git_config_from_file(git_config_include, path, inc);\n \t\tinc->depth--;\n \t}\n@@ -210,9 +212,15 @@ int git_config_from_parameters(config_fn_t fn, void *data)\n \tconst char **argv = NULL;\n \tint nr = 0, alloc = 0;\n \tint i;\n+\tstruct config_source source;\n \n \tif (!env)\n \t\treturn 0;\n+\n+\tmemset(&source, 0, sizeof(source));\n+\tsource.prev = cf;\n+\tcf = &source;\n+\n \t/* sq_dequote will write over it */\n \tenvw = xstrdup(env);\n \n@@ -231,6 +239,7 @@ int git_config_from_parameters(config_fn_t fn, void *data)\n out:\n \tfree(argv);\n \tfree(envw);\n+\tcf = source.prev;\n \treturn ret;\n }\n \n@@ -1345,7 +1354,9 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n \tl_item->e = e;\n \tl_item->value_index = e->value_list.nr - 1;\n \n-\tif (cf) {\n+\tif (!cf)\n+\t\tdie(\"BUG: configset_add_value has no source\");\n+\tif (cf->name) {\n \t\tkv_info->filename = strintern(cf->name);\n \t\tkv_info->linenr = cf->linenr;\n \t} else {\n@@ -2431,10 +2442,14 @@ int parse_config_key(const char *var,\n \n const char *current_config_origin_type(void)\n {\n-\treturn cf && cf->origin_type ? cf->origin_type : \"command line\";\n+\tif (!cf)\n+\t\tdie(\"BUG: current_config_origin_type called outside config callback\");\n+\treturn cf->origin_type ? cf->origin_type : \"command line\";\n }\n \n const char *current_config_name(void)\n {\n-\treturn cf && cf->name ? cf->name : \"\";\n+\tif (!cf)\n+\t\tdie(\"BUG: current_config_name called outside config callback\");\n+\treturn cf->name ? cf->name : \"\";\n }\n-- \n2.8.2.888.gecb1fe3\n"},{"id":"286943","messageId":"20160518224323.GD22443@sigill.intra.peff.net","threadId":"42377","inReplyTo":"20160518223712.GA18317@sigill.intra.peff.net","subject":"[PATCH 4/6] config: return configset value for current_config_ functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T22:43:23Z","receivedAt":"2016-05-18T22:43:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When 473166b (config: add 'origin_type' to config_source\nstruct, 2016-02-19) added accessor functions for the origin\ntype and name, it taught them only to look at the \"cf\"\nstruct that is filled in while we are parsing the config.\nThis is sufficient to make it work with git-config, which\nuses git_config_with_options() under the hood. That function\nfreshly parses the config files and triggers the callback\nwhen it parses each key.\n\nMost git programs, however, use git_config(). This interface\nwill populate a cache during the actual parse, and then\nserve values from the cache. Calling current_config_filename()\nin a callback here will find a NULL cf and produce an error.\nThere are no such callers right now, but let's prepare for\nadding some by making this work.\n\nWe already record source information in a struct attached to\neach value. We just need to make it globally available and\nthen consult it from the accessor functions.\n\nSigned-off-by: Jeff King \n---\n cache.h | 1 +\n config.c | 51 +++++++++++++++++++++++++++++++++++++++++---------\n t/helper/test-config.c | 20 ++++++++++++++++++++\n t/t1308-config-set.sh | 23 +++++++++++++++++++++++\n 4 files changed, 86 insertions(+), 9 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6b80a17..29c579b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1693,6 +1693,7 @@ extern int ignore_untracked_cache_config;\n struct key_value_info {\n \tconst char *filename;\n \tint linenr;\n+\tconst char *origin_type;\n };\n \n extern NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));\ndiff --git a/config.c b/config.c\nindex 4beba52..75afdd7 100644\n--- a/config.c\n+++ b/config.c\n@@ -38,7 +38,24 @@ struct config_source {\n \tlong (*do_ftell)(struct config_source *c);\n };\n \n+/*\n+ * These variables record the \"current\" config source, which\n+ * can be accessed by parsing callbacks.\n+ *\n+ * The \"cf\" variable will be non-NULL only when we are actually parsing a real\n+ * config source (file, blob, cmdline, etc).\n+ *\n+ * The \"current_config_kvi\" variable will be non-NULL only when we are feeding\n+ * cached config from a configset into a callback.\n+ *\n+ * They should generally never be non-NULL at the same time. If they are both\n+ * NULL, then we aren't parsing anything (and depending on the function looking\n+ * at the variables, it's either a bug for it to be called in the first place,\n+ * or it's a function which can be reused for non-config purposes, and should\n+ * fall back to some sane behavior).\n+ */\n static struct config_source *cf;\n+static struct key_value_info *current_config_kvi;\n \n static int zlib_compression_seen;\n \n@@ -1288,16 +1305,20 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n \tstruct string_list *values;\n \tstruct config_set_element *entry;\n \tstruct configset_list *list = &cs->list;\n-\tstruct key_value_info *kv_info;\n \n \tfor (i = 0; i < list->nr; i++) {\n \t\tentry = list->items[i].e;\n \t\tvalue_index = list->items[i].value_index;\n \t\tvalues = &entry->value_list;\n-\t\tif (fn(entry->key, values->items[value_index].string, data) < 0) {\n-\t\t\tkv_info = values->items[value_index].util;\n-\t\t\tgit_die_config_linenr(entry->key, kv_info->filename, kv_info->linenr);\n-\t\t}\n+\n+\t\tcurrent_config_kvi = values->items[value_index].util;\n+\n+\t\tif (fn(entry->key, values->items[value_index].string, data) < 0)\n+\t\t\tgit_die_config_linenr(entry->key,\n+\t\t\t\t\t current_config_kvi->filename,\n+\t\t\t\t\t current_config_kvi->linenr);\n+\n+\t\tcurrent_config_kvi = NULL;\n \t}\n }\n \n@@ -1359,10 +1380,12 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n \tif (cf->name) {\n \t\tkv_info->filename = strintern(cf->name);\n \t\tkv_info->linenr = cf->linenr;\n+\t\tkv_info->origin_type = strintern(cf->origin_type);\n \t} else {\n \t\t/* for values read from `git_config_from_parameters()` */\n \t\tkv_info->filename = NULL;\n \t\tkv_info->linenr = -1;\n+\t\tkv_info->origin_type = NULL;\n \t}\n \tsi->util = kv_info;\n \n@@ -2442,14 +2465,24 @@ int parse_config_key(const char *var,\n \n const char *current_config_origin_type(void)\n {\n-\tif (!cf)\n+\tconst char *type;\n+\tif (current_config_kvi)\n+\t\ttype = current_config_kvi->origin_type;\n+\telse if(cf)\n+\t\ttype = cf->origin_type;\n+\telse\n \t\tdie(\"BUG: current_config_origin_type called outside config callback\");\n-\treturn cf->origin_type ? cf->origin_type : \"command line\";\n+\treturn type ? type : \"command line\";\n }\n \n const char *current_config_name(void)\n {\n-\tif (!cf)\n+\tconst char *name;\n+\tif (current_config_kvi)\n+\t\tname = current_config_kvi->filename;\n+\telse if (cf)\n+\t\tname = cf->name;\n+\telse\n \t\tdie(\"BUG: current_config_name called outside config callback\");\n-\treturn cf->name ? cf->name : \"\";\n+\treturn name ? name : \"\";\n }\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 6a77552..3605ef8 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -25,6 +25,9 @@\n * \t\t\t\tascending order of priority from a config_set\n * \t\t\t\tconstructed from files entered as arguments.\n *\n+ * iterate -> iterate over all values using git_config(), and print some\n+ * data for each\n+ *\n * Examples:\n *\n * To print the value with highest priority for key \"foo.bAr Baz.rock\":\n@@ -32,6 +35,20 @@\n *\n */\n \n+static int iterate_cb(const char *var, const char *value, void *data)\n+{\n+\tstatic int nr;\n+\n+\tif (nr++)\n+\t\tputchar('\\n');\n+\n+\tprintf(\"key=%s\\n\", var);\n+\tprintf(\"value=%s\\n\", value ? value : \"(null)\");\n+\tprintf(\"origin=%s\\n\", current_config_origin_type());\n+\tprintf(\"name=%s\\n\", current_config_name());\n+\n+\treturn 0;\n+}\n \n int main(int argc, char **argv)\n {\n@@ -134,6 +151,9 @@ int main(int argc, char **argv)\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tgoto exit1;\n \t\t}\n+\t} else if (!strcmp(argv[1], \"iterate\")) {\n+\t\tgit_config(iterate_cb, NULL);\n+\t\tgoto exit0;\n \t}\n \n \tdie(\"%s: Please check the syntax and the function name\", argv[0]);\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 005d66d..b241338 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -229,4 +229,27 @@ test_expect_success 'error on modifying repo config without repo' '\n \t)\n '\n \n+test_expect_success 'iteration shows correct origins' '\n+\techo \"[foo]bar = from-repo\" >.git/config &&\n+\techo \"[alias]test-config = !test-config\" >.gitconfig &&\n+\tcat >expect <<-EOF &&\n+\tkey=alias.test-config\n+\tvalue=!test-config\n+\torigin=file\n+\tname=$(pwd)/.gitconfig\n+\n+\tkey=foo.bar\n+\tvalue=from-repo\n+\torigin=file\n+\tname=.git/config\n+\n+\tkey=foo.bar\n+\tvalue=from-cmdline\n+\torigin=command line\n+\tname=\n+\tEOF\n+\tgit -c foo.bar=from-cmdline test-config iterate >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.8.2.888.gecb1fe3\n"},{"id":"286944","messageId":"20160518224423.GE22443@sigill.intra.peff.net","threadId":"42377","inReplyTo":"20160518223712.GA18317@sigill.intra.peff.net","subject":"[PATCH 5/6] config: add a notion of \"scope\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T22:44:23Z","receivedAt":"2016-05-18T22:44:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"A config callback passed to git_config() doesn't know very\nmuch about the context in which it sees a variable. It can\nask whether the variable comes from a file, and get the file\nname. But without analyzing the filename (which is hard to\ndo accurately), it cannot tell whether it is in system-level\nconfig, user-level config, or repo-specific config.\n\nGenerally this doesn't matter; the point of not passing this\nto the callback is that it should treat the config the same\nno matter where it comes from. But some programs, like\nupload-pack, are a special case: we should be able to run\nthem in an untrusted repository, which means we cannot use\nany \"dangerous\" config from the repository config file (but\nit is OK to use it from system or user config).\n\nThis patch teaches the config code to record the \"scope\" of\neach variable, and make it available inside config\ncallbacks, similar to how we give access to the filename.\nThe scope is the starting source for a particular parsing\noperation, and remains the same even if we include other\nfiles (so a .git/config which includes another file will\nremain CONFIG_SCOPE_REPO, as it would be similarly\nuntrusted).\n\nSigned-off-by: Jeff King \n---\n cache.h | 11 +++++++++++\n config.c | 23 +++++++++++++++++++++++\n t/helper/test-config.c | 16 ++++++++++++++++\n t/t1308-config-set.sh | 3 +++\n 4 files changed, 53 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 29c579b..81bd96a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1601,6 +1601,16 @@ extern const char *get_log_output_encoding(void);\n extern const char *get_commit_output_encoding(void);\n \n extern int git_config_parse_parameter(const char *, config_fn_t fn, void *data);\n+\n+enum config_scope {\n+\tCONFIG_SCOPE_UNKNOWN = 0,\n+\tCONFIG_SCOPE_SYSTEM,\n+\tCONFIG_SCOPE_GLOBAL,\n+\tCONFIG_SCOPE_REPO,\n+\tCONFIG_SCOPE_CMDLINE,\n+};\n+\n+extern enum config_scope current_config_scope(void);\n extern const char *current_config_origin_type(void);\n extern const char *current_config_name(void);\n \n@@ -1694,6 +1704,7 @@ struct key_value_info {\n \tconst char *filename;\n \tint linenr;\n \tconst char *origin_type;\n+\tenum config_scope scope;\n };\n \n extern NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));\ndiff --git a/config.c b/config.c\nindex 75afdd7..995e886 100644\n--- a/config.c\n+++ b/config.c\n@@ -57,6 +57,15 @@ struct config_source {\n static struct config_source *cf;\n static struct key_value_info *current_config_kvi;\n \n+/*\n+ * Similar to the variables above, this gives access to the \"scope\" of the\n+ * current value (repo, global, etc). For cached values, it can be found via\n+ * the current_config_kvi as above. During parsing, the current value can be\n+ * found in this variable. It's not part of \"cf\" because it transcends a single\n+ * file (i.e., a file included from .git/config is still in \"repo\" scope).\n+ */\n+static enum config_scope current_parsing_scope;\n+\n static int zlib_compression_seen;\n \n /*\n@@ -1233,22 +1242,27 @@ static int do_git_config_sequence(config_fn_t fn, void *data)\n \tchar *user_config = expand_user_path(\"~/.gitconfig\");\n \tchar *repo_config = git_pathdup(\"config\");\n \n+\tcurrent_parsing_scope = CONFIG_SCOPE_SYSTEM;\n \tif (git_config_system() && !access_or_die(git_etc_gitconfig(), R_OK, 0))\n \t\tret += git_config_from_file(fn, git_etc_gitconfig(),\n \t\t\t\t\t data);\n \n+\tcurrent_parsing_scope = CONFIG_SCOPE_GLOBAL;\n \tif (xdg_config && !access_or_die(xdg_config, R_OK, ACCESS_EACCES_OK))\n \t\tret += git_config_from_file(fn, xdg_config, data);\n \n \tif (user_config && !access_or_die(user_config, R_OK, ACCESS_EACCES_OK))\n \t\tret += git_config_from_file(fn, user_config, data);\n \n+\tcurrent_parsing_scope = CONFIG_SCOPE_REPO;\n \tif (repo_config && !access_or_die(repo_config, R_OK, 0))\n \t\tret += git_config_from_file(fn, repo_config, data);\n \n+\tcurrent_parsing_scope = CONFIG_SCOPE_CMDLINE;\n \tif (git_config_from_parameters(fn, data) < 0)\n \t\tdie(_(\"unable to parse command-line config\"));\n \n+\tcurrent_parsing_scope = CONFIG_SCOPE_UNKNOWN;\n \tfree(xdg_config);\n \tfree(user_config);\n \tfree(repo_config);\n@@ -1387,6 +1401,7 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n \t\tkv_info->linenr = -1;\n \t\tkv_info->origin_type = NULL;\n \t}\n+\tkv_info->scope = current_parsing_scope;\n \tsi->util = kv_info;\n \n \treturn 0;\n@@ -2486,3 +2501,11 @@ const char *current_config_name(void)\n \t\tdie(\"BUG: current_config_name called outside config callback\");\n \treturn name ? name : \"\";\n }\n+\n+enum config_scope current_config_scope(void)\n+{\n+\tif (current_config_kvi)\n+\t\treturn current_config_kvi->scope;\n+\telse\n+\t\treturn current_parsing_scope;\n+}\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 3605ef8..509aeef 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -35,6 +35,21 @@\n *\n */\n \n+static const char *scope_name(enum config_scope scope)\n+{\n+\tswitch (scope) {\n+\tcase CONFIG_SCOPE_SYSTEM:\n+\t\treturn \"system\";\n+\tcase CONFIG_SCOPE_GLOBAL:\n+\t\treturn \"global\";\n+\tcase CONFIG_SCOPE_REPO:\n+\t\treturn \"repo\";\n+\tcase CONFIG_SCOPE_CMDLINE:\n+\t\treturn \"cmdline\";\n+\tdefault:\n+\t\treturn \"unknown\";\n+\t}\n+}\n static int iterate_cb(const char *var, const char *value, void *data)\n {\n \tstatic int nr;\n@@ -46,6 +61,7 @@ static int iterate_cb(const char *var, const char *value, void *data)\n \tprintf(\"value=%s\\n\", value ? value : \"(null)\");\n \tprintf(\"origin=%s\\n\", current_config_origin_type());\n \tprintf(\"name=%s\\n\", current_config_name());\n+\tprintf(\"scope=%s\\n\", scope_name(current_config_scope()));\n \n \treturn 0;\n }\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex b241338..486b41c 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -237,16 +237,19 @@ test_expect_success 'iteration shows correct origins' '\n \tvalue=!test-config\n \torigin=file\n \tname=$(pwd)/.gitconfig\n+\tscope=global\n \n \tkey=foo.bar\n \tvalue=from-repo\n \torigin=file\n \tname=.git/config\n+\tscope=repo\n \n \tkey=foo.bar\n \tvalue=from-cmdline\n \torigin=command line\n \tname=\n+\tscope=cmdline\n \tEOF\n \tgit -c foo.bar=from-cmdline test-config iterate >actual &&\n \ttest_cmp expect actual\n-- \n2.8.2.888.gecb1fe3\n"},{"id":"286945","messageId":"20160518224537.GF22443@sigill.intra.peff.net","threadId":"42377","inReplyTo":"20160518223712.GA18317@sigill.intra.peff.net","subject":"[PATCH 6/6] upload-pack: provide a hook for running pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-18T22:45:37Z","receivedAt":"2016-05-18T22:45:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"When upload-pack serves a client request, it turns to\npack-objects to do the heavy lifting of creating a\npackfile. There's no easy way to intercept the call to\npack-objects, but there are a few good reasons to want to do\nso:\n\n 1. If you're debugging a client or server issue with\n fetching, you may want to store a copy of the generated\n packfile.\n\n 2. If you're gathering data from real-world fetches for\n performance analysis or debugging, storing a copy of\n the arguments and stdin lets you replay the pack\n generation at your leisure.\n\n 3. You may want to insert a caching layer around\n pack-objects; it is the most CPU- and memory-intensive\n part of serving a fetch, and its output is a pure\n function[1] of its input, making it an ideal place to\n consolidate identical requests.\n\nThis patch adds a simple \"hook\" interface to intercept calls\nto pack-objects. The new test demonstrates how it can be\nused for debugging (using it for caching is a\nstraightforward extension; the tricky part is writing the\nactual caching layer).\n\nThis hook is unlike the normal hook scripts found in the\n\"hooks/\" directory of a repository. Because we promise that\nupload-pack is safe to run in an untrusted repository, we\ncannot execute arbitrary code or commands found in the\nrepository (neither in hooks/, nor in the config). So\ninstead, this hook is triggered from a config variable that\nis explicitly ignored in the per-repo config.\n\nThe config variable holds the actual shell command to run as\nthe hook. Another approach would be to simply treat it as a\nboolean: \"should I respect the upload-pack hooks in this\nrepo?\", and then run the script from \"hooks/\" as we usually\ndo. However, that isn't as flexible; there's no way to run a\nhook approved by the site administrator (e.g., in\n\"/etc/gitconfig\") on a repository whose contents are not\ntrusted. The approach taken by this patch is more\nfine-grained, if a little less conventional for git hooks\n(it does behave similar to other configured commands like\ndiff.external, etc).\n\n[1] Pack-objects isn't _actually_ a pure function. Its\n output depends on the exact packing of the object\n database, and if multi-threading is used for delta\n compression, can even differ racily. But for the\n purposes of caching, that's OK; of the many possible\n outputs for a given input, it is sufficient only that we\n output one of them.\n\nSigned-off-by: Jeff King \n---\n Documentation/config.txt | 15 +++++++++++\n t/t5544-pack-objects-hook.sh | 62 ++++++++++++++++++++++++++++++++++++++++++++\n upload-pack.c | 13 +++++++++-\n 3 files changed, 89 insertions(+), 1 deletion(-)\n create mode 100755 t/t5544-pack-objects-hook.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex e4cd291..b9b0541 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -2872,6 +2872,21 @@ uploadpack.keepAlive::\n \t`uploadpack.keepAlive` seconds. Setting this option to 0\n \tdisables keepalive packets entirely. The default is 5 seconds.\n \n+uploadpack.packObjectsHook::\n+\tIf this option is set, when `upload-pack` would run\n+\t`git pack-objects` to create a packfile for a client, it will\n+\trun this shell command instead. The `pack-objects` command and\n+\targuments it _would_ have run (including the `git pack-objects`\n+\tat the beginning) are appended to the shell command. The stdin\n+\tand stdout of the hook are treated as if `pack-objects` itself\n+\twas run. I.e., `upload-pack` will feed input intended for\n+\t`pack-objects` to the hook, and expects a completed packfile on\n+\tstdout.\n++\n+Note that this configuration variable is ignored if it is seen in the\n+repository-level config (this is a safety measure against fetching from\n+untrusted repositories).\n+\n url..insteadOf::\n \tAny URL that starts with this value will be rewritten to\n \tstart, instead, with . In cases where some site serves a\ndiff --git a/t/t5544-pack-objects-hook.sh b/t/t5544-pack-objects-hook.sh\nnew file mode 100755\nindex 0000000..4357af1\n--- /dev/null\n+++ b/t/t5544-pack-objects-hook.sh\n@@ -0,0 +1,62 @@\n+#!/bin/sh\n+\n+test_description='test custom script in place of pack-objects'\n+. ./test-lib.sh\n+\n+test_expect_success 'create some history to fetch' '\n+\ttest_commit one &&\n+\ttest_commit two\n+'\n+\n+test_expect_success 'create debugging hook script' '\n+\twrite_script .git/hook <<-\\EOF\n+\t\techo >&2 \"hook running\"\n+\t\techo \"$*\" >hook.args\n+\t\tcat >hook.stdin\n+\t\t\"$@\" hook.stdout\n+\t\tcat hook.stdout\n+\tEOF\n+'\n+\n+clear_hook_results () {\n+\trm -rf .git/hook.* dst.git\n+}\n+\n+test_expect_success 'hook runs via global config' '\n+\tclear_hook_results &&\n+\ttest_config_global uploadpack.packObjectsHook ./hook &&\n+\tgit clone --no-local . dst.git 2>stderr &&\n+\tgrep \"hook running\" stderr\n+'\n+\n+test_expect_success 'hook outputs are sane' '\n+\t# check that we recorded a usable pack\n+\tgit index-pack --stdin <.git/hook.stdout &&\n+\n+\t# check that we recorded args and stdin. We do not check\n+\t# the full argument list or the exact pack contents, as it would make\n+\t# the test brittle. So just sanity check that we could replay\n+\t# the packing procedure.\n+\tgrep \"^git\" .git/hook.args &&\n+\t$(cat .git/hook.args) <.git/hook.stdin >replay\n+'\n+\n+test_expect_success 'hook runs from -c config' '\n+\tclear_hook_results &&\n+\tgit clone --no-local \\\n+\t -u \"git -c uploadpack.packObjectsHook=./hook upload-pack\" \\\n+\t . dst.git 2>stderr &&\n+\tgrep \"hook running\" stderr\n+'\n+\n+test_expect_success 'hook does not run from repo config' '\n+\tclear_hook_results &&\n+\ttest_config uploadpack.packObjectsHook \"./hook\" &&\n+\tgit clone --no-local . dst.git 2>stderr &&\n+\t! grep \"hook running\" stderr &&\n+\ttest_path_is_missing .git/hook.args &&\n+\ttest_path_is_missing .git/hook.stdin &&\n+\ttest_path_is_missing .git/hook.stdout\n+'\n+\n+test_done\ndiff --git a/upload-pack.c b/upload-pack.c\nindex f19444d..8979be6 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -52,6 +52,7 @@ static int keepalive = 5;\n static int use_sideband;\n static int advertise_refs;\n static int stateless_rpc;\n+static const char *pack_objects_hook;\n \n static void reset_timeout(void)\n {\n@@ -93,6 +94,14 @@ static void create_pack_file(void)\n \tint i;\n \tFILE *pipe_fd;\n \n+\tif (!pack_objects_hook)\n+\t\tpack_objects.git_cmd = 1;\n+\telse {\n+\t\targv_array_push(&pack_objects.args, pack_objects_hook);\n+\t\targv_array_push(&pack_objects.args, \"git\");\n+\t\tpack_objects.use_shell = 1;\n+\t}\n+\n \tif (shallow_nr) {\n \t\targv_array_push(&pack_objects.args, \"--shallow-file\");\n \t\targv_array_push(&pack_objects.args, \"\");\n@@ -115,7 +124,6 @@ static void create_pack_file(void)\n \tpack_objects.in = -1;\n \tpack_objects.out = -1;\n \tpack_objects.err = -1;\n-\tpack_objects.git_cmd = 1;\n \n \tif (start_command(&pack_objects))\n \t\tdie(\"git upload-pack: unable to fork git-pack-objects\");\n@@ -812,6 +820,9 @@ static int upload_pack_config(const char *var, const char *value, void *unused)\n \t\tkeepalive = git_config_int(var, value);\n \t\tif (!keepalive)\n \t\t\tkeepalive = -1;\n+\t} else if (current_config_scope() != CONFIG_SCOPE_REPO) {\n+\t\tif (!strcmp(\"uploadpack.packobjectshook\", var))\n+\t\t\treturn git_config_string(&pack_objects_hook, var, value);\n \t}\n \treturn parse_hide_refs_config(var, value, \"uploadpack\");\n }\n-- \n2.8.2.888.gecb1fe3\n"},{"id":"286950","messageId":"20160519000821.GA22543@sigill.intra.peff.net","threadId":"42377","inReplyTo":"20160518224323.GD22443@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] config: return configset value for current_config_ functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-19T00:08:21Z","receivedAt":"2016-05-19T00:08:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 18, 2016 at 06:43:23PM -0400, Jeff King wrote:\n\n> cache.h | 1 +\n> config.c | 51 +++++++++++++++++++++++++++++++++++++++++---------\n> t/helper/test-config.c | 20 ++++++++++++++++++++\n> t/t1308-config-set.sh | 23 +++++++++++++++++++++++\n> [...]\n> +test_expect_success 'iteration shows correct origins' '\n> +\techo \"[alias]test-config = !test-config\" >.gitconfig &&\n> [...]\n> +\tgit -c foo.bar=from-cmdline test-config iterate >actual &&\n\nWhile writing and testing this, I got bit by e6e7530 (test helpers: move\ntest-* to t/helper/ subdirectory, 2016-04-13). I had an old test-config\nbinary leftover in the root of my repository, and the new one was\ncorrectly built in t/helper/. Running \"test-config\" is fine, but inside\nthe git alias, it sticks the repository root at the front of $PATH\n(because it's the exec-path). And so it ran the old version of\ntest-config, which did not understand my new \"iterate\" option.\n\nNow I'll admit what I'm doing here is pretty funny (running test-* from\nan alias). I'm doing it because I want to see how the program operates\nwith the \"-c\" config, and it's nicer to spell it as a user would,\ninstead of munging $GIT_CONFIG_PARAMETERS directly.\n\nSo I'm not sure if it's worth working around or not. The single tree\nstate produced by this commit is fine, but it does behave badly if\nthere's leftover cruft from a pre-e6e7530 build. A more robust version\nwould look more like:\n\n sq=\\' ;# to ease quoting later\n ...\n GIT_CONFIG_PARAMETERS=${sq}foo.bar=from-cmdline${sq} test-config ...\n\nWhich is ugly, but it's probably worth it to avoid the flakiness.\n\nThe other option is to somehow make bin-wrappers more robust. E.g., it\nwould be nice if we didn't actually point into the repository root\ndirectly, but rather somehow linked all of the git-* entries that\n_would_ be installed into the exec-path into a fake exec-path (or\nalternatively, actually build them directly into that fake exec-path).\n\nThat's a much bigger change, though. Given how unlikely the sequence of\nsteps in my test is, maybe it's better to just work around it in this\none case.\n\n-Peff\n"},{"id":"286951","messageId":"20160519001433.GB22543@sigill.intra.peff.net","threadId":"42377","inReplyTo":"20160518224537.GF22443@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] upload-pack: provide a hook for running pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-19T00:14:33Z","receivedAt":"2016-05-19T00:14:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 18, 2016 at 06:45:37PM -0400, Jeff King wrote:\n\n> @@ -93,6 +94,14 @@ static void create_pack_file(void)\n> \tint i;\n> \tFILE *pipe_fd;\n> \n> +\tif (!pack_objects_hook)\n> +\t\tpack_objects.git_cmd = 1;\n> +\telse {\n> +\t\targv_array_push(&pack_objects.args, pack_objects_hook);\n> +\t\targv_array_push(&pack_objects.args, \"git\");\n> +\t\tpack_objects.use_shell = 1;\n> +\t}\n\nI waffled on the \"shell\" thing here. It's more flexible, and matches\nother commands we let the user specify (like diff.external). But it also\nmakes it harder to do more hook-like things, like \"run this hook if it\nexists and has the executable bit set\".\n\nSo should it just be a path to a single command/script?\n\nWith the shell thing, you can set it to:\n\n test -e /path/to/script && /path/to/script ...\n\nif you want, but that's a little more arcane (and incurs an extra shell\ninvocation that we could otherwise skip).\n\nAnd on the same note, if it _were_ just path-to-script, would we want it\nto skip the hook if it's missing or not executable? For normal hooks, if\nthat's the case, we know it's because the user doesn't want to use the\nhook. But here, the user has taken an overt action to tell us about the\nhook; would it be surprising if we silently skipped it because it wasn't\navailable?\n\nI like that it would \"fail open\", so a misconfiguration doesn't break a\nserver. But maybe people would find that off-putting.\n\n-Peff\n"},{"id":"286976","messageId":"CACBZZX5SVJ2CSB0AS3Lj1A8_S+ejGOPUDn6Sc3whotkyFwxEiA@mail.gmail.com","threadId":"42377","inReplyTo":"20160518224537.GF22443@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] upload-pack: provide a hook for running pack-objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-05-19T10:12:43Z","receivedAt":"2016-05-19T10:12:43Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, May 19, 2016 at 12:45 AM, Jeff King wrote:\n> 3. You may want to insert a caching layer around\n> pack-objects; it is the most CPU- and memory-intensive\n> part of serving a fetch, and its output is a pure\n> function[1] of its input, making it an ideal place to\n> consolidate identical requests.\n\nCool to see this on the list after we talked briefly about this at Git\nMerge. Being able to cache this so simply is a great optimization.\n\nAs I recall you guys at GitHub ended up writing your own utility to\ncache output depending on stdin/argv because none existed already.\n\nIf anyone on-list knows about a generic command-line utility like that\n(because apparently Peff couldn't think of any, and neither can I)\nthat would be useful to know.\n\n> This hook is unlike the normal hook scripts found in the\n> \"hooks/\" directory of a repository. Because we promise that\n> upload-pack is safe to run in an untrusted repository, we\n> cannot execute arbitrary code or commands found in the\n> repository (neither in hooks/, nor in the config). So\n> instead, this hook is triggered from a config variable that\n> is explicitly ignored in the per-repo config.\n\nSo do I understand correctly that you're trying to guard against the\ncase where you e.g.:\n\n rsync untrusted.example.com:/tmp/poison.git /tmp/\n git clone /tmp/poison.git /tmp/safe.git\n\nNot hosing your system if the poison.git/config has a\nuploadpack.packObjectsHook that's \"sudo rm -rf /\".\n\nAnd similarly having this run the hook on the remote:\n\n # foo.example.com has a /etc/gitconfig with\nuploadpack.packObjectsHook \"sudo rm -rf /\";\n echo -n | ssh foo.example.com \"git upload-pack /tmp/poison.git\n\nBut not this:\n\n # bar.example.com has a /tmp/poison.git/config with\nuploadpack.packObjectsHook \"sudo rm -rf /\";\n echo -n | ssh foo.example.com \"git upload-pack /tmp/poison.git\n\nWe've already accepted that \"push\" hooks like the pre-receive or\nupdate hook can do something malicious like this, so on one hand maybe\nwe should say if you scp raw *.git repositories with hooks this sort\nof thing might happen, or if you ssh to a remote box and run their\nper-repo hooks it's really their problem to make sure their users\ndon't run malicious hooks on your behalf.\n\nBut I agree with you (if I've understand what this actually does) that\nsaying that it's always safe to \"git clone\" a repository is more\nvaluable and worth jumping through some hoops for.\n\nBut as you point out this makes the hook interface a bit unusual.\nWouldn't this give us the same security and normalize the hook\ninterface:\n\n * Don't do the uploadpack.packObjectsHook variable, just have a\nnormal \"pack-objects\" hook that works like any other git hook\n * By default we don't run this hook unless core.runDangerousHooks (or\nwhatever we call it) is true.\n * The core.runDangerousHooks variable cannot be set on a per-repo\nbasis using your new config facility.\n * If there's a pack-objects hook and core.runDangerousHooks isn't\ntrue we warn \"not executing potentially unsafe hook $path_to_hook\" and\ncarry on\n\nThis would allow use-cases that are a bit inconvenient with your patch\n(again, if I'm understanding it correctly):\n\n * I can set core.runDangerousHooks=true in /etc/gitconfig on my git\nserver because I also control all the repos, and I want to experiment\nwith trying this on a per-repo basis for users that are cloning from\nme.\n * I can similarly play with this locally knowing I'm only cloning\nrepos I trust by setting core.runDangerousHooks=true in ~/.gitconfig\n"},{"id":"286978","messageId":"20160519120848.GC3050@sigill.intra.peff.net","threadId":"42377","inReplyTo":"CACBZZX5SVJ2CSB0AS3Lj1A8_S+ejGOPUDn6Sc3whotkyFwxEiA@mail.gmail.com","subject":"Re: [PATCH 6/6] upload-pack: provide a hook for running pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-19T12:08:49Z","receivedAt":"2016-05-19T12:08:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 19, 2016 at 12:12:43PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> On Thu, May 19, 2016 at 12:45 AM, Jeff King wrote:\n> > 3. You may want to insert a caching layer around\n> > pack-objects; it is the most CPU- and memory-intensive\n> > part of serving a fetch, and its output is a pure\n> > function[1] of its input, making it an ideal place to\n> > consolidate identical requests.\n> \n> Cool to see this on the list after we talked briefly about this at Git\n> Merge. Being able to cache this so simply is a great optimization.\n> \n> As I recall you guys at GitHub ended up writing your own utility to\n> cache output depending on stdin/argv because none existed already.\n\nYeah, we do have such a tool internally. It's possible we may one day\nopen-source that, but there aren't plans to do so right now.\n\nI don't know whether this kind of caching would be useful to most sites\nor not. It's good if you have lots of clients asking you for the same\nthing at roughly the same time (say, somebody using \"git pull\" as a\ndeploy mechanism from their AWS cluster), but otherwise not.\n\n> So do I understand correctly that you're trying to guard against the\n> case where you e.g.:\n> \n> rsync untrusted.example.com:/tmp/poison.git /tmp/\n> git clone /tmp/poison.git /tmp/safe.git\n> \n> Not hosing your system if the poison.git/config has a\n> uploadpack.packObjectsHook that's \"sudo rm -rf /\".\n\nI'm not that worried about this case, as it's just not that common. I\nthink we're more concerned with two cases:\n\n 1. multi-user servers where you ssh as yourself, but then access\n repositories owned by somebody else. This is basically the ssh case\n you described later.\n\n 2. hosting sites that run git-daemon as the \"daemon\" user, but serve\n repositories owned by random untrusted users (where you would not\n want those users to run arbitrary code as \"daemon\").\n\n> We've already accepted that \"push\" hooks like the pre-receive or\n> update hook can do something malicious like this, so on one hand maybe\n> we should say if you scp raw *.git repositories with hooks this sort\n> of thing might happen, or if you ssh to a remote box and run their\n> per-repo hooks it's really their problem to make sure their users\n> don't run malicious hooks on your behalf.\n\nYeah, we make no promises for repositories that you push to. It's _only_\nfor the fetching side. It's kind of a funny distinction, but it's one we\nhave maintained since the beginning of git, and I do think there are\nreal sites that depend on it (see, e.g., the history of the\npost-upload-pack hook added in the v1.6.x time frame).\n\nRsyncing a repository is generally of questionable safety. It's OK to\nfetch from the result, but certainly not to run \"git log\" (which can run\narbitrary commands via external diff, etc).\n\n> But as you point out this makes the hook interface a bit unusual.\n> Wouldn't this give us the same security and normalize the hook\n> interface:\n> \n> * Don't do the uploadpack.packObjectsHook variable, just have a\n> normal \"pack-objects\" hook that works like any other git hook\n> * By default we don't run this hook unless core.runDangerousHooks (or\n> whatever we call it) is true.\n> * The core.runDangerousHooks variable cannot be set on a per-repo\n> basis using your new config facility.\n> * If there's a pack-objects hook and core.runDangerousHooks isn't\n> true we warn \"not executing potentially unsafe hook $path_to_hook\" and\n> carry on\n\nThis is the \"could we just set a bool\" option I discussed in the commit\nmessage. The problem is that it doesn't let the admin say \"I don't trust\nthese repositories, but I _do_ want to run just this one hook when\nserving them, and not any other hooks\".\n\n> This would allow use-cases that are a bit inconvenient with your patch\n> (again, if I'm understanding it correctly):\n> \n> * I can set core.runDangerousHooks=true in /etc/gitconfig on my git\n> server because I also control all the repos, and I want to experiment\n> with trying this on a per-repo basis for users that are cloning from\n> me.\n> * I can similarly play with this locally knowing I'm only cloning\n> repos I trust by setting core.runDangerousHooks=true in ~/.gitconfig\n\nYes, those use cases are not well served by the git config alone. But\nyou can do them (and much more) once your trusted hook is running (by\nchecking $GIT_DIR, or looking in a database, or whatever you want).\n\n-Peff\n"},{"id":"286987","messageId":"CACBZZX7nuy3RYPQ5e_FFqvCTStZka49ZSNNe9q2agffa7fp-EA@mail.gmail.com","threadId":"42377","inReplyTo":"20160519120848.GC3050@sigill.intra.peff.net","subject":"Re: [PATCH 6/6] upload-pack: provide a hook for running pack-objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2016-05-19T14:54:37Z","receivedAt":"2016-05-19T14:54:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, May 19, 2016 at 2:08 PM, Jeff King wrote:\n> On Thu, May 19, 2016 at 12:12:43PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> But as you point out this makes the hook interface a bit unusual.\n>> Wouldn't this give us the same security and normalize the hook\n>> interface:\n>>\n>> * Don't do the uploadpack.packObjectsHook variable, just have a\n>> normal \"pack-objects\" hook that works like any other git hook\n>> * By default we don't run this hook unless core.runDangerousHooks (or\n>> whatever we call it) is true.\n>> * The core.runDangerousHooks variable cannot be set on a per-repo\n>> basis using your new config facility.\n>> * If there's a pack-objects hook and core.runDangerousHooks isn't\n>> true we warn \"not executing potentially unsafe hook $path_to_hook\" and\n>> carry on\n>\n> This is the \"could we just set a bool\" option I discussed in the commit\n> message. The problem is that it doesn't let the admin say \"I don't trust\n> these repositories, but I _do_ want to run just this one hook when\n> serving them, and not any other hooks\".\n\nIndeed. I wonder if there's really any overlap in practice between\nsystems administrators on a central Git server that are going to want\nthis relatively obscure feature *but* have potentially malicious users\n/ repos, or enough to warrant this unusual edge case in how this\nspecific hook is configured, as opposed to reducing the special case\nin how the hook is run with something like core.runDangerousHooks.\n\nI'm definitely not saying that these patches should be blocked by\nthis, but it occurs to me that both your uploadpack.packObjectsHook\nimplementation and my proposed core.runDangerousHooks which normalizes\nit a bit in some ways, but leaves it as a special case in others, are\nboth stumbling their way toward hacks that we might also solve with\nsome generally configurable restrictions system that takes advantage\nof your earlier patches, e.g.:\n\n $ cat /etc/gitconfig\n # Not \"repository\" so hooksPath can't be set per-repo\n [core]\n configRestriction = \"core.hooksPath: system, global\"\n hooksPath = \"/etc/git/hooks\"\n disableHook.pack-objects = false # \"true\" by default\n $ ls /etc/git/hooks/\n # pre-receive, update etc. would just wrap the repository hook,\n # but pack-objects is global\n pre-receive update pack-objects, [...]\n\nOf course those are some rather large hoops to jump through just to\naccomplish this particular thing, but it would be more generally\ncomposable and you could e.g. say users can't disable gc.auto or\nwhatever on their repos if they're hosted on your server. Which of\ncourse assumes that you control the git binary and they can't run\ntheir own.\n\n>> This would allow use-cases that are a bit inconvenient with your patch\n>> (again, if I'm understanding it correctly):\n>>\n>> * I can set core.runDangerousHooks=true in /etc/gitconfig on my git\n>> server because I also control all the repos, and I want to experiment\n>> with trying this on a per-repo basis for users that are cloning from\n>> me.\n>> * I can similarly play with this locally knowing I'm only cloning\n>> repos I trust by setting core.runDangerousHooks=true in ~/.gitconfig\n>\n> Yes, those use cases are not well served by the git config alone. But\n> you can do them (and much more) once your trusted hook is running (by\n> checking $GIT_DIR, or looking in a database, or whatever you want).\n\nYeah, the reason I'm prodding you about this is because I want to test\nthis out at some point, and a *really* nice thing about the Git\nconfiguration facility is that you can test all these sorts of things\non a per-repo basis now due to how all the git-config variables work\nnow.\n\nWith uploadpack.packObjectsHook you *can* do that by defining a global\npass-through hook, but it makes it more of a hassle to test changes\nthat straddle the divide between testing & production.\n\nI.e. I might test this on one repo on our main git server, or on one\nactive repo, i.e. I don't have to deal with the case where I make some\nsilly syntax/logic error in the uploadpack.packObjectsHook dispatch\ncode (which is only needed for the security consideration) and that\nimpacts all repositories on the machine.\n"},{"id":"287470","messageId":"CAPc5daV6bdUKS-ExHmpT4Ppy2S832NXoyPw7aOLP7fG=WrBPgg@mail.gmail.com","threadId":"42377","inReplyTo":"20160518223712.GA18317@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 0/6] pack-objects hook for upload-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-25T00:59:15Z","receivedAt":"2016-05-25T00:59:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Wed, May 18, 2016 at 3:37 PM, Jeff King wrote:\n> I've often wanted to intercept the call from upload-pack to\n> pack-objects. The final patch in this series goes into more detail, but\n> basically it's good for:\n>\n> 1. Capturing the output from pack-objects for debugging/inspection.\n>\n> 2. Capturing the input to pack-objects to replay for debugging or\n> performance analysis.\n>\n> 3. Caching pack-objects output.\n>\n> It's pretty trivial to add a hook to run instead of pack-objects (and\n> the hook would just run pack-objects itself). But we don't want to run\n> hooks in upload-pack, because we don't necessarily trust the repository\n> we're running in.\n\nAlthough I'd need to study the final step a bit more carefully than I did,\noverall I think these are good changes. The way the callbacks learn\nabout the origin of the configuration may have to be rethought in the\nlong run, though. We already have been relying on a filename thing\nkept separately as a global variable, and the approach taken by this\nseries extends it, so we are not making anything fundamentally worse,\nbut at some point we may need to bite the bullet and pass kv-info as\nan extra callback parameter or something.\n\nThanks.\n"},{"id":"287555","messageId":"20160526053758.GA21580@sigill.intra.peff.net","threadId":"42377","inReplyTo":"CACBZZX7nuy3RYPQ5e_FFqvCTStZka49ZSNNe9q2agffa7fp-EA@mail.gmail.com","subject":"Re: [PATCH 6/6] upload-pack: provide a hook for running pack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-26T05:37:58Z","receivedAt":"2016-05-26T05:37:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 19, 2016 at 04:54:37PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> > This is the \"could we just set a bool\" option I discussed in the commit\n> > message. The problem is that it doesn't let the admin say \"I don't trust\n> > these repositories, but I _do_ want to run just this one hook when\n> > serving them, and not any other hooks\".\n> \n> Indeed. I wonder if there's really any overlap in practice between\n> systems administrators on a central Git server that are going to want\n> this relatively obscure feature *but* have potentially malicious users\n> / repos, or enough to warrant this unusual edge case in how this\n> specific hook is configured, as opposed to reducing the special case\n> in how the hook is run with something like core.runDangerousHooks.\n\nI dunno. Certainly I am not running such a site. But something like\nkernel.org might fit into that boat. For a long time I think people had\nactual shell accounts and a common git-daemon served the repositories. I\nthink that these days it might be more locked-down, though.\n\n> I'm definitely not saying that these patches should be blocked by\n> this, but it occurs to me that both your uploadpack.packObjectsHook\n> implementation and my proposed core.runDangerousHooks which normalizes\n> it a bit in some ways, but leaves it as a special case in others, are\n> both stumbling their way toward hacks that we might also solve with\n> some generally configurable restrictions system that takes advantage\n> of your earlier patches, e.g.:\n> \n> $ cat /etc/gitconfig\n> # Not \"repository\" so hooksPath can't be set per-repo\n> [core]\n> configRestriction = \"core.hooksPath: system, global\"\n\nI was hoping to avoid setting up configuration restriction via the\nconfiguration files, if only because it implies some ordering in the\nparsing. So for example, you'd need to do a separate pass to load the\nrestrictions system, and then actually parse the config.\n\nI guess that's not too bad with the caching system that's in place,\nthough.\n\n> Of course those are some rather large hoops to jump through just to\n> accomplish this particular thing, but it would be more generally\n> composable and you could e.g. say users can't disable gc.auto or\n> whatever on their repos if they're hosted on your server. Which of\n> course assumes that you control the git binary and they can't run\n> their own.\n\nYeah, I was also hoping to avoid something too baroque. :) I don't know\nif there's much value in restricting things like gc.auto. If they can\nmake arbitrary edits to the config file, they can run arbitrary code. I\nthink this is _just_ about protecting a git-daemon serving the untrusted\nrepositories (or a user fetching from an untrusted other-user on the\nsystem).\n\n> Yeah, the reason I'm prodding you about this is because I want to test\n> this out at some point, and a *really* nice thing about the Git\n> configuration facility is that you can test all these sorts of things\n> on a per-repo basis now due to how all the git-config variables work\n> now.\n> \n> With uploadpack.packObjectsHook you *can* do that by defining a global\n> pass-through hook, but it makes it more of a hassle to test changes\n> that straddle the divide between testing & production.\n\nOne thing I didn't elaborate on is that the \"don't respect this key from\nthe repo config\" could be made more featureful. For example, your\ncore.allowDangerousHooks could just as easily be an environment\nvariable: $GIT_ALLOW_DANGEROUS_CONFIG. [1] \n\nAnd then you could set that on your servers, and only set\nuploadpack.packObjectsHook in the repositories you wanted, achieving\nyour goal.\n\nThis does still leave the pack-objects hook unlike the other hooks (in\nthat it leaves the command in the config rather than in a script), but I\nactually like that flexibility. Being able to use \"git -c\" to set the\nhook for a one-shot invocation is kind of nice (though you do have to do\ntricks with \"--upload-pack=\" to get it to cross the remote boundary).\n\n-Peff\n\n[1] We also talked long ago (in the v1.6.x days, regarding a\npost-upload-pack hook) of auto-enabling \"dangerous\" hooks when getuid()\nmatched the owner of the hook. We could do the same thing for the config\nfile (though TBH, it is confusing enough of a rule that I think I prefer\nsomething like the explicit environment variable).\n"},{"id":"287556","messageId":"20160526054418.GB21580@sigill.intra.peff.net","threadId":"42377","inReplyTo":"CAPc5daV6bdUKS-ExHmpT4Ppy2S832NXoyPw7aOLP7fG=WrBPgg@mail.gmail.com","subject":"Re: [PATCH/RFC 0/6] pack-objects hook for upload-pack","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-26T05:44:18Z","receivedAt":"2016-05-26T05:44:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 24, 2016 at 05:59:15PM -0700, Junio C Hamano wrote:\n\n> On Wed, May 18, 2016 at 3:37 PM, Jeff King wrote:\n> > I've often wanted to intercept the call from upload-pack to\n> > pack-objects. The final patch in this series goes into more detail, but\n> > basically it's good for:\n> >\n> > 1. Capturing the output from pack-objects for debugging/inspection.\n> >\n> > 2. Capturing the input to pack-objects to replay for debugging or\n> > performance analysis.\n> >\n> > 3. Caching pack-objects output.\n> >\n> > It's pretty trivial to add a hook to run instead of pack-objects (and\n> > the hook would just run pack-objects itself). But we don't want to run\n> > hooks in upload-pack, because we don't necessarily trust the repository\n> > we're running in.\n> \n> Although I'd need to study the final step a bit more carefully than I did,\n> overall I think these are good changes. The way the callbacks learn\n> about the origin of the configuration may have to be rethought in the\n> long run, though. We already have been relying on a filename thing\n> kept separately as a global variable, and the approach taken by this\n> series extends it, so we are not making anything fundamentally worse,\n> but at some point we may need to bite the bullet and pass kv-info as\n> an extra callback parameter or something.\n\nYeah, I had the same thought while working on this, but just didn't want\nto have to tweak every config callback. As you say, I don't think this\nmakes anything fundamentally worse, though. I'm inclined to go with this\nstrategy, especially with the extra die(\"BUG\") safety added here. But I\ncan look into changing the callbacks if you feel strongly.\n\n-Peff\n\nPS Did you have any thoughts on the t/helper problem mentioned in:\n\n http://article.gmane.org/gmane.comp.version-control.git/295029\n\n I suspect it will bite you if you try merging/testing this.\n"},{"id":"287557","messageId":"CACsJy8Ad=yN6aLkH9B6ujUNUvPT-b+jw+CwJORD5Fh1jYeOUZQ@mail.gmail.com","threadId":"42377","inReplyTo":"20160519000821.GA22543@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] config: return configset value for current_config_ functions","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-05-26T07:47:15Z","receivedAt":"2016-05-26T07:47:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 19, 2016 at 7:08 AM, Jeff King wrote:\n> On Wed, May 18, 2016 at 06:43:23PM -0400, Jeff King wrote:\n>\n>> cache.h | 1 +\n>> config.c | 51 +++++++++++++++++++++++++++++++++++++++++---------\n>> t/helper/test-config.c | 20 ++++++++++++++++++++\n>> t/t1308-config-set.sh | 23 +++++++++++++++++++++++\n>> [...]\n>> +test_expect_success 'iteration shows correct origins' '\n>> + echo \"[alias]test-config = !test-config\" >.gitconfig &&\n\nHow about using 'which' to get absolute path for test-config and put\nit here? Then we don't rely on $PATH anymore.\n\n>> [...]\n>> + git -c foo.bar=from-cmdline test-config iterate >actual &&\n>\n> While writing and testing this, I got bit by e6e7530 (test helpers: move\n> test-* to t/helper/ subdirectory, 2016-04-13). I had an old test-config\n> binary leftover in the root of my repository, and the new one was\n> correctly built in t/helper/. Running \"test-config\" is fine, but inside\n> the git alias, it sticks the repository root at the front of $PATH\n> (because it's the exec-path). And so it ran the old version of\n> test-config, which did not understand my new \"iterate\" option.\n>\n> Now I'll admit what I'm doing here is pretty funny (running test-* from\n> an alias). I'm doing it because I want to see how the program operates\n> with the \"-c\" config, and it's nicer to spell it as a user would,\n> instead of munging $GIT_CONFIG_PARAMETERS directly.\n>\n> So I'm not sure if it's worth working around or not. The single tree\n> state produced by this commit is fine, but it does behave badly if\n> there's leftover cruft from a pre-e6e7530 build. A more robust version\n> would look more like:\n>\n> sq=\\' ;# to ease quoting later\n> ...\n> GIT_CONFIG_PARAMETERS=${sq}foo.bar=from-cmdline${sq} test-config ...\n>\n> Which is ugly, but it's probably worth it to avoid the flakiness.\n>\n> The other option is to somehow make bin-wrappers more robust. E.g., it\n> would be nice if we didn't actually point into the repository root\n> directly, but rather somehow linked all of the git-* entries that\n> _would_ be installed into the exec-path into a fake exec-path (or\n> alternatively, actually build them directly into that fake exec-path).\n>\n> That's a much bigger change, though. Given how unlikely the sequence of\n> steps in my test is, maybe it's better to just work around it in this\n> one case.\n>\n> -Peff\n\n\n\n-- \nDuy\n"},{"id":"287589","messageId":"xmqq7fegydmf.fsf@gitster.mtv.corp.google.com","threadId":"42377","inReplyTo":"CACsJy8Ad=yN6aLkH9B6ujUNUvPT-b+jw+CwJORD5Fh1jYeOUZQ@mail.gmail.com","subject":"Re: [PATCH 4/6] config: return configset value for current_config_ functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-26T16:42:48Z","receivedAt":"2016-05-26T16:42:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Thu, May 19, 2016 at 7:08 AM, Jeff King wrote:\n>> On Wed, May 18, 2016 at 06:43:23PM -0400, Jeff King wrote:\n>>\n>>> cache.h | 1 +\n>>> config.c | 51 +++++++++++++++++++++++++++++++++++++++++---------\n>>> t/helper/test-config.c | 20 ++++++++++++++++++++\n>>> t/t1308-config-set.sh | 23 +++++++++++++++++++++++\n>>> [...]\n>>> +test_expect_success 'iteration shows correct origins' '\n>>> + echo \"[alias]test-config = !test-config\" >.gitconfig &&\n>\n> How about using 'which' to get absolute path for test-config and put\n> it here? Then we don't rely on $PATH anymore.\n\nDon't use which, which is not portable.\n\nRemind me why we end up running ./test-config instead of\n./bin-wrappers/test-config? Should our tests be running \nbin-wrappers early in their $PATH, perhaps?\n"},{"id":"287590","messageId":"xmqqzircwyzf.fsf@gitster.mtv.corp.google.com","threadId":"42377","inReplyTo":"20160526054418.GB21580@sigill.intra.peff.net","subject":"Re: [PATCH/RFC 0/6] pack-objects hook for upload-pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-26T16:44:20Z","receivedAt":"2016-05-26T16:44:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yeah, I had the same thought while working on this, but just didn't want\n> to have to tweak every config callback. As you say, I don't think this\n> makes anything fundamentally worse, though. I'm inclined to go with this\n> strategy, especially with the extra die(\"BUG\") safety added here.\n\nFine by me.\n\n> PS Did you have any thoughts on the t/helper problem mentioned in:\n>\n> http://article.gmane.org/gmane.comp.version-control.git/295029\n>\n> I suspect it will bite you if you try merging/testing this.\n\nIt already did but thanks to that message I didn't have to panic ;-).\n"},{"id":"287591","messageId":"20160526165033.GA20355@sigill.intra.peff.net","threadId":"42377","inReplyTo":"xmqq7fegydmf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] config: return configset value for current_config_ functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-26T16:50:33Z","receivedAt":"2016-05-26T16:50:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 26, 2016 at 09:42:48AM -0700, Junio C Hamano wrote:\n\n> Duy Nguyen writes:\n> \n> > On Thu, May 19, 2016 at 7:08 AM, Jeff King wrote:\n> >> On Wed, May 18, 2016 at 06:43:23PM -0400, Jeff King wrote:\n> >>\n> >>> cache.h | 1 +\n> >>> config.c | 51 +++++++++++++++++++++++++++++++++++++++++---------\n> >>> t/helper/test-config.c | 20 ++++++++++++++++++++\n> >>> t/t1308-config-set.sh | 23 +++++++++++++++++++++++\n> >>> [...]\n> >>> +test_expect_success 'iteration shows correct origins' '\n> >>> + echo \"[alias]test-config = !test-config\" >.gitconfig &&\n> >\n> > How about using 'which' to get absolute path for test-config and put\n> > it here? Then we don't rely on $PATH anymore.\n> \n> Don't use which, which is not portable.\n> \n> Remind me why we end up running ./test-config instead of\n> ./bin-wrappers/test-config? Should our tests be running \n> bin-wrappers early in their $PATH, perhaps?\n\nThe problem is running test-config inside of a git alias. The\nbin-wrappers will set the exec-path to the root-level of git's build\ndirectory, which the git binary will then stick at the front of the\n$PATH.\n\nSo the simplest solution really is: don't do that. The only debate in my\nmind is whether this is rare enough that it won't bite somebody again in\nthe future, or if we should look into a solution that makes this Just\nWork.\n\n-Peff\n"},{"id":"287600","messageId":"xmqqd1o8wwk3.fsf@gitster.mtv.corp.google.com","threadId":"42377","inReplyTo":"20160526165033.GA20355@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] config: return configset value for current_config_ functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-26T17:36:44Z","receivedAt":"2016-05-26T17:36:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The problem is running test-config inside of a git alias. The\n> bin-wrappers will set the exec-path to the root-level of git's build\n> directory, which the git binary will then stick at the front of the\n> $PATH.\n\nI was wondering why exec-path does not point at bin-wrappers in the\nfirst place.\n\nA wrapper script needs to know where the real thing lives in order\nto \"exec\" (or \"exec gdb\") anyway, and it hardcodes the path to it.\nIt happens to use GIT_EXEC_PATH to shorten the hardcoded path it\nuses when it does \"exec\", but it does not have to.\n\nWouldn't we want to see \"git\" use any of these wrapped ones when it\ninvokes a non-builtin subcommand when it does so normally, honoring\nGIT_EXEC_PATH? Pointing GIT_EXEC_PATH at the top-level means that\nwrappers are bypassed for such an invocation (if what is run happens\nto have executable at the top-level), and possibly a totally wrong\nthing is run (when we start generating the binaries in different\ndirectories, which is what we are seeing here).\n\n> So the simplest solution really is: don't do that. The only debate\n> in my mind is whether this is rare enough that it won't bite\n> somebody again in the future, or if we should look into a solution\n> that makes this Just Work.\n\nI think it was you who alluded to revamping the test framework along\nthe lines of preparing a \"test installation\" (aka \"make install\"\nwith DESTDIR set to somewhere) and making bin-wrappers to point into\nthat installation (or if we are testing an installed Git that may be\ndifferent from what we have source for, that final installed\nlocation). An installed version of Git will not have test-* helpers\nso they need to come from a freshly built source tree, not from\n\"test installation\" or \"installed Git\". There may be other details\nthat need to be worked out, but as a longer term direction that may\nnot be a bad idea.\n"},{"id":"287625","messageId":"20160527003223.GA1627@sigill.intra.peff.net","threadId":"42377","inReplyTo":"20160526165033.GA20355@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] config: return configset value for current_config_ functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-27T00:32:23Z","receivedAt":"2016-05-27T00:32:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 26, 2016 at 12:50:33PM -0400, Jeff King wrote:\n\n> > Remind me why we end up running ./test-config instead of\n> > ./bin-wrappers/test-config? Should our tests be running \n> > bin-wrappers early in their $PATH, perhaps?\n> \n> The problem is running test-config inside of a git alias. The\n> bin-wrappers will set the exec-path to the root-level of git's build\n> directory, which the git binary will then stick at the front of the\n> $PATH.\n> \n> So the simplest solution really is: don't do that. The only debate in my\n> mind is whether this is rare enough that it won't bite somebody again in\n> the future, or if we should look into a solution that makes this Just\n> Work.\n\nThat being said, it's easy enough to work around this one case, so we\ndon't have to stall the topic thinking about this (and maybe if it never\ncomes up again, we can just not think about it further :) ).\n\nSo here's a replacement patch for patch 4 of jk/upload-pack-hook (the\nothers are untouched, and don't even have conflicts rebasing on top).\n\nI went with just setting $GIT_CONFIG_PARAMETERS. The full-path thing Duy\nsuggested would work, and you can avoid \"which\" by just pointing to\n$TEST_DIRECTORY/helper/test-config. But besides being slightly brittle,\nyou also have to jump through some hoops to handle a $TEST_DIRECTORY\nwith spaces in it. The solution here is pretty straightforward, I think.\n\n-- >8 --\nSubject: config: return configset value for current_config_ functions\n\nWhen 473166b (config: add 'origin_type' to config_source\nstruct, 2016-02-19) added accessor functions for the origin\ntype and name, it taught them only to look at the \"cf\"\nstruct that is filled in while we are parsing the config.\nThis is sufficient to make it work with git-config, which\nuses git_config_with_options() under the hood. That function\nfreshly parses the config files and triggers the callback\nwhen it parses each key.\n\nMost git programs, however, use git_config(). This interface\nwill populate a cache during the actual parse, and then\nserve values from the cache. Calling current_config_filename()\nin a callback here will find a NULL cf and produce an error.\nThere are no such callers right now, but let's prepare for\nadding some by making this work.\n\nWe already record source information in a struct attached to\neach value. We just need to make it globally available and\nthen consult it from the accessor functions.\n\nSigned-off-by: Jeff King \n---\n cache.h | 1 +\n config.c | 51 +++++++++++++++++++++++++++++++++++++++++---------\n t/helper/test-config.c | 20 ++++++++++++++++++++\n t/t1308-config-set.sh | 24 ++++++++++++++++++++++++\n 4 files changed, 87 insertions(+), 9 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 6049f86..1bce212 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1696,6 +1696,7 @@ extern int ignore_untracked_cache_config;\n struct key_value_info {\n \tconst char *filename;\n \tint linenr;\n+\tconst char *origin_type;\n };\n \n extern NORETURN void git_die_config(const char *key, const char *err, ...) __attribute__((format(printf, 2, 3)));\ndiff --git a/config.c b/config.c\nindex 571151f..d555dee 100644\n--- a/config.c\n+++ b/config.c\n@@ -38,7 +38,24 @@ struct config_source {\n \tlong (*do_ftell)(struct config_source *c);\n };\n \n+/*\n+ * These variables record the \"current\" config source, which\n+ * can be accessed by parsing callbacks.\n+ *\n+ * The \"cf\" variable will be non-NULL only when we are actually parsing a real\n+ * config source (file, blob, cmdline, etc).\n+ *\n+ * The \"current_config_kvi\" variable will be non-NULL only when we are feeding\n+ * cached config from a configset into a callback.\n+ *\n+ * They should generally never be non-NULL at the same time. If they are both\n+ * NULL, then we aren't parsing anything (and depending on the function looking\n+ * at the variables, it's either a bug for it to be called in the first place,\n+ * or it's a function which can be reused for non-config purposes, and should\n+ * fall back to some sane behavior).\n+ */\n static struct config_source *cf;\n+static struct key_value_info *current_config_kvi;\n \n static int zlib_compression_seen;\n \n@@ -1284,16 +1301,20 @@ static void configset_iter(struct config_set *cs, config_fn_t fn, void *data)\n \tstruct string_list *values;\n \tstruct config_set_element *entry;\n \tstruct configset_list *list = &cs->list;\n-\tstruct key_value_info *kv_info;\n \n \tfor (i = 0; i < list->nr; i++) {\n \t\tentry = list->items[i].e;\n \t\tvalue_index = list->items[i].value_index;\n \t\tvalues = &entry->value_list;\n-\t\tif (fn(entry->key, values->items[value_index].string, data) < 0) {\n-\t\t\tkv_info = values->items[value_index].util;\n-\t\t\tgit_die_config_linenr(entry->key, kv_info->filename, kv_info->linenr);\n-\t\t}\n+\n+\t\tcurrent_config_kvi = values->items[value_index].util;\n+\n+\t\tif (fn(entry->key, values->items[value_index].string, data) < 0)\n+\t\t\tgit_die_config_linenr(entry->key,\n+\t\t\t\t\t current_config_kvi->filename,\n+\t\t\t\t\t current_config_kvi->linenr);\n+\n+\t\tcurrent_config_kvi = NULL;\n \t}\n }\n \n@@ -1355,10 +1376,12 @@ static int configset_add_value(struct config_set *cs, const char *key, const cha\n \tif (cf->name) {\n \t\tkv_info->filename = strintern(cf->name);\n \t\tkv_info->linenr = cf->linenr;\n+\t\tkv_info->origin_type = strintern(cf->origin_type);\n \t} else {\n \t\t/* for values read from `git_config_from_parameters()` */\n \t\tkv_info->filename = NULL;\n \t\tkv_info->linenr = -1;\n+\t\tkv_info->origin_type = NULL;\n \t}\n \tsi->util = kv_info;\n \n@@ -2438,14 +2461,24 @@ int parse_config_key(const char *var,\n \n const char *current_config_origin_type(void)\n {\n-\tif (!cf)\n+\tconst char *type;\n+\tif (current_config_kvi)\n+\t\ttype = current_config_kvi->origin_type;\n+\telse if(cf)\n+\t\ttype = cf->origin_type;\n+\telse\n \t\tdie(\"BUG: current_config_origin_type called outside config callback\");\n-\treturn cf->origin_type ? cf->origin_type : \"command line\";\n+\treturn type ? type : \"command line\";\n }\n \n const char *current_config_name(void)\n {\n-\tif (!cf)\n+\tconst char *name;\n+\tif (current_config_kvi)\n+\t\tname = current_config_kvi->filename;\n+\telse if (cf)\n+\t\tname = cf->name;\n+\telse\n \t\tdie(\"BUG: current_config_name called outside config callback\");\n-\treturn cf->name ? cf->name : \"\";\n+\treturn name ? name : \"\";\n }\ndiff --git a/t/helper/test-config.c b/t/helper/test-config.c\nindex 6a77552..3605ef8 100644\n--- a/t/helper/test-config.c\n+++ b/t/helper/test-config.c\n@@ -25,6 +25,9 @@\n * \t\t\t\tascending order of priority from a config_set\n * \t\t\t\tconstructed from files entered as arguments.\n *\n+ * iterate -> iterate over all values using git_config(), and print some\n+ * data for each\n+ *\n * Examples:\n *\n * To print the value with highest priority for key \"foo.bAr Baz.rock\":\n@@ -32,6 +35,20 @@\n *\n */\n \n+static int iterate_cb(const char *var, const char *value, void *data)\n+{\n+\tstatic int nr;\n+\n+\tif (nr++)\n+\t\tputchar('\\n');\n+\n+\tprintf(\"key=%s\\n\", var);\n+\tprintf(\"value=%s\\n\", value ? value : \"(null)\");\n+\tprintf(\"origin=%s\\n\", current_config_origin_type());\n+\tprintf(\"name=%s\\n\", current_config_name());\n+\n+\treturn 0;\n+}\n \n int main(int argc, char **argv)\n {\n@@ -134,6 +151,9 @@ int main(int argc, char **argv)\n \t\t\tprintf(\"Value not found for \\\"%s\\\"\\n\", argv[2]);\n \t\t\tgoto exit1;\n \t\t}\n+\t} else if (!strcmp(argv[1], \"iterate\")) {\n+\t\tgit_config(iterate_cb, NULL);\n+\t\tgoto exit0;\n \t}\n \n \tdie(\"%s: Please check the syntax and the function name\", argv[0]);\ndiff --git a/t/t1308-config-set.sh b/t/t1308-config-set.sh\nindex 005d66d..d345a88 100755\n--- a/t/t1308-config-set.sh\n+++ b/t/t1308-config-set.sh\n@@ -229,4 +229,28 @@ test_expect_success 'error on modifying repo config without repo' '\n \t)\n '\n \n+cmdline_config=\"'foo.bar=from-cmdline'\"\n+test_expect_success 'iteration shows correct origins' '\n+\techo \"[foo]bar = from-repo\" >.git/config &&\n+\techo \"[foo]bar = from-home\" >.gitconfig &&\n+\tcat >expect <<-EOF &&\n+\tkey=foo.bar\n+\tvalue=from-home\n+\torigin=file\n+\tname=$(pwd)/.gitconfig\n+\n+\tkey=foo.bar\n+\tvalue=from-repo\n+\torigin=file\n+\tname=.git/config\n+\n+\tkey=foo.bar\n+\tvalue=from-cmdline\n+\torigin=command line\n+\tname=\n+\tEOF\n+\tGIT_CONFIG_PARAMETERS=$cmdline_config test-config iterate >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.9.0.rc0.307.gc679867\n"},{"id":"287627","messageId":"20160527004146.GA4996@sigill.intra.peff.net","threadId":"42377","inReplyTo":"xmqqd1o8wwk3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 4/6] config: return configset value for current_config_ functions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-27T00:41:46Z","receivedAt":"2016-05-27T00:41:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 26, 2016 at 10:36:44AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The problem is running test-config inside of a git alias. The\n> > bin-wrappers will set the exec-path to the root-level of git's build\n> > directory, which the git binary will then stick at the front of the\n> > $PATH.\n> \n> I was wondering why exec-path does not point at bin-wrappers in the\n> first place.\n> \n> A wrapper script needs to know where the real thing lives in order\n> to \"exec\" (or \"exec gdb\") anyway, and it hardcodes the path to it.\n> It happens to use GIT_EXEC_PATH to shorten the hardcoded path it\n> uses when it does \"exec\", but it does not have to.\n> \n> Wouldn't we want to see \"git\" use any of these wrapped ones when it\n> invokes a non-builtin subcommand when it does so normally, honoring\n> GIT_EXEC_PATH? Pointing GIT_EXEC_PATH at the top-level means that\n> wrappers are bypassed for such an invocation (if what is run happens\n> to have executable at the top-level), and possibly a totally wrong\n> thing is run (when we start generating the binaries in different\n> directories, which is what we are seeing here).\n\nI think the issue is that bin-wrappers serves two purposes. One is for\ntesting, but the other is for people who run git directly without\ninstalling. For us to set GIT_EXEC_PATH to bin-wrappers, it would have\nto have all of the git-* external programs, which would then put them\nall in the $PATH of people doing the no-install thing.\n\nThat's certainly not insurmountable. Either we can tell them to live\nwith it, or we can break out a separate wrapper directory that serves as\na pseudo-exec-path.\n\n> > So the simplest solution really is: don't do that. The only debate\n> > in my mind is whether this is rare enough that it won't bite\n> > somebody again in the future, or if we should look into a solution\n> > that makes this Just Work.\n> \n> I think it was you who alluded to revamping the test framework along\n> the lines of preparing a \"test installation\" (aka \"make install\"\n> with DESTDIR set to somewhere) and making bin-wrappers to point into\n> that installation (or if we are testing an installed Git that may be\n> different from what we have source for, that final installed\n> location). An installed version of Git will not have test-* helpers\n> so they need to come from a freshly built source tree, not from\n> \"test installation\" or \"installed Git\". There may be other details\n> that need to be worked out, but as a longer term direction that may\n> not be a bad idea.\n\nI think you can make it even simpler by not really doing a \"make\ninstall\", but just linking or bin-wrappering a fake exec-path. It would\nbe great if we could truly just \"make install\" into a fake area and test\nthat (dropping bin-wrappers entirely), but git cares too much about some\nhard-coded paths, I think. We'd have to first have a truly relocatable\nbinary.\n\n-Peff\n"},{"id":"287628","messageId":"xmqqa8jctfl5.fsf@gitster.mtv.corp.google.com","threadId":"42377","inReplyTo":"20160527004146.GA4996@sigill.intra.peff.net","subject":"Re: [PATCH 4/6] config: return configset value for current_config_ functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-27T02:11:34Z","receivedAt":"2016-05-27T02:11:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think the issue is that bin-wrappers serves two purposes. One is for\n> testing, but the other is for people who run git directly without\n> installing.\n\nHmph. It may have been a useful way to \"run without installing\"\nonce in the past, but with the \"check and run it under GDB\" etc.,\nI am not sure if it still is. I certainly did not think about that\nas a valid use case when I wrote the message you are responding to.\n\n> I think you can make it even simpler by not really doing a \"make\n> install\", but just linking or bin-wrappering a fake exec-path. It would\n> be great if we could truly just \"make install\" into a fake area and test\n> that (dropping bin-wrappers entirely), but git cares too much about some\n> hard-coded paths, I think. We'd have to first have a truly relocatable\n> binary.\n\nLooking at what bin-wrappers do, I do not think \"hard coded paths\"\nis something we need them for. If we wanted to make the \"test what\nwe just built\" and the \"test what is already installed\" closely\nmimic each other, I have a feeling that setting of the environment\nvariable done by the bin-wrappers can and should be moved to the\ntest framework. When testing what we just built, set these\nenvironment variables you see in any of the bin-wrappers/ script to\npoint at various places in the \"make DESTDIR=there install\" tree,\nand when testing what is installed, set them to different values\n(possibly \"nothing\", e.g. GIT_EXEC_PATH would not be needed if we\nare testing an installed and already working version).\n\nSo I suspect \"truly relocatable binary\" is necessary for testing.\n"}]}