{"thread":{"id":"37631","subject":"project wide: git config entry for [diff] renames=true","startedAt":"2014-09-25T15:48:31Z","lastAt":"2014-10-06T17:58:36Z","messageCount":14,"participants":["Joe Perches","Jeff King","Junio C Hamano","Rasmus Villemoes"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"249847","messageId":"1411660111.4026.24.camel@joe-AO725","threadId":"37631","inReplyTo":"20140925150353.GA15325@kroah.com","subject":"project wide: git config entry for [diff] renames=true","fromName":"Joe Perches","fromEmail":"joe@perches.com","sentAt":"2014-09-25T15:48:31Z","receivedAt":"2014-09-25T15:48:31Z","isPatch":false,"sender":{"key":"joe@perches.com","avatar":"https://avatars.githubusercontent.com/u/13122723?v=4"},"body":"On Thu, 2014-09-25 at 17:03 +0200, Greg Kroah-Hartman wrote:\n\n> In the future, please generate a git \"move\" diff, which makes it easier\n> to review, and prove that nothing really changed.  It also helps if the\n> file is a bit different from what you diffed against, which in my case,\n> was true.\n\nMaybe it'd be possible to add \n\n[diff]\n\trenames = true\n\nto the .git/config file.\n\nbut I don't find a mechanism to add anything to the\n.git/config and have it be pulled.\n"},{"id":"249855","messageId":"20140925180005.GA11755@peff.net","threadId":"37631","inReplyTo":"1411660111.4026.24.camel@joe-AO725","subject":"Re: project wide: git config entry for [diff] renames=true","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-09-25T18:00:05Z","receivedAt":"2014-09-25T18:00:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 25, 2014 at 08:48:31AM -0700, Joe Perches wrote:\n\n> On Thu, 2014-09-25 at 17:03 +0200, Greg Kroah-Hartman wrote:\n> \n> > In the future, please generate a git \"move\" diff, which makes it easier\n> > to review, and prove that nothing really changed.  It also helps if the\n> > file is a bit different from what you diffed against, which in my case,\n> > was true.\n> \n> Maybe it'd be possible to add \n> \n> [diff]\n> \trenames = true\n> \n> to the .git/config file.\n> \n> but I don't find a mechanism to add anything to the\n> .git/config and have it be pulled.\n\nThere is no such mechanism within git. We've resisted adding one because\nof the danger of something like:\n\n  [diff]\n    external = rm -rf /\n\ndiff.renames is probably safe, but any config-sharing mechanism would\nhave to deal with either whitelisting, or providing some mechanism for\nthe puller to review changes before blindly following them.\n\n-Peff\n"},{"id":"249858","messageId":"1411668391.3460.2.camel@joe-AO725","threadId":"37631","inReplyTo":"20140925180005.GA11755@peff.net","subject":"Re: project wide: git config entry for [diff] renames=true","fromName":"Joe Perches","fromEmail":"joe@perches.com","sentAt":"2014-09-25T18:06:31Z","receivedAt":"2014-09-25T18:06:31Z","isPatch":false,"sender":{"key":"joe@perches.com","avatar":"https://avatars.githubusercontent.com/u/13122723?v=4"},"body":"On Thu, 2014-09-25 at 14:00 -0400, Jeff King wrote:\n> On Thu, Sep 25, 2014 at 08:48:31AM -0700, Joe Perches wrote:\n> \n> > On Thu, 2014-09-25 at 17:03 +0200, Greg Kroah-Hartman wrote:\n> > \n> > > In the future, please generate a git \"move\" diff, which makes it easier\n> > > to review, and prove that nothing really changed.  It also helps if the\n> > > file is a bit different from what you diffed against, which in my case,\n> > > was true.\n> > \n> > Maybe it'd be possible to add \n> > \n> > [diff]\n> > \trenames = true\n> > \n> > to the .git/config file.\n> > \n> > but I don't find a mechanism to add anything to the\n> > .git/config and have it be pulled.\n> \n> There is no such mechanism within git. We've resisted adding one because\n> of the danger of something like:\n> \n>   [diff]\n>     external = rm -rf /\n> \n> diff.renames is probably safe, but any config-sharing mechanism would\n> have to deal with either whitelisting, or providing some mechanism for\n> the puller to review changes before blindly following them.\n\nAnother mechanism might be to add a repository\ntop level .gitconfig and add whatever to that.\n"},{"id":"249860","messageId":"xmqq61gbbkxc.fsf@gitster.dls.corp.google.com","threadId":"37631","inReplyTo":"1411668391.3460.2.camel@joe-AO725","subject":"Re: project wide: git config entry for [diff] renames=true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-25T18:43:27Z","receivedAt":"2014-09-25T18:43:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Joe Perches <joe@perches.com> writes:\n\n> On Thu, 2014-09-25 at 14:00 -0400, Jeff King wrote:\n> ...\n>> diff.renames is probably safe, but any config-sharing mechanism would\n>> have to deal with either whitelisting, or providing some mechanism for\n>> the puller to review changes before blindly following them.\n>\n> Another mechanism might be to add a repository\n> top level .gitconfig and add whatever to that.\n\nThat could be smaller half of an implementation detail of one of the\ntwo possibilities Jeff mentioned i.e. \"mechanism for the puller to\nreview changes before blindly following\".  It gives the transfer\npart.  You still need a new mechanism to make that file that is\ntracked in the repository to be used as part of your configuration\nvariable set after letting the puller to review and approve.\n\nA puller who blindly trust the project could use the \"include\"\nmechanism from your .git/config to include a file with a well-known\nname that is tracked by the project _without_ review or approval.  I\ndoubt we would recommend that in an open source setting, though.\n"},{"id":"249861","messageId":"xmqqy4t7a5vx.fsf@gitster.dls.corp.google.com","threadId":"37631","inReplyTo":"20140925180005.GA11755-AdEPDUrAXsQ@public.gmane.org","subject":"Re: project wide: git config entry for [diff] renames=true","fromName":"Junio C Hamano","fromEmail":"gitster-e+axbwqsrlaavxtiumwx3w@public.gmane.org","sentAt":"2014-09-25T18:53:38Z","receivedAt":"2014-09-25T18:53:38Z","isPatch":false,"sender":{"key":"gitster-e+axbwqsrlaavxtiumwx3w@public.gmane.org","avatar":null},"body":"Jeff King <peff-AdEPDUrAXsQ@public.gmane.org> writes:\n\n> There is no such mechanism within git. We've resisted adding one because\n> of the danger of something like:\n>\n>   [diff]\n>     external = rm -rf /\n>\n> diff.renames is probably safe, but any config-sharing mechanism would\n> have to deal with either whitelisting, or providing some mechanism for\n> the puller to review changes before blindly following them.\n\nIt might be useful to add a \"safe include\" feature, perhaps?  We\nship a small set of hardcoded default whitelist (diff.renames may be\nincluded in there), and allow the user who do not want to be\naffected to override it with\n\n    [include]\n        safe = !diff.renames\n\nor even\n\n    [config]\n    \tsafe = !*\n\nat the same time allow them to add what we do not hardcode to it\nusing the same mechanism, e.g.\n\n    [config]\n    \tsafe = merge.*\n\nThen\n\n    [include]\n\tsafe\n    \tpath = ../project.gitconfig\n\n    [include]\n    \tpath = $HOME/.gitconfig-variant1\n\nwould only allow the variables include.safe deems safe to affect\nus from the in-tree file, and use everything from my personal set in\nmy home directory.\n\n\n\n    \t\n--\nTo unsubscribe from this list: send the line \"unsubscribe linux-usb\" in\nthe body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org\nMore majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"249862","messageId":"xmqqtx3va5sr.fsf@gitster.dls.corp.google.com","threadId":"37631","inReplyTo":"xmqqy4t7a5vx.fsf@gitster.dls.corp.google.com","subject":"Re: project wide: git config entry for [diff] renames=true","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-09-25T18:55:32Z","receivedAt":"2014-09-25T18:55:32Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>\nwrites:\n\n> or even\n>\n>     [config]\n>     \tsafe = !*\n> ...\n\nGaah, I meant [include] in all places I spelled [config] in the\nprevious message.\n"},{"id":"250188","messageId":"1412300254-11281-1-git-send-email-rv@rasmusvillemoes.dk","threadId":"37631","inReplyTo":"xmqqy4t7a5vx.fsf@gitster.dls.corp.google.com","subject":"[RFC/PATCH 0/2] Introduce safe-include config feature","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2014-10-03T01:37:32Z","receivedAt":"2014-10-03T01:37:32Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"[trimming Ccs]\n\nThis is an attempt at implementing the suggested safe-include config\nfeature. It mostly has the semantics Junio suggested in the parent\npost, but it does not directly extend the current include directive;\ninstead, it uses a separate safe-include directive. This is done so\nthat if a repository is used with both old and new versions of git,\nthe older versions will just silently ignore the safe-include, instead\nof ignoring include.safe and then proceeding to processing \"path =\n../project.gitconfig\".\n\nConfig variables are whitelisted using safe-include.whitelist; the\nvalue is interpreted as a whitespace-separated list of, possibly\nnegated, patterns. Later patterns override earlier ones.\n\nIf the feature is deemed worthwhile and my approach is acceptable,\nI'll go ahead and try to write some documentation. For now, there is\njust a small test script.\n\n\nRasmus Villemoes (2):\n  config: Add safe-include directive\n  config: Add test of safe-include feature\n\n config.c                       | 91 +++++++++++++++++++++++++++++++++++++--\n t/t1309-config-safe-include.sh | 96 ++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 184 insertions(+), 3 deletions(-)\n create mode 100755 t/t1309-config-safe-include.sh\n\n-- \n2.0.4\n"},{"id":"250190","messageId":"1412300254-11281-2-git-send-email-rv@rasmusvillemoes.dk","threadId":"37631","inReplyTo":"1412300254-11281-1-git-send-email-rv@rasmusvillemoes.dk","subject":"[RFC/PATCH 1/2] config: Add safe-include directive","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2014-10-03T01:37:33Z","receivedAt":"2014-10-03T01:37:33Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"This adds a variant of the include directive, where only certain\nconfig variables in the included files are honoured. The set of\nhonoured variables consists of those the user has mentioned in a\nsafe-include.whitelist directive, along with a small set of git.git\nblessed ones.\n\nThis can, for example, be used by a project to supply a set of\nsuggested configuration variables, such as \"diff.renames = true\". The\nproject would provide these in e.g project.gitconfig, and the user then\nhas to explicitly opt-in by putting\n\n[safe-include]\n    path = ../project.gitconfig\n\ninto .git/config, possibly preceding the path directive with a\nwhitelist directive.\n\nThe problem with simply using the ordinary include directive for this\npurpose is that certain configuration variables (e.g. diff.external)\ncan allow arbitrary programs to be run.\n\nOlder versions of git do not understand the safe-include directives,\nso they will effectively just ignore them.\n\nObviously, we must ignore safe-include.whitelist directives when we\nare processing a safe-included file.\n\nSigned-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>\n---\n config.c | 91 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 file changed, 88 insertions(+), 3 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex a677eb6..764cda1 100644\n--- a/config.c\n+++ b/config.c\n@@ -11,6 +11,7 @@\n #include \"quote.h\"\n #include \"hashmap.h\"\n #include \"string-list.h\"\n+#include \"wildmatch.h\"\n \n struct config_source {\n \tstruct config_source *prev;\n@@ -39,6 +40,79 @@ static struct config_source *cf;\n \n static int zlib_compression_seen;\n \n+struct safe_var {\n+\tstruct safe_var *next;\n+\tconst char *pattern;\n+\tint blacklisted;\n+};\n+\n+static int safe_include_depth;\n+static struct safe_var *safe_var_head;\n+\n+static const char *builtin_safe_patterns[] = {\n+\t\"diff.renames\",\n+};\n+\n+static int config_name_is_safe(const char *var)\n+{\n+\tstruct safe_var *sv;\n+\tunsigned i;\n+\n+\tfor (sv = safe_var_head; sv; sv = sv->next) {\n+\t\t/* Handle malformed patterns? */\n+\t\tif (wildmatch(sv->pattern, var, WM_CASEFOLD, NULL) == WM_MATCH)\n+\t\t\treturn !sv->blacklisted;\n+\t}\n+\tfor (i = 0; i < ARRAY_SIZE(builtin_safe_patterns); ++i) {\n+\t\tif (wildmatch(builtin_safe_patterns[i], var, WM_CASEFOLD, NULL) == WM_MATCH)\n+\t\t\treturn 1;\n+\t}\n+\n+\treturn 0;\n+}\n+\n+static void config_add_safe_pattern(const char *p)\n+{\n+\tstruct safe_var *sv;\n+\tint blacklist = 0;\n+\n+\tif (*p == '!') {\n+\t\tblacklist = 1;\n+\t\t++p;\n+\t}\n+\tif (!*p)\n+\t\treturn;\n+\tsv = xmalloc(sizeof(*sv));\n+\tsv->pattern = xstrdup(p);\n+\tsv->blacklisted = blacklist;\n+\tsv->next = safe_var_head;\n+\tsafe_var_head = sv;\n+}\n+\n+static void config_add_safe_names(const char *value)\n+{\n+\tchar *patterns = xstrdup(value);\n+\tchar *p, *save;\n+\n+\t/*\n+\t * This allows giving multiple patterns in a single line, e.g.\n+\t *\n+\t *     whitelist = !* foo.bar squirrel.*\n+\t *\n+\t * to override the builtin list of safe vars and only declare\n+\t * foo.bar and the squirrel section safe. But it has the\n+\t * obvious drawback that one cannot match subsection names\n+\t * containing whitespace. The alternative is that the above\n+\t * would have to be written on three separate whitelist lines.\n+\t */\n+\tfor (p = strtok_r(patterns, \" \\t\", &save); p; p = strtok_r(NULL, \" \\t\", &save)) {\n+\t\tconfig_add_safe_pattern(p);\n+\t}\n+\n+\tfree(patterns);\n+}\n+\n+\n /*\n  * Default config_set that contains key-value pairs from the usual set of config\n  * config files (i.e repo specific .git/config, user wide ~/.gitconfig, XDG\n@@ -142,12 +216,23 @@ int git_config_include(const char *var, const char *value, void *data)\n \t * Pass along all values, including \"include\" directives; this makes it\n \t * possible to query information on the includes themselves.\n \t */\n-\tret = inc->fn(var, value, inc->data);\n-\tif (ret < 0)\n-\t\treturn ret;\n+\tif (safe_include_depth == 0 || config_name_is_safe(var)) {\n+\t\tret = inc->fn(var, value, inc->data);\n+\t\tif (ret < 0)\n+\t\t\treturn ret;\n+\t}\n \n \tif (!strcmp(var, \"include.path\"))\n \t\tret = handle_path_include(value, inc);\n+\telse if (safe_include_depth == 0\n+\t\t && !strcmp(var, \"safe-include.whitelist\")) {\n+\t\tconfig_add_safe_names(value);\n+\t}\n+\telse if (!strcmp(var, \"safe-include.path\")) {\n+\t\tsafe_include_depth++;\n+\t\tret = handle_path_include(value, inc);\n+\t\tsafe_include_depth--;\n+\t}\n \treturn ret;\n }\n \n-- \n2.0.4\n"},{"id":"250189","messageId":"1412300254-11281-3-git-send-email-rv@rasmusvillemoes.dk","threadId":"37631","inReplyTo":"1412300254-11281-1-git-send-email-rv@rasmusvillemoes.dk","subject":"[RFC/PATCH 2/2] config: Add test of safe-include feature","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2014-10-03T01:37:34Z","receivedAt":"2014-10-03T01:37:34Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"This adds a script for testing various aspects of the safe-include feature.\n\nSigned-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>\n---\n t/t1309-config-safe-include.sh | 96 ++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 96 insertions(+)\n create mode 100755 t/t1309-config-safe-include.sh\n\ndiff --git a/t/t1309-config-safe-include.sh b/t/t1309-config-safe-include.sh\nnew file mode 100755\nindex 0000000..b8ccc94\n--- /dev/null\n+++ b/t/t1309-config-safe-include.sh\n@@ -0,0 +1,96 @@\n+#!/bin/sh\n+\n+test_description='test config file safe-include directives'\n+. ./test-lib.sh\n+\n+\n+test_expect_success 'blacklist by default' '\n+\techo \"[diff]external = badprog\" >project &&\n+\techo \"[safe-include]path = project\" >.gitconfig &&\n+\ttest_must_fail git config diff.external\n+'\n+\n+\n+test_expect_success 'builtin safe rules' '\n+\techo \"[diff]renames = true\" >project &&\n+\techo \"[safe-include]path = project\" >.gitconfig &&\n+\techo true >expect &&\n+\tgit config diff.renames >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'user blacklist taking precedence' '\n+\techo \"[diff]renames = true\" >project &&\n+\tcat >.gitconfig <<-\\EOF &&\n+\t[diff]renames = false\n+\t[safe-include]whitelist = !diff.renames\n+\t[safe-include]path = project\n+\tEOF\n+\techo false >expect &&\n+\tgit config diff.renames >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'wildcard matching' '\n+\tcat >project <<-\\EOF &&\n+\t[test]beer = true\n+\t[test]bar = true\n+\t[test]foo = true\n+\tEOF\n+\tcat >.gitconfig <<-\\EOF &&\n+\t[safe-include]whitelist = test.b*r\n+\t[safe-include]path = project\n+\tEOF\n+\tprintf \"test.bar true\\ntest.beer true\\n\" | sort >expect &&\n+\tgit config --get-regexp \"^test\" | sort >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'ignore whitelist directives in safe-included files' '\n+\tcat >project <<-\\EOF &&\n+\t[safe-include]whitelist = *\n+\t[diff]external = badprog\n+\tEOF\n+\techo \"[safe-include]path = project\" >.gitconfig &&\n+\ttest_must_fail git config diff.external\n+'\n+\n+test_expect_success 'multiple whitelist/blacklist patterns in one line' '\n+\tcat >.gitconfig <<-\\EOF &&\n+\t[safe-include]whitelist = !* foo.bar squirrel.* !squirrel.xyz\n+\t[safe-include]path = project\n+\tEOF\n+\tcat >project <<-\\EOF &&\n+\t[diff]renames = true\n+\t[foo]bar = bar\n+\t[squirrel]abc = abc\n+\t[squirrel]xyz = xyz\n+\tEOF\n+\ttest_must_fail git config diff.renames &&\n+\ttest_must_fail git config squirrel.xyz &&\n+\techo bar >expect &&\n+\tgit config foo.bar >actual &&\n+\ttest_cmp expect actual\n+\techo abc >expect &&\n+\tgit config squirrel.abc >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'case insensitivity' '\n+\tcat >.gitconfig <<-\\EOF &&\n+\t[safe-include]whitelist = Test.Abc test.xyz\n+\t[safe-include]path = project\n+\tEOF\n+\tcat >project <<-\\EOF &&\n+\t[test]abc = abc\n+\t[TeST]XyZ = XyZ\n+\tEOF\n+\techo abc >expect &&\n+\tgit config test.abc >actual &&\n+\ttest_cmp expect actual &&\n+\techo XyZ >expect &&\n+\tgit config test.xyz >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n2.0.4\n"},{"id":"250192","messageId":"CAPc5daV_txE9NrwvH5VWhXK+UmE7Avy8R2QaZaX0SsTC_+TU-A@mail.gmail.com","threadId":"37631","inReplyTo":"1412300254-11281-2-git-send-email-rv@rasmusvillemoes.dk","subject":"Re: [RFC/PATCH 1/2] config: Add safe-include directive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-03T05:27:11Z","receivedAt":"2014-10-03T05:27:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, Oct 2, 2014 at 6:37 PM, Rasmus Villemoes <rv@rasmusvillemoes.dk> wrote:\n> This adds a variant of the include directive, where only certain\n> config variables in the included files are honoured. The set of\n> honoured variables consists of those the user has mentioned in a\n> safe-include.whitelist directive, along with a small set of git.git\n> blessed ones.\n>\n> This can, for example, be used by a project to supply a set of\n> suggested configuration variables, such as \"diff.renames = true\". The\n> project would provide these in e.g project.gitconfig, and the user then\n> has to explicitly opt-in by putting\n>\n> [safe-include]\n>     path = ../project.gitconfig\n>\n> into .git/config, possibly preceding the path directive with a\n> whitelist directive.\n\nGood thinking to protect against accidental inclusion by older versions of Git.\n\nEven though I did allude to ../project.gitconfig in the original message, I\nthink there should probably be an explicit syntax to name a path that is\nrelative to the root level of the working tree. People do funky things using\n$GIT_DIR and $GIT_WORK_TREE to break the \".. relative to the config\nfile is the root level of the working tree\" assumption, and also a repository\ncan have a regular file \".git\" that points at the real location of the directory\nthat has \"config\" in it, in which case its parent directory is very unlikely to\nbe the root level of the working tree.\n\nThat syntax _could_ be just a relative path (e.g. project.gitconfig names\nthe file with that name at the top-level of the working tree), and if we are\nto do so, we should forbid any relative path that escapes from the working\ntree (e.g. ../project.gitconfig is forbidden, but down/down/../../.gitconfig\ncould be OK as it is the same as .gitconfig). For that matter, anything with\n/./ and /../ in it can safely be forbidden without losing functionality.\n\nThe reason why I think it is sufficient to take a relative path as relative\nto the working tree is primarily because I do not see a reason why we\nwould want to do a safer inclusion of anything inside $GIT_DIR (which\nwould be the natural interpretation if the relatigve path is taken as relative\nto the including config file, in the same way as how the regular include\nis processed). But I could be missing some other useful use cases.\n\nAnd we can allow absolute path, e.g. /etc/gitconfig, of course, but I'd\nprefer to at least initially forbid an absolute path, due to the same worries\nI have against the \"unset some variables defined in /etc/gitconfig\" topic\nwe discussed earlier today in a separate thread.\n"},{"id":"250193","messageId":"CAPc5daVYwLMiZzk5Dmm4v4etUhxjw3-ZjWvKEmc=RJ_mh9LFDA@mail.gmail.com","threadId":"37631","inReplyTo":"CAPc5daV_txE9NrwvH5VWhXK+UmE7Avy8R2QaZaX0SsTC_+TU-A@mail.gmail.com","subject":"Re: [RFC/PATCH 1/2] config: Add safe-include directive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-03T05:34:28Z","receivedAt":"2014-10-03T05:34:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Thu, Oct 2, 2014 at 10:27 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> On Thu, Oct 2, 2014 at 6:37 PM, Rasmus Villemoes <rv@rasmusvillemoes.dk> wrote:\n>> This adds a variant of the include directive, where only certain\n>> config variables in the included files are honoured. The set of\n>> honoured variables consists of those the user has mentioned in a\n>> safe-include.whitelist directive, along with a small set of git.git\n>> blessed ones.\n\nAnother design decision we would need to make is if it should be\nallowed for a safe-included file to use safe-include directive to\ninclude other files. Offhand I do not think of a reason we absolutely\nneed to support it, but there may be an interesting workflow enabled\nif we did so. I dunno.\n"},{"id":"250205","messageId":"xmqqsij5kmte.fsf@gitster.dls.corp.google.com","threadId":"37631","inReplyTo":"CAPc5daV_txE9NrwvH5VWhXK+UmE7Avy8R2QaZaX0SsTC_+TU-A@mail.gmail.com","subject":"Re: [RFC/PATCH 1/2] config: Add safe-include directive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-03T18:52:45Z","receivedAt":"2014-10-03T18:52:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Even though I did allude to ../project.gitconfig in the original message, I\n> think there should probably be an explicit syntax to name a path that is\n> relative to the root level of the working tree. People do funky things using\n> $GIT_DIR and $GIT_WORK_TREE to break the \".. relative to the config\n> file is the root level of the working tree\" assumption, and also a repository\n> can have a regular file \".git\" that points at the real location of the directory\n> that has \"config\" in it, in which case its parent directory is very unlikely to\n> be the root level of the working tree.\n\nThere is another reason why I suspect that it may make the resulting\nsystem more useful if we had a way to explicitly mark the path you\nused to safeInclude (by the way, we do not do dashes in names for\nconfiguration by convention) as referring to something inside the\nproject's working tree.  In a bare repository, we might want to grab\nthe blob at that path in HEAD (i.e. the project's primary branch)\nand include its contents.\n"},{"id":"250266","messageId":"878uktwnqs.fsf@rasmusvillemoes.dk","threadId":"37631","inReplyTo":"CAPc5daV_txE9NrwvH5VWhXK+UmE7Avy8R2QaZaX0SsTC_+TU-A@mail.gmail.com","subject":"Re: [RFC/PATCH 1/2] config: Add safe-include directive","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2014-10-06T09:28:43Z","receivedAt":"2014-10-06T09:28:43Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n\n> (by the way, we do not do dashes in names for configuration by\n> convention)\n\nOK. Actually, I now think I'd prefer a subsection [include \"safe\"], but\nI don't have any strong preferences regarding the names.\n\n> That syntax _could_ be just a relative path (e.g. project.gitconfig names\n> the file with that name at the top-level of the working tree), and if we are\n> to do so, we should forbid any relative path that escapes from the working\n> tree (e.g. ../project.gitconfig is forbidden, but down/down/../../.gitconfig\n> could be OK as it is the same as .gitconfig). For that matter, anything with\n> /./ and /../ in it can safely be forbidden without losing functionality.\n\nI agree that it would be most useful to interpret relative paths as\nbeing relative to the working tree. I'm not sure what would be gained by\nchecking for ./ and ../ components, a symlink could easily be used to\ncircumvent that.\n\n> And we can allow absolute path, e.g. /etc/gitconfig, of course, but I'd\n> prefer to at least initially forbid an absolute path, due to the same worries\n> I have against the \"unset some variables defined in /etc/gitconfig\" topic\n> we discussed earlier today in a separate thread.\n\nOne might (ab)use the feature to only use some settings from a global\nfile, e.g.\n\n[include \"safe\"]\n    whitelist = !foo.*\n    path = ~/extra.gitconfig\n\nBut I'm fine with forbidding absolute paths until someone actually comes\nwith such a use case.\n\n> Another design decision we would need to make is if it should be\n> allowed for a safe-included file to use safe-include directive to\n> include other files. Offhand I do not think of a reason we absolutely\n> need to support it, but there may be an interesting workflow enabled\n> if we did so. I dunno.\n\nAfter one level of safe-include, any safe-include can also be done as a\nnormal include (but one may need to spell the path differently if the\ntwo included files are not both at the top of the working tree). One\ncould imagine a project supplying lots of defaults and splitting those\ninto separate files, each included from a single project.gitconfig.\n\nAnyway, my proposal allows nesting includes and safe-includes inside\nsafe-includes; forbidding it would just be a matter of adding a\nsafe_include_depth == 0 check in two places. (Then safe_include_depth\nprobably could/should be renamed in_safe_include.) I think I have a\nslight preference to allowing nested includes, but if absolute paths are\nforbidden for safe-includes, they should also be forbidden for\ninclude-inside-safe-include.\n\nRasmus\n"},{"id":"250268","messageId":"xmqq7g0djd0z.fsf@gitster.dls.corp.google.com","threadId":"37631","inReplyTo":"878uktwnqs.fsf@rasmusvillemoes.dk","subject":"Re: [RFC/PATCH 1/2] config: Add safe-include directive","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-10-06T17:58:36Z","receivedAt":"2014-10-06T17:58:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Rasmus Villemoes <rv@rasmusvillemoes.dk> writes:\n\n> Junio C Hamano <gitster@pobox.com> wrote:\n>\n>> (by the way, we do not do dashes in names for configuration by\n>> convention)\n>\n> OK. Actually, I now think I'd prefer a subsection [include \"safe\"], but\n> I don't have any strong preferences regarding the names.\n\nI think Peff mentioned something about having the second level\nbetween include and path, so I'll defer it to him.\n\n>> That syntax _could_ be just a relative path (e.g. project.gitconfig names\n>> the file with that name at the top-level of the working tree), and if we are\n>> to do so, we should forbid any relative path that escapes from the working\n>> tree (e.g. ../project.gitconfig is forbidden, but down/down/../../.gitconfig\n>> could be OK as it is the same as .gitconfig). For that matter, anything with\n>> /./ and /../ in it can safely be forbidden without losing functionality.\n>\n> I agree that it would be most useful to interpret relative paths as\n> being relative to the working tree. I'm not sure what would be gained by\n> checking for ./ and ../ components, a symlink could easily be used to\n> circumvent that.\n\nIf the \"limit to the the working tree\" is the reason to suggest a\nrelative path to be taken as relative to the working tree, which my\nsuggestion clearly was, the reader should be intelligent enough to\ninfer that an implementation working in that mode should make sure\nsymlinks and any other means do not step outside it.\n\nAnd as you noticed that, you apparently are ;-)\n\n> One might (ab)use the feature to only use some settings from a global\n> file, e.g.\n>\n> [include \"safe\"]\n>     whitelist = !foo.*\n>     path = ~/extra.gitconfig\n\nYou do not have to write something you do not want to use in your\nown ~/extra.gitconfig that is under your $HOME/, so I'd prefer to\nexplicitly forbidding such a use case at least in the beginning.\n"}]}