Re: [PATCH] repack.c: rename and unlink pack file if it exists

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 5, 2014, 20:57 UTC
Message-ID: <xmqqfvnx2s7p.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20140205203740.GA17077@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 35 quoted lines

> On Wed, Feb 05, 2014 at 12:31:34PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > The minimal fix you posted below does make sense to me as a stopgap, and
>> > we can look into dropping the code entirely during the next cycle. It
>> > would be nice to have a test to cover this case, though.
>> 
>> Sounds sensible.  Run "repack -a -d" once, and then another while
>> forcing it to be single threaded, or something?
>
> Certainly that's the way to trigger the code, but doing this:
>
> diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
> index b45bd1e..6647ba1 100755
> --- a/t/t7700-repack.sh
> +++ b/t/t7700-repack.sh
> @@ -162,7 +162,12 @@ test_expect_success 'objects made unreachable by grafts only are kept' '
>  	git reflog expire --expire=$test_tick --expire-unreachable=$test_tick --all &&
>  	git repack -a -d &&
>  	git cat-file -t $H1
> -	'
> +'
> +
> +test_expect_success 'repack can handle generating the same pack again' '
> +	git -c pack.threads=1 repack -ad &&
> +	git -c pack.threads=1 repack -ad
> +'
>  
>  test_done
>  
>
> ...does not seem to fail, and it does not seem to leave any cruft in
> place. So maybe I am misunderstanding the thing the patch is meant to
> fix. Is it that we simply do not replace the pack in this instance?

Yes. Not just the command finishing OK, but the packfile left by the first repack needs to be left intact. One bug was that after learning that a new packfile XXXX needs to be installed, the command did not check existing .git/objects/pack/pack-XXXX.{pack,idx}, but checked .git/objects/pack/XXXX.{pack,idx}, deciding that there is nothing that needs to be saved by renaming with s|pack-|old-|. This would have caused it to rename the new packfile left by pack-object at .git/objects/pack/.tmp-$pid-pack-XXXX.{pack,idx} directly to the final .git/objects/pack/pack-XXXX.{pack,idx}, which would work only on filesystems that allow renaming over an existing file.

Another bug fixed by Torsten was in the codepath to roll the rename back from .git/objects/pack/old-XXXX.{pack,idx} to the original (the command tried to rename .git/objects/pack/old-pack-XXXX.{pack,idx} which would not have been the ones it renamed), but because of the earlier bug, it would never have triggered in the first place.

> I guess we would have to generate a pack with the identical set of
> objects, then, but somehow different in its pack parameters (perhaps
> turning off deltas?).

Unfortunately, that would trigger different codepath on v1.9-rc0 and later with 1190a1ac (pack-objects: name pack files after trailer hash, 2013-12-05), as it is likely not to name the packfiles the same.

We could use test-chmtime to reset the timestamp of the packfile generated by the first repack to somewhere reasonably old and then rerun the repack to see that it is a different file, which may be more portable than inspecting the inum of the packfile.