Re: [PATCH v2 2/3] count-objects: report garbage files in pack directory too
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Feb 9, 2013, 01:58 UTC
- Message-ID
- <CACsJy8D1qMpLRS0nyayRc=sq2A4X-dbiXhdVdfP6OOTCkxms4Q@mail.gmail.com>
- In-Reply-To
- <7vwqui1w84.fsf@alter.siamese.dyndns.org>
On Sat, Feb 9, 2013 at 1:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
>> +static void real_report_pack_garbage(const char *path, int len, const char *name)
>> +{
>
> Don't some callers call this on paths outside objects/pack/
> directory? Is it still report-pack-garbage?In fact 3/3 uses it to report loose garbage. Will rename.
>> +static const char *known_pack_extensions[] = { ".pack", ".keep", NULL };
>
> This sounds wrong. Isn't ".idx" also known?I had a comment saying "all known extensions related to a pack, except .idx" but I dropped it. .idx being used as the pointer file needs to be handled separately.
Show 12 quoted lines
>> @@ -1024,14 +1074,37 @@ static void prepare_packed_git_one(char *objdir, int local)
>> int namelen = strlen(de->d_name);
>> struct packed_git *p;
>>
>> - if (!has_extension(de->d_name, ".idx"))
>> + if (len + namelen + 1 > sizeof(path)) {
>> + if (report_pack_garbage)
>> + report_pack_garbage(path, len - 1, de->d_name);
>
> A pack/in/a/very/long/path/pack-0000000000000000000000000000000000000000.pack
> may pass when fed to "git verify-pack", but this will report it as "garbage",
> without reporting what is wrong with it. Wouldn't that confuse users?If all other git commands do not recognize the pack, then I think it's still considered garbage. We could either
- make prepare_packed_git_one accept pack/in/a/very/long/path-... - show the reason why we consider it garbage
Which option do we do? Option 1 may involve chdir in, stat stat stat and chdir out to get around short PATH_MAX limit on some system.
Show 40 quoted lines
>> - if (len + namelen + 1 > sizeof(path))
>> + if (!has_extension(de->d_name, ".idx")) {
>> + struct string_list_item *item;
>> + int i, n;
>> + if (!report_pack_garbage)
>> + continue;
>> + if (is_dot_or_dotdot(de->d_name))
>> + continue;
>> + for (i = 0; known_pack_extensions[i]; i++)
>> + if (has_extension(de->d_name,
>> + known_pack_extensions[i]))
>> + break;
>> + if (!known_pack_extensions[i]) {
>> + report_pack_garbage(path, 0, NULL);
>> + continue;
>> + }
>> + n = strlen(path) - strlen(known_pack_extensions[i]);
>> + item = string_list_append_nodup(&garbage,
>> + xstrndup(path, n));
>> + item->util = (void*)known_pack_extensions[i];
>> continue;
>> + }
>
> Why isn't this part more like this?
>
> if (dot-or-dotdot) {
> continue;
> } else if (has_extension(de->d_name, ".idx")) {
> do things for the .idx file;
> } else if (has_extension(de->d_name, ".pack") {
> do things for the .pack file, including
> queuing the name if we haven't seen
> corresponding .idx for later examination;
> } else if (has_extension(de->d_name, ".keep") {
> nothing special for now but we may
> want to add some other checks later
> } else {
> everything else is a garbage
> report_pack_garbage();
> }Originally I checked known_extensions again in report_pack_garbage() but after restructuring, yeah we can drop known_extensions and do it your way. Looks much clearer.
-- Duy