Stefan Beller <sbeller@xxxxxxxxxx> writes:
>> - if (get_oid_hex(x, commit_id) < 0)
>> - return -1;
>> + if (!ret && get_oid_hex(x, commit_id) < 0)
>> + ret = -1;
>>
>
> In similar cases of fixing mem leaks, we'd put a label here
> and make excessive use of goto, instead of setting ret to -1.
> As "ret" and the commands are short, this is visually just as appealing.
I wouldn't call that visually appealing. Fixing resource leaks is
good, and only because this function is short enough and the funny
way to skip over various operation need to last for just a few
instructions, it is tolerable. If the function were shorter, then
we may have used
if (!strbuf_getline_lf() &&
skip_prefix() &&
!get_oid_hex())
ret = 0; /* happy */
else
ret = -1;
release resources here;
return ret;
and that would have been OK. If longer, as you said, jumping to a
label at the end of function to release the acquired resource would
be a lot more maintainable way than either of the above alternatives
that are only usable for short functions.
The patch as posted makes the function fail to return -1 when it
finds problems, by the way. It needs to return "ret" not the
hardcoded "0" at the end.