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

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

[Dmitry Ivankov]

This is a second version of [1]. It features one more test for a
fsck message and reworked commit messages.

Aside from fixing a parsing bug in fast-import and improving error
messages in git-fsck it makes them both to deny "name> <email>"
identities.

[1] http://thread.gmane.org/gmane.comp.version-control.git/178035

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

 Documentation/git-fast-import.txt |    4 +-
 fast-import.c                     |   33 ++++++++-----
 fsck.c                            |   10 ++--
 t/t1450-fsck.sh                   |   36 +++++++++++++
 t/t9300-fast-import.sh            |   99 +++++++++++++++++++++++++++++++++++++
 5 files changed, 164 insertions(+), 18 deletions(-)

-- 
1.7.3.4

[PATCH v2 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 f256475..0844e9d 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 v2 2/5] fast-import: don't fail on omitted committer name

[Dmitry Ivankov]

fast-import format declares 'committer_name SP' to be optional in
'committer_name SP LT email GT'. But for a (commit) object SP is
obligatory while zero length committer_name is ok. git-fsck checks
that SP is present, so fast-import must prepend it if the name SP
part is omitted. It doesn't do so and thus for "LT email GT" ident
it writes a bad object.

Name cannot contain LT or GT, ident always comes after SP in fast-import.
So if ident starts with LT reuse the SP as if a valid 'SP LT email GT'
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 7cc2262..ed1f7c9 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1973,6 +1973,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 0844e9d..8f3938c 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 v2 3/5] fast-import: check committer name more strictly

[Dmitry Ivankov]

The documentation declares following identity format:
(<name> SP)? LT <email> GT
where name is any string without LF and LT characters.
But fast-import just accepts any string up to first GT
instead of checking the whole format, and moreover just
writes it as is to the commit object.

git-fsck checks for [^<\n]* <[^<>\n]*> format. Note that the
space is mandatory. And the space quirk is already handled via
extending the string to the left when needed.

Modify fast-import input identity format to a slightly stricter
one - deny LF, LT and GT in both <name> and <email>. And check
for it.

This is stricter then git-fsck as fsck accepts "Name> <email>"
currently, but soon fsck check will be adjusted likewise.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 Documentation/git-fast-import.txt |    4 ++--
 fast-import.c                     |   29 +++++++++++++++++------------
 t/t9300-fast-import.sh            |   10 +++++-----
 3 files changed, 24 insertions(+), 19 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 2969388..ba16889 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -425,8 +425,8 @@ Here `<name>` is the person's display name (for example
 (``cm@example.com'').  `LT` and `GT` are the literal less-than (\x3c)
 and greater-than (\x3e) symbols.  These are required to delimit
 the email address from the other fields in the line.  Note that
-`<name>` is free-form and may contain any sequence of bytes, except
-`LT` and `LF`.  It is typically UTF-8 encoded.
+`<name>` and `<email>` are free-form and may contain any sequence
+of bytes, except `LT`, `GT` and `LF`.  `<name>` is typically UTF-8 encoded.
 
 The time of the change is specified by `<when>` using the date format
 that was selected by the \--date-format=<fmt> command line option.
diff --git a/fast-import.c b/fast-import.c
index ed1f7c9..6d491b9 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1969,7 +1969,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;
 
@@ -1977,28 +1977,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 8f3938c..e53ca90 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 v2 4/5] fsck: add a few committer name tests

[Dmitry Ivankov]

fsck reports "missing space before <email>" for committer string equal
to "name email>" or to "". It'd be nicer to say "missing email" for
the second string and "name is bad" (has > in it) for the first one.
Add a failing test for these messages.

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 |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index bb01d5a..01ccefd 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -110,6 +110,42 @@ 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 'missing email is reported nicely' '
+	git cat-file commit HEAD >basis &&
+	sed "s/[a-z]* <[^>]*>//" 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.* - missing email" out
+'
+
+test_expect_failure '> in name is reported' '
+	git cat-file commit HEAD >basis &&
+	sed "s/ </> </" basis >bad-email-4 &&
+	new=$(git hash-object -t commit -w --stdin <bad-email-4) &&
+	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 v2 5/5] fsck: improve committer/author check

[Dmitry Ivankov]

fsck allows a name with > character in it like "name> <email>". Also for
"name email>" fsck says "missing space before email".

More precisely, it seeks for a first '<', checks that ' ' preceeds it.
Then seeks to '<' or '>' and checks that it is the '>'. Missing space is
reported if either '<' is not found or it's not preceeded with ' '.

Change it to following. Seek to '<' or '>', check that it is '<' and is
preceeded with ' '. Seek to '<' or '>' and check that it is '>'. So now
"name> <email>" is rejected as "bad name". More strict name check is the
only change in what is accepted.

Report 'missing space' only if '<' is found and is not preceeded with a
space.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 fsck.c          |   10 ++++++----
 t/t1450-fsck.sh |    6 +++---
 2 files changed, 9 insertions(+), 7 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 01ccefd..523ce9c 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 'missing email is reported nicely' '
+test_expect_success 'missing email is reported nicely' '
 	git cat-file commit HEAD >basis &&
 	sed "s/[a-z]* <[^>]*>//" basis >bad-email-3 &&
 	new=$(git hash-object -t commit -w --stdin <bad-email-3) &&
@@ -134,7 +134,7 @@ test_expect_failure 'missing email is reported nicely' '
 	grep "error in commit $new.* - missing email" 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-4 &&
 	new=$(git hash-object -t commit -w --stdin <bad-email-4) &&
-- 
1.7.3.4

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

[Junio C Hamano]

Dmitry Ivankov <divanorama@gmail.com> writes:

> diff --git a/fast-import.c b/fast-import.c
> index 7cc2262..ed1f7c9 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1973,6 +1973,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;
> +

This is somewhat cryptic, even though it may be correct, especially if the
reader of the code does not know that this function is called by the
caller after reading "author " (or committer/tagger) and buf points at one
byte beyond that SP after the string that specifies the kind of the
person, hence "--buf" [*1*] makes the subsequent strncpy() start copying
from that SP which makes the result correct.

Perhaps an additional comment before the function is in order?

Thanks.

[Footnote]

*1* by the way, as pure style thing, I think our codebase favors to use
post-*crement, i.e. "buf--", if you are doing pre/post-*crement purely for
its side effect.