Re: [PATCH] Make repack less likely to corrupt repository

From: Robin Rosenberg <robin.rosenberg.lists@dewire.com>
Date: Feb 10, 2009, 23:51 UTC
Message-ID: <200902110051.20975.robin.rosenberg.lists@dewire.com>
In-Reply-To: <7vd4dqm4io.fsf@gitster.siamese.dyndns.org>

tisdag 10 februari 2009 21:16:31 skrev Junio C Hamano:

Show 20 quoted lines

> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> 
> >> I was not talking about any loss.  The result would be a funny mixture of
> >> permutations of {old-,}pack-*.{pack,idx} the user needs to match up after
> >
> > We don't leave old-files around unless we go very very wrong and only in
> > that case would be leave "old"-files for one pack around and only if gc wants
> > to replace a pack with the same name. That would not be fatal and the
> > user can continue repacking to get rid of the redundant stuff once the cause
> > of them problem is fixed. 
> 
> You can succeed for the first name and then fail for the second name, for
> example, and can end up with old-pack-* and pack-* with the same name.  I
> found that potentially confusing.  Since you are trying to improve the
> area, it would be nicer to make it less prone to fail and easier to
> recover.
> 
> Here is another attempt to rewrite it, which is closer to what you are
> doing in your patch, but hopefully easier to understand what is going on
> and more atomic.

Almost perfect.

Show 23 quoted lines

> +# If renaming failed for any of them, roll the ones we have
> +# already renamed back to their original names.
> +if test -n "$failed"
> +then
> +	rollback_failure=
> +	for file in $rollback
> +	do
> +		mv "$PACKDIR/old-$file" "$PACKDIR/$file" ||
> +		rollback_failure="$rollback_failure $file"
> +	done
> +	if test -n "$rollback_failure"
> +	then
> +		echo >&2 "WARNING: Some packs in use have been renamed by"
> +		echo >&2 "WARNING: prefixing old- to their name, in order to"
> +		echo >&2 "WARNING: replace them with the new version of the"
> +		echo >&2 "WARNING: file.  But the operation failed, and"
> +		echo >&2 "WARNING: attempt to rename them back to their"
> +		echo >&2 "WARNING: original names also failed."
> +		echo >&2 "WARNING: Please rename them in $PACKDIR manually:"
> +		for file in $rollback_failure
> +		do
> +			echo >&2 "WARNING:   old-$file -> $file"
> +		done

Exit 1 here.  We did not succeed in rolling back

> +	fi
> +	exit 1

But here we should exit 0 because we succeeded in rolling back the changes, so we do not need to scare the user.

Show 22 quoted lines

> +fi
> +
> +# Now the ones with the same name are out of the way...
> +fullbases=
> +for name in $names
> +do
> +	fullbases="$fullbases pack-$name"
> +	chmod a-w "$PACKTMP-$name.pack"
> +	chmod a-w "$PACKTMP-$name.idx"
>  	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"
> +	mv -f "$PACKTMP-$name.idx"  "$PACKDIR/pack-$name.idx" ||
> +	exit
>  done

Here is a safe place to remove the old-packs.

>  if test "$remove_redundant" = t
>

-- robin

Tested on msysgit with different sizes pack.packSizeLimit so we have different number of packs. After a few rounds of repacking, the pack names tend to stabilize and no damage occurred despite files were locked. After unlocking repacking succeeds normally and redundant files are cleaned up.

Patch-patch:

>From baf79b5f8b03002611115e858cd43ede7d8e7f64 Mon Sep 17 00:00:00 2001

From: Robin Rosenberg <robin.rosenberg@dewire.com>
Date: Wed, 11 Feb 2009 00:32:13 +0100
Subject: [PATCH] Try to remove the old packs if we succeed. Exit success if rollback fails after
 failing to rename old packs.

---
 git-repack.sh |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/git-repack.sh b/git-repack.sh
index 0f13043..80673be 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -136,8 +136,9 @@ then
 		do
 			echo >&2 "WARNING:   old-$file -> $file"
 		done
+		exit 1
 	fi
-	exit 1
+	exit
 fi
 
 # Now the ones with the same name are out of the way...
@@ -152,6 +153,15 @@ do
 	exit
 done
 
+# Remove the "old-" files
+for name in $names
+do
+	rm -f "$PACKDIR/old-pack-$name.idx"
+	rm -f "$PACKDIR/old-pack-$name.pack"
+done
+
+# End of pack replacement.
+
 if test "$remove_redundant" = t
 then
 	# We know $existing are all redundant.

-- 
1.6.1.285.g35d8b