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

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

[Martin Koegler]

Report a commit date (returned by parse_commit), if it is zero
as an error (instead by a printf).

A zero date means missing author/commiter line or a totally
corrupted commiter line.

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

diff --git a/builtin-fsck.c b/builtin-fsck.c
index e4874f6..c4e91bf 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -360,6 +360,10 @@ 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/commiter line in %s",
+				sha1_to_hex(commit->object.sha1));
+
 	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 +382,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;
 }
 
-- 
gitgui.0.9.1.gdeb16

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

[Johannes Schindelin]

Hi,

On Sun, 20 Jan 2008, Martin Koegler wrote:

> +	if (!commit->date)
> +		return objerror(&commit->object, "invalid author/commiter line in %s",

s/commiter/committer/

It makes me wonder, though, if that's correct.  AFAICT it is the committer 
date, and the rest of the line _might_ be just fine.

Why not be less intrusive and just change the printf() into a return 
objerror(), like the commit message suggests?

Ciao,
Dscho

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

[Martin Koegler]

On Sun, Jan 20, 2008 at 07:20:54PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Sun, 20 Jan 2008, Martin Koegler wrote:
> 
> > +	if (!commit->date)
> > +		return objerror(&commit->object, "invalid author/commiter line in %s",
> 
> s/commiter/committer/

Thanks. Will change in next version.

> It makes me wonder, though, if that's correct.  AFAICT it is the committer 
> date, and the rest of the line _might_ be just fine.
>
> Why not be less intrusive and just change the printf() into a return 
> objerror(), like the commit message suggests?

parse_commit_date returns zero, if
* the author line is missing
* the author line is not terminated with \n
* the committer line is missing
* the committer line does not contain >
* the > is not followed by a number bigger than zero
* the commiter line is not terminated with \n

So, in my option the message "bad commit date in %s" is incorrect/misleading.

fsck_commit could be further simplified:

//  If tree is missing, parse_commit should return zero => fsck_commit would not be called 
-        if (memcmp(buffer, "tree ", 5))
-                return objerror(&commit->object, "invalid format - expected 'tree' line");
//  If sha1 is missing/incorrect, parse_commit should return zero => fsck_commit would not be called 
-        if (get_sha1_hex(buffer+5, tree_sha1) || buffer[45] != '\n')
-                return objerror(&commit->object, "invalid 'tree' line format - bad sha1");
         buffer += 46;
         while (!memcmp(buffer, "parent ", 7)) {
//  If sha1 is missing/incorrect, parse_commit should return zero => fsck_commit would not be called 
-                if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
-                        return objerror(&commit->object, "invalid 'parent' line format - bad sha1");
                buffer += 48;
         }
         if (memcmp(buffer, "author ", 7))
                 return objerror(&commit->object, "invalid format - expected 'author' line");

I'm not sure, if this 6 lines should be dropped too.

The memcmp(buffer, "author ", 7) check is already done by
(!commit->date), but I prefer to keep it, as further committer/author
line checks can be based on it.

mfg Martin Kögler
PS:
While writting this mail, I noticed that parse_object_buffer does not
check the return value of parse_XX_buffer. I'll send a patch, which
correct this.