[PATCH v4 0/4] count-objects improvements

> Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
>
>> +/* 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-<.

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

>> +             } 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?

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