Re: [PATCH] bundle-uri: plug leak in unbundle_from_file()

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

> On Mon, Aug 26, 2024 at 10:30:52AM +0200, Toon Claes wrote:
>> When the function returns early, the variable bundle_ref is not released
>> through strbuf_release().
>> 
>> Fix this leak. And while at it, remove assignments in the conditions of
>> the "if" statements as suggested in the CodingGuidelines.
> ...
>> -	if ((result = unbundle(r, &header, bundle_fd, NULL,
>> -			       VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0))))
>> -		return 1;
>> +	result = unbundle(r, &header, bundle_fd, NULL,
>> +			  VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));
>> +	if (result)
>> +		goto cleanup;
>
> This changes the returned error code from `1` to whatever `unbundle()`
> returns. Is this intentional? If so, the commit message should explain
> why this change is safe.

Thanks for reviewing carefully.

Both of two callers of unbundle_from_file() are used as the
condition of an if() statement, so unbundle() that signals an error
with -1 wouldn't be a problem, I would think.

It may not be a bad idea as a #leftoverbits item, after the dust
settles, to clean up the calling convention in this file (may not be
limited to the code path that reaches this function) to follow the
usual "signal success with 0, failures are signalled with a negative
value".  Then we can just return the value we got from a failing
read_bundle_header(), just the same way we return the value we got
from a failing unbundle().

> Other than that this looks good to me, and the fix does not conflict
> with any of my leak-plugging series.

Yup.  Thanks, both.