From 670c359da357639f9f9a814ed646b4d854ec5d55 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 3 Oct 2016 16:34:17 -0400 Subject: link_alt_odb_entry: handle normalize_path errors When we add a new alternate to the list, we try to normalize out any redundant "..", etc. However, we do not look at the return value of normalize_path_copy(), and will happily continue with a path that could not be normalized. Worse, the normalizing process is done in-place, so we are left with whatever half-finished working state the normalizing function was in. Fortunately, this cannot cause us to read past the end of our buffer, as that working state will always leave the NUL from the original path in place. And we do tend to notice problems when we check is_directory() on the path. But you can see the nonsense that we feed to is_directory with an entry like: this/../../is/../../way/../../too/../../deep/../../to/../../resolve in your objects/info/alternates, which yields: error: object directory /to/e/deep/too/way//ects/this/../../is/../../way/../../too/../../deep/../../to/../../resolve does not exist; check .git/objects/info/alternates. We can easily fix this just by checking the return value. But that makes it hard to generate a good error message, since we're normalizing in-place and our input value has been overwritten by cruft. Instead, let's provide a strbuf helper that does an in-place normalize, but restores the original contents on error. This uses a second buffer under the hood, which is slightly less efficient, but this is not a performance-critical code path. The strbuf helper can also properly set the "len" parameter of the strbuf before returning. Just doing: normalize_path_copy(buf.buf, buf.buf); will shorten the string, but leave buf.len at the original length. That may be confusing to later code which uses the strbuf. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sha1_file.c | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) (limited to 'sha1_file.c') diff --git a/sha1_file.c b/sha1_file.c index 94daf31ec6..057262d46e 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -263,7 +263,12 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, } strbuf_addstr(&pathbuf, entry); - normalize_path_copy(pathbuf.buf, pathbuf.buf); + if (strbuf_normalize_path(&pathbuf) < 0) { + error("unable to normalize alternate object path: %s", + pathbuf.buf); + strbuf_release(&pathbuf); + return -1; + } pfxlen = strlen(pathbuf.buf); @@ -335,7 +340,9 @@ static void link_alt_odb_entries(const char *alt, int len, int sep, } strbuf_add_absolute_path(&objdirbuf, get_object_directory()); - normalize_path_copy(objdirbuf.buf, objdirbuf.buf); + if (strbuf_normalize_path(&objdirbuf) < 0) + die("unable to normalize object directory: %s", + objdirbuf.buf); alt_copy = xmemdupz(alt, len); string_list_split_in_place(&entries, alt_copy, sep, -1); -- cgit v1.3-5-g9baa From 4ea82473aa310de7543141f96c2e6b23ef9fcd4c Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 3 Oct 2016 16:34:48 -0400 Subject: link_alt_odb_entry: refactor string handling The string handling in link_alt_odb_entry() is mostly an artifact of the original version, which took the path as a ptr/len combo, and did not have a NUL-terminated string until we created one in the alternate_object_database struct. But since 5bdf0a8 (sha1_file: normalize alt_odb path before comparing and storing, 2011-09-07), the first thing we do is put the path into a strbuf, which gives us some easy opportunities for cleanup. In particular: - we call strlen(pathbuf.buf), which is silly; we can look at pathbuf.len. - even though we have a strbuf, we don't maintain its "len" field when chomping extra slashes from the end, and instead keep a separate "pfxlen" variable. We can fix this and then drop "pfxlen" entirely. - we don't check whether the path is usable until after we allocate the new struct, making extra cleanup work for ourselves. Since we have a NUL-terminated string, we can bump the "is it usable" checks higher in the function. While we're at it, we can move that logic to its own helper, which makes the flow of link_alt_odb_entry() easier to follow. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sha1_file.c | 83 +++++++++++++++++++++++++++++++++---------------------------- 1 file changed, 45 insertions(+), 38 deletions(-) (limited to 'sha1_file.c') diff --git a/sha1_file.c b/sha1_file.c index 057262d46e..3d8c1e88a5 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -234,6 +234,36 @@ char *sha1_pack_index_name(const unsigned char *sha1) struct alternate_object_database *alt_odb_list; static struct alternate_object_database **alt_odb_tail; +/* + * Return non-zero iff the path is usable as an alternate object database. + */ +static int alt_odb_usable(struct strbuf *path, const char *normalized_objdir) +{ + struct alternate_object_database *alt; + + /* Detect cases where alternate disappeared */ + if (!is_directory(path->buf)) { + error("object directory %s does not exist; " + "check .git/objects/info/alternates.", + path->buf); + return 0; + } + + /* + * Prevent the common mistake of listing the same + * 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)) + return 0; + } + if (!fspathcmp(path->buf, normalized_objdir)) + return 0; + + return 1; +} + /* * Prepare alternate object database registry. * @@ -253,8 +283,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, int depth, const char *normalized_objdir) { struct alternate_object_database *ent; - struct alternate_object_database *alt; - size_t pfxlen, entlen; + size_t entlen; struct strbuf pathbuf = STRBUF_INIT; if (!is_absolute_path(entry) && relative_base) { @@ -270,47 +299,26 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, return -1; } - pfxlen = strlen(pathbuf.buf); - /* * The trailing slash after the directory name is given by * this function at the end. Remove duplicates. */ - while (pfxlen && pathbuf.buf[pfxlen-1] == '/') - pfxlen -= 1; - - entlen = st_add(pfxlen, 43); /* '/' + 2 hex + '/' + 38 hex + NUL */ - ent = xmalloc(st_add(sizeof(*ent), entlen)); - memcpy(ent->base, pathbuf.buf, pfxlen); - strbuf_release(&pathbuf); - - ent->name = ent->base + pfxlen + 1; - ent->base[pfxlen + 3] = '/'; - ent->base[pfxlen] = ent->base[entlen-1] = 0; + while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/') + strbuf_setlen(&pathbuf, pathbuf.len - 1); - /* Detect cases where alternate disappeared */ - if (!is_directory(ent->base)) { - error("object directory %s does not exist; " - "check .git/objects/info/alternates.", - ent->base); - free(ent); + if (!alt_odb_usable(&pathbuf, normalized_objdir)) { + strbuf_release(&pathbuf); return -1; } - /* Prevent the common mistake of listing the same - * thing twice, or object directory itself. - */ - for (alt = alt_odb_list; alt; alt = alt->next) { - if (pfxlen == alt->name - alt->base - 1 && - !memcmp(ent->base, alt->base, pfxlen)) { - free(ent); - return -1; - } - } - if (!fspathcmp(ent->base, normalized_objdir)) { - free(ent); - 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; /* add the alternate entry */ *alt_odb_tail = ent; @@ -318,10 +326,9 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base, ent->next = NULL; /* recursively add alternates */ - read_info_alternates(ent->base, depth + 1); - - ent->base[pfxlen] = '/'; + read_info_alternates(pathbuf.buf, depth + 1); + strbuf_release(&pathbuf); return 0; } -- cgit v1.3-5-g9baa 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 +++++++++++++- sha1_file.c | 11 +++++++++++ submodule.c | 23 +---------------------- 3 files changed, 25 insertions(+), 23 deletions(-) (limited to 'sha1_file.c') 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; diff --git a/sha1_file.c b/sha1_file.c index 3d8c1e88a5..7a3b745448 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -440,6 +440,17 @@ void add_to_alternates_file(const char *reference) free(alts); } +void add_to_alternates_memory(const char *reference) +{ + /* + * Make sure alternates are initialized, or else our entry may be + * overwritten when they are. + */ + prepare_alt_odb(); + + link_alt_odb_entries(reference, strlen(reference), '\n', NULL, 0); +} + /* * Compute the exact path an alternate is at and returns it. In case of * error NULL is returned and the human readable error is added to `err` diff --git a/submodule.c b/submodule.c index 0ef2ff4321..8b3274a9dc 100644 --- a/submodule.c +++ b/submodule.c @@ -123,9 +123,7 @@ void stage_updated_gitmodules(void) static int add_submodule_odb(const char *path) { struct strbuf objects_directory = STRBUF_INIT; - struct alternate_object_database *alt_odb; int ret = 0; - size_t alloc; ret = strbuf_git_path_submodule(&objects_directory, path, "objects/"); if (ret) @@ -134,26 +132,7 @@ static int add_submodule_odb(const char *path) ret = -1; goto done; } - /* avoid adding it twice */ - prepare_alt_odb(); - for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next) - if (alt_odb->name - alt_odb->base == objects_directory.len && - !strncmp(alt_odb->base, objects_directory.buf, - objects_directory.len)) - goto done; - - alloc = st_add(objects_directory.len, 42); /* for "12/345..." sha1 */ - alt_odb = xmalloc(st_add(sizeof(*alt_odb), alloc)); - alt_odb->next = alt_odb_list; - xsnprintf(alt_odb->base, alloc, "%s", objects_directory.buf); - alt_odb->name = alt_odb->base + objects_directory.len; - alt_odb->name[2] = '/'; - alt_odb->name[40] = '\0'; - alt_odb->name[41] = '\0'; - alt_odb_list = alt_odb; - - /* add possible alternates from the submodule */ - read_info_alternates(objects_directory.buf, 0); + add_to_alternates_memory(objects_directory.buf); done: strbuf_release(&objects_directory); return ret; -- cgit v1.3-5-g9baa 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 'sha1_file.c') 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-5-g9baa From 29ec6af2b81894d3236f2aec100323138023ef4d Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 3 Oct 2016 16:35:43 -0400 Subject: alternates: encapsulate alt->base munging The alternate_object_database struct holds a path to the alternate objects, but we also use that buffer as scratch space for forming loose object filenames. Let's pull that logic into a helper function so that we can more easily modify it. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sha1_file.c | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) (limited to 'sha1_file.c') diff --git a/sha1_file.c b/sha1_file.c index 636d07ad14..b284cea8fb 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -204,6 +204,13 @@ const char *sha1_file_name(const unsigned char *sha1) return buf; } +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 the name of the pack or index file with the specified sha1 * in its filename. *base and *name are scratch space that must be @@ -601,8 +608,8 @@ static int check_and_freshen_nonlocal(const unsigned char *sha1, int freshen) struct alternate_object_database *alt; prepare_alt_odb(); for (alt = alt_odb_list; alt; alt = alt->next) { - fill_sha1_path(alt->name, sha1); - if (check_and_freshen_file(alt->base, freshen)) + const char *path = alt_sha1_path(alt, sha1); + if (check_and_freshen_file(path, freshen)) return 1; } return 0; @@ -1600,8 +1607,8 @@ static int stat_sha1_file(const unsigned char *sha1, struct stat *st) prepare_alt_odb(); errno = ENOENT; for (alt = alt_odb_list; alt; alt = alt->next) { - fill_sha1_path(alt->name, sha1); - if (!lstat(alt->base, st)) + const char *path = alt_sha1_path(alt, sha1); + if (!lstat(path, st)) return 0; } @@ -1621,8 +1628,8 @@ static int open_sha1_file(const unsigned char *sha1) prepare_alt_odb(); for (alt = alt_odb_list; alt; alt = alt->next) { - fill_sha1_path(alt->name, sha1); - fd = git_open_noatime(alt->base); + const char *path = alt_sha1_path(alt, sha1); + fd = git_open_noatime(path); if (fd >= 0) return fd; if (most_interesting_errno == ENOENT) -- cgit v1.3-5-g9baa 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 'sha1_file.c') 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-5-g9baa From afbba2f09ab06424d8b38908f4d76a1503f49a25 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 3 Oct 2016 16:35:55 -0400 Subject: fill_sha1_file: write "boring" characters This function forms a sha1 as "xx/yyyy...", but skips over the slot for the slash rather than writing it, leaving it to the caller to do so. It also does not bother to put in a trailing NUL, even though every caller would want it (we're forming a path which by definition is not a directory, so the only thing to do with it is feed it to a system call). Let's make the lives of our callers easier by just writing out the internal "/" and the NUL. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sha1_file.c | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) (limited to 'sha1_file.c') diff --git a/sha1_file.c b/sha1_file.c index 011532a709..da7b922605 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -178,10 +178,12 @@ static void fill_sha1_path(char *pathbuf, const unsigned char *sha1) for (i = 0; i < 20; i++) { static char hex[] = "0123456789abcdef"; unsigned int val = sha1[i]; - char *pos = pathbuf + i*2 + (i > 0); - *pos++ = hex[val >> 4]; - *pos = hex[val & 0xf]; + *pathbuf++ = hex[val >> 4]; + *pathbuf++ = hex[val & 0xf]; + if (!i) + *pathbuf++ = '/'; } + *pathbuf = '\0'; } const char *sha1_file_name(const unsigned char *sha1) @@ -198,8 +200,6 @@ const char *sha1_file_name(const unsigned char *sha1) die("insanely long object directory %s", objdir); memcpy(buf, objdir, len); buf[len] = '/'; - buf[len+3] = '/'; - buf[len+42] = '\0'; fill_sha1_path(buf + len + 1, sha1); return buf; } @@ -406,8 +406,6 @@ struct alternate_object_database *alloc_alt_odb(const char *dir) ent->name = ent->scratch + dirlen + 1; ent->scratch[dirlen] = '/'; - ent->scratch[dirlen + 3] = '/'; - ent->scratch[entlen-1] = 0; return ent; } -- cgit v1.3-5-g9baa 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 'sha1_file.c') 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-5-g9baa From f7b7774f34b86fed8a2e9554a9fe865c62a0a5eb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 3 Oct 2016 16:36:09 -0400 Subject: fill_sha1_file: write into a strbuf It's currently the responsibility of the caller to give fill_sha1_file() enough bytes to write into, leading them to manually compute the required lengths. Instead, let's just write into a strbuf so that it's impossible to get this wrong. The alt_odb caller already has a strbuf, so this makes things strictly simpler. The other caller, sha1_file_name(), uses a static PATH_MAX buffer and dies when it would overflow. We can convert this to a static strbuf, which means our allocation cost is amortized (and as a bonus, we no longer have to worry about PATH_MAX being too short for normal use). This does introduce some small overhead in fill_sha1_file(), as each strbuf_addchar() will check whether it needs to grow. However, between the optimization in fec501d (strbuf_addch: avoid calling strbuf_grow, 2015-04-16) and the fact that this is not generally called in a tight loop (after all, the next step is typically to access the file!) this probably doesn't matter. And even if it did, the right place to micro-optimize is inside fill_sha1_file(), by calling a single strbuf_grow() there. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sha1_file.c | 34 ++++++++++------------------------ 1 file changed, 10 insertions(+), 24 deletions(-) (limited to 'sha1_file.c') diff --git a/sha1_file.c b/sha1_file.c index 51d40241a7..1b86a3ae71 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -172,36 +172,28 @@ enum scld_error safe_create_leading_directories_const(const char *path) return result; } -static void fill_sha1_path(char *pathbuf, const unsigned char *sha1) +static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1) { int i; for (i = 0; i < 20; i++) { static char hex[] = "0123456789abcdef"; unsigned int val = sha1[i]; - *pathbuf++ = hex[val >> 4]; - *pathbuf++ = hex[val & 0xf]; + strbuf_addch(buf, hex[val >> 4]); + strbuf_addch(buf, hex[val & 0xf]); if (!i) - *pathbuf++ = '/'; + strbuf_addch(buf, '/'); } - *pathbuf = '\0'; } const char *sha1_file_name(const unsigned char *sha1) { - static char buf[PATH_MAX]; - const char *objdir; - int len; + static struct strbuf buf = STRBUF_INIT; - objdir = get_object_directory(); - len = strlen(objdir); + strbuf_reset(&buf); + strbuf_addf(&buf, "%s/", get_object_directory()); - /* '/' + sha1(2) + '/' + sha1(38) + '\0' */ - if (len + 43 > PATH_MAX) - die("insanely long object directory %s", objdir); - memcpy(buf, objdir, len); - buf[len] = '/'; - fill_sha1_path(buf + len + 1, sha1); - return buf; + fill_sha1_path(&buf, sha1); + return buf.buf; } struct strbuf *alt_scratch_buf(struct alternate_object_database *alt) @@ -213,14 +205,8 @@ struct strbuf *alt_scratch_buf(struct alternate_object_database *alt) static const char *alt_sha1_path(struct alternate_object_database *alt, const unsigned char *sha1) { - /* 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); - + fill_sha1_path(buf, sha1); return buf->buf; } -- cgit v1.3-5-g9baa From 087b6d584062f5b704356286d6445bcc84d686fb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 3 Oct 2016 16:36:22 -0400 Subject: sha1_file: always allow relative paths to alternates We recursively expand alternates repositories, so that if A borrows from B which borrows from C, A can see all objects. For the root object database, we allow relative paths, so A can point to B as "../B/objects". However, we currently do not allow relative paths when recursing, so B must use an absolute path to reach C. That is an ancient protection from c2f493a (Transitively read alternatives, 2006-05-07) that tries to avoid adding the same alternate through two different paths. Since 5bdf0a8 (sha1_file: normalize alt_odb path before comparing and storing, 2011-09-07), we use a normalized absolute path for each alt_odb entry. This means that in most cases the protection is no longer necessary; we will detect the duplicate no matter how we got there (but see below). And it's a good idea to get rid of it, as it creates an unnecessary complication when setting up recursive alternates (B has to know that A is going to borrow from it and make sure to use an absolute path). Note that our normalization doesn't actually look at the filesystem, so it can still be fooled by crossing symbolic links. But that's also true of absolute paths, so it's not a good reason to disallow only relative paths (it's potentially a reason to switch to real_path(), but that's a separate and non-trivial change). We adjust the test script here to demonstrate that this now works, and add new tests to show that the normalization does indeed suppress duplicates. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- sha1_file.c | 7 +------ t/t5613-info-alternate.sh | 24 ++++++++++++++++++++++-- 2 files changed, 23 insertions(+), 8 deletions(-) (limited to 'sha1_file.c') diff --git a/sha1_file.c b/sha1_file.c index 1b86a3ae71..9cad56f7b0 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -354,12 +354,7 @@ static void link_alt_odb_entries(const char *alt, int len, int sep, const char *entry = entries.items[i].string; if (entry[0] == '\0' || entry[0] == '#') continue; - if (!is_absolute_path(entry) && depth) { - error("%s: ignoring relative alternate object store %s", - relative_base, entry); - } else { - link_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf); - } + link_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf); } string_list_clear(&entries, 0); free(alt_copy); diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh index e146a8def6..76f1a20e2c 100755 --- a/t/t5613-info-alternate.sh +++ b/t/t5613-info-alternate.sh @@ -95,8 +95,28 @@ test_expect_success 'that relative alternate is possible for current dir' ' git fsck ' -test_expect_success 'that relative alternate is only possible for current dir' ' - test_must_fail git -C D fsck +test_expect_success 'that relative alternate is recursive' ' + git -C D fsck +' + +# we can reach "A" from our new repo both directly, and via "C". +# The deep/subdir is there to make sure we are not doing a stupid +# pure-text comparison of the alternate names. +test_expect_success 'relative duplicates are eliminated' ' + mkdir -p deep/subdir && + git init --bare deep/subdir/duplicate.git && + cat >deep/subdir/duplicate.git/objects/info/alternates <<-\EOF && + ../../../../C/.git/objects + ../../../../A/.git/objects + EOF + cat >expect <<-EOF && + alternate: $(pwd)/C/.git/objects + alternate: $(pwd)/B/.git/objects + alternate: $(pwd)/A/.git/objects + EOF + git -C deep/subdir/duplicate.git count-objects -v >actual && + grep ^alternate: actual >actual.alternates && + test_cmp expect actual.alternates ' test_done -- cgit v1.3-5-g9baa From ea0fc3b4176a424a2b20eb76a6a503dc4d59cebb Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 3 Oct 2016 16:36:26 -0400 Subject: alternates: use fspathcmp to detect duplicates On a case-insensitive filesystem, we should realize that "a/objects" and "A/objects" are the same path. We already use fspathcmp() to check against the main object directory, but until recently we couldn't use it for comparing against other alternates (because their paths were not NUL-terminated strings). But now we can, so let's do so. Note that we also need to adjust count-objects to load the config, so that it can see the setting of core.ignorecase (this is required by the test, but is also a general bugfix for users of count-objects). Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- builtin/count-objects.c | 2 ++ sha1_file.c | 2 +- t/t5613-info-alternate.sh | 17 +++++++++++++++++ 3 files changed, 20 insertions(+), 1 deletion(-) (limited to 'sha1_file.c') diff --git a/builtin/count-objects.c b/builtin/count-objects.c index a700409bf5..a04b4f2ef3 100644 --- a/builtin/count-objects.c +++ b/builtin/count-objects.c @@ -97,6 +97,8 @@ int cmd_count_objects(int argc, const char **argv, const char *prefix) OPT_END(), }; + git_config(git_default_config, NULL); + argc = parse_options(argc, argv, prefix, opts, count_objects_usage, 0); /* we do not take arguments other than flags for now */ if (argc) diff --git a/sha1_file.c b/sha1_file.c index 9cad56f7b0..064651947d 100644 --- a/sha1_file.c +++ b/sha1_file.c @@ -260,7 +260,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 (!strcmp(path->buf, alt->path)) + if (!fspathcmp(path->buf, alt->path)) return 0; } if (!fspathcmp(path->buf, normalized_objdir)) diff --git a/t/t5613-info-alternate.sh b/t/t5613-info-alternate.sh index 76f1a20e2c..895f46bb91 100755 --- a/t/t5613-info-alternate.sh +++ b/t/t5613-info-alternate.sh @@ -119,4 +119,21 @@ test_expect_success 'relative duplicates are eliminated' ' test_cmp expect actual.alternates ' +test_expect_success CASE_INSENSITIVE_FS 'dup finding can be case-insensitive' ' + git init --bare insensitive.git && + # the previous entry for "A" will have used uppercase + cat >insensitive.git/objects/info/alternates <<-\EOF && + ../../C/.git/objects + ../../a/.git/objects + EOF + cat >expect <<-EOF && + alternate: $(pwd)/C/.git/objects + alternate: $(pwd)/B/.git/objects + alternate: $(pwd)/A/.git/objects + EOF + git -C insensitive.git count-objects -v >actual && + grep ^alternate: actual >actual.alternates && + test_cmp expect actual.alternates +' + test_done -- cgit v1.3-5-g9baa