# [PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()` 4 messages from 2018-05-06 to 2018-05-09. Participants: Martin Ågren, David Turner, Jeff King. Thread: https://gitlist.dev/t/48421 ## Martin Ågren, 2018-05-06 14:10 Subject: [PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()` Message-ID: <20180506141031.30204-4-martin.agren@gmail.com> URL: https://gitlist.dev/e/20180506141031.30204-4-martin.agren%40gmail.com ``` After taking the lock we check whether we got it and die otherwise. But since we take the lock using `LOCK_DIE_ON_ERROR`, we would already have died. Unlike in the previous patch, this function is not prepared for indicating errors via a `strbuf err`, so let's just drop the dead code. Any improved error-handling can be added later. While at it, make the lock non-static and reduce its scope. Signed-off-by: Martin Ågren --- refs.c | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/refs.c b/refs.c index 8c50b8b139..7abd30dfe1 100644 --- a/refs.c +++ b/refs.c @@ -689,20 +689,17 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid, static int delete_pseudoref(const char *pseudoref, const struct object_id *old_oid) { - static struct lock_file lock; const char *filename; filename = git_path("%s", pseudoref); if (old_oid && !is_null_oid(old_oid)) { - int fd; + struct lock_file lock = LOCK_INIT; struct object_id actual_old_oid; - fd = hold_lock_file_for_update_timeout( + hold_lock_file_for_update_timeout( &lock, filename, LOCK_DIE_ON_ERROR, get_files_ref_lock_timeout_ms()); - if (fd < 0) - die_errno(_("Could not open '%s' for writing"), filename); if (read_ref(pseudoref, &actual_old_oid)) die("could not read ref '%s'", pseudoref); if (oidcmp(&actual_old_oid, old_oid)) { -- 2.17.0.411.g9fd64c8e46 ``` ## David Turner, 2018-05-06 15:54 Subject: Re: [PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()` Message-ID: <1525622054.16035.12.camel@novalis.org> URL: https://gitlist.dev/e/1525622054.16035.12.camel%40novalis.org In-Reply-To: <20180506141031.30204-4-martin.agren@gmail.com> ``` Same concern here about staticness. On Sun, 2018-05-06 at 16:10 +0200, Martin Ågren wrote: > After taking the lock we check whether we got it and die otherwise. > But > since we take the lock using `LOCK_DIE_ON_ERROR`, we would already > have > died. > > Unlike in the previous patch, this function is not prepared for > indicating errors via a `strbuf err`, so let's just drop the dead > code. > Any improved error-handling can be added later. > > While at it, make the lock non-static and reduce its scope. > > Signed-off-by: Martin Ågren > --- > refs.c | 7 ++----- > 1 file changed, 2 insertions(+), 5 deletions(-) > > diff --git a/refs.c b/refs.c > index 8c50b8b139..7abd30dfe1 100644 > --- a/refs.c > +++ b/refs.c > @@ -689,20 +689,17 @@ static int write_pseudoref(const char > *pseudoref, const struct object_id *oid, > > static int delete_pseudoref(const char *pseudoref, const struct > object_id *old_oid) > { > - static struct lock_file lock; > const char *filename; > > filename = git_path("%s", pseudoref); > > if (old_oid && !is_null_oid(old_oid)) { > - int fd; > + struct lock_file lock = LOCK_INIT; > struct object_id actual_old_oid; > > - fd = hold_lock_file_for_update_timeout( > + hold_lock_file_for_update_timeout( > &lock, filename, LOCK_DIE_ON_ERROR, > get_files_ref_lock_timeout_ms()); > - if (fd < 0) > - die_errno(_("Could not open '%s' for > writing"), filename); > if (read_ref(pseudoref, &actual_old_oid)) > die("could not read ref '%s'", pseudoref); > if (oidcmp(&actual_old_oid, old_oid)) { ``` ## Jeff King, 2018-05-08 18:10 Subject: Re: [PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()` Message-ID: <20180508181034.GB7210@sigill.intra.peff.net> URL: https://gitlist.dev/e/20180508181034.GB7210%40sigill.intra.peff.net In-Reply-To: <20180506141031.30204-4-martin.agren@gmail.com> ``` On Sun, May 06, 2018 at 04:10:29PM +0200, Martin Ågren wrote: > After taking the lock we check whether we got it and die otherwise. But > since we take the lock using `LOCK_DIE_ON_ERROR`, we would already have > died. > > Unlike in the previous patch, this function is not prepared for > indicating errors via a `strbuf err`, so let's just drop the dead code. > Any improved error-handling can be added later. I suspect this ought to eventually be converted to return an error, to match the rest of the refs code. And in that sense, this is a step backwards versus leaving the current die_errno in place and dropping the LOCK_DIE_ON_ERROR flag. But it probably doesn't matter much either way, and whoever does the conversion later can deal with it. -Peff ``` ## Martin Ågren, 2018-05-09 04:23 Subject: Re: [PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()` Message-ID: URL: https://gitlist.dev/e/CAN0heSps9reV82%3DSy0J7g3Ewr2fqiPcCT2XBVPNk2yPi%2B4%2BmrQ%40mail.gmail.com In-Reply-To: <20180508181034.GB7210@sigill.intra.peff.net> ``` On 8 May 2018 at 20:10, Jeff King wrote: > On Sun, May 06, 2018 at 04:10:29PM +0200, Martin Ågren wrote: >> Unlike in the previous patch, this function is not prepared for >> indicating errors via a `strbuf err`, so let's just drop the dead code. >> Any improved error-handling can be added later. > > I suspect this ought to eventually be converted to return an error, to > match the rest of the refs code. And in that sense, this is a step > backwards versus leaving the current die_errno in place and dropping the > LOCK_DIE_ON_ERROR flag. But it probably doesn't matter much either way, > and whoever does the conversion later can deal with it. Thank you for your comments on this series. It's much appreciated. I will be rerolling this series with extended commit messages about the lock-handling. Looking over this code again, I notice that a larger cleanup of the error-handling might be warranted. It would indeed feel better to do a minor part of that, instead of taking a small step in the wrong direction... Martin ```