Re: [PATCH v2 3/3] Revert "pack-objects: lazily set up "struct rev_info", don't leak"
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Nov 28, 2022, 11:12 UTC
- Message-ID
- <221128.868rjvmi3l.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <xmqqv8mz5ras.fsf@gitster.g>
On Mon, Nov 28 2022, Junio C Hamano wrote:
Show 17 quoted lines
> René Scharfe <l.s.r@web.de> writes: > >> This reverts commit 5cb28270a1ff94a0a23e67b479bbbec3bc993518. >> >> 5cb28270a1 (pack-objects: lazily set up "struct rev_info", don't leak, >> 2022-03-28) avoided leaking rev_info allocations in many cases by >> calling repo_init_revisions() only when the .filter member was actually >> needed, but then still leaking it. That was fixed later by 2108fe4a19 >> (revisions API users: add straightforward release_revisions(), >> 2022-04-13), making the reverted commit unnecessary. > > Hmph, with this merged, 'seen' breaks linux-leaks job in a strange > way. > > https://github.com/git/git/actions/runs/3563546608/jobs/5986458300#step:5:3917 > > Does anybody want to help looking into it?
It's because the tip of that branch introduces a new leak, but it's only revealed in our test suite by a "git" invocation that we don't check the exit code of. So testing with "GIT_TEST_SANITIZE_LEAK_LOG=true" (which is there to check exactly those edge cases) catches it.
René, testing locally with e.g.:
GIT_TEST_PASSING_SANITIZE_LEAK=check \ GIT_TEST_SANITIZE_LEAK_LOG=true \ make test
Should catch it, but note that we might still have new leaks in tests that are already failing due to other leaks. So diff-ing the resulting t/test-results/*.leak/* would be a good sanity check to see if there's any other leak we've missed.
As to the bug itself: This exact edge case is why I went for the lazy-setup part of 5cb28270a1f (pack-objects: lazily set up "struct rev_info", don't leak, 2022-03-28), and indeed the example mentioned in its commit message now leaks again, but not on master:
echo e83c5163316f89bfbde7d9ab23ca2e25604af290 | ./git pack-objects initial
Now, maybe I'm missing something, but it the tip of that branch points out a legitimate bug in my 5cb28270a1f.
But I don't see why the fix for it needs to be a full revert of
5cb28270a1f. If I replace all the C code changes in that commit with
this the test it flipped also passes:
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index 573d0b20b76..0de53c5a2df 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -4158,8 +4158,10 @@ static struct list_objects_filter_options *po_filter_revs_init(void *value)
{
struct po_filter_data *data = value;
- repo_init_revisions(the_repository, &data->revs, NULL);
- data->have_revs = 1;
+ if (!data->have_revs) {
+ repo_init_revisions(the_repository, &data->revs, NULL);
+ data->have_revs = 1;
+ }
return &data->revs.filter;
}I.e. the commit is correct that we shouldn't be clobbering the "rev_info" when we see --filter again, but then let's just stop doing that.
Well, I know why, it's because René initially started poking at this to rid the codebase of the reliance of C99's J.5.7. I think our two opinions on that are quite clear, so I won't repeat that here (I don't per-se mind, I just think it's a nothingburger).
But in any case, if we're going to also refactor this code to get rd of the J.5.7 reliance I'd think we could agree that it really belongs in its own change. That change could then be a pure refactoring change, and wouldn't need to also explain how it's fixing a bug while-at-it.