Re: [PATCH] packfile: free packed_git memory when closing object store
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 26, 2019, 18:06 UTC
- Message-ID
- <xmqqftlnhkd6.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20190826024508.8444-1-mh@glandium.org>
Mike Hommey <mh@glandium.org> writes:
Show 6 quoted lines
> Signed-off-by: Mike Hommey <mh@glandium.org> > --- > packfile.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > Note, I'm not sure this is the right place to do it.
I do not think this patch is complete, given that o->packed_git still has a non-NULL pointer. IIRC, close_pack() was written for the explicit purpose of releasing resources while allowing us to reopen with open_packed_git() on it, so with the current arrangement, after releasing the resources held for this object store and doing something else, you should be able to come back to this object store and work in it again---this patch makes it harder if not impossible to do so.
I _think_ the patch is OK if you assigned NULL to o->packed_git, after making sure that the intention of all the callers of close_object_store() is to declare that this object store will not be accessed any longer during the lifetime of the process, and write it down as the contract between the callers and this function in a comment perhaps in packfile.h where the function is declared.
Thanks.
Show 25 quoted lines
> diff --git a/packfile.c b/packfile.c
> index fc43a6c52c..b0cb84adda 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -339,13 +339,16 @@ void close_pack(struct packed_git *p)
>
> void close_object_store(struct raw_object_store *o)
> {
> - struct packed_git *p;
> + struct packed_git *p = o->packed_git;
>
> - for (p = o->packed_git; p; p = p->next)
> + while (p) {
> + struct packed_git *current = p;
> if (p->do_not_close)
> BUG("want to close pack marked 'do-not-close'");
> - else
> - close_pack(p);
> + close_pack(p);
> + p = p->next;
> + free(current);
> + }
>
> if (o->multi_pack_index) {
> close_midx(o->multi_pack_index);