[PATCH v3] builtin-fsck: reports missing parent commits

[PATCH v3] builtin-fsck: reports missing parent commits

[Martin Koegler]

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

diff --git a/builtin-fsck.c b/builtin-fsck.c
index 512346a..fae7e22 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -394,6 +394,8 @@ static int fsck_commit(struct commit *commit)
 {
 	char *buffer = commit->buffer;
 	unsigned char tree_sha1[20], sha1[20];
+	struct commit_graft *graft;
+	int parents = 0;
 
 	if (verbose)
 		fprintf(stderr, "Checking commit %s\n",
@@ -411,6 +413,28 @@ static int fsck_commit(struct commit *commit)
 		if (get_sha1_hex(buffer+7, sha1) || buffer[47] != '\n')
 			return objerror(&commit->object, "invalid 'parent' line format - bad sha1");
 		buffer += 48;
+		parents++;
+	}
+	graft = lookup_commit_graft(commit->object.sha1);
+	if (graft) {
+		struct commit_list *p = commit->parents;
+		parents = 0;
+		while (p) {
+			p = p->next;
+			parents++;
+		}
+		if (graft->nr_parent == -1 && !parents)
+			; /* shallow commit */
+		else if (graft->nr_parent != parents)
+			return objerror(&commit->object, "graft objects missing");
+	} else {
+		struct commit_list *p = commit->parents;
+		while (p && parents) {
+			p = p->next;
+			parents--;
+		}
+		if (p || parents)
+			return objerror(&commit->object, "parent objects missing");
 	}
 	if (memcmp(buffer, "author ", 7))
 		return objerror(&commit->object, "invalid format - expected 'author' line");
diff --git a/commit.c b/commit.c
index 6684c4e..94d5b3d 100644
--- a/commit.c
+++ b/commit.c
@@ -193,7 +193,7 @@ static void prepare_commit_graft(void)
 	commit_graft_prepared = 1;
 }
 
-static struct commit_graft *lookup_commit_graft(const unsigned char *sha1)
+struct commit_graft *lookup_commit_graft(const unsigned char *sha1)
 {
 	int pos;
 	prepare_commit_graft();
diff --git a/commit.h b/commit.h
index 80d65b9..a1e9591 100644
--- a/commit.h
+++ b/commit.h
@@ -116,6 +116,7 @@ struct commit_graft {
 struct commit_graft *read_graft_line(char *buf, int len);
 int register_commit_graft(struct commit_graft *, int);
 int read_graft_file(const char *graft_file);
+struct commit_graft *lookup_commit_graft(const unsigned char *sha1);
 
 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2, int cleanup);
 
-- 
1.5.4.2.g01825.dirty

Re: [PATCH v3] builtin-fsck: reports missing parent commits

[Junio C Hamano]

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

> Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>

As far as I can tell, the new test is not testing the commit
object we are looking at from the object database.  What it is
testing is if the code that parsed and prepared the information
in "struct commit" found the same number of parents an extra
check we are doing here by hand (if not grafted --- but
presumably whoever gave the struct commit we are handling here
would have obtained that information by doing the same parsing),
or the parsing of the graft file (when grafted --- but
presumably whoever gave the struct commit we are handling here
would have obtained that information by calling the same
llokup_commit_graft()).

So I am not sure what problems in the repository objects these
new checks are designed to catch.

This needs a lot of explanation than what's in your commit log
message.

Re: [PATCH v3] builtin-fsck: reports missing parent commits

[Martin Koegler]

On Sun, Feb 24, 2008 at 07:08:52PM -0800, Junio C Hamano wrote:
> Martin Koegler <mkoegler@auto.tuwien.ac.at> writes:
> As far as I can tell, the new test is not testing the commit
> object we are looking at from the object database.  What it is
> testing is if the code that parsed and prepared the information
> in "struct commit" found the same number of parents an extra
> check we are doing here by hand (if not grafted --- but
> presumably whoever gave the struct commit we are handling here
> would have obtained that information by doing the same parsing),
> or the parsing of the graft file (when grafted --- but
> presumably whoever gave the struct commit we are handling here
> would have obtained that information by calling the same
> llokup_commit_graft()).
> 
> So I am not sure what problems in the repository objects these
> new checks are designed to catch.
> 
> This needs a lot of explanation than what's in your commit log
> message.

If we have already parsed a non-commit object and parse_commit_buffer
hits such a sha1 in a parent line, it simply drops it (same for grafts).

I hope that you agree with me, that fsck should catch such an error.

As you don't want to tighten parse_commit_buffer, it must be in fsck.

mfg Martin Kögler

Re: [PATCH v3] builtin-fsck: reports missing parent commits

[Junio C Hamano]

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

> On Sun, Feb 24, 2008 at 07:08:52PM -0800, Junio C Hamano wrote:
> ...
>> So I am not sure what problems in the repository objects these
>> new checks are designed to catch.
>> 
>> This needs a lot of explanation than what's in your commit log
>> message.
>
> If we have already parsed a non-commit object and parse_commit_buffer
> hits such a sha1 in a parent line, it simply drops it (same for grafts).
>
> I hope that you agree with me, that fsck should catch such an error.

See?  That's exactly my point.

You would need to _explain that_ in your commit log message.
Who on the list other than you noticed that it is what you are
checking?