@matheusbsbs / matheus-bagliano / commits / def390d593

builtin/repack.c: avoid dir traversal in `collect_pack_filenames()`

When repacking, the function `collect_pack_filenames()` is responsible for collecting the set of existing packs in the repository, and partitioning them into "kept" (if the pack has a ".keep" file or was given via `--keep-pack`) and "nonkept" (otherwise) lists. This function comes from the original C port of git-repack.sh from back in a1bbc6c0176 (repack: rewrite the shell script in C, 2013-09-15), where it first appears as `get_non_kept_pack_filenames()`. At the time, the implementation was a fairly direct translation from the relevant portion of git-repack.sh, which looped over the results of find "$PACKDIR" -type f -name '*.pack' either ignoring the pack as kept, or adding it to the list of existing packs. So the choice to directly translate this function in terms of `readdir()` in a1bbc6c0176 made sense. At the time, it was possible to refine the C version in terms of packed_git structs, but was never done. However, manually enumerating a repository's packs via `readdir()` is confusing and error-prone. It leads to frustrating inconsistencies between which packs Git considers to be part of a repository (i.e., could be found in the list of packs from `get_all_packs()`), and which packs `collect_pack_filenames()` considers to meet the same criteria. This bit us in 73320e49ad (builtin/repack.c: only collect fully-formed packs, 2023-06-07), and again in the previous commit. Prevent these issues from biting us in the future by implementing the `collect_pack_filenames()` function by looping over an array of pointers to `packed_git` structs, ensuring that we use the same criteria to determine the set of available packs. One gotcha here is that we have to ignore non-local packs, since the original version of `collect_pack_filenames()` only looks at the local pack directory to collect existing packs. Signed-off-by: Taylor Blau <me@ttaylorr.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Taylor Blau committed Jul 11, 2023 at 13:32 UTC def390d5939d97f4bd642684e0554885e716370e
1 file changed +15 -26
builtin/repack.c
+15 -26
@@ -106,49 +106,38 @@ static void collect_pack_filenames(struct string_list *fname_nonkept_list,
106 struct string_list *fname_kept_list,
107 const struct string_list *extra_keep)
108 {
109 - DIR *dir;
110 - struct dirent *e;
111 - char *fname;
109 + struct packed_git *p;
110 struct strbuf buf = STRBUF_INIT;
111
114 - if (!(dir = opendir(packdir)))
115 - return;
116 -
117 - while ((e = readdir(dir)) != NULL) {
118 - size_t len;
112 + for (p = get_all_packs(the_repository); p; p = p->next) {
113 int i;
114 + const char *base;
115
121 - if (!strip_suffix(e->d_name, ".idx", &len))
116 + if (!p->pack_local)
117 continue;
118
124 - strbuf_reset(&buf);
125 - strbuf_add(&buf, e->d_name, len);
126 - strbuf_addstr(&buf, ".pack");
127 -
128 - if (!file_exists(mkpath("%s/%s", packdir, buf.buf)))
129 - continue;
119 + base = pack_basename(p);
120
121 for (i = 0; i < extra_keep->nr; i++)
132 - if (!fspathcmp(buf.buf, extra_keep->items[i].string))
122 + if (!fspathcmp(base, extra_keep->items[i].string))
123 break;
124
135 - fname = xmemdupz(e->d_name, len);
125 + strbuf_reset(&buf);
126 + strbuf_addstr(&buf, base);
127 + strbuf_strip_suffix(&buf, ".pack");
128
137 - if ((extra_keep->nr > 0 && i < extra_keep->nr) ||
138 - (file_exists(mkpath("%s/%s.keep", packdir, fname)))) {
139 - string_list_append_nodup(fname_kept_list, fname);
140 - } else {
129 + if ((extra_keep->nr > 0 && i < extra_keep->nr) || p->pack_keep)
130 + string_list_append(fname_kept_list, buf.buf);
131 + else {
132 struct string_list_item *item;
142 - item = string_list_append_nodup(fname_nonkept_list,
143 - fname);
144 - if (file_exists(mkpath("%s/%s.mtimes", packdir, fname)))
133 + item = string_list_append(fname_nonkept_list, buf.buf);
134 + if (p->is_cruft)
135 item->util = (void*)(uintptr_t)CRUFT_PACK;
136 }
137 }
148 - closedir(dir);
149 - strbuf_release(&buf);
138
139 string_list_sort(fname_kept_list);
140 + strbuf_release(&buf);
141 }
142
143 static void remove_redundant_pack(const char *dir_name, const char *base_name)