patchMake repack less likely to corrupt repository

> diff --git a/git-repack.sh b/git-repack.sh
> index 458a497..6a7ba90 100755
> --- a/git-repack.sh
> +++ b/git-repack.sh
> @@ -93,22 +93,43 @@ for name in $names ; do
>  	chmod a-w "$PACKTMP-$name.pack"
>  	chmod a-w "$PACKTMP-$name.idx"
>  	mkdir -p "$PACKDIR" || exit

> +	if test -f "$PACKDIR/pack-$name.pack"
> +	then
> +		mv -f "$PACKDIR/pack-$name.pack" \
> +			"$PACKDIR/old-pack-$name.pack"
> +	fi &&
> +	if test -f "$PACKDIR/pack-$name.idx"
> +	then
> +		mv -f "$PACKDIR/pack-$name.idx" \
> +			"$PACKDIR/old-pack-$name.idx" ||
> +		(
> +			mv -f "$PACKDIR/old-pack-$name.pack" \
> +			"$PACKDIR/pack-$name.pack" || (
> +				echo >&2 "Failed to restore after a failure to rename"\
> +					"pack-$name{pack,idx} to old-$pack{pack,idx} in $PACKDIR"
> +				echo >&2 "Please acquire advice on how to recover from this"\
> +					"situation before you proceed."
> +				exit 1
> +			) || false
> +		) || (
> +			echo >&2 "Failed to replace the existing pack with updated one."
> +			echo >&2 "We recovered from the situation, but cannot continue".
> +			echo >&2 "repacking."
> +			exit 0
> +		)
> +	fi &&
>  	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."
> +		if (test -f "$PACKDIR/old-pack-$name.pack" ||
> +			test -f "$PACKDIR/old-pack-$name.idx")

> +		then
> +			echo >&2 "The original set of packs have been saved as"
> +			echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
> +		fi
>  		exit 1

> måndag 09 februari 2009 07:04:46 skrev Junio C Hamano:
>> What's troubling more is that this would seem to leave the result even
>> more inconsistent if there are more than one packs that need to be
>> replaced.
>
> Why? We don't prune the old objects if we fail. The result might be a lot
> of redundancy, but nothing should be lost.

> Robin Rosenberg <robin.rosenberg@dewire.com> writes:
> 
> > måndag 09 februari 2009 07:04:46 skrev Junio C Hamano:
> >> What's troubling more is that this would seem to leave the result even
> >> more inconsistent if there are more than one packs that need to be
> >> replaced.
> >
> > Why? We don't prune the old objects if we fail. The result might be a lot
> > of redundancy, but nothing should be lost.
> 
> 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

> figuring out what corresponds with what other one, and I think an expert
> who is asked for help would have hard time matching them up too, even
> though that is what you suggested in the message.
> 
> That troubled me and I was wondering if we can make the recovery simpler
> for the users.

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

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

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

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

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

> Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:
> 
> > 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.
> 
> Up to this point, I felt my earlier misconception corrected, but then ...
> 
> > The unlink error might be enough.
> 
> ... I think we should not even show unlink errors, if "this is not an
> error, nothing to worry about" is the official stance about such failure;
> otherwise the errors will scare people, *and* others then doubly complain
> that even the command detects errors, the whole thing does *not* error
> out.

> Robin Rosenberg <robin.rosenberg.lists@dewire.com> writes:
> 
> > 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.
> 
> Up to this point, I felt my earlier misconception corrected, but then ...
> 
> > The unlink error might be enough.
> 
> ... I think we should not even show unlink errors, if "this is not an
> error, nothing to worry about" is the official stance about such failure;
> otherwise the errors will scare people, *and* others then doubly complain
> that even the command detects errors, the whole thing does *not* error
> out.

> -	exit 1
> +	echo >&2 "INFO: We recovered from the repack error, but your repo"
> +	echo >&2 "INFO: is probably suboptimally packed. You may try to repack"
> +	echo >&2 "INFO: later. A common reason for repacking failure is that"
> +	echo >&2 "INFO: a Windows program was locking one of the old pack files."
> +	echo >&2 "INFO: To repack successfully you may have to close that program"
> +	echo >&2 "INFO: before repacking."

> Hi,
> 
> On Sun, 15 Feb 2009, Robin Rosenberg wrote:
> > -	exit 1
> > +	echo >&2 "INFO: We recovered from the repack error, but your repo"
> > +	echo >&2 "INFO: is probably suboptimally packed. You may try to repack"
> > +	echo >&2 "INFO: later. A common reason for repacking failure is that"
> > +	echo >&2 "INFO: a Windows program was locking one of the old pack files."
> > +	echo >&2 "INFO: To repack successfully you may have to close that program"
> > +	echo >&2 "INFO: before repacking."
> 
> cat >&2 << EOF?

> söndag 15 februari 2009 17:46:59 skrev Johannes Schindelin:
>> Hi,
>> 
>> On Sun, 15 Feb 2009, Robin Rosenberg wrote:
>> > -	exit 1
>> > +	echo >&2 "INFO: We recovered from the repack error, but your repo"
>> > +	echo >&2 "INFO: is probably suboptimally packed. You may try to repack"
>> > +	echo >&2 "INFO: later. A common reason for repacking failure is that"
>> > +	echo >&2 "INFO: a Windows program was locking one of the old pack files."
>> > +	echo >&2 "INFO: To repack successfully you may have to close that program"
>> > +	echo >&2 "INFO: before repacking."
>> 
>> cat >&2 << EOF?
>
> Yeah, but I followed the pattern from the warnings just prior to these messages.
>
>> > +	exit
>> 
>> You lose the error condition here, but I cannot find a convincing argument 
>> about that in the commit message.
>
> I was thinking of my patch as an ammendment to Junios patch. In that context.
>
> "Exit success if rollback fails after failing to rename old packs." Doesn't this
> count?

> If you were arguing for using a different but still non-zero exit status
> to signal the "you asked us to repack but I refused to do so because I
> couldn't move the in-use packs away; by the way I did not corrupt your
> repository because I successfully rolled back everything I did, so do not
> worry to much about it" case, I agree that such a change would be better
> than what we have.  It would allow an automated process to tell a more
> grave repository error and "I need to kill the git object server that is
> pinning some of the packfiles open and re-run the repack" situation apart.

> 
> -- robin
> 
> From 67347a63ce5ba324a750eb2c1ed7b9b0260d966a Mon Sep 17 00:00:00 2001
> From: Robin Rosenberg <robin.rosenberg@dewire.com>
> Date: Thu, 19 Feb 2009 23:18:59 +0100
> Subject: [PATCH] Recover from some known repack failure situations
> 
> For the know problems that we can recover from we exit
> with code 2 instead of 1.

> diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
> index aaa8852..e5ecd66 100644
> --- a/Documentation/git-repack.txt
> +++ b/Documentation/git-repack.txt
> @@ -24,6 +24,12 @@ associated index file.
>  Packs are used to reduce the load on mirror systems, backup
>  engines, disk storage, etc.
>  
> +On Windows repacking may fail because packs that should be replaced
> +are locked by other programs. In this case the program will recover
> +from the situation with an exit code of 2. The user does not have
> +to take any action to recover. For repacking to succeed the user
> +mustc stop the offending program or wait for it to close the packs.

> +
>  OPTIONS
>  -------
>  
> diff --git a/git-repack.sh b/git-repack.sh
> index 0f13043..519c83a 100755
> --- a/git-repack.sh
> +++ b/git-repack.sh
> @@ -136,8 +136,15 @@ then
>  		do
>  			echo >&2 "WARNING:   old-$file -> $file"
>  		done
> +		exit 1
>  	fi
> -	exit 1
> +	echo >&2 "INFO: We recovered from the repack error, but your repo"
> +	echo >&2 "INFO: is probably suboptimally packed. You may try to repack"
> +	echo >&2 "INFO: later. A common reason for repacking failure is that"
> +	echo >&2 "INFO: a Windows program was locking one of the old pack files."
> +	echo >&2 "INFO: To repack successfully you may have to close that program"

> +	echo >&2 "INFO: before repacking."
> +	exit 2
>  fi
>  
>  # Now the ones with the same name are out of the way...
> @@ -152,6 +159,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.

>> +for name in $names
>> +do
>> +	rm -f "$PACKDIR/old-pack-$name.idx"
>> +	rm -f "$PACKDIR/old-pack-$name.pack"
>> +done
>> +
>> +# End of pack replacement.
>
> Is this comment really needed here?

>> +for name in $names
>> +do
>> +	rm -f "$PACKDIR/old-pack-$name.idx"
>> +	rm -f "$PACKDIR/old-pack-$name.pack"
>> +done
>> +
>> +# End of pack replacement.
>
> Is this comment really needed here?