Subject: Re: [PATCH 00/24] revision.[ch]: add and use release_revisions()

From: Ævar Arnfjörð Bjarmason <[email-masked]>
Date: Wed, 09 Mar 2022 20:39:14 +0100

On Wed, Mar 09 2022, Derrick Stolee wrote:

> On 3/9/2022 9:34 AM, Derrick Stolee wrote:
>> On 3/9/2022 8:16 AM, Ævar Arnfjörð Bjarmason wrote:
>>> == For Junio ==
>>>
>>> This has a trivially resolved conflict with Derrick Stolee's
>>> aaf251cdc5c (revision: put object filter into struct rev_info,
>>> 2022-03-08) currently in "seen" in builtin/rev-list.c.
>>>
>>> The resolution is to just keep the "goto cleanup" in place of "return
>>> 0" in the conflicting lines, but to otherwise keep Derrick's version.
>>>
>>> It will pass with/without SANITIZE=leak when applied to both "master"
>>> and "seen". I omitted one test change (described in a relevant commit
>>> message) due to the latter not being true (no fault of "seen", just a
>>> new leaking command being added to a test).
>> 
>> Since ds/partial-bundles will soon be updated in v4 to change the
>> pointer added to struct rev_info, it is even more likely that there
>> will be more important things to do with regards to clearing the
>> memory of rev_infos based on that change. It might be better to wait
>> for that update (coming soon) and then rebase directly on top.
>
> I took a look at the series as it stands now and have a few nits
> here and there. Generally, things are pretty standard in this kind
> of series you've been working diligently on for a while.

Thanks for the review. From a quick skim I didn't see anything I needed
to address other than with an eventual re-roll, so I'll incorporate it
into a v2, pending some further feedback.

> The only thing I can recommend is to check that your leak-check
> statements are still true when reaching the end of the series, now
> that the filter member exists. Likely the tests that you are marking
> as leak-free do nothing with object filters, so they will still be
> true. Just something to keep in mind and maybe add a patch that
> recursively frees the contents of 'revs->filter' at the end.

*nod*. For what it's worth I tested this with 'git rebase -x' and a
local patch I have that allows you to run the tests in a mode where it
asserts that there's a 1=1 mapping between tests marked for the
linux-leaks job, and those that currently pass. I.e. a passing test that
isn't opted-in via TEST_PASSES_SANITIZE_LEAK=true will fail if it passes
with SANITIZE=leak.

I'll run the v2 with that again, and will lean on the side of just
ejecting any changes that step on the toes of other in-flight topics.

I.e. as noted in the cover letter this isn't all of the revisions.[ch]
leaks, just most of them, so I can always eject a few and leave those
for a follow-up series (which I already have patches for, but those are
all a bit more complex, so I left them out for now).