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

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

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

> tisdag 10 februari 2009 21:16:31 skrev Junio C Hamano:
>> 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.
>
>> +# 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.

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