[PATCH 1/3] git-fsck: report missing author/commit line in a commit as an error

[PATCH 1/3] git-fsck: report missing author/commit line in a commit as an error

[Martin Koegler]

A zero commit date could be caused by:
* a missing author line
* a missing commiter line
* a malformed email address in the commiter line
* a malformed commit date

Simply reporting it as zero commit date is missleading.

Additionally, it upgrades the message to an error (instead of an printf).

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 builtin-fsck.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin-fsck.c b/builtin-fsck.c
index 6fc9525..cc7524b 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -360,6 +360,9 @@ static int fsck_commit(struct commit *commit)
 		fprintf(stderr, "Checking commit %s\n",
 			sha1_to_hex(commit->object.sha1));
 
+	if (!commit->date)
+		return objerror(&commit->object, "invalid author/committer line");
+
 	if (memcmp(buffer, "tree ", 5))
 		return objerror(&commit->object, "invalid format - expected 'tree' line");
 	if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n')
@@ -378,9 +381,6 @@ static int fsck_commit(struct commit *commit)
 		return objerror(&commit->object, "could not load commit's tree %s", tree_sha1);
 	if (!commit->parents && show_root)
 		printf("root %s\n", sha1_to_hex(commit->object.sha1));
-	if (!commit->date)
-		printf("bad commit date in %s\n",
-		       sha1_to_hex(commit->object.sha1));
 	return 0;
 }
 
-- 
1.5.4.g22bc

[PATCH 2/3] fsck_commit: remove duplicate tests

[Martin Koegler]

All tests on the commit buffer in fsck_cmd are ready done by
parse_commit_buffer.

This patch rips out all redundant tests. It still leaves the check for
author, as this is can be used as starting point for validation the
author/committer information.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 builtin-fsck.c |   13 ++++---------
 1 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/builtin-fsck.c b/builtin-fsck.c
index cc7524b..ba785ec 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -354,7 +354,7 @@ static int fsck_tree(struct tree *item)
 static int fsck_commit(struct commit *commit)
 {
 	char *buffer = commit->buffer;
-	unsigned char tree_sha1[20], sha1[20];
+	unsigned char tree_sha1[20];
 
 	if (verbose)
 		fprintf(stderr, "Checking commit %s\n",
@@ -363,22 +363,17 @@ static int fsck_commit(struct commit *commit)
 	if (!commit->date)
 		return objerror(&commit->object, "invalid author/committer line");
 
-	if (memcmp(buffer, "tree ", 5))
-		return objerror(&commit->object, "invalid format - expected 'tree' line");
-	if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n')
+	if (get_sha1_hex(buffer+5, tree_sha1))
 		return objerror(&commit->object, "invalid 'tree' line format - bad sha1");
 	buffer += 46;
-	while (!memcmp(buffer, "parent ", 7)) {
-		if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
-			return objerror(&commit->object, "invalid 'parent' line format - bad sha1");
+	while (!memcmp(buffer, "parent ", 7))
 		buffer += 48;
-	}
 	if (memcmp(buffer, "author ", 7))
 		return objerror(&commit->object, "invalid format - expected 'author' line");
 	free(commit->buffer);
 	commit->buffer = NULL;
 	if (!commit->tree)
-		return objerror(&commit->object, "could not load commit's tree %s", tree_sha1);
+		return objerror(&commit->object, "could not load commit's tree %s", sha1_to_hex(tree_sha1));
 	if (!commit->parents && show_root)
 		printf("root %s\n", sha1_to_hex(commit->object.sha1));
 	return 0;
-- 
1.5.4.g22bc

[PATCH 3/3] parse_object_buffer: don't ignore errors from the object specific parsing functions

[Martin Koegler]

In the case of an malformed object, the object specific parsing functions
would return an error, which is currently ignored. The object can be partial
initialized in this case.

This patch make parse_object_buffer propagate such errors.

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 object.c |   12 ++++++++----
 1 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/object.c b/object.c
index 5a5ebe2..4e7f27a 100644
--- a/object.c
+++ b/object.c
@@ -140,7 +140,8 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 	if (type == OBJ_BLOB) {
 		struct blob *blob = lookup_blob(sha1);
 		if (blob) {
-			parse_blob_buffer(blob, buffer, size);
+			if (parse_blob_buffer(blob, buffer, size))
+				return NULL;
 			obj = &blob->object;
 		}
 	} else if (type == OBJ_TREE) {
@@ -148,14 +149,16 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 		if (tree) {
 			obj = &tree->object;
 			if (!tree->object.parsed) {
-				parse_tree_buffer(tree, buffer, size);
+				if (parse_tree_buffer(tree, buffer, size))
+					return NULL;
 				eaten = 1;
 			}
 		}
 	} else if (type == OBJ_COMMIT) {
 		struct commit *commit = lookup_commit(sha1);
 		if (commit) {
-			parse_commit_buffer(commit, buffer, size);
+			if (parse_commit_buffer(commit, buffer, size))
+				return NULL;	
 			if (!commit->buffer) {
 				commit->buffer = buffer;
 				eaten = 1;
@@ -165,7 +168,8 @@ struct object *parse_object_buffer(const unsigned char *sha1, enum object_type t
 	} else if (type == OBJ_TAG) {
 		struct tag *tag = lookup_tag(sha1);
 		if (tag) {
-			parse_tag_buffer(tag, buffer, size);
+			if (parse_tag_buffer(tag, buffer, size))
+			       return NULL;
 			obj = &tag->object;
 		}
 	} else {
-- 
1.5.4.g22bc

Re: [PATCH 2/3] fsck_commit: remove duplicate tests

[Junio C Hamano]

Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:

> All tests on the commit buffer in fsck_cmd are ready done by
> parse_commit_buffer.
>
> This patch rips out all redundant tests.

As I believe in belt-and-suspenders when it comes to validation,
I am somewhat uneasy with this change.  If it ever turns out
that we would need to make parse_commit_buffer() more lenient
during the normal operation, some of this would need to be
reverted (for example, you may not need to have a valid "tree"
to salvage the log messages from a corrupt history by running
"git log"), and forgetting to do so will result in fsck that
does not validate enough.

The validation done by parse_commit_buffer() and fsck serve two
different purposes, and they can have different leniency
requirements.  I do not necessarily agree that it is a good idea
to tie them together like this patch.

Re: [PATCH 2/3] fsck_commit: remove duplicate tests

[Johannes Schindelin]

Hi,

On Sun, 3 Feb 2008, Junio C Hamano wrote:

> Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:
> 
> > All tests on the commit buffer in fsck_cmd are ready done by 
> > parse_commit_buffer.
> >
> > This patch rips out all redundant tests.
> 
> As I believe in belt-and-suspenders when it comes to validation, I am 
> somewhat uneasy with this change.

Besides, should we really change fsck?  It's not _that_ much of a 
performance-critical operation.  Accuracy is much more important.

fsck is the reason I trust git with my data.

Ciao,
Dscho

Re: [PATCH 2/3] fsck_commit: remove duplicate tests

[Martin Koegler]

On Mon, Feb 04, 2008 at 12:04:31AM +0000, Johannes Schindelin wrote:
> On Sun, 3 Feb 2008, Junio C Hamano wrote:
> > Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:
> > 
> > > All tests on the commit buffer in fsck_cmd are ready done by 
> > > parse_commit_buffer.
> > >
> > > This patch rips out all redundant tests.
> > 
> > As I believe in belt-and-suspenders when it comes to validation, I am 
> > somewhat uneasy with this change.
> 
> Besides, should we really change fsck?  It's not _that_ much of a 
> performance-critical operation.  Accuracy is much more important.
> 
> fsck is the reason I trust git with my data.

Then please drop this patch. I hope the other two patches are OK.

mfg Martin Kögler

Re: [PATCH 2/3] fsck_commit: remove duplicate tests

[Junio C Hamano]

mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:

> On Mon, Feb 04, 2008 at 12:04:31AM +0000, Johannes Schindelin wrote:
>> On Sun, 3 Feb 2008, Junio C Hamano wrote:
>> > Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:
>> > 
>> > > All tests on the commit buffer in fsck_cmd are ready done by 
>> > > parse_commit_buffer.
>> > >
>> > > This patch rips out all redundant tests.
>> > 
>> > As I believe in belt-and-suspenders when it comes to validation, I am 
>> > somewhat uneasy with this change.
>> 
>> Besides, should we really change fsck?  It's not _that_ much of a 
>> performance-critical operation.  Accuracy is much more important.
>> 
>> fsck is the reason I trust git with my data.
>
> Then please drop this patch. I hope the other two patches are OK.

Yeah, thanks for the other two, which I've queued already.