Re: [PATCH v2 2/3] count-objects: report garbage files in pack directory too

On Sat, Feb 9, 2013 at 1:44 AM, Junio C Hamano [off-list ref] wrote:

quoted

+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.

quoted

+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.

quoted

@@ -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.

quoted

-             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