threads / patch / 12300

v3builtin-fsck: reports missing parent commits

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

## tl;dr

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

replies: 3people: 2as markdown or json

Martin Koegler· Feb 24, 2008, 20:58 UTC · lore

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(-)

Show changes to 3 files +26 −1

builtin-fsck.c, commit.c, commit.h

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

Junio C Hamano· Feb 25, 2008, 03:08 UTC · re: Martin Koegler · lore

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

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.

Martin Koegler· Feb 25, 2008, 07:16 UTC · re: Junio C Hamano · lore

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

On Sun, Feb 24, 2008 at 07:08:52PM -0800, Junio C Hamano wrote:

Show 18 quoted lines

> 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

Junio C Hamano· Feb 25, 2008, 07:24 UTC · re: Martin Koegler · lore

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

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

Show 12 quoted lines

> 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?