Hi Junio & Stefan, On Wed, 26 Apr 2017, Junio C Hamano wrote: > 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; I did almost what you suggested here, but I avoided the explicit ret = 0, as it is initialized that way. > 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. Oops. Ciao, Dscho