Re: [PATCH 3/4] t5304: Ensure wanted files are not deleted

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 13, 2016, 20:55 UTC
Message-ID: <xmqqmvs9mc6h.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <670a9d9268beb0d70fb877a7c62d769062babba9.1452704305.git.dougk.ff7@gmail.com>

Doug Kelly <dougk.ff7@gmail.com> writes:

> Subject: Re: [PATCH 3/4] t5304: Ensure wanted files are not deleted

I'd suggest s/wanted/non-garbage/.

> Explicitly test for and ensure files that may be wanted are not
> deleted during a gc operation.  These include .pack without .idx
> (which may be in-flight), garbage in the directory, and .keep files
> the user created.

"garbage in the directory" is not well defined. "files in the directory that clearly are not related to packing" is probably what you meant, but the definition of "related to packing" is still fuzzy. Please clarify.

The following is me thinking aloud about things that you would need to think about while attempting to clarify this.

What should the code do if we find

    pack-b0a9d62a7471e58832a575a78d57f8fb26822125.frotz

in $GIT_OBJECT_DIRECTORY/pack/ directory? Is it a "garbage in the directory"? The filename looks so similar to the usual things with know suffixes .pack, .idx, .bitmap, and .keep, that we may want to consider that it might be another file related to the packing left by a future version of Git and if we do not see corresponding .pack we would want to remove it? Or do we want to do something else?

What should "gc" do if we find

    pack-frotz.idx

without corresponding ".pack"? Wouldn't it be safer to consider it a garbage unrelated to packing (because regular packing would have given it with 40-hex name, not "frotz") and leave it undeleted?

Thanks.

Show 33 quoted lines

> Signed-off-by: Doug Kelly <dougk.ff7@gmail.com>
> ---
>  t/t5304-prune.sh | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
>
> diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
> index 4fa6e7a..f7c380c 100755
> --- a/t/t5304-prune.sh
> +++ b/t/t5304-prune.sh
> @@ -285,6 +285,23 @@ EOF
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'ensure unknown garbage kept with gc' '
> +	test_when_finished "rm -f .git/objects/pack/fake*" &&
> +	test_when_finished "rm -f .git/objects/pack/foo*" &&
> +	: >.git/objects/pack/foo.keep &&
> +	: >.git/objects/pack/fake.pack &&
> +	: >.git/objects/pack/fake2.foo &&
> +	git gc &&
> +	git count-objects -v 2>stderr &&
> +	grep "^warning:" stderr | sort >actual &&
> +	cat >expected <<\EOF &&
> +warning: garbage found: .git/objects/pack/fake2.foo
> +warning: no corresponding .idx or .pack: .git/objects/pack/foo.keep
> +warning: no corresponding .idx: .git/objects/pack/fake.pack
> +EOF
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'prune .git/shallow' '
>  	SHA1=`echo hi|git commit-tree HEAD^{tree}` &&
>  	echo $SHA1 >.git/shallow &&