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

Robin Rosenberg [off-list ref] writes:

quoted

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.

This is not the "prepare objects/new-pack/ and if we manage to create it
into a good shape replace objects/pack/ with it" approach that I suggested
earlier, though.  That would be a lot more atomic.

 git-repack.sh |   78 ++++++++++++++++++++++++++++++++++++++++++--------------
 1 files changed, 58 insertions(+), 20 deletions(-)

diff --git a/git-repack.sh b/git-repack.sh
index 458a497..1e38559 100755
--- a/git-repack.sh
+++ b/git-repack.sh

@@ -88,30 +88,68 @@ if [ -z "$names" ]; then
 		echo Nothing new to pack.
 	fi
 fi
-for name in $names ; do
-	fullbases="$fullbases pack-$name"
-	chmod a-w "$PACKTMP-$name.pack"
-	chmod a-w "$PACKTMP-$name.idx"
-	mkdir -p "$PACKDIR" || exit
 
+# Ok we have prepared all new packfiles.
+mkdir -p "$PACKDIR" || exit
+
+# First see if there are packs of the same name and if so
+# if we can move them out of the way (this can happen if we
+# repacked immediately after packing fully.
+rollback=
+failed=
+for name in $names
+do
 	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 &&
+		file=pack-$name.$sfx
+		test -f "$PACKDIR/$file" || continue
+		rm -f "$PACKDIR/old-$file" &&
+		mv "$PACKDIR/$file" "$PACKDIR/old-$file" || {
+			failed=t
+			break
+		}
+		rollback="$rollback $file"
+	done
+	test -z "$failed" || break
+done
+
+# 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
+	fi
+	exit 1
+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
 
 if test "$remove_redundant" = t

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

Robin Rosenberg [off-list ref] writes:

quoted

quoted

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.

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

+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 <redacted>
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