Skip to content

Commit

Permalink
prepare_packed_git_one: refactor duplicate-pack check
Browse files Browse the repository at this point in the history
When we are reloading the list of packs, we check whether a
particular pack has been loaded. This is slightly tricky,
because we load packs based on the presence of their ".idx"
files, but record the name of the matching ".pack" file.
Therefore we want to compare their bases.

The existing code stripped off ".idx" from a file we found,
then compared that whole base length to strings containing
the ".pack" version. This meant we could end up comparing
bytes past what the ".pack" string contained, if the ".idx"
file name was much longer.

In practice, it worked OK because memcmp would end up seeing
a difference in the two strings and would return early
before hitting the full length. However, memcmp may
sometimes read extra bytes past a difference (e.g., because
it is comparing 64-bit words), or is even free to compare in
reverse order.

Furthermore, our memcmp made no guarantees that we matched
the whole pack name, up to ".pack". So "foo.idx" would match
"foo-bar.pack", which is wrong (but does not typically
happen, because our pack names have a fixed size).

We can fix both issues, avoid magic numbers, and document
that we expect to compare against a string with ".pack" by
using strip_suffix.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Jeff King authored and Junio C Hamano committed Jun 30, 2014
1 parent d6cd00c commit 47bf4b0
Showing 1 changed file with 7 additions and 2 deletions.
9 changes: 7 additions & 2 deletions sha1_file.c
Original file line number Diff line number Diff line change
Expand Up @@ -1197,17 +1197,22 @@ static void prepare_packed_git_one(char *objdir, int local)
dirnamelen = path.len;
while ((de = readdir(dir)) != NULL) {
struct packed_git *p;
size_t base_len;

if (is_dot_or_dotdot(de->d_name))
continue;

strbuf_setlen(&path, dirnamelen);
strbuf_addstr(&path, de->d_name);

if (ends_with(de->d_name, ".idx")) {
base_len = path.len;
if (strip_suffix_mem(path.buf, &base_len, ".idx")) {
/* Don't reopen a pack we already have. */
for (p = packed_git; p; p = p->next) {
if (!memcmp(path.buf, p->pack_name, path.len - 4))
size_t len;
if (strip_suffix(p->pack_name, ".pack", &len) &&
len == base_len &&
!memcmp(p->pack_name, path.buf, len))
break;
}
if (p == NULL &&
Expand Down

0 comments on commit 47bf4b0

Please sign in to comment.