{"thread":{"id":"48565","subject":"BUG: No way to set fsck. when cloning","startedAt":"2018-05-24T15:25:35Z","lastAt":"2018-07-30T22:13:45Z","messageCount":69,"participants":["Ævar Arnfjörð Bjarmason","Kevin Daudt","Jeff King","Eric Sunshine","Thomas Braun","Junio C Hamano","Christian Couder","Duy Nguyen","SZEDER Gábor"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"348435","messageId":"87vabd6pp2.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":null,"subject":"BUG: No way to set fsck. when cloning","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-24T15:25:29Z","receivedAt":"2018-05-24T15:25:35Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When I do:\n\n git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git\n\nI get:\n\n error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes\n fatal: Error in object\n fatal: index-pack failed\n\nThe docs (https://git-scm.com/docs/git-config#git-config-fsckltmsg-idgt)\nsay you can override this with -c fsck.zeroPaddedFilemode = ignore, but\nI see in builtin/fsck.c that just fsck_config() knows about this, and\nindeed this works *after* you clone the repo when you use 'git fsck'.\n\nI don't have time to fix this now, but what's the best approach here?\nMake all the relevant commands copy what fsck_config() is doing, or\nshould fsck_object() be lazily looking up this config by itself?\n"},{"id":"348436","messageId":"20180524155807.GA11253@alpha","threadId":"48565","inReplyTo":"87vabd6pp2.fsf@evledraar.gmail.com","subject":"Re: BUG: No way to set fsck. when cloning","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2018-05-24T15:58:08Z","receivedAt":"2018-05-24T15:58:25Z","isPatch":false,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Thu, May 24, 2018 at 05:25:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n> When I do:\n> \n> git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git\n> \n> I get:\n> \n> error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes\n> fatal: Error in object\n> fatal: index-pack failed\n> \n> The docs (https://git-scm.com/docs/git-config#git-config-fsckltmsg-idgt)\n> say you can override this with -c fsck.zeroPaddedFilemode = ignore, but\n> I see in builtin/fsck.c that just fsck_config() knows about this, and\n> indeed this works *after* you clone the repo when you use 'git fsck'.\n> \n> I don't have time to fix this now, but what's the best approach here?\n> Make all the relevant commands copy what fsck_config() is doing, or\n> should fsck_object() be lazily looking up this config by itself?\n\nApparently someone reported this earlier[0]. Johannes replied:\n\n> Well, you can apparently have your cake and eat it too (see\n> https://git-scm.com/docs/git-config#git-config-receivefsckltmsg-idgt):\n> \n> receive.fsck.::\n> When `receive.fsckObjects` is set to true, errors can be switched\n> to warnings and vice versa by configuring the `receive.fsck.`\n> setting where the `` is the fsck message ID and the value\n> is one of `error`, `warn` or `ignore`. For convenience, fsck prefixes\n> the error/warning with the message ID, e.g. \"missingEmail: invalid\n> author/committer line - missing email\" means that setting\n> `receive.fsck.missingEmail = ignore` will hide that issue.\n> \n> In your case, use receive.fsck.zeroPaddedFilemode=ignore=warn (or\n> =ignore).\n\n[0]https://public-inbox.org/git/alpine.DEB.2.21.1.1801042125430.32@MININT-6BKU6QN.europe.corp.microsoft.com/\n\nHope this helps, Kevin.\n"},{"id":"348460","messageId":"87tvqx6l4r.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":"20180524155807.GA11253@alpha","subject":"Re: BUG: No way to set fsck. when cloning","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-24T17:04:04Z","receivedAt":"2018-05-24T17:04:11Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, May 24 2018, Kevin Daudt wrote:\n\n> On Thu, May 24, 2018 at 05:25:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>> When I do:\n>>\n>> git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git\n>>\n>> I get:\n>>\n>> error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes\n>> fatal: Error in object\n>> fatal: index-pack failed\n>>\n>> The docs (https://git-scm.com/docs/git-config#git-config-fsckltmsg-idgt)\n>> say you can override this with -c fsck.zeroPaddedFilemode = ignore, but\n>> I see in builtin/fsck.c that just fsck_config() knows about this, and\n>> indeed this works *after* you clone the repo when you use 'git fsck'.\n>>\n>> I don't have time to fix this now, but what's the best approach here?\n>> Make all the relevant commands copy what fsck_config() is doing, or\n>> should fsck_object() be lazily looking up this config by itself?\n>\n> Apparently someone reported this earlier[0]. Johannes replied:\n>\n>> Well, you can apparently have your cake and eat it too (see\n>> https://git-scm.com/docs/git-config#git-config-receivefsckltmsg-idgt):\n>>\n>> receive.fsck.::\n>> When `receive.fsckObjects` is set to true, errors can be switched\n>> to warnings and vice versa by configuring the `receive.fsck.`\n>> setting where the `` is the fsck message ID and the value\n>> is one of `error`, `warn` or `ignore`. For convenience, fsck prefixes\n>> the error/warning with the message ID, e.g. \"missingEmail: invalid\n>> author/committer line - missing email\" means that setting\n>> `receive.fsck.missingEmail = ignore` will hide that issue.\n>>\n>> In your case, use receive.fsck.zeroPaddedFilemode=ignore=warn (or\n>> =ignore).\n>\n> [0]https://public-inbox.org/git/alpine.DEB.2.21.1.1801042125430.32@MININT-6BKU6QN.europe.corp.microsoft.com/\n>\n> Hope this helps, Kevin.\n\nThat doesn't work, because that's for the server-side, but I need the\nfetch.fsck.* that doesn't exist. This works (I'll send a better patch\nwith tests / docs etc. soon):\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 490c38f833..9e4282788e 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -19,6 +19,7 @@\n #include \"sha1-array.h\"\n #include \"oidset.h\"\n #include \"packfile.h\"\n+#include \"fsck.h\"\n\n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -33,6 +34,7 @@ static int agent_supported;\n static int server_supports_filtering;\n static struct lock_file shallow_lock;\n static const char *alternate_shallow_file;\n+static struct strbuf fsck_msg_types = STRBUF_INIT;\n\n /* Remember to update object flag allocation in object.h */\n #define COMPLETE\t(1U << 0)\n@@ -935,7 +937,8 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\t */\n \t\t\targv_array_push(&cmd.args, \"--fsck-objects\");\n \t\telse\n-\t\t\targv_array_push(&cmd.args, \"--strict\");\n+\t\t\targv_array_pushf(&cmd.args, \"--strict%s\",\n+\t\t\t\t\t fsck_msg_types.buf);\n \t}\n\n \tcmd.in = demux.out;\n@@ -1409,6 +1412,31 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \treturn ref;\n }\n\n+static int fetch_pack_config_cb(const char *var, const char *value, void *cb)\n+{\n+\tif (strcmp(var, \"fetch.fsck.skiplist\") == 0) {\n+\t\tconst char *path;\n+\n+\t\tif (git_config_pathname(&path, var, value))\n+\t\t\treturn 1;\n+\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n+\t\t\tfsck_msg_types.len ? ',' : '=', path);\n+\t\tfree((char *)path);\n+\t\treturn 0;\n+\t}\n+\n+\tif (skip_prefix(var, \"fetch.fsck.\", &var)) {\n+\t\tif (is_valid_msg_type(var, value))\n+\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n+\t\t\t\tfsck_msg_types.len ? ',' : '=', var, value);\n+\t\telse\n+\t\t\twarning(\"Skipping unknown msg id '%s'\", var);\n+\t\treturn 0;\n+\t}\n+\n+\treturn git_default_config(var, value, cb);\n+}\n+\n static void fetch_pack_config(void)\n {\n \tgit_config_get_int(\"fetch.unpacklimit\", &fetch_unpack_limit);\n@@ -1417,7 +1445,7 @@ static void fetch_pack_config(void)\n \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n\n-\tgit_config(git_default_config, NULL);\n+\tgit_config(fetch_pack_config_cb, NULL);\n }\n\n static void fetch_pack_setup(void)\n"},{"id":"348461","messageId":"20180524170430.GA14876@sigill.intra.peff.net","threadId":"48565","inReplyTo":"87vabd6pp2.fsf@evledraar.gmail.com","subject":"Re: BUG: No way to set fsck. when cloning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-24T17:04:30Z","receivedAt":"2018-05-24T17:04:37Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 24, 2018 at 05:25:29PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> When I do:\n> \n> git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git\n> \n> I get:\n> \n> error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes\n> fatal: Error in object\n> fatal: index-pack failed\n> \n> The docs (https://git-scm.com/docs/git-config#git-config-fsckltmsg-idgt)\n> say you can override this with -c fsck.zeroPaddedFilemode = ignore, but\n> I see in builtin/fsck.c that just fsck_config() knows about this, and\n> indeed this works *after* you clone the repo when you use 'git fsck'.\n> \n> I don't have time to fix this now, but what's the best approach here?\n> Make all the relevant commands copy what fsck_config() is doing, or\n> should fsck_object() be lazily looking up this config by itself?\n\nI think the relevant commands need to do it themselves. We already have\nreceive.fsck.*. I don't think there's an equivalent for fetching, but\nthere probably should be. But fsck_object() doesn't have the context to\nknow which set should be used.\n\n-Peff\n"},{"id":"348465","messageId":"20180524190214.GA21354@sigill.intra.peff.net","threadId":"48565","inReplyTo":"87tvqx6l4r.fsf@evledraar.gmail.com","subject":"Re: BUG: No way to set fsck. when cloning","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-24T19:02:14Z","receivedAt":"2018-05-24T19:02:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 24, 2018 at 07:04:04PM +0200, Ævar Arnfjörð Bjarmason wrote:\n\n> That doesn't work, because that's for the server-side, but I need the\n> fetch.fsck.* that doesn't exist. This works (I'll send a better patch\n> with tests / docs etc. soon):\n\nYeah, I think this is the right direction.\n\n> +static int fetch_pack_config_cb(const char *var, const char *value, void *cb)\n> +{\n> +\tif (strcmp(var, \"fetch.fsck.skiplist\") == 0) {\n> +\t\tconst char *path;\n> +\n> +\t\tif (git_config_pathname(&path, var, value))\n> +\t\t\treturn 1;\n> +\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n> +\t\t\tfsck_msg_types.len ? ',' : '=', path);\n> +\t\tfree((char *)path);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (skip_prefix(var, \"fetch.fsck.\", &var)) {\n> +\t\tif (is_valid_msg_type(var, value))\n> +\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n> +\t\t\t\tfsck_msg_types.len ? ',' : '=', var, value);\n> +\t\telse\n> +\t\t\twarning(\"Skipping unknown msg id '%s'\", var);\n> +\t\treturn 0;\n> +\t}\n\nThis matches what's in receive-pack, though the way we stuff all of the\noptions into a flat string is kind of nasty. I also wonder if we'd\neventually run up against command-line limits if somebody had a\ncomplicated fsck config.\n\nI wonder if we should simply be telling index-pack \"please read fsck\nconfig from the prefix 'fetch.fsck'\" instead.\n\nI dunno, maybe I am just creating work. Certainly I don't think it\nshould be a blocker for adding fetch.fsck support. But if you want to\nthink about it while you are here or write a patch, I wouldn't complain. :)\n\n-Peff\n"},{"id":"348472","messageId":"20180524193516.28713-1-avarab@gmail.com","threadId":"48565","inReplyTo":"20180524190214.GA21354@sigill.intra.peff.net","subject":"[PATCH 0/4] fsck: doc fixes & fetch.fsck.* implementation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-24T19:35:12Z","receivedAt":"2018-05-24T19:35:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Thu, May 24, 2018 at 9:02 PM, Jeff King wrote:\n> On Thu, May 24, 2018 at 07:04:04PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>\n>> That doesn't work, because that's for the server-side, but I need the\n>> fetch.fsck.* that doesn't exist. This works (I'll send a better patch\n>> with tests / docs etc. soon):\n>\n> Yeah, I think this is the right direction.\n>\n>> +static int fetch_pack_config_cb(const char *var, const char *value, void *cb)\n>> +{\n>> +     if (strcmp(var, \"fetch.fsck.skiplist\") == 0) {\n>> +             const char *path;\n>> +\n>> +             if (git_config_pathname(&path, var, value))\n>> +                     return 1;\n>> +             strbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n>> +                     fsck_msg_types.len ? ',' : '=', path);\n>> +             free((char *)path);\n>> +             return 0;\n>> +     }\n>> +\n>> +     if (skip_prefix(var, \"fetch.fsck.\", &var)) {\n>> +             if (is_valid_msg_type(var, value))\n>> +                     strbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n>> +                             fsck_msg_types.len ? ',' : '=', var, value);\n>> +             else\n>> +                     warning(\"Skipping unknown msg id '%s'\", var);\n>> +             return 0;\n>> +     }\n>\n> This matches what's in receive-pack, though the way we stuff all of the\n> options into a flat string is kind of nasty. I also wonder if we'd\n> eventually run up against command-line limits if somebody had a\n> complicated fsck config.\n\nYeah, but I'm leaving this for the future. I doubt that it's going to\nhappen in practice, although if you have a huge skip-list...\n\n> I wonder if we should simply be telling index-pack \"please read fsck\n> config from the prefix 'fetch.fsck'\" instead.\n\nI think this whole notion of reading the same config from two places\nsucks, and now with my patches it's three. But I can't think of a\nreasonable way to make it better without even more complexity, and\nmaybe it's better to split it up anyway, i.e. the stuff you want to\naccept is different that fsck and fetch.\n\n> I dunno, maybe I am just creating work. Certainly I don't think it\n> should be a blocker for adding fetch.fsck support. But if you want to\n> think about it while you are here or write a patch, I wouldn't complain. :)\n\nSorry, too late. I already wrote all of this :)\n\nÆvar Arnfjörð Bjarmason (4):\n config doc: don't describe *.fetchObjects twice\n config doc: unify the description of fsck.* and receive.fsck.*\n config doc: elaborate on what transfer.fsckObjects does\n fetch: implement fetch.fsck.*\n\n Documentation/config.txt | 113 ++++++++++++++++++++------------\n fetch-pack.c | 32 ++++++++-\n t/t5504-fetch-receive-strict.sh | 46 +++++++++++++\n 3 files changed, 148 insertions(+), 43 deletions(-)\n\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348473","messageId":"20180524193516.28713-2-avarab@gmail.com","threadId":"48565","inReplyTo":"20180524193516.28713-1-avarab@gmail.com","subject":"[PATCH 1/4] config doc: don't describe *.fetchObjects twice","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-24T19:35:13Z","receivedAt":"2018-05-24T19:35:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the copy/pasted description of the fetch.fsckObjects and\nreceive.fsckObjects variables to refer to transfer.fsckObjects\ninstead.\n\nLet's not duplicate the description of what *.fsckObjects does twice.\ninstead let's refer to transfer.fsckObjects from both fetch.* and\nreceive.*.\n\nI don't think this description of it makes much sense, but for now I'm\njust moving the existing documentation around. Making it better will\nbe done in a later patch.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 84e2891aed..623dffd198 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1422,10 +1422,9 @@ fetch.recurseSubmodules::\n \n fetch.fsckObjects::\n \tIf it is set to true, git-fetch-pack will check all fetched\n-\tobjects. It will abort in the case of a malformed object or a\n-\tbroken link. The result of an abort are only dangling objects.\n-\tDefaults to false. If not set, the value of `transfer.fsckObjects`\n-\tis used instead.\n+\tobjects. See `transfer.fsckObjects` for what's\n+\tchecked. Defaults to false. If not set, the value of\n+\t`transfer.fsckObjects` is used instead.\n \n fetch.unpackLimit::\n \tIf the number of objects fetched over the Git native\n@@ -2845,10 +2844,9 @@ receive.certNonceSlop::\n \n receive.fsckObjects::\n \tIf it is set to true, git-receive-pack will check all received\n-\tobjects. It will abort in the case of a malformed object or a\n-\tbroken link. The result of an abort are only dangling objects.\n-\tDefaults to false. If not set, the value of `transfer.fsckObjects`\n-\tis used instead.\n+\tobjects. See `transfer.fsckObjects` for what's checked.\n+\tDefaults to false. If not set, the value of\n+\t`transfer.fsckObjects` is used instead.\n \n receive.fsck.::\n \tWhen `receive.fsckObjects` is set to true, errors can be switched\n@@ -3332,6 +3330,10 @@ transfer.fsckObjects::\n \tWhen `fetch.fsckObjects` or `receive.fsckObjects` are\n \tnot set, the value of this variable is used instead.\n \tDefaults to false.\n++\n+When set, the fetch or receive will abort in the case of a malformed\n+object or a broken link. The result of an abort are only dangling\n+objects.\n \n transfer.hideRefs::\n \tString(s) `receive-pack` and `upload-pack` use to decide which\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348474","messageId":"20180524193516.28713-4-avarab@gmail.com","threadId":"48565","inReplyTo":"20180524193516.28713-1-avarab@gmail.com","subject":"[PATCH 3/4] config doc: elaborate on what transfer.fsckObjects does","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-24T19:35:15Z","receivedAt":"2018-05-24T19:35:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The existing documentation led the user to believe that all we were\ndoing were basic readability sanity checks, but that hasn't been true\nfor a very long time. Update the description to match reality, and\nnote the caveat that there's a quarantine for accepting pushes, but\nnot for fetching.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 16 +++++++++++++---\n 1 file changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 351c541ab8..124f7a187c 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3339,9 +3339,19 @@ transfer.fsckObjects::\n \tnot set, the value of this variable is used instead.\n \tDefaults to false.\n +\n-When set, the fetch or receive will abort in the case of a malformed\n-object or a broken link. The result of an abort are only dangling\n-objects.\n+When set, the fetch receive will abort in the case of a malformed\n+object or a link to a nonexisting object. In addition, various other\n+issues are checked for, including legacy issues (see `fsck.`),\n+and potential security issues like there being a `.GIT` directory (see\n+the release notes for v2.2.1 for details). Other sanity and security\n+checks may be added in future releases.\n++\n+On the receiving side failing fsckObjects will make those objects\n+unreachable, see \"QUARANTINE ENVIRONMENT\" in\n+linkgit:git-receive-pack[1]. On the fetch side the malformed objects\n+will instead be left unreferenced in the repository. That's considered\n+a bug, and hopefully future git release will implement a quarantine\n+for the \"fetch\" side as well.\n \n transfer.hideRefs::\n \tString(s) `receive-pack` and `upload-pack` use to decide which\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348475","messageId":"20180524193516.28713-3-avarab@gmail.com","threadId":"48565","inReplyTo":"20180524193516.28713-1-avarab@gmail.com","subject":"[PATCH 2/4] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-24T19:35:14Z","receivedAt":"2018-05-24T19:35:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The documentation for the fsck. and receive.fsck.\nvariables was mostly duplicated in two places, with fsck.\nmaking no mention of the corresponding receive.fsck., and the\nsame for fsck.skipList.\n\nI spent quite a lot of time today wondering why setting the\nfsck. variant wasn't working to clone a legacy repository (not\nthat that would have worked anyway, but a subsequent patch implements\nfetch.fsck.).\n\nRectify this situation by describing the feature in general terms\nunder the fsck.* documentation, and make the receive.fsck.*\ndocumentation refer to those variables instead.\n\nThis documentation was initially added in 2becf00ff7 (\"fsck: support\ndemoting errors to warnings\", 2015-06-22) and 4b55b9b479 (\"fsck:\ndocument the new receive.fsck. options\", 2015-06-22).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 74 ++++++++++++++++++++++------------------\n 1 file changed, 41 insertions(+), 33 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 623dffd198..351c541ab8 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1554,23 +1554,41 @@ filter..smudge::\n \tlinkgit:gitattributes[5] for details.\n \n fsck.::\n-\tAllows overriding the message type (error, warn or ignore) of a\n-\tspecific message ID such as `missingEmail`.\n-+\n-For convenience, fsck prefixes the error/warning with the message ID,\n-e.g. \"missingEmail: invalid author/committer line - missing email\" means\n-that setting `fsck.missingEmail = ignore` will hide that issue.\n-+\n-This feature is intended to support working with legacy repositories\n-which cannot be repaired without disruptive changes.\n+\tDuring fsck git may find issues with legacy data which\n+\twouldn't be generated by current versions of git, and which\n+\twouldn't be sent over the wire if `transfer.fsckObjects` was\n+\tset. This feature is intended to support working with legacy\n+\trepositories containing such data.\n++\n+Setting `fsck.` will be picked up by linkgit:git-fsck[1], but\n+to accept pushes of such data set `receive.fsck.` instead. The\n+rest of the documentation discusses `fsck.*` for brevity, but the same\n+applies for the corresponding `receive.fsck.*` variables.\n++\n+When `fsck.` is set, errors can be switched to warnings and\n+vice versa by configuring the `fsck.` setting where the\n+`` is the fsck message ID and the value is one of `error`,\n+`warn` or `ignore`. For convenience, fsck prefixes the error/warning\n+with the message ID, e.g. \"missingEmail: invalid author/committer line\n+- missing email\" means that setting `fsck.missingEmail = ignore` will\n+hide that issue.\n++\n+Depending on the circumstances it might be better to use\n+`fsck.skipList` instead to explicitly whitelist those objects that\n+have issues, otherwise new occurrences of the same issue will be\n+hidden going forward, although that's unlikely to happen in practice\n+unless someone is being deliberately malicious.\n \n fsck.skipList::\n-\tThe path to a sorted list of object names (i.e. one SHA-1 per\n-\tline) that are known to be broken in a non-fatal way and should\n-\tbe ignored. This feature is useful when an established project\n-\tshould be accepted despite early commits containing errors that\n-\tcan be safely ignored such as invalid committer email addresses.\n-\tNote: corrupt objects cannot be skipped with this setting.\n+\tLike `fsck.` this variable has a corresponding\n+\t`receive.fsck.skipList` variant.\n++\n+The path to a sorted list of object names (i.e. one SHA-1 per line)\n+that are known to be broken in a non-fatal way and should be\n+ignored. This feature is useful when an established project should be\n+accepted despite early commits containing errors that can be safely\n+ignored such as invalid committer email addresses. Note: corrupt\n+objects cannot be skipped with this setting.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\n@@ -2849,26 +2867,16 @@ receive.fsckObjects::\n \t`transfer.fsckObjects` is used instead.\n \n receive.fsck.::\n-\tWhen `receive.fsckObjects` is set to true, errors can be switched\n-\tto warnings and vice versa by configuring the `receive.fsck.`\n-\tsetting where the `` is the fsck message ID and the value\n-\tis one of `error`, `warn` or `ignore`. For convenience, fsck prefixes\n-\tthe error/warning with the message ID, e.g. \"missingEmail: invalid\n-\tauthor/committer line - missing email\" means that setting\n-\t`receive.fsck.missingEmail = ignore` will hide that issue.\n-+\n-This feature is intended to support working with legacy repositories\n-which would not pass pushing when `receive.fsckObjects = true`, allowing\n-the host to accept repositories with certain known issues but still catch\n-other issues.\n+\tActs like `fsck.`, but is used by\n+\tlinkgit:git-receive-pack[1] instead of\n+\tlinkgit:git-fsck[1]. See the `fsck.` documentation for\n+\tdetails.\n \n receive.fsck.skipList::\n-\tThe path to a sorted list of object names (i.e. one SHA-1 per\n-\tline) that are known to be broken in a non-fatal way and should\n-\tbe ignored. This feature is useful when an established project\n-\tshould be accepted despite early commits containing errors that\n-\tcan be safely ignored such as invalid committer email addresses.\n-\tNote: corrupt objects cannot be skipped with this setting.\n+\tActs like `fsck.skipList`, but is used by\n+\tlinkgit:git-receive-pack[1] instead of\n+\tlinkgit:git-fsck[1]. See the `fsck.skipList` documentation for\n+\tdetails.\n \n receive.keepAlive::\n \tAfter receiving the pack from the client, `receive-pack` may\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348476","messageId":"20180524193516.28713-5-avarab@gmail.com","threadId":"48565","inReplyTo":"20180524193516.28713-1-avarab@gmail.com","subject":"[PATCH 4/4] fetch: implement fetch.fsck.*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-24T19:35:16Z","receivedAt":"2018-05-24T19:35:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Implement support for fetch.fsck.* corresponding with the existing\nreceive.fsck.*. This allows for pedantically cloning repositories with\nspecific issues without turning off fetch.fsckObjects.\n\nOne such repository is https://github.com/robbyrussell/oh-my-zsh.git\nwhich before this change will emit this error when cloned with\nfetch.fsckObjects:\n\n error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes\n fatal: Error in object\n fatal: index-pack failed\n\nNow with fetch.fsck.zeroPaddedFilemode=warn we'll warn about that\nissue, but the clone will succeed:\n\n warning: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes\n warning: object a18c4d13c2a5fa2d4ecd5346c50e119b999b807d: zeroPaddedFilemode: contains zero-padded file modes\n warning: object 84df066176c8da3fd59b13731a86d90f4f1e5c9d: zeroPaddedFilemode: contains zero-padded file modes\n\nThe motivation for this is to be able to turn on fetch.fsckObjects\nglobally across a fleet of computers but still be able to manually\nclone various legacy repositories by either white-listing specific\nissues, or better yet whitelist specific objects.\n\nThe use of --git-dir=* instead of -C in the tests could be considered\nsomewhat archaic, but the tests I'm adding here are duplicating the\ncorresponding receive.* tests with as few changes as possible.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 21 +++++++++++----\n fetch-pack.c | 32 +++++++++++++++++++++--\n t/t5504-fetch-receive-strict.sh | 46 +++++++++++++++++++++++++++++++++\n 3 files changed, 92 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 124f7a187c..af6187f6b5 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1426,6 +1426,16 @@ fetch.fsckObjects::\n \tchecked. Defaults to false. If not set, the value of\n \t`transfer.fsckObjects` is used instead.\n \n+fetch.fsck.::\n+\tActs like `fsck.`, but is used by\n+\tlinkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See\n+\tthe `fsck.` documentation for details.\n+\n+fetch.fsck.skipList::\n+\tActs like `fsck.skipList`, but is used by\n+\tlinkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See\n+\tthe `fsck.skipList` documentation for details.\n+\n fetch.unpackLimit::\n \tIf the number of objects fetched over the Git native\n \ttransfer is below this\n@@ -1561,9 +1571,10 @@ fsck.::\n \trepositories containing such data.\n +\n Setting `fsck.` will be picked up by linkgit:git-fsck[1], but\n-to accept pushes of such data set `receive.fsck.` instead. The\n-rest of the documentation discusses `fsck.*` for brevity, but the same\n-applies for the corresponding `receive.fsck.*` variables.\n+to accept pushes of such data set `receive.fsck.` instead, or\n+to clone or fetch it set `fetch.fsck.`. The rest of the\n+documentation discusses `fsck.*` for brevity, but the same applies for\n+the corresponding `receive.fsck.*` and `fetch..*`. variables.\n +\n When `fsck.` is set, errors can be switched to warnings and\n vice versa by configuring the `fsck.` setting where the\n@@ -1580,8 +1591,8 @@ hidden going forward, although that's unlikely to happen in practice\n unless someone is being deliberately malicious.\n \n fsck.skipList::\n-\tLike `fsck.` this variable has a corresponding\n-\t`receive.fsck.skipList` variant.\n+\tLike `fsck.` this variable has corresponding\n+\t`receive.fsck.skipList` and `fetch.fsck.skipList` variants.\n +\n The path to a sorted list of object names (i.e. one SHA-1 per line)\n that are known to be broken in a non-fatal way and should be\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 490c38f833..9e4282788e 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -19,6 +19,7 @@\n #include \"sha1-array.h\"\n #include \"oidset.h\"\n #include \"packfile.h\"\n+#include \"fsck.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -33,6 +34,7 @@ static int agent_supported;\n static int server_supports_filtering;\n static struct lock_file shallow_lock;\n static const char *alternate_shallow_file;\n+static struct strbuf fsck_msg_types = STRBUF_INIT;\n \n /* Remember to update object flag allocation in object.h */\n #define COMPLETE\t(1U << 0)\n@@ -935,7 +937,8 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\t */\n \t\t\targv_array_push(&cmd.args, \"--fsck-objects\");\n \t\telse\n-\t\t\targv_array_push(&cmd.args, \"--strict\");\n+\t\t\targv_array_pushf(&cmd.args, \"--strict%s\",\n+\t\t\t\t\t fsck_msg_types.buf);\n \t}\n \n \tcmd.in = demux.out;\n@@ -1409,6 +1412,31 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \treturn ref;\n }\n \n+static int fetch_pack_config_cb(const char *var, const char *value, void *cb)\n+{\n+\tif (strcmp(var, \"fetch.fsck.skiplist\") == 0) {\n+\t\tconst char *path;\n+\n+\t\tif (git_config_pathname(&path, var, value))\n+\t\t\treturn 1;\n+\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n+\t\t\tfsck_msg_types.len ? ',' : '=', path);\n+\t\tfree((char *)path);\n+\t\treturn 0;\n+\t}\n+\n+\tif (skip_prefix(var, \"fetch.fsck.\", &var)) {\n+\t\tif (is_valid_msg_type(var, value))\n+\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n+\t\t\t\tfsck_msg_types.len ? ',' : '=', var, value);\n+\t\telse\n+\t\t\twarning(\"Skipping unknown msg id '%s'\", var);\n+\t\treturn 0;\n+\t}\n+\n+\treturn git_default_config(var, value, cb);\n+}\n+\n static void fetch_pack_config(void)\n {\n \tgit_config_get_int(\"fetch.unpacklimit\", &fetch_unpack_limit);\n@@ -1417,7 +1445,7 @@ static void fetch_pack_config(void)\n \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(fetch_pack_config_cb, NULL);\n }\n \n static void fetch_pack_setup(void)\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 49d3621a92..b7f5222c2b 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -135,6 +135,20 @@ test_expect_success 'push with receive.fsck.skipList' '\n \tgit push --porcelain dst bogus\n '\n \n+test_expect_success 'fetch with fetch.fsck.skipList' '\n+\tcommit=\"$(git hash-object -t commit -w --stdin dst/.git/SKIP &&\n+\tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec\n+'\n+\n+\n test_expect_success 'push with receive.fsck.missingEmail=warn' '\n \tcommit=\"$(git hash-object -t commit -w --stdin act 2>&1 &&\n+\tgrep \"missingEmail\" act &&\n+\tgit --git-dir=dst/.git branch -D bogus &&\n+\tgit --git-dir=dst/.git config --add \\\n+\t\treceive.fsck.missingEmail ignore &&\n+\tgit --git-dir=dst/.git config --add \\\n+\t\treceive.fsck.badDate warn &&\n+\tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec >act 2>&1 &&\n+\t! grep \"missingEmail\" act\n+'\n+\n test_expect_success \\\n \t'receive.fsck.unterminatedHeader=warn triggers error' '\n \trm -rf dst &&\n@@ -166,4 +201,15 @@ test_expect_success \\\n \tgrep \"Cannot demote unterminatedheader\" act\n '\n \n+test_expect_success \\\n+\t'fetch.fsck.unterminatedHeader=warn triggers error' '\n+\trm -rf dst &&\n+\tgit init dst &&\n+\tgit --git-dir=dst/.git config fetch.fsckobjects true &&\n+\tgit --git-dir=dst/.git config \\\n+\t\tfetch.fsck.unterminatedheader warn &&\n+\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" HEAD &&\n+\tgrep \"Cannot demote unterminatedheader\" act\n+'\n+\n test_done\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348480","messageId":"CAPig+cTr2J6yj39NNdV1vT8CQP2qyPxNB1ggaE9bGgXukuq1yQ@mail.gmail.com","threadId":"48565","inReplyTo":"20180524193516.28713-3-avarab@gmail.com","subject":"Re: [PATCH 2/4] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-24T19:53:57Z","receivedAt":"2018-05-24T19:54:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 24, 2018 at 3:35 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> The documentation for the fsck. and receive.fsck.\n> variables was mostly duplicated in two places, with fsck.\n> making no mention of the corresponding receive.fsck., and the\n> same for fsck.skipList.\n> [...]\n> Rectify this situation by describing the feature in general terms\n> under the fsck.* documentation, and make the receive.fsck.*\n> documentation refer to those variables instead.\n> [...]\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> @@ -1554,23 +1554,41 @@ filter..smudge::\n> fsck.skipList::\n> - The path to a sorted list of object names (i.e. one SHA-1 per\n> - line) that are known to be broken in a non-fatal way and should\n> - be ignored. This feature is useful when an established project\n> - should be accepted despite early commits containing errors that\n> - can be safely ignored such as invalid committer email addresses.\n> - Note: corrupt objects cannot be skipped with this setting.\n> + Like `fsck.` this variable has a corresponding\n> + `receive.fsck.skipList` variant.\n> ++\n> +The path to a sorted list of object names (i.e. one SHA-1 per line)\n> +that are known to be broken in a non-fatal way and should be\n> +ignored. This feature is useful when an established project should be\n> +accepted despite early commits containing errors that can be safely\n> +ignored such as invalid committer email addresses. Note: corrupt\n> +objects cannot be skipped with this setting.\n\nNit: This organization seems backward. Typically, one would describe\nwhat the option is for and then add the incidental note (\"Like\nfsck.<...>, this variable...\") at the end. It's not clear why this\npatch demotes the description to a secondary paragraph and considers\nthe incidental note as primary.\n"},{"id":"348484","messageId":"87r2m07qz6.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":"CAPig+cTr2J6yj39NNdV1vT8CQP2qyPxNB1ggaE9bGgXukuq1yQ@mail.gmail.com","subject":"Re: [PATCH 2/4] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-24T20:12:29Z","receivedAt":"2018-05-24T20:13:03Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, May 24 2018, Eric Sunshine wrote:\n\n> On Thu, May 24, 2018 at 3:35 PM, Ævar Arnfjörð Bjarmason\n> wrote:\n>> The documentation for the fsck. and receive.fsck.\n>> variables was mostly duplicated in two places, with fsck.\n>> making no mention of the corresponding receive.fsck., and the\n>> same for fsck.skipList.\n>> [...]\n>> Rectify this situation by describing the feature in general terms\n>> under the fsck.* documentation, and make the receive.fsck.*\n>> documentation refer to those variables instead.\n>> [...]\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> @@ -1554,23 +1554,41 @@ filter..smudge::\n>> fsck.skipList::\n>> - The path to a sorted list of object names (i.e. one SHA-1 per\n>> - line) that are known to be broken in a non-fatal way and should\n>> - be ignored. This feature is useful when an established project\n>> - should be accepted despite early commits containing errors that\n>> - can be safely ignored such as invalid committer email addresses.\n>> - Note: corrupt objects cannot be skipped with this setting.\n>> + Like `fsck.` this variable has a corresponding\n>> + `receive.fsck.skipList` variant.\n>> ++\n>> +The path to a sorted list of object names (i.e. one SHA-1 per line)\n>> +that are known to be broken in a non-fatal way and should be\n>> +ignored. This feature is useful when an established project should be\n>> +accepted despite early commits containing errors that can be safely\n>> +ignored such as invalid committer email addresses. Note: corrupt\n>> +objects cannot be skipped with this setting.\n>\n> Nit: This organization seems backward. Typically, one would describe\n> what the option is for and then add the incidental note (\"Like\n> fsck.<...>, this variable...\") at the end. It's not clear why this\n> patch demotes the description to a secondary paragraph and considers\n> the incidental note as primary.\n\nI could change it like that. I was thinking that later in the series\nfetch.fsck.* is going to be first in the file, and then the user is told\nto look at this variable, so it made sense to note from the outset that\nwe're describing several variables here.\n\nWhat do you think?\n"},{"id":"348486","messageId":"CAPig+cSd32O3ELaHxLD0_yRFmjMAo2k-jApopJmuEt7Z6W40-g@mail.gmail.com","threadId":"48565","inReplyTo":"20180524193516.28713-4-avarab@gmail.com","subject":"Re: [PATCH 3/4] config doc: elaborate on what transfer.fsckObjects does","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-24T20:15:33Z","receivedAt":"2018-05-24T20:15:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 24, 2018 at 3:35 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> The existing documentation led the user to believe that all we were\n> doing were basic readability sanity checks, but that hasn't been true\n> for a very long time. Update the description to match reality, and\n> note the caveat that there's a quarantine for accepting pushes, but\n> not for fetching.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> @@ -3339,9 +3339,19 @@ transfer.fsckObjects::\n> -When set, the fetch or receive will abort in the case of a malformed\n> -object or a broken link. The result of an abort are only dangling\n> -objects.\n> +When set, the fetch receive will abort in the case of a malformed\n\n\"fetch receive\"? Did you mean \"fetch or receive\" (like the original)?\n\n> +object or a link to a nonexisting object. In addition, various other\n\ns/nonexisting/nonexistent/\n\n> +issues are checked for, including legacy issues (see `fsck.`),\n> +and potential security issues like there being a `.GIT` directory (see\n\ns/there being/existence of/\n\n> +the release notes for v2.2.1 for details). Other sanity and security\n> +checks may be added in future releases.\n> ++\n> +On the receiving side failing fsckObjects will make those objects\n\ns/side/&,/\n\n> +unreachable, see \"QUARANTINE ENVIRONMENT\" in\n> +linkgit:git-receive-pack[1]. On the fetch side the malformed objects\n\ns/side/&,/\n\n> +will instead be left unreferenced in the repository. That's considered\n> +a bug, and hopefully future git release will implement a quarantine\n> +for the \"fetch\" side as well.\n\nIf this was a \"BUGS\" section in a man-page, the above might be less\nscary. In this context, however, I wonder if it makes sense to tone it\ndown a bit:\n\n On the fetch side, malformed objects will instead be left\n unreferenced in the repository. (However, in the future, such\n objects may be quarantined for \"fetch\", as well.)\n"},{"id":"348488","messageId":"3e4f0b82-b67a-24d2-c89e-efbe9ee45dd4@virtuell-zuhause.de","threadId":"48565","inReplyTo":"87vabd6pp2.fsf@evledraar.gmail.com","subject":"BUG: No way to set fsck. when cloning","fromName":"Thomas Braun","fromEmail":"thomas.braun@virtuell-zuhause.de","sentAt":"2018-05-24T20:48:16Z","receivedAt":"2018-05-24T20:48:26Z","isPatch":false,"sender":{"key":"thomas.braun@virtuell-zuhause.de","avatar":"https://avatars.githubusercontent.com/u/1185677?v=4"},"body":"Am 24.05.2018 um 17:25 schrieb Ævar Arnfjörð Bjarmason:\n> When I do:\n> \n> git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git\n> \n> I get:\n> \n> error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes\n> fatal: Error in object\n> fatal: index-pack failed\n>\n> [...]\n\nDoing\n\ngit clone --config transfer.fsckobjects=false --config\nreceive.fsckobjects=false --config fetch.fsckobjects=false\ngit@github.com:robbyrussell/oh-my-zsh.git\n\ndoes the trick here (stolen from [1]).\n\n$ git --version\ngit version 2.17.0.windows.1\n\nI don't know why though.\n\n[1]:\nhttps://github.com/michaeljones/breathe/issues/340#issuecomment-390775142\n"},{"id":"348496","messageId":"CAPig+cQeNDfkwJXNVB0xP50KUPFMKTmJhrCSdsAOJTqcNFegAA@mail.gmail.com","threadId":"48565","inReplyTo":"87r2m07qz6.fsf@evledraar.gmail.com","subject":"Re: [PATCH 2/4] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-24T22:49:55Z","receivedAt":"2018-05-24T22:50:04Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, May 24, 2018 at 4:12 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> On Thu, May 24 2018, Eric Sunshine wrote:\n>> On Thu, May 24, 2018 at 3:35 PM, Ævar Arnfjörð Bjarmason\n>> wrote:\n>>> fsck.skipList::\n>>> + Like `fsck.` this variable has a corresponding\n>>> + `receive.fsck.skipList` variant.\n>>> ++\n>>> +The path to a sorted list of object names (i.e. one SHA-1 per line)\n>>> +that are known to be broken in a non-fatal way and should be\n>>> +ignored. This feature is useful when an established project should be\n>>> +accepted despite early commits containing errors that can be safely\n>>> +ignored such as invalid committer email addresses. Note: corrupt\n>>> +objects cannot be skipped with this setting.\n>>\n>> Nit: This organization seems backward. Typically, one would describe\n>> what the option is for and then add the incidental note (\"Like\n>> fsck.<...>, this variable...\") at the end. It's not clear why this\n>> patch demotes the description to a secondary paragraph and considers\n>> the incidental note as primary.\n>\n> I could change it like that. I was thinking that later in the series\n> fetch.fsck.* is going to be first in the file, and then the user is told\n> to look at this variable, so it made sense to note from the outset that\n> we're describing several variables here.\n> What do you think?\n\nI see where you're coming from, however, I would think that readers\narriving at this topic (generally) do so as a result of actively\nlooking for it (as opposed to happening upon it), in which case they\nprobably are directly seeking information about it; the incidental\ninformation is just a bonus after reading what they came to learn.\n\nAnyhow, I don't care too strongly about it (it was just a \"nit\", after all).\n"},{"id":"348508","messageId":"xmqqd0xkpjx8.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"CAPig+cQeNDfkwJXNVB0xP50KUPFMKTmJhrCSdsAOJTqcNFegAA@mail.gmail.com","subject":"Re: [PATCH 2/4] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-25T02:07:31Z","receivedAt":"2018-05-25T02:07:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> I see where you're coming from, however, I would think that readers\n> arriving at this topic (generally) do so as a result of actively\n> looking for it (as opposed to happening upon it), in which case they\n> probably are directly seeking information about it; the incidental\n> information is just a bonus after reading what they came to learn.\n\nYup. That matches my mental model as well.\n"},{"id":"348510","messageId":"xmqq8t88pgmm.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180524193516.28713-2-avarab@gmail.com","subject":"Re: [PATCH 1/4] config doc: don't describe *.fetchObjects twice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-25T03:18:41Z","receivedAt":"2018-05-25T03:18:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the copy/pasted description of the fetch.fsckObjects and\n> receive.fsckObjects variables to refer to transfer.fsckObjects\n> instead.\n>\n> Let's not duplicate the description of what *.fsckObjects does twice.\n> instead let's refer to transfer.fsckObjects from both fetch.* and\n> receive.*.\n\nThe two paragraphs above are duplicating what each other says,\nperhaps meant as a half-joke? Well played if that is the case ;-).\n\nThanks for polishing this area.\n"},{"id":"348511","messageId":"xmqq4liwpggs.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"CAPig+cSd32O3ELaHxLD0_yRFmjMAo2k-jApopJmuEt7Z6W40-g@mail.gmail.com","subject":"Re: [PATCH 3/4] config doc: elaborate on what transfer.fsckObjects does","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-25T03:22:11Z","receivedAt":"2018-05-25T03:28:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> +will instead be left unreferenced in the repository. That's considered\n>> +a bug, and hopefully future git release will implement a quarantine\n>> +for the \"fetch\" side as well.\n>\n> If this was a \"BUGS\" section in a man-page, the above might be less\n> scary. In this context, however, I wonder if it makes sense to tone it\n> down a bit:\n>\n> On the fetch side, malformed objects will instead be left\n> unreferenced in the repository. (However, in the future, such\n> objects may be quarantined for \"fetch\", as well.)\n\nI had an impression that nobody else sayd it is considered as a\nbug. Do we need to say it in this series? I'd rather not--with or\nwithout such a future modification (or lack of plan thereof),\nteaching the fetch side to pay attention to the various fsck tweaks\nis an improvement.\n\n"},{"id":"348514","messageId":"xmqqy3g8nzph.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180524193516.28713-5-avarab@gmail.com","subject":"Re: [PATCH 4/4] fetch: implement fetch.fsck.*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-25T04:09:30Z","receivedAt":"2018-05-25T04:09:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> fsck.skipList::\n> -\tLike `fsck.` this variable has a corresponding\n> -\t`receive.fsck.skipList` variant.\n> +\tLike `fsck.` this variable has corresponding\n> +\t`receive.fsck.skipList` and `fetch.fsck.skipList` variants.\n> +\n> The path to a sorted list of object names (i.e. one SHA-1 per line)\n> that are known to be broken in a non-fatal way and should be\n\nI think I've said this already, but I tend to agree with Eric that\nthis is the other way around. Perhaps that is because I consider\nfsck. the most basic one people would want to understand first,\nand then corresponding .fsck. a mere specialization of\nit. So \"Here is what fsck.skipList does\" followed by \"By the way,\nyou can configure it only for the (internal) fsck run during the\nobject transfer with transfer.fsck.skipList\" feels more natural\npresentation order.\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 490c38f833..9e4282788e 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -19,6 +19,7 @@\n> #include \"sha1-array.h\"\n> #include \"oidset.h\"\n> #include \"packfile.h\"\n> +#include \"fsck.h\"\n> \n> static int transfer_unpack_limit = -1;\n> static int fetch_unpack_limit = -1;\n> @@ -33,6 +34,7 @@ static int agent_supported;\n> static int server_supports_filtering;\n> static struct lock_file shallow_lock;\n> static const char *alternate_shallow_file;\n> +static struct strbuf fsck_msg_types = STRBUF_INIT;\n> \n> /* Remember to update object flag allocation in object.h */\n> #define COMPLETE\t(1U << 0)\n> @@ -935,7 +937,8 @@ static int get_pack(struct fetch_pack_args *args,\n> \t\t\t */\n> \t\t\targv_array_push(&cmd.args, \"--fsck-objects\");\n> \t\telse\n> -\t\t\targv_array_push(&cmd.args, \"--strict\");\n> +\t\t\targv_array_pushf(&cmd.args, \"--strict%s\",\n> +\t\t\t\t\t fsck_msg_types.buf);\n\nThis made a reader wonder what fsck_msg_types.buf[] has.\n\nIt is empty or a comma separated list of things, prefixed with =,\nthat is constructed by repeated calls to fetch_pack_config_cb(), so\nsyntactically what we feed index-pack looks like \"--strict\",\n\"--strict=thing\", or \"--strict=thing1,thing2,....,thingn\". And each\n\"thing\" is either \"=\" or \"skiplist=\".\n\nThe buffer that has both msgtype specification and object name\nshould not be called fsck_msg_types, though. It is probably\nfsck_exception or something.\n\n> +\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n> +\t\t\tfsck_msg_types.len ? ',' : '=', path);\n> +\t\tfree((char *)path);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\tif (skip_prefix(var, \"fetch.fsck.\", &var)) {\n> +\t\tif (is_valid_msg_type(var, value))\n> +\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n> +\t\t\t\tfsck_msg_types.len ? ',' : '=', var, value);\n> +\t\telse\n> +\t\t\twarning(\"Skipping unknown msg id '%s'\", var);\n> +\t\treturn 0;\n> +\t}\n> +\n> +\treturn git_default_config(var, value, cb);\n> +}\n"},{"id":"348520","messageId":"87o9h46vb7.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":"3e4f0b82-b67a-24d2-c89e-efbe9ee45dd4@virtuell-zuhause.de","subject":"Re: BUG: No way to set fsck. when cloning","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-25T07:36:28Z","receivedAt":"2018-05-25T07:36:34Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, May 24 2018, Thomas Braun wrote:\n\n> Am 24.05.2018 um 17:25 schrieb Ævar Arnfjörð Bjarmason:\n>> When I do:\n>>\n>> git -c fetch.fsckObjects=true clone git@github.com:robbyrussell/oh-my-zsh.git\n>>\n>> I get:\n>>\n>> error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes\n>> fatal: Error in object\n>> fatal: index-pack failed\n>>\n>> [...]\n>\n> Doing\n>\n> git clone --config transfer.fsckobjects=false --config\n> receive.fsckobjects=false --config fetch.fsckobjects=false\n> git@github.com:robbyrussell/oh-my-zsh.git\n>\n> does the trick here (stolen from [1]).\n>\n> $ git --version\n> git version 2.17.0.windows.1\n>\n> I don't know why though.\n\nYeah, because it entirely turns off fsck on fetch, and we didn't have\nany way to selectively disable just some stuff until the patch series I\njust sent\n(https://public-inbox.org/git/20180524193516.28713-1-avarab@gmail.com/)\n\n> [1]:\n> https://github.com/michaeljones/breathe/issues/340#issuecomment-390775142\n"},{"id":"348550","messageId":"20180525192811.25680-1-avarab@gmail.com","threadId":"48565","inReplyTo":"20180524193516.28713-1-avarab@gmail.com","subject":"[PATCH v2 0/5] fsck: doc fixes & fetch.fsck.* implementation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-25T19:28:06Z","receivedAt":"2018-05-25T19:28:26Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This should address all the comments to v1. Inter-diff:\n \n 1: a9cd795db5 ! 1: 3d61e44cb8 config doc: don't describe *.fetchObjects twice\n @@ -1,10 +1,6 @@\n Author: Ævar Arnfjörð Bjarmason \n \n config doc: don't describe *.fetchObjects twice\n - \n - Change the copy/pasted description of the fetch.fsckObjects and\n - receive.fsckObjects variables to refer to transfer.fsckObjects\n - instead.\n \n Let's not duplicate the description of what *.fsckObjects does twice.\n instead let's refer to transfer.fsckObjects from both fetch.* and\n 2: 637c2d4241 ! 2: 9683fd2ec6 config doc: unify the description of fsck.* and receive.fsck.*\n @@ -64,21 +64,21 @@\n +unless someone is being deliberately malicious.\n \n fsck.skipList::\n --\tThe path to a sorted list of object names (i.e. one SHA-1 per\n + \tThe path to a sorted list of object names (i.e. one SHA-1 per\n -\tline) that are known to be broken in a non-fatal way and should\n -\tbe ignored. This feature is useful when an established project\n -\tshould be accepted despite early commits containing errors that\n -\tcan be safely ignored such as invalid committer email addresses.\n -\tNote: corrupt objects cannot be skipped with this setting.\n -+\tLike `fsck.` this variable has a corresponding\n -+\t`receive.fsck.skipList` variant.\n ++\tline) that are known to be broken in a non-fatal way and\n ++\tshould be ignored. This feature is useful when an established\n ++\tproject should be accepted despite early commits containing\n ++\terrors that can be safely ignored such as invalid committer\n ++\temail addresses. Note: corrupt objects cannot be skipped with\n ++\tthis setting.\n ++\n -+The path to a sorted list of object names (i.e. one SHA-1 per line)\n -+that are known to be broken in a non-fatal way and should be\n -+ignored. This feature is useful when an established project should be\n -+accepted despite early commits containing errors that can be safely\n -+ignored such as invalid committer email addresses. Note: corrupt\n -+objects cannot be skipped with this setting.\n ++Like `fsck.` this variable has a corresponding\n ++`receive.fsck.skipList` variant.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\n 3: 55dc555196 < -: ------- config doc: elaborate on what transfer.fsckObjects does\n -: ------- > 3: 8e9646a6ce config doc: elaborate on what transfer.fsckObjects does\n -: ------- > 4: 2b3aafdfde config doc: mention future aspirations for transfer.fsckObjects\n 4: 13f4d994c0 ! 5: be32b19696 fetch: implement fetch.fsck.*\n @@ -67,16 +67,16 @@\n When `fsck.` is set, errors can be switched to warnings and\n vice versa by configuring the `fsck.` setting where the\n @@\n - unless someone is being deliberately malicious.\n - \n - fsck.skipList::\n --\tLike `fsck.` this variable has a corresponding\n --\t`receive.fsck.skipList` variant.\n -+\tLike `fsck.` this variable has corresponding\n -+\t`receive.fsck.skipList` and `fetch.fsck.skipList` variants.\n + \temail addresses. Note: corrupt objects cannot be skipped with\n + \tthis setting.\n +\n - The path to a sorted list of object names (i.e. one SHA-1 per line)\n - that are known to be broken in a non-fatal way and should be\n +-Like `fsck.` this variable has a corresponding\n +-`receive.fsck.skipList` variant.\n ++Like `fsck.` this variable has corresponding\n ++`receive.fsck.skipList` and `fetch.fsck.skipList` variants.\n + \n + gc.aggressiveDepth::\n + \tThe depth parameter used in the delta compression\n \n diff --git a/fetch-pack.c b/fetch-pack.c\n --- a/fetch-pack.c\n\nThe \"mention future aspirations for transfer.fsckObjects\" patch is\nnew. I've split up the \"we're probably going to quarantine fetches\ntoo\" part of this.\n\nÆvar Arnfjörð Bjarmason (5):\n config doc: don't describe *.fetchObjects twice\n config doc: unify the description of fsck.* and receive.fsck.*\n config doc: elaborate on what transfer.fsckObjects does\n config doc: mention future aspirations for transfer.fsckObjects\n fetch: implement fetch.fsck.*\n\n Documentation/config.txt | 112 ++++++++++++++++++++------------\n fetch-pack.c | 32 ++++++++-\n t/t5504-fetch-receive-strict.sh | 46 +++++++++++++\n 3 files changed, 148 insertions(+), 42 deletions(-)\n\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348551","messageId":"20180525192811.25680-2-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v2 1/5] config doc: don't describe *.fetchObjects twice","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-25T19:28:07Z","receivedAt":"2018-05-25T19:28:29Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Let's not duplicate the description of what *.fsckObjects does twice.\ninstead let's refer to transfer.fsckObjects from both fetch.* and\nreceive.*.\n\nI don't think this description of it makes much sense, but for now I'm\njust moving the existing documentation around. Making it better will\nbe done in a later patch.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 84e2891aed..623dffd198 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1422,10 +1422,9 @@ fetch.recurseSubmodules::\n \n fetch.fsckObjects::\n \tIf it is set to true, git-fetch-pack will check all fetched\n-\tobjects. It will abort in the case of a malformed object or a\n-\tbroken link. The result of an abort are only dangling objects.\n-\tDefaults to false. If not set, the value of `transfer.fsckObjects`\n-\tis used instead.\n+\tobjects. See `transfer.fsckObjects` for what's\n+\tchecked. Defaults to false. If not set, the value of\n+\t`transfer.fsckObjects` is used instead.\n \n fetch.unpackLimit::\n \tIf the number of objects fetched over the Git native\n@@ -2845,10 +2844,9 @@ receive.certNonceSlop::\n \n receive.fsckObjects::\n \tIf it is set to true, git-receive-pack will check all received\n-\tobjects. It will abort in the case of a malformed object or a\n-\tbroken link. The result of an abort are only dangling objects.\n-\tDefaults to false. If not set, the value of `transfer.fsckObjects`\n-\tis used instead.\n+\tobjects. See `transfer.fsckObjects` for what's checked.\n+\tDefaults to false. If not set, the value of\n+\t`transfer.fsckObjects` is used instead.\n \n receive.fsck.::\n \tWhen `receive.fsckObjects` is set to true, errors can be switched\n@@ -3332,6 +3330,10 @@ transfer.fsckObjects::\n \tWhen `fetch.fsckObjects` or `receive.fsckObjects` are\n \tnot set, the value of this variable is used instead.\n \tDefaults to false.\n++\n+When set, the fetch or receive will abort in the case of a malformed\n+object or a broken link. The result of an abort are only dangling\n+objects.\n \n transfer.hideRefs::\n \tString(s) `receive-pack` and `upload-pack` use to decide which\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348552","messageId":"20180525192811.25680-3-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-25T19:28:08Z","receivedAt":"2018-05-25T19:28:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The documentation for the fsck. and receive.fsck.\nvariables was mostly duplicated in two places, with fsck.\nmaking no mention of the corresponding receive.fsck., and the\nsame for fsck.skipList.\n\nI spent quite a lot of time today wondering why setting the\nfsck. variant wasn't working to clone a legacy repository (not\nthat that would have worked anyway, but a subsequent patch implements\nfetch.fsck.).\n\nRectify this situation by describing the feature in general terms\nunder the fsck.* documentation, and make the receive.fsck.*\ndocumentation refer to those variables instead.\n\nThis documentation was initially added in 2becf00ff7 (\"fsck: support\ndemoting errors to warnings\", 2015-06-22) and 4b55b9b479 (\"fsck:\ndocument the new receive.fsck. options\", 2015-06-22).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 73 ++++++++++++++++++++++------------------\n 1 file changed, 41 insertions(+), 32 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 623dffd198..af7311e73f 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1554,23 +1554,42 @@ filter..smudge::\n \tlinkgit:gitattributes[5] for details.\n \n fsck.::\n-\tAllows overriding the message type (error, warn or ignore) of a\n-\tspecific message ID such as `missingEmail`.\n-+\n-For convenience, fsck prefixes the error/warning with the message ID,\n-e.g. \"missingEmail: invalid author/committer line - missing email\" means\n-that setting `fsck.missingEmail = ignore` will hide that issue.\n-+\n-This feature is intended to support working with legacy repositories\n-which cannot be repaired without disruptive changes.\n+\tDuring fsck git may find issues with legacy data which\n+\twouldn't be generated by current versions of git, and which\n+\twouldn't be sent over the wire if `transfer.fsckObjects` was\n+\tset. This feature is intended to support working with legacy\n+\trepositories containing such data.\n++\n+Setting `fsck.` will be picked up by linkgit:git-fsck[1], but\n+to accept pushes of such data set `receive.fsck.` instead. The\n+rest of the documentation discusses `fsck.*` for brevity, but the same\n+applies for the corresponding `receive.fsck.*` variables.\n++\n+When `fsck.` is set, errors can be switched to warnings and\n+vice versa by configuring the `fsck.` setting where the\n+`` is the fsck message ID and the value is one of `error`,\n+`warn` or `ignore`. For convenience, fsck prefixes the error/warning\n+with the message ID, e.g. \"missingEmail: invalid author/committer line\n+- missing email\" means that setting `fsck.missingEmail = ignore` will\n+hide that issue.\n++\n+Depending on the circumstances it might be better to use\n+`fsck.skipList` instead to explicitly whitelist those objects that\n+have issues, otherwise new occurrences of the same issue will be\n+hidden going forward, although that's unlikely to happen in practice\n+unless someone is being deliberately malicious.\n \n fsck.skipList::\n \tThe path to a sorted list of object names (i.e. one SHA-1 per\n-\tline) that are known to be broken in a non-fatal way and should\n-\tbe ignored. This feature is useful when an established project\n-\tshould be accepted despite early commits containing errors that\n-\tcan be safely ignored such as invalid committer email addresses.\n-\tNote: corrupt objects cannot be skipped with this setting.\n+\tline) that are known to be broken in a non-fatal way and\n+\tshould be ignored. This feature is useful when an established\n+\tproject should be accepted despite early commits containing\n+\terrors that can be safely ignored such as invalid committer\n+\temail addresses. Note: corrupt objects cannot be skipped with\n+\tthis setting.\n++\n+Like `fsck.` this variable has a corresponding\n+`receive.fsck.skipList` variant.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\n@@ -2849,26 +2868,16 @@ receive.fsckObjects::\n \t`transfer.fsckObjects` is used instead.\n \n receive.fsck.::\n-\tWhen `receive.fsckObjects` is set to true, errors can be switched\n-\tto warnings and vice versa by configuring the `receive.fsck.`\n-\tsetting where the `` is the fsck message ID and the value\n-\tis one of `error`, `warn` or `ignore`. For convenience, fsck prefixes\n-\tthe error/warning with the message ID, e.g. \"missingEmail: invalid\n-\tauthor/committer line - missing email\" means that setting\n-\t`receive.fsck.missingEmail = ignore` will hide that issue.\n-+\n-This feature is intended to support working with legacy repositories\n-which would not pass pushing when `receive.fsckObjects = true`, allowing\n-the host to accept repositories with certain known issues but still catch\n-other issues.\n+\tActs like `fsck.`, but is used by\n+\tlinkgit:git-receive-pack[1] instead of\n+\tlinkgit:git-fsck[1]. See the `fsck.` documentation for\n+\tdetails.\n \n receive.fsck.skipList::\n-\tThe path to a sorted list of object names (i.e. one SHA-1 per\n-\tline) that are known to be broken in a non-fatal way and should\n-\tbe ignored. This feature is useful when an established project\n-\tshould be accepted despite early commits containing errors that\n-\tcan be safely ignored such as invalid committer email addresses.\n-\tNote: corrupt objects cannot be skipped with this setting.\n+\tActs like `fsck.skipList`, but is used by\n+\tlinkgit:git-receive-pack[1] instead of\n+\tlinkgit:git-fsck[1]. See the `fsck.skipList` documentation for\n+\tdetails.\n \n receive.keepAlive::\n \tAfter receiving the pack from the client, `receive-pack` may\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348553","messageId":"20180525192811.25680-5-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v2 4/5] config doc: mention future aspirations for transfer.fsckObjects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-25T19:28:10Z","receivedAt":"2018-05-25T19:28:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Jeff King has said on at least a couple of occasions (at least one of\nwhich may have been in person over beer) that leaving corrupt objects\nin the local object store after a \"fetch\" that fails\ntransfer.fsckObjects should be fixed, and we should have something\nlike the server-side quarantine environment on the client-side.\n\nLet's note that in the documentation so we don't seem to be claiming\nthat this is by design. A previous version of this change called the\ncurrent behavior a \"bug\", that's probably too strong a claim, but I\ndon't think anyone would dislike a hypothetical local quarantine\npatch, so let's we might change this in the future.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 71b3805b4e..f97f21c022 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3350,7 +3350,9 @@ security checks may be added in future releases.\n On the receiving side, failing fsckObjects will make those objects\n unreachable, see \"QUARANTINE ENVIRONMENT\" in\n linkgit:git-receive-pack[1]. On the fetch side, malformed objects will\n-instead be left unreferenced in the repository.\n+instead be left unreferenced in the repository. That difference in\n+behavior should not be relied upon. In the future, such objects may be\n+quarantined for \"fetch\" as well.\n \n transfer.hideRefs::\n \tString(s) `receive-pack` and `upload-pack` use to decide which\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348554","messageId":"20180525192811.25680-6-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v2 5/5] fetch: implement fetch.fsck.*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-25T19:28:11Z","receivedAt":"2018-05-25T19:28:35Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Implement support for fetch.fsck.* corresponding with the existing\nreceive.fsck.*. This allows for pedantically cloning repositories with\nspecific issues without turning off fetch.fsckObjects.\n\nOne such repository is https://github.com/robbyrussell/oh-my-zsh.git\nwhich before this change will emit this error when cloned with\nfetch.fsckObjects:\n\n error: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes\n fatal: Error in object\n fatal: index-pack failed\n\nNow with fetch.fsck.zeroPaddedFilemode=warn we'll warn about that\nissue, but the clone will succeed:\n\n warning: object 2b7227859263b6aabcc28355b0b994995b7148b6: zeroPaddedFilemode: contains zero-padded file modes\n warning: object a18c4d13c2a5fa2d4ecd5346c50e119b999b807d: zeroPaddedFilemode: contains zero-padded file modes\n warning: object 84df066176c8da3fd59b13731a86d90f4f1e5c9d: zeroPaddedFilemode: contains zero-padded file modes\n\nThe motivation for this is to be able to turn on fetch.fsckObjects\nglobally across a fleet of computers but still be able to manually\nclone various legacy repositories by either white-listing specific\nissues, or better yet whitelist specific objects.\n\nThe use of --git-dir=* instead of -C in the tests could be considered\nsomewhat archaic, but the tests I'm adding here are duplicating the\ncorresponding receive.* tests with as few changes as possible.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 21 +++++++++++----\n fetch-pack.c | 32 +++++++++++++++++++++--\n t/t5504-fetch-receive-strict.sh | 46 +++++++++++++++++++++++++++++++++\n 3 files changed, 92 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex f97f21c022..f69cd31ad0 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1426,6 +1426,16 @@ fetch.fsckObjects::\n \tchecked. Defaults to false. If not set, the value of\n \t`transfer.fsckObjects` is used instead.\n \n+fetch.fsck.::\n+\tActs like `fsck.`, but is used by\n+\tlinkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See\n+\tthe `fsck.` documentation for details.\n+\n+fetch.fsck.skipList::\n+\tActs like `fsck.skipList`, but is used by\n+\tlinkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See\n+\tthe `fsck.skipList` documentation for details.\n+\n fetch.unpackLimit::\n \tIf the number of objects fetched over the Git native\n \ttransfer is below this\n@@ -1561,9 +1571,10 @@ fsck.::\n \trepositories containing such data.\n +\n Setting `fsck.` will be picked up by linkgit:git-fsck[1], but\n-to accept pushes of such data set `receive.fsck.` instead. The\n-rest of the documentation discusses `fsck.*` for brevity, but the same\n-applies for the corresponding `receive.fsck.*` variables.\n+to accept pushes of such data set `receive.fsck.` instead, or\n+to clone or fetch it set `fetch.fsck.`. The rest of the\n+documentation discusses `fsck.*` for brevity, but the same applies for\n+the corresponding `receive.fsck.*` and `fetch..*`. variables.\n +\n When `fsck.` is set, errors can be switched to warnings and\n vice versa by configuring the `fsck.` setting where the\n@@ -1588,8 +1599,8 @@ fsck.skipList::\n \temail addresses. Note: corrupt objects cannot be skipped with\n \tthis setting.\n +\n-Like `fsck.` this variable has a corresponding\n-`receive.fsck.skipList` variant.\n+Like `fsck.` this variable has corresponding\n+`receive.fsck.skipList` and `fetch.fsck.skipList` variants.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 490c38f833..9e4282788e 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -19,6 +19,7 @@\n #include \"sha1-array.h\"\n #include \"oidset.h\"\n #include \"packfile.h\"\n+#include \"fsck.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -33,6 +34,7 @@ static int agent_supported;\n static int server_supports_filtering;\n static struct lock_file shallow_lock;\n static const char *alternate_shallow_file;\n+static struct strbuf fsck_msg_types = STRBUF_INIT;\n \n /* Remember to update object flag allocation in object.h */\n #define COMPLETE\t(1U << 0)\n@@ -935,7 +937,8 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\t */\n \t\t\targv_array_push(&cmd.args, \"--fsck-objects\");\n \t\telse\n-\t\t\targv_array_push(&cmd.args, \"--strict\");\n+\t\t\targv_array_pushf(&cmd.args, \"--strict%s\",\n+\t\t\t\t\t fsck_msg_types.buf);\n \t}\n \n \tcmd.in = demux.out;\n@@ -1409,6 +1412,31 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \treturn ref;\n }\n \n+static int fetch_pack_config_cb(const char *var, const char *value, void *cb)\n+{\n+\tif (strcmp(var, \"fetch.fsck.skiplist\") == 0) {\n+\t\tconst char *path;\n+\n+\t\tif (git_config_pathname(&path, var, value))\n+\t\t\treturn 1;\n+\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n+\t\t\tfsck_msg_types.len ? ',' : '=', path);\n+\t\tfree((char *)path);\n+\t\treturn 0;\n+\t}\n+\n+\tif (skip_prefix(var, \"fetch.fsck.\", &var)) {\n+\t\tif (is_valid_msg_type(var, value))\n+\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n+\t\t\t\tfsck_msg_types.len ? ',' : '=', var, value);\n+\t\telse\n+\t\t\twarning(\"Skipping unknown msg id '%s'\", var);\n+\t\treturn 0;\n+\t}\n+\n+\treturn git_default_config(var, value, cb);\n+}\n+\n static void fetch_pack_config(void)\n {\n \tgit_config_get_int(\"fetch.unpacklimit\", &fetch_unpack_limit);\n@@ -1417,7 +1445,7 @@ static void fetch_pack_config(void)\n \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(fetch_pack_config_cb, NULL);\n }\n \n static void fetch_pack_setup(void)\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 49d3621a92..b7f5222c2b 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -135,6 +135,20 @@ test_expect_success 'push with receive.fsck.skipList' '\n \tgit push --porcelain dst bogus\n '\n \n+test_expect_success 'fetch with fetch.fsck.skipList' '\n+\tcommit=\"$(git hash-object -t commit -w --stdin dst/.git/SKIP &&\n+\tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec\n+'\n+\n+\n test_expect_success 'push with receive.fsck.missingEmail=warn' '\n \tcommit=\"$(git hash-object -t commit -w --stdin act 2>&1 &&\n+\tgrep \"missingEmail\" act &&\n+\tgit --git-dir=dst/.git branch -D bogus &&\n+\tgit --git-dir=dst/.git config --add \\\n+\t\treceive.fsck.missingEmail ignore &&\n+\tgit --git-dir=dst/.git config --add \\\n+\t\treceive.fsck.badDate warn &&\n+\tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec >act 2>&1 &&\n+\t! grep \"missingEmail\" act\n+'\n+\n test_expect_success \\\n \t'receive.fsck.unterminatedHeader=warn triggers error' '\n \trm -rf dst &&\n@@ -166,4 +201,15 @@ test_expect_success \\\n \tgrep \"Cannot demote unterminatedheader\" act\n '\n \n+test_expect_success \\\n+\t'fetch.fsck.unterminatedHeader=warn triggers error' '\n+\trm -rf dst &&\n+\tgit init dst &&\n+\tgit --git-dir=dst/.git config fetch.fsckobjects true &&\n+\tgit --git-dir=dst/.git config \\\n+\t\tfetch.fsck.unterminatedheader warn &&\n+\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" HEAD &&\n+\tgrep \"Cannot demote unterminatedheader\" act\n+'\n+\n test_done\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348555","messageId":"20180525192811.25680-4-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v2 3/5] config doc: elaborate on what transfer.fsckObjects does","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-25T19:28:09Z","receivedAt":"2018-05-25T19:28:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The existing documentation led the user to believe that all we were\ndoing were basic reachability sanity checks, but that hasn't been true\nfor a very long time. Update the description to match reality, and\nnote the caveat that there's a quarantine for accepting pushes, but\nnot for fetching.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex af7311e73f..71b3805b4e 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3341,8 +3341,16 @@ transfer.fsckObjects::\n \tDefaults to false.\n +\n When set, the fetch or receive will abort in the case of a malformed\n-object or a broken link. The result of an abort are only dangling\n-objects.\n+object or a link to a nonexistent object. In addition, various other\n+issues are checked for, including legacy issues (see `fsck.`),\n+and potential security issues like the existence of a `.GIT` directory\n+(see the release notes for v2.2.1 for details). Other sanity and\n+security checks may be added in future releases.\n++\n+On the receiving side, failing fsckObjects will make those objects\n+unreachable, see \"QUARANTINE ENVIRONMENT\" in\n+linkgit:git-receive-pack[1]. On the fetch side, malformed objects will\n+instead be left unreferenced in the repository.\n \n transfer.hideRefs::\n \tString(s) `receive-pack` and `upload-pack` use to decide which\n-- \n2.17.0.290.gded63e768a\n\n"},{"id":"348557","messageId":"CAP8UFD2wpdTnis1zcjkHFnWDN5ZB+puWrGDX4m_cFWsgvNHn5g@mail.gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-5-avarab@gmail.com","subject":"Re: [PATCH v2 4/5] config doc: mention future aspirations for transfer.fsckObjects","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2018-05-25T20:33:19Z","receivedAt":"2018-05-25T20:33:26Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, May 25, 2018 at 9:28 PM, Ævar Arnfjörð Bjarmason\n wrote:\n>\n> Jeff King has said on at least a couple of occasions (at least one of\n> Let's note that in the documentation so we don't seem to be claiming\n> that this is by design. A previous version of this change called the\n> current behavior a \"bug\", that's probably too strong a claim, but I\n> don't think anyone would dislike a hypothetical local quarantine\n> patch, so let's we might change this in the future.\n\nMaybe: s/so let's we might/so let's say we might/\n"},{"id":"348562","messageId":"CAPig+cTK1iKiYnxCH5D2RnhZxaNocgx9UuNeakxzf4LgFSnd9Q@mail.gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-2-avarab@gmail.com","subject":"Re: [PATCH v2 1/5] config doc: don't describe *.fetchObjects twice","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-25T21:07:54Z","receivedAt":"2018-05-25T21:07:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 25, 2018 at 3:28 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> Let's not duplicate the description of what *.fsckObjects does twice.\n\nNit: \"duplicate\" and \"twice\" smell redundant.\n\n> instead let's refer to transfer.fsckObjects from both fetch.* and\n> receive.*.\n\ns/instead/Instead/\n\nPerhaps the above paragraph can be rewritten:\n\n Refer readers of fetch.fsckObjects and receive.fsckObjects to\n transfer.fsckObjects instead of repeating the description at each\n location.\n\n(Not at all worth a re-roll.)\n\n> I don't think this description of it makes much sense, but for now I'm\n> just moving the existing documentation around. Making it better will\n> be done in a later patch.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n"},{"id":"348563","messageId":"CAPig+cT=i0XYOrMymNjsBJqcLfJ38Ft_=MZPZD3N-AMo8raAHQ@mail.gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-3-avarab@gmail.com","subject":"Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-25T21:16:38Z","receivedAt":"2018-05-25T21:16:42Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 25, 2018 at 3:28 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> The documentation for the fsck. and receive.fsck.\n> variables was mostly duplicated in two places, with fsck.\n> making no mention of the corresponding receive.fsck., and the\n> same for fsck.skipList.\n> [...]\n> Rectify this situation by describing the feature in general terms\n> under the fsck.* documentation, and make the receive.fsck.*\n> documentation refer to those variables instead.\n> [...]\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> @@ -1554,23 +1554,42 @@ filter..smudge::\n> +Depending on the circumstances it might be better to use\n> +`fsck.skipList` instead to explicitly whitelist those objects that\n> +have issues, otherwise new occurrences of the same issue will be\n> +hidden going forward, although that's unlikely to happen in practice\n> +unless someone is being deliberately malicious.\n\nIs it worth mentioning buggy tools and/or other buggy Git\nimplementations as sources?\n\n> fsck.skipList::\n> The path to a sorted list of object names (i.e. one SHA-1 per\n> - line) that are known to be broken in a non-fatal way and should\n> - be ignored. This feature is useful when an established project\n> - should be accepted despite early commits containing errors that\n> - can be safely ignored such as invalid committer email addresses.\n> - Note: corrupt objects cannot be skipped with this setting.\n> + line) that are known to be broken in a non-fatal way and\n> + should be ignored. This feature is useful when an established\n> + project should be accepted despite early commits containing\n> + errors that can be safely ignored such as invalid committer\n> + email addresses. Note: corrupt objects cannot be skipped with\n> + this setting.\n\nNot sure why this paragraph got re-wrapped (without, as far as I can\nsee, any other changes), thus making the patch unnecessarily noisy.\n\n> +Like `fsck.` this variable has a corresponding\n> +`receive.fsck.skipList` variant.\n"},{"id":"348564","messageId":"CAPig+cRzFwKrSC22Th4hJtQAcyqT96mcXBpBdRv38_LK4nSVSg@mail.gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-4-avarab@gmail.com","subject":"Re: [PATCH v2 3/5] config doc: elaborate on what transfer.fsckObjects does","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2018-05-25T21:19:15Z","receivedAt":"2018-05-25T21:19:19Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 25, 2018 at 3:28 PM, Ævar Arnfjörð Bjarmason\n wrote:\n> The existing documentation led the user to believe that all we were\n> doing were basic reachability sanity checks, but that hasn't been true\n> for a very long time. Update the description to match reality, and\n> note the caveat that there's a quarantine for accepting pushes, but\n> not for fetching.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> @@ -3341,8 +3341,16 @@ transfer.fsckObjects::\n> When set, the fetch or receive will abort in the case of a malformed\n> +object or a link to a nonexistent object. In addition, various other\n> +issues are checked for, including legacy issues (see `fsck.`),\n> +and potential security issues like the existence of a `.GIT` directory\n> +(see the release notes for v2.2.1 for details). Other sanity and\n> +security checks may be added in future releases.\n> ++\n> +On the receiving side, failing fsckObjects will make those objects\n> +unreachable, see \"QUARANTINE ENVIRONMENT\" in\n> +linkgit:git-receive-pack[1]. On the fetch side, malformed objects will\n> +instead be left unreferenced in the repository.\n\nThis version looks better. Thanks.\n"},{"id":"348655","messageId":"xmqqa7skjep9.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"CAPig+cT=i0XYOrMymNjsBJqcLfJ38Ft_=MZPZD3N-AMo8raAHQ@mail.gmail.com","subject":"Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-28T09:45:54Z","receivedAt":"2018-05-28T09:45:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n>> @@ -1554,23 +1554,42 @@ filter..smudge::\n>> +Depending on the circumstances it might be better to use\n>> +`fsck.skipList` instead to explicitly whitelist those objects that\n>> +have issues, otherwise new occurrences of the same issue will be\n\nI had to read the \"instead to ...\" part three times before\nconvincing myself that this is a good description, and I agree with\nthe assessment. Perhaps we would want to make the recommendation a\nbit stronger, even.\n\n\tIn general, it is better to enumerate existing objects with\n\tproblems with skipList, instead of listing the kind of\n\tbreakages these problematic objects share to be ignored, as\n\tdoing the latter will allow new instances of the same\n\tbreakages go unnoticed.\n\nIf the project has some tool constraints and have to accept new\n\"broken\" objects on ongoing basis, then fsck. facility may\nmake sense, but that is probably a very narrow special use case.\n\n>> +hidden going forward, although that's unlikely to happen in practice\n>> +unless someone is being deliberately malicious.\n>\n> Is it worth mentioning buggy tools and/or other buggy Git\n> implementations as sources?\n\nOr old Git implementations we ourselves shipped. I do not think it\nis worth mentioning it, but I do agree that \"unless somebody is\nbeing deliberatly malicious\" is misleading, if that is what you are\ngetting at. One use of fsck is about noticing that you are under\nattack, so \"unless someone is being malicious\" there in the sentence\ndoes not make sense. When somebody is attacking you, you do not\nwant to use fsck. to ignore it.\n\nBut that becomes a moot point, if we were to follow the line of\nrewrite I suggested above.\n"},{"id":"348656","messageId":"xmqq6038jelk.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180524193516.28713-1-avarab@gmail.com","subject":"Re: [PATCH 0/4] fsck: doc fixes & fetch.fsck.* implementation","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-28T09:48:07Z","receivedAt":"2018-05-28T09:48:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Thu, May 24, 2018 at 9:02 PM, Jeff King wrote:\n>> On Thu, May 24, 2018 at 07:04:04PM +0200, Ævar Arnfjörð Bjarmason wrote:\n>>\n>>> That doesn't work, because that's for the server-side, but I need the\n>>> fetch.fsck.* that doesn't exist. This works (I'll send a better patch\n>>> with tests / docs etc. soon):\n>>\n>> Yeah, I think this is the right direction.\n\nIt seems that this 4-patch series is sufficiently commented and\nearlier parts can be further polished soonish using help made by\nothers. I need to re-read the last patch (implementation) but I\nthink this generally is good.\n\nThanks.\n"},{"id":"348675","messageId":"CACBZZX5mSj2=JXtVPrPZ8Y1gCyu9p+EMYoSPeFZtuvm3FsdLNg@mail.gmail.com","threadId":"48565","inReplyTo":"xmqqa7skjep9.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-28T16:44:25Z","receivedAt":"2018-05-28T16:44:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Mon, May 28, 2018 at 11:45 AM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>>> @@ -1554,23 +1554,42 @@ filter..smudge::\n>>> +Depending on the circumstances it might be better to use\n>>> +`fsck.skipList` instead to explicitly whitelist those objects that\n>>> +have issues, otherwise new occurrences of the same issue will be\n>\n> I had to read the \"instead to ...\" part three times before\n> convincing myself that this is a good description, and I agree with\n> the assessment. Perhaps we would want to make the recommendation a\n> bit stronger, even.\n>\n> In general, it is better to enumerate existing objects with\n> problems with skipList, instead of listing the kind of\n> breakages these problematic objects share to be ignored, as\n> doing the latter will allow new instances of the same\n> breakages go unnoticed.\n>\n> If the project has some tool constraints and have to accept new\n> \"broken\" objects on ongoing basis, then fsck. facility may\n> make sense, but that is probably a very narrow special use case.\n\nThat makes sense. I'll reword this bit.\n\n>>> +hidden going forward, although that's unlikely to happen in practice\n>>> +unless someone is being deliberately malicious.\n>>\n>> Is it worth mentioning buggy tools and/or other buggy Git\n>> implementations as sources?\n>\n> Or old Git implementations we ourselves shipped. I do not think it\n> is worth mentioning it, but I do agree that \"unless somebody is\n> being deliberatly malicious\" is misleading, if that is what you are\n> getting at. One use of fsck is about noticing that you are under\n> attack, so \"unless someone is being malicious\" there in the sentence\n> does not make sense. When somebody is attacking you, you do not\n> want to use fsck. to ignore it.\n>\n> But that becomes a moot point, if we were to follow the line of\n> rewrite I suggested above.\n\nI'll try to clarify this, but I think we really should have some bit\nthere about historical tools. Realistically no new git tools produce\nthese, so the user needs to be made aware of what the trade-off of\nturning these on is.\n\nThe reality of that is that these objects are exceedingly rare, and\nmostly found in various old repositories. Something like that need to\nbe mentioned so the user can weigh the trade-off of turning this on.\n"},{"id":"348777","messageId":"xmqqr2ltetcy.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"CACBZZX5mSj2=JXtVPrPZ8Y1gCyu9p+EMYoSPeFZtuvm3FsdLNg@mail.gmail.com","subject":"Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-30T03:05:01Z","receivedAt":"2018-05-30T03:05:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> On Mon, May 28, 2018 at 11:45 AM, Junio C Hamano wrote:\n>> If the project has some tool constraints and have to accept new\n>> \"broken\" objects on ongoing basis, then fsck. facility may\n>> make sense, but that is probably a very narrow special use case.\n>\n> That makes sense. I'll reword this bit.\n> ...\n> I'll try to clarify this, but I think we really should have some bit\n> there about historical tools. Realistically no new git tools produce\n> these, so the user needs to be made aware of what the trade-off of\n> turning these on is.\n>\n> The reality of that is that these objects are exceedingly rare, and\n> mostly found in various old repositories. Something like that need to\n> be mentioned so the user can weigh the trade-off of turning this on.\n\nRare or not, once we say \"avoid fsck. unless you have a good\nreason not to\", wouldn't that be sufficient? \n\nBetween \"fsck. makes sense only when you use these rare and\nyou-probably-never-heard-of tools ongoing basis\" and \"when you\nalready have (slightly)broken objects, naming each of them in\nskiplist, rather than covering the class, is better because you want\n*new* instances of the same breakage\", I'd imagine the latter would be\nmore helpful.\n\nIn any case, let's see if there are more input to this topic and\nthen wrap it up in v3 ;-)\n\nThanks.\n"},{"id":"348779","messageId":"xmqqmuwherrc.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"xmqqr2ltetcy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-30T03:39:35Z","receivedAt":"2018-05-30T03:39:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Between \"fsck. makes sense only when you use these rare and\n> you-probably-never-heard-of tools ongoing basis\" and \"when you\n> already have (slightly)broken objects, naming each of them in\n> skiplist, rather than covering the class, is better because you want\n> *new* instances of the same breakage\", I'd imagine the latter would be\n\ns/breakage/& caught/; obviously, otherwise what I typed does not\nmake much sense. Sorry about the premature .\n\n> more helpful.\n>\n> In any case, let's see if there are more input to this topic and\n> then wrap it up in v3 ;-)\n>\n> Thanks.\n"},{"id":"348780","messageId":"xmqqin75ere0.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180525192811.25680-6-avarab@gmail.com","subject":"Re: [PATCH v2 5/5] fetch: implement fetch.fsck.*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-05-30T03:47:35Z","receivedAt":"2018-05-30T03:47:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Earlier I mumbled \"this 4-patch series generally looks good but I\nneed to re-read the implementation step\"; I meant this 5-patch\nseries and here is my impression after re-reading the implementation\nstep.\n\nÆvar Arnfjörð Bjarmason writes:\n\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index f97f21c022..f69cd31ad0 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1426,6 +1426,16 @@ fetch.fsckObjects::\n> ...\n\nNicely written.\n\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index 490c38f833..9e4282788e 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -19,6 +19,7 @@\n> #include \"sha1-array.h\"\n> #include \"oidset.h\"\n> #include \"packfile.h\"\n> +#include \"fsck.h\"\n> \n> static int transfer_unpack_limit = -1;\n> static int fetch_unpack_limit = -1;\n> @@ -33,6 +34,7 @@ static int agent_supported;\n> static int server_supports_filtering;\n> static struct lock_file shallow_lock;\n> static const char *alternate_shallow_file;\n> +static struct strbuf fsck_msg_types = STRBUF_INIT;\n\ns/msg_types[]/exclude[]/ or something, as this is not just about \"we\ntolerate known and future instances of 0-padded mode in trees\", but\nalso \"we know this and that object is bad so do not complain\" as well.\n\nOther than that, the implementation looks good.\n\n> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\n> index 49d3621a92..b7f5222c2b 100755\n> --- a/t/t5504-fetch-receive-strict.sh\n> +++ b/t/t5504-fetch-receive-strict.sh\n> @@ -135,6 +135,20 @@ test_expect_success 'push with receive.fsck.skipList' '\n> \tgit push --porcelain dst bogus\n> '\n> \n> +test_expect_success 'fetch with fetch.fsck.skipList' '\n> +\tcommit=\"$(git hash-object -t commit -w --stdin +\trefspec=refs/heads/bogus:refs/heads/bogus &&\n> +\tgit push . $commit:refs/heads/bogus &&\n> +\trm -rf dst &&\n> +\tgit init dst &&\n> +\tgit --git-dir=dst/.git config fetch.fsckObjects true &&\n> +\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n> +\tgit --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP &&\n> +\techo $commit >dst/.git/SKIP &&\n> +\tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec\n> +'\n\nIf the test does not change meaning when file://$(pwd) is replaced\nwith \".\" (or \"..\" or whatever if other tests below moves cwd\naround), I'd think it is preferrable. Seeing $(pwd) always makes me\nnervous about Windows.\n"},{"id":"348880","messageId":"87fu289tph.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":"xmqqr2ltetcy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-31T07:20:58Z","receivedAt":"2018-05-31T07:21:05Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 30 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> On Mon, May 28, 2018 at 11:45 AM, Junio C Hamano wrote:\n>>> If the project has some tool constraints and have to accept new\n>>> \"broken\" objects on ongoing basis, then fsck. facility may\n>>> make sense, but that is probably a very narrow special use case.\n>>\n>> That makes sense. I'll reword this bit.\n>> ...\n>> I'll try to clarify this, but I think we really should have some bit\n>> there about historical tools. Realistically no new git tools produce\n>> these, so the user needs to be made aware of what the trade-off of\n>> turning these on is.\n>>\n>> The reality of that is that these objects are exceedingly rare, and\n>> mostly found in various old repositories. Something like that need to\n>> be mentioned so the user can weigh the trade-off of turning this on.\n>\n> Rare or not, once we say \"avoid fsck. unless you have a good\n> reason not to\", wouldn't that be sufficient?\n\nIt's our documentation that should be clearly stating those reasons. If\nwe're not saying anything about these being historical bugs, then e.g. I\n(not knowing the implementation) wouldn't have turned this on globally\non my site knowing that because I have none of these now I'm *very*\nunlikely to have them in the future.\n\nThat's different from something that just happens rarely, because a rare\nnon-historical event can be expected to happen in the future.\n\n> Between \"fsck. makes sense only when you use these rare and\n> you-probably-never-heard-of tools ongoing basis\" and \"when you\n> already have (slightly)broken objects, naming each of them in\n> skiplist, rather than covering the class, is better because you want\n> *new* instances of the same breakage\", I'd imagine the latter would be\n> more helpful.\n>\n> In any case, let's see if there are more input to this topic and\n> then wrap it up in v3 ;-)\n>\n> Thanks.\n"},{"id":"348881","messageId":"87efhs9tlw.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":"xmqqin75ere0.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 5/5] fetch: implement fetch.fsck.*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-31T07:23:07Z","receivedAt":"2018-05-31T07:23:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, May 30 2018, Junio C Hamano wrote:\n\n> Earlier I mumbled \"this 4-patch series generally looks good but I\n> need to re-read the implementation step\"; I meant this 5-patch\n> series and here is my impression after re-reading the implementation\n> step.\n>\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index f97f21c022..f69cd31ad0 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -1426,6 +1426,16 @@ fetch.fsckObjects::\n>> ...\n>\n> Nicely written.\n>\n>> diff --git a/fetch-pack.c b/fetch-pack.c\n>> index 490c38f833..9e4282788e 100644\n>> --- a/fetch-pack.c\n>> +++ b/fetch-pack.c\n>> @@ -19,6 +19,7 @@\n>> #include \"sha1-array.h\"\n>> #include \"oidset.h\"\n>> #include \"packfile.h\"\n>> +#include \"fsck.h\"\n>>\n>> static int transfer_unpack_limit = -1;\n>> static int fetch_unpack_limit = -1;\n>> @@ -33,6 +34,7 @@ static int agent_supported;\n>> static int server_supports_filtering;\n>> static struct lock_file shallow_lock;\n>> static const char *alternate_shallow_file;\n>> +static struct strbuf fsck_msg_types = STRBUF_INIT;\n>\n> s/msg_types[]/exclude[]/ or something, as this is not just about \"we\n> tolerate known and future instances of 0-padded mode in trees\", but\n> also \"we know this and that object is bad so do not complain\" as well.\n\nI copied the fsck_msg_types variable as-is from builtin/receive-pack.c\nwhich Johannes added in 5d477a334a (\"fsck (receive-pack): allow demoting\nerrors to warnings\", 2015-06-22).\n\nThat's not a justification for doing the same here, but does your\ncritique also extend to that variable name, so I could fix it there\nwhile I'm at it?\n\n> Other than that, the implementation looks good.\n>\n>> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\n>> index 49d3621a92..b7f5222c2b 100755\n>> --- a/t/t5504-fetch-receive-strict.sh\n>> +++ b/t/t5504-fetch-receive-strict.sh\n>> @@ -135,6 +135,20 @@ test_expect_success 'push with receive.fsck.skipList' '\n>> \tgit push --porcelain dst bogus\n>> '\n>>\n>> +test_expect_success 'fetch with fetch.fsck.skipList' '\n>> +\tcommit=\"$(git hash-object -t commit -w --stdin > +\trefspec=refs/heads/bogus:refs/heads/bogus &&\n>> +\tgit push . $commit:refs/heads/bogus &&\n>> +\trm -rf dst &&\n>> +\tgit init dst &&\n>> +\tgit --git-dir=dst/.git config fetch.fsckObjects true &&\n>> +\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n>> +\tgit --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP &&\n>> +\techo $commit >dst/.git/SKIP &&\n>> +\tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec\n>> +'\n>\n> If the test does not change meaning when file://$(pwd) is replaced\n> with \".\" (or \"..\" or whatever if other tests below moves cwd\n> around), I'd think it is preferrable. Seeing $(pwd) always makes me\n> nervous about Windows.\n\nThanks. Will fix.\n"},{"id":"348883","messageId":"87d0xc9t6f.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":"xmqq4liwpggs.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 3/4] config doc: elaborate on what transfer.fsckObjects does","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-05-31T07:32:24Z","receivedAt":"2018-05-31T07:32:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, May 25 2018, Junio C Hamano wrote:\n\n> Eric Sunshine writes:\n>\n>>> +will instead be left unreferenced in the repository. That's considered\n>>> +a bug, and hopefully future git release will implement a quarantine\n>>> +for the \"fetch\" side as well.\n>>\n>> If this was a \"BUGS\" section in a man-page, the above might be less\n>> scary. In this context, however, I wonder if it makes sense to tone it\n>> down a bit:\n>>\n>> On the fetch side, malformed objects will instead be left\n>> unreferenced in the repository. (However, in the future, such\n>> objects may be quarantined for \"fetch\", as well.)\n>\n> I had an impression that nobody else sayd it is considered as a\n> bug. Do we need to say it in this series? I'd rather not--with or\n> without such a future modification (or lack of plan thereof),\n> teaching the fetch side to pay attention to the various fsck tweaks\n> is an improvement.\n\nI changed this in v2 to tone it down, but given what Jeff's mentioned in\nhttps://public-inbox.org/git/20180531060259.GE17344@sigill.intra.peff.net/\nI'm inclined to bring back some stronger wording for it. Something like:\n\n Due to the non-quarantine nature of the fetch.fsckObjects\n implementation it can not be relied upon like receive.fsckObjects\n can. As objects are unpacked they're written to the object store, so\n there can be cases where malicious objects get introduced even\n though the \"fetch\" fail, only to have a subsequent \"fetch\" succeed\n because only new incoming objects are checked, not those that have\n already been written to the store.\n\n This is considered a bug and will likely be fixed in future versions\n of git. For now the paranoid need to find some way to emulate the\n quarantine environment if they'd like the same protection as\n \"push\". E.g. in the case of an internal mirror do the mirroring in\n two steps, one to fetch the untrusted objects, and then do a second\n \"push\" (which will use the quarantine) to another internal repo, and\n have internal clients consume this pushed-to repository, or embargo\n internal fetches and only allow them once a full \"fsck\" has run (and\n no new fetches have happened in the meantime).\n"},{"id":"348950","messageId":"xmqq4lincqna.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"87fu289tph.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/5] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-01T00:11:05Z","receivedAt":"2018-06-01T00:11:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> It's our documentation that should be clearly stating those reasons. If\n> we're not saying anything about these being historical bugs, then e.g. I\n> (not knowing the implementation) wouldn't have turned this on globally\n> on my site knowing that because I have none of these now I'm *very*\n> unlikely to have them in the future.\n>\n> That's different from something that just happens rarely, because a rare\n> non-historical event can be expected to happen in the future.\n\nInteresting. If I did not know Git at all, I would decide\ncompletely opposite---because I have none of these now, I'm very\nunlikely to have them in the future, so I would leave fsck.\nalone to the generally recommended state (i.e. not customizing\nwithout understanding what I am doing). That way, (1) if that\nunlikely thing happens, I would notice and have a chance to deal\nwith it, and (2) otherwise, I wouldn't have to worry about that\nunlikely event at all.\n\nAnd that decision would not change even if I _knew_ these knobs'\ncategories were invented to align with bugs and anomalies in older\nimplementations of Git.\n\n>\n>> Between \"fsck. makes sense only when you use these rare and\n>> you-probably-never-heard-of tools ongoing basis\" and \"when you\n>> already have (slightly)broken objects, naming each of them in\n>> skiplist, rather than covering the class, is better because you want\n>> *new* instances of the same breakage\", I'd imagine the latter would be\n>> more helpful.\n>>\n>> In any case, let's see if there are more input to this topic and\n>> then wrap it up in v3 ;-)\n>>\n>> Thanks.\n"},{"id":"353707","messageId":"20180727143720.14948-1-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v3 00/10] fsck: doc fixes & fetch.fsck.* implementation","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T14:37:10Z","receivedAt":"2018-07-27T14:37:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"It's been a couple of months since I submitted v2 of this. This takes\ninto account all the feedback in the previous thread\n(https://public-inbox.org/git/20180525192811.25680-1-avarab@gmail.com/).\n\nTo recap, this series is mainly about implementing\nfetch.fsck., so you can turn on transfer.fsckObjects=true and\nstill allow \"fetch\" to fetch a whitelist of repos.\n\nI wrote this because I turned transfer.fsckObjects=true on internally\nwhile the .gitmodules security issue was still under embargo, but then\nfound that we couldn't clone various repos with historical issues\nanymore.\n\nIt's now 10 patches instead of 5. I rolled the spiritual successor of\nhttps://public-inbox.org/git/20180529211950.26896-1-avarab@gmail.com/\ninto this series (as documentation, not tests), and noticed many more\nthings along the way that were already broken or not tested for.\n\nThree of those come after the \"fetch.fsck.*\" implementation, not\nbecause they rely on that new feature, but because now fetch.fsck.* is\none of the three modes (along with fsck and receive) that we'd like to\ntest.\n\nÆvar Arnfjörð Bjarmason (10):\n receive.fsck. tests: remove dead code\n config doc: don't describe *.fetchObjects twice\n config doc: unify the description of fsck.* and receive.fsck.*\n config doc: elaborate on what transfer.fsckObjects does\n config doc: elaborate on fetch.fsckObjects security\n transfer.fsckObjects tests: untangle confusing setup\n fetch: implement fetch.fsck.*\n fsck: test & document {fetch,receive}.fsck.* config fallback\n fsck: add stress tests for fsck.skipList\n fsck: test and document unknown fsck. values\n\n Documentation/config.txt | 138 ++++++++++++++++++++++++--------\n fetch-pack.c | 32 +++++++-\n t/t5504-fetch-receive-strict.sh | 126 ++++++++++++++++++++++++++++-\n 3 files changed, 255 insertions(+), 41 deletions(-)\n\n-- \n2.18.0.345.g5c9ce644c3\n\n"},{"id":"353708","messageId":"20180727143720.14948-2-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v3 01/10] receive.fsck. tests: remove dead code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T14:37:11Z","receivedAt":"2018-07-27T14:37:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Remove the setting of a receive.fsck.badDate config variable to\n\"ignore\". This was added in efaba7cc77 (\"fsck: optionally ignore\nspecific fsck issues completely\", 2015-06-22) but never did anything,\npresumably it was part of some work-in-progress code that never made\nit into git.git.\n\nNone of these tests will emit the \"invalid author/committer line - bad\ndate\" warning. The dates on the commit objects we're setting up are\nnot invalid.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5504-fetch-receive-strict.sh | 2 --\n 1 file changed, 2 deletions(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 49d3621a92..e1f8768094 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -149,8 +149,6 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '\n \tgit --git-dir=dst/.git branch -D bogus &&\n \tgit --git-dir=dst/.git config --add \\\n \t\treceive.fsck.missingEmail ignore &&\n-\tgit --git-dir=dst/.git config --add \\\n-\t\treceive.fsck.badDate warn &&\n \tgit push --porcelain dst bogus >act 2>&1 &&\n \t! grep \"missingEmail\" act\n '\n-- \n2.18.0.345.g5c9ce644c3\n\n"},{"id":"353709","messageId":"20180727143720.14948-3-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v3 02/10] config doc: don't describe *.fetchObjects twice","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T14:37:12Z","receivedAt":"2018-07-27T14:37:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Refer readers of fetch.fsckObjects and receive.fsckObjects to\ntransfer.fsckObjects instead of repeating the description at each\nlocation.\n\nI don't think this description of them makes much sense, but for now\nI'm just moving the existing documentation around. Making it better\nwill be done in a later patch.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 18 ++++++++++--------\n 1 file changed, 10 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 43b2de7b5f..6b99cf8d71 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1463,10 +1463,9 @@ fetch.recurseSubmodules::\n \n fetch.fsckObjects::\n \tIf it is set to true, git-fetch-pack will check all fetched\n-\tobjects. It will abort in the case of a malformed object or a\n-\tbroken link. The result of an abort are only dangling objects.\n-\tDefaults to false. If not set, the value of `transfer.fsckObjects`\n-\tis used instead.\n+\tobjects. See `transfer.fsckObjects` for what's\n+\tchecked. Defaults to false. If not set, the value of\n+\t`transfer.fsckObjects` is used instead.\n \n fetch.unpackLimit::\n \tIf the number of objects fetched over the Git native\n@@ -2889,10 +2888,9 @@ receive.certNonceSlop::\n \n receive.fsckObjects::\n \tIf it is set to true, git-receive-pack will check all received\n-\tobjects. It will abort in the case of a malformed object or a\n-\tbroken link. The result of an abort are only dangling objects.\n-\tDefaults to false. If not set, the value of `transfer.fsckObjects`\n-\tis used instead.\n+\tobjects. See `transfer.fsckObjects` for what's checked.\n+\tDefaults to false. If not set, the value of\n+\t`transfer.fsckObjects` is used instead.\n \n receive.fsck.::\n \tWhen `receive.fsckObjects` is set to true, errors can be switched\n@@ -3389,6 +3387,10 @@ transfer.fsckObjects::\n \tWhen `fetch.fsckObjects` or `receive.fsckObjects` are\n \tnot set, the value of this variable is used instead.\n \tDefaults to false.\n++\n+When set, the fetch or receive will abort in the case of a malformed\n+object or a broken link. The result of an abort are only dangling\n+objects.\n \n transfer.hideRefs::\n \tString(s) `receive-pack` and `upload-pack` use to decide which\n-- \n2.18.0.345.g5c9ce644c3\n\n"},{"id":"353710","messageId":"20180727143720.14948-4-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v3 03/10] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T14:37:13Z","receivedAt":"2018-07-27T14:37:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The documentation for the fsck. and receive.fsck.\nvariables was mostly duplicated in two places, with fsck.\nmaking no mention of the corresponding receive.fsck., and the\nsame for fsck.skipList.\n\nI spent quite a lot of time today wondering why setting the\nfsck. variant wasn't working to clone a legacy repository (not\nthat that would have worked anyway, but a subsequent patch implements\nfetch.fsck.).\n\nRectify this situation by describing the feature in general terms\nunder the fsck.* documentation, and make the receive.fsck.*\ndocumentation refer to those variables instead.\n\nThis documentation was initially added in 2becf00ff7 (\"fsck: support\ndemoting errors to warnings\", 2015-06-22) and 4b55b9b479 (\"fsck:\ndocument the new receive.fsck. options\", 2015-06-22).\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 62 +++++++++++++++++++++++-----------------\n 1 file changed, 35 insertions(+), 27 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 6b99cf8d71..8d08250a5b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1595,15 +1595,30 @@ filter..smudge::\n \tlinkgit:gitattributes[5] for details.\n \n fsck.::\n-\tAllows overriding the message type (error, warn or ignore) of a\n-\tspecific message ID such as `missingEmail`.\n-+\n-For convenience, fsck prefixes the error/warning with the message ID,\n-e.g. \"missingEmail: invalid author/committer line - missing email\" means\n-that setting `fsck.missingEmail = ignore` will hide that issue.\n-+\n-This feature is intended to support working with legacy repositories\n-which cannot be repaired without disruptive changes.\n+\tDuring fsck git may find issues with legacy data which\n+\twouldn't be generated by current versions of git, and which\n+\twouldn't be sent over the wire if `transfer.fsckObjects` was\n+\tset. This feature is intended to support working with legacy\n+\trepositories containing such data.\n++\n+Setting `fsck.` will be picked up by linkgit:git-fsck[1], but\n+to accept pushes of such data set `receive.fsck.` instead.\n++\n+The rest of the documentation discusses `fsck.*` for brevity, but the\n+same applies for the corresponding `receive.fsck.*` variables.\n++\n+When `fsck.` is set, errors can be switched to warnings and\n+vice versa by configuring the `fsck.` setting where the\n+`` is the fsck message ID and the value is one of `error`,\n+`warn` or `ignore`. For convenience, fsck prefixes the error/warning\n+with the message ID, e.g. \"missingEmail: invalid author/committer line\n+- missing email\" means that setting `fsck.missingEmail = ignore` will\n+hide that issue.\n++\n+In general, it is better to enumerate existing objects with problems\n+with `fsck.skipList`, instead of listing the kind of breakages these\n+problematic objects share to be ignored, as doing the latter will\n+allow new instances of the same breakages go unnoticed.\n \n fsck.skipList::\n \tThe path to a sorted list of object names (i.e. one SHA-1 per\n@@ -1612,6 +1627,9 @@ fsck.skipList::\n \tshould be accepted despite early commits containing errors that\n \tcan be safely ignored such as invalid committer email addresses.\n \tNote: corrupt objects cannot be skipped with this setting.\n++\n+Like `fsck.` this variable has a corresponding\n+`receive.fsck.skipList` variant.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\n@@ -2893,26 +2911,16 @@ receive.fsckObjects::\n \t`transfer.fsckObjects` is used instead.\n \n receive.fsck.::\n-\tWhen `receive.fsckObjects` is set to true, errors can be switched\n-\tto warnings and vice versa by configuring the `receive.fsck.`\n-\tsetting where the `` is the fsck message ID and the value\n-\tis one of `error`, `warn` or `ignore`. For convenience, fsck prefixes\n-\tthe error/warning with the message ID, e.g. \"missingEmail: invalid\n-\tauthor/committer line - missing email\" means that setting\n-\t`receive.fsck.missingEmail = ignore` will hide that issue.\n-+\n-This feature is intended to support working with legacy repositories\n-which would not pass pushing when `receive.fsckObjects = true`, allowing\n-the host to accept repositories with certain known issues but still catch\n-other issues.\n+\tActs like `fsck.`, but is used by\n+\tlinkgit:git-receive-pack[1] instead of\n+\tlinkgit:git-fsck[1]. See the `fsck.` documentation for\n+\tdetails.\n \n receive.fsck.skipList::\n-\tThe path to a sorted list of object names (i.e. one SHA-1 per\n-\tline) that are known to be broken in a non-fatal way and should\n-\tbe ignored. This feature is useful when an established project\n-\tshould be accepted despite early commits containing errors that\n-\tcan be safely ignored such as invalid committer email addresses.\n-\tNote: corrupt objects cannot be skipped with this setting.\n+\tActs like `fsck.skipList`, but is used by\n+\tlinkgit:git-receive-pack[1] instead of\n+\tlinkgit:git-fsck[1]. See the `fsck.skipList` documentation for\n+\tdetails.\n \n receive.keepAlive::\n \tAfter receiving the pack from the client, `receive-pack` may\n-- \n2.18.0.345.g5c9ce644c3\n\n"},{"id":"353711","messageId":"20180727143720.14948-5-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v3 04/10] config doc: elaborate on what transfer.fsckObjects does","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T14:37:14Z","receivedAt":"2018-07-27T14:37:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The existing documentation led the user to believe that all we were\ndoing were basic reachability sanity checks, but that hasn't been true\nfor a very long time. Update the description to match reality, and\nnote the caveat that there's a quarantine for accepting pushes, but\nnot for fetching.\n\nAlso mention that the fsck checks for security issues, which was my\ninitial motivation for writing this fetch.fsck.* series.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 13 +++++++++++--\n 1 file changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 8d08250a5b..291b4f3c57 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3397,8 +3397,17 @@ transfer.fsckObjects::\n \tDefaults to false.\n +\n When set, the fetch or receive will abort in the case of a malformed\n-object or a broken link. The result of an abort are only dangling\n-objects.\n+object or a link to a nonexistent object. In addition, various other\n+issues are checked for, including legacy issues (see `fsck.`),\n+and potential security issues like the existence of a `.GIT` directory\n+or a malicious `.gitmodules` file (see the release notes for v2.2.1\n+and v2.17.1 for details). Other sanity and security checks may be\n+added in future releases.\n++\n+On the receiving side, failing fsckObjects will make those objects\n+unreachable, see \"QUARANTINE ENVIRONMENT\" in\n+linkgit:git-receive-pack[1]. On the fetch side, malformed objects will\n+instead be left unreferenced in the repository.\n \n transfer.hideRefs::\n \tString(s) `receive-pack` and `upload-pack` use to decide which\n-- \n2.18.0.345.g5c9ce644c3\n\n"},{"id":"353712","messageId":"20180727143720.14948-6-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v3 05/10] config doc: elaborate on fetch.fsckObjects security","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T14:37:15Z","receivedAt":"2018-07-27T14:37:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the transfer.fsckObjects documentation to explicitly note the\nunique security and/or corruption issues fetch.fsckObjects suffers\nfrom, since it doesn't have a quarantine environment.\n\nThis was already alluded to in the existing documentation, but let's\nspell it out so there's no confusion here, and give a concrete example\nof how to work around this limitation.\n\nLet's also prominently note that this is considered to be a limitation\nof the current implementation, rather than something that's intended\nand by design, since we might change this in the future.\n\nSee\nhttps://public-inbox.org/git/20180531060259.GE17344@sigill.intra.peff.net/\nfor further details.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 21 +++++++++++++++++++++\n 1 file changed, 21 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 291b4f3c57..7ff453c53b 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -3408,6 +3408,27 @@ On the receiving side, failing fsckObjects will make those objects\n unreachable, see \"QUARANTINE ENVIRONMENT\" in\n linkgit:git-receive-pack[1]. On the fetch side, malformed objects will\n instead be left unreferenced in the repository.\n++\n+Due to the non-quarantine nature of the `fetch.fsckObjects`\n+implementation it can not be relied upon to leave the object store\n+clean like `receive.fsckObjects` can.\n++\n+As objects are unpacked they're written to the object store, so there\n+can be cases where malicious objects get introduced even though the\n+\"fetch\" failed, only to have a subsequent \"fetch\" succeed because only\n+new incoming objects are checked, not those that have already been\n+written to the object store. That difference in behavior should not be\n+relied upon. In the future, such objects may be quarantined for\n+\"fetch\" as well.\n++\n+For now, the paranoid need to find some way to emulate the quarantine\n+environment if they'd like the same protection as \"push\". E.g. in the\n+case of an internal mirror do the mirroring in two steps, one to fetch\n+the untrusted objects, and then do a second \"push\" (which will use the\n+quarantine) to another internal repo, and have internal clients\n+consume this pushed-to repository, or embargo internal fetches and\n+only allow them once a full \"fsck\" has run (and no new fetches have\n+happened in the meantime).\n \n transfer.hideRefs::\n \tString(s) `receive-pack` and `upload-pack` use to decide which\n-- \n2.18.0.345.g5c9ce644c3\n\n"},{"id":"353713","messageId":"20180727143720.14948-7-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v3 06/10] transfer.fsckObjects tests: untangle confusing setup","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T14:37:16Z","receivedAt":"2018-07-27T14:37:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The tests for transfer.fsckObjects have grown organically over time to\nnot make much sense.\n\nInitially when these were added in b10a53583f (\"test: fetch/receive\nwith fsckobjects\", 2011-09-04) they were only testing the \"corrupt or\nmissing object\" case, but later on in 70a4ae73d8 (\"fsck: add a simple\ntest for receive.fsck.\", 2015-06-22) they were expanded to\ncheck for the fsck. feature.\n\nThe problem was that we still kept the same corrupt test repo, making\nit harder to add new tests that check the entirety of the repository\nbetween operations via \"git fsck\" to see whether only known issues\nthat can be ignored with fsck. have occurred.\n\nThe tests only did the right thing because such a full \"git fsck\" was\nnever done after a certain point, and instead we were only\nmanipulating specific refs. This makes it harder to add new tests, and\nnone of the fsck. tests relied on this.\n\nSo let's not confuse the two and repair the corrupt repository before\nwe run the fsck. tests.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5504-fetch-receive-strict.sh | 12 +++++++++++-\n 1 file changed, 11 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex e1f8768094..57ff78c201 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -3,13 +3,16 @@\n test_description='fetch/receive strict mode'\n . ./test-lib.sh\n \n-test_expect_success setup '\n+test_expect_success 'setup and inject \"corrupt or missing\" object' '\n \techo hello >greetings &&\n \tgit add greetings &&\n \tgit commit -m greetings &&\n \n \tS=$(git rev-parse :greetings | sed -e \"s|^..|&/|\") &&\n \tX=$(echo bye | git hash-object -w --stdin | sed -e \"s|^..|&/|\") &&\n+\techo $S >S &&\n+\techo $X >X &&\n+\tcp .git/objects/$S .git/objects/$S.back &&\n \tmv -f .git/objects/$X .git/objects/$S &&\n \n \ttest_must_fail git fsck\n@@ -115,6 +118,13 @@ test_expect_success 'push with transfer.fsckobjects' '\n \ttest_cmp exp act\n '\n \n+test_expect_success 'repair the \"corrupt or missing\" object' '\n+\tmv -f .git/objects/$(cat S) .git/objects/$(cat X) &&\n+\tmv .git/objects/$(cat S).back .git/objects/$(cat S) &&\n+\trm -rf .git/objects/$(cat X) &&\n+\tgit fsck\n+'\n+\n cat >bogus-commit <\n---\n Documentation/config.txt | 20 +++++++++++---\n fetch-pack.c | 32 +++++++++++++++++++++--\n t/t5504-fetch-receive-strict.sh | 46 +++++++++++++++++++++++++++++++++\n 3 files changed, 92 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 7ff453c53b..8dace49daa 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1467,6 +1467,16 @@ fetch.fsckObjects::\n \tchecked. Defaults to false. If not set, the value of\n \t`transfer.fsckObjects` is used instead.\n \n+fetch.fsck.::\n+\tActs like `fsck.`, but is used by\n+\tlinkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See\n+\tthe `fsck.` documentation for details.\n+\n+fetch.fsck.skipList::\n+\tActs like `fsck.skipList`, but is used by\n+\tlinkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See\n+\tthe `fsck.skipList` documentation for details.\n+\n fetch.unpackLimit::\n \tIf the number of objects fetched over the Git native\n \ttransfer is below this\n@@ -1602,10 +1612,12 @@ fsck.::\n \trepositories containing such data.\n +\n Setting `fsck.` will be picked up by linkgit:git-fsck[1], but\n-to accept pushes of such data set `receive.fsck.` instead.\n+to accept pushes of such data set `receive.fsck.` instead, or\n+to clone or fetch it set `fetch.fsck.`.\n +\n The rest of the documentation discusses `fsck.*` for brevity, but the\n-same applies for the corresponding `receive.fsck.*` variables.\n+same applies for the corresponding `receive.fsck.*` and\n+`fetch..*`. variables.\n +\n When `fsck.` is set, errors can be switched to warnings and\n vice versa by configuring the `fsck.` setting where the\n@@ -1628,8 +1640,8 @@ fsck.skipList::\n \tcan be safely ignored such as invalid committer email addresses.\n \tNote: corrupt objects cannot be skipped with this setting.\n +\n-Like `fsck.` this variable has a corresponding\n-`receive.fsck.skipList` variant.\n+Like `fsck.` this variable has corresponding\n+`receive.fsck.skipList` and `fetch.fsck.skipList` variants.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 7ccb9c0d45..aea2f6cf26 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -21,6 +21,7 @@\n #include \"packfile.h\"\n #include \"object-store.h\"\n #include \"connected.h\"\n+#include \"fsck.h\"\n \n static int transfer_unpack_limit = -1;\n static int fetch_unpack_limit = -1;\n@@ -35,6 +36,7 @@ static int agent_supported;\n static int server_supports_filtering;\n static struct lock_file shallow_lock;\n static const char *alternate_shallow_file;\n+static struct strbuf fsck_msg_types = STRBUF_INIT;\n \n /* Remember to update object flag allocation in object.h */\n #define COMPLETE\t(1U << 0)\n@@ -937,7 +939,8 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\t */\n \t\t\targv_array_push(&cmd.args, \"--fsck-objects\");\n \t\telse\n-\t\t\targv_array_push(&cmd.args, \"--strict\");\n+\t\t\targv_array_pushf(&cmd.args, \"--strict%s\",\n+\t\t\t\t\t fsck_msg_types.buf);\n \t}\n \n \tcmd.in = demux.out;\n@@ -1458,6 +1461,31 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \treturn ref;\n }\n \n+static int fetch_pack_config_cb(const char *var, const char *value, void *cb)\n+{\n+\tif (strcmp(var, \"fetch.fsck.skiplist\") == 0) {\n+\t\tconst char *path;\n+\n+\t\tif (git_config_pathname(&path, var, value))\n+\t\t\treturn 1;\n+\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n+\t\t\tfsck_msg_types.len ? ',' : '=', path);\n+\t\tfree((char *)path);\n+\t\treturn 0;\n+\t}\n+\n+\tif (skip_prefix(var, \"fetch.fsck.\", &var)) {\n+\t\tif (is_valid_msg_type(var, value))\n+\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n+\t\t\t\tfsck_msg_types.len ? ',' : '=', var, value);\n+\t\telse\n+\t\t\twarning(\"Skipping unknown msg id '%s'\", var);\n+\t\treturn 0;\n+\t}\n+\n+\treturn git_default_config(var, value, cb);\n+}\n+\n static void fetch_pack_config(void)\n {\n \tgit_config_get_int(\"fetch.unpacklimit\", &fetch_unpack_limit);\n@@ -1466,7 +1494,7 @@ static void fetch_pack_config(void)\n \tgit_config_get_bool(\"fetch.fsckobjects\", &fetch_fsck_objects);\n \tgit_config_get_bool(\"transfer.fsckobjects\", &transfer_fsck_objects);\n \n-\tgit_config(git_default_config, NULL);\n+\tgit_config(fetch_pack_config_cb, NULL);\n }\n \n static void fetch_pack_setup(void)\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 57ff78c201..004bfebe98 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -145,6 +145,20 @@ test_expect_success 'push with receive.fsck.skipList' '\n \tgit push --porcelain dst bogus\n '\n \n+test_expect_success 'fetch with fetch.fsck.skipList' '\n+\tcommit=\"$(git hash-object -t commit -w --stdin dst/.git/SKIP &&\n+\tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec\n+'\n+\n+\n test_expect_success 'push with receive.fsck.missingEmail=warn' '\n \tcommit=\"$(git hash-object -t commit -w --stdin act 2>&1 &&\n+\tgrep \"missingEmail\" act &&\n+\trm -rf dst &&\n+\tgit init dst &&\n+\tgit --git-dir=dst/.git config fetch.fsckobjects true &&\n+\tgit --git-dir=dst/.git config \\\n+\t\tfetch.fsck.missingEmail ignore &&\n+\tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec >act 2>&1 &&\n+\t! grep \"missingEmail\" act\n+'\n+\n test_expect_success \\\n \t'receive.fsck.unterminatedHeader=warn triggers error' '\n \trm -rf dst &&\n@@ -174,4 +209,15 @@ test_expect_success \\\n \tgrep \"Cannot demote unterminatedheader\" act\n '\n \n+test_expect_success \\\n+\t'fetch.fsck.unterminatedHeader=warn triggers error' '\n+\trm -rf dst &&\n+\tgit init dst &&\n+\tgit --git-dir=dst/.git config fetch.fsckobjects true &&\n+\tgit --git-dir=dst/.git config \\\n+\t\tfetch.fsck.unterminatedheader warn &&\n+\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" HEAD &&\n+\tgrep \"Cannot demote unterminatedheader\" act\n+'\n+\n test_done\n-- \n2.18.0.345.g5c9ce644c3\n\n"},{"id":"353715","messageId":"20180727143720.14948-9-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v3 08/10] fsck: test & document {fetch,receive}.fsck.* config fallback","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T14:37:18Z","receivedAt":"2018-07-27T14:37:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Test and document that the {fetch,receive}.fsck.* family of variables\ndoesn't fall back on the corresponding .fsck.* variables.\n\nThis was alluded to in the existing documentation by saying that\n\"receive\" looks at receive.fsck.* and \"fsck\" looks at fsck.* etc., but\nit wasn't explicitly stated that there was no fallback, and if you'd\ne.g. like to configure the skipList you need to do that for all three.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 12 ++++++++++++\n t/t5504-fetch-receive-strict.sh | 26 ++++++++++++++++++++++++--\n 2 files changed, 36 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 8dace49daa..57c463c6e2 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1619,6 +1619,12 @@ The rest of the documentation discusses `fsck.*` for brevity, but the\n same applies for the corresponding `receive.fsck.*` and\n `fetch..*`. variables.\n +\n+Unlike variables like `color.ui` and `core.editor` the\n+`receive.fsck.` and `fetch.fsck.` variables will not\n+fall back on the `fsck.` configuration if they aren't set. To\n+uniformly configure the same fsck settings in different circumstances\n+all three of them they must all set to the same values.\n++\n When `fsck.` is set, errors can be switched to warnings and\n vice versa by configuring the `fsck.` setting where the\n `` is the fsck message ID and the value is one of `error`,\n@@ -1642,6 +1648,12 @@ fsck.skipList::\n +\n Like `fsck.` this variable has corresponding\n `receive.fsck.skipList` and `fetch.fsck.skipList` variants.\n++\n+Unlike variables like `color.ui` and `core.editor` the\n+`receive.fsck.skipList` and `fetch.fsck.skipList` variables will not\n+fall back on the `fsck.skipList` configuration if they aren't set. To\n+uniformly configure the same fsck settings in different circumstances\n+all three of them they must all set to the same values.\n \n gc.aggressiveDepth::\n \tThe depth parameter used in the delta compression\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 004bfebe98..771a94b4b6 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -140,8 +140,13 @@ test_expect_success 'push with receive.fsck.skipList' '\n \tgit init dst &&\n \tgit --git-dir=dst/.git config receive.fsckObjects true &&\n \ttest_must_fail git push --porcelain dst bogus &&\n-\tgit --git-dir=dst/.git config receive.fsck.skipList SKIP &&\n \techo $commit >dst/.git/SKIP &&\n+\n+\t# receive.fsck.* does not fall back on fsck.*\n+\tgit --git-dir=dst/.git config fsck.skipList SKIP &&\n+\ttest_must_fail git push --porcelain dst bogus &&\n+\n+\tgit --git-dir=dst/.git config receive.fsck.skipList SKIP &&\n \tgit push --porcelain dst bogus\n '\n \n@@ -153,8 +158,15 @@ test_expect_success 'fetch with fetch.fsck.skipList' '\n \tgit init dst &&\n \tgit --git-dir=dst/.git config fetch.fsckObjects true &&\n \ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n-\tgit --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP &&\n+\tgit --git-dir=dst/.git config fetch.fsck.skipList /dev/null &&\n+\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n \techo $commit >dst/.git/SKIP &&\n+\n+\t# fetch.fsck.* does not fall back on fsck.*\n+\tgit --git-dir=dst/.git config fsck.skipList dst/.git/SKIP &&\n+\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n+\n+\tgit --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP &&\n \tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec\n '\n \n@@ -166,6 +178,11 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '\n \tgit init dst &&\n \tgit --git-dir=dst/.git config receive.fsckobjects true &&\n \ttest_must_fail git push --porcelain dst bogus &&\n+\n+\t# receive.fsck. does not fall back on fsck.\n+\tgit --git-dir=dst/.git config fsck.missingEmail warn &&\n+\ttest_must_fail git push --porcelain dst bogus &&\n+\n \tgit --git-dir=dst/.git config \\\n \t\treceive.fsck.missingEmail warn &&\n \tgit push --porcelain dst bogus >act 2>&1 &&\n@@ -185,6 +202,11 @@ test_expect_success 'fetch with fetch.fsck.missingEmail=warn' '\n \tgit init dst &&\n \tgit --git-dir=dst/.git config fetch.fsckobjects true &&\n \ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n+\n+\t# fetch.fsck. does not fall back on fsck.\n+\tgit --git-dir=dst/.git config fsck.missingEmail warn &&\n+\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n+\n \tgit --git-dir=dst/.git config \\\n \t\tfetch.fsck.missingEmail warn &&\n \tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec >act 2>&1 &&\n-- \n2.18.0.345.g5c9ce644c3\n\n"},{"id":"353716","messageId":"20180727143720.14948-10-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v3 09/10] fsck: add stress tests for fsck.skipList","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T14:37:19Z","receivedAt":"2018-07-27T14:37:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Stress test the parsing logic shared by fsck.skipList and\n{fetch,receive}.fsck.skipList added in cd94c6f91e (\"fsck: git\nreceive-pack: support excluding objects from fsck'ing\",\n2015-06-22). There were no tests for the work done by the\ninit_skiplist() routine, e.g. how it dies on invalid input.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t5504-fetch-receive-strict.sh | 28 ++++++++++++++++++++++++++++\n 1 file changed, 28 insertions(+)\n\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 771a94b4b6..7f06b537d3 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -133,6 +133,14 @@ committer Bugs Bunny 1234567890 +0000\n This commit object intentionally broken\n EOF\n \n+test_expect_success 'fsck with invalid or bogus skipList input' '\n+\tgit -c fsck.skipList=/dev/null -c fsck.missingEmail=ignore fsck &&\n+\ttest_must_fail git -c fsck.skipList=does-not-exist -c fsck.missingEmail=ignore fsck 2>err &&\n+\ttest_i18ngrep \"Could not open skip list: does-not-exist\" err &&\n+\ttest_must_fail git -c fsck.skipList=.git/config -c fsck.missingEmail=ignore fsck 2>err &&\n+\ttest_i18ngrep \"Invalid SHA-1: \\[core\\]\" err\n+'\n+\n test_expect_success 'push with receive.fsck.skipList' '\n \tcommit=\"$(git hash-object -t commit -w --stdin err &&\n+\ttest_i18ngrep \"Could not open skip list: does-not-exist\" err &&\n+\tgit --git-dir=dst/.git config receive.fsck.skipList config &&\n+\ttest_must_fail git push --porcelain dst bogus 2>err &&\n+\ttest_i18ngrep \"Invalid SHA-1: \\[core\\]\" err &&\n+\n \tgit --git-dir=dst/.git config receive.fsck.skipList SKIP &&\n \tgit push --porcelain dst bogus\n '\n@@ -166,6 +184,16 @@ test_expect_success 'fetch with fetch.fsck.skipList' '\n \tgit --git-dir=dst/.git config fsck.skipList dst/.git/SKIP &&\n \ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n \n+\t# Invalid and/or bogus skipList input\n+\tgit --git-dir=dst/.git config fetch.fsck.skipList /dev/null &&\n+\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n+\tgit --git-dir=dst/.git config fetch.fsck.skipList does-not-exist &&\n+\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec 2>err &&\n+\ttest_i18ngrep \"Could not open skip list: does-not-exist\" err &&\n+\tgit --git-dir=dst/.git config fetch.fsck.skipList dst/.git/config &&\n+\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec 2>err &&\n+\ttest_i18ngrep \"Invalid SHA-1: \\[core\\]\" err &&\n+\n \tgit --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP &&\n \tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec\n '\n-- \n2.18.0.345.g5c9ce644c3\n\n"},{"id":"353717","messageId":"20180727143720.14948-11-avarab@gmail.com","threadId":"48565","inReplyTo":"20180525192811.25680-1-avarab@gmail.com","subject":"[PATCH v3 10/10] fsck: test and document unknown fsck. values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T14:37:20Z","receivedAt":"2018-07-27T14:37:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When fsck. is set to an unknown value it'll cause \"fsck\" to\ndie, but the same is not rue of the \"fetch\" and \"receive\"\nvariants. Document this and test for it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/config.txt | 4 ++++\n t/t5504-fetch-receive-strict.sh | 14 ++++++++++++++\n 2 files changed, 18 insertions(+)\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 57c463c6e2..4cead6119a 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1637,6 +1637,10 @@ In general, it is better to enumerate existing objects with problems\n with `fsck.skipList`, instead of listing the kind of breakages these\n problematic objects share to be ignored, as doing the latter will\n allow new instances of the same breakages go unnoticed.\n++\n+Setting an unknown `fsck.` value will cause fsck to die, but\n+doing the same for `receive.fsck.` and `fetch.fsck.`\n+will only cause git to warn.\n \n fsck.skipList::\n \tThe path to a sorted list of object names (i.e. one SHA-1 per\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 7f06b537d3..62f3569891 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -198,6 +198,10 @@ test_expect_success 'fetch with fetch.fsck.skipList' '\n \tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec\n '\n \n+test_expect_success 'fsck. dies' '\n+\ttest_must_fail git -c fsck.whatEver=ignore fsck 2>err &&\n+\ttest_i18ngrep \"Unhandled message id: whatever\" err\n+'\n \n test_expect_success 'push with receive.fsck.missingEmail=warn' '\n \tcommit=\"$(git hash-object -t commit -w --stdin warns\n+\tgit --git-dir=dst/.git config \\\n+\t\treceive.fsck.whatEver error &&\n+\n \tgit --git-dir=dst/.git config \\\n \t\treceive.fsck.missingEmail warn &&\n \tgit push --porcelain dst bogus >act 2>&1 &&\n \tgrep \"missingEmail\" act &&\n+\ttest_i18ngrep \"Skipping unknown msg id.*whatever\" act &&\n \tgit --git-dir=dst/.git branch -D bogus &&\n \tgit --git-dir=dst/.git config --add \\\n \t\treceive.fsck.missingEmail ignore &&\n@@ -235,10 +244,15 @@ test_expect_success 'fetch with fetch.fsck.missingEmail=warn' '\n \tgit --git-dir=dst/.git config fsck.missingEmail warn &&\n \ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n \n+\t# receive.fsck. warns\n+\tgit --git-dir=dst/.git config \\\n+\t\tfetch.fsck.whatEver error &&\n+\n \tgit --git-dir=dst/.git config \\\n \t\tfetch.fsck.missingEmail warn &&\n \tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec >act 2>&1 &&\n \tgrep \"missingEmail\" act &&\n+\ttest_i18ngrep \"Skipping unknown msg id.*whatever\" act &&\n \trm -rf dst &&\n \tgit init dst &&\n \tgit --git-dir=dst/.git config fetch.fsckobjects true &&\n-- \n2.18.0.345.g5c9ce644c3\n\n"},{"id":"353743","messageId":"xmqqva90mr2l.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180727143720.14948-2-avarab@gmail.com","subject":"Re: [PATCH v3 01/10] receive.fsck. tests: remove dead code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T19:11:46Z","receivedAt":"2018-07-27T19:11:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Remove the setting of a receive.fsck.badDate config variable to\n> \"ignore\". This was added in efaba7cc77 (\"fsck: optionally ignore\n> specific fsck issues completely\", 2015-06-22) but never did anything,\n> presumably it was part of some work-in-progress code that never made\n> it into git.git.\n>\n> None of these tests will emit the \"invalid author/committer line - bad\n> date\" warning. The dates on the commit objects we're setting up are\n> not invalid.\n\nIt is a timestamp somewhere mid February of 2009. Perhaps the code\nis playing defensive against the lack of email address on the\ndeliberately broken author line, i.e.\n\n author Bugs Bunny 1234567890 +0000\n committer Bugs Bunny 1234567890 +0000\n\nin case the parser punted and failed to parse that timestamp\ncorrectly? IOW, the above _could_ be a commit written by \"Bugs\nBunny 1234567890\" with missing e-mail and missing timestamp.\n\nSo I dunno. It won't break the test with today's system if we\nremoved this config, but with an updated parser from the next year,\nit may start to break.\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> t/t5504-fetch-receive-strict.sh | 2 --\n> 1 file changed, 2 deletions(-)\n>\n> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\n> index 49d3621a92..e1f8768094 100755\n> --- a/t/t5504-fetch-receive-strict.sh\n> +++ b/t/t5504-fetch-receive-strict.sh\n> @@ -149,8 +149,6 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '\n> \tgit --git-dir=dst/.git branch -D bogus &&\n> \tgit --git-dir=dst/.git config --add \\\n> \t\treceive.fsck.missingEmail ignore &&\n> -\tgit --git-dir=dst/.git config --add \\\n> -\t\treceive.fsck.badDate warn &&\n> \tgit push --porcelain dst bogus >act 2>&1 &&\n> \t! grep \"missingEmail\" act\n> '\n"},{"id":"353744","messageId":"xmqqr2jomqpa.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180727143720.14948-3-avarab@gmail.com","subject":"Re: [PATCH v3 02/10] config doc: don't describe *.fetchObjects twice","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T19:19:45Z","receivedAt":"2018-07-27T19:19:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Refer readers of fetch.fsckObjects and receive.fsckObjects to\n> transfer.fsckObjects instead of repeating the description at each\n> location.\n>\n> I don't think this description of them makes much sense, but for now\n> I'm just moving the existing documentation around. Making it better\n> will be done in a later patch.\n\nGood clean-up to make them better in a later step, which makes sense.\n"},{"id":"353745","messageId":"xmqqmuucmq8s.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180727143720.14948-4-avarab@gmail.com","subject":"Re: [PATCH v3 03/10] config doc: unify the description of fsck.* and receive.fsck.*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T19:29:39Z","receivedAt":"2018-07-27T19:29:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The documentation for the fsck. and receive.fsck.\n> variables was mostly duplicated in two places, with fsck.\n> making no mention of the corresponding receive.fsck., and the\n> same for fsck.skipList.\n\nOverall the result reads a lot easily. Especially this part:\n\n> +\tDuring fsck git may find issues with legacy data which\n> +\twouldn't be generated by current versions of git, and which\n> +\twouldn't be sent over the wire if `transfer.fsckObjects` was\n> +\tset. This feature is intended to support working with legacy\n> +\trepositories containing such data.\n\nthat adds only a few sentences and clarifies the impact of\n\"legacy\"-ness to this issue rather nicely.\n\nI first found it a bit odd that fsck.* and receive.fsck.* are\ndiscussed with an apparent asymmetry but it actually is correct,\nbecause we haven't introduced fetch.fsck.* at this step in the\nseries.\n\nThanks.\n"},{"id":"353746","messageId":"xmqqin50mppn.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180727143720.14948-5-avarab@gmail.com","subject":"Re: [PATCH v3 04/10] config doc: elaborate on what transfer.fsckObjects does","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T19:41:08Z","receivedAt":"2018-07-27T19:41:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The existing documentation led the user to believe that all we were\n> doing were basic reachability sanity checks, but that hasn't been true\n> for a very long time. Update the description to match reality, and\n> note the caveat that there's a quarantine for accepting pushes, but\n> not for fetching.\n>\n> Also mention that the fsck checks for security issues, which was my\n> initial motivation for writing this fetch.fsck.* series.\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/config.txt | 13 +++++++++++--\n> 1 file changed, 11 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 8d08250a5b..291b4f3c57 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -3397,8 +3397,17 @@ transfer.fsckObjects::\n> \tDefaults to false.\n> +\n> When set, the fetch or receive will abort in the case of a malformed\n> ...\n> +On the receiving side, failing fsckObjects will make those objects\n> +unreachable, see \"QUARANTINE ENVIRONMENT\" in\n> +linkgit:git-receive-pack[1]. On the fetch side, malformed objects will\n> +instead be left unreferenced in the repository.\n\n\"On the receiving side\" would contrast better if the counterpart\nwere \"On the fetching side\", no? \n\nIt may be clear to everybody who updates this document and reviews\nsuch updates that \"receive\" is what happens on the other side when\nyou \"push\", but I think it is helpful to new readers if there were a\nhint that indicates the linkage nearby (if merely as a reminder).\n\n\tWhen set, the fetch or receive (i.e. the other side that\n\taccepts your \"push\") will abort in the case of ...\n\n"},{"id":"353747","messageId":"87d0v85uoy.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":"xmqqva90mr2l.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 01/10] receive.fsck. tests: remove dead code","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T19:45:33Z","receivedAt":"2018-07-27T19:45:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jul 27 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Remove the setting of a receive.fsck.badDate config variable to\n>> \"ignore\". This was added in efaba7cc77 (\"fsck: optionally ignore\n>> specific fsck issues completely\", 2015-06-22) but never did anything,\n>> presumably it was part of some work-in-progress code that never made\n>> it into git.git.\n>>\n>> None of these tests will emit the \"invalid author/committer line - bad\n>> date\" warning. The dates on the commit objects we're setting up are\n>> not invalid.\n>\n> It is a timestamp somewhere mid February of 2009. Perhaps the code\n> is playing defensive against the lack of email address on the\n> deliberately broken author line, i.e.\n>\n> author Bugs Bunny 1234567890 +0000\n> committer Bugs Bunny 1234567890 +0000\n\nThis is covered by the \"missingEmail\" part of the test, but there's\nnothing wrong with the timestamp itself.\n\nI doubt Johannes remembers why he did this almost a decade ago, but it\nlooks to me like he was working on some test where the date was also\nbad, and never finished it. There's no point in having that \"badDate\"\nnow.\n\n> in case the parser punted and failed to parse that timestamp\n> correctly? IOW, the above _could_ be a commit written by \"Bugs\n> Bunny 1234567890\" with missing e-mail and missing timestamp.\n>\n> So I dunno. It won't break the test with today's system if we\n> removed this config, but with an updated parser from the next year,\n> it may start to break.\n\nI still think it makes sense to remove this particular thing. Let's add\nexhaustive tests for all this fsck.* stuff in another series, but no\npoint in testing for arbitrary fsck errors that aren't going to be\ntriggered in unrelated tests.\n\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> t/t5504-fetch-receive-strict.sh | 2 --\n>> 1 file changed, 2 deletions(-)\n>>\n>> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\n>> index 49d3621a92..e1f8768094 100755\n>> --- a/t/t5504-fetch-receive-strict.sh\n>> +++ b/t/t5504-fetch-receive-strict.sh\n>> @@ -149,8 +149,6 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '\n>> \tgit --git-dir=dst/.git branch -D bogus &&\n>> \tgit --git-dir=dst/.git config --add \\\n>> \t\treceive.fsck.missingEmail ignore &&\n>> -\tgit --git-dir=dst/.git config --add \\\n>> -\t\treceive.fsck.badDate warn &&\n>> \tgit push --porcelain dst bogus >act 2>&1 &&\n>> \t! grep \"missingEmail\" act\n>> '\n"},{"id":"353748","messageId":"xmqqeffomphy.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180727143720.14948-6-avarab@gmail.com","subject":"Re: [PATCH v3 05/10] config doc: elaborate on fetch.fsckObjects security","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T19:45:45Z","receivedAt":"2018-07-27T19:45:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +For now, the paranoid need to find some way to emulate the quarantine\n> +environment if they'd like the same protection as \"push\". E.g. in the\n\nWe probably should mention that you can immediately prune, as these\nunwanted crufts are unreferenced. That would probably be a lot easier\nworkaround for the intended readers of this document than \"find some\nway to emulate\".\n"},{"id":"353749","messageId":"87bmas5uh6.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":"20180727143720.14948-11-avarab@gmail.com","subject":"Re: [PATCH v3 10/10] fsck: test and document unknown fsck. values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-27T19:50:13Z","receivedAt":"2018-07-27T19:50:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"a\nOn Fri, Jul 27 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> When fsck. is set to an unknown value it'll cause \"fsck\" to\n> die, but the same is not rue of the \"fetch\" and \"receive\"\n\ns/rue/t&/. Looks like we're headed for a v4. I'll fix this typo.\n"},{"id":"353750","messageId":"xmqq6010mo03.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180727143720.14948-8-avarab@gmail.com","subject":"Re: [PATCH v3 07/10] fetch: implement fetch.fsck.*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T20:18:04Z","receivedAt":"2018-07-27T20:18:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> +\n> Setting `fsck.` will be picked up by linkgit:git-fsck[1], but\n> -to accept pushes of such data set `receive.fsck.` instead.\n> +to accept pushes of such data set `receive.fsck.` instead, or\n\nInherited from the original, but I find it a lot more readable to\nwrite \"to accept pushes of such data, set X\" with a comma, given\nespecially that \"data set\" is a noun that has little to do with what\nthis sentence wants to talk about.\n\nI'd suggest doing s/instead/& to loosen the error checking/ or\nsomething, as you are only talking about \"to accept\" (iow, the\ntarget audience is not those who want to reject more by promoting\n\"warn\" to \"error\"---you are talking to those who are only interested\nin demoting \"error\" to \"warn\" or \"ignore\").\n\n> +to clone or fetch it set `fetch.fsck.`.\n\nIt is unclear to me what \"it\" in the last sentence refers to. \n\nThe best rewrite I came up with is \"... to clone or fetch from a\nrepository with such data\" but that is not an exact counterpart to\n\"to accept pushes of such data\", so I am hesitant to suggest it.\n\n\"... to accept such data when fetching or cloning\" might be a better\ncounterpart, but repeated \"to accept\" in this single sentence bothers\nme somewhat. I dunno.\n\nI'll comment on the implementation part of this patch after lunch.\nThanks for working on this.\n"},{"id":"353754","messageId":"xmqqva90l73e.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180727143720.14948-8-avarab@gmail.com","subject":"Re: [PATCH v3 07/10] fetch: implement fetch.fsck.*","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T21:08:37Z","receivedAt":"2018-07-27T21:08:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> -\t\t\targv_array_push(&cmd.args, \"--strict\");\n> +\t\t\targv_array_pushf(&cmd.args, \"--strict%s\",\n> +\t\t\t\t\t fsck_msg_types.buf);\n> ...\n> +\t\tif (git_config_pathname(&path, var, value))\n> +\t\t\treturn 1;\n> +\t\tstrbuf_addf(&fsck_msg_types, \"%cskiplist=%s\",\n> +\t\t\tfsck_msg_types.len ? ',' : '=', path);\n> ...\n> +\t\tif (is_valid_msg_type(var, value))\n> +\t\t\tstrbuf_addf(&fsck_msg_types, \"%c%s=%s\",\n> +\t\t\t\tfsck_msg_types.len ? ',' : '=', var, value);\n> +\t\telse\n> +\t\t\twarning(\"Skipping unknown msg id '%s'\", var);\n\nThis follows quite familiar pattern found in receive_pack_config();\nlooking good.\n\n> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\n> index 57ff78c201..004bfebe98 100755\n> --- a/t/t5504-fetch-receive-strict.sh\n> +++ b/t/t5504-fetch-receive-strict.sh\n> @@ -145,6 +145,20 @@ test_expect_success 'push with receive.fsck.skipList' '\n> \tgit push --porcelain dst bogus\n> '\n> \n> +test_expect_success 'fetch with fetch.fsck.skipList' '\n> +\tcommit=\"$(git hash-object -t commit -w --stdin +\trefspec=refs/heads/bogus:refs/heads/bogus &&\n> +\tgit push . $commit:refs/heads/bogus &&\n\nI see this used in the previous test for receive.fsck.skipList, but\nit is an interesting implementation of \"git update-ref\" that could\nbe affected by potential fsck error in push-to-receive-pack transport.\nAs we are interested in transport into \"dst\" and we want this creation\nof our 'bogus' branch to succeed no matter what, it probably is not\na good idea to use \"git push .\" like this in the context of this test.\n\nPerhaps leave a 'leftoverbits' comment to force us remember to update\nall these uses of local push from the script in the future?\n\n> +\trm -rf dst &&\n> +\tgit init dst &&\n> +\tgit --git-dir=dst/.git config fetch.fsckObjects true &&\n> +\ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n\nWe see that by default fetch.fsckObjects errors out when it notices\nthe bogus commit object.\n\n> +\tgit --git-dir=dst/.git config fetch.fsck.skipList dst/.git/SKIP &&\n> +\techo $commit >dst/.git/SKIP &&\n\nAnd then we set up a skip to see ...\n\n> +\tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec\n\n... if that is ignored. Looks great.\n\nWould this second attempt succeed _without_ the SKIP list, I wonder,\nthough? \n\nAfter the initial attempt that transferred the object, inspected it\nand then aborted before pointing a ref to make the object reachable,\nwouldn't it be possible for the quickfetch codepath to say \"ah, we\nlocally have that object, so let's see it is a descendant of the tip\nof one of our refs *and* all the objects it points at (recursively)\nare all available in this repository\", as we do not quarantine on\nthe fetch side?\n"},{"id":"353756","messageId":"xmqqo9esl66u.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180727143720.14948-9-avarab@gmail.com","subject":"Re: [PATCH v3 08/10] fsck: test & document {fetch,receive}.fsck.* config fallback","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T21:28:09Z","receivedAt":"2018-07-27T21:28:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Test and document that the {fetch,receive}.fsck.* family of variables\n> doesn't fall back on the corresponding .fsck.* variables.\n>\n> This was alluded to in the existing documentation by saying that\n> \"receive\" looks at receive.fsck.* and \"fsck\" looks at fsck.* etc., but\n> it wasn't explicitly stated that there was no fallback, and if you'd\n> e.g. like to configure the skipList you need to do that for all three.\n\nHmph, personally I felt that the updates so far made it clear and\nexplicit enough.\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/config.txt | 12 ++++++++++++\n> t/t5504-fetch-receive-strict.sh | 26 ++++++++++++++++++++++++--\n> 2 files changed, 36 insertions(+), 2 deletions(-)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 8dace49daa..57c463c6e2 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1619,6 +1619,12 @@ The rest of the documentation discusses `fsck.*` for brevity, but the\n> same applies for the corresponding `receive.fsck.*` and\n> `fetch..*`. variables.\n> +\n> +Unlike variables like `color.ui` and `core.editor` the\n> +`receive.fsck.` and `fetch.fsck.` variables will not\n> +fall back on the `fsck.` configuration if they aren't set. To\n\nI do not think \"Unlike ...\" and mention of two sample variables\n(color.ui and core.editor) adds much value to the paragraph,\nprimarily because it does not mention \"color.status\" etc. to show\nwhat kind of fallback the sentence wants to discuss. I actually\nfind the paragraph even clearer to read if it began with \"The\n`receive.fsck.` and ...\".\n\nIf you insist keeping that \"Unlike...\" thing, please have a comma\nbefore \"the `receive.fsck.`\", but I'd recommend to remove\nit.\n\n> +uniformly configure the same fsck settings in different circumstances\n> +all three of them they must all set to the same values.\n\nThat is not incorrect per-se, but I doubt it is a good idea to give\nan impression that most people would want al three to be the same.\n\nYou want fsck..* to be stricter to ensure you do not create\nnew problems, together with fsck.skipList to pardon mistakes that\nhave already happened, and with $transfer.fsck., optionally\nloosen certain class of breakage if a particular project has special\nneeds (e.g. the tool they use is known to produce incorrect\nobjects). Which makes me expect that in most cases, fsck..*\nwould be empty, fsck.skipList to be populated with known-bad ones,\nand $transfer.fsck. may be empty, or may not be empty.\n\nOn the other hand, there may be value in allowing skipList to be\nshared across three, though. You may have known-bad ones and you'd\nneed to give a copy of that known-bad thing to other participants of\nthe project anyway.\n\n"},{"id":"353759","messageId":"xmqqbmasl5hq.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"20180727143720.14948-11-avarab@gmail.com","subject":"Re: [PATCH v3 10/10] fsck: test and document unknown fsck. values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T21:43:13Z","receivedAt":"2018-07-27T21:43:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> When fsck. is set to an unknown value it'll cause \"fsck\" to\n> die, but the same is not rue of the \"fetch\" and \"receive\"\n> variants. Document this and test for it.\n\nInteresting. Before documenting and adding a test to cast the\ncurrent behaviour in stone, do we need to see if the discrepancy is\ndesired and designed one, or something we may want to fix?\n\n\n\n>\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> Documentation/config.txt | 4 ++++\n> t/t5504-fetch-receive-strict.sh | 14 ++++++++++++++\n> 2 files changed, 18 insertions(+)\n>\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 57c463c6e2..4cead6119a 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1637,6 +1637,10 @@ In general, it is better to enumerate existing objects with problems\n> with `fsck.skipList`, instead of listing the kind of breakages these\n> problematic objects share to be ignored, as doing the latter will\n> allow new instances of the same breakages go unnoticed.\n> ++\n> +Setting an unknown `fsck.` value will cause fsck to die, but\n> +doing the same for `receive.fsck.` and `fetch.fsck.`\n> +will only cause git to warn.\n> \n> fsck.skipList::\n> \tThe path to a sorted list of object names (i.e. one SHA-1 per\n> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\n> index 7f06b537d3..62f3569891 100755\n> --- a/t/t5504-fetch-receive-strict.sh\n> +++ b/t/t5504-fetch-receive-strict.sh\n> @@ -198,6 +198,10 @@ test_expect_success 'fetch with fetch.fsck.skipList' '\n> \tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec\n> '\n> \n> +test_expect_success 'fsck. dies' '\n> +\ttest_must_fail git -c fsck.whatEver=ignore fsck 2>err &&\n> +\ttest_i18ngrep \"Unhandled message id: whatever\" err\n> +'\n> \n> test_expect_success 'push with receive.fsck.missingEmail=warn' '\n> \tcommit=\"$(git hash-object -t commit -w --stdin @@ -211,10 +215,15 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '\n> \tgit --git-dir=dst/.git config fsck.missingEmail warn &&\n> \ttest_must_fail git push --porcelain dst bogus &&\n> \n> +\t# receive.fsck. warns\n> +\tgit --git-dir=dst/.git config \\\n> +\t\treceive.fsck.whatEver error &&\n> +\n> \tgit --git-dir=dst/.git config \\\n> \t\treceive.fsck.missingEmail warn &&\n> \tgit push --porcelain dst bogus >act 2>&1 &&\n> \tgrep \"missingEmail\" act &&\n> +\ttest_i18ngrep \"Skipping unknown msg id.*whatever\" act &&\n> \tgit --git-dir=dst/.git branch -D bogus &&\n> \tgit --git-dir=dst/.git config --add \\\n> \t\treceive.fsck.missingEmail ignore &&\n> @@ -235,10 +244,15 @@ test_expect_success 'fetch with fetch.fsck.missingEmail=warn' '\n> \tgit --git-dir=dst/.git config fsck.missingEmail warn &&\n> \ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n> \n> +\t# receive.fsck. warns\n> +\tgit --git-dir=dst/.git config \\\n> +\t\tfetch.fsck.whatEver error &&\n> +\n> \tgit --git-dir=dst/.git config \\\n> \t\tfetch.fsck.missingEmail warn &&\n> \tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec >act 2>&1 &&\n> \tgrep \"missingEmail\" act &&\n> +\ttest_i18ngrep \"Skipping unknown msg id.*whatever\" act &&\n> \trm -rf dst &&\n> \tgit init dst &&\n> \tgit --git-dir=dst/.git config fetch.fsckobjects true &&\n"},{"id":"353765","messageId":"xmqqy3dwjp8n.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"87d0v85uoy.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 01/10] receive.fsck. tests: remove dead code","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-27T22:19:36Z","receivedAt":"2018-07-27T22:19:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>>\n>> author Bugs Bunny 1234567890 +0000\n>> committer Bugs Bunny 1234567890 +0000\n>\n> This is covered by the \"missingEmail\" part of the test, but there's\n> nothing wrong with the timestamp itself.\n\nI think you didn't get what I meant. \n\nWhat makes you think \"1234567890 +0000\" on the first line is a\ntimestamp in the first place? The parser could be updated and split\nthat malformed string (as it lacks <...> thing we expect, which is\nwhat missing Email is about) differently from what you are\nexpecting, causing the line to be broken in two ways, i.e. missing\nemail and badly formatted timestamp.\n\nBut we know for certain that the line is wrong not to have email in\neither parser (the current one, or a possibly updated one). So it\nis defensive to doubt your preconception that there is nothing wrong\nwith the timestamp, and demote possible timestamp errors to warning,\nas that is not what we are interested in here.\n\n> I still think it makes sense to remove this particular thing. Let's add\n> exhaustive tests for all this fsck.* stuff in another series, but no\n> point in testing for arbitrary fsck errors that aren't going to be\n> triggered in unrelated tests.\n\nThat is exactly what I am saying with \"being defensive\". Your\nchange starts testing for arbitrary errors that are not relevant.\n"},{"id":"353799","messageId":"8736w3v51a.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":"xmqqbmasl5hq.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 10/10] fsck: test and document unknown fsck. values","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-28T13:55:13Z","receivedAt":"2018-07-28T13:55:18Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jul 27 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> When fsck. is set to an unknown value it'll cause \"fsck\" to\n>> die, but the same is not rue of the \"fetch\" and \"receive\"\n>> variants. Document this and test for it.\n>\n> Interesting. Before documenting and adding a test to cast the\n> current behaviour in stone, do we need to see if the discrepancy is\n> desired and designed one, or something we may want to fix?\n\nWe could change it. This is just something I ran into and figured it\nshould be tested / documented, and didn't feel any need to change it\nmyself.\n\nThe current behavior is probably more of an organically grown\naccident. Maybe we should make all of these warn.\n\nTrying to post-hoc rationalize these, it probably makes sense for\nreceive.* not to die, since you don't want pushes to fail if you're\ntweaking this on a server and typo it, same with fetch (although less\nso), whereas \"fsck\" tends to be more of an offline validation command.\n\nSo Yeah, I can change this, or not. What do you/others think?\n\n>\n>>\n>> Signed-off-by: Ævar Arnfjörð Bjarmason \n>> ---\n>> Documentation/config.txt | 4 ++++\n>> t/t5504-fetch-receive-strict.sh | 14 ++++++++++++++\n>> 2 files changed, 18 insertions(+)\n>>\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index 57c463c6e2..4cead6119a 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -1637,6 +1637,10 @@ In general, it is better to enumerate existing objects with problems\n>> with `fsck.skipList`, instead of listing the kind of breakages these\n>> problematic objects share to be ignored, as doing the latter will\n>> allow new instances of the same breakages go unnoticed.\n>> ++\n>> +Setting an unknown `fsck.` value will cause fsck to die, but\n>> +doing the same for `receive.fsck.` and `fetch.fsck.`\n>> +will only cause git to warn.\n>>\n>> fsck.skipList::\n>> \tThe path to a sorted list of object names (i.e. one SHA-1 per\n>> diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\n>> index 7f06b537d3..62f3569891 100755\n>> --- a/t/t5504-fetch-receive-strict.sh\n>> +++ b/t/t5504-fetch-receive-strict.sh\n>> @@ -198,6 +198,10 @@ test_expect_success 'fetch with fetch.fsck.skipList' '\n>> \tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec\n>> '\n>>\n>> +test_expect_success 'fsck. dies' '\n>> +\ttest_must_fail git -c fsck.whatEver=ignore fsck 2>err &&\n>> +\ttest_i18ngrep \"Unhandled message id: whatever\" err\n>> +'\n>>\n>> test_expect_success 'push with receive.fsck.missingEmail=warn' '\n>> \tcommit=\"$(git hash-object -t commit -w --stdin > @@ -211,10 +215,15 @@ test_expect_success 'push with receive.fsck.missingEmail=warn' '\n>> \tgit --git-dir=dst/.git config fsck.missingEmail warn &&\n>> \ttest_must_fail git push --porcelain dst bogus &&\n>>\n>> +\t# receive.fsck. warns\n>> +\tgit --git-dir=dst/.git config \\\n>> +\t\treceive.fsck.whatEver error &&\n>> +\n>> \tgit --git-dir=dst/.git config \\\n>> \t\treceive.fsck.missingEmail warn &&\n>> \tgit push --porcelain dst bogus >act 2>&1 &&\n>> \tgrep \"missingEmail\" act &&\n>> +\ttest_i18ngrep \"Skipping unknown msg id.*whatever\" act &&\n>> \tgit --git-dir=dst/.git branch -D bogus &&\n>> \tgit --git-dir=dst/.git config --add \\\n>> \t\treceive.fsck.missingEmail ignore &&\n>> @@ -235,10 +244,15 @@ test_expect_success 'fetch with fetch.fsck.missingEmail=warn' '\n>> \tgit --git-dir=dst/.git config fsck.missingEmail warn &&\n>> \ttest_must_fail git --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec &&\n>>\n>> +\t# receive.fsck. warns\n>> +\tgit --git-dir=dst/.git config \\\n>> +\t\tfetch.fsck.whatEver error &&\n>> +\n>> \tgit --git-dir=dst/.git config \\\n>> \t\tfetch.fsck.missingEmail warn &&\n>> \tgit --git-dir=dst/.git fetch \"file://$(pwd)\" $refspec >act 2>&1 &&\n>> \tgrep \"missingEmail\" act &&\n>> +\ttest_i18ngrep \"Skipping unknown msg id.*whatever\" act &&\n>> \trm -rf dst &&\n>> \tgit init dst &&\n>> \tgit --git-dir=dst/.git config fetch.fsckobjects true &&\n"},{"id":"353800","messageId":"871sbnv4d4.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":"xmqqeffomphy.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 05/10] config doc: elaborate on fetch.fsckObjects security","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-28T14:09:43Z","receivedAt":"2018-07-28T14:09:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Jul 27 2018, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> +For now, the paranoid need to find some way to emulate the quarantine\n>> +environment if they'd like the same protection as \"push\". E.g. in the\n>\n> We probably should mention that you can immediately prune, as these\n> unwanted crufts are unreferenced. That would probably be a lot easier\n> workaround for the intended readers of this document than \"find some\n> way to emulate\".\n\nI'll mention that as well in v6 that \"git prune\" will get rid of these\nobjects.\n\nFor what it's worth I was imagining something like a system where you're\nmirroring every push to some unpatched-git-host.com repo in-house, by\ndoing a local \"git fetch\" when you see new data, and you're paranoid\nthat someone's trying to introduce something like the .gitmodules\nsecurity issue to your local mirror, even if you have\ntransfer.fsckObjects set.\n\nIn a case like that, relying on \"git prune\" is much more fragile. You'd\nneed to implement your mirror as some loop that does (pseudocode):\n\n while ref = poll_new_refs()\n git fetch upstream\n git prune --expire=now\n\nAs opposed to:\n\n while ref = poll_new_refs()\n (git fetch upstream && git prune --expire=now) &\n\nAs you might find in some event-based system. I.e. every time you fetch\nyou need to stop the world and run a full prune, because the potentially\nmalicious upstream can craft a series of ref updates where one ref\nupdate (which you'll refuse) contains the bad data, but at that point\nyou have some of those blobs/trees/commits it in your object store, and\nthen a second ref update references that already existing data and\ncauses you to update the ref.\n\nIt's also much slower and I/O heavy, on an already-pruned linux.git\nrunning 'git prune --expire=now' takes 40 seconds on my machine, as\nopposed to:\n\n while ref = poll_new_refs()\n (git fetch && git push internal-mirror --mirror) &\n\nWhich could take as little time as a second for the whole operation, can\nsafely be run in parallel, and would be protected because the actually\npublished internal mirror gets its refs via receive-pack, which uses the\nquarantine.\n"},{"id":"353879","messageId":"xmqqd0v4kcfd.fsf@gitster-ct.c.googlers.com","threadId":"48565","inReplyTo":"8736w3v51a.fsf@evledraar.gmail.com","subject":"Re: [PATCH v3 10/10] fsck: test and document unknown fsck. values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-07-30T14:47:50Z","receivedAt":"2018-07-30T14:47:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>>> When fsck. is set to an unknown value it'll cause \"fsck\" to\n>>> die, but the same is not rue of the \"fetch\" and \"receive\"\n>>> variants. Document this and test for it.\n> ...\n> We could change it. This is just something I ran into and figured it\n> should be tested / documented, and didn't feel any need to change it\n> myself.\n>\n> The current behavior is probably more of an organically grown\n> accident. Maybe we should make all of these warn.\n\nOr die. I do agree with your assessment that the discrepancy was\nnot designed, and my inclination for a correctness-centered feature\nlike fsck is to err on the side of caution, rather than letting a\nmisconfiguration pass unintended kinds of breakages through.\n\nThanks.\n\n"},{"id":"353883","messageId":"20180730145851.GA8074@duynguyen.home","threadId":"48565","inReplyTo":"20180727143720.14948-8-avarab@gmail.com","subject":"Re: [PATCH v3 07/10] fetch: implement fetch.fsck.*","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-07-30T14:58:51Z","receivedAt":"2018-07-30T14:58:59Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jul 27, 2018 at 02:37:17PM +0000, Ævar Arnfjörð Bjarmason wrote:\n> diff --git a/Documentation/config.txt b/Documentation/config.txt\n> index 7ff453c53b..8dace49daa 100644\n> --- a/Documentation/config.txt\n> +++ b/Documentation/config.txt\n> @@ -1467,6 +1467,16 @@ fetch.fsckObjects::\n> \tchecked. Defaults to false. If not set, the value of\n> \t`transfer.fsckObjects` is used instead.\n> \n> +fetch.fsck.::\n> +\tActs like `fsck.`, but is used by\n> +\tlinkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See\n> +\tthe `fsck.` documentation for details.\n\nCould you squash this patch in? It would let us auto-complete \"git\nconfig fetch.fsck\" in bash/tcsh\n\n-- 8< --\ndiff --git a/help.c b/help.c\nindex 9c0b5a8de9..32fe466dbf 100644\n--- a/help.c\n+++ b/help.c\n@@ -427,6 +427,7 @@ void list_config_help(int for_human)\n \t\t{ \"color.interactive\", \"\", list_config_color_interactive_slots },\n \t\t{ \"color.status\", \"\", list_config_color_status_slots },\n \t\t{ \"fsck\", \"\", list_config_fsck_msg_ids },\n+\t\t{ \"fetch.fsck\", \"\", list_config_fsck_msg_ids },\n \t\t{ \"receive.fsck\", \"\", list_config_fsck_msg_ids },\n \t\t{ NULL, NULL, NULL }\n \t};\n-- 8< --\n--\nDuy\n"},{"id":"353884","messageId":"87va8wu5jn.fsf@evledraar.gmail.com","threadId":"48565","inReplyTo":"20180730145851.GA8074@duynguyen.home","subject":"Re: [PATCH v3 07/10] fetch: implement fetch.fsck.*","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-07-30T15:06:20Z","receivedAt":"2018-07-30T15:06:33Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Mon, Jul 30 2018, Duy Nguyen wrote:\n\n> On Fri, Jul 27, 2018 at 02:37:17PM +0000, Ævar Arnfjörð Bjarmason wrote:\n>> diff --git a/Documentation/config.txt b/Documentation/config.txt\n>> index 7ff453c53b..8dace49daa 100644\n>> --- a/Documentation/config.txt\n>> +++ b/Documentation/config.txt\n>> @@ -1467,6 +1467,16 @@ fetch.fsckObjects::\n>> \tchecked. Defaults to false. If not set, the value of\n>> \t`transfer.fsckObjects` is used instead.\n>> \n>> +fetch.fsck.::\n>> +\tActs like `fsck.`, but is used by\n>> +\tlinkgit:git-fetch-pack[1] instead of linkgit:git-fsck[1]. See\n>> +\tthe `fsck.` documentation for details.\n>\n> Could you squash this patch in? It would let us auto-complete \"git\n> config fetch.fsck\" in bash/tcsh\n>\n> -- 8< --\n> diff --git a/help.c b/help.c\n> index 9c0b5a8de9..32fe466dbf 100644\n> --- a/help.c\n> +++ b/help.c\n> @@ -427,6 +427,7 @@ void list_config_help(int for_human)\n> \t\t{ \"color.interactive\", \"\", list_config_color_interactive_slots },\n> \t\t{ \"color.status\", \"\", list_config_color_status_slots },\n> \t\t{ \"fsck\", \"\", list_config_fsck_msg_ids },\n> +\t\t{ \"fetch.fsck\", \"\", list_config_fsck_msg_ids },\n> \t\t{ \"receive.fsck\", \"\", list_config_fsck_msg_ids },\n> \t\t{ NULL, NULL, NULL }\n> \t};\n> -- 8< --\n\nThanks! I missed that. Willdo.\n"},{"id":"353969","messageId":"20180730221319.8991-1-szeder.dev@gmail.com","threadId":"48565","inReplyTo":"20180727143720.14948-1-avarab@gmail.com","subject":"Re: [PATCH v3 00/10] fsck: doc fixes & fetch.fsck.* implementation","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2018-07-30T22:13:19Z","receivedAt":"2018-07-30T22:13:45Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"\n't5504-fetch-receive-strict.sh', in particular the test 'push with\ntransfer.fsckobjects' failed on me while building this branch the\nother day, caused by 'test_cmp' failing, because 'git push' didn't\nprint anything to its standard input.\n\nI was unable to reproduce the failure with the usual means (running\nthe test repeatedly for a long time while the machine is under heavy\nload), and given that this test has a history of raciness[1] with the\nsame symptom, I'm not sure whether this patch series does something\nwrong, or perhaps the fixes[2] are incomplete. Or the Blood Moon.\n\n\nSidenote: I found some of the error messages from the expectedly\nfailing 'git fetch' and 'git push' commands misleading. Some of them\noutput \"fatal: object of unexpected type\", even though the original\nand the corrupted objects are both of the same type (blob).\n\n\n1 - 8bf4becf0c (add \"ok=sigpipe\" to test_must_fail and use it to fix\n flaky tests, 2015-11-27)\n2 - five commits leading up to c4b27511ab (t5504: drop sigpipe=ok from\n push tests, 2016-04-19)\n\n"}]}