[PATCH v4 0/4] count-objects improvements

On Wed, Feb 13, 2013 at 12:23 AM, Junio C Hamano [off-list ref] wrote:

Nguyễn Thái Ngọc Duy [off-list ref] writes:

quoted

+/* A hook for count-objects to report invalid files in pack directory */
+extern void (*report_garbage)(const char *desc, const char *path, int len, const char *name);

We may want to document the strange way the last three parameters
are used somewhere.  e.g.

        shows "path" (if "name" is NULL), or prepends "path" in
        front of name (otherwise); only for the latter, "path" can
        be a string that is not NUL-terminated but its length
        specified with "len" and in that case a slash is inserted
        between the path and the "name".

When described clearly, it sounds somewhat ugly and incoherent API,
even though it covers the immediate need X-<.

One of the reasons why I did not export it explicitly. Changed it to

void (*report_garbage)(const char *desc, const char *path);

and pushed the ugly part back to callers.

How about doing it something along this line, perhaps?

        int i;
        int beginning_of_this_name = -1;
        int seen_bits = 0; /* 01 for .idx, 02 for .pack */
        for (i = 0; i < list->nr; i++) {
                if (beginning_of_this_name < 0)
                        beginning_of_this_name = i;
                else if (list->items[i] and list->items[beginning_of_this_name]
                         share the same basename)
                        ; /* keep scanning */
                else {
                        /* one name ended at (i-1) */
                        if (seen_bits == 3)
                                ; /* both .idx and .pack exist; good */
                        else
                                report_garbage_for_one_name(list, beginning_of_this_name, i,
                                                seen_bits);
                        seen_bits = 0;
                        beginning_of_this_name = i;
                }
                if (list->items[i] is ".idx")
                        seen_bits |= 1;
                if (list->items[i] is ".pack")
                        seen_bits |= 2;

        }
        if (0 <= beginning_of_this_name && seen_bits != 3)
                report_garbages_for_one_name(list, beginning_of_this_name, list->nr, seen_bits);

with a helper function report_garbage_for_one_name() that would look like this:

        report_garbage_for_one_name(...) {
                int j;
                const char *msg;
                switch (seen_bits) {
                case 0: msg = "no corresponding .idx nor .pack"; break;
                case 1: msg = "no corresponding .pack"; break;
                case 2: msg = "no corresponding .idx; break;
                }
                for (j = beginning_of_this_name; j < i; j++)
                        report_garbage(msg, list->items[j]);
        }

For the above to work, prepare_packed_git_one() needs to retain only the
paths with known extensions in garbage list. "pack-deadbeef.unk" can and
should be reported as a garbage immediately when it is seen without being
placed in the list.

Yup. Looks good.

quoted

+             } else if (has_extension(de->d_name, ".idx")) {
+                     struct string_list_item *item;
+                     int n = strlen(path) - 4;
+                     item = string_list_append_nodup(&garbage,
+                                                     xstrndup(path, n));
+                     item->util = ".idx";
+                     continue;
+             } else
+                     report_garbage("garbage found", path, 0, NULL);

Hmm, where is a ".keep" file handled in this flow?

Apparently I smoked/drank while coding or something. .idx is supposed
to be .keep. This calls for a test to guard my code (part of this v4).

The structure of the if/else cascade is much nicer than the earlier
iterations, but wouldn't it be even more clear to do this?

        if (is .idx file) {
                ... do that .idx thing ...
        }

        if (!report_garbage)
                continue; /* it does not matter what the file is */

        if (is .pack) {
                ... remember that we saw this .pack ...
        } else if (is .idx) {
                ... remember that we saw this .idx ...
        } else if (is .keep) {
                ... remember that we saw this .keep ...
        } else {
                ... all else --- report as garbage immediately ...
        }

Done. 2/4 is updated to make sure the "if (is .idx file)" block does
not shortcut the loop with "continue;" so that we always get .idx
file in the end of the loop.

Nguyễn Thái Ngọc Duy (4):
  git-count-objects.txt: describe each line in -v output
  sha1_file: reorder code in prepare_packed_git_one()
  count-objects: report garbage files in pack directory too
  count-objects: report how much disk space taken by garbage files

 Documentation/git-count-objects.txt |  22 ++++++--
 builtin/count-objects.c             |  30 ++++++++---
 cache.h                             |   3 ++
 sha1_file.c                         | 101 +++++++++++++++++++++++++++++++-----
 t/t5304-prune.sh                    |  26 ++++++++++
 5 files changed, 156 insertions(+), 26 deletions(-)

-- 
1.8.1.2.536.gf441e6d