threads / patch / 11860

patch, 3 partsgit-fsck: report missing author/commit line in a commit as an error

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

## tl;dr

7 messages between Feb 3, 2008 and Feb 4, 2008. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Martin Koegler· Feb 3, 2008, 21:22 UTC · lore
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(-)
Show changes to builtin-fsck.c +3 −3
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
Martin Koegler· Feb 3, 2008, 21:22 UTC · re: Martin Koegler · lore

[PATCH 2/3] fsck_commit: remove duplicate tests

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(-)
Show changes to builtin-fsck.c +4 −9
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
Martin Koegler· Feb 3, 2008, 21:22 UTC · re: Martin Koegler · lore

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

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(-)
Show changes to object.c +8 −4
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
Junio C Hamano· Feb 3, 2008, 23:58 UTC · re: Martin Koegler · lore

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

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.

Johannes Schindelin· Feb 4, 2008, 00:04 UTC · re: Junio C Hamano · lore

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

Hi,
On Sun, 3 Feb 2008, Junio C Hamano wrote:
Show 9 quoted lines
> 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

Martin Koegler· Feb 4, 2008, 07:06 UTC · re: Johannes Schindelin · lore

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

On Mon, Feb 04, 2008 at 12:04:31AM +0000, Johannes Schindelin wrote:
Show 15 quoted lines
> 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
Junio C Hamano· Feb 4, 2008, 07:33 UTC · re: Martin Koegler · lore

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

mkoegler@auto.tuwien.ac.at (Martin Koegler) writes:
Show 18 quoted lines
> 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.

← back to recent threads