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.
> Signed-off-by: Elijah Newren <newren@xxxxxxxxx>
> ---
> merge-recursive.c | 8 +++++---
> 1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index 12300131fc..1163508811 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -934,9 +934,11 @@ static int update_file_flags(struct merge_options *opt,
> }
>
> buf = read_object_file(&contents->oid, &type, &size);
> - if (!buf)
> - return err(opt, _("cannot read object %s '%s'"),
> - oid_to_hex(&contents->oid), path);
> + if (!buf) {
> + ret = err(opt, _("cannot read object %s '%s'"),
> + oid_to_hex(&contents->oid), path);
> + goto free_buf;
> + }
> if (type != OBJ_BLOB) {
> ret = err(opt, _("blob expected for %s '%s'"),
> oid_to_hex(&contents->oid), path);
