From 880fb8de67af3aeeae5a59ba5ce84ad3d0cf1dda Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Scharfe?= Date: Mon, 30 Jun 2014 12:55:52 -0400 Subject: [PATCH 1/9] sha1_file: replace PATH_MAX buffer with strbuf in prepare_packed_git_one() Instead of using strbuf to create a message string in case a path is too long for our fixed-size buffer, replace that buffer with a strbuf and thus get rid of the limitation. Helped-by: Duy Nguyen Signed-off-by: Rene Scharfe Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sha1_file.c | 42 ++++++++++++++++-------------------------- 1 file changed, 16 insertions(+), 26 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 34d527f670..394fa4509e 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -1177,48 +1177,37 @@ static void report_pack_garbage(struct string_list *list) static void prepare_packed_git_one(char *objdir, int local) { - /* Ensure that this buffer is large enough so that we can - append "/pack/" without clobbering the stack even if - strlen(objdir) were PATH_MAX. */ - char path[PATH_MAX + 1 + 4 + 1 + 1]; - int len; + struct strbuf path = STRBUF_INIT; + size_t dirnamelen; DIR *dir; struct dirent *de; struct string_list garbage = STRING_LIST_INIT_DUP; - sprintf(path, "%s/pack", objdir); - len = strlen(path); - dir = opendir(path); + strbuf_addstr(&path, objdir); + strbuf_addstr(&path, "/pack"); + dir = opendir(path.buf); if (!dir) { if (errno != ENOENT) error("unable to open object pack directory: %s: %s", - path, strerror(errno)); + path.buf, strerror(errno)); + strbuf_release(&path); return; } - path[len++] = '/'; + strbuf_addch(&path, '/'); + dirnamelen = path.len; while ((de = readdir(dir)) != NULL) { - int namelen = strlen(de->d_name); struct packed_git *p; - if (len + namelen + 1 > sizeof(path)) { - if (report_garbage) { - struct strbuf sb = STRBUF_INIT; - strbuf_addf(&sb, "%.*s/%s", len - 1, path, de->d_name); - report_garbage("path too long", sb.buf); - strbuf_release(&sb); - } - continue; - } - if (is_dot_or_dotdot(de->d_name)) continue; - strcpy(path + len, de->d_name); + strbuf_setlen(&path, dirnamelen); + strbuf_addstr(&path, de->d_name); if (has_extension(de->d_name, ".idx")) { /* Don't reopen a pack we already have. */ for (p = packed_git; p; p = p->next) { - if (!memcmp(path, p->pack_name, len + namelen - 4)) + if (!memcmp(path.buf, p->pack_name, path.len - 4)) break; } if (p == NULL && @@ -1226,7 +1215,7 @@ static void prepare_packed_git_one(char *objdir, int local) * See if it really is a valid .idx file with * corresponding .pack file that we can map. */ - (p = add_packed_git(path, len + namelen, local)) != NULL) + (p = add_packed_git(path.buf, path.len, local)) != NULL) install_packed_git(p); } @@ -1237,13 +1226,14 @@ static void prepare_packed_git_one(char *objdir, int local) has_extension(de->d_name, ".pack") || has_extension(de->d_name, ".bitmap") || has_extension(de->d_name, ".keep")) - string_list_append(&garbage, path); + string_list_append(&garbage, path.buf); else - report_garbage("garbage found", path); + report_garbage("garbage found", path.buf); } closedir(dir); report_pack_garbage(&garbage); string_list_clear(&garbage, 0); + strbuf_release(&path); } static int sort_pack(const void *a_, const void *b_) From 35480f0b23f2c1824109ddae24392a70d19c6b9c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 30 Jun 2014 12:57:51 -0400 Subject: [PATCH 2/9] add strip_suffix function Many callers of ends_with want to not only find out whether a string has a suffix, but want to also strip it off. Doing that separately has two minor problems: 1. We often run over the string twice (once to find the suffix, and then once more to find its length to subtract the suffix length). 2. We have to specify the suffix length again, which means either a magic number, or repeating ourselves with strlen("suffix"). Just as we have skip_prefix to avoid these cases with starts_with, we can add a strip_suffix to avoid them with ends_with. Note that we add two forms of strip_suffix here: one that takes a string, with the resulting length as an out-parameter; and one that takes a pointer/length pair, and reuses the length as an out-parameter. The latter is more efficient when the caller already has the length (e.g., when using strbufs), but it can be easy to confuse the two, as they take the same number and types of parameters. For that reason, the "mem" form puts its length parameter next to the buffer (since they are a pair), and the string form puts it at the end (since it is an out-parameter). The compiler can notice when you get the order wrong, which should help prevent writing one when you meant the other. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- git-compat-util.h | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/git-compat-util.h b/git-compat-util.h index e6a4159a25..4c451b575d 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -350,6 +350,33 @@ static inline const char *skip_prefix(const char *str, const char *prefix) return NULL; } +/* + * If buf ends with suffix, return 1 and subtract the length of the suffix + * from *len. Otherwise, return 0 and leave *len untouched. + */ +static inline int strip_suffix_mem(const char *buf, size_t *len, + const char *suffix) +{ + size_t suflen = strlen(suffix); + if (*len < suflen || memcmp(buf + (*len - suflen), suffix, suflen)) + return 0; + *len -= suflen; + return 1; +} + +/* + * If str ends with suffix, return 1 and set *len to the size of the string + * without the suffix. Otherwise, return 0 and set *len to the size of the + * string. + * + * Note that we do _not_ NUL-terminate str to the new length. + */ +static inline int strip_suffix(const char *str, const char *suffix, size_t *len) +{ + *len = strlen(str); + return strip_suffix_mem(str, len, suffix); +} + #if defined(NO_MMAP) || defined(USE_WIN32_MMAP) #ifndef PROT_READ From f52a35fd63cc6d570083cedc15576d01c0968d98 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 30 Jun 2014 12:58:08 -0400 Subject: [PATCH 3/9] implement ends_with via strip_suffix The ends_with function is essentially a simplified version of strip_suffix, in which we throw away the stripped length. Implementing it as an inline on top of strip_suffix has two advantages: 1. We save a bit of duplicated code. 2. The suffix is typically a string literal, and we call strlen on it. By making the function inline, many compilers can replace the strlen call with a constant. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- git-compat-util.h | 7 ++++++- strbuf.c | 9 --------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index 4c451b575d..1c9c68cb89 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -339,7 +339,6 @@ extern void set_error_routine(void (*routine)(const char *err, va_list params)); extern void set_die_is_recursing_routine(int (*routine)(void)); extern int starts_with(const char *str, const char *prefix); -extern int ends_with(const char *str, const char *suffix); static inline const char *skip_prefix(const char *str, const char *prefix) { @@ -377,6 +376,12 @@ static inline int strip_suffix(const char *str, const char *suffix, size_t *len) return strip_suffix_mem(str, len, suffix); } +static inline int ends_with(const char *str, const char *suffix) +{ + size_t len; + return strip_suffix(str, suffix, &len); +} + #if defined(NO_MMAP) || defined(USE_WIN32_MMAP) #ifndef PROT_READ diff --git a/strbuf.c b/strbuf.c index ee96dcfb81..63356ccd1b 100644 --- a/strbuf.c +++ b/strbuf.c @@ -10,15 +10,6 @@ int starts_with(const char *str, const char *prefix) return 0; } -int ends_with(const char *str, const char *suffix) -{ - int len = strlen(str), suflen = strlen(suffix); - if (len < suflen) - return 0; - else - return !strcmp(str + len - suflen, suffix); -} - /* * Used as the default ->buf value, so that people can always assume * buf is non NULL and ->buf is NUL terminated even for a freshly From 2975c770ca609ea5afc80631c4ac9087c527b6fd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 30 Jun 2014 12:58:25 -0400 Subject: [PATCH 4/9] replace has_extension with ends_with These two are almost the same function, with the exception that has_extension only matches if there is content before the suffix. So ends_with(".exe", ".exe") is true, but has_extension would not be. This distinction does not matter to any of the callers, though, and we can just replace uses of has_extension with ends_with. We prefer the "ends_with" name because it is more generic, and there is nothing about the function that requires it to be used for file extensions. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 4 ++-- builtin/verify-pack.c | 4 ++-- git-compat-util.h | 7 ------- help.c | 2 +- refs.c | 4 ++-- sha1_file.c | 10 +++++----- 6 files changed, 12 insertions(+), 19 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 18f57de58b..46376b6af7 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1603,7 +1603,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) die(_("--fix-thin cannot be used without --stdin")); if (!index_name && pack_name) { int len = strlen(pack_name); - if (!has_extension(pack_name, ".pack")) + if (!ends_with(pack_name, ".pack")) die(_("packfile name '%s' does not end with '.pack'"), pack_name); index_name_buf = xmalloc(len); @@ -1613,7 +1613,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) } if (keep_msg && !keep_name && pack_name) { int len = strlen(pack_name); - if (!has_extension(pack_name, ".pack")) + if (!ends_with(pack_name, ".pack")) die(_("packfile name '%s' does not end with '.pack'"), pack_name); keep_name_buf = xmalloc(len); diff --git a/builtin/verify-pack.c b/builtin/verify-pack.c index 66cd2df0f8..2fd29cee8d 100644 --- a/builtin/verify-pack.c +++ b/builtin/verify-pack.c @@ -27,9 +27,9 @@ static int verify_one_pack(const char *path, unsigned int flags) * normalize these forms to "foo.pack" for "index-pack --verify". */ strbuf_addstr(&arg, path); - if (has_extension(arg.buf, ".idx")) + if (ends_with(arg.buf, ".idx")) strbuf_splice(&arg, arg.len - 3, 3, "pack", 4); - else if (!has_extension(arg.buf, ".pack")) + else if (!ends_with(arg.buf, ".pack")) strbuf_add(&arg, ".pack", 5); argv[2] = arg.buf; diff --git a/git-compat-util.h b/git-compat-util.h index 1c9c68cb89..47a49c355d 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -578,13 +578,6 @@ static inline size_t xsize_t(off_t len) return (size_t)len; } -static inline int has_extension(const char *filename, const char *ext) -{ - size_t len = strlen(filename); - size_t extlen = strlen(ext); - return len > extlen && !memcmp(filename + len - extlen, ext, extlen); -} - /* in ctype.c, for kwset users */ extern const char tolower_trans_tbl[256]; diff --git a/help.c b/help.c index b266b09320..372728fe7e 100644 --- a/help.c +++ b/help.c @@ -156,7 +156,7 @@ static void list_commands_in_dir(struct cmdnames *cmds, continue; entlen = strlen(de->d_name) - prefix_len; - if (has_extension(de->d_name, ".exe")) + if (ends_with(de->d_name, ".exe")) entlen -= 4; add_cmdname(cmds, de->d_name + prefix_len, entlen); diff --git a/refs.c b/refs.c index 59fb70087a..02ce29c575 100644 --- a/refs.c +++ b/refs.c @@ -1151,7 +1151,7 @@ static void read_loose_refs(const char *dirname, struct ref_dir *dir) if (de->d_name[0] == '.') continue; - if (has_extension(de->d_name, ".lock")) + if (ends_with(de->d_name, ".lock")) continue; strbuf_addstr(&refname, de->d_name); refdir = *refs->name @@ -3215,7 +3215,7 @@ static int do_for_each_reflog(struct strbuf *name, each_ref_fn fn, void *cb_data if (de->d_name[0] == '.') continue; - if (has_extension(de->d_name, ".lock")) + if (ends_with(de->d_name, ".lock")) continue; strbuf_addstr(name, de->d_name); if (stat(git_path("logs/%s", name->buf), &st) < 0) { diff --git a/sha1_file.c b/sha1_file.c index 394fa4509e..93b794f58e 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -1204,7 +1204,7 @@ static void prepare_packed_git_one(char *objdir, int local) strbuf_setlen(&path, dirnamelen); strbuf_addstr(&path, de->d_name); - if (has_extension(de->d_name, ".idx")) { + if (ends_with(de->d_name, ".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)) @@ -1222,10 +1222,10 @@ static void prepare_packed_git_one(char *objdir, int local) if (!report_garbage) continue; - if (has_extension(de->d_name, ".idx") || - has_extension(de->d_name, ".pack") || - has_extension(de->d_name, ".bitmap") || - has_extension(de->d_name, ".keep")) + if (ends_with(de->d_name, ".idx") || + ends_with(de->d_name, ".pack") || + ends_with(de->d_name, ".bitmap") || + ends_with(de->d_name, ".keep")) string_list_append(&garbage, path.buf); else report_garbage("garbage found", path.buf); From 26936bfd9bde1ec46901bea3e53d4fb9ae1b4a4c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 30 Jun 2014 12:58:51 -0400 Subject: [PATCH 5/9] use strip_suffix instead of ends_with in simple cases When stripping a suffix like: if (ends_with(str, "foo")) buf = xmemdupz(str, strlen(str) - 3); we can instead use strip_suffix to avoid the constant 3, which must match the literal "foo" (we sometimes use strlen("foo") instead, but that means we are repeating ourselves). The example above becomes: if (strip_suffix(str, "foo", &len)) buf = xmemdupz(str, len); This also saves a strlen(), since we calculate the string length when detecting the suffix. Note that in some cases we also switch from xstrndup to xmemdupz, which saves a further strlen call. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/remote.c | 13 +++++++------ builtin/repack.c | 5 ++--- connected.c | 6 +++--- help.c | 5 ++--- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/builtin/remote.c b/builtin/remote.c index 9b3e368983..0a6f3ef040 100644 --- a/builtin/remote.c +++ b/builtin/remote.c @@ -265,16 +265,17 @@ static int config_read_branches(const char *key, const char *value, void *cb) struct string_list_item *item; struct branch_info *info; enum { REMOTE, MERGE, REBASE } type; + size_t key_len; key += 7; - if (ends_with(key, ".remote")) { - name = xstrndup(key, strlen(key) - 7); + if (strip_suffix(key, ".remote", &key_len)) { + name = xmemdupz(key, key_len); type = REMOTE; - } else if (ends_with(key, ".merge")) { - name = xstrndup(key, strlen(key) - 6); + } else if (strip_suffix(key, ".merge", &key_len)) { + name = xmemdupz(key, key_len); type = MERGE; - } else if (ends_with(key, ".rebase")) { - name = xstrndup(key, strlen(key) - 7); + } else if (strip_suffix(key, ".rebase", &key_len)) { + name = xmemdupz(key, key_len); type = REBASE; } else return 0; diff --git a/builtin/repack.c b/builtin/repack.c index 6b0b62dcb2..52f22ca3fb 100644 --- a/builtin/repack.c +++ b/builtin/repack.c @@ -77,16 +77,15 @@ static void get_non_kept_pack_filenames(struct string_list *fname_list) DIR *dir; struct dirent *e; char *fname; - size_t len; if (!(dir = opendir(packdir))) return; while ((e = readdir(dir)) != NULL) { - if (!ends_with(e->d_name, ".pack")) + size_t len; + if (!strip_suffix(e->d_name, ".pack", &len)) continue; - len = strlen(e->d_name) - strlen(".pack"); fname = xmemdupz(e->d_name, len); if (!file_exists(mkpath("%s/%s.keep", packdir, fname))) diff --git a/connected.c b/connected.c index be0253e21b..dae9c9972e 100644 --- a/connected.c +++ b/connected.c @@ -31,6 +31,7 @@ static int check_everything_connected_real(sha1_iterate_fn fn, unsigned char sha1[20]; int err = 0, ac = 0; struct packed_git *new_pack = NULL; + size_t base_len; if (fn(cb_data, sha1)) return err; @@ -38,10 +39,9 @@ static int check_everything_connected_real(sha1_iterate_fn fn, if (transport && transport->smart_options && transport->smart_options->self_contained_and_connected && transport->pack_lockfile && - ends_with(transport->pack_lockfile, ".keep")) { + strip_suffix(transport->pack_lockfile, ".keep", &base_len)) { struct strbuf idx_file = STRBUF_INIT; - strbuf_addstr(&idx_file, transport->pack_lockfile); - strbuf_setlen(&idx_file, idx_file.len - 5); /* ".keep" */ + strbuf_add(&idx_file, transport->pack_lockfile, base_len); strbuf_addstr(&idx_file, ".idx"); new_pack = add_packed_git(idx_file.buf, idx_file.len, 1); strbuf_release(&idx_file); diff --git a/help.c b/help.c index 372728fe7e..97567c4523 100644 --- a/help.c +++ b/help.c @@ -145,7 +145,7 @@ static void list_commands_in_dir(struct cmdnames *cmds, len = buf.len; while ((de = readdir(dir)) != NULL) { - int entlen; + size_t entlen; if (!starts_with(de->d_name, prefix)) continue; @@ -156,8 +156,7 @@ static void list_commands_in_dir(struct cmdnames *cmds, continue; entlen = strlen(de->d_name) - prefix_len; - if (ends_with(de->d_name, ".exe")) - entlen -= 4; + strip_suffix(de->d_name, ".exe", &entlen); add_cmdname(cmds, de->d_name + prefix_len, entlen); } From 592ce20820ac36dd868dfd1e61b1aeb3cd3f902a Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 30 Jun 2014 12:59:10 -0400 Subject: [PATCH 6/9] index-pack: use strip_suffix to avoid magic numbers We also switch to using strbufs, which lets us avoid the potentially dangerous combination of a manual malloc followed by a strcpy. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/index-pack.c | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index 46376b6af7..d4b77fd128 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -1505,7 +1505,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) const char *curr_index; const char *index_name = NULL, *pack_name = NULL; const char *keep_name = NULL, *keep_msg = NULL; - char *index_name_buf = NULL, *keep_name_buf = NULL; + struct strbuf index_name_buf = STRBUF_INIT, + keep_name_buf = STRBUF_INIT; struct pack_idx_entry **idx_objects; struct pack_idx_option opts; unsigned char pack_sha1[20]; @@ -1602,24 +1603,22 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) if (fix_thin_pack && !from_stdin) die(_("--fix-thin cannot be used without --stdin")); if (!index_name && pack_name) { - int len = strlen(pack_name); - if (!ends_with(pack_name, ".pack")) + size_t len; + if (!strip_suffix(pack_name, ".pack", &len)) die(_("packfile name '%s' does not end with '.pack'"), pack_name); - index_name_buf = xmalloc(len); - memcpy(index_name_buf, pack_name, len - 5); - strcpy(index_name_buf + len - 5, ".idx"); - index_name = index_name_buf; + strbuf_add(&index_name_buf, pack_name, len); + strbuf_addstr(&index_name_buf, ".idx"); + index_name = index_name_buf.buf; } if (keep_msg && !keep_name && pack_name) { - int len = strlen(pack_name); - if (!ends_with(pack_name, ".pack")) + size_t len; + if (!strip_suffix(pack_name, ".pack", &len)) die(_("packfile name '%s' does not end with '.pack'"), pack_name); - keep_name_buf = xmalloc(len); - memcpy(keep_name_buf, pack_name, len - 5); - strcpy(keep_name_buf + len - 5, ".keep"); - keep_name = keep_name_buf; + strbuf_add(&keep_name_buf, pack_name, len); + strbuf_addstr(&keep_name_buf, ".idx"); + keep_name = keep_name_buf.buf; } if (verify) { if (!index_name) @@ -1667,8 +1666,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix) else close(input_fd); free(objects); - free(index_name_buf); - free(keep_name_buf); + strbuf_release(&index_name_buf); + strbuf_release(&keep_name_buf); if (pack_name == NULL) free((void *) curr_pack); if (index_name == NULL) From 6dda4e60f2c3c309de6e3fe1b86a47846a86dabf Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 30 Jun 2014 13:01:51 -0400 Subject: [PATCH 7/9] strbuf: implement strbuf_strip_suffix You can almost get away with just calling "strip_suffix_mem" on a strbuf's buf and len fields. But we also need to move the NUL-terminator to satisfy strbuf's invariants. Let's provide a convenience wrapper that handles this. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- strbuf.h | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/strbuf.h b/strbuf.h index 39c14cfa38..1d768c1124 100644 --- a/strbuf.h +++ b/strbuf.h @@ -47,6 +47,15 @@ extern void strbuf_rtrim(struct strbuf *); extern void strbuf_ltrim(struct strbuf *); extern int strbuf_cmp(const struct strbuf *, const struct strbuf *); +static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix) +{ + if (strip_suffix_mem(sb->buf, &sb->len, suffix)) { + strbuf_setlen(sb, sb->len); + return 1; + } else + return 0; +} + /* * Split str (of length slen) at the specified terminator character. * Return a null-terminated array of pointers to strbuf objects From d6cd00c76866a6412e0c13da91a022acdd187a47 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 30 Jun 2014 13:02:05 -0400 Subject: [PATCH 8/9] verify-pack: use strbuf_strip_suffix In this code, we try to convert both "foo.idx" and "foo" into "foo.pack". By stripping the suffix, we can avoid a confusing use of strbuf_splice, and make it clear that both cases are adding ".pack" to the end. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/verify-pack.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/builtin/verify-pack.c b/builtin/verify-pack.c index 2fd29cee8d..972579f33c 100644 --- a/builtin/verify-pack.c +++ b/builtin/verify-pack.c @@ -27,10 +27,9 @@ static int verify_one_pack(const char *path, unsigned int flags) * normalize these forms to "foo.pack" for "index-pack --verify". */ strbuf_addstr(&arg, path); - if (ends_with(arg.buf, ".idx")) - strbuf_splice(&arg, arg.len - 3, 3, "pack", 4); - else if (!ends_with(arg.buf, ".pack")) - strbuf_add(&arg, ".pack", 5); + if (strbuf_strip_suffix(&arg, ".idx") || + !ends_with(arg.buf, ".pack")) + strbuf_addstr(&arg, ".pack"); argv[2] = arg.buf; memset(&index_pack, 0, sizeof(index_pack)); From 47bf4b0fc52f3ad5823185a85f5f82325787c84b Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 30 Jun 2014 13:04:03 -0400 Subject: [PATCH 9/9] prepare_packed_git_one: refactor duplicate-pack check 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 Signed-off-by: Junio C Hamano --- sha1_file.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/sha1_file.c b/sha1_file.c index 93b794f58e..129a4c52df 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -1197,6 +1197,7 @@ 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; @@ -1204,10 +1205,14 @@ static void prepare_packed_git_one(char *objdir, int local) 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 &&