{"thread":{"id":"48421","subject":"[PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()`","startedAt":"2018-05-06T14:11:16Z","lastAt":"2018-05-09T04:23:22Z","messageCount":4,"participants":["Martin Ågren","David Turner","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"346765","messageId":"20180506141031.30204-4-martin.agren@gmail.com","threadId":"48421","inReplyTo":null,"subject":"[PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-06T14:10:29Z","receivedAt":"2018-05-06T14:11:16Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After taking the lock we check whether we got it and die otherwise. But\nsince we take the lock using `LOCK_DIE_ON_ERROR`, we would already have\ndied.\n\nUnlike in the previous patch, this function is not prepared for\nindicating errors via a `strbuf err`, so let's just drop the dead code.\nAny improved error-handling can be added later.\n\nWhile at it, make the lock non-static and reduce its scope.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n refs.c | 7 ++-----\n 1 file changed, 2 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8c50b8b139..7abd30dfe1 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -689,20 +689,17 @@ static int write_pseudoref(const char *pseudoref, const struct object_id *oid,\n \n static int delete_pseudoref(const char *pseudoref, const struct object_id *old_oid)\n {\n-\tstatic struct lock_file lock;\n \tconst char *filename;\n \n \tfilename = git_path(\"%s\", pseudoref);\n \n \tif (old_oid && !is_null_oid(old_oid)) {\n-\t\tint fd;\n+\t\tstruct lock_file lock = LOCK_INIT;\n \t\tstruct object_id actual_old_oid;\n \n-\t\tfd = hold_lock_file_for_update_timeout(\n+\t\thold_lock_file_for_update_timeout(\n \t\t\t\t&lock, filename, LOCK_DIE_ON_ERROR,\n \t\t\t\tget_files_ref_lock_timeout_ms());\n-\t\tif (fd < 0)\n-\t\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n \t\tif (read_ref(pseudoref, &actual_old_oid))\n \t\t\tdie(\"could not read ref '%s'\", pseudoref);\n \t\tif (oidcmp(&actual_old_oid, old_oid)) {\n-- \n2.17.0.411.g9fd64c8e46\n\n"},{"id":"346772","messageId":"1525622054.16035.12.camel@novalis.org","threadId":"48421","inReplyTo":"20180506141031.30204-4-martin.agren@gmail.com","subject":"Re: [PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()`","fromName":"David Turner","fromEmail":"novalis@novalis.org","sentAt":"2018-05-06T15:54:14Z","receivedAt":"2018-05-06T15:54:18Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"Same concern here about staticness.\n\nOn Sun, 2018-05-06 at 16:10 +0200, Martin Ågren wrote:\n> After taking the lock we check whether we got it and die otherwise.\n> But\n> since we take the lock using `LOCK_DIE_ON_ERROR`, we would already\n> have\n> died.\n> \n> Unlike in the previous patch, this function is not prepared for\n> indicating errors via a `strbuf err`, so let's just drop the dead\n> code.\n> Any improved error-handling can be added later.\n> \n> While at it, make the lock non-static and reduce its scope.\n> \n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> ---\n>  refs.c | 7 ++-----\n>  1 file changed, 2 insertions(+), 5 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 8c50b8b139..7abd30dfe1 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -689,20 +689,17 @@ static int write_pseudoref(const char\n> *pseudoref, const struct object_id *oid,\n>  \n>  static int delete_pseudoref(const char *pseudoref, const struct\n> object_id *old_oid)\n>  {\n> -\tstatic struct lock_file lock;\n>  \tconst char *filename;\n>  \n>  \tfilename = git_path(\"%s\", pseudoref);\n>  \n>  \tif (old_oid && !is_null_oid(old_oid)) {\n> -\t\tint fd;\n> +\t\tstruct lock_file lock = LOCK_INIT;\n>  \t\tstruct object_id actual_old_oid;\n>  \n> -\t\tfd = hold_lock_file_for_update_timeout(\n> +\t\thold_lock_file_for_update_timeout(\n>  \t\t\t\t&lock, filename, LOCK_DIE_ON_ERROR,\n>  \t\t\t\tget_files_ref_lock_timeout_ms());\n> -\t\tif (fd < 0)\n> -\t\t\tdie_errno(_(\"Could not open '%s' for\n> writing\"), filename);\n>  \t\tif (read_ref(pseudoref, &actual_old_oid))\n>  \t\t\tdie(\"could not read ref '%s'\", pseudoref);\n>  \t\tif (oidcmp(&actual_old_oid, old_oid)) {\n"},{"id":"347001","messageId":"20180508181034.GB7210@sigill.intra.peff.net","threadId":"48421","inReplyTo":"20180506141031.30204-4-martin.agren@gmail.com","subject":"Re: [PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-05-08T18:10:34Z","receivedAt":"2018-05-08T18:10:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 06, 2018 at 04:10:29PM +0200, Martin Ågren wrote:\n\n> After taking the lock we check whether we got it and die otherwise. But\n> since we take the lock using `LOCK_DIE_ON_ERROR`, we would already have\n> died.\n> \n> Unlike in the previous patch, this function is not prepared for\n> indicating errors via a `strbuf err`, so let's just drop the dead code.\n> Any improved error-handling can be added later.\n\nI suspect this ought to eventually be converted to return an error, to\nmatch the rest of the refs code. And in that sense, this is a step\nbackwards versus leaving the current die_errno in place and dropping the\nLOCK_DIE_ON_ERROR flag. But it probably doesn't matter much either way,\nand whoever does the conversion later can deal with it.\n\n-Peff\n"},{"id":"347050","messageId":"CAN0heSps9reV82=Sy0J7g3Ewr2fqiPcCT2XBVPNk2yPi+4+mrQ@mail.gmail.com","threadId":"48421","inReplyTo":"20180508181034.GB7210@sigill.intra.peff.net","subject":"Re: [PATCH 3/5] refs.c: drop dead code checking lock status in `delete_pseudoref()`","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2018-05-09T04:23:18Z","receivedAt":"2018-05-09T04:23:22Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 8 May 2018 at 20:10, Jeff King <peff@peff.net> wrote:\n> On Sun, May 06, 2018 at 04:10:29PM +0200, Martin Ågren wrote:\n>> Unlike in the previous patch, this function is not prepared for\n>> indicating errors via a `strbuf err`, so let's just drop the dead code.\n>> Any improved error-handling can be added later.\n>\n> I suspect this ought to eventually be converted to return an error, to\n> match the rest of the refs code. And in that sense, this is a step\n> backwards versus leaving the current die_errno in place and dropping the\n> LOCK_DIE_ON_ERROR flag. But it probably doesn't matter much either way,\n> and whoever does the conversion later can deal with it.\n\nThank you for your comments on this series. It's much appreciated. I\nwill be rerolling this series with extended commit messages about the\nlock-handling. Looking over this code again, I notice that a larger\ncleanup of the error-handling might be warranted. It would indeed feel\nbetter to do a minor part of that, instead of taking a small step in the\nwrong direction...\n\nMartin\n"}]}