Re: [PATCH] object-file: use real paths when adding alternates
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-11-17 19:47:42
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Thu, Nov 17 2022, Jeff King wrote:
quoted hunk ↗ jump to hunk
On Thu, Nov 17, 2022 at 05:31:13PM +0000, Glen Choo via GitGitGadget wrote: [...]quoted
@@ -596,7 +603,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt, return; } - strbuf_add_absolute_path(&objdirbuf, r->objects->odb->path); + strbuf_realpath(&objdirbuf, r->objects->odb->path, 1); if (strbuf_normalize_path(&objdirbuf) < 0) die(_("unable to normalize object directory: %s"), objdirbuf.buf);Similarly here, I think we'd want to _replace_ the normalize with a realpath. There's no point in doing both. It's OK to die in this one because we assume the object directory can be normalized/realpath'd. So I'd have expected the code portion of your patch to be more like:diff --git a/object-file.c b/object-file.c index 957790098f..c6a195c6dd 100644 --- a/object-file.c +++ b/object-file.c@@ -508,6 +508,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry, { struct object_directory *ent; struct strbuf pathbuf = STRBUF_INIT; + struct strbuf tmp = STRBUF_INIT; khiter_t pos; if (!is_absolute_path(entry->buf) && relative_base) {@@ -516,12 +517,18 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry, } strbuf_addbuf(&pathbuf, entry); - if (strbuf_normalize_path(&pathbuf) < 0 && relative_base) { - error(_("unable to normalize alternate object path: %s"), - pathbuf.buf); - strbuf_release(&pathbuf); - return -1; + if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) { + if (relative_base) { + error(_("unable to normalize alternate object path: %s"), + pathbuf.buf); + strbuf_release(&pathbuf); + return -1; + } + /* allow broken paths from env per 37a95862c625 */ + strbuf_addstr(&tmp, pathbuf.buf); } + strbuf_swap(&pathbuf, &tmp); + strbuf_release(&tmp); /* * The trailing slash after the directory name is given by@@ -596,10 +603,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt, return; } - strbuf_add_absolute_path(&objdirbuf, r->objects->odb->path); - if (strbuf_normalize_path(&objdirbuf) < 0) - die(_("unable to normalize object directory: %s"), - objdirbuf.buf); + strbuf_realpath(&objdirbuf, r->objects->odb->path, 1); while (*alt) { alt = parse_alt_odb_entry(alt, sep, &entry);The "tmp" swapping in link_alt_odb_entry is kind of unfortunate. It would be nice if there were an in-place version of strbuf_realpath, even if it was using two buffers under the hood (which is how the normalize code does it). And then the patch really would be s/normalize/realpath/, which is easier to understand. Possibly this should also be using the "forgiving" version. We eventually error out on missing entries later on, so it's not a big deal to error here. But it would let us keep the error message the same. I don't know that it matters much in practice.
This probably isn't worth it, but I wondered if this wouldn't be easier if we pulled that memory management into the caller, it's not performance sensitive (or maybe, how many alternatives do people have :)), but an advantage of this is that we avoid the free()/malloc() if we only get partway through, i.e. return early and keep looping. In terms of general code smell & how we manage the "return" here, as adding "RESULT_MUST_BE_USED" to this shows we never use the "0" or "-1" (or any other...) return value. That's been the case since this was added in c2f493a4ae1 (Transitively read alternatives, 2006-05-07), so we can probably just make this a "void" and ditch the returns if we're finding ourselves juggling these return values... object-file.c | 44 ++++++++++++++++++++++---------------------- 1 file changed, 22 insertions(+), 22 deletions(-)
diff --git a/object-file.c b/object-file.c
index c6a195c6dd2..1a94d98e0c7 100644
--- a/object-file.c
+++ b/object-file.c@@ -504,47 +504,43 @@ static void read_info_alternates(struct repository *r, const char *relative_base, int depth); static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry, - const char *relative_base, int depth, const char *normalized_objdir) + const char *relative_base, int depth, + const char *normalized_objdir, + struct strbuf *pathbuf) { struct object_directory *ent; - struct strbuf pathbuf = STRBUF_INIT; struct strbuf tmp = STRBUF_INIT; khiter_t pos; if (!is_absolute_path(entry->buf) && relative_base) { - strbuf_realpath(&pathbuf, relative_base, 1); - strbuf_addch(&pathbuf, '/'); + strbuf_realpath(pathbuf, relative_base, 1); + strbuf_addch(pathbuf, '/'); } - strbuf_addbuf(&pathbuf, entry); + strbuf_addbuf(pathbuf, entry); - if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) { - if (relative_base) { - error(_("unable to normalize alternate object path: %s"), - pathbuf.buf); - strbuf_release(&pathbuf); - return -1; - } + if (!strbuf_realpath(&tmp, pathbuf->buf, 0)) { + if (relative_base) + return error(_("unable to normalize alternate object path: %s"), + pathbuf->buf); /* allow broken paths from env per 37a95862c625 */ - strbuf_addstr(&tmp, pathbuf.buf); + strbuf_addstr(&tmp, pathbuf->buf); } - strbuf_swap(&pathbuf, &tmp); + strbuf_swap(pathbuf, &tmp); strbuf_release(&tmp); /* * The trailing slash after the directory name is given by * this function at the end. Remove duplicates. */ - while (pathbuf.len && pathbuf.buf[pathbuf.len - 1] == '/') - strbuf_setlen(&pathbuf, pathbuf.len - 1); + while (pathbuf->len && pathbuf->buf[pathbuf->len - 1] == '/') + strbuf_setlen(pathbuf, pathbuf->len - 1); - if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) { - strbuf_release(&pathbuf); + if (!alt_odb_usable(r->objects, pathbuf, normalized_objdir, &pos)) return -1; - } CALLOC_ARRAY(ent, 1); - /* pathbuf.buf is already in r->objects->odb_by_path */ - ent->path = strbuf_detach(&pathbuf, NULL); + /* pathbuf->buf is already in r->objects->odb_by_path */ + ent->path = strbuf_detach(pathbuf, NULL); /* add the alternate entry */ *r->objects->odb_tail = ent;
@@ -593,6 +589,7 @@ static void link_alt_odb_entries(struct repository *r, const char *alt, { struct strbuf objdirbuf = STRBUF_INIT; struct strbuf entry = STRBUF_INIT; + struct strbuf pathbuf = STRBUF_INIT; if (!alt || !*alt) return;
@@ -610,8 +607,11 @@ static void link_alt_odb_entries(struct repository *r, const char *alt, if (!entry.len) continue; link_alt_odb_entry(r, &entry, - relative_base, depth, objdirbuf.buf); + relative_base, depth, objdirbuf.buf, + &pathbuf); + strbuf_reset(&pathbuf); } + strbuf_release(&pathbuf); strbuf_release(&entry); strbuf_release(&objdirbuf); }