{"thread":{"id":"38005","subject":"[PATCH] refs.c: handle locking failure during transaction better","startedAt":"2014-11-18T23:17:19Z","lastAt":"2014-11-21T09:25:07Z","messageCount":24,"participants":["Stefan Beller","Jeff King","Jonathan Nieder","Junio C Hamano","Michael Haggerty"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252144","messageId":"1416352639-10150-1-git-send-email-sbeller@google.com","threadId":"38005","inReplyTo":null,"subject":"[PATCH] refs.c: handle locking failure during transaction better","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-18T23:17:19Z","receivedAt":"2014-11-18T23:17:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Change lock_ref_sha1_basic to return an error instead of dying,\nwhen we fail to lock a file during a transaction. This function is\nonly called from transaction_commit() and it knows how to handle\nthese failures.\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nCherry-picked-to-master: Stefan Beller <sbeller@google.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n refs.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n \nThis was part of the ref-transactions-rename series[1], \nbut it makes sense to send this one as a separate patch.\n\nThanks,\nStefan\n\n[1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html\n\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..0347328 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2318,6 +2318,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n \tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n \tif (lock->lock_fd < 0) {\n+\t\tlast_errno = errno;\n \t\tif (errno == ENOENT && --attempts_remaining > 0)\n \t\t\t/*\n \t\t\t * Maybe somebody just deleted one of the\n@@ -2325,8 +2326,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t * again:\n \t\t\t */\n \t\t\tgoto retry;\n-\t\telse\n-\t\t\tunable_to_lock_die(ref_file, errno);\n+\t\telse {\n+\t\t\tstruct strbuf err = STRBUF_INIT;\n+\t\t\tunable_to_lock_message(ref_file, errno, &err);\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tstrbuf_reset(&err);\n+\t\t\tgoto error_return;\n+\t\t}\n \t}\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n-- \n2.2.0.rc2.5.gf7b9fb2\n"},{"id":"252147","messageId":"CAGZ79kbH=pqSizhUeuiCeYXZWk015K49Pj0F5pPvkOTqg4PMuQ@mail.gmail.com","threadId":"38005","inReplyTo":"1416352639-10150-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: handle locking failure during transaction better","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-18T23:34:29Z","receivedAt":"2014-11-18T23:34:29Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I messed up authorship on this one.\nThis was of course found and authored by Ronnie.\n\nOn Tue, Nov 18, 2014 at 3:17 PM, Stefan Beller <sbeller@google.com> wrote:\n> Change lock_ref_sha1_basic to return an error instead of dying,\n> when we fail to lock a file during a transaction. This function is\n> only called from transaction_commit() and it knows how to handle\n> these failures.\n>\n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> Cherry-picked-to-master: Stefan Beller <sbeller@google.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>  refs.c | 10 ++++++++--\n>  1 file changed, 8 insertions(+), 2 deletions(-)\n>\n> This was part of the ref-transactions-rename series[1],\n> but it makes sense to send this one as a separate patch.\n>\n> Thanks,\n> Stefan\n>\n> [1] https://www.mail-archive.com/git@vger.kernel.org/msg60604.html\n>\n> diff --git a/refs.c b/refs.c\n> index 5ff457e..0347328 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2318,6 +2318,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>\n>         lock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n>         if (lock->lock_fd < 0) {\n> +               last_errno = errno;\n>                 if (errno == ENOENT && --attempts_remaining > 0)\n>                         /*\n>                          * Maybe somebody just deleted one of the\n> @@ -2325,8 +2326,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>                          * again:\n>                          */\n>                         goto retry;\n> -               else\n> -                       unable_to_lock_die(ref_file, errno);\n> +               else {\n> +                       struct strbuf err = STRBUF_INIT;\n> +                       unable_to_lock_message(ref_file, errno, &err);\n> +                       error(\"%s\", err.buf);\n> +                       strbuf_reset(&err);\n> +                       goto error_return;\n> +               }\n>         }\n>         return old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n>\n> --\n> 2.2.0.rc2.5.gf7b9fb2\n>\n"},{"id":"252156","messageId":"1416359597-15481-1-git-send-email-sbeller@google.com","threadId":"38005","inReplyTo":"CAGZ79kbH=pqSizhUeuiCeYXZWk015K49Pj0F5pPvkOTqg4PMuQ@mail.gmail.com","subject":"[PATCH] refs.c: handle locking failure during transaction better","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-19T01:13:17Z","receivedAt":"2014-11-19T01:13:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg <sahlberg@google.com>\n\nChange lock_ref_sha1_basic to return an error instead of dying,\nwhen we fail to lock a file during a transaction. This function is\nonly called from transaction_commit() and it knows how to handle\nthese failures.\n\n[sb: This was part of a larger patch series, cherry-picked to master]\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n refs.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..0347328 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2318,6 +2318,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n \tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n \tif (lock->lock_fd < 0) {\n+\t\tlast_errno = errno;\n \t\tif (errno == ENOENT && --attempts_remaining > 0)\n \t\t\t/*\n \t\t\t * Maybe somebody just deleted one of the\n@@ -2325,8 +2326,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t * again:\n \t\t\t */\n \t\t\tgoto retry;\n-\t\telse\n-\t\t\tunable_to_lock_die(ref_file, errno);\n+\t\telse {\n+\t\t\tstruct strbuf err = STRBUF_INIT;\n+\t\t\tunable_to_lock_message(ref_file, errno, &err);\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tstrbuf_reset(&err);\n+\t\t\tgoto error_return;\n+\t\t}\n \t}\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n-- \n2.2.0.rc2.5.gf7b9fb2\n"},{"id":"252157","messageId":"20141119013532.GA861@peff.net","threadId":"38005","inReplyTo":"1416359597-15481-1-git-send-email-sbeller@google.com","subject":"[PATCH 0/4] error cleanups in lock_ref_sha1_basic","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-19T01:35:32Z","receivedAt":"2014-11-19T01:35:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 18, 2014 at 05:13:17PM -0800, Stefan Beller wrote:\n\n> From: Ronnie Sahlberg <sahlberg@google.com>\n> \n> Change lock_ref_sha1_basic to return an error instead of dying,\n> when we fail to lock a file during a transaction. This function is\n> only called from transaction_commit() and it knows how to handle\n> these failures.\n> \n> [sb: This was part of a larger patch series, cherry-picked to master]\n> \n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n\nI think this is a good thing to do. I independently wrote the same patch\nrecently, along with some other cleanups. Here's the series I ended up\nwith (I added Ronnie as the author of the final one, which replaces\nthis; even though my discovery was independent, he wrote it first :) ).\n\n  [1/4]: error: save and restore errno\n  [2/4]: lock_ref_sha1_basic: simplify errno handling\n  [3/4]: lock_ref_sha1_basic: simplify error code path\n  [4/4]: lock_ref_sha1_basic: do not die on locking errors\n\n-Peff\n"},{"id":"252158","messageId":"20141119013710.GA2135@peff.net","threadId":"38005","inReplyTo":"20141119013532.GA861@peff.net","subject":"[PATCH 1/4] error: save and restore errno","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-19T01:37:10Z","receivedAt":"2014-11-19T01:37:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"It's common to use error() to return from a function, like:\n\n\tif (open(...) < 0)\n\t\treturn error(\"open failed\");\n\nUnfortunately this may clobber the errno from the open()\ncall. So we often end up with code like this:\n\n        if (open(...) < 0) {\n\t\tint saved_errno = errno;\n\t\terror(\"open failed\");\n\t\terrno = saved_errno;\n\t\treturn -1;\n\t}\n\nwhich is less nice. Let's teach error() to save and restore\nerrno in each call, so that the original errno is preserved.\nThis is slightly less efficient for callers which do not\ncare, but error code paths are generally not performance\ncritical anyway.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIt's pretty minor to just handle errno in the callers, but I feel like\nI've wanted this at least a dozen times, and it seems like it cannot\npossibly hurt (i.e., I imagine there are callers where we _should_ be\ndoing the errno dance but have not realized it).\n\n usage.c | 3 +++\n 1 file changed, 3 insertions(+)\n\ndiff --git a/usage.c b/usage.c\nindex ed14645..ee44d57 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -142,10 +142,13 @@ void NORETURN die_errno(const char *fmt, ...)\n int error(const char *err, ...)\n {\n \tva_list params;\n+\tint saved_errno = errno;\n \n \tva_start(params, err);\n \terror_routine(err, params);\n \tva_end(params);\n+\n+\terrno = saved_errno;\n \treturn -1;\n }\n \n-- \n2.1.2.596.g7379948\n"},{"id":"252159","messageId":"20141119013730.GB2135@peff.net","threadId":"38005","inReplyTo":"20141119013532.GA861@peff.net","subject":"[PATCH 2/4] lock_ref_sha1_basic: simplify errno handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-19T01:37:30Z","receivedAt":"2014-11-19T01:37:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Now that error() does not clobber errno, we do not have to\ntake pains to save it ourselves.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 7 +------\n 1 file changed, 1 insertion(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..169a46d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2232,7 +2232,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tchar *ref_file;\n \tconst char *orig_refname = refname;\n \tstruct ref_lock *lock;\n-\tint last_errno = 0;\n \tint type, lflags;\n \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n \tint resolve_flags = 0;\n@@ -2260,7 +2259,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t */\n \t\tref_file = git_path(\"%s\", orig_refname);\n \t\tif (remove_empty_directories(ref_file)) {\n-\t\t\tlast_errno = errno;\n \t\t\terror(\"there are still refs under '%s'\", orig_refname);\n \t\t\tgoto error_return;\n \t\t}\n@@ -2270,7 +2268,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tif (type_p)\n \t    *type_p = type;\n \tif (!refname) {\n-\t\tlast_errno = errno;\n \t\terror(\"unable to resolve reference %s: %s\",\n \t\t\torig_refname, strerror(errno));\n \t\tgoto error_return;\n@@ -2283,7 +2280,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t */\n \tif (missing &&\n \t     !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {\n-\t\tlast_errno = ENOTDIR;\n+\t\terrno = ENOTDIR;\n \t\tgoto error_return;\n \t}\n \n@@ -2311,7 +2308,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\tgoto retry;\n \t\t/* fall through */\n \tdefault:\n-\t\tlast_errno = errno;\n \t\terror(\"unable to create directory for %s\", ref_file);\n \t\tgoto error_return;\n \t}\n@@ -2332,7 +2328,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n  error_return:\n \tunlock_ref(lock);\n-\terrno = last_errno;\n \treturn NULL;\n }\n \n-- \n2.1.2.596.g7379948\n"},{"id":"252160","messageId":"20141119013739.GC2135@peff.net","threadId":"38005","inReplyTo":"20141119013532.GA861@peff.net","subject":"[PATCH 3/4] lock_ref_sha1_basic: simplify error code path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-19T01:37:40Z","receivedAt":"2014-11-19T01:37:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"For most errors, we jump to a goto label that unlocks the\nref and returns NULL. However, in none of these error paths\nwould we ever have actually locked the ref. By the time we\nactually take the lock, we follow a different path that does\nnot ever hit this goto (we rely on verify_lock to unlock if\nit finds an error).\n\nWe can just drop the goto completely and return NULL as\nappropriate.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 12 ++++--------\n 1 file changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 169a46d..fafae7e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2260,7 +2260,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\tref_file = git_path(\"%s\", orig_refname);\n \t\tif (remove_empty_directories(ref_file)) {\n \t\t\terror(\"there are still refs under '%s'\", orig_refname);\n-\t\t\tgoto error_return;\n+\t\t\treturn NULL;\n \t\t}\n \t\trefname = resolve_ref_unsafe(orig_refname, resolve_flags,\n \t\t\t\t\t     lock->old_sha1, &type);\n@@ -2270,7 +2270,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tif (!refname) {\n \t\terror(\"unable to resolve reference %s: %s\",\n \t\t\torig_refname, strerror(errno));\n-\t\tgoto error_return;\n+\t\treturn NULL;\n \t}\n \tmissing = is_null_sha1(lock->old_sha1);\n \t/* When the ref did not exist and we are creating it,\n@@ -2281,7 +2281,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \tif (missing &&\n \t     !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {\n \t\terrno = ENOTDIR;\n-\t\tgoto error_return;\n+\t\treturn NULL;\n \t}\n \n \tlock->lk = xcalloc(1, sizeof(struct lock_file));\n@@ -2309,7 +2309,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t/* fall through */\n \tdefault:\n \t\terror(\"unable to create directory for %s\", ref_file);\n-\t\tgoto error_return;\n+\t\treturn NULL;\n \t}\n \n \tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n@@ -2325,10 +2325,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\tunable_to_lock_die(ref_file, errno);\n \t}\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n-\n- error_return:\n-\tunlock_ref(lock);\n-\treturn NULL;\n }\n \n struct ref_lock *lock_any_ref_for_update(const char *refname,\n-- \n2.1.2.596.g7379948\n"},{"id":"252162","messageId":"20141119014112.GA2305@peff.net","threadId":"38005","inReplyTo":"20141119013532.GA861@peff.net","subject":"[PATCH 4/4] lock_ref_sha1_basic: do not die on locking errors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-19T01:41:12Z","receivedAt":"2014-11-19T01:41:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Ronnie Sahlberg <sahlberg@google.com>\n\nlock_ref_sha1_basic is inconsistent about when it calls\ndie() and when it returns NULL to signal an error. This is\nannoying to any callers that want to recover from a locking\nerror.\n\nThis seems to be mostly historical accident. It was added in\n4bd18c4 (Improve abstraction of ref lock/write.,\n2006-05-17), which returned an error in all cases except\ncalling safe_create_leading_directories, in which case it\ndied.  Later, 40aaae8 (Better error message when we are\nunable to lock the index file, 2006-08-12) asked\nhold_lock_file_for_update to die for us, leaving the\nresolve_ref code-path the only one which returned NULL.\n\nWe tried to correct that in 5cc3cef (lock_ref_sha1(): do not\nsometimes error() and sometimes die()., 2006-09-30),\nby converting all of the die() calls into returns. But we\nmissed the \"die\" flag passed to the lock code, leaving us\ninconsistent. This state persisted until e5c223e\n(lock_ref_sha1_basic(): if locking fails with ENOENT, retry,\n2014-01-18). Because of its retry scheme, it does not ask\nthe lock code to die, but instead manually dies with\nunable_to_lock_die().\n\nWe can make this consistent with the other return paths by\nconverting this to use unable_to_lock_message(), and\nreturning NULL. This is safe to do because all callers\nalready needed to check the return value of the function,\nsince it could fail (and return NULL) for other reasons.\n\n[jk: Added excessive history explanation and rebased on\n nearby cleanups.]\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nIt's a little sad to have to do the errno dance here after our earlier\ncleanups. But while error() is safe, unable_to_lock_message is anything\nbut (the original patch from Ronnie did not need this, because it leaves\nthe goto and last_errno in place). So I dunno. Maybe my cleanups were\nall for naught, and this end result is worse.\n\n refs.c | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex fafae7e..1f7e136 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2321,8 +2321,15 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t * again:\n \t\t\t */\n \t\t\tgoto retry;\n-\t\telse\n-\t\t\tunable_to_lock_die(ref_file, errno);\n+\t\telse {\n+\t\t\tint saved_errno = errno;\n+\t\t\tstruct strbuf buf = STRBUF_INIT;\n+\t\t\tunable_to_lock_message(ref_file, errno, &buf);\n+\t\t\terror(\"%s\", buf.buf);\n+\t\t\tstrbuf_release(&buf);\n+\t\t\terrno = saved_errno;\n+\t\t\treturn NULL;\n+\t\t}\n \t}\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n }\n-- \n2.1.2.596.g7379948\n"},{"id":"252161","messageId":"CAGZ79kbNatX51P3kocndyR+UVeACQR_u3OhkZiJmZrq_UBoJxg@mail.gmail.com","threadId":"38005","inReplyTo":"20141119013710.GA2135@peff.net","subject":"Re: [PATCH 1/4] error: save and restore errno","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-19T01:41:17Z","receivedAt":"2014-11-19T01:41:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This one makes my day.\nA really good fix as it minimizes maintenance burden for checking\nincoming patches for that pattern.\n\nReviewed-by: Stefan Beller <sbeller@google.com>\n\n\nOn Tue, Nov 18, 2014 at 5:37 PM, Jeff King <peff@peff.net> wrote:\n> It's common to use error() to return from a function, like:\n>\n>         if (open(...) < 0)\n>                 return error(\"open failed\");\n>\n> Unfortunately this may clobber the errno from the open()\n> call. So we often end up with code like this:\n>\n>         if (open(...) < 0) {\n>                 int saved_errno = errno;\n>                 error(\"open failed\");\n>                 errno = saved_errno;\n>                 return -1;\n>         }\n>\n> which is less nice. Let's teach error() to save and restore\n> errno in each call, so that the original errno is preserved.\n> This is slightly less efficient for callers which do not\n> care, but error code paths are generally not performance\n> critical anyway.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n> It's pretty minor to just handle errno in the callers, but I feel like\n> I've wanted this at least a dozen times, and it seems like it cannot\n> possibly hurt (i.e., I imagine there are callers where we _should_ be\n> doing the errno dance but have not realized it).\n>\n>  usage.c | 3 +++\n>  1 file changed, 3 insertions(+)\n>\n> diff --git a/usage.c b/usage.c\n> index ed14645..ee44d57 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -142,10 +142,13 @@ void NORETURN die_errno(const char *fmt, ...)\n>  int error(const char *err, ...)\n>  {\n>         va_list params;\n> +       int saved_errno = errno;\n>\n>         va_start(params, err);\n>         error_routine(err, params);\n>         va_end(params);\n> +\n> +       errno = saved_errno;\n>         return -1;\n>  }\n>\n> --\n> 2.1.2.596.g7379948\n>\n"},{"id":"252163","messageId":"20141119014344.GP6527@google.com","threadId":"38005","inReplyTo":"20141119013710.GA2135@peff.net","subject":"Re: [PATCH 1/4] error: save and restore errno","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-19T01:43:44Z","receivedAt":"2014-11-19T01:43:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> It's common to use error() to return from a function, like:\n>\n> \tif (open(...) < 0)\n> \t\treturn error(\"open failed\");\n>\n> Unfortunately this may clobber the errno from the open()\n> call. So we often end up with code like this:\n>\n>         if (open(...) < 0) {\n> \t\tint saved_errno = errno;\n> \t\terror(\"open failed\");\n> \t\terrno = saved_errno;\n> \t\treturn -1;\n> \t}\n>\n> which is less nice.\n\nWhat the above doesn't explain is why the caller cares about errno.\nAre they going to print another message with strerror(errno)?  Or are\nthey going to consider some errors non-errors (like ENOENT when trying\nto unlink a file), in which case why is printing a message to stderr\nokay?\n\nAll that said, given that there are real examples of code already\ndoing this, the patch seems sane.\n\n[...]\n>  usage.c | 3 +++\n>  1 file changed, 3 insertions(+)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"252165","messageId":"20141119014722.GB2337@peff.net","threadId":"38005","inReplyTo":"20141119014344.GP6527@google.com","subject":"Re: [PATCH 1/4] error: save and restore errno","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-19T01:47:23Z","receivedAt":"2014-11-19T01:47:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 18, 2014 at 05:43:44PM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > It's common to use error() to return from a function, like:\n> >\n> > \tif (open(...) < 0)\n> > \t\treturn error(\"open failed\");\n> >\n> > Unfortunately this may clobber the errno from the open()\n> > call. So we often end up with code like this:\n> >\n> >         if (open(...) < 0) {\n> > \t\tint saved_errno = errno;\n> > \t\terror(\"open failed\");\n> > \t\terrno = saved_errno;\n> > \t\treturn -1;\n> > \t}\n> >\n> > which is less nice.\n> \n> What the above doesn't explain is why the caller cares about errno.\n> Are they going to print another message with strerror(errno)?  Or are\n> they going to consider some errors non-errors (like ENOENT when trying\n> to unlink a file), in which case why is printing a message to stderr\n> okay?\n\nI guess the unsaid bit is:\n\n  Unfortunately this may clobber the errno from the open() call. Even\n  though error() sees the correct errno, the caller to which we are\n  returning may see a bogus errno value.\n\n-Peff\n"},{"id":"252167","messageId":"20141119015452.GQ6527@google.com","threadId":"38005","inReplyTo":"20141119013730.GB2135@peff.net","subject":"Re: [PATCH 2/4] lock_ref_sha1_basic: simplify errno handling","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-19T01:54:52Z","receivedAt":"2014-11-19T01:54:52Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Now that error() does not clobber errno, we do not have to\n> take pains to save it ourselves.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  refs.c | 7 +------\n>  1 file changed, 1 insertion(+), 6 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 5ff457e..169a46d 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2232,7 +2232,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n\nAll the caller to lock_ref_sha1_basic cares about is whether errno ==\nENOTDIR, since in that case we can print a message suggesting running\n\"git remote prune\".\n\nLonger term, I suspect the caller (transaction_commit) should call\nis_refname_available to check for conflicting refs and return early to\ngive \"git fetch\" a chance to print its advice.\n\nIf a conflicting ref appears after that point, then just printing a\nreasonable error message is enough --- it is not so useful to give the\n'remote prune' advice when people are doing funny things like running\nfetch in one terminal and a ref update creating a D/F conflict against\nthat fetch in another terminal.[*]\n\nBy the way, Stefan was mentioning the other day that it might make\nsense for transaction_commit to prevent a conflicting ref from\nappearing mid-transaction by locking 'refs/heads' in addition to\n'refs/heads/master'.\n\nAnyway, in the meantime this is a nice cleanup.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\n[*] If we want to handle that, the 'struct strbuf *err' could be\nreplaced with a larger struct with room for structured data, like\nJunio was hinting at in another thread.\n"},{"id":"252168","messageId":"20141119020009.GR6527@google.com","threadId":"38005","inReplyTo":"20141119013739.GC2135@peff.net","subject":"Re: [PATCH 3/4] lock_ref_sha1_basic: simplify error code path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-19T02:00:09Z","receivedAt":"2014-11-19T02:00:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> For most errors, we jump to a goto label that unlocks the\n> ref and returns NULL. However, in none of these error paths\n> would we ever have actually locked the ref. By the time we\n> actually take the lock, we follow a different path that does\n> not ever hit this goto (we rely on verify_lock to unlock if\n> it finds an error).\n[...]\n>  refs.c | 12 ++++--------\n>  1 file changed, 4 insertions(+), 8 deletions(-)\n\nWouldn't this leak lock (in all cases) and lock->ref_name and\nlock->orig_ref_name (on safe_create_leading_directories failure)?\n"},{"id":"252169","messageId":"20141119020451.GA2734@peff.net","threadId":"38005","inReplyTo":"20141119020009.GR6527@google.com","subject":"Re: [PATCH 3/4] lock_ref_sha1_basic: simplify error code path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-19T02:04:52Z","receivedAt":"2014-11-19T02:04:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 18, 2014 at 06:00:09PM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > For most errors, we jump to a goto label that unlocks the\n> > ref and returns NULL. However, in none of these error paths\n> > would we ever have actually locked the ref. By the time we\n> > actually take the lock, we follow a different path that does\n> > not ever hit this goto (we rely on verify_lock to unlock if\n> > it finds an error).\n> [...]\n> >  refs.c | 12 ++++--------\n> >  1 file changed, 4 insertions(+), 8 deletions(-)\n> \n> Wouldn't this leak lock (in all cases) and lock->ref_name and\n> lock->orig_ref_name (on safe_create_leading_directories failure)?\n\nAh, you're right. I totally missed that unlock_ref is not just about\nunlocking, but about free()ing. We do need to keep the goto/unlock.\n\nHmph. Should we just abandon my series in favor of taking Ronnie's\noriginal patch, then? We can apply the \"save/restore errno in error()\"\npatch independently if we like.\n\n-Peff\n"},{"id":"252170","messageId":"20141119020503.GS6527@google.com","threadId":"38005","inReplyTo":"20141119014112.GA2305@peff.net","subject":"Re: [PATCH 4/4] lock_ref_sha1_basic: do not die on locking errors","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-19T02:05:03Z","receivedAt":"2014-11-19T02:05:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> From: Ronnie Sahlberg <sahlberg@google.com>\n>\n> lock_ref_sha1_basic is inconsistent about when it calls\n> die() and when it returns NULL to signal an error. This is\n> annoying to any callers that want to recover from a locking\n> error.\n\nAnd in addition to the modern transaction stuff, rename_ref() has been\nsuch a caller for a long time.\n\nThanks for a pleasant explanation.\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\n[...]\n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>\n\nNo need for my sign-off here --- it was just an artifact of my\nforwarding the patch to the list before.\n"},{"id":"252171","messageId":"20141119020713.GT6527@google.com","threadId":"38005","inReplyTo":"20141119020451.GA2734@peff.net","subject":"Re: [PATCH 3/4] lock_ref_sha1_basic: simplify error code path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-19T02:07:13Z","receivedAt":"2014-11-19T02:07:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Hmph. Should we just abandon my series in favor of taking Ronnie's\n> original patch, then? We can apply the \"save/restore errno in error()\"\n> patch independently if we like.\n\nI liked patches 1 and 2 and the explanation from patch 4.  Perhaps\npatch 3 should be replaced with a patch renaming unlock_ref to\nfree_ref_lock or something.\n\nThanks,\nJonathan\n"},{"id":"252187","messageId":"xmqqvbmbrrba.fsf@gitster.dls.corp.google.com","threadId":"38005","inReplyTo":"20141119014722.GB2337@peff.net","subject":"Re: [PATCH 1/4] error: save and restore errno","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-19T18:14:17Z","receivedAt":"2014-11-19T18:14:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Nov 18, 2014 at 05:43:44PM -0800, Jonathan Nieder wrote:\n>\n>> Jeff King wrote:\n>> \n>> > It's common to use error() to return from a function, like:\n>> >\n>> > \tif (open(...) < 0)\n>> > \t\treturn error(\"open failed\");\n>> >\n>> > Unfortunately this may clobber the errno from the open()\n>> > call. So we often end up with code like this:\n>> >\n>> >         if (open(...) < 0) {\n>> > \t\tint saved_errno = errno;\n>> > \t\terror(\"open failed\");\n>> > \t\terrno = saved_errno;\n>> > \t\treturn -1;\n>> > \t}\n>> >\n>> > which is less nice.\n>> \n>> What the above doesn't explain is why the caller cares about errno.\n>> Are they going to print another message with strerror(errno)?  Or are\n>> they going to consider some errors non-errors (like ENOENT when trying\n>> to unlink a file), in which case why is printing a message to stderr\n>> okay?\n>\n> I guess the unsaid bit is:\n>\n>   Unfortunately this may clobber the errno from the open() call. Even\n>   though error() sees the correct errno, the caller to which we are\n>   returning may see a bogus errno value.\n>\n> -Peff\n\nI am not sure if that answers the question asked.\n\nIf you have\n\n\tint frotz(...) {\n\t\tint fd = open(...);\n        \tif (fd < 0)\n                \treturn error(\"open failed (%s)\", strerror(errno));\n\t\treturn fd;\n\t}\n\nand the caller calls it and cares about the errno from this open,\nwhat does the caller do?  Jonathan's worried about a codepath that\nmay be familiar to us as we recently saw a patch similar to it:\n\n\tint fd = frotz(...);\n        if (fd < 0) {\n        \tif (errno == ENOENT || errno == EISDIR)\n                \t; /* not quite an error */\n\t\telse\n\t\t\texit(1);\n\t}\n\nIf ENOENT/EISDIR is expected and a non-error, it is not useful for\nfrotz() to give an error message on its own.\n\nI think a more appropriate answer to Jonathan's question is why is\nthe callee (i.e. frotz()) calling error() in the first place if an\nunconditional error message is an issue.\n"},{"id":"252188","messageId":"20141119182812.GA9425@peff.net","threadId":"38005","inReplyTo":"xmqqvbmbrrba.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/4] error: save and restore errno","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-19T18:28:12Z","receivedAt":"2014-11-19T18:28:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 19, 2014 at 10:14:17AM -0800, Junio C Hamano wrote:\n\n> >> What the above doesn't explain is why the caller cares about errno.\n> >> Are they going to print another message with strerror(errno)?  Or are\n> >> they going to consider some errors non-errors (like ENOENT when trying\n> >> to unlink a file), in which case why is printing a message to stderr\n> >> okay?\n> >\n> > I guess the unsaid bit is:\n> >\n> >   Unfortunately this may clobber the errno from the open() call. Even\n> >   though error() sees the correct errno, the caller to which we are\n> >   returning may see a bogus errno value.\n> >\n> I am not sure if that answers the question asked.\n> \n> If you have\n> \n> \tint frotz(...) {\n> \t\tint fd = open(...);\n>         \tif (fd < 0)\n>                 \treturn error(\"open failed (%s)\", strerror(errno));\n> \t\treturn fd;\n> \t}\n> \n> and the caller calls it and cares about the errno from this open,\n> what does the caller do?  Jonathan's worried about a codepath that\n> may be familiar to us as we recently saw a patch similar to it:\n> \n> \tint fd = frotz(...);\n>         if (fd < 0) {\n>         \tif (errno == ENOENT || errno == EISDIR)\n>                 \t; /* not quite an error */\n> \t\telse\n> \t\t\texit(1);\n> \t}\n> \n> If ENOENT/EISDIR is expected and a non-error, it is not useful for\n> frotz() to give an error message on its own.\n\nSure, but isn't that out-of-scope for this patch? We know that some\nfunctions _are_ calling error() and taking care to keep errno valid, and\nwe would prefer to do that with less work.\n\nIf you are arguing \"there are literally zero cases where that makes\nsense; functions should either complain themselves, _or_ they should\npass on a valid errno so the caller can decide whether to complain, but\ndoing both is a recipe for pointless and unwanted error messages\", then\nthis patch is counter-productive (and we should be fixing those call\nsites of error() instead).\n\nBut I am not sure that there are zero legitimate cases. Just as a\nhypothetical, imagine you wanted to complain to stderr _and_ propagate\nthe error value into a specific exit code. You'd want something like:\n\n  fd = frotz(...);\n  error(\"frotz failed (%s)\", strerror(errno));\n  exit(errno == ENOENT ? 1 : 2);\n\nGranted, I do not think we do that particular pattern in git, but we do\ntake pains to save errno across some error() calls. I do not know which\nof those are legitimate and which should be refactored. In the meantime,\nI do not think this patch makes anything worse.\n\n> I think a more appropriate answer to Jonathan's question is why is\n> the callee (i.e. frotz()) calling error() in the first place if an\n> unconditional error message is an issue.\n\nExactly. It is not error()'s business to know what the caller wants to\ndo, or whether or why it cares about retaining errno. But error(), since\nit is designed to be called in error codepaths, should aim to be\nminimally intrusive.\n\n-Peff\n"},{"id":"252215","messageId":"xmqqfvderhq9.fsf@gitster.dls.corp.google.com","threadId":"38005","inReplyTo":"20141119020713.GT6527@google.com","subject":"Re: [PATCH 3/4] lock_ref_sha1_basic: simplify error code path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-19T21:41:18Z","receivedAt":"2014-11-19T21:41:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder <jrnieder@gmail.com> writes:\n\n> Jeff King wrote:\n>\n>> Hmph. Should we just abandon my series in favor of taking Ronnie's\n>> original patch, then? We can apply the \"save/restore errno in error()\"\n>> patch independently if we like.\n>\n> I liked patches 1 and 2 and the explanation from patch 4.  Perhaps\n> patch 3 should be replaced with a patch renaming unlock_ref to\n> free_ref_lock or something.\n\nConcurred.\n"},{"id":"252220","messageId":"20141119222852.GA12236@peff.net","threadId":"38005","inReplyTo":"20141119020713.GT6527@google.com","subject":"Re: [PATCH 3/4] lock_ref_sha1_basic: simplify error code path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-19T22:28:52Z","receivedAt":"2014-11-19T22:28:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 18, 2014 at 06:07:13PM -0800, Jonathan Nieder wrote:\n\n> Jeff King wrote:\n> \n> > Hmph. Should we just abandon my series in favor of taking Ronnie's\n> > original patch, then? We can apply the \"save/restore errno in error()\"\n> > patch independently if we like.\n> \n> I liked patches 1 and 2 and the explanation from patch 4.  Perhaps\n> patch 3 should be replaced with a patch renaming unlock_ref to\n> free_ref_lock or something.\n\nI took a look at this, and it ends up not being very useful.\n\nThe \"return NULL\" from the final patch has to become a \"goto\nerror_return\", so that it can call unlock_ref(). But that means it\ncannot save and restore errno itself, because unlock_ref may clobber\nerrno[1].\n\nSo we still have to keep the last_errno handling in error_return.\nMeaning that we need to drop patch 2 (even though the other cases don't\nneed errno saved/restore, since the goto does it unconditionally, we\nstill need to set last_errno). And therefore patch 1 is not helping\nanyone (we could still apply it, but there's no immediate benefit).\n\nI also looked at renaming unlock_ref, but it is called from other places\nwhere they _do_ care about unlocking. So renaming is out. We could build\na free_ref_lock that unlock_ref builds on, but that brings up another\nconfusion: can we or can we not free the \"struct lockfile\" pointer it\ncontains (and which should free_ref_lock do)? Whether it is safe to do\nso depends on whether we have actually fed it to hold_lock_file_for_update\nor not (even if it fails, it takes ownership of the lock). So we\nactually leak it in some cases, but the only case we could fix is when\nsafe_create_leading_directories the _first_ time (before we have ever\ntried to lock and failed, at which point we loop). Yeesh.\n\nSo I really think we are better off leaving it as-is, and just applying\nsome form of Ronnie's patch (which does the right thing with errno). The\nother cleanups end up making things worse, and the unlock_ref thing was\njust my misunderstanding.\n\nSo here is that patch, with my explanation. Thanks for your patience in\nmy running around in circles. :)\n\n-- >8 --\nSubject: lock_ref_sha1_basic: do not die on locking errors\n\nlock_ref_sha1_basic is inconsistent about when it calls\ndie() and when it returns NULL to signal an error. This is\nannoying to any callers that want to recover from a locking\nerror.\n\nThis seems to be mostly historical accident. It was added in\n4bd18c4 (Improve abstraction of ref lock/write.,\n2006-05-17), which returned an error in all cases except\ncalling safe_create_leading_directories, in which case it\ndied.  Later, 40aaae8 (Better error message when we are\nunable to lock the index file, 2006-08-12) asked\nhold_lock_file_for_update to die for us, leaving the\nresolve_ref code-path the only one which returned NULL.\n\nWe tried to correct that in 5cc3cef (lock_ref_sha1(): do not\nsometimes error() and sometimes die()., 2006-09-30),\nby converting all of the die() calls into returns. But we\nmissed the \"die\" flag passed to the lock code, leaving us\ninconsistent. This state persisted until e5c223e\n(lock_ref_sha1_basic(): if locking fails with ENOENT, retry,\n2014-01-18). Because of its retry scheme, it does not ask\nthe lock code to die, but instead manually dies with\nunable_to_lock_die().\n\nWe can make this consistent with the other return paths by\nconverting this to use unable_to_lock_message(), and\nreturning NULL. This is safe to do because all callers\nalready needed to check the return value of the function,\nsince it could fail (and return NULL) for other reasons.\n\n[jk: Added excessive history explanation]\n\nSigned-off-by: Ronnie Sahlberg <sahlberg@google.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n refs.c | 10 ++++++++--\n 1 file changed, 8 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..0347328 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2318,6 +2318,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \n \tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n \tif (lock->lock_fd < 0) {\n+\t\tlast_errno = errno;\n \t\tif (errno == ENOENT && --attempts_remaining > 0)\n \t\t\t/*\n \t\t\t * Maybe somebody just deleted one of the\n@@ -2325,8 +2326,13 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t * again:\n \t\t\t */\n \t\t\tgoto retry;\n-\t\telse\n-\t\t\tunable_to_lock_die(ref_file, errno);\n+\t\telse {\n+\t\t\tstruct strbuf err = STRBUF_INIT;\n+\t\t\tunable_to_lock_message(ref_file, errno, &err);\n+\t\t\terror(\"%s\", err.buf);\n+\t\t\tstrbuf_reset(&err);\n+\t\t\tgoto error_return;\n+\t\t}\n \t}\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n-- \n2.1.2.596.g7379948\n"},{"id":"252221","messageId":"xmqq61earfa3.fsf@gitster.dls.corp.google.com","threadId":"38005","inReplyTo":"20141119222852.GA12236@peff.net","subject":"Re: [PATCH 3/4] lock_ref_sha1_basic: simplify error code path","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-19T22:34:12Z","receivedAt":"2014-11-19T22:34:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> So we still have to keep the last_errno handling in error_return.\n> Meaning that we need to drop patch 2 (even though the other cases don't\n> need errno saved/restore, since the goto does it unconditionally, we\n> still need to set last_errno). And therefore patch 1 is not helping\n> anyone (we could still apply it, but there's no immediate benefit).\n> ...\n> So here is that patch, with my explanation. Thanks for your patience in\n> my running around in circles. :)\n\nMy head has been spinning, but I think I can relax now ~@~ ;-)\n\n> Subject: lock_ref_sha1_basic: do not die on locking errors\n\nWill queue; thanks.\n"},{"id":"252222","messageId":"20141119223643.GA13599@peff.net","threadId":"38005","inReplyTo":"xmqq61earfa3.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 3/4] lock_ref_sha1_basic: simplify error code path","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-19T22:36:43Z","receivedAt":"2014-11-19T22:36:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 19, 2014 at 02:34:12PM -0800, Junio C Hamano wrote:\n\n> > Subject: lock_ref_sha1_basic: do not die on locking errors\n> \n> Will queue; thanks.\n\nI just noticed that when I pasted it into the mail, I dropped the\n\n  From: Ronnie Sahlberg <sahlberg@google.com>\n\nheader. Can you please make sure to credit Ronnie as you apply?\n\nThanks.\n\n-Peff\n"},{"id":"252228","messageId":"20141120010714.GA6527@google.com","threadId":"38005","inReplyTo":"20141119222852.GA12236@peff.net","subject":"Re: [PATCH 3/4] lock_ref_sha1_basic: simplify error code path","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T01:07:14Z","receivedAt":"2014-11-20T01:07:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Signed-off-by: Ronnie Sahlberg <sahlberg@google.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  refs.c | 10 ++++++++--\n>  1 file changed, 8 insertions(+), 2 deletions(-)\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\n(modulo the author attribution nit you noticed)\n"},{"id":"252325","messageId":"546F04F3.5060407@alum.mit.edu","threadId":"38005","inReplyTo":"20141119013730.GB2135@peff.net","subject":"Re: [PATCH 2/4] lock_ref_sha1_basic: simplify errno handling","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-21T09:25:07Z","receivedAt":"2014-11-21T09:25:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/19/2014 02:37 AM, Jeff King wrote:\n> Now that error() does not clobber errno, we do not have to\n> take pains to save it ourselves.\n> \n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  refs.c | 7 +------\n>  1 file changed, 1 insertion(+), 6 deletions(-)\n> \n> diff --git a/refs.c b/refs.c\n> index 5ff457e..169a46d 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2232,7 +2232,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>  \tchar *ref_file;\n>  \tconst char *orig_refname = refname;\n>  \tstruct ref_lock *lock;\n> -\tint last_errno = 0;\n>  \tint type, lflags;\n>  \tint mustexist = (old_sha1 && !is_null_sha1(old_sha1));\n>  \tint resolve_flags = 0;\n> @@ -2260,7 +2259,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>  \t\t */\n>  \t\tref_file = git_path(\"%s\", orig_refname);\n>  \t\tif (remove_empty_directories(ref_file)) {\n> -\t\t\tlast_errno = errno;\n>  \t\t\terror(\"there are still refs under '%s'\", orig_refname);\n>  \t\t\tgoto error_return;\n>  \t\t}\n> @@ -2270,7 +2268,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>  \tif (type_p)\n>  \t    *type_p = type;\n>  \tif (!refname) {\n> -\t\tlast_errno = errno;\n>  \t\terror(\"unable to resolve reference %s: %s\",\n>  \t\t\torig_refname, strerror(errno));\n>  \t\tgoto error_return;\n> @@ -2283,7 +2280,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>  \t */\n>  \tif (missing &&\n>  \t     !is_refname_available(refname, skip, get_packed_refs(&ref_cache))) {\n> -\t\tlast_errno = ENOTDIR;\n> +\t\terrno = ENOTDIR;\n>  \t\tgoto error_return;\n>  \t}\n>  \n> @@ -2311,7 +2308,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>  \t\t\tgoto retry;\n>  \t\t/* fall through */\n>  \tdefault:\n> -\t\tlast_errno = errno;\n>  \t\terror(\"unable to create directory for %s\", ref_file);\n>  \t\tgoto error_return;\n>  \t}\n> @@ -2332,7 +2328,6 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>  \n>   error_return:\n>  \tunlock_ref(lock);\n> -\terrno = last_errno;\n>  \treturn NULL;\n>  }\n\nerror() doesn't clobber errno anymore, but unlock_ref() still might.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"}]}