# [PATCH 1/3] git-fsck: report missing author/commit line in a commit as an error 7 messages from 2008-02-03 to 2008-02-04. Participants: Martin Koegler, Junio C Hamano, Johannes Schindelin. Thread: https://gitlist.dev/t/11860 ## Martin Koegler, 2008-02-03 21:22 Subject: [PATCH 1/3] git-fsck: report missing author/commit line in a commit as an error Message-ID: <12020737593747-git-send-email-mkoegler@auto.tuwien.ac.at> URL: https://gitlist.dev/e/12020737593747-git-send-email-mkoegler%40auto.tuwien.ac.at ``` 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 --- 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 ``` ## Martin Koegler, 2008-02-03 21:22 Subject: [PATCH 2/3] fsck_commit: remove duplicate tests Message-ID: <12020737601683-git-send-email-mkoegler@auto.tuwien.ac.at> URL: https://gitlist.dev/e/12020737601683-git-send-email-mkoegler%40auto.tuwien.ac.at In-Reply-To: <12020737593747-git-send-email-mkoegler@auto.tuwien.ac.at> ``` 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 --- 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 ``` ## Martin Koegler, 2008-02-03 21:22 Subject: [PATCH 3/3] parse_object_buffer: don't ignore errors from the object specific parsing functions Message-ID: <12020737603543-git-send-email-mkoegler@auto.tuwien.ac.at> URL: https://gitlist.dev/e/12020737603543-git-send-email-mkoegler%40auto.tuwien.ac.at In-Reply-To: <12020737601683-git-send-email-mkoegler@auto.tuwien.ac.at> ``` 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 --- 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 ``` ## Junio C Hamano, 2008-02-03 23:58 Subject: Re: [PATCH 2/3] fsck_commit: remove duplicate tests Message-ID: <7v3as9r3r2.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7v3as9r3r2.fsf%40gitster.siamese.dyndns.org In-Reply-To: <12020737601683-git-send-email-mkoegler@auto.tuwien.ac.at> ``` Martin Koegler 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, 2008-02-04 00:04 Subject: Re: [PATCH 2/3] fsck_commit: remove duplicate tests Message-ID: URL: https://gitlist.dev/e/alpine.LSU.1.00.0802040003360.7372%40racer.site In-Reply-To: <7v3as9r3r2.fsf@gitster.siamese.dyndns.org> ``` Hi, On Sun, 3 Feb 2008, Junio C Hamano wrote: > Martin Koegler 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, 2008-02-04 07:06 Subject: Re: [PATCH 2/3] fsck_commit: remove duplicate tests Message-ID: <20080204070600.GA30099@auto.tuwien.ac.at> URL: https://gitlist.dev/e/20080204070600.GA30099%40auto.tuwien.ac.at In-Reply-To: ``` On Mon, Feb 04, 2008 at 12:04:31AM +0000, Johannes Schindelin wrote: > On Sun, 3 Feb 2008, Junio C Hamano wrote: > > Martin Koegler 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, 2008-02-04 07:33 Subject: Re: [PATCH 2/3] fsck_commit: remove duplicate tests Message-ID: <7vodaxkwgd.fsf@gitster.siamese.dyndns.org> URL: https://gitlist.dev/e/7vodaxkwgd.fsf%40gitster.siamese.dyndns.org In-Reply-To: <20080204070600.GA30099@auto.tuwien.ac.at> ``` 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 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. ```