gc considered dangerous

gc considered dangerous

[Robin Rosenberg]

I've seen this. Running git gc on Windows, while having Elipse open can kill your
object database. 

Eclipse keeps the pack files open most of the time. Then you 
launch git gui which recommends the user to do a git gc. I never
do (it *always* wants to do this), so I haven't encountered the 
issue, but if gc doesn't find a new optimal pack it tries to rewrite a 
new pack with the same id. So it rm's the idx file (fine) and the the
pack file (not ok) and gives up, which means it has a .pack file with 
no index, so it cannot use it. Trying git gc again after eclipse exits 
will execute the final stab on your objects. 

The underlying bug is ofcource that Windows locks files when
they are open. A *nix* user does not suffer from this problem.

I'll investigate more at some other time. This is a preliminary
analysis.

-- robin

Re: gc considered dangerous

[Robin Rosenberg]

söndag 08 februari 2009 03:47:25 skrev Robin Rosenberg:
> 
> I've seen this. Running git gc on Windows, while having Elipse open can kill your
> object database. 
> 
> Eclipse keeps the pack files open most of the time. Then you 
> launch git gui which recommends the user to do a git gc. I never
> do (it *always* wants to do this), so I haven't encountered the 
> issue, but if gc doesn't find a new optimal pack it tries to rewrite a 
> new pack with the same id. So it rm's the idx file (fine) and the the
> pack file (not ok) and gives up, which means it has a .pack file with 
> no index, so it cannot use it. Trying git gc again after eclipse exits 
> will execute the final stab on your objects. 
> 
> The underlying bug is ofcource that Windows locks files when
> they are open. A *nix* user does not suffer from this problem.
> 
> I'll investigate more at some other time. This is a preliminary
> analysis.

A receipe for triggering is available at http://code.google.com/p/msysgit/issues/detail?id=189

-- robin

Re: gc considered dangerous

[Johannes Schindelin]

Hi,

On Sun, 8 Feb 2009, Robin Rosenberg wrote:

> I've seen this. Running git gc on Windows, while having Elipse open can 
> kill your object database.

You had me really scared with the mail subject!

> if gc doesn't find a new optimal pack it tries to rewrite a new pack 
> with the same id. So it rm's the idx file (fine) and the the pack file 
> (not ok) and gives up,

I disagree with your notion that it is fine to kill the existing idx file 
until the new one has been written successfully.

My preliminary guess is that this code in pack-write.c needs to use the 
lock file paradigm:

        if (!index_name) {
		[...]
        } else {
                unlink(index_name);
                fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
        }

Ciao,
Dscho

Re: gc considered dangerous

[Robin Rosenberg]

söndag 08 februari 2009 15:56:38 skrev Johannes Schindelin:
> Hi,
> 
> On Sun, 8 Feb 2009, Robin Rosenberg wrote:
> 
> > I've seen this. Running git gc on Windows, while having Elipse open can 
> > kill your object database.
> 
> You had me really scared with the mail subject!

Losing a repo *is* scary., especially if your boss is the one losing it.
That should simply not happen unless there is a disk failure. Fortunately
I believe the two lost branches did not contain anything useful.

> > if gc doesn't find a new optimal pack it tries to rewrite a new pack 
> > with the same id. So it rm's the idx file (fine) and the the pack file 
> > (not ok) and gives up,
> 
> I disagree with your notion that it is fine to kill the existing idx file 
> until the new one has been written successfully.
Then you misunderstood me. I meant that the repack script thought
it was fine.

> My preliminary guess is that this code in pack-write.c needs to use the 
> lock file paradigm:

> 
>         if (!index_name) {
> 		[...]
>         } else {
>                 unlink(index_name);
>                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
>         }

Yeah, it needs to check that it can actually delete both files before actually
doing it. Then again if it wants to replace a file with a new one just like it,
why shouldn't it just skip it. Isn't that the only case I see where we can lose
data?

GC should also notice that something is not right when there are old-files
present.

-- robin

Re: gc considered dangerous

[Johannes Schindelin]

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

Hi,

On Sun, 8 Feb 2009, Robin Rosenberg wrote:

> söndag 08 februari 2009 15:56:38 skrev Johannes Schindelin:
> 
> > On Sun, 8 Feb 2009, Robin Rosenberg wrote:
> > 
> > > I've seen this. Running git gc on Windows, while having Elipse open 
> > > can kill your object database.
> > 
> > You had me really scared with the mail subject!
> 
> Losing a repo *is* scary., especially if your boss is the one losing it. 
> That should simply not happen unless there is a disk failure. 
> Fortunately I believe the two lost branches did not contain anything 
> useful.

If you care about your data, you don't use Windows, IMHO.

> > > if gc doesn't find a new optimal pack it tries to rewrite a new pack 
> > > with the same id. So it rm's the idx file (fine) and the the pack 
> > > file (not ok) and gives up,
> > 
> > I disagree with your notion that it is fine to kill the existing idx 
> > file until the new one has been written successfully.
>
> Then you misunderstood me. I meant that the repack script thought it was 
> fine.
> 
> > My preliminary guess is that this code in pack-write.c needs to use the 
> > lock file paradigm:
> 
> > 
> >         if (!index_name) {
> > 		[...]
> >         } else {
> >                 unlink(index_name);
> >                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
> >         }
> 
> Yeah, it needs to check that it can actually delete both files before 
> actually doing it.

Nope.  It needs to avoid unlinking until it knows what it did was fine.

> Then again if it wants to replace a file with a new one just like it, 

It might have the same name, but that does not mean that it is bytewise 
identical.  The name depends on the uncompressed objects.

> Isn't that the only case I see where we can lose data?

No, you said so yourself: Eclipse prevents the pack from being deleted.  
There are a gazillion idiotic programs on Windows who think they should 
lock random files.

So you could hit the very same issue even if your repository has twenty 
packs, and therefore could do with some gc'ing.

Ciao,
Dscho

Re: gc considered dangerous

[Steffen Prohaska]

On Feb 8, 2009, at 5:04 PM, Robin Rosenberg wrote:

> söndag 08 februari 2009 15:56:38 skrev Johannes Schindelin:
>> Hi,
>>
>> On Sun, 8 Feb 2009, Robin Rosenberg wrote:
>>
>>> I've seen this. Running git gc on Windows, while having Elipse  
>>> open can
>>> kill your object database.
>>
>> You had me really scared with the mail subject!
>
> Losing a repo *is* scary., especially if your boss is the one losing  
> it.
> That should simply not happen unless there is a disk failure.  
> Fortunately
> I believe the two lost branches did not contain anything useful.


If you have a backup of the corrupted repository right after it got
corrupted, you can create an index by running "git index-pack" on
the pack that is lacking an index.  This might rescue the lost
branches.

	Steffen

Re: gc considered dangerous

[Robin Rosenberg]

söndag 08 februari 2009 19:24:33 skrev Steffen Prohaska:
> If you have a backup of the corrupted repository right after it got
> corrupted, you can create an index by running "git index-pack" on
> the pack that is lacking an index.  This might rescue the lost
> branches.

Yep, but the problem was that git gc was run again on the corrupted repo
so there was only the idx left. (L)Users...

Couldn't we make repack refuse to repack if it discovered left-overs from
a failed repack, or at least make it use the old pack files as reference instead
of ignoring them? Leaving too many objects is, after all, much better than
destroying the repo.

-- robin

Re: gc considered dangerous

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> My preliminary guess is that this code in pack-write.c needs to use the 
> lock file paradigm:
>
>         if (!index_name) {
> 		[...]
>         } else {
>                 unlink(index_name);
>                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
>         }

Whoa.  That particular code has been (and is still) correct.

When repacking we should pack into a temporary pack and idx file and then
replace the real ones after both new pack and its idx are successfully
written, and I thought that is how we've been doing this all the time.
Maybe the caller has been broken at some point?  Sigh...

Re: gc considered dangerous

[Robin Rosenberg]

söndag 08 februari 2009 21:00:57 skrev Junio C Hamano:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > My preliminary guess is that this code in pack-write.c needs to use the 
> > lock file paradigm:
> >
> >         if (!index_name) {
> > 		[...]
> >         } else {
> >                 unlink(index_name);
> >                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
> >         }
> 
> Whoa.  That particular code has been (and is still) correct.
> 
> When repacking we should pack into a temporary pack and idx file and then
> replace the real ones after both new pack and its idx are successfully
> written, and I thought that is how we've been doing this all the time.
> Maybe the caller has been broken at some point?  Sigh...

I intend to test something like this:

Re: gc considered dangerous

[Junio C Hamano]

Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:

> I've seen this. Running git gc on Windows, while having Elipse open can kill your
> object database. 
>
> Eclipse keeps the pack files open most of the time. Then you 
> launch git gui which recommends the user to do a git gc. I never
> do (it *always* wants to do this), so I haven't encountered the 
> issue, but if gc doesn't find a new optimal pack it tries to rewrite a 
> new pack with the same id. So it rm's the idx file (fine) and the the
> pack file (not ok) and gives up, which means it has a .pack file with 
> no index, so it cannot use it. Trying git gc again after eclipse exits 
> will execute the final stab on your objects. 
>
> The underlying bug is ofcource that Windows locks files when
> they are open. A *nix* user does not suffer from this problem.
>
> I'll investigate more at some other time. This is a preliminary
> analysis.

That sounds like you are hitting this codepath in git-repack.sh:

	fullbases="$fullbases pack-$name"
	chmod a-w "$PACKTMP-$name.pack"
	chmod a-w "$PACKTMP-$name.idx"
	mkdir -p "$PACKDIR" || exit

	for sfx in pack idx
	do
		if test -f "$PACKDIR/pack-$name.$sfx"
		then
			mv -f "$PACKDIR/pack-$name.$sfx" \
				"$PACKDIR/old-pack-$name.$sfx"
		fi
	done &&
	mv -f "$PACKTMP-$name.pack" "$PACKDIR/pack-$name.pack" &&
	mv -f "$PACKTMP-$name.idx"  "$PACKDIR/pack-$name.idx" &&
	test -f "$PACKDIR/pack-$name.pack" &&
	test -f "$PACKDIR/pack-$name.idx" || {
		echo >&2 "Couldn't replace the existing pack with updated one."
		echo >&2 "The original set of packs have been saved as"
		echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
		exit 1
	}
	rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"

We created a pack and an idx successfully in a pair of temporary files, we
notice that pack-$name.idx and/or pack-$name.pack exists and try to move
them out of the way, then we install the new ones in their final
destination, and we try to see if that move succeeded.  If any one of
these steps fails, the entire process fails, but along the way we
shouldn't have lost anything.

I see if .pack can be renamed but .idx can't, then it is possible to get
into a state where you have to mix and match pack-$name.pack and
old-pack-$name.idx.

Re: gc considered dangerous

[Robin Rosenberg]

söndag 08 februari 2009 21:00:57 skrev Junio C Hamano:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > My preliminary guess is that this code in pack-write.c needs to use the 
> > lock file paradigm:
> >
> >         if (!index_name) {
> > 		[...]
> >         } else {
> >                 unlink(index_name);
> >                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
> >         }
> 
> Whoa.  That particular code has been (and is still) correct.
> 
> When repacking we should pack into a temporary pack and idx file and then
> replace the real ones after both new pack and its idx are successfully
> written, and I thought that is how we've been doing this all the time.
> Maybe the caller has been broken at some point?  Sigh...
> 
I intend to test something like this (as of yet completely untested)

-- robin

commit 25a77c80efeb221d165db0bef66b4498aacfac96
Author: Robin Rosenberg <robin.rosenberg@dewire.com>
Date:   Sun Feb 8 20:59:30 2009 +0100

    Make repack more fail-safe
    
    If renaming an old pack fails try to restore halfway renames
    befor failing. The basis is the assumption that this occurs
    because a files is locked for reading on Windows.

diff --git a/git-repack.sh b/git-repack.sh
index 458a497..e816997 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -94,14 +94,25 @@ for name in $names ; do
 	chmod a-w "$PACKTMP-$name.idx"
 	mkdir -p "$PACKDIR" || exit
 
-	for sfx in pack idx
-	do
-		if test -f "$PACKDIR/pack-$name.$sfx"
-		then
-			mv -f "$PACKDIR/pack-$name.$sfx" \
-				"$PACKDIR/old-pack-$name.$sfx"
-		fi
-	done &&
+	if test -f "$PACKDIR/pack-$name.pack"
+	then
+		mv -f "$PACKDIR/pack-$name.pack" \
+			"$PACKDIR/old-pack-$name.pack"
+	fi &&
+	if test -f "$PACKDIR/pack-$name.idx"
+	then
+		mv -f "$PACKDIR/pack-$name.idx" \
+			"$PACKDIR/old-pack-$name.idx" ||
+		(
+			mv -f $PACKDIR/old-pack-$name.pack" \
+			"$PACKDIR/pack-$name.pack" || (
+			echo >&2 "Failed to restore after a failure to rename pack-$name to old-$pack"
+			echo >&2 "Please aquire advice on how to recover from this"
+			echo >&2 "situation before you proceed."
+			false
+			)
+		)
+	fi &&
 	mv -f "$PACKTMP-$name.pack" "$PACKDIR/pack-$name.pack" &&
 	mv -f "$PACKTMP-$name.idx"  "$PACKDIR/pack-$name.idx" &&
 	test -f "$PACKDIR/pack-$name.pack" &&
@@ -109,6 +120,8 @@ for name in $names ; do
 		echo >&2 "Couldn't replace the existing pack with updated one."
 		echo >&2 "The original set of packs have been saved as"
 		echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
+		echo >&2 "Please aquire advice on how to recover from this situation"
+		echo >&2 "before you proceed."
 		exit 1
 	}
 	rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"

Re: gc considered dangerous

[Mike Hommey]

On Sun, Feb 08, 2009 at 12:00:57PM -0800, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > My preliminary guess is that this code in pack-write.c needs to use the 
> > lock file paradigm:
> >
> >         if (!index_name) {
> > 		[...]
> >         } else {
> >                 unlink(index_name);
> >                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
> >         }
> 
> Whoa.  That particular code has been (and is still) correct.

Don't both unlink and open fail in the case the index file is locked in
Windows ?

Mike

Re: gc considered dangerous

[Junio C Hamano]

Mike Hommey <mh@glandium.org> writes:

> On Sun, Feb 08, 2009 at 12:00:57PM -0800, Junio C Hamano wrote:
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > My preliminary guess is that this code in pack-write.c needs to use the 
>> > lock file paradigm:
>> >
>> >         if (!index_name) {
>> > 		[...]
>> >         } else {
>> >                 unlink(index_name);
>> >                 fd = open(index_name, O_CREAT|O_EXCL|O_WRONLY, 0600);
>> >         }
>> 
>> Whoa.  That particular code has been (and is still) correct.
>
> Don't both unlink and open fail in the case the index file is locked in
> Windows ?

Read the rest of the thread and read the code.

The index_file in question is not the file git-repack is telling
pack-write.c to unlink and open.