On Thu, Sep 26, 2024 at 03:50:02PM +0200, Patrick Steinhardt wrote:
> > diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> > index cfc6951d23..ef4143eef3 100644
> > --- a/builtin/fetch-pack.c
> > +++ b/builtin/fetch-pack.c
> > @@ -294,5 +294,6 @@ int cmd_fetch_pack(int argc,
> > free_refs(fetched_refs);
> > free_refs(remote_refs);
> > list_objects_filter_release(&args.filter_options);
> > + oid_array_clear(&shallow);
> > return ret;
> > }
>
> I wonder about the early exit path we have when `finish_connect()`
> returns non-zero. Should we make it go via the common cleanup path as
> well, or do we not care in the error case?
Yeah, I think it would technically be a leak if we hit that error case.
But it's pretty unlikely in practice (you'd have to finish the fetch and
then the "ssh" or "upload-pack" sub-process returns a non-zero exit code
anyway).
I'd prefer not to deal with it here for two reasons:
1. The same leak is true of all the existing cleanup. So it really is
orthogonal to what this patch is doing, and should be a separate
patch.
2. I think cleaning up in top-level builtins like this is mostly
cosmetic to appease the leak checker. In the real world the memory
is going back to the OS when we return anyway. Even in a libified
world, I don't think cmd_fetch_pack() is a reasonable entry point
(we already have a more lib-ish fetch_pack() that is used by the
transport code).
So I don't think it would be wrong to refactor the cleanup to trigger on
that other early return. But I also suspect there are many such paths
lurking (and will continue to lurk, even after we run clean with
SANITIZE=leak) and I'm not sure how productive it is to hunt them down.
> > diff --git a/builtin/send-pack.c b/builtin/send-pack.c
> > index 81fc96d423..c49fe6c53c 100644
> > --- a/builtin/send-pack.c
> > +++ b/builtin/send-pack.c
> > @@ -343,5 +343,6 @@ int cmd_send_pack(int argc,
> > free_refs(remote_refs);
> > free_refs(local_refs);
> > refspec_clear(&rs);
> > + oid_array_clear(&shallow);
> > return ret;
> > }
>
> We also have an early exit in this function when `match_push_refs()`
> returns non-zero.
And I think this is in the same boat.
-Peff
