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

[Junio C Hamano <gitster@pobox.com>]

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.

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