On Sat, Feb 9, 2013 at 1:44 AM, Junio C Hamano <gits...@pobox.com> wrote:
>> +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.
>> @@ -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.
>> - 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
