fast-import: accept invalid timezones so we can import existing repos

From: Elijah Newren <[email protected]>

There are multiple repositories in the wild with random, invalid timezones. Most notably is a commit from rails.git with a timezone of "+051800"[1]. However, a few searches found other repos with that same invalid timezone. Further, Peff reports that GitHub relaxed their fsck checks in August 2011 to accept any timezone value[2], and there have been multiple reports to filter-repo about fast-import crashing while trying to import their existing repositories since they had timezone values such as "-7349423" and "-43455309"[3].

It is not clear what created these invalid timezones, but since git has permitted their use and worked with these repositories for years at this point, it seems pointless to make fast-import be the only thing that disallows them. Relax the parsing to allow these timezones when using raw import format; when using --date-format=rfc2822 (which is not the default), we can continue being more strict about timezones.

[1] https://github.com/rails/rails/commit/4cf94979c9f4d6683c9338d694d5eb3106a4e734
[2] https://lore.kernel.org/git/[email protected]/
[3] https://github.com/newren/git-filter-repo/issues/88

---
fast-import: accept invalid timezones so we can import existing repos

Note: this is not a fix for a regression, so you can ignore it for 2.27; it can sit in pu.

Peff leaned towards normalizing these timezones in fast-export[1], but
(A) it's not clear what "valid" timezone we should randomly pick and regardless of what we pick, it seems it'll be wrong for most cases, (B) it would provide yet another way that "git fast-export --all | git fast-import" would not preserve the original history, as users sometimes expect[2], and (C) that'd prevent users from passing a special callback to filter-repo to fix up these values[3].

Since I'm not a fan of picking a random value to reassign these to (in either fast-export or fast-import), I went the route of relaxing the requirements in fast-import, similar to what Peff reports GitHub did about 9 years ago in their incoming fsck checks.

[1]
https://lore.kernel.org/git/[email protected]/
[2]
https://lore.kernel.org/git/CABPp-BFLJ48BZ97Y9mr4i3q7HMqjq18cXMgSYdxqD1cMzH8Spg@mail.gmail.com/
[3] Example:
https://github.com/newren/git-filter-repo/issues/88#issuecomment-629706776

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-795%2Fnewren%2Floosen-fast-import-timezone-parsing-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-795/newren/loosen-fast-import-timezone-parsing-v1
Pull-Request: https://github.com/git/git/pull/795

 fast-import.c          |  7 +++----
 t/t9300-fast-import.sh | 17 +++++++++++++++++
 2 files changed, 20 insertions(+), 4 deletions(-)

index c98970274c4..4a3c193007d 100644
@@ -1915,11 +1915,10 @@ static int validate_raw_date(const char *src, struct strbuf *result)
 {
 	const char *orig_src = src;
 	char *endp;
-	unsigned long num;
 
 	errno = 0;
 
-	num = strtoul(src, &endp, 10);
+	strtoul(src, &endp, 10);
 	/* NEEDSWORK: perhaps check for reasonable values? */
 	if (errno || endp == src || *endp != ' ')
 		return -1;

@@ -1928,8 +1927,8 @@ static int validate_raw_date(const char *src, struct strbuf *result)
 	if (*src != '-' && *src != '+')
 		return -1;
 
-	num = strtoul(src + 1, &endp, 10);
-	if (errno || endp == src + 1 || *endp || 1400 < num)
+	strtoul(src + 1, &endp, 10);
+	if (errno || endp == src + 1 || *endp)
 		return -1;
 
 	strbuf_addstr(result, orig_src);

index 768257b29e0..0e798e68476 100755
@@ -410,6 +410,23 @@ test_expect_success 'B: accept empty committer' '
 	test -z "$out"
 '
 
+test_expect_success 'B: accept invalid timezone' '
+	cat >input <<-INPUT_END &&
+	commit refs/heads/invalid-timezone
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800
+	data <<COMMIT
+	empty commit
+	COMMIT
+	INPUT_END
+
+	test_when_finished "git update-ref -d refs/heads/invalid-timezone
+		git gc
+		git prune" &&
+	git fast-import <input &&
+	git cat-file -p invalid-timezone >out &&
+	grep "1234567890 [+]051800" out
+'
+
 test_expect_success 'B: accept and fixup committer with no name' '
 	cat >input <<-INPUT_END &&
 	commit refs/heads/empty-committer-2

base-commit: 2d5e9f31ac46017895ce6a183467037d29ceb9d3

gitgitgadget

Hi,

There are two different use cases here that we may want to distinguish.

The original motivation for fast-import was for importing a repository from some non-Git storage system (another VCS, a collection of patches and tarballs, or whatever). Such an importer might use --date-format=raw just because that's simple, to avoid overhead. In that case, the strict parsing seems useful for catching bugs in the importer.

"git filter-repo" is for taking an existing repository and modifying it. In this case, it would be *possible* to take an invalid timezone and normalize it to "whatever 'git log' would show", but that's overreaching relative to the caller's intent: the caller has a specific set of history modifications they meant to make, and fixing the time zone wasn't necessarily one of them.

To that end, would it make sense for this to be a new date-format (e.g., --date-format=raw-permissive) to avoid regressing behavior in the original case?

Thanks and hope that helps,
Jonathan

From: Elijah Newren <[email protected]>

There are multiple repositories in the wild with random, invalid timezones. Most notably is a commit from rails.git with a timezone of "+051800"[1]. A few searches will find other repos with that same invalid timezone as well. Further, Peff reports that GitHub relaxed their fsck checks in August 2011 to accept any timezone value[2], and there have been multiple reports to filter-repo about fast-import crashing while trying to import their existing repositories since they had timezone values such as "-7349423" and "-43455309"[3].

The existing check on timezone values inside fast-import may prove useful for people who are crafting fast-import input by hand or with a new script. For them, the check may help them avoid accidentally recording invalid dates. (Note that this check is rather simplistic and there are still several forms of invalid dates that fast-import does not check for: dates in the future, timezone values with minutes that are not divisible by 15, and timezone values with minutes that are 60 or greater.) While this simple check may have some value for those users, other users or tools will want to import existing repositories as-is. Provide a --date-format=raw-permissive format that will not error out on these otherwise invalid timezones so that such existing repositories can be imported.

[1] https://github.com/rails/rails/commit/4cf94979c9f4d6683c9338d694d5eb3106a4e734
[2] https://lore.kernel.org/git/[email protected]/
[3] https://github.com/newren/git-filter-repo/issues/88

---
fast-import: accept invalid timezones so we can import existing repos

Changes since v1:

* Instead of just allowing such timezones outright, did it behind a --date-format=raw-permissive as suggested by Jonathan

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-795%2Fnewren%2Floosen-fast-import-timezone-parsing-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-795/newren/loosen-fast-import-timezone-parsing-v2
Pull-Request: https://github.com/git/git/pull/795

Range-diff vs v1:

1: d3a7dbc3892 ! 1: 9580aacdb21 fast-import: accept invalid timezones so we can import existing repos
@@ Metadata
Author: Elijah Newren <[email protected]>

## Commit message ##
- fast-import: accept invalid timezones so we can import existing repos
+ fast-import: add new --date-format=raw-permissive format

There are multiple repositories in the wild with random, invalid timezones. Most notably is a commit from rails.git with a timezone of
- "+051800"[1]. However, a few searches found other repos with that same
- invalid timezone. Further, Peff reports that GitHub relaxed their fsck
- checks in August 2011 to accept any timezone value[2], and there have
- been multiple reports to filter-repo about fast-import crashing while
- trying to import their existing repositories since they had timezone
- values such as "-7349423" and "-43455309"[3].
+ "+051800"[1]. A few searches will find other repos with that same
+ invalid timezone as well. Further, Peff reports that GitHub relaxed
+ their fsck checks in August 2011 to accept any timezone value[2], and
+ there have been multiple reports to filter-repo about fast-import
+ crashing while trying to import their existing repositories since they
+ had timezone values such as "-7349423" and "-43455309"[3].

- It is not clear what created these invalid timezones, but since git has
- permitted their use and worked with these repositories for years at this
- point, it seems pointless to make fast-import be the only thing that
- disallows them. Relax the parsing to allow these timezones when using
- raw import format; when using --date-format=rfc2822 (which is not the
- default), we can continue being more strict about timezones.
+ The existing check on timezone values inside fast-import may prove
+ useful for people who are crafting fast-import input by hand or with a
+ new script. For them, the check may help them avoid accidentally
+ recording invalid dates. (Note that this check is rather simplistic and
+ there are still several forms of invalid dates that fast-import does not
+ check for: dates in the future, timezone values with minutes that are
+ not divisible by 15, and timezone values with minutes that are 60 or
+ greater.) While this simple check may have some value for those users,
+ other users or tools will want to import existing repositories as-is.
+ Provide a --date-format=raw-permissive format that will not error out on
+ these otherwise invalid timezones so that such existing repositories can
+ be imported.

[1] https://github.com/rails/rails/commit/4cf94979c9f4d6683c9338d694d5eb3106a4e734
[2] https://lore.kernel.org/git/[email protected]/
@@ Commit message

## fast-import.c ##
-@@ fast-import.c: static int validate_raw_date(const char *src, struct strbuf *result) +@@ fast-import.c: struct hash_list {
+
+ typedef enum {
+ WHENSPEC_RAW = 1,
++ WHENSPEC_RAW_PERMISSIVE,
+ WHENSPEC_RFC2822,
+ WHENSPEC_NOW
+ } whenspec_type;
+@@ fast-import.c: static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)
+ return 1;
+ }
+
+-static int validate_raw_date(const char *src, struct strbuf *result)
++static int validate_raw_date(const char *src, struct strbuf *result, int strict)
{
const char *orig_src = src;
char *endp;
-- unsigned long num;
+ unsigned long num;
++ int out_of_range_timezone;

errno = 0;

-- num = strtoul(src, &endp, 10);
-+ strtoul(src, &endp, 10);
- /* NEEDSWORK: perhaps check for reasonable values? */
- if (errno || endp == src || *endp != ' ')
- return -1;
@@ fast-import.c: static int validate_raw_date(const char *src, struct strbuf *result)
- if (*src != '-' && *src != '+')
return -1;

-- num = strtoul(src + 1, &endp, 10);
+ num = strtoul(src + 1, &endp, 10);
- if (errno || endp == src + 1 || *endp || 1400 < num)
-+ strtoul(src + 1, &endp, 10);
-+ if (errno || endp == src + 1 || *endp)
++ out_of_range_timezone = strict && (1400 < num);
++ if (errno || endp == src + 1 || *endp || out_of_range_timezone)
return -1;

strbuf_addstr(result, orig_src);
+@@ fast-import.c: static char *parse_ident(const char *buf) +
+ switch (whenspec) {
+ case WHENSPEC_RAW:
+- if (validate_raw_date(ltgt, &ident) < 0)
++ if (validate_raw_date(ltgt, &ident, 1) < 0)
++ die("Invalid raw date \"%s\" in ident: %s", ltgt, buf);
++ break;
++ case WHENSPEC_RAW_PERMISSIVE:
++ if (validate_raw_date(ltgt, &ident, 0) < 0)
+ die("Invalid raw date \"%s\" in ident: %s", ltgt, buf);
+ break;
+ case WHENSPEC_RFC2822:
+@@ fast-import.c: static void option_date_format(const char *fmt)
+ {
+ if (!strcmp(fmt, "raw"))
+ whenspec = WHENSPEC_RAW;
++ else if (!strcmp(fmt, "raw-permissive"))
++ whenspec = WHENSPEC_RAW_PERMISSIVE;
+ else if (!strcmp(fmt, "rfc2822"))
+ whenspec = WHENSPEC_RFC2822;
+ else if (!strcmp(fmt, "now"))

## t/t9300-fast-import.sh ##
@@ t/t9300-fast-import.sh: test_expect_success 'B: accept empty committer' '
test -z "$out"
'

-+test_expect_success 'B: accept invalid timezone' ' ++test_expect_success 'B: reject invalid timezone' '
++ cat >input <<-INPUT_END &&
++ commit refs/heads/invalid-timezone
++ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800
++ data <<COMMIT
++ empty commit
++ COMMIT
++ INPUT_END
++
++ test_when_finished "git update-ref -d refs/heads/invalid-timezone" &&
++ test_must_fail git fast-import <input
++'
++
++test_expect_success 'B: accept invalid timezone with raw-permissive' '
+ cat >input <<-INPUT_END &&
+ commit refs/heads/invalid-timezone
+ committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800
@@ t/t9300-fast-import.sh: test_expect_success 'B: accept empty committer' '
+ test_when_finished "git update-ref -d refs/heads/invalid-timezone
+ git gc
+ git prune" &&
-+ git fast-import <input &&
++ git fast-import --date-format=raw-permissive <input &&
+ git cat-file -p invalid-timezone >out &&
+ grep "1234567890 [+]051800" out
+'

 fast-import.c          | 15 ++++++++++++---
 t/t9300-fast-import.sh | 30 ++++++++++++++++++++++++++++++
 2 files changed, 42 insertions(+), 3 deletions(-)

index c98970274c4..74d7298bc5a 100644
@@ -139,6 +139,7 @@ struct hash_list {
 
 typedef enum {
 	WHENSPEC_RAW = 1,
+	WHENSPEC_RAW_PERMISSIVE,
 	WHENSPEC_RFC2822,
 	WHENSPEC_NOW
 } whenspec_type;

@@ -1911,11 +1912,12 @@ static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)
 	return 1;
 }
 
-static int validate_raw_date(const char *src, struct strbuf *result)
+static int validate_raw_date(const char *src, struct strbuf *result, int strict)
 {
 	const char *orig_src = src;
 	char *endp;
 	unsigned long num;
+	int out_of_range_timezone;
 
 	errno = 0;
 

@@ -1929,7 +1931,8 @@ static int validate_raw_date(const char *src, struct strbuf *result)
 		return -1;
 
 	num = strtoul(src + 1, &endp, 10);
-	if (errno || endp == src + 1 || *endp || 1400 < num)
+	out_of_range_timezone = strict && (1400 < num);
+	if (errno || endp == src + 1 || *endp || out_of_range_timezone)
 		return -1;
 
 	strbuf_addstr(result, orig_src);

@@ -1963,7 +1966,11 @@ static char *parse_ident(const char *buf)
 
 	switch (whenspec) {
 	case WHENSPEC_RAW:
-		if (validate_raw_date(ltgt, &ident) < 0)
+		if (validate_raw_date(ltgt, &ident, 1) < 0)
+			die("Invalid raw date \"%s\" in ident: %s", ltgt, buf);
+		break;
+	case WHENSPEC_RAW_PERMISSIVE:
+		if (validate_raw_date(ltgt, &ident, 0) < 0)
 			die("Invalid raw date \"%s\" in ident: %s", ltgt, buf);
 		break;
 	case WHENSPEC_RFC2822:

@@ -3258,6 +3265,8 @@ static void option_date_format(const char *fmt)
 {
 	if (!strcmp(fmt, "raw"))
 		whenspec = WHENSPEC_RAW;
+	else if (!strcmp(fmt, "raw-permissive"))
+		whenspec = WHENSPEC_RAW_PERMISSIVE;
 	else if (!strcmp(fmt, "rfc2822"))
 		whenspec = WHENSPEC_RFC2822;
 	else if (!strcmp(fmt, "now"))

index 768257b29e0..14e9baa22db 100755
@@ -410,6 +410,36 @@ test_expect_success 'B: accept empty committer' '
 	test -z "$out"
 '
 
+test_expect_success 'B: reject invalid timezone' '
+	cat >input <<-INPUT_END &&
+	commit refs/heads/invalid-timezone
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800
+	data <<COMMIT
+	empty commit
+	COMMIT
+	INPUT_END
+
+	test_when_finished "git update-ref -d refs/heads/invalid-timezone" &&
+	test_must_fail git fast-import <input
+'
+
+test_expect_success 'B: accept invalid timezone with raw-permissive' '
+	cat >input <<-INPUT_END &&
+	commit refs/heads/invalid-timezone
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800
+	data <<COMMIT
+	empty commit
+	COMMIT
+	INPUT_END
+
+	test_when_finished "git update-ref -d refs/heads/invalid-timezone
+		git gc
+		git prune" &&
+	git fast-import --date-format=raw-permissive <input &&
+	git cat-file -p invalid-timezone >out &&
+	grep "1234567890 [+]051800" out
+'
+
 test_expect_success 'B: accept and fixup committer with no name' '
 	cat >input <<-INPUT_END &&
 	commit refs/heads/empty-committer-2

base-commit: 2d5e9f31ac46017895ce6a183467037d29ceb9d3

gitgitgadget

So the idea is to just propagate whatever raw timestamp plus timezone we read from the input stream, which most likely has been copied from a (broken) raw commit object, to a rewritten commit?

Makes sense. Will queue.

Thanks.

[...]

This could potentially go in a Bug: footer.

Makes sense.

Thanks.

Thanks, I like the safety this gives us against importer bugs. It does mean that people doing "export | filter | import" may have to manually loosen it, but it should be rare enough that this isn't a big burden (and if they're rewriting anyway, maybe it gives them a chance to decide how to fix it up).

Would we need a documentation update for "raw-permissive", too?

    git update-ref -d refs/heads/invalid-timezone &&
    git gc &&
    git prune

Good eyes. I somehow thought this was an internal option but it does need to be known by end-users who use the fast-import tool.

True, but if it is easy to make it kosher, we should.

	if (errno || endp == src + 1 || *endp || /* did not parse */
	    (strict && (1400 < num)) /* parsed a broken timezone */
	   )

perhaps?

Sure. Another alternative is to make it right/modern for only this added test---that makes the inconsistency stand out and gives incentive to others for cleaning up after the dust settles when the patch gets merged to the 'master' branch.

Nice.

From: Elijah Newren <[email protected]>

There are multiple repositories in the wild with random, invalid timezones. Most notably is a commit from rails.git with a timezone of "+051800"[1]. A few searches will find other repos with that same invalid timezone as well. Further, Peff reports that GitHub relaxed their fsck checks in August 2011 to accept any timezone value[2], and there have been multiple reports to filter-repo about fast-import crashing while trying to import their existing repositories since they had timezone values such as "-7349423" and "-43455309"[3].

The existing check on timezone values inside fast-import may prove useful for people who are crafting fast-import input by hand or with a new script. For them, the check may help them avoid accidentally recording invalid dates. (Note that this check is rather simplistic and there are still several forms of invalid dates that fast-import does not check for: dates in the future, timezone values with minutes that are not divisible by 15, and timezone values with minutes that are 60 or greater.) While this simple check may have some value for those users, other users or tools will want to import existing repositories as-is. Provide a --date-format=raw-permissive format that will not error out on these otherwise invalid timezones so that such existing repositories can be imported.

[1] https://github.com/rails/rails/commit/4cf94979c9f4d6683c9338d694d5eb3106a4e734
[2] https://lore.kernel.org/git/[email protected]/
[3] https://github.com/newren/git-filter-repo/issues/88

---
fast-import: accept invalid timezones so we can import existing repos

Changes since v2:

* Add documentation
* Note the fact that the "strict" method really isn't all that strict with some NEEDSWORK comments
* Check for parsed as unsigned before checking that value range makes sense
* Simplify the testcase as suggested by Peff, leaving it to stick out a bit like a sore thumb from the rest of the tests in the same file (#leftoverbits)

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-795%2Fnewren%2Floosen-fast-import-timezone-parsing-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-795/newren/loosen-fast-import-timezone-parsing-v3
Pull-Request: https://github.com/git/git/pull/795

Range-diff vs v2:

1: 9580aacdb21 ! 1: 48326d16dbd fast-import: add new --date-format=raw-permissive format

     @@ Commit message
      
          Signed-off-by: Elijah Newren <[email protected]>
      
     + ## Documentation/git-fast-import.txt ##
     +@@ Documentation/git-fast-import.txt: by users who are located in the same location and time zone.  In this
     + case a reasonable offset from UTC could be assumed.
     + +
     + Unlike the `rfc2822` format, this format is very strict.  Any
     +-variation in formatting will cause fast-import to reject the value.
     ++variation in formatting will cause fast-import to reject the value,
     ++and some sanity checks on the numeric values may also be performed.
     ++
     ++`raw-permissive`::
     ++	This is the same as `raw` except that no sanity checks on
     ++	the numeric epoch and local offset are performed.  This can
     ++	be useful when trying to filter or import an existing history
     ++	with e.g. bogus timezone values.
     + 
     + `rfc2822`::
     + 	This is the standard email format as described by RFC 2822.
     +
       ## fast-import.c ##
      @@ fast-import.c: struct hash_list {
       
     @@ fast-import.c: static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax
       {
       	const char *orig_src = src;
       	char *endp;
     - 	unsigned long num;
     -+	int out_of_range_timezone;
     - 
     +@@ fast-import.c: static int validate_raw_date(const char *src, struct strbuf *result)
       	errno = 0;
       
     + 	num = strtoul(src, &endp, 10);
     +-	/* NEEDSWORK: perhaps check for reasonable values? */
     ++	/*
     ++	 * NEEDSWORK: perhaps check for reasonable values? For example, we
     ++	 *            could error on values representing times more than a
     ++	 *            day in the future.
     ++	 */
     + 	if (errno || endp == src || *endp != ' ')
     + 		return -1;
     + 
      @@ fast-import.c: static int validate_raw_date(const char *src, struct strbuf *result)
       		return -1;
       
       	num = strtoul(src + 1, &endp, 10);
      -	if (errno || endp == src + 1 || *endp || 1400 < num)
     -+	out_of_range_timezone = strict && (1400 < num);
     -+	if (errno || endp == src + 1 || *endp || out_of_range_timezone)
     ++	/*
     ++	 * NEEDSWORK: check for brokenness other than num > 1400, such as
     ++	 *            (num % 100) >= 60, or ((num % 100) % 15) != 0 ?
     ++	 */
     ++	if (errno || endp == src + 1 || *endp || /* did not parse */
     ++	    (strict && (1400 < num))             /* parsed a broken timezone */
     ++	   )
       		return -1;
       
       	strbuf_addstr(result, orig_src);
     @@ t/t9300-fast-import.sh: test_expect_success 'B: accept empty committer' '
      +	COMMIT
      +	INPUT_END
      +
     -+	test_when_finished "git update-ref -d refs/heads/invalid-timezone
     -+		git gc
     -+		git prune" &&
     -+	git fast-import --date-format=raw-permissive <input &&
     -+	git cat-file -p invalid-timezone >out &&
     ++	git init invalid-timezone &&
     ++	git -C invalid-timezone fast-import --date-format=raw-permissive <input &&
     ++	git -C invalid-timezone cat-file -p invalid-timezone >out &&
      +	grep "1234567890 [+]051800" out
      +'
      +

 Documentation/git-fast-import.txt |  9 ++++++++-
 fast-import.c                     | 25 +++++++++++++++++++++----
 t/t9300-fast-import.sh            | 28 ++++++++++++++++++++++++++++
 3 files changed, 57 insertions(+), 5 deletions(-)

index 77c6b3d0019..7d9aad2a7e1 100644
@@ -293,7 +293,14 @@ by users who are located in the same location and time zone.  In this
 case a reasonable offset from UTC could be assumed.
 +
 Unlike the `rfc2822` format, this format is very strict.  Any
-variation in formatting will cause fast-import to reject the value.
+variation in formatting will cause fast-import to reject the value,
+and some sanity checks on the numeric values may also be performed.
+
+`raw-permissive`::
+	This is the same as `raw` except that no sanity checks on
+	the numeric epoch and local offset are performed.  This can
+	be useful when trying to filter or import an existing history
+	with e.g. bogus timezone values.
 
 `rfc2822`::
 	This is the standard email format as described by RFC 2822.

index c98970274c4..0dfa14dc8c3 100644
@@ -139,6 +139,7 @@ struct hash_list {
 
 typedef enum {
 	WHENSPEC_RAW = 1,
+	WHENSPEC_RAW_PERMISSIVE,
 	WHENSPEC_RFC2822,
 	WHENSPEC_NOW
 } whenspec_type;

@@ -1911,7 +1912,7 @@ static int parse_data(struct strbuf *sb, uintmax_t limit, uintmax_t *len_res)
 	return 1;
 }
 
-static int validate_raw_date(const char *src, struct strbuf *result)
+static int validate_raw_date(const char *src, struct strbuf *result, int strict)
 {
 	const char *orig_src = src;
 	char *endp;

@@ -1920,7 +1921,11 @@ static int validate_raw_date(const char *src, struct strbuf *result)
 	errno = 0;
 
 	num = strtoul(src, &endp, 10);
-	/* NEEDSWORK: perhaps check for reasonable values? */
+	/*
+	 * NEEDSWORK: perhaps check for reasonable values? For example, we
+	 *            could error on values representing times more than a
+	 *            day in the future.
+	 */
 	if (errno || endp == src || *endp != ' ')
 		return -1;
 

@@ -1929,7 +1934,13 @@ static int validate_raw_date(const char *src, struct strbuf *result)
 		return -1;
 
 	num = strtoul(src + 1, &endp, 10);
-	if (errno || endp == src + 1 || *endp || 1400 < num)
+	/*
+	 * NEEDSWORK: check for brokenness other than num > 1400, such as
+	 *            (num % 100) >= 60, or ((num % 100) % 15) != 0 ?
+	 */
+	if (errno || endp == src + 1 || *endp || /* did not parse */
+	    (strict && (1400 < num))             /* parsed a broken timezone */
+	   )
 		return -1;
 
 	strbuf_addstr(result, orig_src);

@@ -1963,7 +1974,11 @@ static char *parse_ident(const char *buf)
 
 	switch (whenspec) {
 	case WHENSPEC_RAW:
-		if (validate_raw_date(ltgt, &ident) < 0)
+		if (validate_raw_date(ltgt, &ident, 1) < 0)
+			die("Invalid raw date \"%s\" in ident: %s", ltgt, buf);
+		break;
+	case WHENSPEC_RAW_PERMISSIVE:
+		if (validate_raw_date(ltgt, &ident, 0) < 0)
 			die("Invalid raw date \"%s\" in ident: %s", ltgt, buf);
 		break;
 	case WHENSPEC_RFC2822:

@@ -3258,6 +3273,8 @@ static void option_date_format(const char *fmt)
 {
 	if (!strcmp(fmt, "raw"))
 		whenspec = WHENSPEC_RAW;
+	else if (!strcmp(fmt, "raw-permissive"))
+		whenspec = WHENSPEC_RAW_PERMISSIVE;
 	else if (!strcmp(fmt, "rfc2822"))
 		whenspec = WHENSPEC_RFC2822;
 	else if (!strcmp(fmt, "now"))

index 768257b29e0..e151df81c06 100755
@@ -410,6 +410,34 @@ test_expect_success 'B: accept empty committer' '
 	test -z "$out"
 '
 
+test_expect_success 'B: reject invalid timezone' '
+	cat >input <<-INPUT_END &&
+	commit refs/heads/invalid-timezone
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800
+	data <<COMMIT
+	empty commit
+	COMMIT
+	INPUT_END
+
+	test_when_finished "git update-ref -d refs/heads/invalid-timezone" &&
+	test_must_fail git fast-import <input
+'
+
+test_expect_success 'B: accept invalid timezone with raw-permissive' '
+	cat >input <<-INPUT_END &&
+	commit refs/heads/invalid-timezone
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1234567890 +051800
+	data <<COMMIT
+	empty commit
+	COMMIT
+	INPUT_END
+
+	git init invalid-timezone &&
+	git -C invalid-timezone fast-import --date-format=raw-permissive <input &&
+	git -C invalid-timezone cat-file -p invalid-timezone >out &&
+	grep "1234567890 [+]051800" out
+'
+
 test_expect_success 'B: accept and fixup committer with no name' '
 	cat >input <<-INPUT_END &&
 	commit refs/heads/empty-committer-2

base-commit: 2d5e9f31ac46017895ce6a183467037d29ceb9d3

gitgitgadget

Thanks, this version looks good to me.

-Peff

[Originally sent 5 days ago, but seems to have been a victim of the vger.kernel.org problems at the time, re-sending]

Full snipped E-Mail in the archive:
https://lore.kernel.org/git/[email protected]/

I've been looking at some of our duplicate logic here after my mktag series where we now use fsck validation. It had a hardcoded "1400" offset value, which I see fast-import.c still has.

Then in mailmap.c we have parse_name_and_email(), then there's split_ident_line() in ident.c, and of course
fsck_ident(). record_person_from_buf() in fmt-merge-msg.c, copy_name() and copy_email() in ref-filter.c. Maybe handle_from() in mailinfo.c also counts. Anyway, aside from the last these are all parsers for "author/committer" lines in commits one way or another.

But I was wondering about fast-import.c in particular. I think Elijah's patch here is obviously good an incremental improvement. But stepping back a bit: who cares about sort-of-fsck validation in fast-import.c anyway?

Shouldn't it just pretty much be importing data as-is, and then we could document "if you don't trust it, run fsck afterwards"?

Or, if it's a use-case people actually care about, then I might see about unifying some of these parser functions as part of a series I'm preparing.

"=?utf-8?B?w4Z2YXIgQXJuZmrDtnLDsA==?=" Bjarmason <[email protected]>

Those who want to notice and verify the procedure they used to produce the import data from the original before it is too late?

I.e. data gets imported to Git, victory declared and then old SCM turned gets off---and only then the resulting imported repository is found not to pass fsck.

If it is a small import, the distinction does not matter, but for a huge import, the procedure to produce the data is likely to be mechanical, so even after processing just a very small portion of early part of the datastream, systematic errors would be noticed before fast-import wastes importing too much garbage that need to be discarded after running such fsck. So in that sense, I suspect that there is value in the early validation.

I think allowing people to loosen particular checks for fast-import (or elsewhere for that matter) is a good idea, and you can do so more easily once the existing checking is migrated to your new scheme that shares code with the fsck machinery.

What I was fishing for here is that perhaps since fast-import was originally written this use-case of in-place conversion of primary data on a server might have become too obscure to care about, i.e. as opposed to doing a conversion locally and then "git push"-ing it to something that does transfer.fsckObjects.

...allright, depending on how much of a hassle that is I might just add tests for the differences and leave this particular problem to someone else :)