On Thu, Sep 26, 2024 at 03:50:14PM +0200, Patrick Steinhardt wrote:
> On Tue, Sep 24, 2024 at 05:56:34PM -0400, Jeff King wrote:
> > diff --git a/transport-helper.c b/transport-helper.c
> > index c688967b8c..9c8abd8eca 100644
> > --- a/transport-helper.c
> > +++ b/transport-helper.c
> > @@ -1023,6 +1023,7 @@ static int push_refs_with_push(struct transport *transport,
> > if (atomic) {
> > reject_atomic_push(remote_refs, mirror);
> > string_list_clear(&cas_options, 0);
> > + strbuf_release(&buf);
> > return 0;
> > } else
> > continue;
>
> What's not visible here is that a few lines further down we have another
> early return where we don't clear `buf`. But that exit is conditioned on
> `buf.len == 0`, and given that we never `strbuf_reset()` the buffer we
> know that `buf.len == 0` only when it hasn't ever be allocated.
Yeah, I noticed that, too, and came to the same conclusion. As you note,
it's _possible_ to have an allocated but zero-length strbuf, but I don't
think it happens here. So it's OK in practice.
If we were going to refactor, I think it would make sense to do it with
a cleanup label to cover both of these early returns, plus the cleanup
at the end. I was mostly just going for minimal changes where possible.
-Peff
