Re: [PATCH] pack-objects: name pack files after trailer hash

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 5, 2013, 22:59 UTC
Message-ID: <xmqq4n6m52fy.fsf@gitster.dls.corp.google.com>
In-Reply-To: <20131205202807.GA19042@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

> The second half would be to simplify git-repack. The current behavior is
> to replace the old packfile with a tricky rename dance. Which is still
> correct, but overly complicated. We should be able to just drop the new
> packfile, since we know the bytes are identical (or rename the new one
> over the old, though I think keeping the old is probably kinder to the
> disk cache, especially if another process already has it mmap'd).

Concurred.

> One test needs to be updated, because it actually corrupts a
> pack and expects that re-packing the corrupted bytes will
> use the same name. It won't anymore, but we can easily just
> use the name that pack-objects hands back.

Re-reading the tests in that script, I am not sure if keeping these tests is even a sane thing to do, by the way. It "expects" that certain breakages are propagated, and anybody who breaks that expectation by improving pack-objects etc. to catch such breakages will be yelled at by breaking the test that used to pass.

Seeing that the way the test scripts are line-wrapped follows the ancient convention, I suspect that this may be because it predates our more recent best practice to document known breakages with test_expect_failure.

Show 15 quoted lines

> diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
> index fe82025..4bbb718 100755
> --- a/t/t5302-pack-index.sh
> +++ b/t/t5302-pack-index.sh
> @@ -174,11 +174,11 @@ test_expect_success \
>  test_expect_success \
>      '[index v1] 5) pack-objects happily reuses corrupted data' \
>      'pack4=$(git pack-objects test-4 <obj-list) &&
> -     test -f "test-4-${pack1}.pack"'
> +     test -f "test-4-${pack4}.pack"'
>  
>  test_expect_success \
>      '[index v1] 6) newly created pack is BAD !' \
> -    'test_must_fail git verify-pack -v "test-4-${pack1}.pack"'
> +    'test_must_fail git verify-pack -v "test-4-${pack4}.pack"'

A good thing is that the above hunks are the right thing to do, even if we are to modernise these tests so that they document a known breakage with expect-failure.

Thanks.