{"thread":{"id":"36426","subject":"[PATCH v2 2/2] commit.c: check for lock error and return early","startedAt":"2014-04-16T18:56:51Z","lastAt":"2014-04-17T21:09:04Z","messageCount":6,"participants":["Ronnie Sahlberg","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"238975","messageId":"1397674613-4922-1-git-send-email-sahlberg@google.com","threadId":"36426","inReplyTo":null,"subject":"[PATCH v2 0/2] Check for lock failures early","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-16T18:56:51Z","receivedAt":"2014-04-16T18:56:51Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Callers outside of refs.c use either lock_ref_sha1() or \nlock_any_ref_for_update() to lock a ref during an update.\n\nTwo of these places we do not immediately check the lock for failure\nmaking reading the code harder.\n\nOne place we do some unrelated string manipulation fucntions before we\ncheck for failure and the other place we rely on that write_ref_sha1()\nwill check the lock for failure and return an error.\n\n\nThese two patches updates these two places so that we immediately check the\nlock for failure and act on it.\nIt does not change any functionality or logic but makes the code easier to\nread by being more consistent.\n\nVersion 2:\n* Simplify the return on error case in sequencer.c.\n\n\nRonnie Sahlberg (2):\n  sequencer.c: check for lock failure and bail early in fast_forward_to\n  commit.c: check for lock error and return early\n\n builtin/commit.c | 8 ++++----\n sequencer.c      | 4 ++++\n 2 files changed, 8 insertions(+), 4 deletions(-)\n\n-- \n1.9.1.504.g5a62d94\n"},{"id":"238974","messageId":"1397674613-4922-2-git-send-email-sahlberg@google.com","threadId":"36426","inReplyTo":"1397674613-4922-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 1/2] sequencer.c: check for lock failure and bail early in fast_forward_to","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-16T18:56:52Z","receivedAt":"2014-04-16T18:56:52Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Change fast_forward_to() to check if locking the ref failed, print a nice\nerror message and bail out early.\nThe old code did not check if ref_lock was NULL and relied on the fact\nthat the write_ref_sha1() would safely detect this condition and set the\nreturn variable ret to indicate an error.\nWhile that is safe, it makes the code harder to read for two reasons:\n* Inconsistency.  Almost all other places we do check the lock for NULL\n  explicitely, so the naive reader is confused \"why don't we check here\".\n* And relying on write_ref_sha1() to detect and return an error for when\n  a previous lock_any_ref_for_update() feels obfuscated.\n\nThis change should not change any functionality or logic\naside from adding an extra error message when this condition is triggered.\n(write_ref_sha1() returns an error silently for this condition)\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\n---\n sequencer.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex bde5f04..0a80c58 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -281,8 +281,12 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n \t\texit(1); /* the callee should have complained already */\n \tref_lock = lock_any_ref_for_update(\"HEAD\", unborn ? null_sha1 : from,\n \t\t\t\t\t   0, NULL);\n+\tif (!ref_lock)\n+\t\treturn error(_(\"Failed to lock HEAD during fast_forward_to\"));\n+\n \tstrbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n \tret = write_ref_sha1(ref_lock, to, sb.buf);\n+\n \tstrbuf_release(&sb);\n \treturn ret;\n }\n-- \n1.9.1.504.g5a62d94\n"},{"id":"238973","messageId":"1397674613-4922-3-git-send-email-sahlberg@google.com","threadId":"36426","inReplyTo":"1397674613-4922-1-git-send-email-sahlberg@google.com","subject":"[PATCH v2 2/2] commit.c: check for lock error and return early","fromName":"Ronnie Sahlberg","fromEmail":"sahlberg@google.com","sentAt":"2014-04-16T18:56:53Z","receivedAt":"2014-04-16T18:56:53Z","isPatch":true,"sender":{"key":"sahlberg@google.com","avatar":"https://avatars.githubusercontent.com/u/7320636?v=4"},"body":"Move the check for the lock failure to happen immediately after\nlock_any_ref_for_update().\nPreviously the lock and the check-if-lock-failed was separated by a handful\nof string manipulation statements.\n\nMoving the check to occur immediately after the failed lock makes the\ncode slightly easier to read and makes it follow the pattern of\n try-to-take-a-lock()\n if (check-if-lock-failed){\n    error\n }\n---\n builtin/commit.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex d9550c5..c6320f1 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1672,6 +1672,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t   ? NULL\n \t\t\t\t\t   : current_head->object.sha1,\n \t\t\t\t\t   0, NULL);\n+\tif (!ref_lock) {\n+\t\trollback_index_files();\n+\t\tdie(_(\"cannot lock HEAD ref\"));\n+\t}\n \n \tnl = strchr(sb.buf, '\\n');\n \tif (nl)\n@@ -1681,10 +1685,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n \tstrbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));\n \tstrbuf_insert(&sb, strlen(reflog_msg), \": \", 2);\n \n-\tif (!ref_lock) {\n-\t\trollback_index_files();\n-\t\tdie(_(\"cannot lock HEAD ref\"));\n-\t}\n \tif (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {\n \t\trollback_index_files();\n \t\tdie(_(\"cannot update HEAD ref\"));\n-- \n1.9.1.504.g5a62d94\n"},{"id":"238986","messageId":"534EFE17.7060105@alum.mit.edu","threadId":"36426","inReplyTo":"1397674613-4922-2-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v2 1/2] sequencer.c: check for lock failure and bail early in fast_forward_to","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-16T22:03:03Z","receivedAt":"2014-04-16T22:03:03Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/16/2014 08:56 PM, Ronnie Sahlberg wrote:\n> Change fast_forward_to() to check if locking the ref failed, print a nice\n> error message and bail out early.\n> The old code did not check if ref_lock was NULL and relied on the fact\n> that the write_ref_sha1() would safely detect this condition and set the\n\ns/the write_ref_sha1()/write_ref_sha1()/\n\n> return variable ret to indicate an error.\n> While that is safe, it makes the code harder to read for two reasons:\n> * Inconsistency.  Almost all other places we do check the lock for NULL\n>   explicitely, so the naive reader is confused \"why don't we check here\".\n\ns/explicitely/explicitly/\ns/here\"/here?\"/\n\n> * And relying on write_ref_sha1() to detect and return an error for when\n>   a previous lock_any_ref_for_update() feels obfuscated.\n\ns/feels/failed feels/ maybe?\n\n> \n> This change should not change any functionality or logic\n> aside from adding an extra error message when this condition is triggered.\n> (write_ref_sha1() returns an error silently for this condition)\n\nYou need a period inside the parentheses.\n\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> ---\n>  sequencer.c | 4 ++++\n>  1 file changed, 4 insertions(+)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index bde5f04..0a80c58 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -281,8 +281,12 @@ static int fast_forward_to(const unsigned char *to, const unsigned char *from,\n>  \t\texit(1); /* the callee should have complained already */\n>  \tref_lock = lock_any_ref_for_update(\"HEAD\", unborn ? null_sha1 : from,\n>  \t\t\t\t\t   0, NULL);\n> +\tif (!ref_lock)\n> +\t\treturn error(_(\"Failed to lock HEAD during fast_forward_to\"));\n\nThis error message can be emitted to the user in the normal course of\nthings (i.e., it is not a bug).  So the message should make sense to the\nuser.  Is \"fast_forward_to\" a user-facing term that the user will\nunderstand?  I suspect that you took it from the name of the function,\nwhich is *not* meaningful to a user.\n\nBut unfortunately I'm not familiar enough with the sequencer to be able\nto suggest a better error message.\n\n> +\n>  \tstrbuf_addf(&sb, \"%s: fast-forward\", action_name(opts));\n>  \tret = write_ref_sha1(ref_lock, to, sb.buf);\n> +\n>  \tstrbuf_release(&sb);\n>  \treturn ret;\n>  }\n> \n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"238987","messageId":"534EFEE0.9000806@alum.mit.edu","threadId":"36426","inReplyTo":"1397674613-4922-3-git-send-email-sahlberg@google.com","subject":"Re: [PATCH v2 2/2] commit.c: check for lock error and return early","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-04-16T22:06:24Z","receivedAt":"2014-04-16T22:06:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 04/16/2014 08:56 PM, Ronnie Sahlberg wrote:\n> Move the check for the lock failure to happen immediately after\n> lock_any_ref_for_update().\n> Previously the lock and the check-if-lock-failed was separated by a handful\n> of string manipulation statements.\n\nPlease flow sentences together into paragraphs for easier reading,\nrather than having an extremely ragged right-hand margin.\n\nThe rest looks good.\n\nMichael\n\n>\n> Moving the check to occur immediately after the failed lock makes the\n> code slightly easier to read and makes it follow the pattern of\n>  try-to-take-a-lock()\n>  if (check-if-lock-failed){\n>     error\n>  }\n> ---\n>  builtin/commit.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/builtin/commit.c b/builtin/commit.c\n> index d9550c5..c6320f1 100644\n> --- a/builtin/commit.c\n> +++ b/builtin/commit.c\n> @@ -1672,6 +1672,10 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \t\t\t\t\t   ? NULL\n>  \t\t\t\t\t   : current_head->object.sha1,\n>  \t\t\t\t\t   0, NULL);\n> +\tif (!ref_lock) {\n> +\t\trollback_index_files();\n> +\t\tdie(_(\"cannot lock HEAD ref\"));\n> +\t}\n>  \n>  \tnl = strchr(sb.buf, '\\n');\n>  \tif (nl)\n> @@ -1681,10 +1685,6 @@ int cmd_commit(int argc, const char **argv, const char *prefix)\n>  \tstrbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg));\n>  \tstrbuf_insert(&sb, strlen(reflog_msg), \": \", 2);\n>  \n> -\tif (!ref_lock) {\n> -\t\trollback_index_files();\n> -\t\tdie(_(\"cannot lock HEAD ref\"));\n> -\t}\n>  \tif (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) {\n>  \t\trollback_index_files();\n>  \t\tdie(_(\"cannot update HEAD ref\"));\n> \n\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"239050","messageId":"xmqqppkfd5ov.fsf@gitster.dls.corp.google.com","threadId":"36426","inReplyTo":"534EFEE0.9000806@alum.mit.edu","subject":"Re: [PATCH v2 2/2] commit.c: check for lock error and return early","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-04-17T21:09:04Z","receivedAt":"2014-04-17T21:09:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty <mhagger@alum.mit.edu> writes:\n\n> On 04/16/2014 08:56 PM, Ronnie Sahlberg wrote:\n>> Move the check for the lock failure to happen immediately after\n>> lock_any_ref_for_update().\n>> Previously the lock and the check-if-lock-failed was separated by a handful\n>> of string manipulation statements.\n>\n> Please flow sentences together into paragraphs for easier reading,\n> rather than having an extremely ragged right-hand margin.\n>\n> The rest looks good.\n\nThanks, both.  I tentatively queued with the suggested log message\ntweaks, and I think result reads better.\n"}]}