[PATCH 0/5] fixes for committer/author parsing/check

[PATCH 0/5] fixes for committer/author parsing/check

[Dmitry Ivankov]

fast-import part is clear and should be safe to apply. The test
script is a bit ugly as it uses fsck and prune on a shared (with
nearby tests) repo. But it should work, any hints on simplifying
it are welcome.

fsck part fixes an uncaught bad committer, but also changes error
messages for some of bad committer strings. The messages choice is
a subject for discussion most likely.

Dmitry Ivankov (5):
  fast-import: add input format tests
  fast-import: don't fail on omitted committer name
  fast-import: check committer name more strictly
  fsck: add a few committer name tests
  fsck: improve committer/author check

 fast-import.c          |   33 ++++++++++------
 fsck.c                 |   10 +++--
 t/t1450-fsck.sh        |   24 ++++++++++++
 t/t9300-fast-import.sh |   99 ++++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 150 insertions(+), 16 deletions(-)

-- 
1.7.3.4

[PATCH 1/5] fast-import: add input format tests

[Dmitry Ivankov]

Documentation/git-fast-import.txt says that git-fast-import is strict
about it's input format. But committer/author field parsing is a bit
loose. Invalid values can be unnoticed and written out to the commit,
either with format-conforming input or with non-format-conforming one.

Add one passing and one failing test for empty/absent committer name
with well-formed input. And a failed test with unnoticed ill-formed
input.

Reported-by: SASAKI Suguru <sss.sonik@gmail.com>
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 t/t9300-fast-import.sh |   99 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 99 insertions(+), 0 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 2a53640..a659dd4 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -324,6 +324,105 @@ test_expect_success \
 	 test `git rev-parse master` = `git rev-parse TEMP_TAG^`'
 rm -f .git/TEMP_TAG
 
+git gc 2>/dev/null >/dev/null
+git prune 2>/dev/null >/dev/null
+
+cat >input <<INPUT_END
+commit refs/heads/empty-committer-1
+committer  <> $GIT_COMMITTER_DATE
+data <<COMMIT
+empty commit
+COMMIT
+INPUT_END
+test_expect_success 'B: accept empty committer' '
+	git fast-import <input &&
+	out=$(git fsck) &&
+	echo "$out" &&
+	test -z "$out"
+'
+git update-ref -d refs/heads/empty-committer-1 || true
+
+git gc 2>/dev/null >/dev/null
+git prune 2>/dev/null >/dev/null
+
+cat >input <<INPUT_END
+commit refs/heads/empty-committer-2
+committer <a@b.com> $GIT_COMMITTER_DATE
+data <<COMMIT
+empty commit
+COMMIT
+INPUT_END
+test_expect_failure 'B: accept and fixup committer with no name' '
+	git fast-import <input &&
+	out=$(git fsck) &&
+	echo "$out" &&
+	test -z "$out"
+'
+git update-ref -d refs/heads/empty-committer-2 || true
+
+git gc 2>/dev/null >/dev/null
+git prune 2>/dev/null >/dev/null
+
+cat >input <<INPUT_END
+commit refs/heads/invalid-committer
+committer Name email> $GIT_COMMITTER_DATE
+data <<COMMIT
+empty commit
+COMMIT
+INPUT_END
+test_expect_failure 'B: fail on invalid committer (1)' '
+	test_must_fail git fast-import <input
+'
+git update-ref -d refs/heads/invalid-committer || true
+
+cat >input <<INPUT_END
+commit refs/heads/invalid-committer
+committer Name <e<mail> $GIT_COMMITTER_DATE
+data <<COMMIT
+empty commit
+COMMIT
+INPUT_END
+test_expect_failure 'B: fail on invalid committer (2)' '
+	test_must_fail git fast-import <input
+'
+git update-ref -d refs/heads/invalid-committer || true
+
+cat >input <<INPUT_END
+commit refs/heads/invalid-committer
+committer Name <email>> $GIT_COMMITTER_DATE
+data <<COMMIT
+empty commit
+COMMIT
+INPUT_END
+test_expect_failure 'B: fail on invalid committer (3)' '
+	test_must_fail git fast-import <input
+'
+git update-ref -d refs/heads/invalid-committer || true
+
+cat >input <<INPUT_END
+commit refs/heads/invalid-committer
+committer Name <email $GIT_COMMITTER_DATE
+data <<COMMIT
+empty commit
+COMMIT
+INPUT_END
+test_expect_failure 'B: fail on invalid committer (4)' '
+	test_must_fail git fast-import <input
+'
+git update-ref -d refs/heads/invalid-committer || true
+
+cat >input <<INPUT_END
+commit refs/heads/invalid-committer
+committer Name<email> $GIT_COMMITTER_DATE
+data <<COMMIT
+empty commit
+COMMIT
+INPUT_END
+test_expect_failure 'B: fail on invalid committer (5)' '
+	test_must_fail git fast-import <input
+'
+git update-ref -d refs/heads/invalid-committer || true
+
 ###
 ### series C
 ###
-- 
1.7.3.4

[PATCH 2/5] fast-import: don't fail on omitted committer name

[Dmitry Ivankov]

fast-import format declares 'committer_name SP' to be optional. But SP
between empty or not name and a email is obligatory and checked by
git-fsck, so fast-import must prepend the SP if the name is omitted.
Currently it doesn't.

Name cannot contain LT or GT and ident always comes after SP in
fast-import. So reuse that SP as if a valid 'SP <email>' ident was passed.

This fixes a ident parsing bug for a well-formed fast-import input.
Though the parsing is still loose and can accept a ill-formed input.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 fast-import.c          |    4 ++++
 t/t9300-fast-import.sh |    2 +-
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 9e8d186..3194f4e 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1972,6 +1972,10 @@ static char *parse_ident(const char *buf)
 	size_t name_len;
 	char *ident;
 
+	/* ensure there is a space delimiter even if there is no name */
+	if (*buf == '<')
+		--buf;
+
 	gt = strrchr(buf, '>');
 	if (!gt)
 		die("Missing > in ident string: %s", buf);
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index a659dd4..09ef6ba 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -352,7 +352,7 @@ data <<COMMIT
 empty commit
 COMMIT
 INPUT_END
-test_expect_failure 'B: accept and fixup committer with no name' '
+test_expect_success 'B: accept and fixup committer with no name' '
 	git fast-import <input &&
 	out=$(git fsck) &&
 	echo "$out" &&
-- 
1.7.3.4

[PATCH 3/5] fast-import: check committer name more strictly

[Dmitry Ivankov]

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 fast-import.c          |   29 +++++++++++++++++------------
 t/t9300-fast-import.sh |   10 +++++-----
 2 files changed, 22 insertions(+), 17 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 3194f4e..419744f 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1968,7 +1968,7 @@ static int validate_raw_date(const char *src, char *result, int maxlen)
 
 static char *parse_ident(const char *buf)
 {
-	const char *gt;
+	const char *ltgt;
 	size_t name_len;
 	char *ident;
 
@@ -1976,28 +1976,33 @@ static char *parse_ident(const char *buf)
 	if (*buf == '<')
 		--buf;
 
-	gt = strrchr(buf, '>');
-	if (!gt)
+	ltgt = buf + strcspn(buf, "<>");
+	if (*ltgt != '<')
+		die("Missing < in ident string: %s", buf);
+	if (ltgt != buf && ltgt[-1] != ' ')
+		die("Missing space before < in ident string: %s", buf);
+	ltgt = ltgt + 1 + strcspn(ltgt + 1, "<>");
+	if (*ltgt != '>')
 		die("Missing > in ident string: %s", buf);
-	gt++;
-	if (*gt != ' ')
+	ltgt++;
+	if (*ltgt != ' ')
 		die("Missing space after > in ident string: %s", buf);
-	gt++;
-	name_len = gt - buf;
+	ltgt++;
+	name_len = ltgt - buf;
 	ident = xmalloc(name_len + 24);
 	strncpy(ident, buf, name_len);
 
 	switch (whenspec) {
 	case WHENSPEC_RAW:
-		if (validate_raw_date(gt, ident + name_len, 24) < 0)
-			die("Invalid raw date \"%s\" in ident: %s", gt, buf);
+		if (validate_raw_date(ltgt, ident + name_len, 24) < 0)
+			die("Invalid raw date \"%s\" in ident: %s", ltgt, buf);
 		break;
 	case WHENSPEC_RFC2822:
-		if (parse_date(gt, ident + name_len, 24) < 0)
-			die("Invalid rfc2822 date \"%s\" in ident: %s", gt, buf);
+		if (parse_date(ltgt, ident + name_len, 24) < 0)
+			die("Invalid rfc2822 date \"%s\" in ident: %s", ltgt, buf);
 		break;
 	case WHENSPEC_NOW:
-		if (strcmp("now", gt))
+		if (strcmp("now", ltgt))
 			die("Date in ident must be 'now': %s", buf);
 		datestamp(ident + name_len, 24);
 		break;
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 09ef6ba..18441f8 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -370,7 +370,7 @@ data <<COMMIT
 empty commit
 COMMIT
 INPUT_END
-test_expect_failure 'B: fail on invalid committer (1)' '
+test_expect_success 'B: fail on invalid committer (1)' '
 	test_must_fail git fast-import <input
 '
 git update-ref -d refs/heads/invalid-committer || true
@@ -382,7 +382,7 @@ data <<COMMIT
 empty commit
 COMMIT
 INPUT_END
-test_expect_failure 'B: fail on invalid committer (2)' '
+test_expect_success 'B: fail on invalid committer (2)' '
 	test_must_fail git fast-import <input
 '
 git update-ref -d refs/heads/invalid-committer || true
@@ -394,7 +394,7 @@ data <<COMMIT
 empty commit
 COMMIT
 INPUT_END
-test_expect_failure 'B: fail on invalid committer (3)' '
+test_expect_success 'B: fail on invalid committer (3)' '
 	test_must_fail git fast-import <input
 '
 git update-ref -d refs/heads/invalid-committer || true
@@ -406,7 +406,7 @@ data <<COMMIT
 empty commit
 COMMIT
 INPUT_END
-test_expect_failure 'B: fail on invalid committer (4)' '
+test_expect_success 'B: fail on invalid committer (4)' '
 	test_must_fail git fast-import <input
 '
 git update-ref -d refs/heads/invalid-committer || true
@@ -418,7 +418,7 @@ data <<COMMIT
 empty commit
 COMMIT
 INPUT_END
-test_expect_failure 'B: fail on invalid committer (5)' '
+test_expect_success 'B: fail on invalid committer (5)' '
 	test_must_fail git fast-import <input
 '
 git update-ref -d refs/heads/invalid-committer || true
-- 
1.7.3.4

[PATCH 4/5] fsck: add a few committer name tests

[Dmitry Ivankov]

fsck reports "missing space before <email>" for committer string equal
to "name email>". It'd be nicer to say "missing < in email" or "name is
bad" (has > in it). The second option looks a bit better, add such a
failing test.

For "name> <email>" no error is reported. Looks like a bug, so add
such a failing test."

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 t/t1450-fsck.sh |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index bb01d5a..fc7ee8e 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -110,6 +110,30 @@ test_expect_success 'email with embedded > is not okay' '
 	grep "error in commit $new" out
 '
 
+test_expect_failure 'missing < email delimiter is reported nicely' '
+	git cat-file commit HEAD >basis &&
+	sed "s/<//" basis >bad-email-2 &&
+	new=$(git hash-object -t commit -w --stdin <bad-email-2) &&
+	test_when_finished "remove_object $new" &&
+	git update-ref refs/heads/bogus "$new" &&
+	test_when_finished "git update-ref -d refs/heads/bogus" &&
+	git fsck 2>out &&
+	cat out &&
+	grep "error in commit $new.* - bad name" out
+'
+
+test_expect_failure '> in name is reported' '
+	git cat-file commit HEAD >basis &&
+	sed "s/ </> </" basis >bad-email-3 &&
+	new=$(git hash-object -t commit -w --stdin <bad-email-3) &&
+	test_when_finished "remove_object $new" &&
+	git update-ref refs/heads/bogus "$new" &&
+	test_when_finished "git update-ref -d refs/heads/bogus" &&
+	git fsck 2>out &&
+	cat out &&
+	grep "error in commit $new" out
+'
+
 test_expect_success 'tag pointing to nonexistent' '
 	cat >invalid-tag <<-\EOF &&
 	object ffffffffffffffffffffffffffffffffffffffff
-- 
1.7.3.4

[PATCH 5/5] fsck: improve committer/author check

[Dmitry Ivankov]

Neither name nor email should contain < or >, so split the string with
these and check they come in that order and < is preceeded with a space.

If < is missing don't say a confusing "missing space", say "bad name" if
> isn't missing and "missing email" if both < and > are missing.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 fsck.c          |   10 ++++++----
 t/t1450-fsck.sh |    4 ++--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/fsck.c b/fsck.c
index 60bd4bb..6c855f8 100644
--- a/fsck.c
+++ b/fsck.c
@@ -224,13 +224,15 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
 
 static int fsck_ident(char **ident, struct object *obj, fsck_error error_func)
 {
-	if (**ident == '<' || **ident == '\n')
-		return error_func(obj, FSCK_ERROR, "invalid author/committer line - missing space before email");
-	*ident += strcspn(*ident, "<\n");
-	if ((*ident)[-1] != ' ')
+	if (**ident == '<')
 		return error_func(obj, FSCK_ERROR, "invalid author/committer line - missing space before email");
+	*ident += strcspn(*ident, "<>\n");
+	if (**ident == '>')
+		return error_func(obj, FSCK_ERROR, "invalid author/committer line - bad name");
 	if (**ident != '<')
 		return error_func(obj, FSCK_ERROR, "invalid author/committer line - missing email");
+	if ((*ident)[-1] != ' ')
+		return error_func(obj, FSCK_ERROR, "invalid author/committer line - missing space before email");
 	(*ident)++;
 	*ident += strcspn(*ident, "<>\n");
 	if (**ident != '>')
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index fc7ee8e..0b92d0f 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -110,7 +110,7 @@ test_expect_success 'email with embedded > is not okay' '
 	grep "error in commit $new" out
 '
 
-test_expect_failure 'missing < email delimiter is reported nicely' '
+test_expect_success 'missing < email delimiter is reported nicely' '
 	git cat-file commit HEAD >basis &&
 	sed "s/<//" basis >bad-email-2 &&
 	new=$(git hash-object -t commit -w --stdin <bad-email-2) &&
@@ -122,7 +122,7 @@ test_expect_failure 'missing < email delimiter is reported nicely' '
 	grep "error in commit $new.* - bad name" out
 '
 
-test_expect_failure '> in name is reported' '
+test_expect_success '> in name is reported' '
 	git cat-file commit HEAD >basis &&
 	sed "s/ </> </" basis >bad-email-3 &&
 	new=$(git hash-object -t commit -w --stdin <bad-email-3) &&
-- 
1.7.3.4

Re: [PATCH 2/5] fast-import: don't fail on omitted committer name

[Junio C Hamano]

Dmitry Ivankov <divanorama@gmail.com> writes:

> fast-import format declares 'committer_name SP' to be optional. But SP
> between empty or not name and a email is obligatory and checked by

Sorry, cannot parse this.

> git-fsck, so fast-import must prepend the SP if the name is omitted.
> Currently it doesn't.
>
> Name cannot contain LT or GT and ident always comes after SP in
> fast-import. So reuse that SP as if a valid 'SP <email>' ident was passed.
>
> This fixes a ident parsing bug for a well-formed fast-import input.
> Though the parsing is still loose and can accept a ill-formed input.
>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
> ---
>  fast-import.c          |    4 ++++
>  t/t9300-fast-import.sh |    2 +-
>  2 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/fast-import.c b/fast-import.c
> index 9e8d186..3194f4e 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1972,6 +1972,10 @@ static char *parse_ident(const char *buf)
>  	size_t name_len;
>  	char *ident;
>  
> +	/* ensure there is a space delimiter even if there is no name */
> +	if (*buf == '<')
> +		--buf;
> +
>  	gt = strrchr(buf, '>');
>  	if (!gt)
>  		die("Missing > in ident string: %s", buf);
> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
> index a659dd4..09ef6ba 100755
> --- a/t/t9300-fast-import.sh
> +++ b/t/t9300-fast-import.sh
> @@ -352,7 +352,7 @@ data <<COMMIT
>  empty commit
>  COMMIT
>  INPUT_END
> -test_expect_failure 'B: accept and fixup committer with no name' '
> +test_expect_success 'B: accept and fixup committer with no name' '
>  	git fast-import <input &&
>  	out=$(git fsck) &&
>  	echo "$out" &&

Re: [PATCH 5/5] fsck: improve committer/author check

[Junio C Hamano]

Dmitry Ivankov <divanorama@gmail.com> writes:

> Neither name nor email should contain < or >, so split the string with
> these and check they come in that order and < is preceeded with a space.
>
> If < is missing don't say a confusing "missing space", say "bad name" if
>> isn't missing and "missing email" if both < and > are missing.
>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
> ---

Same comment as 3/5; before starting to talk about how you implemented
your validation, please state what rules you are enforcing.

Thanks.

Re: [PATCH 3/5] fast-import: check committer name more strictly

[Junio C Hamano]

Dmitry Ivankov <divanorama@gmail.com> writes:

> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
> ---

Please describe how you check this field "more strictly" in the body of
the log message, iow, against what rule you are validating, perhaps
something like:

 The identifier must be either "<address>" or "Name <address>" where
 neither address nor Name can contain '<' nor '>'; otherwise the input
 stream is rejected.

As fast-import is used to _create_ new objects, its input is a simple text
file that can be fixed-up as needed, it is a good idea to validate the
input more strictly and rejecting bad ones.

Re: [PATCH 2/5] fast-import: don't fail on omitted committer name

[Dmitry Ivankov]

On Tue, Aug 2, 2011 at 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dmitry Ivankov <divanorama@gmail.com> writes:
>
>> fast-import format declares 'committer_name SP' to be optional. But SP
>> between empty or not name and a email is obligatory and checked by
>
> Sorry, cannot parse this.
Ok, the point is fast-import input format for identities is declared to be
'(name SP)? LT email GT' (followed by a datetime)
where name and email are allowed to be empty (and should not have
LF, LT, GT characters).
While git-fsck checks identities to be in form
'name SP LT email GT' (followed by a datetime)
where name and email are allowed to be empty (and should not have
LF, LT, GT characters).
So fast-import must prepend a space if the name part is omitted. This
patch makes it do so.

>
>> git-fsck, so fast-import must prepend the SP if the name is omitted.
>> Currently it doesn't.
>>
>> Name cannot contain LT or GT and ident always comes after SP in
>> fast-import. So reuse that SP as if a valid 'SP <email>' ident was passed.
>>
>> This fixes a ident parsing bug for a well-formed fast-import input.
>> Though the parsing is still loose and can accept a ill-formed input.
>>
>> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
>> ---
>>  fast-import.c          |    4 ++++
>>  t/t9300-fast-import.sh |    2 +-
>>  2 files changed, 5 insertions(+), 1 deletions(-)
>>
>> diff --git a/fast-import.c b/fast-import.c
>> index 9e8d186..3194f4e 100644
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -1972,6 +1972,10 @@ static char *parse_ident(const char *buf)
>>       size_t name_len;
>>       char *ident;
>>
>> +     /* ensure there is a space delimiter even if there is no name */
>> +     if (*buf == '<')
>> +             --buf;
>> +
>>       gt = strrchr(buf, '>');
>>       if (!gt)
>>               die("Missing > in ident string: %s", buf);
>> diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
>> index a659dd4..09ef6ba 100755
>> --- a/t/t9300-fast-import.sh
>> +++ b/t/t9300-fast-import.sh
>> @@ -352,7 +352,7 @@ data <<COMMIT
>>  empty commit
>>  COMMIT
>>  INPUT_END
>> -test_expect_failure 'B: accept and fixup committer with no name' '
>> +test_expect_success 'B: accept and fixup committer with no name' '
>>       git fast-import <input &&
>>       out=$(git fsck) &&
>>       echo "$out" &&
>

Re: [PATCH 5/5] fsck: improve committer/author check

[Dmitry Ivankov]

On Tue, Aug 2, 2011 at 11:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Dmitry Ivankov <divanorama@gmail.com> writes:
>
>> Neither name nor email should contain < or >, so split the string with
>> these and check they come in that order and < is preceeded with a space.
>>
>> If < is missing don't say a confusing "missing space", say "bad name" if
>>> isn't missing and "missing email" if both < and > are missing.
>>
>> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
>> ---
>
> Same comment as 3/5; before starting to talk about how you implemented
> your validation, please state what rules you are enforcing.
Thanks, will apply and reroll.
But before this, is it ok to reject "Name> <email>" idents in fsck and
fast-import?
fsck already denies '<' and '>' inside email, and '<' in name. But
accepts '>' in name.
It just hit me that Documentation/fast-import.c doesn't deny '>' in
name and it is
consistent with fsck, so there may be a reason behind it.

>
> Thanks.
>