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

Robin Rosenberg [off-list ref] writes:

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

quoted

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.

quoted

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

quoted

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

We failed to honor what the end user wanted: to repack.  Why should we
exit 0 here?

onsdag 11 februari 2009 00:56:49 skrev Junio C Hamano:

We failed to honor what the end user wanted: to repack.  Why should we
exit 0 here?

A repack may or may not yield a better packed repo. In this case, not, but for a
different reason than failing to find better deltas. Given the circumstances that is 
most likely to cause the "failure  (repacking on windows), this is "normal" behaviour
and no reason to scare the user with an error code. The unlink error might be
enough. I think scripts should be able to continue too. The alternative would
be to have a switch to repack that scripts and UI's could enable, like -q for "don't
flag malign errors", or the other way, a --pedantic for "flag failure to make object database smaller".

-- robin