On 29 August 2017 at 10:39, Michael Haggerty <mhagger@xxxxxxxxxxxx> wrote: > On 08/28/2017 10:32 PM, Martin Ågren wrote: >> After the previous patch, none of the functions we call hold on to >> `referent.buf`, so we can safely release the string buffer before >> returning. > > This patch looks good to me, but I did notice a pre-existing problem in > the area... > >> --- >> refs/files-backend.c | 31 ++++++++++++++++++++----------- >> 1 file changed, 20 insertions(+), 11 deletions(-) >> >> diff --git a/refs/files-backend.c b/refs/files-backend.c >> index bdb0e22e5..15f34b10e 100644 >> --- a/refs/files-backend.c >> +++ b/refs/files-backend.c >> [...] >> @@ -2305,10 +2305,12 @@ static int lock_ref_for_update(struct files_ref_store *refs, >> strbuf_addf(err, "cannot lock ref '%s': " >> "error reading reference", >> original_update_refname(update)); >> - return -1; >> + ret = -1; >> + goto out; > > It is incorrect to return -1 here. First of all, stylistically, the > return value should be a symbolic constant. But in fact, it should be > returning `TRANSACTION_GENERIC_ERROR` here, whereas -1 is > `TRANSACTION_NAME_CONFLICT`. So the code is not just stylistically > wrong; it is functionally wrong. > > I know that this is not your mistake, but would you like to add another > patch to your series to fix this up? I'd do it myself, but it's a little > bit awkward because the fix will conflict with your patch. Sure. I'll send out a v3 later today. I'll fix this in a third patch, and I'll also address your comments on the first patch. Thanks a lot. Martin
