From a5b34d21521c015cd41ced4a3fdde57d79891eb3 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 3 Oct 2016 16:35:03 -0400 Subject: alternates: provide helper for adding to alternates list The submodule code wants to temporarily add an alternate object store to our in-memory alt_odb list, but does it manually. Let's provide a helper so it can reuse the code in link_alt_odb_entry(). While we're adding our new add_to_alternates_memory(), let's document add_to_alternates_file(), as the two are related. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) (limited to 'cache.h') diff --git a/cache.h b/cache.h index 6fc0e5ae68..258ce6b0f0 100644 --- a/cache.h +++ b/cache.h @@ -1389,10 +1389,22 @@ extern struct alternate_object_database { extern void prepare_alt_odb(void); extern void read_info_alternates(const char * relative_base, int depth); extern char *compute_alternate_path(const char *path, struct strbuf *err); -extern void add_to_alternates_file(const char *reference); typedef int alt_odb_fn(struct alternate_object_database *, void *); extern int foreach_alt_odb(alt_odb_fn, void*); +/* + * Add the directory to the on-disk alternates file; the new entry will also + * take effect in the current process. + */ +extern void add_to_alternates_file(const char *dir); + +/* + * Add the directory to the in-memory list of alternates (along with any + * recursive alternates it points to), but do not modify the on-disk alternates + * file. + */ +extern void add_to_alternates_memory(const char *dir); + struct pack_window { struct pack_window *next; unsigned char *base; -- cgit v1.3 From 7f0fa2c02a6543bdadae3c4a492daae7dbc8c042 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 3 Oct 2016 16:35:31 -0400 Subject: alternates: provide helper for allocating alternate Allocating a struct alternate_object_database is tricky, as we must over-allocate the buffer to provide scratch space, and then put in particular '/' and NUL markers. Let's encapsulate this in a function so that the complexity doesn't leak into callers (and so that we can modify it later). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache.h | 6 ++++++ sha1_file.c | 28 +++++++++++++++++++--------- sha1_name.c | 7 +------ 3 files changed, 26 insertions(+), 15 deletions(-) (limited to 'cache.h') diff --git a/cache.h b/cache.h index 258ce6b0f0..ece2c7c9b8 100644 --- a/cache.h +++ b/cache.h @@ -1392,6 +1392,12 @@ extern char *compute_alternate_path(const char *path, struct strbuf *err); typedef int alt_odb_fn(struct alternate_object_database *, void *); extern int foreach_alt_odb(alt_odb_fn, void*); +/* + * Allocate a "struct alternate_object_database" but do _not_ actually + * add it to the list of alternates. + */ +struct alternate_object_database *alloc_alt_odb(const char *dir); + /* * Add the directory to the on-disk alternates file; the new entry will also * take effect in the current process. diff --git a/sha1_file.c b/sha1_file.c index 7a3b745448..636d07ad14 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -283,7 +283,6 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, int depth, const char *normalized_objdir) { struct alternate_object_database *ent; - size_t entlen; struct strbuf pathbuf = STRBUF_INIT; if (!is_absolute_path(entry) && relative_base) { @@ -311,14 +310,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, return -1; } - entlen = st_add(pathbuf.len, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */ - ent = xmalloc(st_add(sizeof(*ent), entlen)); - memcpy(ent->base, pathbuf.buf, pathbuf.len); - - ent->name = ent->base + pathbuf.len + 1; - ent->base[pathbuf.len] = '/'; - ent->base[pathbuf.len + 3] = '/'; - ent->base[entlen-1] = 0; + ent = alloc_alt_odb(pathbuf.buf); /* add the alternate entry */ *alt_odb_tail = ent; @@ -395,6 +387,24 @@ void read_info_alternates(const char * relative_base, int depth) munmap(map, mapsz); } +struct alternate_object_database *alloc_alt_odb(const char *dir) +{ + struct alternate_object_database *ent; + size_t dirlen = strlen(dir); + size_t entlen; + + entlen = st_add(dirlen, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */ + ent = xmalloc(st_add(sizeof(*ent), entlen)); + memcpy(ent->base, dir, dirlen); + + ent->name = ent->base + dirlen + 1; + ent->base[dirlen] = '/'; + ent->base[dirlen + 3] = '/'; + ent->base[entlen-1] = 0; + + return ent; +} + void add_to_alternates_file(const char *reference) { struct lock_file *lock = xcalloc(1, sizeof(struct lock_file)); diff --git a/sha1_name.c b/sha1_name.c index faf873cf7f..98152a68ba 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -86,12 +86,7 @@ static void find_short_object_filename(int len, const char *hex_pfx, struct disa * alt->name/alt->base while iterating over the * object databases including our own. */ - const char *objdir = get_object_directory(); - size_t objdir_len = strlen(objdir); - fakeent = xmalloc(st_add3(sizeof(*fakeent), objdir_len, 43)); - memcpy(fakeent->base, objdir, objdir_len); - fakeent->name = fakeent->base + objdir_len + 1; - fakeent->name[-1] = '/'; + fakeent = alloc_alt_odb(get_object_directory()); } fakeent->next = alt_odb_list; -- cgit v1.3 From 597f9134ded20f882e2bf6bca8b0e1f03981b98d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 3 Oct 2016 16:35:51 -0400 Subject: alternates: use a separate scratch space The alternate_object_database struct uses a single buffer both for storing the path to the alternate, and as a scratch buffer for forming object names. This is efficient (since otherwise we'd end up storing the path twice), but it makes life hard for callers who just want to know the path to the alternate. They have to remember to stop reading after "alt->name - alt->base" bytes, and to subtract one for the trailing '/'. It would be much simpler if they could simply access a NUL-terminated path string. We could encapsulate this in a function which puts a NUL in the scratch buffer and returns the string, but that opens up questions about the lifetime of the result. The first time another caller uses the alternate, the scratch buffer may get other data tacked onto it. Let's instead just store the root path separately from the scratch buffer. There aren't enough alternates being stored for the duplicated data to matter for performance, and this keeps things simple and safe for the callers. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/fsck.c | 10 ++-------- builtin/submodule--helper.c | 11 +++-------- cache.h | 5 ++++- sha1_file.c | 28 ++++++++++++---------------- sha1_name.c | 3 ++- transport.c | 4 +--- 6 files changed, 24 insertions(+), 37 deletions(-) (limited to 'cache.h') diff --git a/builtin/fsck.c b/builtin/fsck.c index 055dfdcf9e..f01b81eebf 100644 --- a/builtin/fsck.c +++ b/builtin/fsck.c @@ -644,14 +644,8 @@ int cmd_fsck(int argc, const char **argv, const char *prefix) fsck_object_dir(get_object_directory()); prepare_alt_odb(); - for (alt = alt_odb_list; alt; alt = alt->next) { - /* directory name, minus trailing slash */ - size_t namelen = alt->name - alt->base - 1; - struct strbuf name = STRBUF_INIT; - strbuf_add(&name, alt->base, namelen); - fsck_object_dir(name.buf); - strbuf_release(&name); - } + for (alt = alt_odb_list; alt; alt = alt->next) + fsck_object_dir(alt->path); } if (check_full) { diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index e3fdc0aa78..fd72c90442 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -492,20 +492,16 @@ static int add_possible_reference_from_superproject( { struct submodule_alternate_setup *sas = sas_cb; - /* directory name, minus trailing slash */ - size_t namelen = alt->name - alt->base - 1; - struct strbuf name = STRBUF_INIT; - strbuf_add(&name, alt->base, namelen); - /* * If the alternate object store is another repository, try the * standard layout with .git/modules//objects */ - if (ends_with(name.buf, ".git/objects")) { + if (ends_with(alt->path, ".git/objects")) { char *sm_alternate; struct strbuf sb = STRBUF_INIT; struct strbuf err = STRBUF_INIT; - strbuf_add(&sb, name.buf, name.len - strlen("objects")); + strbuf_add(&sb, alt->path, strlen(alt->path) - strlen("objects")); + /* * We need to end the new path with '/' to mark it as a dir, * otherwise a submodule name containing '/' will be broken @@ -533,7 +529,6 @@ static int add_possible_reference_from_superproject( strbuf_release(&sb); } - strbuf_release(&name); return 0; } diff --git a/cache.h b/cache.h index ece2c7c9b8..555ba713de 100644 --- a/cache.h +++ b/cache.h @@ -1383,8 +1383,11 @@ extern void remove_scheduled_dirs(void); extern struct alternate_object_database { struct alternate_object_database *next; + char *name; - char base[FLEX_ARRAY]; /* more */ + char *scratch; + + char path[FLEX_ARRAY]; } *alt_odb_list; extern void prepare_alt_odb(void); extern void read_info_alternates(const char * relative_base, int depth); diff --git a/sha1_file.c b/sha1_file.c index b284cea8fb..011532a709 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -208,7 +208,7 @@ static const char *alt_sha1_path(struct alternate_object_database *alt, const unsigned char *sha1) { fill_sha1_path(alt->name, sha1); - return alt->base; + return alt->scratch; } /* @@ -261,8 +261,7 @@ static int alt_odb_usable(struct strbuf *path, const char *normalized_objdir) * thing twice, or object directory itself. */ for (alt = alt_odb_list; alt; alt = alt->next) { - if (path->len == alt->name - alt->base - 1 && - !memcmp(path->buf, alt->base, path->len)) + if (!strcmp(path->buf, alt->path)) return 0; } if (!fspathcmp(path->buf, normalized_objdir)) @@ -401,13 +400,14 @@ struct alternate_object_database *alloc_alt_odb(const char *dir) size_t entlen; entlen = st_add(dirlen, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */ - ent = xmalloc(st_add(sizeof(*ent), entlen)); - memcpy(ent->base, dir, dirlen); + FLEX_ALLOC_STR(ent, path, dir); + ent->scratch = xmalloc(entlen); + xsnprintf(ent->scratch, entlen, "%s/", dir); - ent->name = ent->base + dirlen + 1; - ent->base[dirlen] = '/'; - ent->base[dirlen + 3] = '/'; - ent->base[entlen-1] = 0; + ent->name = ent->scratch + dirlen + 1; + ent->scratch[dirlen] = '/'; + ent->scratch[dirlen + 3] = '/'; + ent->scratch[entlen-1] = 0; return ent; } @@ -1485,11 +1485,8 @@ void prepare_packed_git(void) return; prepare_packed_git_one(get_object_directory(), 1); prepare_alt_odb(); - for (alt = alt_odb_list; alt; alt = alt->next) { - alt->name[-1] = 0; - prepare_packed_git_one(alt->base, 0); - alt->name[-1] = '/'; - } + for (alt = alt_odb_list; alt; alt = alt->next) + prepare_packed_git_one(alt->path, 0); rearrange_packed_git(); prepare_packed_git_mru(); prepare_packed_git_run_once = 1; @@ -3692,8 +3689,7 @@ static int loose_from_alt_odb(struct alternate_object_database *alt, struct strbuf buf = STRBUF_INIT; int r; - /* copy base not including trailing '/' */ - strbuf_add(&buf, alt->base, alt->name - alt->base - 1); + strbuf_addstr(&buf, alt->path); r = for_each_loose_file_in_objdir_buf(&buf, data->cb, NULL, NULL, data->data); diff --git a/sha1_name.c b/sha1_name.c index 98152a68ba..770ea4fe80 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -94,12 +94,13 @@ static void find_short_object_filename(int len, const char *hex_pfx, struct disa for (alt = fakeent; alt && !ds->ambiguous; alt = alt->next) { struct dirent *de; DIR *dir; + /* * every alt_odb struct has 42 extra bytes after the base * for exactly this purpose */ xsnprintf(alt->name, 42, "%.2s/", hex_pfx); - dir = opendir(alt->base); + dir = opendir(alt->scratch); if (!dir) continue; diff --git a/transport.c b/transport.c index 94d6dc3725..4bc4eeae54 100644 --- a/transport.c +++ b/transport.c @@ -1084,9 +1084,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e, const struct ref *extra; struct alternate_refs_data *cb = data; - e->name[-1] = '\0'; - other = xstrdup(real_path(e->base)); - e->name[-1] = '/'; + other = xstrdup(real_path(e->path)); len = strlen(other); while (other[len-1] == '/') -- cgit v1.3 From 38dbe5f07837afceaec95fae5981d36eeb4917bd Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 3 Oct 2016 16:36:04 -0400 Subject: alternates: store scratch buffer as strbuf We pre-size the scratch buffer to hold a loose object filename of the form "xx/yyyy...", which leads to allocation code that is hard to verify. We have to use some magic numbers during the initial allocation, and then writers must blindly assume that the buffer is big enough. Using a strbuf makes it more clear that we cannot overflow. Unfortunately, we do still need some magic numbers to grow our strbuf before calling fill_sha1_path(), but the strbuf growth is much closer to the point of use. This makes it easier to see that it's correct, and opens the possibility of pushing it even further down if fill_sha1_path() learns to work on strbufs. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- cache.h | 13 +++++++++++-- sha1_file.c | 28 ++++++++++++++++++---------- sha1_name.c | 9 +++------ 3 files changed, 32 insertions(+), 18 deletions(-) (limited to 'cache.h') diff --git a/cache.h b/cache.h index 555ba713de..5d36ffa1f2 100644 --- a/cache.h +++ b/cache.h @@ -1384,8 +1384,9 @@ extern void remove_scheduled_dirs(void); extern struct alternate_object_database { struct alternate_object_database *next; - char *name; - char *scratch; + /* see alt_scratch_buf() */ + struct strbuf scratch; + size_t base_len; char path[FLEX_ARRAY]; } *alt_odb_list; @@ -1414,6 +1415,14 @@ extern void add_to_alternates_file(const char *dir); */ extern void add_to_alternates_memory(const char *dir); +/* + * Returns a scratch strbuf pre-filled with the alternate object directory, + * including a trailing slash, which can be used to access paths in the + * alternate. Always use this over direct access to alt->scratch, as it + * cleans up any previous use of the scratch buffer. + */ +extern struct strbuf *alt_scratch_buf(struct alternate_object_database *alt); + struct pack_window { struct pack_window *next; unsigned char *base; diff --git a/sha1_file.c b/sha1_file.c index da7b922605..51d40241a7 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -204,11 +204,24 @@ const char *sha1_file_name(const unsigned char *sha1) return buf; } +struct strbuf *alt_scratch_buf(struct alternate_object_database *alt) +{ + strbuf_setlen(&alt->scratch, alt->base_len); + return &alt->scratch; +} + static const char *alt_sha1_path(struct alternate_object_database *alt, const unsigned char *sha1) { - fill_sha1_path(alt->name, sha1); - return alt->scratch; + /* hex sha1 plus internal "/" */ + size_t len = GIT_SHA1_HEXSZ + 1; + struct strbuf *buf = alt_scratch_buf(alt); + + strbuf_grow(buf, len); + fill_sha1_path(buf->buf + buf->len, sha1); + strbuf_setlen(buf, buf->len + len); + + return buf->buf; } /* @@ -396,16 +409,11 @@ void read_info_alternates(const char * relative_base, int depth) struct alternate_object_database *alloc_alt_odb(const char *dir) { struct alternate_object_database *ent; - size_t dirlen = strlen(dir); - size_t entlen; - entlen = st_add(dirlen, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */ FLEX_ALLOC_STR(ent, path, dir); - ent->scratch = xmalloc(entlen); - xsnprintf(ent->scratch, entlen, "%s/", dir); - - ent->name = ent->scratch + dirlen + 1; - ent->scratch[dirlen] = '/'; + strbuf_init(&ent->scratch, 0); + strbuf_addf(&ent->scratch, "%s/", dir); + ent->base_len = ent->scratch.len; return ent; } diff --git a/sha1_name.c b/sha1_name.c index 770ea4fe80..defbb3eb05 100644 --- a/sha1_name.c +++ b/sha1_name.c @@ -92,15 +92,12 @@ static void find_short_object_filename(int len, const char *hex_pfx, struct disa xsnprintf(hex, sizeof(hex), "%.2s", hex_pfx); for (alt = fakeent; alt && !ds->ambiguous; alt = alt->next) { + struct strbuf *buf = alt_scratch_buf(alt); struct dirent *de; DIR *dir; - /* - * every alt_odb struct has 42 extra bytes after the base - * for exactly this purpose - */ - xsnprintf(alt->name, 42, "%.2s/", hex_pfx); - dir = opendir(alt->scratch); + strbuf_addf(buf, "%.2s/", hex_pfx); + dir = opendir(buf->buf); if (!dir) continue; -- cgit v1.3