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

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

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

> On Wed, Jan 13, 2016 at 2:55 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> 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/.
>>
>
> I'm okay with this.
>
>>> 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.
>
> This is probably a good point.  Perhaps a better way to think about it
> would be by rewording the paragraph to something like this:
>
> Explicitly test for and ensure files that may either be desired by the user
> or are possibly not garbage are not deleted during a gc operation.
> These include .pack files missing a corresponding .idx file (possibly due
> to it being in-flight), .keep files created by the user, and other
> unknown garbage in the pack directory.  These files will still be identified
> by "git count-objects -v", but should not be removed automatically by
> gc.  Only files we are absolutely sure are unnecessary will be removed
> as a part of the gc process.

That, especially "other _unknown_ garbage", looks much better than
the original.

> I'm not saying there's nothing to be said for the difference in the base
> filename without extension.  Currently, the logic to remove pack garbage
> doesn't look at that, though: it only considers the extension, and what
> related files are found in the directory.  Whether this is good or bad, I'm
> not sure.  It certainly does what I need at fairly low risk, though.
>
> Does this help clarify the situation more?

I was shooting for making you _think_ exactly about what you wrote
in the above paragraph, i.e. what the current logic does and if it
is sensible, is overly pessimistic for some files, and/or is risky
for some other files and if so in what way, as that would help you
record the thinking behind the different treatment for files based
on various file extentions clearly in the log message.

Thanks.