Something weird is happening...

Something weird is happening...

[H. Peter Anvin]

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 <greg@kroah.com>
 *  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.

Re: Something weird is happening...

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> 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 <greg@kroah.com>
>  *  Copyright (C) 2004 IBM Inc.
>  *
> [... rest of blob deleted ...]
>
> Okay, what is going on here?

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

Re: Something weird is happening...

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> a simple 'git fsck' still reports a ton of dangling and missing objects. 
> (see it below)
> 
> amongst them is the object above:
> 
>   missing blob af0e01d4c663a101f48614e40d006ed6272d5c36
> 
> but this does not seem to cause problems with git version 1.6.0.6 that 
> i'm using now.

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> 

Re: Something weird is happening...

[René Scharfe]

Ingo Molnar schrieb:
> * Ingo Molnar <mingo@elte.hu> wrote:
> 
>> a simple 'git fsck' still reports a ton of dangling and missing objects. 
>> (see it below)
>>
>> amongst them is the object above:
>>
>>   missing blob af0e01d4c663a101f48614e40d006ed6272d5c36
>>
>> but this does not seem to cause problems with git version 1.6.0.6 that 
>> i'm using now.
> 
> 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. :-/

diff --git a/unpack-trees.c b/unpack-trees.c
index 16bc2ca..4b02fbf 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -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;

Re: Something weird is happening...

[Daniel Barkalow]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 700 bytes --]

On Thu, 29 Jan 2009, René Scharfe wrote:

> 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. :-/

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*

[PATCH] merge: fix out-of-bounds memory access

[René Scharfe]

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.

Reported-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 unpack-trees.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/unpack-trees.c b/unpack-trees.c
index 16bc2ca..e547282 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -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

Re: Something weird is happening...

[Sverre Rabbelier]

On Thu, Jan 29, 2009 at 07:56, Junio C Hamano <gitster@pobox.com> wrote:
>> Okay, what is going on here?

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

Re: Something weird is happening...

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Thu, Jan 29, 2009 at 07:56, Junio C Hamano <gitster@pobox.com> wrote:
>>> Okay, what is going on here?
>
> 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?

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.

.

Re: Something weird is happening...

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

> 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

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

	Ingo

Re: Something weird is happening...

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * H. Peter Anvin <hpa@zytor.com> wrote:
> 
> > 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
> 
> This problem went away as i downgraded my version of Git from the 
> 'maint' branch to a distro 1.6.0 version.

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

Re: Something weird is happening...

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> 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 <greg@kroah.com>
>  *  Copyright (C) 2004 IBM Inc.
>  *
> [... rest of blob deleted ...]
>
> Okay, what is going on here?

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.

Re: Something weird is happening...

[H. Peter Anvin]

Junio C Hamano wrote:
> 
> 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.
> 

Yes, it is.

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

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

	-hpa

Re: Something weird is happening...

[Ingo Molnar]

* H. Peter Anvin <hpa@zytor.com> wrote:

>> 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.
>
> Just tried it, it doesn't.  So I guess things are OK.

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

	Ingo

Re: Something weird is happening...

[H. Peter Anvin]

Ingo Molnar wrote:
> 
> So the repo itself is fine, correct?
> 

Looks like it.

	-hpa

Re: Something weird is happening...

[Junio C Hamano]

"H. Peter Anvin" <hpa@zytor.com> writes:

> Ingo Molnar wrote:
>>
>> So the repo itself is fine, correct?
>>
>
> Looks like it.

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

Re: Something weird is happening...

[Linus Torvalds]

On Thu, 29 Jan 2009, Junio C Hamano wrote:
> 
> 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

[PATCH 1/2] fsck: HEAD is part of refs

[Junio C Hamano]

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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * 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

diff --git a/builtin-fsck.c b/builtin-fsck.c
index aecc828..2cfff43 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -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)
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
new file mode 100755
index 0000000..5166566
--- /dev/null
+++ b/t/t1450-fsck.sh
@@ -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

Re: [PATCH 1/2] fsck: HEAD is part of refs

[Johannes Sixt]

Junio C Hamano schrieb:
> 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.
[...]
> +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)

I'm scratching my head over this. This test succeeds even without the
change... (Tested with a34a9db.)

-- Hannes

Re: [PATCH 1/2] fsck: HEAD is part of refs

[Junio C Hamano]

Johannes Sixt <j.sixt@viscovery.net> writes:

> Junio C Hamano schrieb:
>> 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.
> [...]
>> +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)
>
> I'm scratching my head over this. This test succeeds even without the
> change... (Tested with a34a9db.)

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.

Re: [PATCH 1/2] fsck: HEAD is part of refs

[Johannes Schindelin]

Hi,

On Fri, 30 Jan 2009, Junio C Hamano wrote:

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

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

Re: [PATCH 1/2] fsck: HEAD is part of refs

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>:

> 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.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

I think you meant to say commits, not comments.

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

Re: [PATCH 1/2] fsck: HEAD is part of refs

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Junio C Hamano <gitster@pobox.com>:
>
>> 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.
>>
>> Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> I think you meant to say commits, not comments.

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

[PATCH 2/2] fsck: check loose objects from alternate object stores by default

[Junio C Hamano]

"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.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

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

diff --git a/builtin-fsck.c b/builtin-fsck.c
index 2cfff43..64dffa5 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -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 */
diff --git a/t/t1450-fsck.sh b/t/t1450-fsck.sh
index 5166566..6e64e44 100755
--- a/t/t1450-fsck.sh
+++ b/t/t1450-fsck.sh
@@ -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

Re: Something weird is happening...

[Ingo Molnar]

* Junio C Hamano <gitster@pobox.com> wrote:

> 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

Re: Something weird is happening...

[H. Peter Anvin]

Ingo Molnar wrote:
> * Junio C Hamano <gitster@pobox.com> wrote:
> 
>> 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)

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