| Message ID | pull.1382.git.git.1668706274099.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | object-file: use real paths when adding alternates | expand |
On Thu, Nov 17, 2022 at 05:31:13PM +0000, Glen Choo via GitGitGadget wrote: > When adding an alternate ODB, we check if the alternate has the same > path as the object dir, and if so, we do nothing. However, that > comparison does not resolve symlinks. This makes it possible to add the > object dir as an alternate, which may result in bad behavior. For > example, it can trick "git repack -a -l -d" (possibly run by "git gc") > into thinking that all packs come from an alternate and delete all > objects. I think we do attempt to normalize the names. In link_alt_odb_entries(), we call strbuf_normalize_path() on the base object directory, and then in link_alt_odb_entry(), we similarly normalize the proposed odb names. And then we drop duplicates (either of other alternates, or of the main object directory). So it seems like the problem is that "normalize" is not enough here, because it only normalizes the text. We need to be using strbuf_realpath() which will actually evaluate symbolic links. > diff --git a/object-file.c b/object-file.c > index 957790098fa..f901dd272d1 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -455,14 +455,16 @@ static int alt_odb_usable(struct raw_object_store *o, > struct strbuf *path, > const char *normalized_objdir, khiter_t *pos) > { > + int ret = 0; > int r; > + struct strbuf real_path = STRBUF_INIT; > > /* 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; > + goto cleanup; > } This goto seems unnecessary; we know we haven't touched real_path yet, so there is nothing to clean. But... > @@ -478,11 +480,16 @@ static int alt_odb_usable(struct raw_object_store *o, > assert(r == 1); /* never used */ > kh_value(o->odb_by_path, p) = o->odb; > } > - if (fspatheq(path->buf, normalized_objdir)) > - return 0; > + > + strbuf_realpath(&real_path, path->buf, 1); > + if (fspatheq(real_path.buf, normalized_objdir)) > + goto cleanup; > *pos = kh_put_odb_path_map(o->odb_by_path, path->buf, &r); > /* r: 0 = exists, 1 = never used, 2 = deleted */ > - return r == 0 ? 0 : 1; > + ret = r == 0 ? 0 : 1; > + cleanup: > + strbuf_release(&real_path); > + return ret; > } This seems like the wrong place to be doing realpath. We've already normalized earlier in link_alt_odb_entry(). Why not use realpath there? It does mean we'd end up with the realpath in the object_directory struct, but I don't see that as a bad thing (in fact, it may be slightly faster if it saves the kernel crossing a symlink boundary). I do suspect it would change the error message for missing intermediate directories (since realpath would complain, before we even hit the "does this even exist"), but it's an error in both cases. Which brings up another point here: I think right now we treat those errors as warnings, and cnotinue on without the alternate available (because we ignore the result of link_alt_odb_entry). But with your patch, a failure in this realpath would cause us to die immediately. > @@ -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. -Peff
On Thu, Nov 17 2022, Jeff King wrote: > On Thu, Nov 17, 2022 at 05:31:13PM +0000, Glen Choo via GitGitGadget wrote: > [...] >> @@ -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); }
On Thu, Nov 17, 2022 at 05:31:13PM +0000, Glen Choo via GitGitGadget wrote: > From: Glen Choo <chooglen@google.com> > > When adding an alternate ODB, we check if the alternate has the same > path as the object dir, and if so, we do nothing. However, that > comparison does not resolve symlinks. This makes it possible to add the > object dir as an alternate, which may result in bad behavior. For > example, it can trick "git repack -a -l -d" (possibly run by "git gc") > into thinking that all packs come from an alternate and delete all > objects. Nice find and fix. Looks like Peff had a couple of good suggestions which I'd like to see incorporated into another version before we pick this up. Otherwise, looking at the patch myself, it seems obviously good. Thanks for working on it. Thanks, Taylor
On Thu, Nov 17, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote: > 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. I agree with your "probably". This isn't worth it to save a malloc in a very non-hot code path. If the two bits of code were otherwise equal (say, reset-ing a buffer used directly in a loop) I might say "why not?". But crossing a function boundary to me introduces way too many questions in somebody reading the code (like "is pathbuf supposed to have something in it?") to make it worth doing here. But even if we did want to do it, see below. > 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... Yeah, we could ditch the return values. In a sense they are at least documenting how link_alt_odb_entry() sees the world, but if nobody looks at them, I'd be OK dropping them to make it clear that we don't intend to ever act on them. That said, both of these are orthogonal to what Glen's patches are doing. If you want to submit a series later to deal with them, OK, but let's try not to hijack the conversation for patches that are fixing a real bug. -Peff
On Thu, Nov 17, 2022 at 04:57:26PM -0500, Jeff King wrote: > That said, both of these are orthogonal to what Glen's patches are > doing. If you want to submit a series later to deal with them, OK, but > let's try not to hijack the conversation for patches that are fixing a > real bug. Thanks for keeping us on track. Thanks, Taylor
Jeff King <peff@peff.net> writes: > On Thu, Nov 17, 2022 at 08:41:44PM +0100, Ævar Arnfjörð Bjarmason wrote: > >> This probably isn't worth it, but I wondered if this wouldn't be easier >> if we pulled that memory management into the caller [...] > [......] But crossing a function boundary to me introduces way too many > questions in somebody reading the code (like "is pathbuf supposed to > have something in it?") to make it worth doing here. Thanks, both :) I think the loss of readability is enough for me to hold off on this suggestion, but I don't mind reviewing cleanup patches on top of this.
diff --git a/object-file.c b/object-file.c index 957790098fa..f901dd272d1 100644 --- a/object-file.c +++ b/object-file.c @@ -455,14 +455,16 @@ static int alt_odb_usable(struct raw_object_store *o, struct strbuf *path, const char *normalized_objdir, khiter_t *pos) { + int ret = 0; int r; + struct strbuf real_path = STRBUF_INIT; /* 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; + goto cleanup; } /* @@ -478,11 +480,16 @@ static int alt_odb_usable(struct raw_object_store *o, assert(r == 1); /* never used */ kh_value(o->odb_by_path, p) = o->odb; } - if (fspatheq(path->buf, normalized_objdir)) - return 0; + + strbuf_realpath(&real_path, path->buf, 1); + if (fspatheq(real_path.buf, normalized_objdir)) + goto cleanup; *pos = kh_put_odb_path_map(o->odb_by_path, path->buf, &r); /* r: 0 = exists, 1 = never used, 2 = deleted */ - return r == 0 ? 0 : 1; + ret = r == 0 ? 0 : 1; + cleanup: + strbuf_release(&real_path); + return ret; } /* @@ -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); diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index 5be483bf887..ce1954d0977 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -90,6 +90,24 @@ test_expect_success 'loose objects in alternate ODB are not repacked' ' test_has_duplicate_object false ' +test_expect_success '--local keeps packs when alternate is objectdir ' ' + git init alt_symlink && + ( + cd alt_symlink && + git init && + echo content >file4 && + git add file4 && + git commit -m commit_file4 && + git repack -a && + ls .git/objects/pack/*.pack >../expect && + ln -s objects .git/alt_objects && + echo "$(pwd)/.git/alt_objects" >.git/objects/info/alternates && + git repack -a -d -l && + ls .git/objects/pack/*.pack >../actual + ) && + test_cmp expect actual +' + test_expect_success 'packed obs in alt ODB are repacked even when local repo is packless' ' mkdir alt_objects/pack && mv .git/objects/pack/* alt_objects/pack &&