Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
