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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

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