Something weird is happening...

I was investigating a problem that Ingo Molnar reported on the linux-2.6-tip.git repository on kernel.org. Unfortunately I was not able to reproduce his problem (which is a problem in itself) but I did run into another oddity:

: hera 4 ; git fsck

[lots of dangling commits deleted]
missing blob af0e01d4c663a101f48614e40d006ed6272d5c36

: hera 5 ; git cat-file blob af0e01d4c663a101f48614e40d006ed6272d5c36 /*
* debugfs.h - a tiny little debug file system
*
* Copyright (C) 2004 Greg Kroah-Hartman <[email protected]>
* Copyright (C) 2004 IBM Inc.
*
[... rest of blob deleted ...]

Okay, what is going on here?

Since git 1.6.1.1 just got installed on kernel.org, this phenomenon (and hopefully Ingo's problem, too) maybe has something to do with this new version?

-hpa

--
H. Peter Anvin, Intel Open Source Technology Center I work for Intel. I don't speak on their behalf.

This problem went away as i downgraded my version of Git from the 'maint' branch to a distro 1.6.0 version.

Ingo

the last 'bad' version of git that i tried was:

    v1.6.1.1-259-g8712b3c

the 'good' version is:

    git version 1.6.0.6

but i had a previous version as well, about 2 weeks older than v1.6.1.1-259-g8712b3c - and it only started triggering these problems once the kernel.org upgrade happened. I have no clean reproducer so you can ignore this - my use of versions was a bit messy.

Ingo

Is the blob borrowed from one of its alternate object store, and not in a pack? That would explain (note that I am not saying "justify" here, just "explain") the symptom.

"git fsck" unlike "git fsck --full" does not validate objects in pack nor objects you borrow from your neighbour via objects/info/alternates.

But it still does the connectivity check fully from your refs. And if it finds an object that ought to be reachable from your refs that it hasn't validated, it gives the "missing" warning above. There is a check to exclude objects in packs when running "git fsck" without --full from this warning but I do not see a corresponding exclusion of objects that were borrowed from your neighbour which we didn't check either.

But that is just a conjecture from a cursory looking at the current code. I do not remember (or did not know from the beginning) some details of it. And that is why I asked you if "git fsck --full" reports it missing in my earlier response to you.

sorry i did not follow up on that detail. I ran --full yesterday already (before we reported this), and besides taking a lot longer to finish it reported the same screenful of problems.

Should i send the output of it? (i started it, it will take some time to finish)

Ingo

I didn't get the "missing" message with git fsck --full. I get the "dangling" messages, still, but that is more or less expected.

-hpa

Yes, it is.

Just tried it, it doesn't. So I guess things are OK.

-hpa

hm, it reported missing items here - but maybe i misremembered that. Re-running it currently. (takes quite a while)

Ingo

Looks like it.

-hpa

Correct. It looks like "fsck" without --full is misreporting, which is something we should find a fix on our end.

I am not sure how old this bug is (nor if I can make Linus admit it is even a bug---I suspect the code that is involved in this did not change that much over time).

I'll happily admit that it's a bug.

I think our "fsck" logic kind of sucks - the default version really skips too damn much in the name of being fast and only look at loose objects, and --full is _so_ anal that it's silly and overkill for almost everything.

We probably should have a middle ground (that should be the default), which does at least somewhat proper reachability analysis, checks alternates, and make the current "only check local loose objects" be an option like "git fsck --loose" (or "--local").

Out current "git fsck --full" is even mis-documented. The built-in help says that it means "also consider alternate objects", but that's just a small part of what it does. It _also_ verifies the pack-files, and that's actually the really expensive part.

So "check_full" turns on _everything_, but some things that it turns on should be turned on already by default (the "alternates" directory check), while other parts of --full are just too expensive to ever be default (the pack-file check).

Linus

By default we looked at all refs but not HEAD. The only thing that made fsck not lose sight of comments that are only reachable from a detached HEAD was the reflog for the HEAD.

This fixes it, with a new test.

---

* This is unrelated to Peter's "missing blob", but should be an obviously right thing to do.

 builtin-fsck.c  |   13 ++++++++-----
 t/t1450-fsck.sh |   18 ++++++++++++++++++
 2 files changed, 26 insertions(+), 5 deletions(-)
 create mode 100755 t/t1450-fsck.sh

index aecc828..2cfff43 100644
@@ -23,6 +23,7 @@ static int check_full;
 static int check_strict;
 static int keep_cache_objects;
 static unsigned char head_sha1[20];
+static const char *head_points_at;
 static int errors_found;
 static int write_lost_and_found;
 static int verbose;

@@ -473,6 +474,8 @@ static int fsck_handle_ref(const char *refname, const unsigned char *sha1, int f
 
 static void get_default_heads(void)
 {
+	if (head_points_at && !is_null_sha1(head_sha1))
+		fsck_handle_ref("HEAD", head_sha1, 0, NULL);
 	for_each_ref(fsck_handle_ref, NULL);
 	if (include_reflogs)
 		for_each_reflog(fsck_handle_reflog, NULL);

@@ -512,14 +515,13 @@ static void fsck_object_dir(const char *path)
 
 static int fsck_head_link(void)
 {
-	unsigned char sha1[20];
 	int flag;
 	int null_is_error = 0;
-	const char *head_points_at = resolve_ref("HEAD", sha1, 0, &flag);
 
 	if (verbose)
 		fprintf(stderr, "Checking HEAD link\n");
 
+	head_points_at = resolve_ref("HEAD", head_sha1, 0, &flag);
 	if (!head_points_at)
 		return error("Invalid HEAD");
 	if (!strcmp(head_points_at, "HEAD"))

@@ -528,7 +530,7 @@ static int fsck_head_link(void)
 	else if (prefixcmp(head_points_at, "refs/heads/"))
 		return error("HEAD points to something strange (%s)",
 			     head_points_at);
-	if (is_null_sha1(sha1)) {
+	if (is_null_sha1(head_sha1)) {
 		if (null_is_error)
 			return error("HEAD: detached HEAD points at nothing");
 		fprintf(stderr, "notice: HEAD points to an unborn branch (%s)\n",

@@ -624,8 +626,9 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 	heads = 0;
 	for (i = 0; i < argc; i++) {
 		const char *arg = argv[i];
-		if (!get_sha1(arg, head_sha1)) {
-			struct object *obj = lookup_object(head_sha1);
+		unsigned char sha1[20];
+		if (!get_sha1(arg, sha1)) {
+			struct object *obj = lookup_object(sha1);
 
 			/* Error is printed by lookup_object(). */
 			if (!obj)

new file mode 100755
index 0000000..5166566
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+test_description='git fsck random collection of tests'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	test_commit A &&
+	git checkout HEAD^0 &&
+	test_commit B &&
+	git reflog expire --expire=now --all
+'
+
+test_expect_success 'HEAD is part of refs' '
+	test 0 = $(git fsck | wc -l)
+'
+
+test_done

1.6.1.2.312.g5be3c

Quoting Junio C Hamano <[email protected]>:

I think you meant to say commits, not comments.

Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

I'll amend when I fix the broken test J6t pointed out.

[...]

You are right. I forgot that test_commit creates an extra tag. By adding

	git tag -d B &&

after "test_commit B &&", you would expose the issue in the existing code.

Hi,

When fixing the revision machinery to imply HEAD with --all, I thought of fsck, too, but forgot to check if it does actually use the revision walker.

Sorry about this,
Dscho

"git fsck" used to validate only loose objects that are local and nothing else by default. This is not just too little when a repository is borrowing objects from other object stores, but also caused the connectivity check to mistakenly declare loose objects borrowed from them to be missing.

The rationale behind the default mode that validates only loose objects is because these objects are still young and more unlikely to have been pushed to other repositories yet. That holds for loose objects borrowed from alternate object stores as well.

---

* This implements your earlier suggestion. We might want to say where the corrupt object was found when fsck_sha1() reports problems, but that is an issue that already exists when the command is run with --full.

 builtin-fsck.c  |   21 ++++++++++++---------
 t/t1450-fsck.sh |   16 ++++++++++++++--
 2 files changed, 26 insertions(+), 11 deletions(-)

index 2cfff43..64dffa5 100644
@@ -586,6 +586,7 @@ static struct option fsck_opts[] = {
 int cmd_fsck(int argc, const char **argv, const char *prefix)
 {
 	int i, heads;
+	struct alternate_object_database *alt;
 
 	errors_found = 0;
 

@@ -597,17 +598,19 @@ int cmd_fsck(int argc, const char **argv, const char *prefix)
 
 	fsck_head_link();
 	fsck_object_dir(get_object_directory());
+
+	prepare_alt_odb();
+	for (alt = alt_odb_list; alt; alt = alt->next) {
+		char namebuf[PATH_MAX];
+		int namelen = alt->name - alt->base;
+		memcpy(namebuf, alt->base, namelen);
+		namebuf[namelen - 1] = 0;
+		fsck_object_dir(namebuf);
+	}
+
 	if (check_full) {
-		struct alternate_object_database *alt;
 		struct packed_git *p;
-		prepare_alt_odb();
-		for (alt = alt_odb_list; alt; alt = alt->next) {
-			char namebuf[PATH_MAX];
-			int namelen = alt->name - alt->base;
-			memcpy(namebuf, alt->base, namelen);
-			namebuf[namelen - 1] = 0;
-			fsck_object_dir(namebuf);
-		}
+
 		prepare_packed_git();
 		for (p = packed_git; p; p = p->next)
 			/* verify gives error messages itself */

index 5166566..6e64e44 100755
@@ -5,9 +5,9 @@ test_description='git fsck random collection of tests'
 . ./test-lib.sh
 
 test_expect_success setup '
-	test_commit A &&
+	test_commit A fileA one &&
 	git checkout HEAD^0 &&
-	test_commit B &&
+	test_commit B fileB two &&
 	git reflog expire --expire=now --all
 '
 

@@ -15,4 +15,16 @@ test_expect_success 'HEAD is part of refs' '
 	test 0 = $(git fsck | wc -l)
 '
 
+test_expect_success 'loose objects borrowed from alternate are not missing' '
+	mkdir another &&
+	(
+		cd another &&
+		git init &&
+		echo ../../../.git/objects >.git/objects/info/alternates &&
+		test_commit C fileC one &&
+		git fsck >out &&
+		! grep "missing blob" out
+	)
+'
+
 test_done

1.6.1.2.312.g5be3c

Does "git fsck --full" report the blob missing?

hm - maybe it's unrelated, but yesterday, a few hours before the incident i did a higher-order Octopus merge with 1.6.1, that segfaulted.

I didnt think much of it - git often crashes with our crazy -tip tree when i get above the magic 20 branches limit. The crash left a .git/index.lock file around which i removed - and then forgot about the incident. I wanted to report those crashes before but procrastinated it.

I used git/maint snapshots because new versions of Git are much better at doing Octopus merges.

I've written a reproducer for git-1.6.1.1-1.fc11.i386. See the crash log below.

To reproduce, pick up the -tip tree as a remote:

http://people.redhat.com/mingo/tip.git/README

(you can run the README file as a script)

Then do this:

git checkout -b tmp.crash v2.6.29-rc3

   git merge  tip/x86/asm        tip/x86/cleanups     tip/x86/cpudetect    \
              tip/x86/debug      tip/x86/doc          tip/x86/header-fixes \
              tip/x86/mm         tip/x86/paravirt     tip/x86/pat          \
              tip/x86/setup-v2   tip/x86/subarch      tip/x86/uaccess      \
              tip/x86/urgent     tip/core/percpu

and you should see the segfault.

interestingly i did not reproduce with the sha1's hardcoded:

git checkout -b tmp.crash 18e352e4a73465349711a9324767e1b2453383e2

git merge 2d4d57db692ea790e185656516e6ebe8791f1788 a448720ca3248e8a7a426336885549d6e923fd8e b38b0665905538e76e26f2a4c686179abb1f69f6 d5e397cb49b53381e4c99a064ca733c665646de8 e56d0cfe7790fd3218ae4f6aae1335547fea8763 dbca1df48e89d8aa59254fdc10ef16c16e73d94e fb746d0e1365b7472ccc4c3d5b0672b34a092d0b 6522869c34664dd5f05a0a327e93915b1281c90d d639bab8da86d330493487e8c0fea8ca31f53427 042cbaf88ab48e11afb725541e3c2cbf5b483680 5662a2f8e7313f78d6b17ab383f3e4f04971c335 3b4b75700a245d0d48fc52a4d2f67d3155812aba bf3647c44bc76c43c4b2ebb4c37a559e899ac70e 4369f1fb7cd4cf777312f43e1cb9aa5504fc4125

Ingo

-------------------->
earth4:~/tip> git merge x86/asm x86/cleanups x86/cpudetect x86/debug x86/doc x86/header-fixes x86/mm x86/paravirt x86/pat x86/setup-v2 x86/subarch x86/uaccess x86/urgent core/percpu
Trying simple merge with 2d4d57db692ea790e185656516e6ebe8791f1788 Trying simple merge with a448720ca3248e8a7a426336885549d6e923fd8e Simple merge did not work, trying automatic merge. Auto-merging arch/x86/include/asm/io.h
Auto-merging arch/x86/include/asm/spinlock.h
Auto-merging arch/x86/kernel/mpparse.c
Auto-merging arch/x86/kernel/setup_percpu.c
Auto-merging arch/x86/mm/init_32.c
Trying simple merge with b38b0665905538e76e26f2a4c686179abb1f69f6 Simple merge did not work, trying automatic merge. Auto-merging arch/x86/kernel/cpu/common.c
Auto-merging arch/x86/kernel/cpu/intel.c
Auto-merging arch/x86/mm/pat.c
Trying simple merge with d5e397cb49b53381e4c99a064ca733c665646de8 Trying simple merge with e56d0cfe7790fd3218ae4f6aae1335547fea8763 Trying simple merge with dbca1df48e89d8aa59254fdc10ef16c16e73d94e Trying simple merge with fb746d0e1365b7472ccc4c3d5b0672b34a092d0b Trying simple merge with 6522869c34664dd5f05a0a327e93915b1281c90d Simple merge did not work, trying automatic merge. Auto-merging arch/x86/include/asm/paravirt.h
Trying simple merge with d639bab8da86d330493487e8c0fea8ca31f53427 Simple merge did not work, trying automatic merge. Auto-merging arch/x86/include/asm/io.h
Auto-merging arch/x86/mm/ioremap.c
Trying simple merge with 042cbaf88ab48e11afb725541e3c2cbf5b483680 Trying simple merge with 5662a2f8e7313f78d6b17ab383f3e4f04971c335 Trying simple merge with 3b4b75700a245d0d48fc52a4d2f67d3155812aba Simple merge did not work, trying automatic merge. Auto-merging arch/x86/kernel/signal.c
Trying simple merge with bf3647c44bc76c43c4b2ebb4c37a559e899ac70e Simple merge did not work, trying automatic merge. Auto-merging arch/x86/kernel/cpu/intel.c
Trying simple merge with 4369f1fb7cd4cf777312f43e1cb9aa5504fc4125 /usr/libexec/git-core/git-merge-octopus: line 52: 26758 Segmentation fault git read-tree -u -m --aggressive $common $MRT $SHA1
Merge with strategy octopus failed.
earth4:~/tip>

Current master (a34a9dbbc) with the following patch should not segfault anymore (it doesn't here), but I don't know where the magic number six is coming from. :-/

index 16bc2ca..4b02fbf 100644
@@ -8,6 +8,8 @@
 #include "progress.h"
 #include "refs.h"
 
+#define STAGES (5 + 1)
+
 /*
  * Error messages expected by scripts out of plumbing commands such as
  * read-tree.  Non-scripted Porcelain is not required to use these messages

@@ -143,7 +145,7 @@ static inline int call_unpack_fn(struct cache_entry **src, struct unpack_trees_o
 
 static int unpack_index_entry(struct cache_entry *ce, struct unpack_trees_options *o)
 {
-	struct cache_entry *src[5] = { ce, };
+	struct cache_entry *src[STAGES] = { ce, };
 
 	o->pos++;
 	if (ce_stage(ce)) {

@@ -240,7 +242,7 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info, con
 	return ce;
 }
 
-static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmask, struct cache_entry *src[5],
+static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmask, struct cache_entry *src[STAGES],
 	const struct name_entry *names, const struct traverse_info *info)
 {
 	int i;

@@ -291,7 +293,7 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas
 
 static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)
 {
-	struct cache_entry *src[5] = { NULL, };
+	struct cache_entry *src[STAGES] = { NULL, };
 	struct unpack_trees_options *o = info->data;
 	const struct name_entry *p = names;
 

@@ -728,7 +730,7 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
 {
 	struct cache_entry *index;
 	struct cache_entry *head;
-	struct cache_entry *remote = stages[o->head_idx + 1];
+	struct cache_entry *remote;
 	int count;
 	int head_match = 0;
 	int remote_match = 0;

@@ -755,6 +757,9 @@ int threeway_merge(struct cache_entry **stages, struct unpack_trees_options *o)
 		head = NULL;
 	}
 
+	if (o->head_idx + 1 >= STAGES)
+		die("Too many stages (internal error)");
+	remote = stages[o->head_idx + 1];
 	if (remote == o->df_conflict_entry) {
 		df_conflict_remote = 1;
 		remote = NULL;

Looks like octopus is using multiple merge bases directly, rather than combining them using merge-recursive. The merge-recursive concept turned out to be superior, and we kind of left multiple merge base support hanging. We should probably remove the old support and switch octopus to the better way.

My guess is that 6 is just that there happen to be 3 merge bases for this particular merge (plus the merge results and the two sides).

-Daniel
*This .sig left intentionally blank*

The following on top of master lets git finish the merge without segfault (reporting a merge conflict) and valgrind doesn't find an more memory errors here. Can you confirm that, Ingo?

-- 8< --
The parameter n of unpack_callback() can have a value of up to MAX_UNPACK_TREES. The check at the top of unpack_trees() (its only (indirect) caller) makes sure it cannot exceed this limit.

unpack_callback() passes it and the array src to unpack_nondirectories(), which has this loop:

	for (i = 0; i < n; i++) {
		/* ... */
		src[i + o->merge] = o->df_conflict_entry;

o->merge can be 0 or 1, so unpack_nondirectories() potentially accesses the array src at index MAX_UNPACK_TREES. This patch makes it big enough.

---
 unpack-trees.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

index 16bc2ca..e547282 100644
@@ -240,8 +240,11 @@ static struct cache_entry *create_ce_entry(const struct traverse_info *info, con
 	return ce;
 }
 
-static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmask, struct cache_entry *src[5],
-	const struct name_entry *names, const struct traverse_info *info)
+static int unpack_nondirectories(int n, unsigned long mask,
+				 unsigned long dirmask,
+				 struct cache_entry **src,
+				 const struct name_entry *names,
+				 const struct traverse_info *info)
 {
 	int i;
 	struct unpack_trees_options *o = info->data;

@@ -291,7 +294,7 @@ static int unpack_nondirectories(int n, unsigned long mask, unsigned long dirmas
 
 static int unpack_callback(int n, unsigned long mask, unsigned long dirmask, struct name_entry *names, struct traverse_info *info)
 {
-	struct cache_entry *src[5] = { NULL, };
+	struct cache_entry *src[MAX_UNPACK_TREES + 1] = { NULL, };
 	struct unpack_trees_options *o = info->data;
 	const struct name_entry *p = names;
 

1.6.1

Is this perhaps related to the problem the github guys were having, its sounds similar, problem not showing up until after upgrade, missing objects, no?

Cheers,

Sverre Rabbelier

I do not think so. The push problem does not lead to any repository corruption, and does not have anything to do with the fsck misreporting a blob that exists in an alternate object store as missing, either.

.