On 7/26/2019 2:31 PM, Junio C Hamano wrote: > Elijah Newren <newren@xxxxxxxxx> writes: > >> Returning before freeing the allocated buffer is suboptimal; as with >> elsewhere in the same function, make sure buf gets free'd. > > I do not have a real objection to the patch text, but the above > justification does not make much sense to me. The original code > returned an error when buf is NULL, so there is no leak returning > directly, without jumping to free_buf: label (whose only effect is > to free(buf) and return the value in ret). > > The real value of this change is it may future-proof the codepath by > making sure that everybody after an error goes to the same place to > free all resources---which happens to be only buf in the current > code, but this change allows it to change in the future, where some > additional resources may have been allocated before the call to > read_object_file() and freed after free_buf: label. It should be noted that any future change to make the "free_buf:" label mean "free everything" then it should be renamed to "cleanup:" or similar. Not that we should do that now, as that would muddy the blame. Thanks, -Stolee
