{"thread":{"id":"53572","subject":"[PATCH] fast-import: accept invalid timezones so we can import existing repos","startedAt":"2020-05-28T19:15:18Z","lastAt":"2021-02-05T22:29:03Z","messageCount":12,"participants":["Elijah Newren via GitGitGadget","Jonathan Nieder","Junio C Hamano","Jeff King","=?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"398777","messageId":"pull.795.git.git.1590693313099.gitgitgadget@gmail.com","threadId":"53572","inReplyTo":null,"subject":"[PATCH] fast-import: accept invalid timezones so we can import existing repos","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-28T19:15:12Z","receivedAt":"2020-05-28T19:15:18Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThere are multiple repositories in the wild with random, invalid\ntimezones. Most notably is a commit from rails.git with a timezone of\n\"+051800\"[1]. However, a few searches found other repos with that same\ninvalid timezone. Further, Peff reports that GitHub relaxed their fsck\nchecks in August 2011 to accept any timezone value[2], and there have\nbeen multiple reports to filter-repo about fast-import crashing while\ntrying to import their existing repositories since they had timezone\nvalues such as \"-7349423\" and \"-43455309\"[3].\n\nIt is not clear what created these invalid timezones, but since git has\npermitted their use and worked with these repositories for years at this\npoint, it seems pointless to make fast-import be the only thing that\ndisallows them. Relax the parsing to allow these timezones when using\nraw import format; when using --date-format=rfc2822 (which is not the\ndefault), we can continue being more strict about timezones.\n\n[1] https://github.com/rails/rails/commit/4cf94979c9f4d6683c9338d694d5eb3106a4e734\n[2] https://lore.kernel.org/git/20200521195513.GA1542632@coredump.intra.peff.net/\n[3] https://github.com/newren/git-filter-repo/issues/88\n\nSigned-off-by: Elijah Newren \n---\n fast-import: accept invalid timezones so we can import existing repos\n \n Note: this is not a fix for a regression, so you can ignore it for 2.27;\n it can sit in pu.\n \n Peff leaned towards normalizing these timezones in fast-export[1], but\n (A) it's not clear what \"valid\" timezone we should randomly pick and\n regardless of what we pick, it seems it'll be wrong for most cases, (B)\n it would provide yet another way that \"git fast-export --all | git\n fast-import\" would not preserve the original history, as users sometimes\n expect[2], and (C) that'd prevent users from passing a special callback\n to filter-repo to fix up these values[3].\n \n Since I'm not a fan of picking a random value to reassign these to (in\n either fast-export or fast-import), I went the route of relaxing the\n requirements in fast-import, similar to what Peff reports GitHub did\n about 9 years ago in their incoming fsck checks.\n \n [1] \n https://lore.kernel.org/git/20200521195513.GA1542632@coredump.intra.peff.net/\n [2] \n https://lore.kernel.org/git/CABPp-BFLJ48BZ97Y9mr4i3q7HMqjq18cXMgSYdxqD1cMzH8Spg@mail.gmail.com/\n [3] Example: \n https://github.com/newren/git-filter-repo/issues/88#issuecomment-629706776\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-795%2Fnewren%2Floosen-fast-import-timezone-parsing-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-795/newren/loosen-fast-import-timezone-parsing-v1\nPull-Request: https://github.com/git/git/pull/795\n\n fast-import.c | 7 +++----\n t/t9300-fast-import.sh | 17 +++++++++++++++++\n 2 files changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex c98970274c4..4a3c193007d 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1915,11 +1915,10 @@ static int validate_raw_date(const char *src, struct strbuf *result)\n {\n \tconst char *orig_src = src;\n \tchar *endp;\n-\tunsigned long num;\n \n \terrno = 0;\n \n-\tnum = strtoul(src, &endp, 10);\n+\tstrtoul(src, &endp, 10);\n \t/* NEEDSWORK: perhaps check for reasonable values? */\n \tif (errno || endp == src || *endp != ' ')\n \t\treturn -1;\n@@ -1928,8 +1927,8 @@ static int validate_raw_date(const char *src, struct strbuf *result)\n \tif (*src != '-' && *src != '+')\n \t\treturn -1;\n \n-\tnum = strtoul(src + 1, &endp, 10);\n-\tif (errno || endp == src + 1 || *endp || 1400 < num)\n+\tstrtoul(src + 1, &endp, 10);\n+\tif (errno || endp == src + 1 || *endp)\n \t\treturn -1;\n \n \tstrbuf_addstr(result, orig_src);\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 768257b29e0..0e798e68476 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -410,6 +410,23 @@ test_expect_success 'B: accept empty committer' '\n \ttest -z \"$out\"\n '\n \n+test_expect_success 'B: accept invalid timezone' '\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/invalid-timezone\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800\n+\tdata <out &&\n+\tgrep \"1234567890 [+]051800\" out\n+'\n+\n test_expect_success 'B: accept and fixup committer with no name' '\n \tcat >input <<-INPUT_END &&\n \tcommit refs/heads/empty-committer-2\n\nbase-commit: 2d5e9f31ac46017895ce6a183467037d29ceb9d3\n-- \ngitgitgadget\n"},{"id":"398780","messageId":"20200528192624.GB114915@google.com","threadId":"53572","inReplyTo":"pull.795.git.git.1590693313099.gitgitgadget@gmail.com","subject":"Re: [PATCH] fast-import: accept invalid timezones so we can import existing repos","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-05-28T19:26:24Z","receivedAt":"2020-05-28T19:26:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nElijah Newren wrote:\n\n> Relax the parsing to allow these timezones when using\n> raw import format; when using --date-format=rfc2822 (which is not the\n> default), we can continue being more strict about timezones.\n\nThere are two different use cases here that we may want to distinguish.\n\nThe original motivation for fast-import was for importing a repository\nfrom some non-Git storage system (another VCS, a collection of patches\nand tarballs, or whatever). Such an importer might use\n--date-format=raw just because that's simple, to avoid overhead. In\nthat case, the strict parsing seems useful for catching bugs in the\nimporter.\n\n\"git filter-repo\" is for taking an existing repository and modifying\nit. In this case, it would be *possible* to take an invalid timezone\nand normalize it to \"whatever 'git log' would show\", but that's\noverreaching relative to the caller's intent: the caller has a specific\nset of history modifications they meant to make, and fixing the time\nzone wasn't necessarily one of them.\n\nTo that end, would it make sense for this to be a new date-format\n(e.g., --date-format=raw-permissive) to avoid regressing behavior in\nthe original case?\n\nThanks and hope that helps,\nJonathan\n"},{"id":"398799","messageId":"pull.795.v2.git.git.1590698437607.gitgitgadget@gmail.com","threadId":"53572","inReplyTo":"pull.795.git.git.1590693313099.gitgitgadget@gmail.com","subject":"[PATCH v2] fast-import: add new --date-format=raw-permissive format","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-28T20:40:37Z","receivedAt":"2020-05-28T20:40:45Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThere are multiple repositories in the wild with random, invalid\ntimezones. Most notably is a commit from rails.git with a timezone of\n\"+051800\"[1]. A few searches will find other repos with that same\ninvalid timezone as well. Further, Peff reports that GitHub relaxed\ntheir fsck checks in August 2011 to accept any timezone value[2], and\nthere have been multiple reports to filter-repo about fast-import\ncrashing while trying to import their existing repositories since they\nhad timezone values such as \"-7349423\" and \"-43455309\"[3].\n\nThe existing check on timezone values inside fast-import may prove\nuseful for people who are crafting fast-import input by hand or with a\nnew script. For them, the check may help them avoid accidentally\nrecording invalid dates. (Note that this check is rather simplistic and\nthere are still several forms of invalid dates that fast-import does not\ncheck for: dates in the future, timezone values with minutes that are\nnot divisible by 15, and timezone values with minutes that are 60 or\ngreater.) While this simple check may have some value for those users,\nother users or tools will want to import existing repositories as-is.\nProvide a --date-format=raw-permissive format that will not error out on\nthese otherwise invalid timezones so that such existing repositories can\nbe imported.\n\n[1] https://github.com/rails/rails/commit/4cf94979c9f4d6683c9338d694d5eb3106a4e734\n[2] https://lore.kernel.org/git/20200521195513.GA1542632@coredump.intra.peff.net/\n[3] https://github.com/newren/git-filter-repo/issues/88\n\nSigned-off-by: Elijah Newren \n---\n fast-import: accept invalid timezones so we can import existing repos\n \n Changes since v1:\n \n * Instead of just allowing such timezones outright, did it behind a\n --date-format=raw-permissive as suggested by Jonathan\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-795%2Fnewren%2Floosen-fast-import-timezone-parsing-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-795/newren/loosen-fast-import-timezone-parsing-v2\nPull-Request: https://github.com/git/git/pull/795\n\nRange-diff vs v1:\n\n 1: d3a7dbc3892 ! 1: 9580aacdb21 fast-import: accept invalid timezones so we can import existing repos\n @@ Metadata\n Author: Elijah Newren \n \n ## Commit message ##\n - fast-import: accept invalid timezones so we can import existing repos\n + fast-import: add new --date-format=raw-permissive format\n \n There are multiple repositories in the wild with random, invalid\n timezones. Most notably is a commit from rails.git with a timezone of\n - \"+051800\"[1]. However, a few searches found other repos with that same\n - invalid timezone. Further, Peff reports that GitHub relaxed their fsck\n - checks in August 2011 to accept any timezone value[2], and there have\n - been multiple reports to filter-repo about fast-import crashing while\n - trying to import their existing repositories since they had timezone\n - values such as \"-7349423\" and \"-43455309\"[3].\n + \"+051800\"[1]. A few searches will find other repos with that same\n + invalid timezone as well. Further, Peff reports that GitHub relaxed\n + their fsck checks in August 2011 to accept any timezone value[2], and\n + there have been multiple reports to filter-repo about fast-import\n + crashing while trying to import their existing repositories since they\n + had timezone values such as \"-7349423\" and \"-43455309\"[3].\n \n - It is not clear what created these invalid timezones, but since git has\n - permitted their use and worked with these repositories for years at this\n - point, it seems pointless to make fast-import be the only thing that\n - disallows them. Relax the parsing to allow these timezones when using\n - raw import format; when using --date-format=rfc2822 (which is not the\n - default), we can continue being more strict about timezones.\n + The existing check on timezone values inside fast-import may prove\n + useful for people who are crafting fast-import input by hand or with a\n + new script. For them, the check may help them avoid accidentally\n + recording invalid dates. (Note that this check is rather simplistic and\n + there are still several forms of invalid dates that fast-import does not\n + check for: dates in the future, timezone values with minutes that are\n + not divisible by 15, and timezone values with minutes that are 60 or\n + greater.) While this simple check may have some value for those users,\n + other users or tools will want to import existing repositories as-is.\n + Provide a --date-format=raw-permissive format that will not error out on\n + these otherwise invalid timezones so that such existing repositories can\n + be imported.\n \n [1] https://github.com/rails/rails/commit/4cf94979c9f4d6683c9338d694d5eb3106a4e734\n [2] https://lore.kernel.org/git/20200521195513.GA1542632@coredump.intra.peff.net/\n @@ Commit message\n Signed-off-by: Elijah Newren \n \n ## fast-import.c ##\n -@@ fast-import.c: static int validate_raw_date(const char *src, struct strbuf *result)\n +@@ fast-import.c: struct hash_list {\n + \n + typedef enum {\n + \tWHENSPEC_RAW = 1,\n ++\tWHENSPEC_RAW_PERMISSIVE,\n + \tWHENSPEC_RFC2822,\n + \tWHENSPEC_NOW\n + } whenspec_type;\n +@@ fast-import.c: static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)\n + \treturn 1;\n + }\n + \n +-static int validate_raw_date(const char *src, struct strbuf *result)\n ++static int validate_raw_date(const char *src, struct strbuf *result, int strict)\n {\n \tconst char *orig_src = src;\n \tchar *endp;\n --\tunsigned long num;\n + \tunsigned long num;\n ++\tint out_of_range_timezone;\n \n \terrno = 0;\n \n --\tnum = strtoul(src, &endp, 10);\n -+\tstrtoul(src, &endp, 10);\n - \t/* NEEDSWORK: perhaps check for reasonable values? */\n - \tif (errno || endp == src || *endp != ' ')\n - \t\treturn -1;\n @@ fast-import.c: static int validate_raw_date(const char *src, struct strbuf *result)\n - \tif (*src != '-' && *src != '+')\n \t\treturn -1;\n \n --\tnum = strtoul(src + 1, &endp, 10);\n + \tnum = strtoul(src + 1, &endp, 10);\n -\tif (errno || endp == src + 1 || *endp || 1400 < num)\n -+\tstrtoul(src + 1, &endp, 10);\n -+\tif (errno || endp == src + 1 || *endp)\n ++\tout_of_range_timezone = strict && (1400 < num);\n ++\tif (errno || endp == src + 1 || *endp || out_of_range_timezone)\n \t\treturn -1;\n \n \tstrbuf_addstr(result, orig_src);\n +@@ fast-import.c: static char *parse_ident(const char *buf)\n + \n + \tswitch (whenspec) {\n + \tcase WHENSPEC_RAW:\n +-\t\tif (validate_raw_date(ltgt, &ident) < 0)\n ++\t\tif (validate_raw_date(ltgt, &ident, 1) < 0)\n ++\t\t\tdie(\"Invalid raw date \\\"%s\\\" in ident: %s\", ltgt, buf);\n ++\t\tbreak;\n ++\tcase WHENSPEC_RAW_PERMISSIVE:\n ++\t\tif (validate_raw_date(ltgt, &ident, 0) < 0)\n + \t\t\tdie(\"Invalid raw date \\\"%s\\\" in ident: %s\", ltgt, buf);\n + \t\tbreak;\n + \tcase WHENSPEC_RFC2822:\n +@@ fast-import.c: static void option_date_format(const char *fmt)\n + {\n + \tif (!strcmp(fmt, \"raw\"))\n + \t\twhenspec = WHENSPEC_RAW;\n ++\telse if (!strcmp(fmt, \"raw-permissive\"))\n ++\t\twhenspec = WHENSPEC_RAW_PERMISSIVE;\n + \telse if (!strcmp(fmt, \"rfc2822\"))\n + \t\twhenspec = WHENSPEC_RFC2822;\n + \telse if (!strcmp(fmt, \"now\"))\n \n ## t/t9300-fast-import.sh ##\n @@ t/t9300-fast-import.sh: test_expect_success 'B: accept empty committer' '\n \ttest -z \"$out\"\n '\n \n -+test_expect_success 'B: accept invalid timezone' '\n ++test_expect_success 'B: reject invalid timezone' '\n ++\tcat >input <<-INPUT_END &&\n ++\tcommit refs/heads/invalid-timezone\n ++\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800\n ++\tdata <input <<-INPUT_END &&\n +\tcommit refs/heads/invalid-timezone\n +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800\n @@ t/t9300-fast-import.sh: test_expect_success 'B: accept empty committer' '\n +\ttest_when_finished \"git update-ref -d refs/heads/invalid-timezone\n +\t\tgit gc\n +\t\tgit prune\" &&\n -+\tgit fast-import out &&\n +\tgrep \"1234567890 [+]051800\" out\n +'\n\n\n fast-import.c | 15 ++++++++++++---\n t/t9300-fast-import.sh | 30 ++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 3 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex c98970274c4..74d7298bc5a 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -139,6 +139,7 @@ struct hash_list {\n \n typedef enum {\n \tWHENSPEC_RAW = 1,\n+\tWHENSPEC_RAW_PERMISSIVE,\n \tWHENSPEC_RFC2822,\n \tWHENSPEC_NOW\n } whenspec_type;\n@@ -1911,11 +1912,12 @@ static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)\n \treturn 1;\n }\n \n-static int validate_raw_date(const char *src, struct strbuf *result)\n+static int validate_raw_date(const char *src, struct strbuf *result, int strict)\n {\n \tconst char *orig_src = src;\n \tchar *endp;\n \tunsigned long num;\n+\tint out_of_range_timezone;\n \n \terrno = 0;\n \n@@ -1929,7 +1931,8 @@ static int validate_raw_date(const char *src, struct strbuf *result)\n \t\treturn -1;\n \n \tnum = strtoul(src + 1, &endp, 10);\n-\tif (errno || endp == src + 1 || *endp || 1400 < num)\n+\tout_of_range_timezone = strict && (1400 < num);\n+\tif (errno || endp == src + 1 || *endp || out_of_range_timezone)\n \t\treturn -1;\n \n \tstrbuf_addstr(result, orig_src);\n@@ -1963,7 +1966,11 @@ static char *parse_ident(const char *buf)\n \n \tswitch (whenspec) {\n \tcase WHENSPEC_RAW:\n-\t\tif (validate_raw_date(ltgt, &ident) < 0)\n+\t\tif (validate_raw_date(ltgt, &ident, 1) < 0)\n+\t\t\tdie(\"Invalid raw date \\\"%s\\\" in ident: %s\", ltgt, buf);\n+\t\tbreak;\n+\tcase WHENSPEC_RAW_PERMISSIVE:\n+\t\tif (validate_raw_date(ltgt, &ident, 0) < 0)\n \t\t\tdie(\"Invalid raw date \\\"%s\\\" in ident: %s\", ltgt, buf);\n \t\tbreak;\n \tcase WHENSPEC_RFC2822:\n@@ -3258,6 +3265,8 @@ static void option_date_format(const char *fmt)\n {\n \tif (!strcmp(fmt, \"raw\"))\n \t\twhenspec = WHENSPEC_RAW;\n+\telse if (!strcmp(fmt, \"raw-permissive\"))\n+\t\twhenspec = WHENSPEC_RAW_PERMISSIVE;\n \telse if (!strcmp(fmt, \"rfc2822\"))\n \t\twhenspec = WHENSPEC_RFC2822;\n \telse if (!strcmp(fmt, \"now\"))\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 768257b29e0..14e9baa22db 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -410,6 +410,36 @@ test_expect_success 'B: accept empty committer' '\n \ttest -z \"$out\"\n '\n \n+test_expect_success 'B: reject invalid timezone' '\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/invalid-timezone\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800\n+\tdata <input <<-INPUT_END &&\n+\tcommit refs/heads/invalid-timezone\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800\n+\tdata <out &&\n+\tgrep \"1234567890 [+]051800\" out\n+'\n+\n test_expect_success 'B: accept and fixup committer with no name' '\n \tcat >input <<-INPUT_END &&\n \tcommit refs/heads/empty-committer-2\n\nbase-commit: 2d5e9f31ac46017895ce6a183467037d29ceb9d3\n-- \ngitgitgadget\n"},{"id":"398812","messageId":"xmqqpnan1wue.fsf@gitster.c.googlers.com","threadId":"53572","inReplyTo":"pull.795.v2.git.git.1590698437607.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fast-import: add new --date-format=raw-permissive format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-28T23:08:41Z","receivedAt":"2020-05-28T23:08:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Elijah Newren via GitGitGadget\" writes:\n\n> Provide a --date-format=raw-permissive format that will not error out on\n> these otherwise invalid timezones so that such existing repositories can\n> be imported.\n\nSo the idea is to just propagate whatever raw timestamp plus\ntimezone we read from the input stream, which most likely has been\ncopied from a (broken) raw commit object, to a rewritten commit?\n\nMakes sense. Will queue.\n\nThanks.\n"},{"id":"398820","messageId":"20200529002034.GC114915@google.com","threadId":"53572","inReplyTo":"pull.795.v2.git.git.1590698437607.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fast-import: add new --date-format=raw-permissive format","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2020-05-29T00:20:34Z","receivedAt":"2020-05-29T00:20:40Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Elijah Newren wrote:\n\n> From: Elijah Newren \n>\n> There are multiple repositories in the wild with random, invalid\n> timezones. Most notably is a commit from rails.git with a timezone of\n> \"+051800\"[1]. A few searches will find other repos with that same\n[...]\n> Provide a --date-format=raw-permissive format that will not error out on\n> these otherwise invalid timezones so that such existing repositories can\n> be imported.\n>\n> [1] https://github.com/rails/rails/commit/4cf94979c9f4d6683c9338d694d5eb3106a4e734\n> [2] https://lore.kernel.org/git/20200521195513.GA1542632@coredump.intra.peff.net/\n> [3] https://github.com/newren/git-filter-repo/issues/88\n\nThis could potentially go in a Bug: footer.\n\n> Signed-off-by: Elijah Newren \n> ---\n> fast-import.c | 15 ++++++++++++---\n> t/t9300-fast-import.sh | 30 ++++++++++++++++++++++++++++++\n> 2 files changed, 42 insertions(+), 3 deletions(-)\n\nMakes sense.\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"398835","messageId":"20200529061346.GB1294228@coredump.intra.peff.net","threadId":"53572","inReplyTo":"pull.795.v2.git.git.1590698437607.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] fast-import: add new --date-format=raw-permissive format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-29T06:13:46Z","receivedAt":"2020-05-29T06:13:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 28, 2020 at 08:40:37PM +0000, Elijah Newren via GitGitGadget wrote:\n\n> * Instead of just allowing such timezones outright, did it behind a\n> --date-format=raw-permissive as suggested by Jonathan\n\nThanks, I like the safety this gives us against importer bugs. It does\nmean that people doing \"export | filter | import\" may have to manually\nloosen it, but it should be rare enough that this isn't a big burden\n(and if they're rewriting anyway, maybe it gives them a chance to decide\nhow to fix it up).\n\n> fast-import.c | 15 ++++++++++++---\n> t/t9300-fast-import.sh | 30 ++++++++++++++++++++++++++++++\n\nWould we need a documentation update for \"raw-permissive\", too?\n\n> @@ -1929,7 +1931,8 @@ static int validate_raw_date(const char *src, struct strbuf *result)\n> \t\treturn -1;\n> \n> \tnum = strtoul(src + 1, &endp, 10);\n> -\tif (errno || endp == src + 1 || *endp || 1400 < num)\n> +\tout_of_range_timezone = strict && (1400 < num);\n> +\tif (errno || endp == src + 1 || *endp || out_of_range_timezone)\n> \t\treturn -1;\n\nIt's a little funny to do computations on the result of a function\nbefore we've checked whether it produced an error. But since the result\nis just an integer, I don't think we'd do anything unexpected (we might\njust throw away the value, though I imagine an optimizing compiler might\nevaluate it lazily anyway).\n\n> @@ -1963,7 +1966,11 @@ static char *parse_ident(const char *buf)\n> \n> \tswitch (whenspec) {\n> \tcase WHENSPEC_RAW:\n> -\t\tif (validate_raw_date(ltgt, &ident) < 0)\n> +\t\tif (validate_raw_date(ltgt, &ident, 1) < 0)\n> +\t\t\tdie(\"Invalid raw date \\\"%s\\\" in ident: %s\", ltgt, buf);\n> +\t\tbreak;\n> +\tcase WHENSPEC_RAW_PERMISSIVE:\n> +\t\tif (validate_raw_date(ltgt, &ident, 0) < 0)\n> \t\t\tdie(\"Invalid raw date \\\"%s\\\" in ident: %s\", ltgt, buf);\n> \t\tbreak;\n\nThis looks simple enough. We have the bogus date in a buffer at that\npoint, and we stuff that string literally into the commit (or tag)\nobject. If we ever get more clever about storing the timestamp\ninternally as an integer, we may need to get more clever. But your new\ntest should alert us if that becomes the case.\n\n> +test_expect_success 'B: accept invalid timezone with raw-permissive' '\n> +\tcat >input <<-INPUT_END &&\n> +\tcommit refs/heads/invalid-timezone\n> +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800\n> +\tdata < +\tempty commit\n> +\tCOMMIT\n> +\tINPUT_END\n> +\n> +\ttest_when_finished \"git update-ref -d refs/heads/invalid-timezone\n> +\t\tgit gc\n> +\t\tgit prune\" &&\n\nWe check the exit code of when_finished commands, so this should be\n&&-chained as usual. And possibly indented like:\n\n test_when_finished \"\n git update-ref -d refs/heads/invalid-timezone &&\n git gc &&\n git prune\n \" &&\n\nbut I see this all is copying another nearby test. So I'm OK with\nkeeping it consistent for now and cleaning up the whole thing later.\nThough if you want to do that step now, I have no objection. :)\n\nI also also suspect this \"gc\" is not useful these days. For a small\ninput like this, fast-import will write loose objects, since d9545c7f46\n(fast-import: implement unpack limit, 2016-04-25).\n\nTBH, I think it would be easier to understand as:\n\n ...\n git init invalid-timezone &&\n git -C invalid-timezone fast-import out &&\n ...\n\nand don't bother with the when_finished at all. Then you don't have to\nwonder whether the cleanup was sufficient, and it's fewer processes\nto boot (we'll leave the sub-repo cruft sitting around, but a single \"rm\n-rf\" at the end of the test script will wipe them all out).\n\n-Peff\n"},{"id":"398892","messageId":"xmqqwo4uzmj7.fsf@gitster.c.googlers.com","threadId":"53572","inReplyTo":"20200529061346.GB1294228@coredump.intra.peff.net","subject":"Re: [PATCH v2] fast-import: add new --date-format=raw-permissive format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-05-29T17:19:40Z","receivedAt":"2020-05-29T17:19:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Thu, May 28, 2020 at 08:40:37PM +0000, Elijah Newren via GitGitGadget wrote:\n>\n>> * Instead of just allowing such timezones outright, did it behind a\n>> --date-format=raw-permissive as suggested by Jonathan\n>\n> Thanks, I like the safety this gives us against importer bugs. It does\n> mean that people doing \"export | filter | import\" may have to manually\n> loosen it, but it should be rare enough that this isn't a big burden\n> (and if they're rewriting anyway, maybe it gives them a chance to decide\n> how to fix it up).\n>\n>> fast-import.c | 15 ++++++++++++---\n>> t/t9300-fast-import.sh | 30 ++++++++++++++++++++++++++++++\n>\n> Would we need a documentation update for \"raw-permissive\", too?\n\nGood eyes. I somehow thought this was an internal option but it\ndoes need to be known by end-users who use the fast-import tool.\n\n>> @@ -1929,7 +1931,8 @@ static int validate_raw_date(const char *src, struct strbuf *result)\n>> \t\treturn -1;\n>> \n>> \tnum = strtoul(src + 1, &endp, 10);\n>> -\tif (errno || endp == src + 1 || *endp || 1400 < num)\n>> +\tout_of_range_timezone = strict && (1400 < num);\n>> +\tif (errno || endp == src + 1 || *endp || out_of_range_timezone)\n>> \t\treturn -1;\n>\n> It's a little funny to do computations on the result of a function\n> before we've checked whether it produced an error. But since the result\n> is just an integer, I don't think we'd do anything unexpected (we might\n> just throw away the value, though I imagine an optimizing compiler might\n> evaluate it lazily anyway).\n\nTrue, but if it is easy to make it kosher, we should.\n\n\tif (errno || endp == src + 1 || *endp || /* did not parse */\n\t (strict && (1400 < num)) /* parsed a broken timezone */\n\t )\n\nperhaps?\n\n>> +test_expect_success 'B: accept invalid timezone with raw-permissive' '\n>> +\tcat >input <<-INPUT_END &&\n>> +\tcommit refs/heads/invalid-timezone\n>> +\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800\n>> +\tdata <> +\tempty commit\n>> +\tCOMMIT\n>> +\tINPUT_END\n>> +\n>> +\ttest_when_finished \"git update-ref -d refs/heads/invalid-timezone\n>> +\t\tgit gc\n>> +\t\tgit prune\" &&\n>\n> We check the exit code of when_finished commands, so this should be\n> &&-chained as usual. And possibly indented like:\n>\n> test_when_finished \"\n> git update-ref -d refs/heads/invalid-timezone &&\n> git gc &&\n> git prune\n> \" &&\n>\n> but I see this all is copying another nearby test. So I'm OK with\n> keeping it consistent for now and cleaning up the whole thing later.\n> Though if you want to do that step now, I have no objection. :)\n\nSure. Another alternative is to make it right/modern for only this\nadded test---that makes the inconsistency stand out and gives\nincentive to others for cleaning up after the dust settles when the\npatch gets merged to the 'master' branch.\n\n> I also also suspect this \"gc\" is not useful these days. For a small\n> input like this, fast-import will write loose objects, since d9545c7f46\n> (fast-import: implement unpack limit, 2016-04-25).\n>\n> TBH, I think it would be easier to understand as:\n>\n> ...\n> git init invalid-timezone &&\n> git -C invalid-timezone fast-import git -C invalid-timezone cat-file -p master >out &&\n> ...\n>\n> and don't bother with the when_finished at all. Then you don't have to\n> wonder whether the cleanup was sufficient, and it's fewer processes\n> to boot (we'll leave the sub-repo cruft sitting around, but a single \"rm\n> -rf\" at the end of the test script will wipe them all out).\n\nNice.\n\n"},{"id":"398946","messageId":"pull.795.v3.git.git.1590870357549.gitgitgadget@gmail.com","threadId":"53572","inReplyTo":"pull.795.v2.git.git.1590698437607.gitgitgadget@gmail.com","subject":"[PATCH v3] fast-import: add new --date-format=raw-permissive format","fromName":"Elijah Newren via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-05-30T20:25:57Z","receivedAt":"2020-05-30T20:26:04Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"From: Elijah Newren \n\nThere are multiple repositories in the wild with random, invalid\ntimezones. Most notably is a commit from rails.git with a timezone of\n\"+051800\"[1]. A few searches will find other repos with that same\ninvalid timezone as well. Further, Peff reports that GitHub relaxed\ntheir fsck checks in August 2011 to accept any timezone value[2], and\nthere have been multiple reports to filter-repo about fast-import\ncrashing while trying to import their existing repositories since they\nhad timezone values such as \"-7349423\" and \"-43455309\"[3].\n\nThe existing check on timezone values inside fast-import may prove\nuseful for people who are crafting fast-import input by hand or with a\nnew script. For them, the check may help them avoid accidentally\nrecording invalid dates. (Note that this check is rather simplistic and\nthere are still several forms of invalid dates that fast-import does not\ncheck for: dates in the future, timezone values with minutes that are\nnot divisible by 15, and timezone values with minutes that are 60 or\ngreater.) While this simple check may have some value for those users,\nother users or tools will want to import existing repositories as-is.\nProvide a --date-format=raw-permissive format that will not error out on\nthese otherwise invalid timezones so that such existing repositories can\nbe imported.\n\n[1] https://github.com/rails/rails/commit/4cf94979c9f4d6683c9338d694d5eb3106a4e734\n[2] https://lore.kernel.org/git/20200521195513.GA1542632@coredump.intra.peff.net/\n[3] https://github.com/newren/git-filter-repo/issues/88\n\nSigned-off-by: Elijah Newren \n---\n fast-import: accept invalid timezones so we can import existing repos\n \n Changes since v2:\n \n * Add documentation\n * Note the fact that the \"strict\" method really isn't all that strict\n with some NEEDSWORK comments\n * Check for parsed as unsigned before checking that value range makes\n sense\n * Simplify the testcase as suggested by Peff, leaving it to stick out a\n bit like a sore thumb from the rest of the tests in the same file\n (#leftoverbits)\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-795%2Fnewren%2Floosen-fast-import-timezone-parsing-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-795/newren/loosen-fast-import-timezone-parsing-v3\nPull-Request: https://github.com/git/git/pull/795\n\nRange-diff vs v2:\n\n 1: 9580aacdb21 ! 1: 48326d16dbd fast-import: add new --date-format=raw-permissive format\n @@ Commit message\n \n Signed-off-by: Elijah Newren \n \n + ## Documentation/git-fast-import.txt ##\n +@@ Documentation/git-fast-import.txt: by users who are located in the same location and time zone. In this\n + case a reasonable offset from UTC could be assumed.\n + +\n + Unlike the `rfc2822` format, this format is very strict. Any\n +-variation in formatting will cause fast-import to reject the value.\n ++variation in formatting will cause fast-import to reject the value,\n ++and some sanity checks on the numeric values may also be performed.\n ++\n ++`raw-permissive`::\n ++\tThis is the same as `raw` except that no sanity checks on\n ++\tthe numeric epoch and local offset are performed. This can\n ++\tbe useful when trying to filter or import an existing history\n ++\twith e.g. bogus timezone values.\n + \n + `rfc2822`::\n + \tThis is the standard email format as described by RFC 2822.\n +\n ## fast-import.c ##\n @@ fast-import.c: struct hash_list {\n \n @@ fast-import.c: static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax\n {\n \tconst char *orig_src = src;\n \tchar *endp;\n - \tunsigned long num;\n -+\tint out_of_range_timezone;\n - \n +@@ fast-import.c: static int validate_raw_date(const char *src, struct strbuf *result)\n \terrno = 0;\n \n + \tnum = strtoul(src, &endp, 10);\n +-\t/* NEEDSWORK: perhaps check for reasonable values? */\n ++\t/*\n ++\t * NEEDSWORK: perhaps check for reasonable values? For example, we\n ++\t * could error on values representing times more than a\n ++\t * day in the future.\n ++\t */\n + \tif (errno || endp == src || *endp != ' ')\n + \t\treturn -1;\n + \n @@ fast-import.c: static int validate_raw_date(const char *src, struct strbuf *result)\n \t\treturn -1;\n \n \tnum = strtoul(src + 1, &endp, 10);\n -\tif (errno || endp == src + 1 || *endp || 1400 < num)\n -+\tout_of_range_timezone = strict && (1400 < num);\n -+\tif (errno || endp == src + 1 || *endp || out_of_range_timezone)\n ++\t/*\n ++\t * NEEDSWORK: check for brokenness other than num > 1400, such as\n ++\t * (num % 100) >= 60, or ((num % 100) % 15) != 0 ?\n ++\t */\n ++\tif (errno || endp == src + 1 || *endp || /* did not parse */\n ++\t (strict && (1400 < num)) /* parsed a broken timezone */\n ++\t )\n \t\treturn -1;\n \n \tstrbuf_addstr(result, orig_src);\n @@ t/t9300-fast-import.sh: test_expect_success 'B: accept empty committer' '\n +\tCOMMIT\n +\tINPUT_END\n +\n -+\ttest_when_finished \"git update-ref -d refs/heads/invalid-timezone\n -+\t\tgit gc\n -+\t\tgit prune\" &&\n -+\tgit fast-import --date-format=raw-permissive out &&\n ++\tgit init invalid-timezone &&\n ++\tgit -C invalid-timezone fast-import --date-format=raw-permissive out &&\n +\tgrep \"1234567890 [+]051800\" out\n +'\n +\n\n\n Documentation/git-fast-import.txt | 9 ++++++++-\n fast-import.c | 25 +++++++++++++++++++++----\n t/t9300-fast-import.sh | 28 ++++++++++++++++++++++++++++\n 3 files changed, 57 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 77c6b3d0019..7d9aad2a7e1 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -293,7 +293,14 @@ by users who are located in the same location and time zone. In this\n case a reasonable offset from UTC could be assumed.\n +\n Unlike the `rfc2822` format, this format is very strict. Any\n-variation in formatting will cause fast-import to reject the value.\n+variation in formatting will cause fast-import to reject the value,\n+and some sanity checks on the numeric values may also be performed.\n+\n+`raw-permissive`::\n+\tThis is the same as `raw` except that no sanity checks on\n+\tthe numeric epoch and local offset are performed. This can\n+\tbe useful when trying to filter or import an existing history\n+\twith e.g. bogus timezone values.\n \n `rfc2822`::\n \tThis is the standard email format as described by RFC 2822.\ndiff --git a/fast-import.c b/fast-import.c\nindex c98970274c4..0dfa14dc8c3 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -139,6 +139,7 @@ struct hash_list {\n \n typedef enum {\n \tWHENSPEC_RAW = 1,\n+\tWHENSPEC_RAW_PERMISSIVE,\n \tWHENSPEC_RFC2822,\n \tWHENSPEC_NOW\n } whenspec_type;\n@@ -1911,7 +1912,7 @@ static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)\n \treturn 1;\n }\n \n-static int validate_raw_date(const char *src, struct strbuf *result)\n+static int validate_raw_date(const char *src, struct strbuf *result, int strict)\n {\n \tconst char *orig_src = src;\n \tchar *endp;\n@@ -1920,7 +1921,11 @@ static int validate_raw_date(const char *src, struct strbuf *result)\n \terrno = 0;\n \n \tnum = strtoul(src, &endp, 10);\n-\t/* NEEDSWORK: perhaps check for reasonable values? */\n+\t/*\n+\t * NEEDSWORK: perhaps check for reasonable values? For example, we\n+\t * could error on values representing times more than a\n+\t * day in the future.\n+\t */\n \tif (errno || endp == src || *endp != ' ')\n \t\treturn -1;\n \n@@ -1929,7 +1934,13 @@ static int validate_raw_date(const char *src, struct strbuf *result)\n \t\treturn -1;\n \n \tnum = strtoul(src + 1, &endp, 10);\n-\tif (errno || endp == src + 1 || *endp || 1400 < num)\n+\t/*\n+\t * NEEDSWORK: check for brokenness other than num > 1400, such as\n+\t * (num % 100) >= 60, or ((num % 100) % 15) != 0 ?\n+\t */\n+\tif (errno || endp == src + 1 || *endp || /* did not parse */\n+\t (strict && (1400 < num)) /* parsed a broken timezone */\n+\t )\n \t\treturn -1;\n \n \tstrbuf_addstr(result, orig_src);\n@@ -1963,7 +1974,11 @@ static char *parse_ident(const char *buf)\n \n \tswitch (whenspec) {\n \tcase WHENSPEC_RAW:\n-\t\tif (validate_raw_date(ltgt, &ident) < 0)\n+\t\tif (validate_raw_date(ltgt, &ident, 1) < 0)\n+\t\t\tdie(\"Invalid raw date \\\"%s\\\" in ident: %s\", ltgt, buf);\n+\t\tbreak;\n+\tcase WHENSPEC_RAW_PERMISSIVE:\n+\t\tif (validate_raw_date(ltgt, &ident, 0) < 0)\n \t\t\tdie(\"Invalid raw date \\\"%s\\\" in ident: %s\", ltgt, buf);\n \t\tbreak;\n \tcase WHENSPEC_RFC2822:\n@@ -3258,6 +3273,8 @@ static void option_date_format(const char *fmt)\n {\n \tif (!strcmp(fmt, \"raw\"))\n \t\twhenspec = WHENSPEC_RAW;\n+\telse if (!strcmp(fmt, \"raw-permissive\"))\n+\t\twhenspec = WHENSPEC_RAW_PERMISSIVE;\n \telse if (!strcmp(fmt, \"rfc2822\"))\n \t\twhenspec = WHENSPEC_RFC2822;\n \telse if (!strcmp(fmt, \"now\"))\ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 768257b29e0..e151df81c06 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -410,6 +410,34 @@ test_expect_success 'B: accept empty committer' '\n \ttest -z \"$out\"\n '\n \n+test_expect_success 'B: reject invalid timezone' '\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/invalid-timezone\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800\n+\tdata <input <<-INPUT_END &&\n+\tcommit refs/heads/invalid-timezone\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800\n+\tdata <out &&\n+\tgrep \"1234567890 [+]051800\" out\n+'\n+\n test_expect_success 'B: accept and fixup committer with no name' '\n \tcat >input <<-INPUT_END &&\n \tcommit refs/heads/empty-committer-2\n\nbase-commit: 2d5e9f31ac46017895ce6a183467037d29ceb9d3\n-- \ngitgitgadget\n"},{"id":"398947","messageId":"20200530231351.GA1920649@coredump.intra.peff.net","threadId":"53572","inReplyTo":"pull.795.v3.git.git.1590870357549.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] fast-import: add new --date-format=raw-permissive format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-05-30T23:13:51Z","receivedAt":"2020-05-30T23:13:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 30, 2020 at 08:25:57PM +0000, Elijah Newren via GitGitGadget wrote:\n\n> fast-import: accept invalid timezones so we can import existing repos\n> \n> Changes since v2:\n\nThanks, this version looks good to me.\n\n-Peff\n"},{"id":"416038","messageId":"87bld8ov9q.fsf@evledraar.gmail.com","threadId":"53572","inReplyTo":"pull.795.v3.git.git.1590870357549.gitgitgadget@gmail.com","subject":"Why does fast-import need to check the validity of idents? + Other ident adventures","fromName":"=?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?= Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-03T11:57:08Z","receivedAt":"2021-02-03T11:57:55Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\n[Originally sent 5 days ago, but seems to have been a victim of the\nvger.kernel.org problems at the time, re-sending]\n\nOn Sat, May 30 2020, Elijah Newren via GitGitGadget wrote:\n\n> From: Elijah Newren \n\nFull snipped E-Mail in the archive:\nhttps://lore.kernel.org/git/pull.795.v3.git.git.1590870357549.gitgitgadget@gmail.com/\n\n> There are multiple repositories in the wild with random, invalid\n> timezones. Most notably is a commit from rails.git with a timezone of\n> \"+051800\"[1]. A few searches will find other repos with that same\n> invalid timezone as well. Further, Peff reports that GitHub relaxed\n> their fsck checks in August 2011 to accept any timezone value[2], and\n> there have been multiple reports to filter-repo about fast-import\n> crashing while trying to import their existing repositories since they\n> had timezone values such as \"-7349423\" and \"-43455309\"[3].\n\nI've been looking at some of our duplicate logic here after my mktag\nseries where we now use fsck validation. It had a hardcoded \"1400\"\noffset value, which I see fast-import.c still has.\n\nThen in mailmap.c we have parse_name_and_email(), then there's\nsplit_ident_line() in ident.c, and of course\nfsck_ident(). record_person_from_buf() in fmt-merge-msg.c, copy_name()\nand copy_email() in ref-filter.c. Maybe handle_from() in mailinfo.c also\ncounts. Anyway, aside from the last these are all parsers for\n\"author/committer\" lines in commits one way or another.\n\nBut I was wondering about fast-import.c in particular. I think Elijah's\npatch here is obviously good an incremental improvement. But stepping\nback a bit: who cares about sort-of-fsck validation in fast-import.c\nanyway?\n\nShouldn't it just pretty much be importing data as-is, and then we could\ndocument \"if you don't trust it, run fsck afterwards\"?\n\nOr, if it's a use-case people actually care about, then I might see\nabout unifying some of these parser functions as part of a series I'm\npreparing.\n"},{"id":"416079","messageId":"xmqq7dnpc610.fsf@gitster.c.googlers.com","threadId":"53572","inReplyTo":"87bld8ov9q.fsf@evledraar.gmail.com","subject":"Re: Why does fast-import need to check the validity of idents? + Other ident adventures","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-02-03T19:20:27Z","receivedAt":"2021-02-03T19:21:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"=?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?=\" Bjarmason \nwrites:\n\n> But I was wondering about fast-import.c in particular. I think Elijah's\n> patch here is obviously good an incremental improvement. But stepping\n> back a bit: who cares about sort-of-fsck validation in fast-import.c\n> anyway?\n\nThose who want to notice and verify the procedure they used to\nproduce the import data from the original before it is too late?\n\nI.e. data gets imported to Git, victory declared and then old SCM\nturned gets off---and only then the resulting imported repository is\nfound not to pass fsck.\n\n> Shouldn't it just pretty much be importing data as-is, and then we could\n> document \"if you don't trust it, run fsck afterwards\"?\n\nIf it is a small import, the distinction does not matter, but for a\nhuge import, the procedure to produce the data is likely to be\nmechanical, so even after processing just a very small portion of\nearly part of the datastream, systematic errors would be noticed\nbefore fast-import wastes importing too much garbage that need to be\ndiscarded after running such fsck. So in that sense, I suspect that\nthere is value in the early validation.\n\n> Or, if it's a use-case people actually care about, then I might see\n> about unifying some of these parser functions as part of a series I'm\n> preparing.\n\nI think allowing people to loosen particular checks for fast-import\n(or elsewhere for that matter) is a good idea, and you can do so\nmore easily once the existing checking is migrated to your new\nscheme that shares code with the fsck machinery.\n"},{"id":"416322","messageId":"87k0rmcza4.fsf@evledraar.gmail.com","threadId":"53572","inReplyTo":"xmqq7dnpc610.fsf@gitster.c.googlers.com","subject":"Re: Why does fast-import need to check the validity of idents? + Other ident adventures","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-02-05T15:25:23Z","receivedAt":"2021-02-05T22:29:03Z","isPatch":false,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Wed, Feb 03 2021, Junio C Hamano wrote:\n\n> \"=?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?=\" Bjarmason \n> writes:\n>\n>> But I was wondering about fast-import.c in particular. I think Elijah's\n>> patch here is obviously good an incremental improvement. But stepping\n>> back a bit: who cares about sort-of-fsck validation in fast-import.c\n>> anyway?\n>\n> Those who want to notice and verify the procedure they used to\n> produce the import data from the original before it is too late?\n>\n> I.e. data gets imported to Git, victory declared and then old SCM\n> turned gets off---and only then the resulting imported repository is\n> found not to pass fsck.\n>\n>> Shouldn't it just pretty much be importing data as-is, and then we could\n>> document \"if you don't trust it, run fsck afterwards\"?\n>\n> If it is a small import, the distinction does not matter, but for a\n> huge import, the procedure to produce the data is likely to be\n> mechanical, so even after processing just a very small portion of\n> early part of the datastream, systematic errors would be noticed\n> before fast-import wastes importing too much garbage that need to be\n> discarded after running such fsck. So in that sense, I suspect that\n> there is value in the early validation.\n\nWhat I was fishing for here is that perhaps since fast-import was\noriginally written this use-case of in-place conversion of primary data\non a server might have become too obscure to care about, i.e. as opposed\nto doing a conversion locally and then \"git push\"-ing it to something\nthat does transfer.fsckObjects.\n\n>> Or, if it's a use-case people actually care about, then I might see\n>> about unifying some of these parser functions as part of a series I'm\n>> preparing.\n>\n> I think allowing people to loosen particular checks for fast-import\n> (or elsewhere for that matter) is a good idea, and you can do so\n> more easily once the existing checking is migrated to your new\n> scheme that shares code with the fsck machinery.\n\n...allright, depending on how much of a hassle that is I might just add\ntests for the differences and leave this particular problem to someone\nelse :)\n"}]}