| Message ID | pull.1382.v2.git.git.1669074557348.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] object-file: use real paths when adding alternates | expand |
On Mon, Nov 21 2022, Glen Choo via GitGitGadget wrote: > From: Glen Choo <chooglen@google.com> Aside from some small nits this looks good to me. > object-file.c | 12 ++++++------ > t/t7700-repack.sh | 18 ++++++++++++++++++ > 2 files changed, 24 insertions(+), 6 deletions(-) > > diff --git a/object-file.c b/object-file.c > index 957790098fa..ef2b762234d 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,14 @@ 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) { > + if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) { > error(_("unable to normalize alternate object path: %s"), > - pathbuf.buf); > + pathbuf.buf); This is a mis-indentation, it was OK in the pre-image, not now. > strbuf_release(&pathbuf); Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the &tmp you pass in here) and then "does a "goto error_out". It then *resets* the strbuf, but doesn't release it, assuming that you're going to pass it in again. So in that case we'd leak here, no? I.e. a NULL return value from strbuf_realpath() doesn't mean that it didn't allocate in the scratch area passed to it, so we need to strbuf_release(&tmp) here too. Perhaps this on top is simpler (but also see below)?: diff --git a/object-file.c b/object-file.c index ef2b762234d..d5d502504bb 100644 --- a/object-file.c +++ b/object-file.c @@ -510,6 +510,7 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry, struct strbuf pathbuf = STRBUF_INIT; struct strbuf tmp = STRBUF_INIT; khiter_t pos; + int ret = -1; if (!is_absolute_path(entry->buf) && relative_base) { strbuf_realpath(&pathbuf, relative_base, 1); @@ -520,11 +521,9 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry, if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) { error(_("unable to normalize alternate object path: %s"), pathbuf.buf); - strbuf_release(&pathbuf); - return -1; + goto error; } strbuf_swap(&pathbuf, &tmp); - strbuf_release(&tmp); /* * The trailing slash after the directory name is given by @@ -533,10 +532,8 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry, 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); - return -1; - } + if (!alt_odb_usable(r->objects, &pathbuf, normalized_objdir, &pos)) + goto error; CALLOC_ARRAY(ent, 1); /* pathbuf.buf is already in r->objects->odb_by_path */ @@ -552,7 +549,11 @@ static int link_alt_odb_entry(struct repository *r, const struct strbuf *entry, /* recursively add alternates */ read_info_alternates(r, ent->path, depth + 1); - return 0; + ret = 0; +error: + strbuf_release(&tmp); + strbuf_release(&pathbuf); + return ret; } static const char *parse_alt_odb_entry(const char *string, > return -1; > } > + strbuf_swap(&pathbuf, &tmp); > + strbuf_release(&tmp); > > /* > * The trailing slash after the directory name is given by > @@ -596,10 +599,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); > 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 && The tests pass without this re-"git init", left over from development? > + 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 > +' > + I think this is better squashed in: diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh index ce1954d0977..79eef5b4aa7 100755 --- a/t/t7700-repack.sh +++ b/t/t7700-repack.sh @@ -91,13 +91,11 @@ test_expect_success 'loose objects in alternate ODB are not repacked' ' ' test_expect_success '--local keeps packs when alternate is objectdir ' ' - git init alt_symlink && + test_when_finished "rm -rf repo" && + git init repo && + test_commit -C repo A && ( - cd alt_symlink && - git init && - echo content >file4 && - git add file4 && - git commit -m commit_file4 && + cd repo && git repack -a && ls .git/objects/pack/*.pack >../expect && ln -s objects .git/alt_objects && Because: * If it's not a setup for a later test let's call it "repo" and clean it up at the end. * The "file4" you're creating doesn't go with the existing pattern, the file{1..3} are created in the top-level .git, here you're making a file4 in another repo. This just calls it "A.t", and makes it with test_commit, since all you need is a dummy commit. * I think we typically use "find .. -type f", not "ls", see e.g. t5351-unpack-large-objects.sh, but I left it in-place. I think aside from that test there's some other "let's compare the packed before & after" in the test suite, but I can't remember offhand...
On Mon, Nov 21, 2022 at 11:49:17PM +0000, Glen Choo via GitGitGadget wrote: > Thanks for the feedback on v1. This version takes nearly all of Peff's > patch [1] except for the comment about making an exception for relative > paths in the environment. My reading of the commit [2] is that it was a > workaround for strbuf_normalize_path() not being able to handle relative > paths, so the only reason to special-case the environment is to preserve > the behavior of respecting broken paths, which (unlike relative paths) I > don't think will be missed. Yeah, that makes sense. If realpath fails because a path isn't present, then we would throw it away anyway. So we don't need to quietly tolerate, unless we really care about the difference between reporting "this directory doesn't seem to exist" versus "I couldn't run realpath on this directory". One is a subset of the other. > diff --git a/object-file.c b/object-file.c > index 957790098fa..ef2b762234d 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,14 @@ 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) { > + if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) { > error(_("unable to normalize alternate object path: %s"), > - pathbuf.buf); > + pathbuf.buf); > strbuf_release(&pathbuf); > return -1; > } > + strbuf_swap(&pathbuf, &tmp); > + strbuf_release(&tmp); So here we are looking at an alternates entry (either from a file or from the environment). We do note all errors, even in relative ones from the environment, but we don't die, so we'll just ignore the failed alternate. Good. > @@ -596,10 +599,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); And here we are resolving the actual object directory, and we always died if that couldn't be normalized. And we'll continue to do so by realpath. Good. > diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh And then that's all we needed in the C code, since we already do duplicate checks. Good. :) > 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 > +' This probably needs to be protected with a SYMLINKS prereq. -Peff
On Tue, Nov 22, 2022 at 01:56:09AM +0100, Ævar Arnfjörð Bjarmason wrote: > > @@ -516,12 +517,14 @@ 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) { > > + if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) { > > error(_("unable to normalize alternate object path: %s"), > > - pathbuf.buf); > > + pathbuf.buf); > > This is a mis-indentation, it was OK in the pre-image, not now. > > > strbuf_release(&pathbuf); > > Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the > "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the > &tmp you pass in here) and then "does a "goto error_out". > > It then *resets* the strbuf, but doesn't release it, assuming that > you're going to pass it in again. So in that case we'd leak here, no? > > I.e. a NULL return value from strbuf_realpath() doesn't mean that it > didn't allocate in the scratch area passed to it, so we need to > strbuf_release(&tmp) here too. We don't use MANY_MISSING in this code path, but I didn't read strbuf_realpath_1() carefully enough to see if that is the only case. But regardless, I think it is a bug in strbuf_realpath(). All of the strbuf functions generally try to leave a buffer untouched on error. So IMHO we would want a preparatory patch with s/reset/release/ in that function, which better matches the intent (we might be freeing an allocated buffer, but that's OK from the caller perspective). In theory it ought to just roll back the length for whatever it put into the buffer, but it looks like the rest of the function is happy to clobber what's in the buf, even on non-error. That's why we have strbuf_add_real_path(), but of course it doesn't allow for setting the die_on_error flag. -Peff
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> object-file.c | 12 ++++++------ >> t/t7700-repack.sh | 18 ++++++++++++++++++ >> 2 files changed, 24 insertions(+), 6 deletions(-) >> >> diff --git a/object-file.c b/object-file.c >> index 957790098fa..ef2b762234d 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,14 @@ 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) { >> + if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) { >> error(_("unable to normalize alternate object path: %s"), >> - pathbuf.buf); >> + pathbuf.buf); > > This is a mis-indentation, it was OK in the pre-image, not now. Strange, this came from "make style", and in the GitHub web UI, it shows the next line as aligned with the opening ". Meh, I'll undo it. > Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the > "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the > &tmp you pass in here) and then "does a "goto error_out". > > It then *resets* the strbuf, but doesn't release it, assuming that > you're going to pass it in again. So in that case we'd leak here, no? > > I.e. a NULL return value from strbuf_realpath() doesn't mean that it > didn't allocate in the scratch area passed to it, so we need to > strbuf_release(&tmp) here too. Yeah, you're right. At any rate, it's a lot of cognitive overload to check if strbuf_realpath() will or won't allocate, so free()-ing in the caller makes sense. Separately, Peff mentioned that strbuf_realpath() not free()-ing is a real bug, but I'll leave that for a future cleanup. >> + 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 >> +' >> + > > I think this is better squashed in: > > diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh > index ce1954d0977..79eef5b4aa7 100755 > --- a/t/t7700-repack.sh > +++ b/t/t7700-repack.sh > @@ -91,13 +91,11 @@ test_expect_success 'loose objects in alternate ODB are not repacked' ' > ' > > test_expect_success '--local keeps packs when alternate is objectdir ' ' > - git init alt_symlink && > + test_when_finished "rm -rf repo" && > + git init repo && > + test_commit -C repo A && > ( > - cd alt_symlink && > - git init && > - echo content >file4 && > - git add file4 && > - git commit -m commit_file4 && > + cd repo && > git repack -a && > ls .git/objects/pack/*.pack >../expect && > ln -s objects .git/alt_objects && > > Because: > > * If it's not a setup for a later test let's call it "repo" and clean > it up at the end. > > * The "file4" you're creating doesn't go with the existing pattern, the > file{1..3} are created in the top-level .git, here you're making a > file4 in another repo. > > This just calls it "A.t", and makes it with test_commit, since all > you need is a dummy commit. > > * I think we typically use "find .. -type f", not "ls", see > e.g. t5351-unpack-large-objects.sh, but I left it in-place. I think > aside from that test there's some other "let's compare the packed > before & after" in the test suite, but I can't remember offhand... It seems like t7700-repack.sh itself isn't consistent either (which is probably how I ended up with "ls"). I'll also leave it alone unless someone has strong opinions.
Jeff King <peff@peff.net> writes: >> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the >> "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the >> &tmp you pass in here) and then "does a "goto error_out". >> >> It then *resets* the strbuf, but doesn't release it, assuming that >> you're going to pass it in again. So in that case we'd leak here, no? >> >> I.e. a NULL return value from strbuf_realpath() doesn't mean that it >> didn't allocate in the scratch area passed to it, so we need to >> strbuf_release(&tmp) here too. > > We don't use MANY_MISSING in this code path, but I didn't read > strbuf_realpath_1() carefully enough to see if that is the only case. > But regardless, I think it is a bug in strbuf_realpath(). All of the > strbuf functions generally try to leave a buffer untouched on error. > > So IMHO we would want a preparatory patch with s/reset/release/ in that > function, which better matches the intent (we might be freeing an > allocated buffer, but that's OK from the caller perspective). Is that always OK? I would think that we'd do something closer to strbuf_getcwd(): int strbuf_getcwd(struct strbuf *sb) { size_t oldalloc = sb->alloc; /* ... */ if (oldalloc == 0) strbuf_release(sb); else strbuf_reset(sb); } i.e. if the caller passed in a strbuf with allocated contents, they're responsible for free()-ing it, otherwise we free() it. That does fix the leak in this patch, but I don't feel strongly enough about changing strbuf_realpath() to do it now, so I'll do without the change for now.
On Wed, Nov 23, 2022 at 04:50:02PM -0800, Glen Choo wrote: > Jeff King <peff@peff.net> writes: > > >> Doesn't this leak? I've just skimmed strbuf_realpath_1() but e.g. in the > >> "REALPATH_MANY_MISSING" case it'll have allocated the "resolved" (the > >> &tmp you pass in here) and then "does a "goto error_out". > >> > >> It then *resets* the strbuf, but doesn't release it, assuming that > >> you're going to pass it in again. So in that case we'd leak here, no? > >> > >> I.e. a NULL return value from strbuf_realpath() doesn't mean that it > >> didn't allocate in the scratch area passed to it, so we need to > >> strbuf_release(&tmp) here too. > > > > We don't use MANY_MISSING in this code path, but I didn't read > > strbuf_realpath_1() carefully enough to see if that is the only case. > > But regardless, I think it is a bug in strbuf_realpath(). All of the > > strbuf functions generally try to leave a buffer untouched on error. > > > > So IMHO we would want a preparatory patch with s/reset/release/ in that > > function, which better matches the intent (we might be freeing an > > allocated buffer, but that's OK from the caller perspective). > > Is that always OK? I would think that we'd do something closer to > strbuf_getcwd(): > > int strbuf_getcwd(struct strbuf *sb) > { > size_t oldalloc = sb->alloc; > /* ... */ > if (oldalloc == 0) > strbuf_release(sb); > else > strbuf_reset(sb); > } > > i.e. if the caller passed in a strbuf with allocated contents, they're > responsible for free()-ing it, otherwise we free() it. That does fix the > leak in this patch, but I don't feel strongly enough about changing > strbuf_realpath() to do it now, so I'll do without the change for now. That's what I was getting at with "that's OK from the caller perspective". strbuf_realpath() is also unlike other strbuf functions in that it clobbers the contents of the buffer, even on success (rather than adding on success and rolling back to the original state on error). Since the caller is OK with the buffer being clobbered anyway, it should not matter to it whether we clobbered an allocated buffer back to an unallocated one on error. The confusing thing (and the current behavior) is when we do the opposite: change an unallocated one to an allocated one. -Peff
diff --git a/object-file.c b/object-file.c index 957790098fa..ef2b762234d 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,14 @@ 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) { + if (!strbuf_realpath(&tmp, pathbuf.buf, 0)) { error(_("unable to normalize alternate object path: %s"), - pathbuf.buf); + pathbuf.buf); strbuf_release(&pathbuf); return -1; } + strbuf_swap(&pathbuf, &tmp); + strbuf_release(&tmp); /* * The trailing slash after the directory name is given by @@ -596,10 +599,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); 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 &&