[PATCH] Make repack less likely to corrupt repository

[PATCH] Make repack less likely to corrupt repository

[Robin Rosenberg]

Repack could easily leave a repo in bad state after a failed
repack on Windows if a pack file that repack wanted to replace
was locked by a reader. A second attempt at running repack
would then destroy the repo by removing the pack file. This
attempts to make repack leave the repo in a good state, although
not optimal, in order to avoid disasters.

If renaming an old pack fails we will try to restore halfway renames
before exiting repack.

For severe situations we encourage the user to seek advice.

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 git-repack.sh |   43 ++++++++++++++++++++++++++++++++-----------
 1 files changed, 32 insertions(+), 11 deletions(-)

Here is an attempt at fixing this It was tested by making sure the
idx or pack file was opened by some process and verifying that repo
was ok after git repack -a -d. That was a manual test. We should
probably add an automatic one too of some sort but I submit this 
for reading  and ad-hoc testing by interested parties.

-- robin

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
-
-	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 &&
+	ok=t
+	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."
-		echo >&2 "The original set of packs have been saved as"
-		echo >&2 "old-pack-$name.{pack,idx} in $PACKDIR."
+		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
 	}
 	rm -f "$PACKDIR/old-pack-$name.pack" "$PACKDIR/old-pack-$name.idx"
-- 
1.6.1.285.g35d8b

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

[Junio C Hamano]

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

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

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

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

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

[Robin Rosenberg]

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

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

[Junio C Hamano]

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.

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

[Robin Rosenberg]

tisdag 10 februari 2009 16:59:16 skrev Junio C Hamano:
> 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

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. 

For the simple case of "failure" I recover and return 0 (but don't prune the
old packs), because no damage is done so I don't expect anyone to actually even try manual cleanup and why should they?

For the hard case, the user seek advice because I cannot image what
would be the cause. Today even a simple and likely problem will cause a fatal
corruption of the repo and that is my concern right now, not what happens
when the disk fails in between the mv's.

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

-- robin

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

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

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

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

[Robin Rosenberg]

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.

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

Here is a safe place to remove the old-packs.

>  if test "$remove_redundant" = t
> 

-- robin

Tested on msysgit with different sizes pack.packSizeLimit so we have different
number of packs. After a few rounds of repacking,  the pack names tend to
stabilize and no damage occurred despite files were locked. After unlocking
repacking succeeds normally and redundant files are cleaned up.

Patch-patch:

>From baf79b5f8b03002611115e858cd43ede7d8e7f64 Mon Sep 17 00:00:00 2001
From: Robin Rosenberg <robin.rosenberg@dewire.com>
Date: Wed, 11 Feb 2009 00:32:13 +0100
Subject: [PATCH] Try to remove the old packs if we succeed. Exit success if rollback fails after
 failing to rename old packs.

---
 git-repack.sh |   12 +++++++++++-
 1 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/git-repack.sh b/git-repack.sh
index 0f13043..80673be 100755
--- a/git-repack.sh
+++ b/git-repack.sh
@@ -136,8 +136,9 @@ then
 		do
 			echo >&2 "WARNING:   old-$file -> $file"
 		done
+		exit 1
 	fi
-	exit 1
+	exit
 fi
 
 # Now the ones with the same name are out of the way...
@@ -152,6 +153,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.
+
 if test "$remove_redundant" = t
 then
 	# We know $existing are all redundant.
-- 
1.6.1.285.g35d8b

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

[Junio C Hamano]

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?

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

[Robin Rosenberg]

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

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

[Junio C Hamano]

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.

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

[Robin Rosenberg]

onsdag 11 februari 2009 01:31:07 skrev Junio C Hamano:
> 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.

Hmm, ok drop the error at that point. But maybe we'd need to save it so we
can display it in case the recovery fails, in which case it may actually contain
some useful hint about went wrong.

-- robin

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

[Robin Rosenberg]

onsdag 11 februari 2009 01:31:07 skrev Junio C Hamano:
> 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.

Here is an amendment, the only change is that it outputs a more verbose
explanation. Remember the issue we are trying to fix is a critical bug, not
a cosmetic error. I'd rather hear people complain about the wording of error
messages, than lost repositories.

-- robin

>From 3598881d6591e7c89b1a6a3c8da526f847382a35 Mon Sep 17 00:00:00 2001
From: Robin Rosenberg <robin.rosenberg@dewire.com>
Date: Sun, 15 Feb 2009 17:05:49 +0100
Subject: [PATCH] Try to remove the old packs if we succeed. Exit success if rollback fails after
 failing to rename old packs.

---
 git-repack.sh |   18 +++++++++++++++++-
 1 files changed, 17 insertions(+), 1 deletions(-)

diff --git a/git-repack.sh b/git-repack.sh
index 0f13043..194af86 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
 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.
+
 if test "$remove_redundant" = t
 then
 	# We know $existing are all redundant.
-- 
1.6.1.285.g35d8b

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

[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?

> +	exit

You lose the error condition here, but I cannot find a convincing argument 
about that in the commit message.

Ciao,
Dscho

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

[Robin Rosenberg]

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?

-- robin

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

[Junio C Hamano]

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

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

No, it does not count, because that sentence is just a statement about
_what_ your change does, and does not explain _why_ it is better to report
success, other than an unstated "because I think that is more appropriate."

I think your "INFO:" lines are good to give additional useful cue for
the human operators, though.

If we wanted to move away old ones to replace them with new ones, and we
failed to comply with what the user wished for because the preparatory
step of moving-them-away failed, it _is_ a failure, and for this reason,
it should be reported as such.  That would be my counter-statement that
explains _why_.

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.

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

[Robin Rosenberg]

Junio C Hamano <gitster@pobox.com> wrote:

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

I can live with that. So what are our exit codes then?

0 = successful, repack did what we wanted it to

1 = serious error, no idea really, user should investigate
	(now we know they don't anyway, but that's another problem).

2 = not too good, we didn't succeed in repacking, but we didn't destroy
	anything and the user does not necessarily have to do anything.
	GUI's should probably flag this differently from exit code 1.

This would simply introduce a new error code, but the drawback is that
they are "out of order", i.e. most severe does not have the highest code.

-- robin

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

[Robin Rosenberg]

Here is a new version on my ammendments to Junios patch.

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

Also removed the old packs when repack succeeds

Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
 Documentation/git-repack.txt |    6 ++++++
 git-repack.sh                |   18 +++++++++++++++++-
 2 files changed, 23 insertions(+), 1 deletions(-)

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.
+
 if test "$remove_redundant" = t
 then
 	# We know $existing are all redundant.
-- 
1.6.1.285.g35d8b

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

[Johannes Schindelin]

Hi,

On Thu, 19 Feb 2009, Robin Rosenberg wrote:

> Here is a new version on my ammendments to Junios patch.

s/on/of/

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

s/know/known/

> Also removed the old packs when repack succeeds

s/removed/remove/  s/$/./

> 
> Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
> ---

I'd suggest a complete rewording of the commit message, though:

	repack: exit with status 2 for non-fatal errors

	Under certain circumstances, e.g. on Windows, when a pack file 
	is still in use, deleting files can fail.

	This is a non-fatal error condition, as the repository is not 
	corrupted as a consequence; however, it is an error nevertheless, 
	as $GIT_DIR takes more space than it should.

	We indicate that special condition with an exit status 2.

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

s/The user does not have to take any action to recover. //

(It is not true)

s/mustc/must/

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

Two long lines.

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

s/old-/old-pack-/

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

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Is that hunk still needed?  I think the removal part is already there all
the way down to maint.

Other than that, I think your comments are all sane.

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

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

Is that hunk still needed?  I think the removal part is already there all
the way down to maint.

Other than that, I think your comments are all sane.