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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 9, 2009, 06:04 UTC
Message-ID: <7vwsc0uow1.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1234140299-29785-1-git-send-email-robin.rosenberg@dewire.com>

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

Show 8 quoted lines

> 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

> +	ok=t

This does not seem to be used at all.

Show 32 quoted lines

> +	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")

Why fork a subshell?

Show 5 quoted lines

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

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.

I wonder if a completely different strategy would be less problematic.

 (1) create a new directory objects/new-pack/, copy ones with the same
     name, and hardlink the rest;

 (2) Do the usual "mv temp to final" dance into objects/new-pack/, but
     without any old-pack-$name part; if any fail, do not even try to
     recover but just barf, perhaps removing new-pack directory;

 (3) If all succeed, rename pack/ to old-pack/, rename new-pack/ to pack/.
     If the former fails, you can stop and report that your repack did not
     quite work, but new packs are still found in new-pack.  If the latter
     fails, you can stop and report that your repack did not quite work,
     but original packs are still found in old-pack.

 (4) If the directory rename succeed, remove old-pack/