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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 15, 2009, 20:09 UTC
Message-ID: <7vvdrbjwbt.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <200902151942.15055.robin.rosenberg.lists@dewire.com>

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

Show 25 quoted lines

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