{"thread":{"id":"39414","subject":"[PATCH 2/5] verify_lock(): on errors, let the caller unlock the lock","startedAt":"2015-05-22T23:34:52Z","lastAt":"2015-05-27T21:18:05Z","messageCount":12,"participants":["Michael Haggerty","Stefan Beller","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"261959","messageId":"1432337697-29161-1-git-send-email-mhagger@alum.mit.edu","threadId":"39414","inReplyTo":null,"subject":"[PATCH 0/5] Fix verify_lock() to report errors via strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-22T23:34:52Z","receivedAt":"2015-05-22T23:34:52Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"verify_lock() is a helper function called while committing reference\ntransactions. But when it fails, instead of recording its error\nmessage in a strbuf to be passed back to the caller of\nref_transaction_commit(), the error message was being written directly\nto stderr.\n\nInstead, report the errors via a strbuf so that they make it back to\nthe caller of ref_transaction_commit().\n\nThe last two patches remove the capitalization of some error messages,\nto be consistent with our usual practice. These are a slight backwards\nincompatibility; if any scripts are trying to grep for these error\nmessage strings, they might have to be adjusted. So feel free to drop\nthem if you think consistency across time is more important than\nconsistency across commands.\n\nThis is the patch series that I mentioned here [1]. It applies on top\nof mh/ref-directory-file [2] and is thematically a continuation of\nthat series in the sense that it further cleans up the error handling\nwithin reference transactions. It would be easy to rebase to master if\nthat is preferred.\n\nThese patches are also available on my GitHub account [3] as branch\n\"verify-lock-strbuf-err\".\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/269006\n[2] http://thread.gmane.org/gmane.comp.version-control.git/268778\n[3] https://github.com/mhagger/git\n\nMichael Haggerty (5):\n  verify_lock(): return 0/-1 rather than struct ref_lock *\n  verify_lock(): on errors, let the caller unlock the lock\n  verify_lock(): report errors via a strbuf\n  verify_lock(): do not capitalize error messages\n  ref_transaction_commit(): do not capitalize error messages\n\n refs.c                | 40 ++++++++++++++++++++++++++--------------\n t/t1400-update-ref.sh | 14 +++++++-------\n 2 files changed, 33 insertions(+), 21 deletions(-)\n\n-- \n2.1.4\n"},{"id":"261961","messageId":"1432337697-29161-2-git-send-email-mhagger@alum.mit.edu","threadId":"39414","inReplyTo":"1432337697-29161-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/5] verify_lock(): return 0/-1 rather than struct ref_lock *","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-22T23:34:53Z","receivedAt":"2015-05-22T23:34:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Its return value wasn't conveying any extra information, but it made\nthe reader wonder whether the ref_lock that it returned might be\ndifferent than the one that was passed to it. So change the function\nto the traditional \"return 0 on success or a negative value on error\".\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 21 ++++++++++++++-------\n 1 file changed, 14 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 97043fd..4432bc9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2195,9 +2195,14 @@ static void unlock_ref(struct ref_lock *lock)\n \tfree(lock);\n }\n \n-/* This function should make sure errno is meaningful on error */\n-static struct ref_lock *verify_lock(struct ref_lock *lock,\n-\tconst unsigned char *old_sha1, int mustexist)\n+/*\n+ * Verify that the reference locked by lock has the value old_sha1.\n+ * Fail if the reference doesn't exist and mustexist is set. Return 0\n+ * on success or a negative value on error. This function should make\n+ * sure errno is meaningful on error.\n+ */\n+static int verify_lock(struct ref_lock *lock,\n+\t\t       const unsigned char *old_sha1, int mustexist)\n {\n \tif (read_ref_full(lock->ref_name,\n \t\t\t  mustexist ? RESOLVE_REF_READING : 0,\n@@ -2206,16 +2211,16 @@ static struct ref_lock *verify_lock(struct ref_lock *lock,\n \t\terror(\"Can't verify ref %s\", lock->ref_name);\n \t\tunlock_ref(lock);\n \t\terrno = save_errno;\n-\t\treturn NULL;\n+\t\treturn -1;\n \t}\n \tif (hashcmp(lock->old_sha1, old_sha1)) {\n \t\terror(\"Ref %s is at %s but expected %s\", lock->ref_name,\n \t\t\tsha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));\n \t\tunlock_ref(lock);\n \t\terrno = EBUSY;\n-\t\treturn NULL;\n+\t\treturn -1;\n \t}\n-\treturn lock;\n+\treturn 0;\n }\n \n static int remove_empty_directories(const char *file)\n@@ -2445,7 +2450,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\tgoto error_return;\n \t\t}\n \t}\n-\treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n+\tif (old_sha1 && verify_lock(lock, old_sha1, mustexist))\n+\t\treturn NULL;\n+\treturn lock;\n \n  error_return:\n \tunlock_ref(lock);\n-- \n2.1.4\n"},{"id":"261958","messageId":"1432337697-29161-3-git-send-email-mhagger@alum.mit.edu","threadId":"39414","inReplyTo":"1432337697-29161-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/5] verify_lock(): on errors, let the caller unlock the lock","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-22T23:34:54Z","receivedAt":"2015-05-22T23:34:54Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"The caller already knows how to do it, so always do it in the same\nplace.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4432bc9..100a767 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2209,14 +2209,12 @@ static int verify_lock(struct ref_lock *lock,\n \t\t\t  lock->old_sha1, NULL)) {\n \t\tint save_errno = errno;\n \t\terror(\"Can't verify ref %s\", lock->ref_name);\n-\t\tunlock_ref(lock);\n \t\terrno = save_errno;\n \t\treturn -1;\n \t}\n \tif (hashcmp(lock->old_sha1, old_sha1)) {\n \t\terror(\"Ref %s is at %s but expected %s\", lock->ref_name,\n \t\t\tsha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));\n-\t\tunlock_ref(lock);\n \t\terrno = EBUSY;\n \t\treturn -1;\n \t}\n@@ -2450,8 +2448,10 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\tgoto error_return;\n \t\t}\n \t}\n-\tif (old_sha1 && verify_lock(lock, old_sha1, mustexist))\n-\t\treturn NULL;\n+\tif (old_sha1 && verify_lock(lock, old_sha1, mustexist)) {\n+\t\tlast_errno = errno;\n+\t\tgoto error_return;\n+\t}\n \treturn lock;\n \n  error_return:\n-- \n2.1.4\n"},{"id":"261960","messageId":"1432337697-29161-4-git-send-email-mhagger@alum.mit.edu","threadId":"39414","inReplyTo":"1432337697-29161-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/5] verify_lock(): report errors via a strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-22T23:34:55Z","receivedAt":"2015-05-22T23:34:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of writing error messages directly to stderr, write them to a\n\"strbuf *err\". In lock_ref_sha1_basic(), arrange for these errors to\nbe returned to its caller.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 19 ++++++++++++-------\n 1 file changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 100a767..625e69f 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2198,23 +2198,28 @@ static void unlock_ref(struct ref_lock *lock)\n /*\n  * Verify that the reference locked by lock has the value old_sha1.\n  * Fail if the reference doesn't exist and mustexist is set. Return 0\n- * on success or a negative value on error. This function should make\n- * sure errno is meaningful on error.\n+ * on success. On error, write an error message to err, set errno, and\n+ * return a negative value.\n  */\n static int verify_lock(struct ref_lock *lock,\n-\t\t       const unsigned char *old_sha1, int mustexist)\n+\t\t       const unsigned char *old_sha1, int mustexist,\n+\t\t       struct strbuf *err)\n {\n+\tassert(err);\n+\n \tif (read_ref_full(lock->ref_name,\n \t\t\t  mustexist ? RESOLVE_REF_READING : 0,\n \t\t\t  lock->old_sha1, NULL)) {\n \t\tint save_errno = errno;\n-\t\terror(\"Can't verify ref %s\", lock->ref_name);\n+\t\tstrbuf_addf(err, \"Can't verify ref %s\", lock->ref_name);\n \t\terrno = save_errno;\n \t\treturn -1;\n \t}\n \tif (hashcmp(lock->old_sha1, old_sha1)) {\n-\t\terror(\"Ref %s is at %s but expected %s\", lock->ref_name,\n-\t\t\tsha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));\n+\t\tstrbuf_addf(err, \"Ref %s is at %s but expected %s\",\n+\t\t\t    lock->ref_name,\n+\t\t\t    sha1_to_hex(lock->old_sha1),\n+\t\t\t    sha1_to_hex(old_sha1));\n \t\terrno = EBUSY;\n \t\treturn -1;\n \t}\n@@ -2448,7 +2453,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\tgoto error_return;\n \t\t}\n \t}\n-\tif (old_sha1 && verify_lock(lock, old_sha1, mustexist)) {\n+\tif (old_sha1 && verify_lock(lock, old_sha1, mustexist, err)) {\n \t\tlast_errno = errno;\n \t\tgoto error_return;\n \t}\n-- \n2.1.4\n"},{"id":"261962","messageId":"1432337697-29161-5-git-send-email-mhagger@alum.mit.edu","threadId":"39414","inReplyTo":"1432337697-29161-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/5] verify_lock(): do not capitalize error messages","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-22T23:34:56Z","receivedAt":"2015-05-22T23:34:56Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Our convention is for error messages to start with a lower-case\nletter.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 625e69f..48aff79 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2211,12 +2211,12 @@ static int verify_lock(struct ref_lock *lock,\n \t\t\t  mustexist ? RESOLVE_REF_READING : 0,\n \t\t\t  lock->old_sha1, NULL)) {\n \t\tint save_errno = errno;\n-\t\tstrbuf_addf(err, \"Can't verify ref %s\", lock->ref_name);\n+\t\tstrbuf_addf(err, \"can't verify ref %s\", lock->ref_name);\n \t\terrno = save_errno;\n \t\treturn -1;\n \t}\n \tif (hashcmp(lock->old_sha1, old_sha1)) {\n-\t\tstrbuf_addf(err, \"Ref %s is at %s but expected %s\",\n+\t\tstrbuf_addf(err, \"ref %s is at %s but expected %s\",\n \t\t\t    lock->ref_name,\n \t\t\t    sha1_to_hex(lock->old_sha1),\n \t\t\t    sha1_to_hex(old_sha1));\n-- \n2.1.4\n"},{"id":"261963","messageId":"1432337697-29161-6-git-send-email-mhagger@alum.mit.edu","threadId":"39414","inReplyTo":"1432337697-29161-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 5/5] ref_transaction_commit(): do not capitalize error messages","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-22T23:34:57Z","receivedAt":"2015-05-22T23:34:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Our convention is for error messages to start with a lower-case\nletter.\n\nSigned-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n---\n refs.c                |  4 ++--\n t/t1400-update-ref.sh | 14 +++++++-------\n 2 files changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 48aff79..1d60fcd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3856,7 +3856,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t? TRANSACTION_NAME_CONFLICT\n \t\t\t\t: TRANSACTION_GENERIC_ERROR;\n \t\t\treason = strbuf_detach(err, NULL);\n-\t\t\tstrbuf_addf(err, \"Cannot lock ref '%s': %s\",\n+\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n \t\t\t\t    update->refname, reason);\n \t\t\tfree(reason);\n \t\t\tgoto cleanup;\n@@ -3883,7 +3883,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t} else if (write_ref_sha1(update->lock, update->new_sha1,\n \t\t\t\t\t\t  update->msg)) {\n \t\t\t\tupdate->lock = NULL; /* freed by write_ref_sha1 */\n-\t\t\t\tstrbuf_addf(err, \"Cannot update the ref '%s'.\",\n+\t\t\t\tstrbuf_addf(err, \"cannot update the ref '%s'.\",\n \t\t\t\t\t    update->refname);\n \t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\tgoto cleanup;\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 86fa468..9c133c1 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -519,7 +519,7 @@ test_expect_success 'stdin create ref works with path with space to blob' '\n test_expect_success 'stdin update ref fails with wrong old value' '\n \techo \"update $c $m $m~1\" >stdin &&\n \ttest_must_fail git update-ref --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n@@ -555,7 +555,7 @@ test_expect_success 'stdin update ref works with right old value' '\n test_expect_success 'stdin delete ref fails with wrong old value' '\n \techo \"delete $a $m~1\" >stdin &&\n \ttest_must_fail git update-ref --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock ref '\"'\"'$a'\"'\"'\" err &&\n+\tgrep \"fatal: cannot lock ref '\"'\"'$a'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual\n@@ -688,7 +688,7 @@ test_expect_success 'stdin update refs fails with wrong old value' '\n \tupdate $c  ''\n \tEOF\n \ttest_must_fail git update-ref --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual &&\n@@ -883,7 +883,7 @@ test_expect_success 'stdin -z create ref works with path with space to blob' '\n test_expect_success 'stdin -z update ref fails with wrong old value' '\n \tprintf $F \"update $c\" \"$m\" \"$m~1\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n \ttest_must_fail git rev-parse --verify -q $c\n '\n \n@@ -899,7 +899,7 @@ test_expect_success 'stdin -z create ref fails when ref exists' '\n \tgit rev-parse \"$c\" >expect &&\n \tprintf $F \"create $c\" \"$m~1\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n \tgit rev-parse \"$c\" >actual &&\n \ttest_cmp expect actual\n '\n@@ -930,7 +930,7 @@ test_expect_success 'stdin -z update ref works with right old value' '\n test_expect_success 'stdin -z delete ref fails with wrong old value' '\n \tprintf $F \"delete $a\" \"$m~1\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock ref '\"'\"'$a'\"'\"'\" err &&\n+\tgrep \"fatal: cannot lock ref '\"'\"'$a'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual\n@@ -1045,7 +1045,7 @@ test_expect_success 'stdin -z update refs fails with wrong old value' '\n \tgit update-ref $c $m &&\n \tprintf $F \"update $a\" \"$m\" \"$m\" \"update $b\" \"$m\" \"$m\" \"update $c\" \"$m\" \"$Z\" >stdin &&\n \ttest_must_fail git update-ref -z --stdin <stdin 2>err &&\n-\tgrep \"fatal: Cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n+\tgrep \"fatal: cannot lock ref '\"'\"'$c'\"'\"'\" err &&\n \tgit rev-parse $m >expect &&\n \tgit rev-parse $a >actual &&\n \ttest_cmp expect actual &&\n-- \n2.1.4\n"},{"id":"261970","messageId":"CAGZ79kZnpruF+DfO9WO2cT1796j_0XUU8E3qFAg1qoufdBz4Pg@mail.gmail.com","threadId":"39414","inReplyTo":"1432337697-29161-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 1/5] verify_lock(): return 0/-1 rather than struct ref_lock *","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-23T00:09:45Z","receivedAt":"2015-05-23T00:09:45Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 22, 2015 at 4:34 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> Its return value wasn't conveying any extra information, but it made\n> the reader wonder whether the ref_lock that it returned might be\n> different than the one that was passed to it. So change the function\n> to the traditional \"return 0 on success or a negative value on error\".\n>\n> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>\n\nBonus points for the documentation!\nReviewed-by: Stefan Beller <sbeller@google.com>\n\n> ---\n>  refs.c | 21 ++++++++++++++-------\n>  1 file changed, 14 insertions(+), 7 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 97043fd..4432bc9 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2195,9 +2195,14 @@ static void unlock_ref(struct ref_lock *lock)\n>         free(lock);\n>  }\n>\n> -/* This function should make sure errno is meaningful on error */\n> -static struct ref_lock *verify_lock(struct ref_lock *lock,\n> -       const unsigned char *old_sha1, int mustexist)\n> +/*\n> + * Verify that the reference locked by lock has the value old_sha1.\n> + * Fail if the reference doesn't exist and mustexist is set. Return 0\n> + * on success or a negative value on error. This function should make\n> + * sure errno is meaningful on error.\n> + */\n> +static int verify_lock(struct ref_lock *lock,\n> +                      const unsigned char *old_sha1, int mustexist)\n>  {\n>         if (read_ref_full(lock->ref_name,\n>                           mustexist ? RESOLVE_REF_READING : 0,\n> @@ -2206,16 +2211,16 @@ static struct ref_lock *verify_lock(struct ref_lock *lock,\n>                 error(\"Can't verify ref %s\", lock->ref_name);\n>                 unlock_ref(lock);\n>                 errno = save_errno;\n> -               return NULL;\n> +               return -1;\n>         }\n>         if (hashcmp(lock->old_sha1, old_sha1)) {\n>                 error(\"Ref %s is at %s but expected %s\", lock->ref_name,\n>                         sha1_to_hex(lock->old_sha1), sha1_to_hex(old_sha1));\n>                 unlock_ref(lock);\n>                 errno = EBUSY;\n> -               return NULL;\n> +               return -1;\n>         }\n> -       return lock;\n> +       return 0;\n>  }\n>\n>  static int remove_empty_directories(const char *file)\n> @@ -2445,7 +2450,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>                         goto error_return;\n>                 }\n>         }\n> -       return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n> +       if (old_sha1 && verify_lock(lock, old_sha1, mustexist))\n> +               return NULL;\n> +       return lock;\n>\n>   error_return:\n>         unlock_ref(lock);\n> --\n> 2.1.4\n>\n"},{"id":"261971","messageId":"CAGZ79kYa7hcb1xejp+aS3cELh4RBbm9+Um_r_4UvEu9C1yyPhw@mail.gmail.com","threadId":"39414","inReplyTo":"1432337697-29161-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/5] Fix verify_lock() to report errors via strbuf","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-05-23T00:15:21Z","receivedAt":"2015-05-23T00:15:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, May 22, 2015 at 4:34 PM, Michael Haggerty <mhagger@alum.mit.edu> wrote:\n> verify_lock() is a helper function called while committing reference\n> transactions. But when it fails, instead of recording its error\n> message in a strbuf to be passed back to the caller of\n> ref_transaction_commit(), the error message was being written directly\n> to stderr.\n>\n> Instead, report the errors via a strbuf so that they make it back to\n> the caller of ref_transaction_commit().\n>\n> The last two patches remove the capitalization of some error messages,\n> to be consistent with our usual practice. These are a slight backwards\n> incompatibility; if any scripts are trying to grep for these error\n> message strings, they might have to be adjusted. So feel free to drop\n> them if you think consistency across time is more important than\n> consistency across commands.\n>\n> This is the patch series that I mentioned here [1]. It applies on top\n> of mh/ref-directory-file [2] and is thematically a continuation of\n> that series in the sense that it further cleans up the error handling\n> within reference transactions. It would be easy to rebase to master if\n> that is preferred.\n\nI was confused at first as I only looked at the patches and the corresponding\nline numbers did not match with the files as currently open in my editor.\n(they are roughly origin/master)\n\nNow that I read the cover letter I can explain the line number being slightly\ndifferent. :)\n\nThe series looks good to me.\n\n>\n> These patches are also available on my GitHub account [3] as branch\n> \"verify-lock-strbuf-err\".\n>\n> [1] http://article.gmane.org/gmane.comp.version-control.git/269006\n> [2] http://thread.gmane.org/gmane.comp.version-control.git/268778\n> [3] https://github.com/mhagger/git\n>\n> Michael Haggerty (5):\n>   verify_lock(): return 0/-1 rather than struct ref_lock *\n>   verify_lock(): on errors, let the caller unlock the lock\n>   verify_lock(): report errors via a strbuf\n>   verify_lock(): do not capitalize error messages\n>   ref_transaction_commit(): do not capitalize error messages\n>\n>  refs.c                | 40 ++++++++++++++++++++++++++--------------\n>  t/t1400-update-ref.sh | 14 +++++++-------\n>  2 files changed, 33 insertions(+), 21 deletions(-)\n>\n> --\n> 2.1.4\n>\n"},{"id":"262224","messageId":"5565B1AE.3010708@alum.mit.edu","threadId":"39414","inReplyTo":"1432337697-29161-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/5] Fix verify_lock() to report errors via strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-27T11:59:42Z","receivedAt":"2015-05-27T11:59:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/23/2015 01:34 AM, Michael Haggerty wrote:\n> verify_lock() is a helper function called while committing reference\n> transactions. But when it fails, instead of recording its error\n> message in a strbuf to be passed back to the caller of\n> ref_transaction_commit(), the error message was being written directly\n> to stderr.\n> \n> Instead, report the errors via a strbuf so that they make it back to\n> the caller of ref_transaction_commit().\n> \n> [...]\n> \n> This is the patch series that I mentioned here [1]. It applies on top\n> of mh/ref-directory-file [2] and is thematically a continuation of\n> that series in the sense that it further cleans up the error handling\n> within reference transactions. It would be easy to rebase to master if\n> that is preferred.\n\nThe last sentence is nonsense. This patch series relies on\nlock_ref_sha1_basic() having a \"strbuf *err\" parameter, which is only\nthe case since\n\n    4a32b2e lock_ref_sha1_basic(): report errors via a \"struct strbuf\n*err\" (2015-05-11)\n\nThe latter commit is in mh/ref-directory-file (which has now been merged\nto master, so technically the last sentence is now correct again).\n\nSorry for the confusion.\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"262255","messageId":"xmqqk2vtls2q.fsf@gitster.dls.corp.google.com","threadId":"39414","inReplyTo":"1432337697-29161-4-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 3/5] verify_lock(): report errors via a strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-27T19:48:45Z","receivedAt":"2015-05-27T19:48:45Z","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> Instead of writing error messages directly to stderr, write them to a\n> \"strbuf *err\". In lock_ref_sha1_basic(), arrange for these errors to\n> be returned to its caller.\n\nI had to scratch my head and view long outside the context before\nrealizing that the caller lock_ref_sha1_basic() already arranges\nwith its caller that errors from it are passed via the strbuf, and\nthis change is just turning verify_lock(), a callee from it, follows\nthat already established pattern.\n\nLooks sensible, but the last sentence was misleading at least to me.\n\n\tThe caller, lock_ref_sha1_basic(), uses this error reporting\n\tconvention with all the other callees, and reports its error\n        this way to its callers.\n\nperhaps?\n"},{"id":"262256","messageId":"xmqqfv6hlrs1.fsf@gitster.dls.corp.google.com","threadId":"39414","inReplyTo":"5565B1AE.3010708@alum.mit.edu","subject":"Re: [PATCH 0/5] Fix verify_lock() to report errors via strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-05-27T19:55:10Z","receivedAt":"2015-05-27T19:55:10Z","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> The last sentence is nonsense. This patch series relies on\n> lock_ref_sha1_basic() having a \"strbuf *err\" parameter, which is only\n> the case since\n>\n>     4a32b2e lock_ref_sha1_basic(): report errors via a \"struct strbuf\n> *err\" (2015-05-11)\n>\n> The latter commit is in mh/ref-directory-file (which has now been merged\n> to master, so technically the last sentence is now correct again).\n\n[5/5] seems to conflict with the write_ref_sha1() vs write_ref_to_lockfile()\nupdates; I think I can manage, though ;-)\n"},{"id":"262280","messageId":"5566348D.7040506@alum.mit.edu","threadId":"39414","inReplyTo":"xmqqk2vtls2q.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/5] verify_lock(): report errors via a strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-05-27T21:18:05Z","receivedAt":"2015-05-27T21:18:05Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 05/27/2015 09:48 PM, Junio C Hamano wrote:\n> Michael Haggerty <mhagger@alum.mit.edu> writes:\n> \n>> Instead of writing error messages directly to stderr, write them to a\n>> \"strbuf *err\". In lock_ref_sha1_basic(), arrange for these errors to\n>> be returned to its caller.\n> \n> I had to scratch my head and view long outside the context before\n> realizing that the caller lock_ref_sha1_basic() already arranges\n> with its caller that errors from it are passed via the strbuf, and\n> this change is just turning verify_lock(), a callee from it, follows\n> that already established pattern.\n> \n> Looks sensible, but the last sentence was misleading at least to me.\n> \n> \tThe caller, lock_ref_sha1_basic(), uses this error reporting\n> \tconvention with all the other callees, and reports its error\n>         this way to its callers.\n> \n> perhaps?\n\n+1\n\nThanks for clarifying this sentence.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}