{"thread":{"id":"37988","subject":"[PATCH] copy.c: make copy_fd preserve meaningful errno","startedAt":"2014-11-17T22:14:13Z","lastAt":"2015-02-17T22:46:48Z","messageCount":83,"participants":["Stefan Beller","Junio C Hamano","Jonathan Nieder","Michael Haggerty","Jeff King","Torsten Bögershausen","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252006","messageId":"1416262453-30349-1-git-send-email-sbeller@google.com","threadId":"37988","inReplyTo":null,"subject":"[PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-17T22:14:13Z","receivedAt":"2014-11-17T22:14:13Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nUpdate copy_fd to return a meaningful errno on failure and also \npreserve the existing errno variable.\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\nAs announced in [1], I'm going to take over the \nref-transactions-reflog series by Ronnie Sahlberg.\n\nThis patch was sent previously to the list as part of\nthat series[2], but it seems to be unrelated to me.\n\nThanks,\nStefan\n\n[1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html\n[2] http://www.spinics.net/lists/git/msg240784.html\n copy.c | 15 ++++++++++-----\n 1 file changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/copy.c b/copy.c\nindex f2970ec..a8d366e 100644\n--- a/copy.c\n+++ b/copy.c\n@@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)\n \t\tif (!len)\n \t\t\tbreak;\n \t\tif (len < 0) {\n-\t\t\treturn error(\"copy-fd: read returned %s\",\n-\t\t\t\t strerror(errno));\n+\t\t\tint save_errno = errno;\n+\t\t\terror(\"copy-fd: read returned %s\", strerror(errno));\n+\t\t\terrno = save_errno;\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (write_in_full(ofd, buffer, len) < 0) {\n+\t\t\tint save_errno = errno;\n+\t\t\terror(\"copy-fd: write returned %s\", strerror(errno));\n+\t\t\terrno = save_errno;\n+\t\t\treturn -1;\n \t\t}\n-\t\tif (write_in_full(ofd, buffer, len) < 0)\n-\t\t\treturn error(\"copy-fd: write returned %s\",\n-\t\t\t\t strerror(errno));\n \t}\n \treturn 0;\n }\n-- \n2.2.0.rc1.24.g562add4\n"},{"id":"252010","messageId":"xmqq4mtxxw5z.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"1416262453-30349-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-17T23:08:24Z","receivedAt":"2014-11-17T23:08:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> This patch was sent previously to the list as part of\n> that series[2], but it seems to be unrelated to me.\n\nI am fine to queue obvious and trivial bits first before the larger\nmain course. For now I'll queue this one and also the series that\nhas been queued for a while, but at some point I suspect we would\nhave to drop the latter.\n\nThanks.\n\n\n> [1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html\n> [2] http://www.spinics.net/lists/git/msg240784.html\n> copy.c | 15 ++++++++++-----\n> 1 file changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/copy.c b/copy.c\n> index f2970ec..a8d366e 100644\n> --- a/copy.c\n> +++ b/copy.c\n> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)\n> \t\tif (!len)\n> \t\t\tbreak;\n> \t\tif (len < 0) {\n> -\t\t\treturn error(\"copy-fd: read returned %s\",\n> -\t\t\t\t strerror(errno));\n> +\t\t\tint save_errno = errno;\n> +\t\t\terror(\"copy-fd: read returned %s\", strerror(errno));\n> +\t\t\terrno = save_errno;\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\tif (write_in_full(ofd, buffer, len) < 0) {\n> +\t\t\tint save_errno = errno;\n> +\t\t\terror(\"copy-fd: write returned %s\", strerror(errno));\n> +\t\t\terrno = save_errno;\n> +\t\t\treturn -1;\n> \t\t}\n> -\t\tif (write_in_full(ofd, buffer, len) < 0)\n> -\t\t\treturn error(\"copy-fd: write returned %s\",\n> -\t\t\t\t strerror(errno));\n> \t}\n> \treturn 0;\n> }\n"},{"id":"252011","messageId":"CAGZ79kYHzd8ODEXoZyGSb7dkZfXHAoWoiPEUqhsjedMSAjTiXg@mail.gmail.com","threadId":"37988","inReplyTo":"xmqq4mtxxw5z.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-17T23:13:27Z","receivedAt":"2014-11-17T23:13:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"I am reviewing the series and about to resend it with very minor nits\nfixed. I just want to point out this fix is orthogonal to the series\nand can be picked up no matter how long the reviewing/discussion of\nthe series goes.\n\nThanks,\nStefan\n\nOn Mon, Nov 17, 2014 at 3:08 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> This patch was sent previously to the list as part of\n>> that series[2], but it seems to be unrelated to me.\n>\n> I am fine to queue obvious and trivial bits first before the larger\n> main course. For now I'll queue this one and also the series that\n> has been queued for a while, but at some point I suspect we would\n> have to drop the latter.\n>\n> Thanks.\n>\n>\n>> [1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html\n>> [2] http://www.spinics.net/lists/git/msg240784.html\n>> copy.c | 15 ++++++++++-----\n>> 1 file changed, 10 insertions(+), 5 deletions(-)\n>>\n>> diff --git a/copy.c b/copy.c\n>> index f2970ec..a8d366e 100644\n>> --- a/copy.c\n>> +++ b/copy.c\n>> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)\n>> if (!len)\n>> break;\n>> if (len < 0) {\n>> - return error(\"copy-fd: read returned %s\",\n>> - strerror(errno));\n>> + int save_errno = errno;\n>> + error(\"copy-fd: read returned %s\", strerror(errno));\n>> + errno = save_errno;\n>> + return -1;\n>> + }\n>> + if (write_in_full(ofd, buffer, len) < 0) {\n>> + int save_errno = errno;\n>> + error(\"copy-fd: write returned %s\", strerror(errno));\n>> + errno = save_errno;\n>> + return -1;\n>> }\n>> - if (write_in_full(ofd, buffer, len) < 0)\n>> - return error(\"copy-fd: write returned %s\",\n>> - strerror(errno));\n>> }\n>> return 0;\n>> }\n"},{"id":"252014","messageId":"20141117233525.GC4336@google.com","threadId":"37988","inReplyTo":"1416262453-30349-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-17T23:35:25Z","receivedAt":"2014-11-17T23:35:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nStefan Beller wrote:\n\n> This patch was sent previously to the list as part of\n> that series[2], but it seems to be unrelated to me.\n\nThanks. Good call.\n\n[...]\n> From: Ronnie Sahlberg \n>\n> Update copy_fd to return a meaningful errno on failure and also\n> preserve the existing errno variable.\n\nSome functions in git make errno meaningful on error and others don't.\nIn general, the more we only use errno immediately after a system\ncall, the better, so based on the above description this seems like a\nstep in the wrong direction.\n\nDo any callers care about errno? Does the function's API\ndocumentation say it will make errno meaningful on error, so people\nmaking changes to copy_fd in the future know to maintain that\nproperty?\n\n*searches*\n\nLooks like callers are:\n\n convert.c::filter_buffer_or_fd, which doesn't care\n\n copy.c::copy_file, which also doesn't care\n\n lockfile.c::hold_lock_file_for_append, which started caring\n in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno\n before returning, 2014-10-01). But no callers of that function\n care yet.\n\nSo this is about fixing a bug-waiting-to-happen in\nhold_lock_file_for_append. That would be enough to motivate the\nchange.\n\n[...]\n> copy.c | 15 ++++++++++-----\n> 1 file changed, 10 insertions(+), 5 deletions(-)\n\nPlease also update the API documentation in cache.h so we remember not\nto backslide in the future.\n\n[...]\n> --- a/copy.c\n> +++ b/copy.c\n> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)\n> \t\tif (!len)\n> \t\t\tbreak;\n> \t\tif (len < 0) {\n> -\t\t\treturn error(\"copy-fd: read returned %s\",\n> -\t\t\t\t strerror(errno));\n> +\t\t\tint save_errno = errno;\n> +\t\t\terror(\"copy-fd: read returned %s\", strerror(errno));\n> +\t\t\terrno = save_errno;\n> +\t\t\treturn -1;\n\nAny caller is presumably going to turn around and print strerror(errno)\nagain, producing repetitive output.\n\nCan we do better? E.g., if the signature were\n\n\tint copy_fd(int ifd, int ofd, struct strbuf *err);\n\nthen we could write the error message to the err strbuf for the\ncaller to print. The error handling would be more explicit so\nthere would be no need to protect errno from clobbering by other\nsystem calls (both here and in callers).\n\nSomething like this:\n\nSigned-off-by: Jonathan Nieder \n\ndiff --git i/cache.h w/cache.h\nindex 99ed096..ddaa30f 100644\n--- i/cache.h\n+++ w/cache.h\n@@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;\n extern void maybe_flush_or_die(FILE *, const char *);\n __attribute__((format (printf, 2, 3)))\n extern void fprintf_or_die(FILE *, const char *fmt, ...);\n-extern int copy_fd(int ifd, int ofd);\n+extern int copy_fd(int ifd, int ofd, struct strbuf *err);\n extern int copy_file(const char *dst, const char *src, int mode);\n extern int copy_file_with_time(const char *dst, const char *src, int mode);\n extern void write_or_die(int fd, const void *buf, size_t count);\ndiff --git i/convert.c w/convert.c\nindex 9a5612e..e301447 100644\n--- i/convert.c\n+++ w/convert.c\n@@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \tif (params->src) {\n \t\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n \t} else {\n-\t\twrite_err = copy_fd(params->fd, child_process.in);\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\twrite_err = copy_fd(params->fd, child_process.in, &err);\n+\t\tif (write_err)\n+\t\t\terror(\"copy-fd: %s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t}\n \n \tif (close(child_process.in))\ndiff --git i/copy.c w/copy.c\nindex f2970ec..828661a 100644\n--- i/copy.c\n+++ w/copy.c\n@@ -1,19 +1,22 @@\n #include \"cache.h\"\n \n-int copy_fd(int ifd, int ofd)\n+int copy_fd(int ifd, int ofd, struct strbuf *err)\n {\n+\tassert(err);\n+\n \twhile (1) {\n \t\tchar buffer[8192];\n \t\tssize_t len = xread(ifd, buffer, sizeof(buffer));\n \t\tif (!len)\n \t\t\tbreak;\n \t\tif (len < 0) {\n-\t\t\treturn error(\"copy-fd: read returned %s\",\n-\t\t\t\t strerror(errno));\n+\t\t\tstrbuf_addf(err, \"read returned %s\", strerror(errno));\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (write_in_full(ofd, buffer, len) < 0) {\n+\t\t\tstrbuf_addf(err, \"write returned %s\", strerror(errno));\n+\t\t\treturn -1;\n \t\t}\n-\t\tif (write_in_full(ofd, buffer, len) < 0)\n-\t\t\treturn error(\"copy-fd: write returned %s\",\n-\t\t\t\t strerror(errno));\n \t}\n \treturn 0;\n }\n@@ -33,7 +36,8 @@ static int copy_times(const char *dst, const char *src)\n \n int copy_file(const char *dst, const char *src, int mode)\n {\n-\tint fdi, fdo, status;\n+\tint fdi, fdo;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tmode = (mode & 0111) ? 0777 : 0666;\n \tif ((fdi = open(src, O_RDONLY)) < 0)\n@@ -42,15 +46,21 @@ int copy_file(const char *dst, const char *src, int mode)\n \t\tclose(fdi);\n \t\treturn fdo;\n \t}\n-\tstatus = copy_fd(fdi, fdo);\n+\tif (copy_fd(fdi, fdo, &err)) {\n+\t\tclose(fdi);\n+\t\tclose(fdo);\n+\t\terror(\"copy-fd: %s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n+\tstrbuf_release(&err);\n \tclose(fdi);\n \tif (close(fdo) != 0)\n \t\treturn error(\"%s: close error: %s\", dst, strerror(errno));\n-\n-\tif (!status && adjust_shared_perm(dst))\n+\tif (adjust_shared_perm(dst))\n \t\treturn -1;\n \n-\treturn status;\n+\treturn 0;\n }\n \n int copy_file_with_time(const char *dst, const char *src, int mode)\ndiff --git i/lockfile.c w/lockfile.c\nindex 4f16ee7..c47976e 100644\n--- i/lockfile.c\n+++ w/lockfile.c\n@@ -179,37 +179,52 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n \treturn fd;\n }\n \n-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n+/*\n+ * Like strbuf_addf but inserts at the front of a strbuf instead\n+ * of appending.\n+ */\n+static void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)\n+{\n+\tsize_t pos, len;\n+\tva_list ap;\n+\n+\tpos = sb->len;\n+\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(sb, fmt, ap);\n+\tva_end(ap);\n+\n+\tlen = sb->len - pos;\n+\tstrbuf_insert(sb, 0, sb->buf + pos, len);\n+\tstrbuf_remove(sb, pos + len, len);\n+}\n+\n+int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n+\t\t\t int flags, struct strbuf *err)\n {\n \tint fd, orig_fd;\n \n+\tassert(!(flags & LOCK_DIE_ON_ERROR));\n+\tassert(err && !err->len);\n+\n \tfd = lock_file(lk, path, flags);\n \tif (fd < 0) {\n-\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\tunable_to_lock_die(path, errno);\n+\t\tunable_to_lock_message(path, errno, err);\n \t\treturn fd;\n \t}\n \n \torig_fd = open(path, O_RDONLY);\n \tif (orig_fd < 0) {\n \t\tif (errno != ENOENT) {\n-\t\t\tint save_errno = errno;\n-\n-\t\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\t\tdie(\"cannot open '%s' for copying\", path);\n+\t\t\tstrbuf_addf(err, \"cannot open '%s' for copying: %s\",\n+\t\t\t\t path, strerror(errno));\n \t\t\trollback_lock_file(lk);\n-\t\t\terror(\"cannot open '%s' for copying\", path);\n-\t\t\terrno = save_errno;\n \t\t\treturn -1;\n \t\t}\n-\t} else if (copy_fd(orig_fd, fd)) {\n-\t\tint save_errno = errno;\n-\n-\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\texit(128);\n+\t} else if (copy_fd(orig_fd, fd, err)) {\n+\t\tstrbuf_prefixf(err, \"cannot copy '%s': \", path);\n \t\tclose(orig_fd);\n \t\trollback_lock_file(lk);\n-\t\terrno = save_errno;\n \t\treturn -1;\n \t} else {\n \t\tclose(orig_fd);\ndiff --git i/lockfile.h w/lockfile.h\nindex cd2ec95..ca36a1d 100644\n--- i/lockfile.h\n+++ w/lockfile.h\n@@ -75,7 +75,8 @@ extern void unable_to_lock_message(const char *path, int err,\n \t\t\t\t struct strbuf *buf);\n extern NORETURN void unable_to_lock_die(const char *path, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n-extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n+extern int hold_lock_file_for_append(struct lock_file *, const char *path,\n+\t\t\t\t int, struct strbuf *err);\n extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);\n extern char *get_locked_file_path(struct lock_file *);\n extern int commit_lock_file_to(struct lock_file *, const char *path);\ndiff --git i/sha1_file.c w/sha1_file.c\nindex d7f1838..9ae1839 100644\n--- i/sha1_file.c\n+++ w/sha1_file.c\n@@ -403,14 +403,24 @@ void read_info_alternates(const char * relative_base, int depth)\n \n void add_to_alternates_file(const char *reference)\n {\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n-\tint fd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"), LOCK_DIE_ON_ERROR);\n-\tchar *alt = mkpath(\"%s\\n\", reference);\n+\tstruct lock_file *lock;\n+\tint fd;\n+\tchar *alt;\n+\tstruct strbuf err = STRBUF_INIT;\n+\n+\tlock = xcalloc(1, sizeof(*lock));\n+\tfd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"),\n+\t\t\t\t 0, &err);\n+\tif (fd < 0)\n+\t\tdie(\"%s\", err.buf);\n+\talt = mkpath(\"%s\\n\", reference);\n \twrite_or_die(fd, alt, strlen(alt));\n \tif (commit_lock_file(lock))\n \t\tdie(\"could not close alternates file\");\n \tif (alt_odb_tail)\n \t\tlink_alt_odb_entries(alt, strlen(alt), '\\n', NULL, 0);\n+\n+\tstrbuf_release(&err);\n }\n \n int foreach_alt_odb(alt_odb_fn fn, void *cb)\n"},{"id":"252016","messageId":"CAGZ79kYU1f1COjtv+4MzgbPLi42m1JQsXsuuCr3WXsuR8XrO7w@mail.gmail.com","threadId":"37988","inReplyTo":"20141117233525.GC4336@google.com","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-18T00:18:19Z","receivedAt":"2014-11-18T00:18:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 17, 2014 at 3:35 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Stefan Beller wrote:\n>\n>> This patch was sent previously to the list as part of\n>> that series[2], but it seems to be unrelated to me.\n>\n> Thanks. Good call.\n>\n> [...]\n>> From: Ronnie Sahlberg \n>>\n>> Update copy_fd to return a meaningful errno on failure and also\n>> preserve the existing errno variable.\n>\n> Some functions in git make errno meaningful on error and others don't.\n> In general, the more we only use errno immediately after a system\n> call, the better, so based on the above description this seems like a\n> step in the wrong direction.\n\nI did reword the commit message badly. Before it just read\n\"Update copy_fd to return a meaningful errno\".\n\nIn fact the proposed patch doesn't guarantee the errno, which is set\nat the beginning of the function to be the same at the end.\n\nWhat it really should preserve is the errno from xread, while\nevaluating error(\"copy-fd: read returned %s\", strerror(errno));\nSo the function call of error(...) or strerror(...) may change the errno.\n\n>\n> Do any callers care about errno? Does the function's API\n> documentation say it will make errno meaningful on error, so people\n> making changes to copy_fd in the future know to maintain that\n> property?\n>\n> *searches*\n>\n> Looks like callers are:\n>\n> convert.c::filter_buffer_or_fd, which doesn't care\n>\n> copy.c::copy_file, which also doesn't care\n>\n> lockfile.c::hold_lock_file_for_append, which started caring\n> in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno\n> before returning, 2014-10-01). But no callers of that function\n> care yet.\n>\n> So this is about fixing a bug-waiting-to-happen in\n> hold_lock_file_for_append. That would be enough to motivate the\n> change.\n>\n> [...]\n>> copy.c | 15 ++++++++++-----\n>> 1 file changed, 10 insertions(+), 5 deletions(-)\n>\n> Please also update the API documentation in cache.h so we remember not\n> to backslide in the future.\n>\n> [...]\n>> --- a/copy.c\n>> +++ b/copy.c\n>> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)\n>> if (!len)\n>> break;\n>> if (len < 0) {\n>> - return error(\"copy-fd: read returned %s\",\n>> - strerror(errno));\n>> + int save_errno = errno;\n>> + error(\"copy-fd: read returned %s\", strerror(errno));\n>> + errno = save_errno;\n>> + return -1;\n>\n> Any caller is presumably going to turn around and print strerror(errno)\n> again, producing repetitive output.\n>\n> Can we do better? E.g., if the signature were\n>\n> int copy_fd(int ifd, int ofd, struct strbuf *err);\n>\n> then we could write the error message to the err strbuf for the\n> caller to print. The error handling would be more explicit so\n> there would be no need to protect errno from clobbering by other\n> system calls (both here and in callers).\n>\n> Something like this:\n\nI like this approach, though your patch is not about the original\nintention as this one\n(having strerror(...) not messing with the errno), but rather\naccumulating the errors not\nin numbers but string buffers?\n\n>\n> Signed-off-by: Jonathan Nieder \n>\n> diff --git i/cache.h w/cache.h\n> index 99ed096..ddaa30f 100644\n> --- i/cache.h\n> +++ w/cache.h\n> @@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;\n> extern void maybe_flush_or_die(FILE *, const char *);\n> __attribute__((format (printf, 2, 3)))\n> extern void fprintf_or_die(FILE *, const char *fmt, ...);\n> -extern int copy_fd(int ifd, int ofd);\n> +extern int copy_fd(int ifd, int ofd, struct strbuf *err);\n> extern int copy_file(const char *dst, const char *src, int mode);\n> extern int copy_file_with_time(const char *dst, const char *src, int mode);\n> extern void write_or_die(int fd, const void *buf, size_t count);\n> diff --git i/convert.c w/convert.c\n> index 9a5612e..e301447 100644\n> --- i/convert.c\n> +++ w/convert.c\n> @@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n> if (params->src) {\n> write_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n> } else {\n> - write_err = copy_fd(params->fd, child_process.in);\n> + struct strbuf err = STRBUF_INIT;\n> + write_err = copy_fd(params->fd, child_process.in, &err);\n> + if (write_err)\n> + error(\"copy-fd: %s\", err.buf);\n> + strbuf_release(&err);\n> }\n>\n> if (close(child_process.in))\n> diff --git i/copy.c w/copy.c\n> index f2970ec..828661a 100644\n> --- i/copy.c\n> +++ w/copy.c\n> @@ -1,19 +1,22 @@\n> #include \"cache.h\"\n>\n> -int copy_fd(int ifd, int ofd)\n> +int copy_fd(int ifd, int ofd, struct strbuf *err)\n> {\n> + assert(err);\n> +\n> while (1) {\n> char buffer[8192];\n> ssize_t len = xread(ifd, buffer, sizeof(buffer));\n> if (!len)\n> break;\n> if (len < 0) {\n> - return error(\"copy-fd: read returned %s\",\n> - strerror(errno));\n> + strbuf_addf(err, \"read returned %s\", strerror(errno));\n> + return -1;\n> + }\n> + if (write_in_full(ofd, buffer, len) < 0) {\n> + strbuf_addf(err, \"write returned %s\", strerror(errno));\n> + return -1;\n> }\n> - if (write_in_full(ofd, buffer, len) < 0)\n> - return error(\"copy-fd: write returned %s\",\n> - strerror(errno));\n> }\n> return 0;\n> }\n> @@ -33,7 +36,8 @@ static int copy_times(const char *dst, const char *src)\n>\n> int copy_file(const char *dst, const char *src, int mode)\n> {\n> - int fdi, fdo, status;\n> + int fdi, fdo;\n> + struct strbuf err = STRBUF_INIT;\n>\n> mode = (mode & 0111) ? 0777 : 0666;\n> if ((fdi = open(src, O_RDONLY)) < 0)\n> @@ -42,15 +46,21 @@ int copy_file(const char *dst, const char *src, int mode)\n> close(fdi);\n> return fdo;\n> }\n> - status = copy_fd(fdi, fdo);\n> + if (copy_fd(fdi, fdo, &err)) {\n> + close(fdi);\n> + close(fdo);\n> + error(\"copy-fd: %s\", err.buf);\n> + strbuf_release(&err);\n> + return -1;\n> + }\n> + strbuf_release(&err);\n> close(fdi);\n> if (close(fdo) != 0)\n> return error(\"%s: close error: %s\", dst, strerror(errno));\n> -\n> - if (!status && adjust_shared_perm(dst))\n> + if (adjust_shared_perm(dst))\n> return -1;\n>\n> - return status;\n> + return 0;\n> }\n>\n> int copy_file_with_time(const char *dst, const char *src, int mode)\n> diff --git i/lockfile.c w/lockfile.c\n> index 4f16ee7..c47976e 100644\n> --- i/lockfile.c\n> +++ w/lockfile.c\n> @@ -179,37 +179,52 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n> return fd;\n> }\n>\n> -int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n> +/*\n> + * Like strbuf_addf but inserts at the front of a strbuf instead\n> + * of appending.\n> + */\n> +static void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)\n> +{\n> + size_t pos, len;\n> + va_list ap;\n> +\n> + pos = sb->len;\n> +\n> + va_start(ap, fmt);\n> + strbuf_vaddf(sb, fmt, ap);\n> + va_end(ap);\n> +\n> + len = sb->len - pos;\n> + strbuf_insert(sb, 0, sb->buf + pos, len);\n> + strbuf_remove(sb, pos + len, len);\n> +}\n> +\n> +int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n> + int flags, struct strbuf *err)\n> {\n> int fd, orig_fd;\n>\n> + assert(!(flags & LOCK_DIE_ON_ERROR));\n> + assert(err && !err->len);\n> +\n> fd = lock_file(lk, path, flags);\n> if (fd < 0) {\n> - if (flags & LOCK_DIE_ON_ERROR)\n> - unable_to_lock_die(path, errno);\n> + unable_to_lock_message(path, errno, err);\n> return fd;\n> }\n>\n> orig_fd = open(path, O_RDONLY);\n> if (orig_fd < 0) {\n> if (errno != ENOENT) {\n> - int save_errno = errno;\n> -\n> - if (flags & LOCK_DIE_ON_ERROR)\n> - die(\"cannot open '%s' for copying\", path);\n> + strbuf_addf(err, \"cannot open '%s' for copying: %s\",\n> + path, strerror(errno));\n> rollback_lock_file(lk);\n> - error(\"cannot open '%s' for copying\", path);\n> - errno = save_errno;\n> return -1;\n> }\n> - } else if (copy_fd(orig_fd, fd)) {\n> - int save_errno = errno;\n> -\n> - if (flags & LOCK_DIE_ON_ERROR)\n> - exit(128);\n> + } else if (copy_fd(orig_fd, fd, err)) {\n> + strbuf_prefixf(err, \"cannot copy '%s': \", path);\n> close(orig_fd);\n> rollback_lock_file(lk);\n> - errno = save_errno;\n> return -1;\n> } else {\n> close(orig_fd);\n> diff --git i/lockfile.h w/lockfile.h\n> index cd2ec95..ca36a1d 100644\n> --- i/lockfile.h\n> +++ w/lockfile.h\n> @@ -75,7 +75,8 @@ extern void unable_to_lock_message(const char *path, int err,\n> struct strbuf *buf);\n> extern NORETURN void unable_to_lock_die(const char *path, int err);\n> extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n> -extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n> +extern int hold_lock_file_for_append(struct lock_file *, const char *path,\n> + int, struct strbuf *err);\n> extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);\n> extern char *get_locked_file_path(struct lock_file *);\n> extern int commit_lock_file_to(struct lock_file *, const char *path);\n> diff --git i/sha1_file.c w/sha1_file.c\n> index d7f1838..9ae1839 100644\n> --- i/sha1_file.c\n> +++ w/sha1_file.c\n> @@ -403,14 +403,24 @@ void read_info_alternates(const char * relative_base, int depth)\n>\n> void add_to_alternates_file(const char *reference)\n> {\n> - struct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n> - int fd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"), LOCK_DIE_ON_ERROR);\n> - char *alt = mkpath(\"%s\\n\", reference);\n> + struct lock_file *lock;\n> + int fd;\n> + char *alt;\n> + struct strbuf err = STRBUF_INIT;\n> +\n> + lock = xcalloc(1, sizeof(*lock));\n> + fd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"),\n> + 0, &err);\n> + if (fd < 0)\n> + die(\"%s\", err.buf);\n> + alt = mkpath(\"%s\\n\", reference);\n> write_or_die(fd, alt, strlen(alt));\n> if (commit_lock_file(lock))\n> die(\"could not close alternates file\");\n> if (alt_odb_tail)\n> link_alt_odb_entries(alt, strlen(alt), '\\n', NULL, 0);\n> +\n> + strbuf_release(&err);\n> }\n>\n> int foreach_alt_odb(alt_odb_fn fn, void *cb)\n"},{"id":"252019","messageId":"20141118004841.GE4336@google.com","threadId":"37988","inReplyTo":"CAGZ79kYU1f1COjtv+4MzgbPLi42m1JQsXsuuCr3WXsuR8XrO7w@mail.gmail.com","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-18T00:48:41Z","receivedAt":"2014-11-18T00:48:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(meta-comment: please snip out the context you are not responding to,\n to make reading easier)\nStefan Beller wrote:\n> On Mon, Nov 17, 2014 at 3:35 PM, Jonathan Nieder wrote:\n>> Stefan Beller wrote:\n\n>>> Update copy_fd to return a meaningful errno on failure and also\n>>> preserve the existing errno variable.\n>>\n>> Some functions in git make errno meaningful on error and others don't.\n>> In general, the more we only use errno immediately after a system\n>> call, the better, so based on the above description this seems like a\n>> step in the wrong direction.\n>\n> I did reword the commit message badly. Before it just read\n> \"Update copy_fd to return a meaningful errno\".\n>\n> In fact the proposed patch doesn't guarantee the errno, which is set\n> at the beginning of the function to be the same at the end.\n>\n> What it really should preserve is the errno from xread, while\n> evaluating error(\"copy-fd: read returned %s\", strerror(errno));\n> So the function call of error(...) or strerror(...) may change the errno.\n\nA successful call to strerror() is guaranteed not to change errno, but\na call to error() does I/O so it can clobber errno.\n\nThe basic question is whether errno is and should be part of\ncopy_fd()'s contract. Until v2.2.0-rc0~53^2~2 (2014-10-01), it\nwasn't. Even after that change, there's no user-visible effect to\nclobbering errno (so this patch is about maintainability, as opposed\nto fixing a user-visible bad behavior).\n\n[...]\n>> Can we do better? E.g., if the signature were\n>>\n>> int copy_fd(int ifd, int ofd, struct strbuf *err);\n>>\n>> then we could write the error message to the err strbuf for the\n>> caller to print. The error handling would be more explicit so\n>> there would be no need to protect errno from clobbering by other\n>> system calls (both here and in callers).\n>>\n>> Something like this:\n>\n> I like this approach, though your patch is not about the original\n> intention as this one\n> (having strerror(...) not messing with the errno), but rather\n> accumulating the errors not\n> in numbers but string buffers?\n\nAfter this patch, setting errno is not part of the contract of\ncopy_fd, so the bug Ronnie was fixing is gone.\n\nBut it's a little more invasive. What do you think?\n\nThanks,\nJonathan\n"},{"id":"252021","messageId":"CAGZ79kbF6JjxgHX2KZFhSh9QyGOXeS=cVK0z=CM4n9-ErRDJ8A@mail.gmail.com","threadId":"37988","inReplyTo":"20141118004841.GE4336@google.com","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-18T01:01:16Z","receivedAt":"2014-11-18T01:01:16Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, Nov 17, 2014 at 4:48 PM, Jonathan Nieder wrote:\n> (meta-comment: please snip out the context you are not responding to,\n> to make reading easier)\n\nwill do\n\n>\n> After this patch, setting errno is not part of the contract of\n> copy_fd, so the bug Ronnie was fixing is gone.\n>\n> But it's a little more invasive. What do you think?\n>\n\nI really like that approach and would be happy if we'd take it instead\nof the one I sent.\n"},{"id":"252085","messageId":"xmqqtx1wwjtv.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141117233525.GC4336@google.com","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-18T16:32:28Z","receivedAt":"2014-11-18T16:32:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Do any callers care about errno? Does the function's API\n> documentation say it will make errno meaningful on error, so people\n> making changes to copy_fd in the future know to maintain that\n> property?\n>\n> *searches*\n>\n> Looks like callers are:\n>\n> convert.c::filter_buffer_or_fd, which doesn't care\n>\n> copy.c::copy_file, which also doesn't care\n>\n> lockfile.c::hold_lock_file_for_append, which started caring\n> in order to propagate errno in v2.2.0-rc0~53^2~2 (restore errno\n> before returning, 2014-10-01). But no callers of that function\n> care yet.\n>\n> So this is about fixing a bug-waiting-to-happen in\n> hold_lock_file_for_append. That would be enough to motivate the\n> change.\n\nOK. Perhaps convert.c wants to be fixed?\n\n>> +\t\t\tint save_errno = errno;\n>> +\t\t\terror(\"copy-fd: read returned %s\", strerror(errno));\n>> +\t\t\terrno = save_errno;\n>> +\t\t\treturn -1;\n>\n> Any caller is presumably going to turn around and print strerror(errno)\n> again, producing repetitive output.\n>\n> Can we do better? E.g., if the signature were\n>\n> \tint copy_fd(int ifd, int ofd, struct strbuf *err);\n>\n> then we could write the error message to the err strbuf for the\n> caller to print.\n\nThe problem you are solving is \"because the caller may want to do\nits own error message, stop the callee from emitting the error\nmessage unconditionally\", but if we are addressing \"the caller may\nwant to...\", I think we should find a single solution that addresses\nother kind fo things the caller may want to do.\n\nFor example, two callers may want to phrase the same error condition\ndifferently, e.g. depending on what the user asked to do. We'd want\nsomething better than the ERRORMSG trick used in unpack-trees.c,\nwhich does not scale, and I think passing some data that represents\n\"here is how the caller wants the errors to be handled and\npresented\" is probably a part of the solution, but strbuf *err is\nnot that.\n\nIn short I am not a very big fan of passing around strbuf *err to\nlow level helper API functions myself.\n\nBut the approach does not make things much worse than it currently\nis, other than code churns to pass an extra pointer around.\n"},{"id":"252087","messageId":"xmqqk32swi5r.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"xmqqtx1wwjtv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-18T17:08:32Z","receivedAt":"2014-11-18T17:08:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> In short I am not a very big fan of passing around strbuf *err to\n> low level helper API functions myself.\n>\n> But the approach does not make things much worse than it currently\n> is, other than code churns to pass an extra pointer around.\n\nSorry I left the conclusion out of the message.\n\nAs it does not make things much worse, and does give slightly better\nflexibility on error message emission to the callers, let's go with\nthe \"strbuf *err\" arpporach for now.\n\nUntil we hit a wall we cannot climb over, at which time we may need\nto redo it, but let's first see how it goes.\n"},{"id":"252323","messageId":"546F0284.7050904@alum.mit.edu","threadId":"37988","inReplyTo":"1416262453-30349-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-21T09:14:44Z","receivedAt":"2014-11-21T09:14:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/17/2014 11:14 PM, Stefan Beller wrote:\n> From: Ronnie Sahlberg \n> \n> Update copy_fd to return a meaningful errno on failure and also \n> preserve the existing errno variable.\n> \n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Jonathan Nieder \n> Signed-off-by: Stefan Beller \n> ---\n> As announced in [1], I'm going to take over the \n> ref-transactions-reflog series by Ronnie Sahlberg.\n> \n> This patch was sent previously to the list as part of\n> that series[2], but it seems to be unrelated to me.\n> \n> Thanks,\n> Stefan\n> \n> [1] http://www.mail-archive.com/git@vger.kernel.org/msg61051.html\n> [2] http://www.spinics.net/lists/git/msg240784.html\n> copy.c | 15 ++++++++++-----\n> 1 file changed, 10 insertions(+), 5 deletions(-)\n> \n> diff --git a/copy.c b/copy.c\n> index f2970ec..a8d366e 100644\n> --- a/copy.c\n> +++ b/copy.c\n> @@ -8,12 +8,17 @@ int copy_fd(int ifd, int ofd)\n> \t\tif (!len)\n> \t\t\tbreak;\n> \t\tif (len < 0) {\n> -\t\t\treturn error(\"copy-fd: read returned %s\",\n> -\t\t\t\t strerror(errno));\n> +\t\t\tint save_errno = errno;\n> +\t\t\terror(\"copy-fd: read returned %s\", strerror(errno));\n> +\t\t\terrno = save_errno;\n> +\t\t\treturn -1;\n> +\t\t}\n> +\t\tif (write_in_full(ofd, buffer, len) < 0) {\n> +\t\t\tint save_errno = errno;\n> +\t\t\terror(\"copy-fd: write returned %s\", strerror(errno));\n> +\t\t\terrno = save_errno;\n> +\t\t\treturn -1;\n> \t\t}\n> -\t\tif (write_in_full(ofd, buffer, len) < 0)\n> -\t\t\treturn error(\"copy-fd: write returned %s\",\n> -\t\t\t\t strerror(errno));\n> \t}\n> \treturn 0;\n> }\n> \n\nCouldn't we save ourselves a lot of this \"save_errno\" boilerplate by\nmaking error() and warning() preserve errno? They don't do any\nmeaningful internal error checking anyway, so even if they overwrite\nerrno, that value is useless to callers (who undoubtedly wouldn't check\nsuch a value anyway). Something like\n\n int error(const char *err, ...)\n {\n \tva_list params;\n+\tint save_errno = errno;\n\n \tva_start(params, err);\n \terror_routine(err, params);\n \tva_end(params);\n+\terrno = save_errno;\n \treturn -1;\n }\n\nand the same for warning()?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"252324","messageId":"546F033F.7030201@alum.mit.edu","threadId":"37988","inReplyTo":"546F0284.7050904@alum.mit.edu","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-11-21T09:17:51Z","receivedAt":"2014-11-21T09:17:51Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 11/21/2014 10:14 AM, Michael Haggerty wrote:\n> Couldn't we save ourselves a lot of this \"save_errno\" boilerplate by\n> making error() and warning() preserve errno? [...]\n\nNever mind; I see that Peff already submitted a patch to this effect.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"252350","messageId":"xmqqa93k1m3g.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"546F033F.7030201@alum.mit.edu","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-21T17:48:19Z","receivedAt":"2014-11-21T17:48:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 11/21/2014 10:14 AM, Michael Haggerty wrote:\n>> Couldn't we save ourselves a lot of this \"save_errno\" boilerplate by\n>> making error() and warning() preserve errno? [...]\n>\n> Never mind; I see that Peff already submitted a patch to this effect.\n\nMy understanding of the conclusion of those four patches was that\nonly a single updated one is needed, and \"moving save/restore inside\nerror()\" did not have to survive.\n\n http://article.gmane.org/gmane.comp.version-control.git/259911\n"},{"id":"252351","messageId":"20141121175446.GA26650@peff.net","threadId":"37988","inReplyTo":"xmqqa93k1m3g.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-11-21T17:54:47Z","receivedAt":"2014-11-21T17:54:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 21, 2014 at 09:48:19AM -0800, Junio C Hamano wrote:\n\n> Michael Haggerty writes:\n> \n> > On 11/21/2014 10:14 AM, Michael Haggerty wrote:\n> >> Couldn't we save ourselves a lot of this \"save_errno\" boilerplate by\n> >> making error() and warning() preserve errno? [...]\n> >\n> > Never mind; I see that Peff already submitted a patch to this effect.\n> \n> My understanding of the conclusion of those four patches was that\n> only a single updated one is needed, and \"moving save/restore inside\n> error()\" did not have to survive.\n> \n> http://article.gmane.org/gmane.comp.version-control.git/259911\n\nYeah, the callsite I had intended ended up needing to save it across\nmore than just error(). And I think that is probably why we have never\ndone any errno-handling inside error() before (it is certainly not the\nfirst time I thought of doing such a thing): real-world cases tend to be\na little more complicated.\n\nThat being said, if this copy() case is one that could benefit, I do not\nhave any problem with picking up (or just reusing the concept of) my\n1/4 from that series. It probably does not _hurt_ anything, and if it\ncan help in even a few cases, it may be worth it.\n\n-Peff\n"},{"id":"252356","messageId":"xmqq1tow1k3v.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141121175446.GA26650@peff.net","subject":"Re: [PATCH] copy.c: make copy_fd preserve meaningful errno","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-21T18:31:16Z","receivedAt":"2014-11-21T18:31:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Nov 21, 2014 at 09:48:19AM -0800, Junio C Hamano wrote:\n>\n>> Michael Haggerty writes:\n>> \n>> > On 11/21/2014 10:14 AM, Michael Haggerty wrote:\n>> >> Couldn't we save ourselves a lot of this \"save_errno\" boilerplate by\n>> >> making error() and warning() preserve errno? [...]\n>> >\n>> > Never mind; I see that Peff already submitted a patch to this effect.\n>> \n>> My understanding of the conclusion of those four patches was that\n>> only a single updated one is needed, and \"moving save/restore inside\n>> error()\" did not have to survive.\n>> \n>> http://article.gmane.org/gmane.comp.version-control.git/259911\n>\n> Yeah, the callsite I had intended ended up needing to save it across\n> more than just error(). And I think that is probably why we have never\n> done any errno-handling inside error() before (it is certainly not the\n> first time I thought of doing such a thing): real-world cases tend to be\n> a little more complicated.\n>\n> That being said, if this copy() case is one that could benefit, I do not\n> have any problem with picking up (or just reusing the concept of) my\n> 1/4 from that series. It probably does not _hurt_ anything, and if it\n> can help in even a few cases, it may be worth it.\n\nYeah, I agree. My summary was \"did not have to survive\", not \"had\nto die\". Like you, I do not mind the change as long as other code\npaths benefit from it.\n"},{"id":"252920","messageId":"20141203050217.GJ6527@google.com","threadId":"37988","inReplyTo":"CAGZ79kbF6JjxgHX2KZFhSh9QyGOXeS=cVK0z=CM4n9-ErRDJ8A@mail.gmail.com","subject":"[PATCH 0/14] Re: copy.c: make copy_fd preserve meaningful errno","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:02:17Z","receivedAt":"2014-12-03T05:02:17Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Mon, Nov 17, 2014 at 4:48 PM, Jonathan Nieder wrote:\n\n>> After this patch, setting errno is not part of the contract of\n>> copy_fd, so the bug Ronnie was fixing is gone.\n>>\n>> But it's a little more invasive. What do you think?\n>\n> I really like that approach and would be happy if\n\nThanks for looking it over, and sorry for the delay. Here's a series\nthat carries out that approach.\n\nThe patch I'm least happy with in this series is 12/14, mostly because\nit's just so noisy. It would be safe (and non-leaky) to leave out\nmost of those strbuf_release calls since nobody appends to 'err' in\nthe non-error case, but always free-ing means that normal escape\nanalysis can work. I could be convinced to go either way.\n\nJonathan Nieder (14):\n strbuf: introduce strbuf_prefixf()\n add_to_alternates_file: respect GIT_OBJECT_DIRECTORY\n copy_fd: pass error message back through a strbuf\n hold_lock_file_for_append: pass error message back through a strbuf\n lock_packed_refs: pass error message back through a strbuf\n lockfile: introduce flag for locks outside .git\n fast-import: use message from lockfile API when writing marks fails\n credentials: use message from lockfile API when locking\n ~/.git-credentials fails\n config: use message from lockfile API when locking config file fails\n rerere: error out on autoupdate failure\n hold_locked_index: pass error message back through a strbuf\n hold_lock_file_for_update: pass error message back through a strbuf\n lockfile: remove unused function 'unable_to_lock_die'\n lockfile: make 'unable_to_lock_message' private\n\n Documentation/technical/api-lockfile.txt | 41 ++++++---------\n builtin/add.c | 12 +++--\n builtin/apply.c | 15 ++++--\n builtin/checkout-index.c | 10 +++-\n builtin/checkout.c | 55 ++++++++++++++------\n builtin/clone.c | 12 ++++-\n builtin/commit.c | 36 +++++++++----\n builtin/describe.c | 11 ++--\n builtin/diff.c | 12 +++--\n builtin/gc.c | 8 ++-\n builtin/merge.c | 14 +++--\n builtin/mv.c | 5 +-\n builtin/read-tree.c | 9 +++-\n builtin/reset.c | 10 +++-\n builtin/rm.c | 9 +++-\n builtin/update-index.c | 10 ++--\n bundle.c | 10 ++--\n cache-tree.c | 18 +++++--\n cache.h | 4 +-\n config.c | 14 +++--\n convert.c | 6 ++-\n copy.c | 32 ++++++++----\n credential-store.c | 8 ++-\n fast-import.c | 9 ++--\n lockfile.c | 89 ++++++++++++++------------------\n lockfile.h | 13 +++--\n merge-recursive.c | 13 +++--\n merge.c | 17 ++++--\n read-cache.c | 7 +--\n refs.c | 28 ++++++----\n refs.h | 8 +--\n rerere.c | 24 +++++----\n sequencer.c | 33 +++++++++---\n sha1_file.c | 17 ++++--\n shallow.c | 16 ++++--\n strbuf.c | 16 ++++++\n strbuf.h | 4 ++\n test-scrap-cache-tree.c | 5 +-\n 38 files changed, 434 insertions(+), 226 deletions(-)\n"},{"id":"252921","messageId":"20141203051016.GK6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 01/14] strbuf: introduce strbuf_prefixf()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:10:16Z","receivedAt":"2014-12-03T05:10:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When preparing an error message in a strbuf, it can be convenient\nto add a formatted string to the beginning:\n\n\tif (transaction_commit(&t, err)) {\n\t\tstrbuf_prefixf(err, \"cannot fetch '%s': \", remotename);\n\t\treturn -1;\n\t}\n\nThe new strbuf_prefixf is like strbuf_addf, except it writes its\nresult to the beginning of a strbuf instead of the end.\n\nThe current implementation uses strlen(strfmt(fmt, ...)) extra bytes\nat the end of the strbuf as temporary scratch space for convenience\nand simplicity. A later patch can optimize if needed.\n\nSigned-off-by: Jonathan Nieder \n---\n strbuf.c | 16 ++++++++++++++++\n strbuf.h | 4 ++++\n 2 files changed, 20 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 0346e74..3f4aaa3 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -219,6 +219,22 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n \tva_end(ap);\n }\n \n+void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)\n+{\n+\tva_list ap;\n+\tsize_t pos, len;\n+\n+\tpos = sb->len;\n+\n+\tva_start(ap, fmt);\n+\tstrbuf_vaddf(sb, fmt, ap);\n+\tva_end(ap);\n+\n+\tlen = sb->len - pos;\n+\tstrbuf_insert(sb, 0, sb->buf + pos, len);\n+\tstrbuf_remove(sb, pos + len, len);\n+}\n+\n static void add_lines(struct strbuf *out,\n \t\t\tconst char *prefix1,\n \t\t\tconst char *prefix2,\ndiff --git a/strbuf.h b/strbuf.h\nindex 652b6c4..5dae48e 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -158,6 +158,10 @@ extern void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap);\n \n extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *buf, size_t size);\n \n+/* Like strbuf_addf but insert at the front of sb instead of appending. */\n+__attribute__((format (printf,2,3)))\n+extern void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...);\n+\n /*\n * Append s to sb, with the characters '<', '>', '&' and '\"' converted\n * into XML entities.\n"},{"id":"252922","messageId":"20141203051220.GL6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 02/14] add_to_alternates_file: respect GIT_OBJECT_DIRECTORY","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:12:20Z","receivedAt":"2014-12-03T05:12:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The objects directory is spelled as get_object_directory(), not\ngit_path(\"objects\"). Some other code still hard-codes the objects/\ndirectory name, so in the long term we may want to get rid of the\npretense of support for GIT_OBJECT_DIRECTORY altogether, but this\nmakes the code more consistent for now.\n\nWhile at it, split variable declarations from the rest of the\nfunction. This makes the function a little easier to read, at the\ncost of some vertical space.\n\nSigned-off-by: Jonathan Nieder \n---\nThis is mainly for consistency / cleanliness. I wouldn't mind\ndropping it.\n\nThe rest of 'git clone' is not careful about paying attention to\nGIT_OBJECT_DIRECTORY either. I suspect GIT_OBJECT_DIRECTORY was a bit\nof a failed experiment.\n\n sha1_file.c | 12 +++++++++---\n 1 file changed, 9 insertions(+), 3 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex d7f1838..e1945e2 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -403,9 +403,15 @@ void read_info_alternates(const char * relative_base, int depth)\n \n void add_to_alternates_file(const char *reference)\n {\n-\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n-\tint fd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"), LOCK_DIE_ON_ERROR);\n-\tchar *alt = mkpath(\"%s\\n\", reference);\n+\tstruct lock_file *lock;\n+\tint fd;\n+\tchar *alt;\n+\n+\tlock = xcalloc(1, sizeof(*lock));\n+\tfd = hold_lock_file_for_append(lock, mkpath(\"%s/info/alternates\",\n+\t\t\t\t\t\t get_object_directory()),\n+\t\t\t\t LOCK_DIE_ON_ERROR);\n+\talt = mkpath(\"%s\\n\", reference);\n \twrite_or_die(fd, alt, strlen(alt));\n \tif (commit_lock_file(lock))\n \t\tdie(\"could not close alternates file\");\n"},{"id":"252923","messageId":"20141203051344.GM6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:13:44Z","receivedAt":"2014-12-03T05:13:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"This way, callers can put the message in context or even avoid\nprinting the message altogether.\n\nCurrently hold_lock_file_for_append tries to save errno in order to\nproduce a meaningful message about the failure and tries to print a\nsecond message using errno. Unfortunately the errno it uses is not\nmeaningful because copy_fd makes no effort to set a meaningful errno\nvalue. This change is preparation for simplifying the\nhold_lock_file_for_append behavior.\n\nNo user-visible change intended yet.\n\nSigned-off-by: Jonathan Nieder \n---\nThe title feature.\n\n cache.h | 2 +-\n convert.c | 6 +++++-\n copy.c | 32 +++++++++++++++++++++-----------\n lockfile.c | 6 +++++-\n 4 files changed, 32 insertions(+), 14 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 99ed096..ddaa30f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1479,7 +1479,7 @@ extern const char *git_mailmap_blob;\n extern void maybe_flush_or_die(FILE *, const char *);\n __attribute__((format (printf, 2, 3)))\n extern void fprintf_or_die(FILE *, const char *fmt, ...);\n-extern int copy_fd(int ifd, int ofd);\n+extern int copy_fd(int ifd, int ofd, struct strbuf *err);\n extern int copy_file(const char *dst, const char *src, int mode);\n extern int copy_file_with_time(const char *dst, const char *src, int mode);\n extern void write_or_die(int fd, const void *buf, size_t count);\ndiff --git a/convert.c b/convert.c\nindex 9a5612e..e301447 100644\n--- a/convert.c\n+++ b/convert.c\n@@ -358,7 +358,11 @@ static int filter_buffer_or_fd(int in, int out, void *data)\n \tif (params->src) {\n \t\twrite_err = (write_in_full(child_process.in, params->src, params->size) < 0);\n \t} else {\n-\t\twrite_err = copy_fd(params->fd, child_process.in);\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\twrite_err = copy_fd(params->fd, child_process.in, &err);\n+\t\tif (write_err)\n+\t\t\terror(\"copy-fd: %s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t}\n \n \tif (close(child_process.in))\ndiff --git a/copy.c b/copy.c\nindex f2970ec..828661a 100644\n--- a/copy.c\n+++ b/copy.c\n@@ -1,19 +1,22 @@\n #include \"cache.h\"\n \n-int copy_fd(int ifd, int ofd)\n+int copy_fd(int ifd, int ofd, struct strbuf *err)\n {\n+\tassert(err);\n+\n \twhile (1) {\n \t\tchar buffer[8192];\n \t\tssize_t len = xread(ifd, buffer, sizeof(buffer));\n \t\tif (!len)\n \t\t\tbreak;\n \t\tif (len < 0) {\n-\t\t\treturn error(\"copy-fd: read returned %s\",\n-\t\t\t\t strerror(errno));\n+\t\t\tstrbuf_addf(err, \"read returned %s\", strerror(errno));\n+\t\t\treturn -1;\n+\t\t}\n+\t\tif (write_in_full(ofd, buffer, len) < 0) {\n+\t\t\tstrbuf_addf(err, \"write returned %s\", strerror(errno));\n+\t\t\treturn -1;\n \t\t}\n-\t\tif (write_in_full(ofd, buffer, len) < 0)\n-\t\t\treturn error(\"copy-fd: write returned %s\",\n-\t\t\t\t strerror(errno));\n \t}\n \treturn 0;\n }\n@@ -33,7 +36,8 @@ static int copy_times(const char *dst, const char *src)\n \n int copy_file(const char *dst, const char *src, int mode)\n {\n-\tint fdi, fdo, status;\n+\tint fdi, fdo;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tmode = (mode & 0111) ? 0777 : 0666;\n \tif ((fdi = open(src, O_RDONLY)) < 0)\n@@ -42,15 +46,21 @@ int copy_file(const char *dst, const char *src, int mode)\n \t\tclose(fdi);\n \t\treturn fdo;\n \t}\n-\tstatus = copy_fd(fdi, fdo);\n+\tif (copy_fd(fdi, fdo, &err)) {\n+\t\tclose(fdi);\n+\t\tclose(fdo);\n+\t\terror(\"copy-fd: %s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t\treturn -1;\n+\t}\n+\tstrbuf_release(&err);\n \tclose(fdi);\n \tif (close(fdo) != 0)\n \t\treturn error(\"%s: close error: %s\", dst, strerror(errno));\n-\n-\tif (!status && adjust_shared_perm(dst))\n+\tif (adjust_shared_perm(dst))\n \t\treturn -1;\n \n-\treturn status;\n+\treturn 0;\n }\n \n int copy_file_with_time(const char *dst, const char *src, int mode)\ndiff --git a/lockfile.c b/lockfile.c\nindex 4f16ee7..b3020f3 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -182,6 +182,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n {\n \tint fd, orig_fd;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tfd = lock_file(lk, path, flags);\n \tif (fd < 0) {\n@@ -202,9 +203,11 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \t\t\terrno = save_errno;\n \t\t\treturn -1;\n \t\t}\n-\t} else if (copy_fd(orig_fd, fd)) {\n+\t} else if (copy_fd(orig_fd, fd, &err)) {\n \t\tint save_errno = errno;\n \n+\t\terror(\"copy-fd: %s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t\tif (flags & LOCK_DIE_ON_ERROR)\n \t\t\texit(128);\n \t\tclose(orig_fd);\n@@ -214,6 +217,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n \t} else {\n \t\tclose(orig_fd);\n \t}\n+\tstrbuf_release(&err);\n \treturn fd;\n }\n \n"},{"id":"252924","messageId":"20141203051459.GN6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 04/14] hold_lock_file_for_append: pass error message back through a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:14:59Z","receivedAt":"2014-12-03T05:14:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"This way, the code does not need to carefully safeguard errno to allow\ncallers to print a reasonable error message when they choose to do\nsome cleanup before die()-ing.\n\nFixes a bug waiting to happen where copy_fd would clobber the errno\npassed back via hold_lock_file_for_append from read() or write() when\nflags did not contain LOCK_DIE_ON_ERROR. Luckily the only caller uses\nflags == LOCK_DIE_ON_ERROR, avoiding that bug in practice.\n\nReported-by: Ronnie Sahlberg \nSigned-off-by: Jonathan Nieder \n---\nThe advertised bugfix.\n\n lockfile.c | 29 ++++++++++-------------------\n lockfile.h | 3 ++-\n sha1_file.c | 7 ++++++-\n 3 files changed, 18 insertions(+), 21 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex b3020f3..8685c68 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -179,45 +179,36 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n \treturn fd;\n }\n \n-int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n+int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n+\t\t\t int flags, struct strbuf *err)\n {\n \tint fd, orig_fd;\n-\tstruct strbuf err = STRBUF_INIT;\n+\n+\tassert(!(flags & LOCK_DIE_ON_ERROR));\n+\tassert(err && !err->len);\n \n \tfd = lock_file(lk, path, flags);\n \tif (fd < 0) {\n-\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\tunable_to_lock_die(path, errno);\n+\t\tunable_to_lock_message(path, errno, err);\n \t\treturn fd;\n \t}\n \n \torig_fd = open(path, O_RDONLY);\n \tif (orig_fd < 0) {\n \t\tif (errno != ENOENT) {\n-\t\t\tint save_errno = errno;\n-\n-\t\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\t\tdie(\"cannot open '%s' for copying\", path);\n+\t\t\tstrbuf_addf(err, \"cannot open '%s' for copying: %s\",\n+\t\t\t\t path, strerror(errno));\n \t\t\trollback_lock_file(lk);\n-\t\t\terror(\"cannot open '%s' for copying\", path);\n-\t\t\terrno = save_errno;\n \t\t\treturn -1;\n \t\t}\n-\t} else if (copy_fd(orig_fd, fd, &err)) {\n-\t\tint save_errno = errno;\n-\n-\t\terror(\"copy-fd: %s\", err.buf);\n-\t\tstrbuf_release(&err);\n-\t\tif (flags & LOCK_DIE_ON_ERROR)\n-\t\t\texit(128);\n+\t} else if (copy_fd(orig_fd, fd, err)) {\n+\t\tstrbuf_prefixf(err, \"cannot copy '%s': \", path);\n \t\tclose(orig_fd);\n \t\trollback_lock_file(lk);\n-\t\terrno = save_errno;\n \t\treturn -1;\n \t} else {\n \t\tclose(orig_fd);\n \t}\n-\tstrbuf_release(&err);\n \treturn fd;\n }\n \ndiff --git a/lockfile.h b/lockfile.h\nindex cd2ec95..ca36a1d 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -75,7 +75,8 @@ extern void unable_to_lock_message(const char *path, int err,\n \t\t\t\t struct strbuf *buf);\n extern NORETURN void unable_to_lock_die(const char *path, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n-extern int hold_lock_file_for_append(struct lock_file *, const char *path, int);\n+extern int hold_lock_file_for_append(struct lock_file *, const char *path,\n+\t\t\t\t int, struct strbuf *err);\n extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);\n extern char *get_locked_file_path(struct lock_file *);\n extern int commit_lock_file_to(struct lock_file *, const char *path);\ndiff --git a/sha1_file.c b/sha1_file.c\nindex e1945e2..6c0ab3b 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -406,17 +406,22 @@ void add_to_alternates_file(const char *reference)\n \tstruct lock_file *lock;\n \tint fd;\n \tchar *alt;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tlock = xcalloc(1, sizeof(*lock));\n \tfd = hold_lock_file_for_append(lock, mkpath(\"%s/info/alternates\",\n \t\t\t\t\t\t get_object_directory()),\n-\t\t\t\t LOCK_DIE_ON_ERROR);\n+\t\t\t\t 0, &err);\n+\tif (fd < 0)\n+\t\tdie(\"%s\", err.buf);\n \talt = mkpath(\"%s\\n\", reference);\n \twrite_or_die(fd, alt, strlen(alt));\n \tif (commit_lock_file(lock))\n \t\tdie(\"could not close alternates file\");\n \tif (alt_odb_tail)\n \t\tlink_alt_odb_entries(alt, strlen(alt), '\\n', NULL, 0);\n+\n+\tstrbuf_release(&err);\n }\n \n int foreach_alt_odb(alt_odb_fn fn, void *cb)\n"},{"id":"252925","messageId":"20141203051615.GO6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 05/14] lock_packed_refs: pass error message back through a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:16:15Z","receivedAt":"2014-12-03T05:16:15Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"This saves us from having to be careful about preserving errno and\nmakes it more explicit in the die-on-error case that the caller is\nexiting without a chance to clean up.\n\nSigned-off-by: Jonathan Nieder \n---\n builtin/clone.c | 6 +++++-\n refs.c | 17 ++++++++++-------\n refs.h | 8 ++------\n 3 files changed, 17 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex d5e7532..b07d740 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -495,8 +495,10 @@ static struct ref *wanted_peer_refs(const struct ref *refs,\n static void write_remote_refs(const struct ref *local_refs)\n {\n \tconst struct ref *r;\n+\tstruct strbuf err = STRBUF_INIT;\n \n-\tlock_packed_refs(LOCK_DIE_ON_ERROR);\n+\tif (lock_packed_refs(&err))\n+\t\tdie(\"%s\", err.buf);\n \n \tfor (r = local_refs; r; r = r->next) {\n \t\tif (!r->peer_ref)\n@@ -506,6 +508,8 @@ static void write_remote_refs(const struct ref *local_refs)\n \n \tif (commit_packed_refs())\n \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n+\n+\tstrbuf_release(&err);\n }\n \n static void write_followtags(const struct ref *refs, const char *msg)\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..917f8fc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2371,13 +2371,15 @@ static int write_packed_entry_fn(struct ref_entry *entry, void *cb_data)\n \treturn 0;\n }\n \n-/* This should return a meaningful errno on failure */\n-int lock_packed_refs(int flags)\n+int lock_packed_refs(struct strbuf *err)\n {\n \tstruct packed_ref_cache *packed_ref_cache;\n \n-\tif (hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), flags) < 0)\n+\tif (hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), 0) < 0) {\n+\t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n \t\treturn -1;\n+\t}\n+\n \t/*\n \t * Get the current packed-refs while holding the lock. If the\n \t * packed-refs file has been modified since we last read it,\n@@ -2565,11 +2567,13 @@ static void prune_refs(struct ref_to_prune *r)\n int pack_refs(unsigned int flags)\n {\n \tstruct pack_refs_cb_data cbdata;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tmemset(&cbdata, 0, sizeof(cbdata));\n \tcbdata.flags = flags;\n \n-\tlock_packed_refs(LOCK_DIE_ON_ERROR);\n+\tif (lock_packed_refs(&err))\n+\t\tdie(\"%s\", err.buf);\n \tcbdata.packed_refs = get_packed_refs(&ref_cache);\n \n \tdo_for_each_entry_in_dir(get_loose_refs(&ref_cache), 0,\n@@ -2579,6 +2583,7 @@ int pack_refs(unsigned int flags)\n \t\tdie_errno(\"unable to overwrite old ref-pack file\");\n \n \tprune_refs(cbdata.ref_to_prune);\n+\tstrbuf_release(&err);\n \treturn 0;\n }\n \n@@ -2657,10 +2662,8 @@ int repack_without_refs(const char **refnames, int n, struct strbuf *err)\n \tif (i == n)\n \t\treturn 0; /* no refname exists in packed refs */\n \n-\tif (lock_packed_refs(0)) {\n-\t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n+\tif (lock_packed_refs(err))\n \t\treturn -1;\n-\t}\n \tpacked = get_packed_refs(&ref_cache);\n \n \t/* Remove refnames from the cache */\ndiff --git a/refs.h b/refs.h\nindex 2bc3556..ca6a567 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -119,12 +119,8 @@ extern int for_each_rawref(each_ref_fn, void *);\n extern void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname);\n extern void warn_dangling_symrefs(FILE *fp, const char *msg_fmt, const struct string_list *refnames);\n \n-/*\n- * Lock the packed-refs file for writing. Flags is passed to\n- * hold_lock_file_for_update(). Return 0 on success.\n- * Errno is set to something meaningful on error.\n- */\n-extern int lock_packed_refs(int flags);\n+/* Lock the packed-refs file for writing. Returns 0 on success. */\n+extern int lock_packed_refs(struct strbuf *err);\n \n /*\n * Add a reference to the in-memory packed reference cache. This may\n-- \n2.2.0.rc0.207.ga3a616c\n"},{"id":"252926","messageId":"20141203051911.GP6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 06/14] lockfile: introduce flag for locks outside .git","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:19:12Z","receivedAt":"2014-12-03T05:19:12Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"When the lockfile API was introduced, it was only used for the index\nfile and error messages like\n\n fatal: Unable to create '/path/to/foo.lock': File exists.\n\n If no other git process is currently running, this probably means a\n git process crashed in this repository earlier. Make sure no other git\n process is running and remove the file manually to continue.\n\nwere appropriate advice for that case.\n\nNowadays, the lockfile API is used for other files that need similar\nlock-then-update semantics, including files not associated to any\nrepository, such as /etc/gitconfig, ../my.bundle, and /tmp/temp.marks.\nAdd a flag that makes the message stop referring to \"this repository\"\nfor such cases.\n\nThis should make it possible to print nicer error messages from\nsuch non-core users of the lockfile API.\n\nThis introduces the flag. Later patches will use it.\n\nSigned-off-by: Jonathan Nieder \n---\n\n Documentation/technical/api-lockfile.txt | 12 ++++++++++++\n builtin/update-index.c | 2 +-\n lockfile.c | 26 +++++++++++++++++---------\n lockfile.h | 5 +++--\n refs.c | 4 ++--\n 5 files changed, 35 insertions(+), 14 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex 93b5f23..fa60cfd 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -124,6 +124,18 @@ LOCK_NO_DEREF::\n \tfor backwards-compatibility reasons can be a symbolic link\n \tcontaining the name of the referred-to-reference.\n \n+LOCK_OUTSIDE_REPOSITORY:\n+\n+\tWhen the \".lock\" file being created already exists, the error\n+\tmessage usually explains that another git process must have\n+\tcrashed in the same Git repository. If `LOCK_OUTSIDE_REPOSITORY`\n+\tis set, then the message is replaced with something more\n+\tappropriate when updating files that are not inside a\n+\trepository.\n++\n+For example, this flag should be set when locking a configuration\n+file in the user's home directory.\n+\n LOCK_DIE_ON_ERROR::\n \n \tIf a lock is already taken for the file, `die()` with an error\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex b0e3dc9..56abd18 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -943,7 +943,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tif (newfd < 0) {\n \t\t\tif (refresh_args.flags & REFRESH_QUIET)\n \t\t\t\texit(128);\n-\t\t\tunable_to_lock_die(get_index_file(), lock_error);\n+\t\t\tunable_to_lock_die(get_index_file(), 0, lock_error);\n \t\t}\n \t\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n \t\t\tdie(\"Unable to write new index file\");\ndiff --git a/lockfile.c b/lockfile.c\nindex 8685c68..102584f 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -149,24 +149,32 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \treturn lk->fd;\n }\n \n-void unable_to_lock_message(const char *path, int err, struct strbuf *buf)\n+void unable_to_lock_message(const char *path, int flags, int err,\n+\t\t\t struct strbuf *buf)\n {\n-\tif (err == EEXIST) {\n+\tif (err != EEXIST) {\n+\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s\",\n+\t\t\t absolute_path(path), strerror(err));\n+\t} else if (flags & LOCK_OUTSIDE_REPOSITORY) {\n+\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n+\t\t \"If no other git process is currently running, this probably means\\n\"\n+\t\t \"another git process crashed earlier. Make sure no other git process\\n\"\n+\t\t \"is running and remove the file manually to continue.\",\n+\t\t\t absolute_path(path), strerror(err));\n+\t} else {\n \t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n \t\t \"If no other git process is currently running, this probably means a\\n\"\n \t\t \"git process crashed in this repository earlier. Make sure no other git\\n\"\n \t\t \"process is running and remove the file manually to continue.\",\n \t\t\t absolute_path(path), strerror(err));\n-\t} else\n-\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s\",\n-\t\t\t absolute_path(path), strerror(err));\n+\t}\n }\n \n-NORETURN void unable_to_lock_die(const char *path, int err)\n+NORETURN void unable_to_lock_die(const char *path, int flags, int err)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n \n-\tunable_to_lock_message(path, err, &buf);\n+\tunable_to_lock_message(path, flags, err, &buf);\n \tdie(\"%s\", buf.buf);\n }\n \n@@ -175,7 +183,7 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n {\n \tint fd = lock_file(lk, path, flags);\n \tif (fd < 0 && (flags & LOCK_DIE_ON_ERROR))\n-\t\tunable_to_lock_die(path, errno);\n+\t\tunable_to_lock_die(path, flags, errno);\n \treturn fd;\n }\n \n@@ -189,7 +197,7 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n \n \tfd = lock_file(lk, path, flags);\n \tif (fd < 0) {\n-\t\tunable_to_lock_message(path, errno, err);\n+\t\tunable_to_lock_message(path, flags, errno, err);\n \t\treturn fd;\n \t}\n \ndiff --git a/lockfile.h b/lockfile.h\nindex ca36a1d..779a992 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -70,10 +70,11 @@ struct lock_file {\n \n #define LOCK_DIE_ON_ERROR 1\n #define LOCK_NO_DEREF 2\n+#define LOCK_OUTSIDE_REPOSITORY 4\n \n-extern void unable_to_lock_message(const char *path, int err,\n+extern void unable_to_lock_message(const char *path, int, int err,\n \t\t\t\t struct strbuf *buf);\n-extern NORETURN void unable_to_lock_die(const char *path, int err);\n+extern NORETURN void unable_to_lock_die(const char *path, int, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path,\n \t\t\t\t int, struct strbuf *err);\ndiff --git a/refs.c b/refs.c\nindex 917f8fc..39e43cf 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2326,7 +2326,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t */\n \t\t\tgoto retry;\n \t\telse\n-\t\t\tunable_to_lock_die(ref_file, errno);\n+\t\t\tunable_to_lock_die(ref_file, lflags, errno);\n \t}\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n@@ -2376,7 +2376,7 @@ int lock_packed_refs(struct strbuf *err)\n \tstruct packed_ref_cache *packed_ref_cache;\n \n \tif (hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), 0) < 0) {\n-\t\tunable_to_lock_message(git_path(\"packed-refs\"), errno, err);\n+\t\tunable_to_lock_message(git_path(\"packed-refs\"), 0, errno, err);\n \t\treturn -1;\n \t}\n \n"},{"id":"252927","messageId":"20141203051942.GQ6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 07/14] fast-import: use message from lockfile API when writing marks fails","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:19:42Z","receivedAt":"2014-12-03T05:19:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The usual way to handle errors from hold_lock_file_for_update is to\nget a message from unable_to_lock_message or unable_to_lock_die, which\ncan explain to the user how to check for other git processes running\nconcurrently and unlink the .lock file if safe.\n\nfast-import didn't use the unable_to_lock_message helper because at\nthe time it started using the lockfile API (v1.5.1-rc1~69^2~2,\n2007-03-07) that helper didn't exist. Later it still was not\nappropriate to use that helper because the message assumed the file\nbeing locked was inside a git repository. Now there is a flag to\nindicate that this lockfile is not part of the repository, so we can\nfinally use the helper.\n\nSigned-off-by: Jonathan Nieder \n---\n fast-import.c | 11 ++++++++---\n 1 file changed, 8 insertions(+), 3 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex d0bd285..bf8faa9 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1799,9 +1799,14 @@ static void dump_marks(void)\n \tif (!export_marks_file)\n \t\treturn;\n \n-\tif (hold_lock_file_for_update(&mark_lock, export_marks_file, 0) < 0) {\n-\t\tfailure |= error(\"Unable to write marks file %s: %s\",\n-\t\t\texport_marks_file, strerror(errno));\n+\tif (hold_lock_file_for_update(&mark_lock, export_marks_file,\n+\t\t\t\t LOCK_OUTSIDE_REPOSITORY) < 0) {\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\tunable_to_lock_message(export_marks_file,\n+\t\t\t\t LOCK_OUTSIDE_REPOSITORY, errno, &err);\n+\t\tfailure |= error(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t\treturn;\n \t}\n \n"},{"id":"252928","messageId":"20141203052044.GR6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 08/14] credentials: use message from lockfile API when locking ~/.git-credentials fails","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:20:44Z","receivedAt":"2014-12-03T05:20:44Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"If writing $HOME/.git-credentials.lock (or locking another file\nspecified with the --file option) fails due to the lock file already\nexisting, chances are that it is because another git process already\nlocked the file. Use the message from the lockfile API that says so,\nto help the user to look for running git processes and remove the\nstray .lock file when it is safe.\n\nSigned-off-by: Jonathan Nieder \n---\n credential-store.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex d435514..cd71156 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -55,8 +55,8 @@ static void print_line(struct strbuf *buf)\n static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\t\t\t struct strbuf *extra)\n {\n-\tif (hold_lock_file_for_update(&credential_lock, fn, 0) < 0)\n-\t\tdie_errno(\"unable to get credential storage lock\");\n+\thold_lock_file_for_update(&credential_lock, fn,\n+\t\t\t\t LOCK_DIE_ON_ERROR | LOCK_OUTSIDE_REPOSITORY);\n \tif (extra)\n \t\tprint_line(extra);\n \tparse_credential_file(fn, c, NULL, print_line);\n"},{"id":"252929","messageId":"20141203052124.GS6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 09/14] config: use message from lockfile API when locking config file fails","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:21:24Z","receivedAt":"2014-12-03T05:21:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The message from the lockfile API includes advice about how to remove\na stale lock file after a previous git process crashed.\n\nPass the LOCK_OUTSIDE_REPOSITORY flag to avoid confusing error\nmessages that imply the lockfile is under .git. These functions (and\n'git config --file') are useful for arbitrary files in git config\nformat, including both files outside .git such as /etc/gitconfig and\n$XDG_CONFIG_HOME/git/config and files under .git such as\n$GIT_DIR/config.\n\nSigned-off-by: Jonathan Nieder \n---\n config.c | 20 ++++++++++++++++----\n 1 file changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/config.c b/config.c\nindex 15a2983..dacde5f 100644\n--- a/config.c\n+++ b/config.c\n@@ -1940,9 +1940,15 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t * contents of .git/config will be written into it.\n \t */\n \tlock = xcalloc(1, sizeof(struct lock_file));\n-\tfd = hold_lock_file_for_update(lock, config_filename, 0);\n+\tfd = hold_lock_file_for_update(lock, config_filename,\n+\t\t\t\t LOCK_OUTSIDE_REPOSITORY);\n \tif (fd < 0) {\n-\t\terror(\"could not lock config file %s: %s\", config_filename, strerror(errno));\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\tunable_to_lock_message(config_filename,\n+\t\t\t\t LOCK_OUTSIDE_REPOSITORY, errno, &err);\n+\t\terror(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t\tfree(store.key);\n \t\tret = CONFIG_NO_LOCK;\n \t\tgoto out_free;\n@@ -2211,9 +2217,15 @@ int git_config_rename_section_in_file(const char *config_filename,\n \t\tconfig_filename = filename_buf = git_pathdup(\"config\");\n \n \tlock = xcalloc(1, sizeof(struct lock_file));\n-\tout_fd = hold_lock_file_for_update(lock, config_filename, 0);\n+\tout_fd = hold_lock_file_for_update(lock, config_filename,\n+\t\t\t\t\t LOCK_OUTSIDE_REPOSITORY);\n \tif (out_fd < 0) {\n-\t\tret = error(\"could not lock config file %s\", config_filename);\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\n+\t\tunable_to_lock_message(config_filename,\n+\t\t\t\t LOCK_OUTSIDE_REPOSITORY, errno, &err);\n+\t\tret = error(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n \t\tgoto out;\n \t}\n \n"},{"id":"252930","messageId":"20141203052227.GT6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 10/14] rerere: error out on autoupdate failure","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:22:27Z","receivedAt":"2014-12-03T05:22:27Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"We have been silently tolerating errors by returning early with an\nerror that the caller ignores since rerere.autoupdate was introduced\nin v1.6.0-rc0~120^2 (2008-06-22). So on error (for example if the\nindex is already locked), rerere can return success silently without\nupdating the index or with only some items in the index updated.\n\nBetter to treat such failures as a fatal error so the operator can\nfigure out what is wrong and fix it.\n\nSigned-off-by: Jonathan Nieder \n---\nAlso sent separately at\nhttp://thread.gmane.org/gmane.comp.version-control.git/260623\n\n rerere.c | 14 +++++---------\n 1 file changed, 5 insertions(+), 9 deletions(-)\n\ndiff --git a/rerere.c b/rerere.c\nindex 1b0555f..195f663 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -477,27 +477,23 @@ out:\n \n static struct lock_file index_lock;\n \n-static int update_paths(struct string_list *update)\n+static void update_paths(struct string_list *update)\n {\n \tint i;\n-\tint fd = hold_locked_index(&index_lock, 0);\n-\tint status = 0;\n \n-\tif (fd < 0)\n-\t\treturn -1;\n+\thold_locked_index(&index_lock, 1);\n \n \tfor (i = 0; i < update->nr; i++) {\n \t\tstruct string_list_item *item = &update->items[i];\n \t\tif (add_file_to_cache(item->string, ADD_CACHE_IGNORE_ERRORS))\n-\t\t\tstatus = -1;\n+\t\t\tdie(\"staging updated %s failed\", item->string);\n \t}\n \n-\tif (!status && active_cache_changed) {\n+\tif (active_cache_changed) {\n \t\tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n \t\t\tdie(\"Unable to write new index file\");\n-\t} else if (fd >= 0)\n+\t} else\n \t\trollback_lock_file(&index_lock);\n-\treturn status;\n }\n \n static int do_plain_rerere(struct string_list *rr, int fd)\n"},{"id":"252931","messageId":"20141203052519.GU6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 11/14] hold_locked_index: pass error message back through a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:25:19Z","receivedAt":"2014-12-03T05:25:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Today hold_locked_index takes a boolean parameter indicating whether\nto die with a message or to return -1 with errno indicating the nature\nof the problem on error.\n\nPass back an error message through an 'err' parameter instead. This\nmethod of error reporting was introduced in the ref transaction API\nand makes it more obvious when callers are not reporting errors (for\nexample, this helped catch rerere.autoupdate skipping the autoupdate\nwhen unable to lock the index).\n\nIt also makes it easier for callers to clean up before exiting instead\nof having to die() right away and paves the way for simplifying\nhold_lock_file_for_update error handling in a later patch.\n\nSigned-off-by: Jonathan Nieder \n---\n builtin/add.c | 12 ++++++++---\n builtin/apply.c | 10 +++++++--\n builtin/checkout-index.c | 10 +++++++--\n builtin/checkout.c | 55 ++++++++++++++++++++++++++++++++++--------------\n builtin/clone.c | 6 +++++-\n builtin/commit.c | 27 ++++++++++++++++++------\n builtin/describe.c | 11 +++++++---\n builtin/diff.c | 12 ++++++++---\n builtin/merge.c | 14 +++++++++---\n builtin/mv.c | 5 ++++-\n builtin/read-tree.c | 9 ++++++--\n builtin/reset.c | 10 +++++++--\n builtin/rm.c | 9 ++++++--\n builtin/update-index.c | 10 ++++-----\n cache-tree.c | 18 ++++++++++++----\n cache.h | 2 +-\n merge-recursive.c | 13 +++++++++---\n merge.c | 17 +++++++++++----\n read-cache.c | 14 +++++++-----\n rerere.c | 5 ++++-\n sequencer.c | 14 ++++++++++--\n test-scrap-cache-tree.c | 5 ++++-\n 22 files changed, 214 insertions(+), 74 deletions(-)\n\ndiff --git a/builtin/add.c b/builtin/add.c\nindex ae6d3e2..e912d77 100644\n--- a/builtin/add.c\n+++ b/builtin/add.c\n@@ -299,6 +299,7 @@ static int add_files(struct dir_struct *dir, int flags)\n int cmd_add(int argc, const char **argv, const char *prefix)\n {\n \tint exit_status = 0;\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct pathspec pathspec;\n \tstruct dir_struct dir;\n \tint flags;\n@@ -315,8 +316,10 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tif (add_interactive)\n \t\texit(interactive_add(argc - 1, argv + 1, prefix, patch_interactive));\n \n-\tif (edit_interactive)\n-\t\treturn(edit_patch(argc, argv, prefix));\n+\tif (edit_interactive) {\n+\t\tstrbuf_release(&err);\n+\t\treturn edit_patch(argc, argv, prefix);\n+\t}\n \targc--;\n \targv++;\n \n@@ -344,7 +347,8 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tadd_new_files = !take_worktree_changes && !refresh_only;\n \trequire_pathspec = !take_worktree_changes;\n \n-\thold_locked_index(&lock_file, 1);\n+\tif (hold_locked_index(&lock_file, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \n \tflags = ((verbose ? ADD_CACHE_VERBOSE : 0) |\n \t\t (show_only ? ADD_CACHE_PRETEND : 0) |\n@@ -356,6 +360,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)\n \tif (require_pathspec && argc == 0) {\n \t\tfprintf(stderr, _(\"Nothing specified, nothing added.\\n\"));\n \t\tfprintf(stderr, _(\"Maybe you wanted to say 'git add .'?\\n\"));\n+\t\tstrbuf_release(&err);\n \t\treturn 0;\n \t}\n \n@@ -446,5 +451,6 @@ finish:\n \t\t\tdie(_(\"Unable to write new index file\"));\n \t}\n \n+\tstrbuf_release(&err);\n \treturn exit_status;\n }\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex 6696ea4..cda438f 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -4200,6 +4200,7 @@ static int apply_patch(int fd, const char *filename, int options)\n {\n \tsize_t offset;\n \tstruct strbuf buf = STRBUF_INIT; /* owns the patch text */\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct patch *list = NULL, **listp = &list;\n \tint skipped_patch = 0;\n \n@@ -4237,8 +4238,11 @@ static int apply_patch(int fd, const char *filename, int options)\n \t\tapply = 0;\n \n \tupdate_index = check_index && apply;\n-\tif (update_index && newfd < 0)\n-\t\tnewfd = hold_locked_index(&lock_file, 1);\n+\tif (update_index && newfd < 0) {\n+\t\tnewfd = hold_locked_index(&lock_file, &err);\n+\t\tif (newfd < 0)\n+\t\t\tdie(\"%s\", err.buf);\n+\t}\n \n \tif (check_index) {\n \t\tif (read_cache() < 0)\n@@ -4254,6 +4258,7 @@ static int apply_patch(int fd, const char *filename, int options)\n \t\tif (apply_with_reject)\n \t\t\texit(1);\n \t\t/* with --3way, we still need to write the index out */\n+\t\tstrbuf_release(&err);\n \t\treturn 1;\n \t}\n \n@@ -4272,6 +4277,7 @@ static int apply_patch(int fd, const char *filename, int options)\n \tfree_patch_list(list);\n \tstrbuf_release(&buf);\n \tstring_list_clear(&fn_table, 0);\n+\tstrbuf_release(&err);\n \treturn 0;\n }\n \ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex 383dccf..07164f5 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -133,11 +133,17 @@ static int option_parse_u(const struct option *opt,\n \t\t\t const char *arg, int unset)\n {\n \tint *newfd = opt->value;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tstate.refresh_cache = 1;\n \tstate.istate = &the_index;\n-\tif (*newfd < 0)\n-\t\t*newfd = hold_locked_index(&lock_file, 1);\n+\tif (*newfd < 0) {\n+\t\tint fd = hold_locked_index(&lock_file, &err);\n+\t\tif (fd < 0)\n+\t\t\tdie(\"%s\", err.buf);\n+\t\t*newfd = fd;\n+\t}\n+\tstrbuf_release(&err);\n \treturn 0;\n }\n \ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 5410dac..d2699cd 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -218,6 +218,7 @@ static int checkout_merged(int pos, struct checkout *state)\n static int checkout_paths(const struct checkout_opts *opts,\n \t\t\t const char *revision)\n {\n+\tstruct strbuf err = STRBUF_INIT;\n \tint pos;\n \tstruct checkout state;\n \tstatic char *ps_matched;\n@@ -249,15 +250,20 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\tdie(_(\"Cannot update paths and switch to branch '%s' at the same time.\"),\n \t\t opts->new_branch);\n \n-\tif (opts->patch_mode)\n+\tif (opts->patch_mode) {\n+\t\tstrbuf_release(&err);\n \t\treturn run_add_interactive(revision, \"--patch=checkout\",\n \t\t\t\t\t &opts->pathspec);\n+\t}\n \n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\thold_locked_index(lock_file, 1);\n-\tif (read_cache_preload(&opts->pathspec) < 0)\n+\tif (hold_locked_index(lock_file, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n+\tif (read_cache_preload(&opts->pathspec) < 0) {\n+\t\tstrbuf_release(&err);\n \t\treturn error(_(\"corrupt index file\"));\n+\t}\n \n \tif (opts->source_tree)\n \t\tread_tree_some(opts->source_tree, &opts->pathspec);\n@@ -302,6 +308,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \n \tif (report_path_error(ps_matched, &opts->pathspec, opts->prefix)) {\n \t\tfree(ps_matched);\n+\t\tstrbuf_release(&err);\n \t\treturn 1;\n \t}\n \tfree(ps_matched);\n@@ -329,8 +336,10 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t\tpos = skip_same_name(ce, pos) - 1;\n \t\t}\n \t}\n-\tif (errs)\n+\tif (errs) {\n+\t\tstrbuf_release(&err);\n \t\treturn 1;\n+\t}\n \n \t/* Now we are committed to check them out */\n \tmemset(&state, 0, sizeof(state));\n@@ -359,6 +368,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \thead = lookup_commit_reference_gently(rev, 1);\n \n \terrs |= post_checkout_hook(head, head, 0);\n+\tstrbuf_release(&err);\n \treturn errs;\n }\n \n@@ -442,17 +452,22 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\t int *writeout_error)\n {\n \tint ret;\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\thold_locked_index(lock_file, 1);\n-\tif (read_cache_preload(NULL) < 0)\n-\t\treturn error(_(\"corrupt index file\"));\n+\tif (hold_locked_index(lock_file, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n+\n+\tif (read_cache_preload(NULL) < 0) {\n+\t\tret = error(_(\"corrupt index file\"));\n+\t\tgoto done;\n+\t}\n \n \tresolve_undo_clear();\n \tif (opts->force) {\n \t\tret = reset_tree(new->commit->tree, opts, 1, writeout_error);\n \t\tif (ret)\n-\t\t\treturn ret;\n+\t\t\tgoto done;\n \t} else {\n \t\tstruct tree_desc trees[2];\n \t\tstruct tree *tree;\n@@ -469,7 +484,8 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \n \t\tif (unmerged_cache()) {\n \t\t\terror(_(\"you need to resolve your current index first\"));\n-\t\t\treturn 1;\n+\t\t\tret = 1;\n+\t\t\tgoto done;\n \t\t}\n \n \t\t/* 2-way merge to the new branch */\n@@ -501,15 +517,19 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\tstruct tree *result;\n \t\t\tstruct tree *work;\n \t\t\tstruct merge_options o;\n-\t\t\tif (!opts->merge)\n-\t\t\t\treturn 1;\n+\t\t\tif (!opts->merge) {\n+\t\t\t\tret = 1;\n+\t\t\t\tgoto done;\n+\t\t\t}\n \n \t\t\t/*\n \t\t\t * Without old->commit, the below is the same as\n \t\t\t * the two-tree unpack we already tried and failed.\n \t\t\t */\n-\t\t\tif (!old->commit)\n-\t\t\t\treturn 1;\n+\t\t\tif (!old->commit) {\n+\t\t\t\tret = 1;\n+\t\t\t\tgoto done;\n+\t\t\t}\n \n \t\t\t/* Do more real merge */\n \n@@ -539,7 +559,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\tret = reset_tree(new->commit->tree, opts, 1,\n \t\t\t\t\t writeout_error);\n \t\t\tif (ret)\n-\t\t\t\treturn ret;\n+\t\t\t\tgoto done;\n \t\t\to.ancestor = old->name;\n \t\t\to.branch1 = new->name;\n \t\t\to.branch2 = \"local\";\n@@ -548,7 +568,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t\tret = reset_tree(new->commit->tree, opts, 0,\n \t\t\t\t\t writeout_error);\n \t\t\tif (ret)\n-\t\t\t\treturn ret;\n+\t\t\t\tgoto done;\n \t\t}\n \t}\n \n@@ -564,7 +584,10 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \tif (!opts->force && !opts->quiet)\n \t\tshow_local_changes(&new->commit->object, &opts->diff_options);\n \n-\treturn 0;\n+\tret = 0;\n+done:\n+\tstrbuf_release(&err);\n+\treturn ret;\n }\n \n static void report_tracking(struct branch_info *new)\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex b07d740..911a6a4 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -613,6 +613,7 @@ static void update_head(const struct ref *our, const struct ref *remote,\n \n static int checkout(void)\n {\n+\tstruct strbuf msg = STRBUF_INIT;\n \tunsigned char sha1[20];\n \tchar *head;\n \tstruct lock_file *lock_file;\n@@ -628,6 +629,7 @@ static int checkout(void)\n \tif (!head) {\n \t\twarning(_(\"remote HEAD refers to nonexistent ref, \"\n \t\t\t \"unable to checkout.\\n\"));\n+\t\tstrbuf_release(&msg);\n \t\treturn 0;\n \t}\n \tif (!strcmp(head, \"HEAD\")) {\n@@ -643,7 +645,8 @@ static int checkout(void)\n \tsetup_work_tree();\n \n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\thold_locked_index(lock_file, 1);\n+\tif (hold_locked_index(lock_file, &msg) < 0)\n+\t\tdie(\"%s\", msg.buf);\n \n \tmemset(&opts, 0, sizeof opts);\n \topts.update = 1;\n@@ -668,6 +671,7 @@ static int checkout(void)\n \tif (!err && option_recursive)\n \t\terr = run_command_v_opt(argv_submodule, RUN_GIT_CMD);\n \n+\tstrbuf_release(&msg);\n \treturn err;\n }\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex e108c53..edc4493 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -319,6 +319,7 @@ static void refresh_cache_or_die(int refresh_flags)\n static const char *prepare_index(int argc, const char **argv, const char *prefix,\n \t\t\t\t const struct commit *current_head, int is_status)\n {\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct string_list partial;\n \tstruct pathspec pathspec;\n \tint refresh_flags = REFRESH_QUIET;\n@@ -334,8 +335,9 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \n \tif (interactive) {\n \t\tchar *old_index_env = NULL;\n-\t\thold_locked_index(&index_lock, 1);\n \n+\t\tif (hold_locked_index(&index_lock, &err) < 0)\n+\t\t\tdie(\"%s\", err.buf);\n \t\trefresh_cache_or_die(refresh_flags);\n \n \t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n@@ -363,6 +365,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\twarning(_(\"Failed to update main cache tree\"));\n \n \t\tcommit_style = COMMIT_NORMAL;\n+\t\tstrbuf_release(&err);\n \t\treturn index_lock.filename.buf;\n \t}\n \n@@ -379,13 +382,15 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t * (B) on failure, rollback the real index.\n \t */\n \tif (all || (also && pathspec.nr)) {\n-\t\thold_locked_index(&index_lock, 1);\n+\t\tif (hold_locked_index(&index_lock, &err) < 0)\n+\t\t\tdie(\"%s\", err.buf);\n \t\tadd_files_to_cache(also ? prefix : NULL, &pathspec, 0);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tupdate_main_cache_tree(WRITE_TREE_SILENT);\n \t\tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n \t\t\tdie(_(\"unable to write new_index file\"));\n \t\tcommit_style = COMMIT_NORMAL;\n+\t\tstrbuf_release(&err);\n \t\treturn index_lock.filename.buf;\n \t}\n \n@@ -399,7 +404,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t * We still need to refresh the index here.\n \t */\n \tif (!only && !pathspec.nr) {\n-\t\thold_locked_index(&index_lock, 1);\n+\t\tif (hold_locked_index(&index_lock, &err) < 0)\n+\t\t\tdie(\"%s\", err.buf);\n \t\trefresh_cache_or_die(refresh_flags);\n \t\tif (active_cache_changed\n \t\t || !cache_tree_fully_valid(active_cache_tree)) {\n@@ -414,6 +420,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \t\t\trollback_lock_file(&index_lock);\n \t\t}\n \t\tcommit_style = COMMIT_AS_IS;\n+\t\tstrbuf_release(&err);\n \t\treturn get_index_file();\n \t}\n \n@@ -453,7 +460,8 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tif (read_cache() < 0)\n \t\tdie(_(\"cannot read the index\"));\n \n-\thold_locked_index(&index_lock, 1);\n+\tif (hold_locked_index(&index_lock, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \tadd_remove_files(&partial);\n \trefresh_cache(REFRESH_QUIET);\n \tupdate_main_cache_tree(WRITE_TREE_SILENT);\n@@ -475,6 +483,7 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tdiscard_cache();\n \tread_cache_from(false_lock.filename.buf);\n \n+\tstrbuf_release(&err);\n \treturn false_lock.filename.buf;\n }\n \n@@ -1356,8 +1365,8 @@ static int git_status_config(const char *k, const char *v, void *cb)\n int cmd_status(int argc, const char **argv, const char *prefix)\n {\n \tstatic struct wt_status s;\n-\tint fd;\n \tunsigned char sha1[20];\n+\tstruct strbuf err = STRBUF_INIT;\n \tstatic struct option builtin_status_options[] = {\n \t\tOPT__VERBOSE(&verbose, N_(\"be verbose\")),\n \t\tOPT_SET_INT('s', \"short\", &status_format,\n@@ -1405,8 +1414,11 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \tread_cache_preload(&s.pathspec);\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, &s.pathspec, NULL, NULL);\n \n-\tfd = hold_locked_index(&index_lock, 0);\n-\tif (0 <= fd)\n+\t/* Refresh the index if possible. */\n+\tif (hold_locked_index(&index_lock, &err) < 0)\n+\t\t/* errors (e.g., read-only filesystem) are fine */\n+\t\tstrbuf_reset(&err);\n+\telse\n \t\tupdate_index_if_able(&the_index, &index_lock);\n \n \ts.is_initial = get_sha1(s.reference, sha1) ? 1 : 0;\n@@ -1433,6 +1445,7 @@ int cmd_status(int argc, const char **argv, const char *prefix)\n \t\twt_status_print(&s);\n \t\tbreak;\n \t}\n+\tstrbuf_release(&err);\n \treturn 0;\n }\n \ndiff --git a/builtin/describe.c b/builtin/describe.c\nindex 9103193..1e7e970 100644\n--- a/builtin/describe.c\n+++ b/builtin/describe.c\n@@ -457,19 +457,24 @@ int cmd_describe(int argc, const char **argv, const char *prefix)\n \n \tif (argc == 0) {\n \t\tif (dirty) {\n+\t\t\tstruct strbuf err = STRBUF_INIT;\n \t\t\tstatic struct lock_file index_lock;\n-\t\t\tint fd;\n \n \t\t\tread_cache_preload(NULL);\n \t\t\trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED,\n \t\t\t\t NULL, NULL, NULL);\n-\t\t\tfd = hold_locked_index(&index_lock, 0);\n-\t\t\tif (0 <= fd)\n+\n+\t\t\t/* Refresh the index if possible. */\n+\t\t\tif (hold_locked_index(&index_lock, &err) < 0)\n+\t\t\t\t/* Failure is ok (e.g, read-only filesystem). */\n+\t\t\t\tstrbuf_reset(&err);\n+\t\t\telse\n \t\t\t\tupdate_index_if_able(&the_index, &index_lock);\n \n \t\t\tif (!cmd_diff_index(ARRAY_SIZE(diff_index_args) - 1,\n \t\t\t\t\t diff_index_args, prefix))\n \t\t\t\tdirty = NULL;\n+\t\t\tstrbuf_release(&err);\n \t\t}\n \t\tdescribe(\"HEAD\", 1);\n \t} else if (dirty) {\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex 4326fa5..9dd7b93 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -206,16 +206,22 @@ static int builtin_diff_combined(struct rev_info *revs,\n static void refresh_index_quietly(void)\n {\n \tstruct lock_file *lock_file;\n-\tint fd;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n-\tfd = hold_locked_index(lock_file, 0);\n-\tif (fd < 0)\n+\tif (hold_locked_index(lock_file, &err) < 0) {\n+\t\t/*\n+\t\t * Locking the index failed (e.g., read-only filesystem).\n+\t\t * That's okay.\n+\t\t */\n+\t\tstrbuf_release(&err);\n \t\treturn;\n+\t}\n \tdiscard_cache();\n \tread_cache();\n \trefresh_cache(REFRESH_QUIET|REFRESH_UNMERGED);\n \tupdate_index_if_able(&the_index, lock_file);\n+\tstrbuf_release(&err);\n }\n \n static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv)\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex bebbe5b..1ecea33 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -658,12 +658,16 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\t\t struct commit *head, const char *head_arg)\n {\n \tstatic struct lock_file lock;\n+\tstruct strbuf err = STRBUF_INIT;\n \n-\thold_locked_index(&lock, 1);\n+\tif (hold_locked_index(&lock, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \trefresh_cache(REFRESH_QUIET);\n \tif (active_cache_changed &&\n-\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n+\t write_locked_index(&the_index, &lock, COMMIT_LOCK)) {\n+\t\tstrbuf_release(&err);\n \t\treturn error(_(\"Unable to write index.\"));\n+\t}\n \trollback_lock_file(&lock);\n \n \tif (!strcmp(strategy, \"recursive\") || !strcmp(strategy, \"subtree\")) {\n@@ -675,6 +679,7 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \n \t\tif (remoteheads->next) {\n \t\t\terror(_(\"Not handling anything other than two heads merge.\"));\n+\t\t\tstrbuf_release(&err);\n \t\t\treturn 2;\n \t\t}\n \n@@ -696,15 +701,18 @@ static int try_merge_strategy(const char *strategy, struct commit_list *common,\n \t\tfor (j = common; j; j = j->next)\n \t\t\tcommit_list_insert(j->item, &reversed);\n \n-\t\thold_locked_index(&lock, 1);\n+\t\tif (hold_locked_index(&lock, &err) < 0)\n+\t\t\tdie(\"%s\", err.buf);\n \t\tclean = merge_recursive(&o, head,\n \t\t\t\tremoteheads->item, reversed, &result);\n \t\tif (active_cache_changed &&\n \t\t write_locked_index(&the_index, &lock, COMMIT_LOCK))\n \t\t\tdie (_(\"unable to write %s\"), get_index_file());\n \t\trollback_lock_file(&lock);\n+\t\tstrbuf_release(&err);\n \t\treturn clean ? 0 : 1;\n \t} else {\n+\t\tstrbuf_release(&err);\n \t\treturn try_merge_command(strategy, xopts_nr, xopts,\n \t\t\t\t\t\tcommon, head_arg, remoteheads);\n \t}\ndiff --git a/builtin/mv.c b/builtin/mv.c\nindex 563d05b..e87817f 100644\n--- a/builtin/mv.c\n+++ b/builtin/mv.c\n@@ -116,6 +116,7 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tenum update_mode { BOTH = 0, WORKING_DIRECTORY, INDEX } *modes;\n \tstruct stat st;\n \tstruct string_list src_for_dst = STRING_LIST_INIT_NODUP;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tgitmodules_config();\n \tgit_config(git_default_config, NULL);\n@@ -125,7 +126,8 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \tif (--argc < 1)\n \t\tusage_with_options(builtin_mv_usage, builtin_mv_options);\n \n-\thold_locked_index(&lock_file, 1);\n+\tif (hold_locked_index(&lock_file, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n \n@@ -278,5 +280,6 @@ int cmd_mv(int argc, const char **argv, const char *prefix)\n \t write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(_(\"Unable to write new index file\"));\n \n+\tstrbuf_release(&err);\n \treturn 0;\n }\ndiff --git a/builtin/read-tree.c b/builtin/read-tree.c\nindex 43b47f7..2328b96 100644\n--- a/builtin/read-tree.c\n+++ b/builtin/read-tree.c\n@@ -100,6 +100,7 @@ static struct lock_file lock_file;\n \n int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n {\n+\tstruct strbuf err = STRBUF_INIT;\n \tint i, stage = 0;\n \tunsigned char sha1[20];\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n@@ -150,7 +151,8 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \targc = parse_options(argc, argv, unused_prefix, read_tree_options,\n \t\t\t read_tree_usage, 0);\n \n-\thold_locked_index(&lock_file, 1);\n+\tif (hold_locked_index(&lock_file, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \n \tprefix_set = opts.prefix ? 1 : 0;\n \tif (1 < opts.merge + opts.reset + prefix_set)\n@@ -228,8 +230,10 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \t\tparse_tree(tree);\n \t\tinit_tree_desc(t+i, tree->buffer, tree->size);\n \t}\n-\tif (unpack_trees(nr_trees, t, &opts))\n+\tif (unpack_trees(nr_trees, t, &opts)) {\n+\t\tstrbuf_release(&err);\n \t\treturn 128;\n+\t}\n \n \tif (opts.debug_unpack || opts.dry_run)\n \t\treturn 0; /* do not write the index out */\n@@ -245,5 +249,6 @@ int cmd_read_tree(int argc, const char **argv, const char *unused_prefix)\n \n \tif (write_locked_index(&the_index, &lock_file, COMMIT_LOCK))\n \t\tdie(\"unable to write new index file\");\n+\tstrbuf_release(&err);\n \treturn 0;\n }\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex 4c08ddc..f83df72 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -266,6 +266,7 @@ static int reset_refs(const char *rev, const unsigned char *sha1)\n \n int cmd_reset(int argc, const char **argv, const char *prefix)\n {\n+\tstruct strbuf msg = STRBUF_INIT;\n \tint reset_type = NONE, update_ref_status = 0, quiet = 0;\n \tint patch_mode = 0, unborn;\n \tconst char *rev;\n@@ -320,6 +321,7 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tif (patch_mode) {\n \t\tif (reset_type != NONE)\n \t\t\tdie(_(\"--patch is incompatible with --{hard,mixed,soft}\"));\n+\t\tstrbuf_release(&msg);\n \t\treturn run_add_interactive(rev, \"--patch=reset\", &pathspec);\n \t}\n \n@@ -354,11 +356,14 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \n \tif (reset_type != SOFT) {\n \t\tstruct lock_file *lock = xcalloc(1, sizeof(*lock));\n-\t\thold_locked_index(lock, 1);\n+\t\tif (hold_locked_index(lock, &msg) < 0)\n+\t\t\tdie(\"%s\", msg.buf);\n \t\tif (reset_type == MIXED) {\n \t\t\tint flags = quiet ? REFRESH_QUIET : REFRESH_IN_PORCELAIN;\n-\t\t\tif (read_from_tree(&pathspec, sha1, intent_to_add))\n+\t\t\tif (read_from_tree(&pathspec, sha1, intent_to_add)) {\n+\t\t\t\tstrbuf_release(&msg);\n \t\t\t\treturn 1;\n+\t\t\t}\n \t\t\tif (get_git_work_tree())\n \t\t\t\trefresh_index(&the_index, flags, NULL, NULL,\n \t\t\t\t\t _(\"Unstaged changes after reset:\"));\n@@ -385,5 +390,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \tif (!pathspec.nr)\n \t\tremove_branch_state();\n \n+\tstrbuf_release(&msg);\n \treturn update_ref_status;\n }\ndiff --git a/builtin/rm.c b/builtin/rm.c\nindex d8a9c86..d54538e 100644\n--- a/builtin/rm.c\n+++ b/builtin/rm.c\n@@ -281,6 +281,7 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tint i;\n \tstruct pathspec pathspec;\n \tchar *seen;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \tgitmodules_config();\n \tgit_config(git_default_config, NULL);\n@@ -293,7 +294,8 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \tif (!index_only)\n \t\tsetup_work_tree();\n \n-\thold_locked_index(&lock_file, 1);\n+\tif (hold_locked_index(&lock_file, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \n \tif (read_cache() < 0)\n \t\tdie(_(\"index file corrupt\"));\n@@ -375,8 +377,10 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"git rm: unable to remove %s\"), path);\n \t}\n \n-\tif (show_only)\n+\tif (show_only) {\n+\t\tstrbuf_release(&err);\n \t\treturn 0;\n+\t}\n \n \t/*\n \t * Then, unless we used \"--cached\", remove the filenames from\n@@ -431,5 +435,6 @@ int cmd_rm(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"Unable to write new index file\"));\n \t}\n \n+\tstrbuf_release(&err);\n \treturn 0;\n }\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 56abd18..f3875db 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -746,7 +746,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \tint preferred_index_format = 0;\n \tchar set_executable_bit = 0;\n \tstruct refresh_params refresh_args = {0, &has_errors};\n-\tint lock_error = 0;\n+\tstruct strbuf lock_error = STRBUF_INIT;\n \tint split_index = -1;\n \tstruct lock_file *lock_file;\n \tstruct parse_opt_ctx_t ctx;\n@@ -843,9 +843,7 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t/* We can't free this memory, it becomes part of a linked list parsed atexit() */\n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\tnewfd = hold_locked_index(lock_file, 0);\n-\tif (newfd < 0)\n-\t\tlock_error = errno;\n+\tnewfd = hold_locked_index(lock_file, &lock_error);\n \n \tentries = read_cache();\n \tif (entries < 0)\n@@ -943,13 +941,13 @@ int cmd_update_index(int argc, const char **argv, const char *prefix)\n \t\tif (newfd < 0) {\n \t\t\tif (refresh_args.flags & REFRESH_QUIET)\n \t\t\t\texit(128);\n-\t\t\tunable_to_lock_die(get_index_file(), 0, lock_error);\n+\t\t\tdie(\"%s\", lock_error.buf);\n \t\t}\n \t\tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n \t\t\tdie(\"Unable to write new index file\");\n \t}\n \n \trollback_lock_file(lock_file);\n-\n+\tstrbuf_release(&lock_error);\n \treturn has_errors ? 1 : 0;\n }\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 32772b9..b883b8f 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -595,6 +595,7 @@ static struct cache_tree *cache_tree_find(struct cache_tree *it, const char *pat\n int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n {\n \tint entries, was_valid, newfd;\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct lock_file *lock_file;\n \n \t/*\n@@ -603,11 +604,15 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \t */\n \tlock_file = xcalloc(1, sizeof(struct lock_file));\n \n-\tnewfd = hold_locked_index(lock_file, 1);\n+\tnewfd = hold_locked_index(lock_file, &err);\n+\tif (newfd < 0)\n+\t\tdie(\"%s\", err.buf);\n \n \tentries = read_cache();\n-\tif (entries < 0)\n+\tif (entries < 0) {\n+\t\tstrbuf_release(&err);\n \t\treturn WRITE_TREE_UNREADABLE_INDEX;\n+\t}\n \tif (flags & WRITE_TREE_IGNORE_CACHE_TREE)\n \t\tcache_tree_free(&(active_cache_tree));\n \n@@ -616,8 +621,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \n \twas_valid = cache_tree_fully_valid(active_cache_tree);\n \tif (!was_valid) {\n-\t\tif (cache_tree_update(&the_index, flags) < 0)\n+\t\tif (cache_tree_update(&the_index, flags) < 0) {\n+\t\t\tstrbuf_release(&err);\n \t\t\treturn WRITE_TREE_UNMERGED_INDEX;\n+\t\t}\n \t\tif (0 <= newfd) {\n \t\t\tif (!write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n \t\t\t\tnewfd = -1;\n@@ -633,8 +640,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \tif (prefix) {\n \t\tstruct cache_tree *subtree =\n \t\t\tcache_tree_find(active_cache_tree, prefix);\n-\t\tif (!subtree)\n+\t\tif (!subtree) {\n+\t\t\tstrbuf_release(&err);\n \t\t\treturn WRITE_TREE_PREFIX_ERROR;\n+\t\t}\n \t\thashcpy(sha1, subtree->sha1);\n \t}\n \telse\n@@ -643,6 +652,7 @@ int write_cache_as_tree(unsigned char *sha1, int flags, const char *prefix)\n \tif (0 <= newfd)\n \t\trollback_lock_file(lock_file);\n \n+\tstrbuf_release(&err);\n \treturn 0;\n }\n \ndiff --git a/cache.h b/cache.h\nindex ddaa30f..bfff653 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -572,7 +572,7 @@ extern int refresh_index(struct index_state *, unsigned int flags, const struct\n \n extern void update_index_if_able(struct index_state *, struct lock_file *);\n \n-extern int hold_locked_index(struct lock_file *, int);\n+extern int hold_locked_index(struct lock_file *, struct strbuf *err);\n extern void set_alternate_index_output(const char *);\n \n extern int delete_ref(const char *, const unsigned char *sha1, int delopt);\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex fdb7d0f..82a2330 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1995,6 +1995,7 @@ int merge_recursive_generic(struct merge_options *o,\n \t\t\t struct commit **result)\n {\n \tint clean;\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n \tstruct commit *head_commit = get_ref(head, o->branch1);\n \tstruct commit *next_commit = get_ref(merge, o->branch2);\n@@ -2004,20 +2005,26 @@ int merge_recursive_generic(struct merge_options *o,\n \t\tint i;\n \t\tfor (i = 0; i < num_base_list; ++i) {\n \t\t\tstruct commit *base;\n-\t\t\tif (!(base = get_ref(base_list[i], sha1_to_hex(base_list[i]))))\n+\t\t\tif (!(base = get_ref(base_list[i], sha1_to_hex(base_list[i])))) {\n+\t\t\t\tstrbuf_release(&err);\n \t\t\t\treturn error(_(\"Could not parse object '%s'\"),\n \t\t\t\t\tsha1_to_hex(base_list[i]));\n+\t\t\t}\n \t\t\tcommit_list_insert(base, &ca);\n \t\t}\n \t}\n \n-\thold_locked_index(lock, 1);\n+\tif (hold_locked_index(lock, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \tclean = merge_recursive(o, head_commit, next_commit, ca,\n \t\t\tresult);\n \tif (active_cache_changed &&\n-\t write_locked_index(&the_index, lock, COMMIT_LOCK))\n+\t write_locked_index(&the_index, lock, COMMIT_LOCK)) {\n+\t\tstrbuf_release(&err);\n \t\treturn error(_(\"Unable to write index.\"));\n+\t}\n \n+\tstrbuf_release(&err);\n \treturn clean ? 0 : 1;\n }\n \ndiff --git a/merge.c b/merge.c\nindex fcff632..b26b7f3 100644\n--- a/merge.c\n+++ b/merge.c\n@@ -53,11 +53,13 @@ int checkout_fast_forward(const unsigned char *head,\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n \tint i, nr_trees = 0;\n \tstruct dir_struct dir;\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct lock_file *lock_file = xcalloc(1, sizeof(struct lock_file));\n \n \trefresh_cache(REFRESH_QUIET);\n \n-\thold_locked_index(lock_file, 1);\n+\tif (hold_locked_index(lock_file, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \n \tmemset(&trees, 0, sizeof(trees));\n \tmemset(&opts, 0, sizeof(opts));\n@@ -79,18 +81,25 @@ int checkout_fast_forward(const unsigned char *head,\n \tsetup_unpack_trees_porcelain(&opts, \"merge\");\n \n \ttrees[nr_trees] = parse_tree_indirect(head);\n-\tif (!trees[nr_trees++])\n+\tif (!trees[nr_trees++]) {\n+\t\tstrbuf_release(&err);\n \t\treturn -1;\n+\t}\n \ttrees[nr_trees] = parse_tree_indirect(remote);\n-\tif (!trees[nr_trees++])\n+\tif (!trees[nr_trees++]) {\n+\t\tstrbuf_release(&err);\n \t\treturn -1;\n+\t}\n \tfor (i = 0; i < nr_trees; i++) {\n \t\tparse_tree(trees[i]);\n \t\tinit_tree_desc(t+i, trees[i]->buffer, trees[i]->size);\n \t}\n-\tif (unpack_trees(nr_trees, t, &opts))\n+\tif (unpack_trees(nr_trees, t, &opts)) {\n+\t\tstrbuf_release(&err);\n \t\treturn -1;\n+\t}\n \tif (write_locked_index(&the_index, lock_file, COMMIT_LOCK))\n \t\tdie(_(\"unable to write new index file\"));\n+\tstrbuf_release(&err);\n \treturn 0;\n }\ndiff --git a/read-cache.c b/read-cache.c\nindex 8f3e9eb..2ce1a76 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1368,12 +1368,16 @@ static int read_index_extension(struct index_state *istate,\n \treturn 0;\n }\n \n-int hold_locked_index(struct lock_file *lk, int die_on_error)\n+int hold_locked_index(struct lock_file *lk, struct strbuf *err)\n {\n-\treturn hold_lock_file_for_update(lk, get_index_file(),\n-\t\t\t\t\t die_on_error\n-\t\t\t\t\t ? LOCK_DIE_ON_ERROR\n-\t\t\t\t\t : 0);\n+\tconst char *path;\n+\tint fd;\n+\n+\tpath = get_index_file();\n+\tfd = hold_lock_file_for_update(lk, path, 0);\n+\tif (fd < 0)\n+\t\tunable_to_lock_message(path, 0, errno, err);\n+\treturn fd;\n }\n \n int read_index(struct index_state *istate)\ndiff --git a/rerere.c b/rerere.c\nindex 195f663..044e279 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -479,9 +479,11 @@ static struct lock_file index_lock;\n \n static void update_paths(struct string_list *update)\n {\n+\tstruct strbuf err = STRBUF_INIT;\n \tint i;\n \n-\thold_locked_index(&index_lock, 1);\n+\tif (hold_locked_index(&index_lock, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \n \tfor (i = 0; i < update->nr; i++) {\n \t\tstruct string_list_item *item = &update->items[i];\n@@ -494,6 +496,7 @@ static void update_paths(struct string_list *update)\n \t\t\tdie(\"Unable to write new index file\");\n \t} else\n \t\trollback_lock_file(&index_lock);\n+\tstrbuf_release(&err);\n }\n \n static int do_plain_rerere(struct string_list *rr, int fd)\ndiff --git a/sequencer.c b/sequencer.c\nindex a03d4fa..3b3a869 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -276,9 +276,11 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \tstruct tree *result, *next_tree, *base_tree, *head_tree;\n \tint clean;\n \tconst char **xopt;\n+\tstruct strbuf err = STRBUF_INIT;\n \tstatic struct lock_file index_lock;\n \n-\thold_locked_index(&index_lock, 1);\n+\tif (hold_locked_index(&index_lock, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \n \tread_cache();\n \n@@ -323,6 +325,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \t\t}\n \t}\n \n+\tstrbuf_release(&err);\n \treturn !clean;\n }\n \n@@ -647,7 +650,13 @@ static void prepare_revs(struct replay_opts *opts)\n static void read_and_refresh_cache(struct replay_opts *opts)\n {\n \tstatic struct lock_file index_lock;\n-\tint index_fd = hold_locked_index(&index_lock, 0);\n+\tstruct strbuf err = STRBUF_INIT;\n+\tint index_fd;\n+\n+\tindex_fd = hold_locked_index(&index_lock, &err);\n+\tif (index_fd < 0)\n+\t\t/* ignore the error */\n+\t\tstrbuf_reset(&err);\n \tif (read_index_preload(&the_index, NULL) < 0)\n \t\tdie(_(\"git %s: failed to read the index\"), action_name(opts));\n \trefresh_index(&the_index, REFRESH_QUIET|REFRESH_UNMERGED, NULL, NULL, NULL);\n@@ -656,6 +665,7 @@ static void read_and_refresh_cache(struct replay_opts *opts)\n \t\t\tdie(_(\"git %s: failed to refresh the index\"), action_name(opts));\n \t}\n \trollback_lock_file(&index_lock);\n+\tstrbuf_release(&err);\n }\n \n static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\ndiff --git a/test-scrap-cache-tree.c b/test-scrap-cache-tree.c\nindex 6efee31..3e30e8d 100644\n--- a/test-scrap-cache-tree.c\n+++ b/test-scrap-cache-tree.c\n@@ -7,11 +7,14 @@ static struct lock_file index_lock;\n \n int main(int ac, char **av)\n {\n-\thold_locked_index(&index_lock, 1);\n+\tstruct strbuf err = STRBUF_INIT;\n+\tif (hold_locked_index(&index_lock, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \tif (read_cache() < 0)\n \t\tdie(\"unable to read index file\");\n \tactive_cache_tree = NULL;\n \tif (write_locked_index(&the_index, &index_lock, COMMIT_LOCK))\n \t\tdie(\"unable to write index file\");\n+\tstrbuf_release(&err);\n \treturn 0;\n }\n"},{"id":"252932","messageId":"20141203052602.GV6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 12/14] hold_lock_file_for_update: pass error message back through a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:26:03Z","receivedAt":"2014-12-03T05:26:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"This makes it more obvious when callers forget to print a message on\nerror, while still giving callers a chance to clean up before exiting.\n\nSigned-off-by: Jonathan Nieder \n---\n Documentation/technical/api-lockfile.txt | 33 +++++++-------------------------\n builtin/apply.c | 5 ++++-\n builtin/commit.c | 9 +++++----\n builtin/gc.c | 8 ++++++--\n bundle.c | 10 +++++++---\n config.c | 18 ++++++-----------\n credential-store.c | 8 ++++++--\n fast-import.c | 8 +++-----\n lockfile.c | 26 ++++++++++---------------\n lockfile.h | 8 ++++----\n read-cache.c | 9 +--------\n refs.c | 17 +++++++++-------\n rerere.c | 7 +++++--\n sequencer.c | 19 ++++++++++++++----\n shallow.c | 16 ++++++++++++----\n 15 files changed, 101 insertions(+), 100 deletions(-)\n\ndiff --git a/Documentation/technical/api-lockfile.txt b/Documentation/technical/api-lockfile.txt\nindex fa60cfd..8cb6704 100644\n--- a/Documentation/technical/api-lockfile.txt\n+++ b/Documentation/technical/api-lockfile.txt\n@@ -87,21 +87,8 @@ Error handling\n --------------\n \n The `hold_lock_file_*` functions return a file descriptor on success\n-or -1 on failure (unless `LOCK_DIE_ON_ERROR` is used; see below). On\n-errors, `errno` describes the reason for failure. Errors can be\n-reported by passing `errno` to one of the following helper functions:\n-\n-unable_to_lock_message::\n-\n-\tAppend an appropriate error message to a `strbuf`.\n-\n-unable_to_lock_error::\n-\n-\tEmit an appropriate error message using `error()`.\n-\n-unable_to_lock_die::\n-\n-\tEmit an appropriate error message and `die()`.\n+or -1 on failure. On errors, a message describing the reason for\n+failure is appended to the strbuf specified using the 'err' argument.\n \n Similarly, `commit_lock_file`, `commit_lock_file_to`, and\n `close_lock_file` return 0 on success. On failure they set `errno`\n@@ -111,7 +98,7 @@ appropriately, do their best to roll back the lockfile, and return -1.\n Flags\n -----\n \n-The following flags can be passed to `hold_lock_file_for_update` or\n+The following flag can be passed to `hold_lock_file_for_update` or\n `hold_lock_file_for_append`:\n \n LOCK_NO_DEREF::\n@@ -136,22 +123,16 @@ LOCK_OUTSIDE_REPOSITORY:\n For example, this flag should be set when locking a configuration\n file in the user's home directory.\n \n-LOCK_DIE_ON_ERROR::\n-\n-\tIf a lock is already taken for the file, `die()` with an error\n-\tmessage. If this option is not specified, trying to lock a\n-\tfile that is already locked returns -1 to the caller.\n-\n-\n The functions\n -------------\n \n hold_lock_file_for_update::\n \n \tTake a pointer to `struct lock_file`, the path of the file to\n-\tbe locked (e.g. `$GIT_DIR/index`) and a flags argument (see\n-\tabove). Attempt to create a lockfile for the destination and\n-\treturn the file descriptor for writing to the file.\n+\tbe locked (e.g. `$GIT_DIR/index`), a flags argument (see\n+\tabove), and a strbuf for error messages. Attempt to create a\n+\tlockfile for the destination and return the file descriptor for\n+\twriting to the file.\n \n hold_lock_file_for_append::\n \ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex cda438f..07626fb 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -3711,6 +3711,7 @@ static void build_fake_ancestor(struct patch *list, const char *filename)\n \tstruct patch *patch;\n \tstruct index_state result = { NULL };\n \tstatic struct lock_file lock;\n+\tstruct strbuf err = STRBUF_INIT;\n \n \t/* Once we start supporting the reverse patch, it may be\n \t * worth showing the new sha1 prefix, but until then...\n@@ -3748,11 +3749,13 @@ static void build_fake_ancestor(struct patch *list, const char *filename)\n \t\t\tdie (\"Could not add %s to temporary index\", name);\n \t}\n \n-\thold_lock_file_for_update(&lock, filename, LOCK_DIE_ON_ERROR);\n+\tif (hold_lock_file_for_update(&lock, filename, 0, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \tif (write_locked_index(&result, &lock, COMMIT_LOCK))\n \t\tdie (\"Could not write temporary index to %s\", filename);\n \n \tdiscard_index(&result);\n+\tstrbuf_release(&err);\n }\n \n static void stat_patch_list(struct patch *patch)\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex edc4493..f64024c 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -468,10 +468,11 @@ static const char *prepare_index(int argc, const char **argv, const char *prefix\n \tif (write_locked_index(&the_index, &index_lock, CLOSE_LOCK))\n \t\tdie(_(\"unable to write new_index file\"));\n \n-\thold_lock_file_for_update(&false_lock,\n-\t\t\t\t git_path(\"next-index-%\"PRIuMAX,\n-\t\t\t\t\t (uintmax_t) getpid()),\n-\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tif (hold_lock_file_for_update(&false_lock,\n+\t\t\t\t git_path(\"next-index-%\"PRIuMAX,\n+\t\t\t\t\t (uintmax_t) getpid()), 0,\n+\t\t\t\t &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n \n \tcreate_base_index(current_head);\n \tadd_remove_files(&partial);\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex 005adbe..86e9eca 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -190,6 +190,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tstatic struct lock_file lock;\n \tchar my_host[128];\n \tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tstruct stat st;\n \tuintmax_t pid;\n \tFILE *fp;\n@@ -202,8 +203,9 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tif (gethostname(my_host, sizeof(my_host)))\n \t\tstrcpy(my_host, \"unknown\");\n \n-\tfd = hold_lock_file_for_update(&lock, git_path(\"gc.pid\"),\n-\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&lock, git_path(\"gc.pid\"), 0, &err);\n+\tif (fd < 0)\n+\t\tdie(\"%s\", err.buf);\n \tif (!force) {\n \t\tstatic char locking_host[128];\n \t\tint should_exit;\n@@ -231,6 +233,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \t\t\tif (fd >= 0)\n \t\t\t\trollback_lock_file(&lock);\n \t\t\t*ret_pid = pid;\n+\t\t\tstrbuf_release(&err);\n \t\t\treturn locking_host;\n \t\t}\n \t}\n@@ -245,6 +248,7 @@ static const char *lock_repo_for_gc(int force, pid_t* ret_pid)\n \tsigchain_push_common(remove_pidfile_on_signal);\n \tatexit(remove_pidfile);\n \n+\tstrbuf_release(&err);\n \treturn NULL;\n }\n \ndiff --git a/bundle.c b/bundle.c\nindex 2e2dbd5..db03782 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -417,9 +417,13 @@ int create_bundle(struct bundle_header *header, const char *path,\n \tbundle_to_stdout = !strcmp(path, \"-\");\n \tif (bundle_to_stdout)\n \t\tbundle_fd = 1;\n-\telse\n-\t\tbundle_fd = hold_lock_file_for_update(&lock, path,\n-\t\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\telse {\n+\t\tstruct strbuf err = STRBUF_INIT;\n+\t\tbundle_fd = hold_lock_file_for_update(&lock, path, 0, &err);\n+\t\tif (bundle_fd < 0)\n+\t\t\tdie(\"%s\", err.buf);\n+\t\tstrbuf_release(&err);\n+\t}\n \n \t/* write signature */\n \twrite_or_die(bundle_fd, bundle_signature, strlen(bundle_signature));\ndiff --git a/config.c b/config.c\nindex dacde5f..4117ff0 100644\n--- a/config.c\n+++ b/config.c\n@@ -1923,6 +1923,7 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \tint fd = -1, in_fd;\n \tint ret;\n \tstruct lock_file *lock = NULL;\n+\tstruct strbuf err = STRBUF_INIT;\n \tchar *filename_buf = NULL;\n \n \t/* parse-key returns negative; flip the sign to feed exit(3) */\n@@ -1941,14 +1942,9 @@ int git_config_set_multivar_in_file(const char *config_filename,\n \t */\n \tlock = xcalloc(1, sizeof(struct lock_file));\n \tfd = hold_lock_file_for_update(lock, config_filename,\n-\t\t\t\t LOCK_OUTSIDE_REPOSITORY);\n+\t\t\t\t LOCK_OUTSIDE_REPOSITORY, &err);\n \tif (fd < 0) {\n-\t\tstruct strbuf err = STRBUF_INIT;\n-\n-\t\tunable_to_lock_message(config_filename,\n-\t\t\t\t LOCK_OUTSIDE_REPOSITORY, errno, &err);\n \t\terror(\"%s\", err.buf);\n-\t\tstrbuf_release(&err);\n \t\tfree(store.key);\n \t\tret = CONFIG_NO_LOCK;\n \t\tgoto out_free;\n@@ -2126,6 +2122,7 @@ out_free:\n \tif (lock)\n \t\trollback_lock_file(lock);\n \tfree(filename_buf);\n+\tstrbuf_release(&err);\n \treturn ret;\n \n write_err_out:\n@@ -2203,6 +2200,7 @@ int git_config_rename_section_in_file(const char *config_filename,\n \tint ret = 0, remove = 0;\n \tchar *filename_buf = NULL;\n \tstruct lock_file *lock;\n+\tstruct strbuf err = STRBUF_INIT;\n \tint out_fd;\n \tchar buf[1024];\n \tFILE *config_file;\n@@ -2218,14 +2216,9 @@ int git_config_rename_section_in_file(const char *config_filename,\n \n \tlock = xcalloc(1, sizeof(struct lock_file));\n \tout_fd = hold_lock_file_for_update(lock, config_filename,\n-\t\t\t\t\t LOCK_OUTSIDE_REPOSITORY);\n+\t\t\t\t\t LOCK_OUTSIDE_REPOSITORY, &err);\n \tif (out_fd < 0) {\n-\t\tstruct strbuf err = STRBUF_INIT;\n-\n-\t\tunable_to_lock_message(config_filename,\n-\t\t\t\t LOCK_OUTSIDE_REPOSITORY, errno, &err);\n \t\tret = error(\"%s\", err.buf);\n-\t\tstrbuf_release(&err);\n \t\tgoto out;\n \t}\n \n@@ -2295,6 +2288,7 @@ unlock_and_out:\n \t\tret = error(\"could not commit config file %s\", config_filename);\n out:\n \tfree(filename_buf);\n+\tstrbuf_release(&err);\n \treturn ret;\n }\n \ndiff --git a/credential-store.c b/credential-store.c\nindex cd71156..beffa87 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -55,13 +55,17 @@ static void print_line(struct strbuf *buf)\n static void rewrite_credential_file(const char *fn, struct credential *c,\n \t\t\t\t struct strbuf *extra)\n {\n-\thold_lock_file_for_update(&credential_lock, fn,\n-\t\t\t\t LOCK_DIE_ON_ERROR | LOCK_OUTSIDE_REPOSITORY);\n+\tstruct strbuf err = STRBUF_INIT;\n+\n+\tif (hold_lock_file_for_update(&credential_lock, fn,\n+\t\t\t\t LOCK_OUTSIDE_REPOSITORY, &err) < 0);\n+\t\tdie(\"%s\", err.buf);\n \tif (extra)\n \t\tprint_line(extra);\n \tparse_credential_file(fn, c, NULL, print_line);\n \tif (commit_lock_file(&credential_lock) < 0)\n \t\tdie_errno(\"unable to commit credential store\");\n+\tstrbuf_release(&err);\n }\n \n static void store_credential(const char *fn, struct credential *c)\ndiff --git a/fast-import.c b/fast-import.c\nindex bf8faa9..736c37b 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1794,21 +1794,19 @@ static void dump_marks_helper(FILE *f,\n static void dump_marks(void)\n {\n \tstatic struct lock_file mark_lock;\n+\tstruct strbuf err = STRBUF_INIT;\n \tFILE *f;\n \n \tif (!export_marks_file)\n \t\treturn;\n \n \tif (hold_lock_file_for_update(&mark_lock, export_marks_file,\n-\t\t\t\t LOCK_OUTSIDE_REPOSITORY) < 0) {\n-\t\tstruct strbuf err = STRBUF_INIT;\n-\n-\t\tunable_to_lock_message(export_marks_file,\n-\t\t\t\t LOCK_OUTSIDE_REPOSITORY, errno, &err);\n+\t\t\t\t LOCK_OUTSIDE_REPOSITORY, &err) < 0) {\n \t\tfailure |= error(\"%s\", err.buf);\n \t\tstrbuf_release(&err);\n \t\treturn;\n \t}\n+\tstrbuf_release(&err);\n \n \tf = fdopen_lock_file(&mark_lock, \"w\");\n \tif (!f) {\ndiff --git a/lockfile.c b/lockfile.c\nindex 102584f..a79679b 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -98,8 +98,8 @@ static void resolve_symlink(struct strbuf *path)\n \tstrbuf_reset(&link);\n }\n \n-/* Make sure errno contains a meaningful value on error */\n-static int lock_file(struct lock_file *lk, const char *path, int flags)\n+static int lock_file(struct lock_file *lk, const char *path,\n+\t\t int flags, struct strbuf *err)\n {\n \tsize_t pathlen = strlen(path);\n \n@@ -134,16 +134,16 @@ static int lock_file(struct lock_file *lk, const char *path, int flags)\n \tstrbuf_addstr(&lk->filename, LOCK_SUFFIX);\n \tlk->fd = open(lk->filename.buf, O_RDWR | O_CREAT | O_EXCL, 0666);\n \tif (lk->fd < 0) {\n+\t\tunable_to_lock_message(path, flags, errno, err);\n \t\tstrbuf_reset(&lk->filename);\n \t\treturn -1;\n \t}\n \tlk->owner = getpid();\n \tlk->active = 1;\n \tif (adjust_shared_perm(lk->filename.buf)) {\n-\t\tint save_errno = errno;\n-\t\terror(\"cannot fix permission bits on %s\", lk->filename.buf);\n+\t\tstrbuf_addf(err, \"cannot fix permission bits on %s\",\n+\t\t\t lk->filename.buf);\n \t\trollback_lock_file(lk);\n-\t\terrno = save_errno;\n \t\treturn -1;\n \t}\n \treturn lk->fd;\n@@ -178,13 +178,10 @@ NORETURN void unable_to_lock_die(const char *path, int flags, int err)\n \tdie(\"%s\", buf.buf);\n }\n \n-/* This should return a meaningful errno on failure */\n-int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n+int hold_lock_file_for_update(struct lock_file *lk, const char *path,\n+\t\t\t int flags, struct strbuf *err)\n {\n-\tint fd = lock_file(lk, path, flags);\n-\tif (fd < 0 && (flags & LOCK_DIE_ON_ERROR))\n-\t\tunable_to_lock_die(path, flags, errno);\n-\treturn fd;\n+\treturn lock_file(lk, path, flags, err);\n }\n \n int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n@@ -192,14 +189,11 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n {\n \tint fd, orig_fd;\n \n-\tassert(!(flags & LOCK_DIE_ON_ERROR));\n \tassert(err && !err->len);\n \n-\tfd = lock_file(lk, path, flags);\n-\tif (fd < 0) {\n-\t\tunable_to_lock_message(path, flags, errno, err);\n+\tfd = lock_file(lk, path, flags, err);\n+\tif (fd < 0)\n \t\treturn fd;\n-\t}\n \n \torig_fd = open(path, O_RDONLY);\n \tif (orig_fd < 0) {\ndiff --git a/lockfile.h b/lockfile.h\nindex 779a992..6d0a9bb 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -68,14 +68,14 @@ struct lock_file {\n #define LOCK_SUFFIX \".lock\"\n #define LOCK_SUFFIX_LEN 5\n \n-#define LOCK_DIE_ON_ERROR 1\n-#define LOCK_NO_DEREF 2\n-#define LOCK_OUTSIDE_REPOSITORY 4\n+#define LOCK_NO_DEREF 1\n+#define LOCK_OUTSIDE_REPOSITORY 2\n \n extern void unable_to_lock_message(const char *path, int, int err,\n \t\t\t\t struct strbuf *buf);\n extern NORETURN void unable_to_lock_die(const char *path, int, int err);\n-extern int hold_lock_file_for_update(struct lock_file *, const char *path, int);\n+extern int hold_lock_file_for_update(struct lock_file *, const char *path,\n+\t\t\t\t int, struct strbuf *err);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path,\n \t\t\t\t int, struct strbuf *err);\n extern FILE *fdopen_lock_file(struct lock_file *, const char *mode);\ndiff --git a/read-cache.c b/read-cache.c\nindex 2ce1a76..229eea0 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1370,14 +1370,7 @@ static int read_index_extension(struct index_state *istate,\n \n int hold_locked_index(struct lock_file *lk, struct strbuf *err)\n {\n-\tconst char *path;\n-\tint fd;\n-\n-\tpath = get_index_file();\n-\tfd = hold_lock_file_for_update(lk, path, 0);\n-\tif (fd < 0)\n-\t\tunable_to_lock_message(path, 0, errno, err);\n-\treturn fd;\n+\treturn hold_lock_file_for_update(lk, get_index_file(), 0, err);\n }\n \n int read_index(struct index_state *istate)\ndiff --git a/refs.c b/refs.c\nindex 39e43cf..f39c29c 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2229,6 +2229,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\t\t\t\t const struct string_list *skip,\n \t\t\t\t\t int flags, int *type_p)\n {\n+\tstruct strbuf err = STRBUF_INIT;\n \tchar *ref_file;\n \tconst char *orig_refname = refname;\n \tstruct ref_lock *lock;\n@@ -2316,22 +2317,25 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n \t\tgoto error_return;\n \t}\n \n-\tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags);\n+\tlock->lock_fd = hold_lock_file_for_update(lock->lk, ref_file, lflags, &err);\n \tif (lock->lock_fd < 0) {\n-\t\tif (errno == ENOENT && --attempts_remaining > 0)\n+\t\tif (errno == ENOENT && --attempts_remaining > 0) {\n \t\t\t/*\n \t\t\t * Maybe somebody just deleted one of the\n \t\t\t * directories leading to ref_file. Try\n \t\t\t * again:\n \t\t\t */\n+\t\t\tstrbuf_reset(&err);\n \t\t\tgoto retry;\n-\t\telse\n-\t\t\tunable_to_lock_die(ref_file, lflags, errno);\n+\t\t}\n+\t\tdie(\"%s\", err.buf);\n \t}\n+\tstrbuf_release(&err);\n \treturn old_sha1 ? verify_lock(lock, old_sha1, mustexist) : lock;\n \n error_return:\n \tunlock_ref(lock);\n+\tstrbuf_release(&err);\n \terrno = last_errno;\n \treturn NULL;\n }\n@@ -2375,10 +2379,9 @@ int lock_packed_refs(struct strbuf *err)\n {\n \tstruct packed_ref_cache *packed_ref_cache;\n \n-\tif (hold_lock_file_for_update(&packlock, git_path(\"packed-refs\"), 0) < 0) {\n-\t\tunable_to_lock_message(git_path(\"packed-refs\"), 0, errno, err);\n+\tif (hold_lock_file_for_update(&packlock,\n+\t\t\t\t git_path(\"packed-refs\"), 0, err) < 0)\n \t\treturn -1;\n-\t}\n \n \t/*\n \t * Get the current packed-refs while holding the lock. If the\ndiff --git a/rerere.c b/rerere.c\nindex 044e279..c1eaadd 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -600,6 +600,7 @@ static int is_rerere_enabled(void)\n \n int setup_rerere(struct string_list *merge_rr, int flags)\n {\n+\tstruct strbuf err = STRBUF_INIT;\n \tint fd;\n \n \tgit_rerere_config();\n@@ -609,9 +610,11 @@ int setup_rerere(struct string_list *merge_rr, int flags)\n \tif (flags & (RERERE_AUTOUPDATE|RERERE_NOAUTOUPDATE))\n \t\trerere_autoupdate = !!(flags & RERERE_AUTOUPDATE);\n \tmerge_rr_path = git_pathdup(\"MERGE_RR\");\n-\tfd = hold_lock_file_for_update(&write_lock, merge_rr_path,\n-\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&write_lock, merge_rr_path, 0, &err);\n+\tif (fd < 0)\n+\t\tdie(\"%s\", err.buf);\n \tread_rr(merge_rr);\n+\tstrbuf_release(&err);\n \treturn fd;\n }\n \ndiff --git a/sequencer.c b/sequencer.c\nindex 3b3a869..c0d8864 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -204,14 +204,17 @@ static void print_advice(int show_hint, struct replay_opts *opts)\n static void write_message(struct strbuf *msgbuf, const char *filename)\n {\n \tstatic struct lock_file msg_file;\n+\tstruct strbuf err = STRBUF_INIT;\n \n-\tint msg_fd = hold_lock_file_for_update(&msg_file, filename,\n-\t\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tint msg_fd = hold_lock_file_for_update(&msg_file, filename, 0, &err);\n+\tif (msg_fd < 0)\n+\t\tdie(\"%s\", err.buf);\n \tif (write_in_full(msg_fd, msgbuf->buf, msgbuf->len) < 0)\n \t\tdie_errno(_(\"Could not write to %s\"), filename);\n \tstrbuf_release(msgbuf);\n \tif (commit_lock_file(&msg_file) < 0)\n \t\tdie(_(\"Error wrapping up %s\"), filename);\n+\tstrbuf_release(&err);\n }\n \n static struct tree *empty_tree(void)\n@@ -854,14 +857,18 @@ static void save_head(const char *head)\n \tconst char *head_file = git_path(SEQ_HEAD_FILE);\n \tstatic struct lock_file head_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(&head_lock, head_file, LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&head_lock, head_file, 0, &err);\n+\tif (fd < 0)\n+\t\tdie(\"%s\", err.buf);\n \tstrbuf_addf(&buf, \"%s\\n\", head);\n \tif (write_in_full(fd, buf.buf, buf.len) < 0)\n \t\tdie_errno(_(\"Could not write to %s\"), head_file);\n \tif (commit_lock_file(&head_lock) < 0)\n \t\tdie(_(\"Error wrapping up %s.\"), head_file);\n+\tstrbuf_release(&err);\n }\n \n static int reset_for_rollback(const unsigned char *sha1)\n@@ -935,9 +942,12 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \tconst char *todo_file = git_path(SEQ_TODO_FILE);\n \tstatic struct lock_file todo_lock;\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(&todo_lock, todo_file, LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&todo_lock, todo_file, 0, &err);\n+\tif (fd < 0)\n+\t\tdie(\"%s\", err.buf);\n \tif (format_todo(&buf, todo_list, opts) < 0)\n \t\tdie(_(\"Could not format %s.\"), todo_file);\n \tif (write_in_full(fd, buf.buf, buf.len) < 0) {\n@@ -949,6 +959,7 @@ static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)\n \t\tdie(_(\"Error wrapping up %s.\"), todo_file);\n \t}\n \tstrbuf_release(&buf);\n+\tstrbuf_release(&err);\n }\n \n static void save_opts(struct replay_opts *opts)\ndiff --git a/shallow.c b/shallow.c\nindex cdd0775..4a30a77 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -259,10 +259,13 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n \t\t\t const struct sha1_array *extra)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tint fd;\n \n-\tfd = hold_lock_file_for_update(shallow_lock, git_path(\"shallow\"),\n-\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(shallow_lock,\n+\t\t\t\t git_path(\"shallow\"), 0, &err);\n+\tif (fd < 0)\n+\t\tdie(\"%s\", err.buf);\n \tcheck_shallow_file_for_update();\n \tif (write_shallow_commits(&sb, 0, extra)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n@@ -276,6 +279,7 @@ void setup_alternate_shallow(struct lock_file *shallow_lock,\n \t\t */\n \t\t*alternate_shallow_file = \"\";\n \tstrbuf_release(&sb);\n+\tstrbuf_release(&err);\n }\n \n static int advertise_shallow_grafts_cb(const struct commit_graft *graft, void *cb)\n@@ -301,6 +305,7 @@ void prune_shallow(int show_only)\n {\n \tstatic struct lock_file shallow_lock;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tstruct strbuf err = STRBUF_INIT;\n \tint fd;\n \n \tif (show_only) {\n@@ -308,8 +313,10 @@ void prune_shallow(int show_only)\n \t\tstrbuf_release(&sb);\n \t\treturn;\n \t}\n-\tfd = hold_lock_file_for_update(&shallow_lock, git_path(\"shallow\"),\n-\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tfd = hold_lock_file_for_update(&shallow_lock,\n+\t\t\t\t git_path(\"shallow\"), 0, &err);\n+\tif (fd < 0)\n+\t\tdie(\"%s\", err.buf);\n \tcheck_shallow_file_for_update();\n \tif (write_shallow_commits_1(&sb, 0, NULL, SEEN_ONLY)) {\n \t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n@@ -321,6 +328,7 @@ void prune_shallow(int show_only)\n \t\trollback_lock_file(&shallow_lock);\n \t}\n \tstrbuf_release(&sb);\n+\tstrbuf_release(&err);\n }\n \n struct trace_key trace_shallow = TRACE_KEY_INIT(SHALLOW);\n"},{"id":"252933","messageId":"20141203052654.GW6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 13/14] lockfile: remove unused function 'unable_to_lock_die'","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:26:54Z","receivedAt":"2014-12-03T05:26:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The old callers now use the message passed back by\nhold_lock_file_for_update / hold_lock_file_for_append instead of\ntrying to interpret errno.\n\nSigned-off-by: Jonathan Nieder \n---\n lockfile.c | 8 --------\n lockfile.h | 1 -\n 2 files changed, 9 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex a79679b..8d8d5ed 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -170,14 +170,6 @@ void unable_to_lock_message(const char *path, int flags, int err,\n \t}\n }\n \n-NORETURN void unable_to_lock_die(const char *path, int flags, int err)\n-{\n-\tstruct strbuf buf = STRBUF_INIT;\n-\n-\tunable_to_lock_message(path, flags, err, &buf);\n-\tdie(\"%s\", buf.buf);\n-}\n-\n int hold_lock_file_for_update(struct lock_file *lk, const char *path,\n \t\t\t int flags, struct strbuf *err)\n {\ndiff --git a/lockfile.h b/lockfile.h\nindex 6d0a9bb..b4d29a3 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -73,7 +73,6 @@ struct lock_file {\n \n extern void unable_to_lock_message(const char *path, int, int err,\n \t\t\t\t struct strbuf *buf);\n-extern NORETURN void unable_to_lock_die(const char *path, int, int err);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path,\n \t\t\t\t int, struct strbuf *err);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path,\n"},{"id":"252934","messageId":"20141203052734.GX6527@google.com","threadId":"37988","inReplyTo":"20141203050217.GJ6527@google.com","subject":"[PATCH 14/14] lockfile: make 'unable_to_lock_message' private","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T05:27:34Z","receivedAt":"2014-12-03T05:27:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"The old external callers now use the message passed back by\nhold_lock_file_for_update / hold_lock_file_for_append instead of\ntrying to interpret errno.\n\nSigned-off-by: Jonathan Nieder \n---\nThat's the end of the series. Thanks for reading.\n\nThoughts?\n\n lockfile.c | 42 +++++++++++++++++++++---------------------\n lockfile.h | 2 --\n 2 files changed, 21 insertions(+), 23 deletions(-)\n\ndiff --git a/lockfile.c b/lockfile.c\nindex 8d8d5ed..7121370 100644\n--- a/lockfile.c\n+++ b/lockfile.c\n@@ -98,6 +98,27 @@ static void resolve_symlink(struct strbuf *path)\n \tstrbuf_reset(&link);\n }\n \n+static void unable_to_lock_message(const char *path, int flags, int err,\n+\t\t\t struct strbuf *buf)\n+{\n+\tif (err != EEXIST) {\n+\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s\",\n+\t\t\t absolute_path(path), strerror(err));\n+\t} else if (flags & LOCK_OUTSIDE_REPOSITORY) {\n+\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n+\t\t \"If no other git process is currently running, this probably means\\n\"\n+\t\t \"another git process crashed earlier. Make sure no other git process\\n\"\n+\t\t \"is running and remove the file manually to continue.\",\n+\t\t\t absolute_path(path), strerror(err));\n+\t} else {\n+\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n+\t\t \"If no other git process is currently running, this probably means a\\n\"\n+\t\t \"git process crashed in this repository earlier. Make sure no other git\\n\"\n+\t\t \"process is running and remove the file manually to continue.\",\n+\t\t\t absolute_path(path), strerror(err));\n+\t}\n+}\n+\n static int lock_file(struct lock_file *lk, const char *path,\n \t\t int flags, struct strbuf *err)\n {\n@@ -149,27 +170,6 @@ static int lock_file(struct lock_file *lk, const char *path,\n \treturn lk->fd;\n }\n \n-void unable_to_lock_message(const char *path, int flags, int err,\n-\t\t\t struct strbuf *buf)\n-{\n-\tif (err != EEXIST) {\n-\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s\",\n-\t\t\t absolute_path(path), strerror(err));\n-\t} else if (flags & LOCK_OUTSIDE_REPOSITORY) {\n-\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n-\t\t \"If no other git process is currently running, this probably means\\n\"\n-\t\t \"another git process crashed earlier. Make sure no other git process\\n\"\n-\t\t \"is running and remove the file manually to continue.\",\n-\t\t\t absolute_path(path), strerror(err));\n-\t} else {\n-\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n-\t\t \"If no other git process is currently running, this probably means a\\n\"\n-\t\t \"git process crashed in this repository earlier. Make sure no other git\\n\"\n-\t\t \"process is running and remove the file manually to continue.\",\n-\t\t\t absolute_path(path), strerror(err));\n-\t}\n-}\n-\n int hold_lock_file_for_update(struct lock_file *lk, const char *path,\n \t\t\t int flags, struct strbuf *err)\n {\ndiff --git a/lockfile.h b/lockfile.h\nindex b4d29a3..02e26fe 100644\n--- a/lockfile.h\n+++ b/lockfile.h\n@@ -71,8 +71,6 @@ struct lock_file {\n #define LOCK_NO_DEREF 1\n #define LOCK_OUTSIDE_REPOSITORY 2\n \n-extern void unable_to_lock_message(const char *path, int, int err,\n-\t\t\t\t struct strbuf *buf);\n extern int hold_lock_file_for_update(struct lock_file *, const char *path,\n \t\t\t\t int, struct strbuf *err);\n extern int hold_lock_file_for_append(struct lock_file *, const char *path,\n"},{"id":"252935","messageId":"547EA918.30003@web.de","threadId":"37988","inReplyTo":"20141203051459.GN6527@google.com","subject":"Re: [PATCH 04/14] hold_lock_file_for_append: pass error message back through a strbuf","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2014-12-03T06:09:28Z","receivedAt":"2014-12-03T06:09:28Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 12/03/2014 06:14 AM, Jonathan Nieder wrote:\n> This way, the code does not need to carefully safeguard errno to allow\n> callers to print a reasonable error message when they choose to do\n> some cleanup before die()-ing.\n>\n> Fixes a bug waiting to happen where copy_fd would clobber the errno\n> passed back via hold_lock_file_for_append from read() or write() when\n> flags did not contain LOCK_DIE_ON_ERROR. Luckily the only caller uses\n> flags == LOCK_DIE_ON_ERROR, avoiding that bug in practice.\n>\n> Reported-by: Ronnie Sahlberg \n> Signed-off-by: Jonathan Nieder \n> ---\n> The advertised bugfix.\n>\n> lockfile.c | 29 ++++++++++-------------------\n> lockfile.h | 3 ++-\n> sha1_file.c | 7 ++++++-\n> 3 files changed, 18 insertions(+), 21 deletions(-)\n>\n> diff --git a/lockfile.c b/lockfile.c\n> index b3020f3..8685c68 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -179,45 +179,36 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n> \treturn fd;\n> }\n> \n> -int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n> +int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n> +\t\t\t int flags, struct strbuf *err)\n> {\n> \tint fd, orig_fd;\n> -\tstruct strbuf err = STRBUF_INIT;\n> +\n> +\tassert(!(flags & LOCK_DIE_ON_ERROR));\n> +\tassert(err && !err->len);\n> \nWhat do I miss ?\nIn the old code we die() printing the errno when LOCK_DIE_ON_ERRORis set.\nNow we die() in an assert() or two ?\nAnd should'nt we assert in strbuf_addf() instead ?\n\nAnd in general, does the commit message deserve an update?\n\n>Luckily the only caller uses flags == LOCK_DIE_ON_ERROR,\nThe first impression was: Why do we not simplify the code and remove\nthe flag completely ?\nThis feels somewhat like maintaining dead code, which may be used later.\n(Unless it will be used in later commit, and the we could mention it here)\n"},{"id":"252936","messageId":"20141203070411.GA24344@google.com","threadId":"37988","inReplyTo":"547EA918.30003@web.de","subject":"Re: [PATCH 04/14] hold_lock_file_for_append: pass error message back through a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T07:04:11Z","receivedAt":"2014-12-03T07:04:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Torsten Bögershausen wrote:\n> On 12/03/2014 06:14 AM, Jonathan Nieder wrote:\n\n>> --- a/lockfile.c\n>> +++ b/lockfile.c\n>> @@ -179,45 +179,36 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n>> \treturn fd;\n>> }\n>> -int hold_lock_file_for_append(struct lock_file *lk, const char *path, int flags)\n>> +int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n>> +\t\t\t int flags, struct strbuf *err)\n>> {\n>> \tint fd, orig_fd;\n>> -\tstruct strbuf err = STRBUF_INIT;\n>> +\n>> +\tassert(!(flags & LOCK_DIE_ON_ERROR));\n>> +\tassert(err && !err->len);\n>\n> What do I miss ?\n> In the old code we die() printing the errno when LOCK_DIE_ON_ERRORis set.\n\nThe assertion documents an assumption that no caller will set the\nLOCK_DIE_ON_ERROR flag bit. A later patch in the series eliminates\nthat flag bit completely.\n\n> Now we die() in an assert() or two ?\n\nassert() is not safe to use for anything other than documenting\nassumptions. It can't be relied on to exit (let alone to exit with a\nnice message like die()). So the die() that was removed and assert()\nthat this patch adds have different purposes.\n\n> And should'nt we assert in strbuf_addf() instead ?\n\nstrbuf_addf can be used to append to a nonempty strbuf.\n\n[...]\n> (Unless it will be used in later commit, and the we could mention it here)\n\nGood idea. I'll add to the commit message if rerolling.\n\nThanks,\nJonathan\n"},{"id":"252973","messageId":"20141203185303.GB31609@google.com","threadId":"37988","inReplyTo":"20141203052602.GV6527@google.com","subject":"Re: [PATCH 12/14] hold_lock_file_for_update: pass error message back through a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T18:53:03Z","receivedAt":"2014-12-03T18:53:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> --- a/credential-store.c\n> +++ b/credential-store.c\n> @@ -55,13 +55,17 @@ static void print_line(struct strbuf *buf)\n> static void rewrite_credential_file(const char *fn, struct credential *c,\n> \t\t\t\t struct strbuf *extra)\n> {\n> -\thold_lock_file_for_update(&credential_lock, fn,\n> -\t\t\t\t LOCK_DIE_ON_ERROR | LOCK_OUTSIDE_REPOSITORY);\n> +\tstruct strbuf err = STRBUF_INIT;\n> +\n> +\tif (hold_lock_file_for_update(&credential_lock, fn,\n> +\t\t\t\t LOCK_OUTSIDE_REPOSITORY, &err) < 0);\n> +\t\tdie(\"%s\", err.buf);\n\nI forgot to squash this in. Sorry for the confusion. (I'm planning to\nreroll with this and the commit clarification tboegi mentioned --- other\ncomments welcome before then.)\n\n-- >8 --\nSubject: fixup! hold_lock_file_for_update: pass error message back through a strbuf\n\nSigned-off-by: Jonathan Nieder \n---\n credential-store.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/credential-store.c b/credential-store.c\nindex beffa87..2d5d92f 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -58,7 +58,7 @@ static void rewrite_credential_file(const char *fn, struct credential *c,\n \tstruct strbuf err = STRBUF_INIT;\n \n \tif (hold_lock_file_for_update(&credential_lock, fn,\n-\t\t\t\t LOCK_OUTSIDE_REPOSITORY, &err) < 0);\n+\t\t\t\t LOCK_OUTSIDE_REPOSITORY, &err) < 0)\n \t\tdie(\"%s\", err.buf);\n \tif (extra)\n \t\tprint_line(extra);\n"},{"id":"252974","messageId":"xmqq4mtcimzn.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141203051220.GL6527@google.com","subject":"Re: [PATCH 02/14] add_to_alternates_file: respect GIT_OBJECT_DIRECTORY","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T18:53:16Z","receivedAt":"2014-12-03T18:53:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> The objects directory is spelled as get_object_directory(), not\n> git_path(\"objects\"). Some other code still hard-codes the objects/\n> directory name, so in the long term we may want to get rid of the\n> pretense of support for GIT_OBJECT_DIRECTORY altogether, but this\n> makes the code more consistent for now.\n>\n> While at it, split variable declarations from the rest of the\n> function. This makes the function a little easier to read, at the\n> cost of some vertical space.\n>\n> Signed-off-by: Jonathan Nieder \n> ---\n> This is mainly for consistency / cleanliness. I wouldn't mind\n> dropping it.\n\nI think this is a worthwhile thing to do, but you probably would\nwant to Cc author of nd/multiple-work-trees topic which would need\nto address similar concern.\n\n> The rest of 'git clone' is not careful about paying attention to\n> GIT_OBJECT_DIRECTORY either. I suspect GIT_OBJECT_DIRECTORY was a bit\n> of a failed experiment.\n\n\"clone\" is a bit special in that it needs to be careful about the\n\"local\" case. Does GIT_OBJECT_DIRECTORY (or GIT_DIR for that matter)\nrefer to the source or to the destination?\n\n> sha1_file.c | 12 +++++++++---\n> 1 file changed, 9 insertions(+), 3 deletions(-)\n>\n> diff --git a/sha1_file.c b/sha1_file.c\n> index d7f1838..e1945e2 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -403,9 +403,15 @@ void read_info_alternates(const char * relative_base, int depth)\n> \n> void add_to_alternates_file(const char *reference)\n> {\n> -\tstruct lock_file *lock = xcalloc(1, sizeof(struct lock_file));\n> -\tint fd = hold_lock_file_for_append(lock, git_path(\"objects/info/alternates\"), LOCK_DIE_ON_ERROR);\n> -\tchar *alt = mkpath(\"%s\\n\", reference);\n> +\tstruct lock_file *lock;\n> +\tint fd;\n> +\tchar *alt;\n> +\n> +\tlock = xcalloc(1, sizeof(*lock));\n> +\tfd = hold_lock_file_for_append(lock, mkpath(\"%s/info/alternates\",\n> +\t\t\t\t\t\t get_object_directory()),\n> +\t\t\t\t LOCK_DIE_ON_ERROR);\n> +\talt = mkpath(\"%s\\n\", reference);\n> \twrite_or_die(fd, alt, strlen(alt));\n> \tif (commit_lock_file(lock))\n> \t\tdie(\"could not close alternates file\");\n"},{"id":"252975","messageId":"xmqqzjb4h823.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141203051344.GM6527@google.com","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T19:01:08Z","receivedAt":"2014-12-03T19:01:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> -extern int copy_fd(int ifd, int ofd);\n> +extern int copy_fd(int ifd, int ofd, struct strbuf *err);\n\nIt is not limited to this single function, but what contract do we\nenvision this \"error messages are given back to the caller via\nstrbuf\" convention should give between the callers and the callee?\n\nFor example, is it a bug in the callee to touch \"err\" when there is\nno error to report? Another example is if we should allow the\ncallers to pass NULL there when they do not care about the nature of\nthe error (e.g. \"git cmd -q\").\n\nThere may be other rules we want to enforce consistently across\nfunctions that adopt this convention.\n\nThe change in this patch looks sensible.\n\nThanks.\n"},{"id":"252977","messageId":"xmqqvblsh5ca.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141203052124.GS6527@google.com","subject":"Re: [PATCH 09/14] config: use message from lockfile API when locking config file fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T19:59:49Z","receivedAt":"2014-12-03T19:59:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Up to this point the patches looked sensible (I am not saying that\nthe remainder is junk---I haven't looked at them yet is all I am\nsaying).\n\nThanks.\n"},{"id":"252978","messageId":"xmqqr3wgh57t.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141203051344.GM6527@google.com","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T20:02:30Z","receivedAt":"2014-12-03T20:02:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> This way, callers can put the message in context or even avoid\n> printing the message altogether.\n>\n> Currently hold_lock_file_for_append tries to save errno in order to\n> produce a meaningful message about the failure and tries to print a\n> second message using errno. Unfortunately the errno it uses is not\n> meaningful because copy_fd makes no effort to set a meaningful errno\n> value. This change is preparation for simplifying the\n> hold_lock_file_for_append behavior.\n>\n> No user-visible change intended yet.\n>\n> Signed-off-by: Jonathan Nieder \n> ---\n> The title feature.\n\nBy the way, this seems to address the same thing as sb/copy-fd-errno\ntopic that has been cooking in 'pu'? Should we drop that other\ntopic and use this one instead?\n"},{"id":"252979","messageId":"CAPig+cS6Z=UW80sn6ru15W29Vko-f2g4CUq0EQtWGU7ZjoJcog@mail.gmail.com","threadId":"37988","inReplyTo":"20141203051016.GK6527@google.com","subject":"Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-12-03T20:10:13Z","receivedAt":"2014-12-03T20:10:13Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 3, 2014 at 12:10 AM, Jonathan Nieder wrote:\n> When preparing an error message in a strbuf, it can be convenient\n> to add a formatted string to the beginning:\n>\n> if (transaction_commit(&t, err)) {\n> strbuf_prefixf(err, \"cannot fetch '%s': \", remotename);\n> return -1;\n> }\n>\n> The new strbuf_prefixf is like strbuf_addf, except it writes its\n> result to the beginning of a strbuf instead of the end.\n>\n> The current implementation uses strlen(strfmt(fmt, ...)) extra bytes\n> at the end of the strbuf as temporary scratch space for convenience\n> and simplicity. A later patch can optimize if needed.\n>\n> Signed-off-by: Jonathan Nieder \n> ---\n> diff --git a/strbuf.c b/strbuf.c\n> index 0346e74..3f4aaa3 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -219,6 +219,22 @@ void strbuf_addf(struct strbuf *sb, const char *fmt, ...)\n> va_end(ap);\n> }\n>\n> +void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)\n> +{\n> + va_list ap;\n> + size_t pos, len;\n> +\n> + pos = sb->len;\n> +\n> + va_start(ap, fmt);\n> + strbuf_vaddf(sb, fmt, ap);\n> + va_end(ap);\n> +\n> + len = sb->len - pos;\n> + strbuf_insert(sb, 0, sb->buf + pos, len);\n> + strbuf_remove(sb, pos + len, len);\n\nWould a strbuf_setlen(sb, pos), rather than strbuf_remove(), make it\nclearer to the reader that this is merely performing a truncation?\n\n> +}\n> +\n> static void add_lines(struct strbuf *out,\n> const char *prefix1,\n> const char *prefix2,\n"},{"id":"252980","messageId":"20141203201416.GY6527@google.com","threadId":"37988","inReplyTo":"CAPig+cS6Z=UW80sn6ru15W29Vko-f2g4CUq0EQtWGU7ZjoJcog@mail.gmail.com","subject":"Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T20:14:16Z","receivedAt":"2014-12-03T20:14:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Eric Sunshine wrote:\n> On Wed, Dec 3, 2014 at 12:10 AM, Jonathan Nieder wrote:\n\n>> +void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)\n>> +{\n>> + va_list ap;\n>> + size_t pos, len;\n>> +\n>> + pos = sb->len;\n>> +\n>> + va_start(ap, fmt);\n>> + strbuf_vaddf(sb, fmt, ap);\n>> + va_end(ap);\n>> +\n>> + len = sb->len - pos;\n>> + strbuf_insert(sb, 0, sb->buf + pos, len);\n>> + strbuf_remove(sb, pos + len, len);\n>\n> Would a strbuf_setlen(sb, pos), rather than strbuf_remove(), make it\n> clearer to the reader that this is merely performing a truncation?\n\nGood idea. I'll do that.\n\nThanks,\nJonathan\n"},{"id":"252981","messageId":"20141203201636.GZ6527@google.com","threadId":"37988","inReplyTo":"xmqqvblsh5ca.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 09/14] config: use message from lockfile API when locking config file fails","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T20:16:36Z","receivedAt":"2014-12-03T20:16:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> Up to this point the patches looked sensible (I am not saying that\n> the remainder is junk---I haven't looked at them yet is all I am\n> saying).\n\nNice to hear. Thanks for looking it over.\n\nJonathan\n"},{"id":"252982","messageId":"20141203201808.GA6527@google.com","threadId":"37988","inReplyTo":"xmqqr3wgh57t.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T20:18:08Z","receivedAt":"2014-12-03T20:18:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> By the way, this seems to address the same thing as sb/copy-fd-errno\n> topic that has been cooking in 'pu'? Should we drop that other\n> topic and use this one instead?\n\nYes, please.\n\nI'll give it an hour or two to collect more comments and then send a\nreroll reflecting them.\n\nThanks,\nJonathan\n"},{"id":"252983","messageId":"CAGZ79kYsH3QgghjTnxczxBA85fUizgtYm-qTz28rspyhRs=big@mail.gmail.com","threadId":"37988","inReplyTo":"xmqqr3wgh57t.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-03T20:20:09Z","receivedAt":"2014-12-03T20:20:09Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 3, 2014 at 12:02 PM, Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>\n>> This way, callers can put the message in context or even avoid\n>> printing the message altogether.\n>>\n>> Currently hold_lock_file_for_append tries to save errno in order to\n>> produce a meaningful message about the failure and tries to print a\n>> second message using errno. Unfortunately the errno it uses is not\n>> meaningful because copy_fd makes no effort to set a meaningful errno\n>> value. This change is preparation for simplifying the\n>> hold_lock_file_for_append behavior.\n>>\n>> No user-visible change intended yet.\n>>\n>> Signed-off-by: Jonathan Nieder \n>> ---\n>> The title feature.\n>\n> By the way, this seems to address the same thing as sb/copy-fd-errno\n> topic that has been cooking in 'pu'? Should we drop that other\n> topic and use this one instead?\n>\n\nThis series makes the code more readable and maintainable in the end\nas we don't need to hunt down the path of when the errno is changed or\naccessed.\nThe currently cooking sb/copy-fd-errno is more of a hot fix, while\nthis series is fixing the problem more at the basic level and more in\nlong term.\n\nI'd be happy if this replaces the currently cooking version.\n"},{"id":"252984","messageId":"20141203202805.GB6527@google.com","threadId":"37988","inReplyTo":"xmqqzjb4h823.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T20:28:05Z","receivedAt":"2014-12-03T20:28:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> -extern int copy_fd(int ifd, int ofd);\n>> +extern int copy_fd(int ifd, int ofd, struct strbuf *err);\n>\n> It is not limited to this single function, but what contract do we\n> envision this \"error messages are given back to the caller via\n> strbuf\" convention should give between the callers and the callee?\n\nGood point. I'll add a Documentation/technical/api-error-handling.txt\ndescribing die(), error(), warning(), and the strbuf-based method.\n\nThanks,\nJonathan\n"},{"id":"252985","messageId":"20141203204227.GA26810@google.com","threadId":"37988","inReplyTo":"20141203052734.GX6527@google.com","subject":"Re: [PATCH 14/14] lockfile: make 'unable_to_lock_message' private","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-03T20:42:27Z","receivedAt":"2014-12-03T20:42:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Tue, Dec 02, 2014 at 09:27:34PM -0800, Jonathan Nieder wrote:\n> The old external callers now use the message passed back by\n> hold_lock_file_for_update / hold_lock_file_for_append instead of\n> trying to interpret errno.\n> \n> Signed-off-by: Jonathan Nieder \n> ---\n> That's the end of the series. Thanks for reading.\n> \n> Thoughts?\n\nI reviewed the whole series and seems fine with me.\n\n> \n> lockfile.c | 42 +++++++++++++++++++++---------------------\n> lockfile.h | 2 --\n> 2 files changed, 21 insertions(+), 23 deletions(-)\n> \n> diff --git a/lockfile.c b/lockfile.c\n> index 8d8d5ed..7121370 100644\n> --- a/lockfile.c\n> +++ b/lockfile.c\n> @@ -98,6 +98,27 @@ static void resolve_symlink(struct strbuf *path)\n> \tstrbuf_reset(&link);\n> }\n> \n> +static void unable_to_lock_message(const char *path, int flags, int err,\n> +\t\t\t struct strbuf *buf)\n> +{\n> +\tif (err != EEXIST) {\n> +\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s\",\n> +\t\t\t absolute_path(path), strerror(err));\n> +\t} else if (flags & LOCK_OUTSIDE_REPOSITORY) {\n> +\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n> +\t\t \"If no other git process is currently running, this probably means\\n\"\n> +\t\t \"another git process crashed earlier. Make sure no other git process\\n\"\n> +\t\t \"is running and remove the file manually to continue.\",\n> +\t\t\t absolute_path(path), strerror(err));\n> +\t} else {\n> +\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n> +\t\t \"If no other git process is currently running, this probably means a\\n\"\n> +\t\t \"git process crashed in this repository earlier. Make sure no other git\\n\"\n> +\t\t \"process is running and remove the file manually to continue.\",\n> +\t\t\t absolute_path(path), strerror(err));\n> +\t}\n> +}\n> +\n> static int lock_file(struct lock_file *lk, const char *path,\n> \t\t int flags, struct strbuf *err)\n> {\n> @@ -149,27 +170,6 @@ static int lock_file(struct lock_file *lk, const char *path,\n> \treturn lk->fd;\n> }\n> \n> -void unable_to_lock_message(const char *path, int flags, int err,\n> -\t\t\t struct strbuf *buf)\n> -{\n> -\tif (err != EEXIST) {\n> -\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s\",\n> -\t\t\t absolute_path(path), strerror(err));\n> -\t} else if (flags & LOCK_OUTSIDE_REPOSITORY) {\n> -\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n> -\t\t \"If no other git process is currently running, this probably means\\n\"\n> -\t\t \"another git process crashed earlier. Make sure no other git process\\n\"\n> -\t\t \"is running and remove the file manually to continue.\",\n> -\t\t\t absolute_path(path), strerror(err));\n> -\t} else {\n> -\t\tstrbuf_addf(buf, \"Unable to create '%s.lock': %s.\\n\\n\"\n> -\t\t \"If no other git process is currently running, this probably means a\\n\"\n> -\t\t \"git process crashed in this repository earlier. Make sure no other git\\n\"\n> -\t\t \"process is running and remove the file manually to continue.\",\n> -\t\t\t absolute_path(path), strerror(err));\n> -\t}\n> -}\n> -\n> int hold_lock_file_for_update(struct lock_file *lk, const char *path,\n> \t\t\t int flags, struct strbuf *err)\n> {\n> diff --git a/lockfile.h b/lockfile.h\n> index b4d29a3..02e26fe 100644\n> --- a/lockfile.h\n> +++ b/lockfile.h\n> @@ -71,8 +71,6 @@ struct lock_file {\n> #define LOCK_NO_DEREF 1\n> #define LOCK_OUTSIDE_REPOSITORY 2\n> \n> -extern void unable_to_lock_message(const char *path, int, int err,\n> -\t\t\t\t struct strbuf *buf);\n> extern int hold_lock_file_for_update(struct lock_file *, const char *path,\n> \t\t\t\t int, struct strbuf *err);\n> extern int hold_lock_file_for_append(struct lock_file *, const char *path,\n"},{"id":"252986","messageId":"20141203210031.GA6631@peff.net","threadId":"37988","inReplyTo":"xmqqzjb4h823.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-03T21:00:31Z","receivedAt":"2014-12-03T21:00:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 03, 2014 at 11:01:08AM -0800, Junio C Hamano wrote:\n\n> Jonathan Nieder writes:\n> \n> > -extern int copy_fd(int ifd, int ofd);\n> > +extern int copy_fd(int ifd, int ofd, struct strbuf *err);\n> \n> It is not limited to this single function, but what contract do we\n> envision this \"error messages are given back to the caller via\n> strbuf\" convention should give between the callers and the callee?\n> \n> For example, is it a bug in the callee to touch \"err\" when there is\n> no error to report? Another example is if we should allow the\n> callers to pass NULL there when they do not care about the nature of\n> the error (e.g. \"git cmd -q\").\n> \n> There may be other rules we want to enforce consistently across\n> functions that adopt this convention.\n\nIt seems like we are really re-designing the error-handling chain here.\nMaybe it is worth taking a step back and thinking about our overall\nstrategy.\n\nWhy do we dislike errno? Because it is global state, and it is easy for\nit to get stomped by unrelated operations. Another reason is that it\ncarries no parameters. You see \"ENOENT\", but you do not have any context\n(e.g., which file).\n\nWhat's good about errno? It is machine-readable (i.e., callers can check\nfor ENOENT, as opposed to text in a strbuf). It does not require any\nallocation. Besides making it slightly more robust, it removes any\nresponsibility for resource cleanup from the caller. It's globalness is\nalso convenient; you do not need to add an extra parameter to each\nfunction to handle errors.\n\nSo what are some alternatives?\n\nPassing back \"-errno\" instead of \"-1\" helps with the stomping issue (and\nwithout adding extra parameters). It also retains machine-readability\n(which I'll call just readability from now on). But it doesn't help\nwith context, or better messaging.\n\nYour solution adds a strbuf. That helps with context and stomping, but\nloses readability and adds allocation.\n\nIf we changed the strbuf to a fixed-size buffer, that would help the\nallocation issue. Some messages might be truncated, but it seems\nunlikely in practice. It still loses readability, though.\n\nWhat about a struct that has an errno-like value _and_ a fixed-size\nbuffer? I'm thinking something like:\n\n struct error {\n\tint code;\n\tchar msg[1024];\n };\n\n /* caller who wants to print the message; no need to free message */\n struct error err;\n if (copy_fd(from, to, &err))\n\treturn error(\"%s\", err.msg);\n\n /* caller who does not; they can pass NULL */\n if (copy_fd(from, to, NULL))\n\treturn -1;\n\n /* or they can use it to grab the errno value */\n struct error err;\n if (copy_fd(from, to, &err)) {\n\tif (err.code == EPIPE)\n\t\texit(141);\n\treturn -1;\n }\n\n /* function which generates error */\n void read_foo(const char *fn, struct error *err)\n {\n\tif (open(fn, O_RDONLY))\n\t\treturn mkerror(err, errno, \"unable to open %s\", fn);\n\t... do other stuff ...\n\treturn 0;\n }\n\n /* helper function for generating errors */\n int mkerror(struct error *err, int code, const char *fmt, ...)\n {\n\tva_list ap;\n\tint len;\n\n\tif (!err)\n\t\treturn;\n\n\terr->code = code;\n\tva_start(ap, fmt);\n\tlen = vsnprintf(err->msg, sizeof(err->msg), fmt, ap);\n\tva_end(ap);\n\n\tif (code)\n\t\tsnprintf(err->msg + len, sizeof(err->msg) - len,\n\t\t\t \": %s\", strerror(code));\n\n\treturn -1;\n }\n\nYou can also take the machine-readable thing a step further, like:\n\n struct error {\n\tint code;\n\tchar param1[1024];\n\tchar param2[1024];\n\t/* 2 parameters should be big enough for anyone, right? */\n }\n\nand then generate the message on the fly when printing. This gives the\ncallers more information. But it also means defining a constant for\n\"code\" for every error message, which is annoying. Libraries often do\nthis, but I do not think we need to go that far here.\n\n-Peff\n"},{"id":"252990","messageId":"20141203213858.GC6527@google.com","threadId":"37988","inReplyTo":"20141203210031.GA6631@peff.net","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T21:38:58Z","receivedAt":"2014-12-03T21:38:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> What about a struct that has an errno-like value _and_ a fixed-size\n> buffer? I'm thinking something like:\n>\n> struct error {\n> \tint code;\n> \tchar msg[1024];\n> };\n\nMy experience with errno is that it is very hard to anticipate what\ngranularity to use with error codes, especially if the error code is\ngoing to (in some contexts) determine the message printed to the user.\nIn practice, it is easier to come up with a message at the error\ndetection site and use a generic \"something happened\" error code\nexcept in those places where the caller is going to act on specific\nerror types that need to be distinguished.\n\nPassing a message back through a strbuf leaves room for such\ninterpretations for the int return value.\n\nThe \"< 0 means error\" convention gives room to use different exit\ncodes for the errors that need to be programmatically distinguished.\nFor example, the ref transaction API uses this to distinguish D/F\nconflicts from other errors, while still returning an error message\nthrough a strbuf output parameter.\n\nThis doesn't help with functions that want to return a pointer (and\nNULL on error). For those, we can use an int output parameter, copy\nthe kernel's err_ptr trick, or use other sentinels for recoverable\nerrors that the caller wants to know about. It hasn't come up in\npractice enough for me to be motivated to want to choose between\nthose.\n\n> If we changed the strbuf to a fixed-size buffer, that would help the\n> allocation issue. Some messages might be truncated, but it seems\n> unlikely in practice. It still loses readability, though.\n\nI don't like the idea of having to choose a size in advance and not\nbeing able to fit more detailed (perhaps language-specific, and\nincluding user-specified input) messages lest the buffer overflow.\nThe allocation of a variable-sized buffer is a small overhead that I\ndon't mind incurring on error. In the non-error case, the caller\ndoesn't actually have to free the buffer, and if they choose to, the\noverhead incurred is that of free(NULL)'.\n\nMy two cents,\nJonathan\n"},{"id":"252991","messageId":"xmqqiohswgsq.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141203201808.GA6527@google.com","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T21:43:17Z","receivedAt":"2014-12-03T21:43:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>\n>> By the way, this seems to address the same thing as sb/copy-fd-errno\n>> topic that has been cooking in 'pu'? Should we drop that other\n>> topic and use this one instead?\n>\n> Yes, please.\n\nOK.\n\nIt felt strange that two people in a same office stomping on each\nother's toes, which I interpreted as an apparent lack of\ncommunication, but perhaps there was an off-line communication\nbetween you two with an understanding that this will supersede the\nother one, which I probably failed to spot in the cover letter.\n\n> I'll give it an hour or two to collect more comments and then send a\n> reroll reflecting them.\n\nMake that one revolution of earth, perhaps?\n\ne.g. http://thread.gmane.org/gmane.comp.version-control.git/259831/focus=260349\n"},{"id":"252992","messageId":"xmqqbnnkwgpg.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141203051016.GK6527@google.com","subject":"Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T21:45:15Z","receivedAt":"2014-12-03T21:45:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> When preparing an error message in a strbuf, it can be convenient\n> to add a formatted string to the beginning:\n>\n> \tif (transaction_commit(&t, err)) {\n> \t\tstrbuf_prefixf(err, \"cannot fetch '%s': \", remotename);\n> \t\treturn -1;\n> \t}\n\nI am somewhat unhappy with this justification, as it is not very\nclear why \"cannot fetch '%s': \" must come at the beginning without\nknowing what kind of string transaction_commit() is expected to add\nto err when it is called. It could be a sign that the convention\nfor transaction_commit() to report its errors is screwed up, and\nnot a sign that prefixf is necessary (not that I think that is the\ncase---there is not enough explanation here to decide).\n\n> +void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)\n> +{\n> +\tva_list ap;\n> +\tsize_t pos, len;\n> +\n> +\tpos = sb->len;\n> +\n> +\tva_start(ap, fmt);\n> +\tstrbuf_vaddf(sb, fmt, ap);\n> +\tva_end(ap);\n> +\n> +\tlen = sb->len - pos;\n> +\tstrbuf_insert(sb, 0, sb->buf + pos, len);\n> +\tstrbuf_remove(sb, pos + len, len);\n> +}\n\nThis indeed is strange to read; it would be more straightforward to\nuse a second strbuf for temporary storage you need to do this,\ninstead of using the tail-end of the original strbuf and shuffling\nbytes around.\n\nIn any case, instead of this:\n\n\tstruct strbuf tc_err = STRBUF_INIT;\n if (transaction_commit(&t, &tc_err)) {\n\t\tstrbuf_addf(err, \"cannot fetch '%s': %s\", remotename, \n\t\t\ttc_err.buf); \n\t\tstrbuf_release(&tc_err);\n\t\treturn -1;\n\t}\t \n\nyou can use the four-line version you cited above, which might be an\nimprovement. I dunno if it is such a big deal, though.\n"},{"id":"252993","messageId":"20141203215125.GE6527@google.com","threadId":"37988","inReplyTo":"xmqqiohswgsq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T21:51:26Z","receivedAt":"2014-12-03T21:51:26Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n>> Junio C Hamano wrote:\n\n>>> By the way, this seems to address the same thing as sb/copy-fd-errno\n>>> topic that has been cooking in 'pu'? Should we drop that other\n>>> topic and use this one instead?\n>>\n>> Yes, please.\n>\n> OK.\n>\n> It felt strange that two people in a same office stomping on each\n> other's toes\n\nI just did a bad job of explaining the context in the cover letter.\n\nI wrote this series at Stefan's request (see the message that the\ncover letter is a response to for context[1]).\n\nHope that helps,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/259695/focus=259710\nand http://thread.gmane.org/gmane.comp.version-control.git/259695/focus=259703\n"},{"id":"252994","messageId":"20141203215918.GF6527@google.com","threadId":"37988","inReplyTo":"xmqqbnnkwgpg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T21:59:18Z","receivedAt":"2014-12-03T21:59:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> +void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)\n>> +{\n>> +\tva_list ap;\n>> +\tsize_t pos, len;\n>> +\n>> +\tpos = sb->len;\n>> +\n>> +\tva_start(ap, fmt);\n>> +\tstrbuf_vaddf(sb, fmt, ap);\n>> +\tva_end(ap);\n>> +\n>> +\tlen = sb->len - pos;\n>> +\tstrbuf_insert(sb, 0, sb->buf + pos, len);\n>> +\tstrbuf_remove(sb, pos + len, len);\n>> +}\n>\n> This indeed is strange to read; it would be more straightforward to\n> use a second strbuf for temporary storage you need to do this,\n> instead of using the tail-end of the original strbuf and shuffling\n> bytes around.\n\nI could do that. It's less efficient but if the prevailing sentiment\nis that it's worth it then I don't mind.\n\nWould adding a comment to the implementation of strbuf_prefixf help?\n\n> In any case, instead of this:\n>\n> \tstruct strbuf tc_err = STRBUF_INIT;\n> if (transaction_commit(&t, &tc_err)) {\n> \t\tstrbuf_addf(err, \"cannot fetch '%s': %s\", remotename,\n> \t\t\ttc_err.buf);\n> \t\tstrbuf_release(&tc_err);\n> \t\treturn -1;\n> \t}\n>\n> you can use the four-line version you cited above, which might be an\n> improvement.\n\nYes, that's the idea.\n\nI'll do the tc_err thing, since I'm not getting a clear feeling that\nI've offered enough motivation for the prefixf approach.\n\nJonathan\n"},{"id":"252997","messageId":"20141203220942.GG6527@google.com","threadId":"37988","inReplyTo":"20141203215918.GF6527@google.com","subject":"Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T22:09:42Z","receivedAt":"2014-12-03T22:09:42Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n> Junio C Hamano wrote:\n\n>> In any case, instead of this:\n>>\n>> \tstruct strbuf tc_err = STRBUF_INIT;\n>> if (transaction_commit(&t, &tc_err)) {\n>> \t\tstrbuf_addf(err, \"cannot fetch '%s': %s\", remotename,\n>> \t\t\ttc_err.buf);\n>> \t\tstrbuf_release(&tc_err);\n>> \t\treturn -1;\n>> \t}\n>>\n>> you can use the four-line version you cited above, which might be an\n>> improvement.\n>\n> Yes, that's the idea.\n>\n> I'll do the tc_err thing, since I'm not getting a clear feeling that\n> I've offered enough motivation for the prefixf approach.\n\nThe tc_err approach looks like this.\n\nSigned-off-by: Jonathan Nieder \n\n lockfile.c | 25 +++++++++++++++----------\n 1 file changed, 15 insertions(+), 10 deletions(-)\n\ndiff --git i/lockfile.c w/lockfile.c\nindex 8685c68..4e2dfa3 100644\n--- i/lockfile.c\n+++ w/lockfile.c\n@@ -182,15 +182,16 @@ int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags)\n int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n \t\t\t int flags, struct strbuf *err)\n {\n-\tint fd, orig_fd;\n+\tstruct strbuf tc_err = STRBUF_INIT;\n+\tint fd, orig_fd = -1;\n \n \tassert(!(flags & LOCK_DIE_ON_ERROR));\n-\tassert(err && !err->len);\n+\tassert(err);\n \n \tfd = lock_file(lk, path, flags);\n \tif (fd < 0) {\n \t\tunable_to_lock_message(path, errno, err);\n-\t\treturn fd;\n+\t\tgoto fail;\n \t}\n \n \torig_fd = open(path, O_RDONLY);\n@@ -198,18 +199,22 @@ int hold_lock_file_for_append(struct lock_file *lk, const char *path,\n \t\tif (errno != ENOENT) {\n \t\t\tstrbuf_addf(err, \"cannot open '%s' for copying: %s\",\n \t\t\t\t path, strerror(errno));\n-\t\t\trollback_lock_file(lk);\n-\t\t\treturn -1;\n+\t\t\tgoto fail;\n \t\t}\n-\t} else if (copy_fd(orig_fd, fd, err)) {\n-\t\tstrbuf_prefixf(err, \"cannot copy '%s': \", path);\n-\t\tclose(orig_fd);\n-\t\trollback_lock_file(lk);\n-\t\treturn -1;\n+\t} else if (copy_fd(orig_fd, fd, &tc_err)) {\n+\t\tstrbuf_addf(err, \"cannot copy '%s': %s\", path, tc_err.buf);\n+\t\tgoto fail;\n \t} else {\n \t\tclose(orig_fd);\n \t}\n+\tstrbuf_release(&tc_err);\n \treturn fd;\n+fail:\n+\tif (orig_fd >= 0)\n+\t\tclose(orig_fd);\n+\trollback_lock_file(lk);\n+\tstrbuf_release(&tc_err);\n+\treturn -1;\n }\n \n FILE *fdopen_lock_file(struct lock_file *lk, const char *mode)\n"},{"id":"253000","messageId":"xmqq388wwe58.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141203215918.GF6527@google.com","subject":"Re: [PATCH 01/14] strbuf: introduce strbuf_prefixf()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T22:40:35Z","receivedAt":"2014-12-03T22:40:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>> Jonathan Nieder writes:\n>\n>>> +void strbuf_prefixf(struct strbuf *sb, const char *fmt, ...)\n>>> +{\n>>> +\tva_list ap;\n>>> +\tsize_t pos, len;\n>>> +\n>>> +\tpos = sb->len;\n>>> +\n>>> +\tva_start(ap, fmt);\n>>> +\tstrbuf_vaddf(sb, fmt, ap);\n>>> +\tva_end(ap);\n>>> +\n>>> +\tlen = sb->len - pos;\n>>> +\tstrbuf_insert(sb, 0, sb->buf + pos, len);\n>>> +\tstrbuf_remove(sb, pos + len, len);\n>>> +}\n>>\n>> This indeed is strange to read; it would be more straightforward to\n>> use a second strbuf for temporary storage you need to do this,\n>> instead of using the tail-end of the original strbuf and shuffling\n>> bytes around.\n>\n> I could do that. It's less efficient but if the prevailing sentiment\n> is that it's worth it then I don't mind.\n>\n> Would adding a comment to the implementation of strbuf_prefixf help?\n\nPerhaps.\n\nThe reason why it felt strange to me was primarily because this was\na short-hand way of writing something like this in the caller:\n\n\tif (transaction_commit(&t, err)) {\n\t\tstruct strbuf scratch = STRBUF_INIT;\n\t\tstrbuf_addf(&scratch, \"cannot fetch '%s': \", remotename);\n\t\tstrbuf_splice(err, 0, 0, sctach.buf, scratch.len);\n strbuf_reset(&scratch);\n\t}\n\nComing from that point of view, it looked strange not to be using a\nseparate scratch area; that's all.\n"},{"id":"253002","messageId":"xmqqvblsuy2e.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141203051911.GP6527@google.com","subject":"Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T23:13:13Z","receivedAt":"2014-12-03T23:13:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> diff --git a/refs.c b/refs.c\n> index 917f8fc..39e43cf 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2326,7 +2326,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n> \t\t\t */\n> \t\t\tgoto retry;\n> \t\telse\n> -\t\t\tunable_to_lock_die(ref_file, errno);\n> +\t\t\tunable_to_lock_die(ref_file, lflags, errno);\n> \t}\n\nThis has unfortunate interaction with 06839515 (lock_ref_sha1_basic:\ndo not die on locking errors, 2014-11-19). The fact that the helper\nunable-to-lock-message() is now hidden inside lockfile.c does not\nhelp, either.\n"},{"id":"253003","messageId":"20141203232424.GI6527@google.com","threadId":"37988","inReplyTo":"xmqqvblsuy2e.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T23:24:24Z","receivedAt":"2014-12-03T23:24:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> diff --git a/refs.c b/refs.c\n>> index 917f8fc..39e43cf 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2326,7 +2326,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>> \t\t\t */\n>> \t\t\tgoto retry;\n>> \t\telse\n>> -\t\t\tunable_to_lock_die(ref_file, errno);\n>> +\t\t\tunable_to_lock_die(ref_file, lflags, errno);\n>> \t}\n>\n> This has unfortunate interaction with 06839515 (lock_ref_sha1_basic:\n> do not die on locking errors, 2014-11-19).\n\nI'll add that patch to the series. 06839515 becomes a little simpler\nwith the updated API.\n\nThanks,\nJonathan\n"},{"id":"253004","messageId":"xmqqr3wguxhv.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"xmqqvblsuy2e.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T23:25:32Z","receivedAt":"2014-12-03T23:25:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Nieder writes:\n>\n>> diff --git a/refs.c b/refs.c\n>> index 917f8fc..39e43cf 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -2326,7 +2326,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname,\n>> \t\t\t */\n>> \t\t\tgoto retry;\n>> \t\telse\n>> -\t\t\tunable_to_lock_die(ref_file, errno);\n>> +\t\t\tunable_to_lock_die(ref_file, lflags, errno);\n>> \t}\n>\n> This has unfortunate interaction with 06839515 (lock_ref_sha1_basic:\n> do not die on locking errors, 2014-11-19). The fact that the helper\n> unable-to-lock-message() is now hidden inside lockfile.c does not\n> help, either.\n\nI tried to merge the 14-patch series with obvious fix-ups after\ndropping the rerere abortion change you sent separately and in\nduplicate and also dropping sb/copy-fd, but I've ran out of patience\nwith this step, at least for today's integration cycle. Should we\nalso drop jk/lock-ref-sha1-basec-return-errors topic as well?\n\nThe 14-patch series may have been internally consistent and its\nindividual patches, when each of them was taken alone by itself, may\nhave made sense, but it appears that the aggregated whole these\nseparate topics took their root from is inconsistent with itself\nin minor ways like this here and there X-<.\n"},{"id":"253005","messageId":"20141203232951.GJ6527@google.com","threadId":"37988","inReplyTo":"xmqqr3wguxhv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T23:29:51Z","receivedAt":"2014-12-03T23:29:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> I tried to merge the 14-patch series with obvious fix-ups after\n> dropping the rerere abortion change you sent separately and in\n> duplicate and also dropping sb/copy-fd, but I've ran out of patience\n> with this step, at least for today's integration cycle. Should we\n> also drop jk/lock-ref-sha1-basec-return-errors topic as well?\n\nI don't mind adding it to the series. Or feel free to hold off on\npicking up this 14-patch series until tomorrow's reroll. (Helping\nwith today's integration was why I was thinking of re-sending. I\nshould have been clearer about saying that I prefer you don't pick\nthis up yet when I decided to give reviewers a little more time.)\n\n> The 14-patch series may have been internally consistent and its\n> individual patches, when each of them was taken alone by itself, may\n> have made sense, but it appears that the aggregated whole these\n> separate topics took their root from is inconsistent with itself\n> in minor ways like this here and there X-<.\n\nI don't follow. It's normal for an API change to affect code that\nuses the API --- what inconsistency are you talking about here?\n\nPuzzled,\nJonathan\n"},{"id":"253007","messageId":"xmqqmw74uwx0.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141203232951.GJ6527@google.com","subject":"Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T23:38:03Z","receivedAt":"2014-12-03T23:38:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> The 14-patch series may have been internally consistent and its\n>> individual patches, when each of them was taken alone by itself, may\n>> have made sense, but it appears that the aggregated whole these\n>> separate topics took their root from is inconsistent with itself\n>> in minor ways like this here and there X-<.\n>\n> I don't follow. It's normal for an API change to affect code that\n> uses the API --- what inconsistency are you talking about here?\n\nI was under the impression that the purpose of the series was to\npropose an API update to be used together with the remainder of the\nsystem, not just \"update code in master, breaking unstated set of\ntopics and leaving them behind without updating them for now\".\n\nSuch a series is perfectly fine as a feeler to see if people are\nhappy with the updated API that it does not completely cover the\ntopics in flight, but I wouldn't have had to waste time trying to\nfigure out what you are doing differently to cause other topics in\nflight to break if such a feeler series were labeled clearly as\nRFC/PATCH, and/or \"these other topics need to be adjusted to this\nnew API when this series settles\", or somesuch.\n"},{"id":"253008","messageId":"20141203234105.GK6527@google.com","threadId":"37988","inReplyTo":"xmqqmw74uwx0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-03T23:41:05Z","receivedAt":"2014-12-03T23:41:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> I was under the impression that the purpose of the series was to\n> propose an API update to be used together with the remainder of the\n> system, not just \"update code in master, breaking unstated set of\n> topics and leaving them behind without updating them for now\".\n\nGot it --- before the reroll I will try a test-merge against 'next'\nand 'pu' (and likewise when sending future patches).\n"},{"id":"253009","messageId":"xmqqiohsuwnk.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"xmqqmw74uwx0.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-03T23:43:43Z","receivedAt":"2014-12-03T23:43:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Nieder writes:\n>\n>>> The 14-patch series may have been internally consistent and its\n>>> individual patches, when each of them was taken alone by itself, may\n>>> have made sense, but it appears that the aggregated whole these\n>>> separate topics took their root from is inconsistent with itself\n>>> in minor ways like this here and there X-<.\n>>\n>> I don't follow. It's normal for an API change to affect code that\n>> uses the API --- what inconsistency are you talking about here?\n>\n> I was under the impression that the purpose of the series was to\n> propose an API update to be used together with the remainder of the\n> system, not just \"update code in master, breaking unstated set of\n> topics and leaving them behind without updating them for now\".\n> ...\n\nAbout the \"inconsistent with itself\" bit.\n\nThe other topic is a piece from the long-ish series by Ronnie, and\nsomehow I had assumed this 14-patch series also originated from\nanother part of that series, especially given that it is largely\nabout refs. That's where \"itself\" came from.\n\nLooking at it again, this errno thing is more or less separate and\nindependent, so your \"I don't follow\" is perfectly understandable to\nme now. Sorry about being grumpy.\n"},{"id":"253010","messageId":"20141203235742.GA7387@peff.net","threadId":"37988","inReplyTo":"20141203232951.GJ6527@google.com","subject":"Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-03T23:57:42Z","receivedAt":"2014-12-03T23:57:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 03, 2014 at 03:29:51PM -0800, Jonathan Nieder wrote:\n\n> Junio C Hamano wrote:\n> \n> > I tried to merge the 14-patch series with obvious fix-ups after\n> > dropping the rerere abortion change you sent separately and in\n> > duplicate and also dropping sb/copy-fd, but I've ran out of patience\n> > with this step, at least for today's integration cycle. Should we\n> > also drop jk/lock-ref-sha1-basec-return-errors topic as well?\n> \n> I don't mind adding it to the series.\n\nPlease do add it in, rather than dropping it. It fixes a real racy\ncondition in which \"pack-refs --prune\" wants to lock a ref only to prune\nit, and it's OK to continue on if the lock fails (without that patch,\npack-refs dies prematurely).\n\n-Peff\n"},{"id":"253017","messageId":"20141204030133.GA16345@google.com","threadId":"37988","inReplyTo":"xmqqzjb4h823.fsf@gitster.dls.corp.google.com","subject":"[PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-04T03:01:33Z","receivedAt":"2014-12-04T03:01:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Signed-off-by: Jonathan Nieder \n---\nJunio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> -extern int copy_fd(int ifd, int ofd);\n>> +extern int copy_fd(int ifd, int ofd, struct strbuf *err);\n>\n> It is not limited to this single function, but what contract do we\n> envision this \"error messages are given back to the caller via\n> strbuf\" convention should give between the callers and the callee?\n\nHere's a draft for documentation on that.\n\n Documentation/technical/api-error-handling.txt | 75 ++++++++++++++++++++++++++\n 1 file changed, 75 insertions(+)\n create mode 100644 Documentation/technical/api-error-handling.txt\n\ndiff --git a/Documentation/technical/api-error-handling.txt b/Documentation/technical/api-error-handling.txt\nnew file mode 100644\nindex 0000000..fc68db1\n--- /dev/null\n+++ b/Documentation/technical/api-error-handling.txt\n@@ -0,0 +1,75 @@\n+Error reporting in git\n+======================\n+\n+`die`, `usage`, `error`, and `warning` report errors of various\n+kinds.\n+\n+- `die` is for fatal application errors. It prints a message to\n+ the user and exits with status 128.\n+\n+- `usage` is for errors in command line usage. After printing its\n+ message, it exits with status 129. (See also `usage_with_options`\n+ in the link:api-parse-options.html[parse-options API].)\n+\n+- `error` is for non-fatal library errors. It prints a message\n+ to the user and returns -1 for convenience in signaling the error\n+ to the caller.\n+\n+- `warning` is for reporting situations that probably should not\n+ occur but which the user (and Git) can continue to work around\n+ without running into too many problems. Like `error`, it\n+ returns -1 after reporting the situation to the caller.\n+\n+Customizable error handlers\n+---------------------------\n+\n+The default behavior of `die` and `error` is to write a message to\n+stderr and then exit or return as appropriate. This behavior can be\n+overridden using `set_die_routine` and `set_error_routine`. For\n+example, \"git daemon\" uses set_die_routine to write the reason `die`\n+was called to syslog before exiting.\n+\n+Library errors\n+--------------\n+\n+Functions return a negative integer on error. Details beyond that\n+vary from function to function:\n+\n+- Some functions return -1 for all errors. Others return a more\n+ specific value depending on how the caller might want to react\n+ to the error.\n+\n+- Some functions report the error to stderr with `error`,\n+ while others leave that for the caller to do.\n+\n+- errno is not meaningful on return from most functions (except\n+ for thin wrappers for system calls).\n+\n+Check the function's API documentation to be sure.\n+\n+Caller-handled errors\n+---------------------\n+\n+An increasing number of functions take a parameter 'struct strbuf *err'.\n+On error, such functions append a message about what went wrong to the\n+'err' strbuf. The message is meant to be complete enough to be passed\n+to `die` or `error` as-is. For example:\n+\n+\tif (ref_transaction_commit(transaction, &err))\n+\t\tdie(\"%s\", err.buf);\n+\n+The 'err' parameter will be untouched if no error occured, so multiple\n+function calls can be chained:\n+\n+\tt = ref_transaction_begin(&err);\n+\tif (!t ||\n+\t ref_transaction_update(t, \"HEAD\", ..., &err) ||\n+\t ret_transaction_commit(t, &err))\n+\t\tdie(\"%s\", err.buf);\n+\n+The 'err' parameter must be a pointer to a valid strbuf. To silence\n+a message, pass a strbuf that is explicitly ignored:\n+\n+\tif (thing_that_can_fail_in_an_ignorable_way(..., &err))\n+\t\t/* This failure is okay. */\n+\t\tstrbuf_reset(&err);\n"},{"id":"253022","messageId":"xmqqbnnkufmt.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141203232951.GJ6527@google.com","subject":"Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-04T05:51:22Z","receivedAt":"2014-12-04T05:51:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Junio C Hamano wrote:\n>\n>> I tried to merge the 14-patch series with obvious fix-ups after\n>> dropping the rerere abortion change you sent separately and in\n>> duplicate and also dropping sb/copy-fd, but I've ran out of patience\n>> with this step, at least for today's integration cycle. Should we\n>> also drop jk/lock-ref-sha1-basec-return-errors topic as well?\n>\n> I don't mind adding it to the series....\n\nWe may want to see it done the other way around, though. That is,\nto allow the pack-refs race fix, which was reviewed and has been\ncooking, graduate without having to depend on this API rewrite, so\nthat we may be able to merge it down even to v2.2.x maintenance\ntrack.\n\nThanks.\n"},{"id":"253025","messageId":"20141204075920.GA27142@peff.net","threadId":"37988","inReplyTo":"20141203213858.GC6527@google.com","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-04T07:59:20Z","receivedAt":"2014-12-04T07:59:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 03, 2014 at 01:38:58PM -0800, Jonathan Nieder wrote:\n\n> > What about a struct that has an errno-like value _and_ a fixed-size\n> > buffer? I'm thinking something like:\n> >\n> > struct error {\n> > \tint code;\n> > \tchar msg[1024];\n> > };\n> \n> My experience with errno is that it is very hard to anticipate what\n> granularity to use with error codes, especially if the error code is\n> going to (in some contexts) determine the message printed to the user.\n> In practice, it is easier to come up with a message at the error\n> detection site and use a generic \"something happened\" error code\n> except in those places where the caller is going to act on specific\n> error types that need to be distinguished.\n\nYeah, I agree. I think error.msg there would be the main use, and\nerror.code is just gravy. I agree it could simply come back through the\ninteger return value, too.\n\n> The \"< 0 means error\" convention gives room to use different exit\n> codes for the errors that need to be programmatically distinguished.\n> For example, the ref transaction API uses this to distinguish D/F\n> conflicts from other errors, while still returning an error message\n> through a strbuf output parameter.\n\nYup. One nice thing about stuffing it into the error struct is that it\nsaves the caller from declaring a separate variable (assuming they need\n\"err\" anyway to collect the message). I.e.:\n\n struct error err;\n\n if (some_func(fd, &err))\n\treact_to(err.code);\n\nas opposed to:\n\n struct error err;\n int ret;\n\n ret = some_func(fd, &err);\n if (ret)\n\treact_to(ret);\n\nBut I think it's a minority of cases where we care about the specific\nvalue anyway, so it's probably not a big deal either way.\n\n> > If we changed the strbuf to a fixed-size buffer, that would help the\n> > allocation issue. Some messages might be truncated, but it seems\n> > unlikely in practice. It still loses readability, though.\n> \n> I don't like the idea of having to choose a size in advance and not\n> being able to fit more detailed (perhaps language-specific, and\n> including user-specified input) messages lest the buffer overflow.\n\nIs that really a problem in practice? Is a message larger than 1024\ncharacters actually readable? It would have to be broken across many\nlines, and then I think we are no longer dealing with an error message,\nbut some advice (which probably shouldn't go through this mechanism\nanyway).\n\n> The allocation of a variable-sized buffer is a small overhead that I\n> don't mind incurring on error. In the non-error case, the caller\n> doesn't actually have to free the buffer, and if they choose to, the\n> overhead incurred is that of free(NULL)'.\n\nI don't care at all about overhead. I care about extra work on the part\nof the caller to avoid a leak. It turns:\n\n if (some_func(fd, &err))\n\treturn error(\"%s\", err.msg);\n\ninto:\n\n if (some_func(fd, &err)) {\n\terror(\"%s\", err.buf);\n\tstrbuf_release(&err);\n\treturn -1;\n }\n\nIt may make things a little easier when an intermediate function has\nto further munge the message. For example, your 04/14 uses\nstrbuf_prefixf. But that can also be expressed in a buffer world as:\n\n /* \"parent_err\" is passed to us from caller, \"err\" is a local buffer */\n if (copy_fd(orig, fd, &err))\n\treturn mkerror(&parent_err, \"cannot copy '%s': %s\", path, err.msg);\n\nStrbufs make more complicated munging possible, but I'd expect prefixing\nto be the most common case (except for not munging at all, which I think\nis even more common).\n\nI dunno. I just hate to see a system where it's very easy for callers to\nintroduce memory leaks by forgetting a strbuf_release, or that bogs\ncallers down with error-handling boilerplate.\n\n-Peff\n"},{"id":"253041","messageId":"CAGZ79kby-RdwzgXWvVgD42uiEis7SCnuAdSjO2EL03uN4s=LgA@mail.gmail.com","threadId":"37988","inReplyTo":"20141204075920.GA27142@peff.net","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-12-04T08:36:46Z","receivedAt":"2014-12-04T08:36:46Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"> Your solution adds a strbuf. That helps with context and stomping, but\n> loses readability and adds allocation.\n\n> If we changed the strbuf to a fixed-size buffer, that would help the\n> allocation issue. Some messages might be truncated, but it seems\n> unlikely in practice. It still loses readability, though.\n\n> What about a struct that has an errno-like value _and_ a fixed-size\n> buffer? I'm thinking something like:\n\nWhat do you mean by the allocation being an issue?\nWe're only populating the error buffer in the error case, so you're\nnot talking about performance/speed I'd assume?\nAs error handling breaks in the least expected ways, I'd rather go\nwith well tested string buffer codes there?\n"},{"id":"253042","messageId":"20141204090406.GA27455@peff.net","threadId":"37988","inReplyTo":"CAGZ79kby-RdwzgXWvVgD42uiEis7SCnuAdSjO2EL03uN4s=LgA@mail.gmail.com","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-04T09:04:06Z","receivedAt":"2014-12-04T09:04:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 04, 2014 at 12:36:46AM -0800, Stefan Beller wrote:\n\n> > Your solution adds a strbuf. That helps with context and stomping, but\n> > loses readability and adds allocation.\n> \n> > If we changed the strbuf to a fixed-size buffer, that would help the\n> > allocation issue. Some messages might be truncated, but it seems\n> > unlikely in practice. It still loses readability, though.\n> \n> > What about a struct that has an errno-like value _and_ a fixed-size\n> > buffer? I'm thinking something like:\n> \n> What do you mean by the allocation being an issue?\n\nI mean that the caller has to take care of releasing the memory. This\nadds boilerplate to the caller, and is a potential source of leaks.\n\n> We're only populating the error buffer in the error case, so you're\n> not talking about performance/speed I'd assume?\n\nNo, I don't care about performance here. Only code maintainability.\n\n> As error handling breaks in the least expected ways, I'd rather go\n> with well tested string buffer codes there?\n\nWe'd still be building on snprintf. And with a known-size buffer, we can\nwrap the formatting so that the callers don't even have to care (see the\nmkerror example I posted).\n\n-Peff\n"},{"id":"253066","messageId":"20141204175654.GC9992@google.com","threadId":"37988","inReplyTo":"xmqqbnnkufmt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 06/14] lockfile: introduce flag for locks outside .git","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-04T17:56:54Z","receivedAt":"2014-12-04T17:56:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> We may want to see it done the other way around, though. That is,\n> to allow the pack-refs race fix, which was reviewed and has been\n> cooking, graduate without having to depend on this API rewrite, so\n> that we may be able to merge it down even to v2.2.x maintenance\n> track.\n\nGood idea. When I send out the reroll of this API change today, it\nwill be based on top of master + that patch.\n\nThanks,\nJonathan\n"},{"id":"253130","messageId":"xmqqy4qnq9m2.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141204030133.GA16345@google.com","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-04T23:27:17Z","receivedAt":"2014-12-04T23:27:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Signed-off-by: Jonathan Nieder \n> ---\n> Junio C Hamano wrote:\n>> Jonathan Nieder writes:\n>\n>>> -extern int copy_fd(int ifd, int ofd);\n>>> +extern int copy_fd(int ifd, int ofd, struct strbuf *err);\n>>\n>> It is not limited to this single function, but what contract do we\n>> envision this \"error messages are given back to the caller via\n>> strbuf\" convention should give between the callers and the callee?\n>\n> Here's a draft for documentation on that.\n\nThanks; looks reasonable; even if the discussion between you and\nPeff took us to a slightly different direction than what you\ndescribed here, the earlier description of long established practice\nis a welcome addition.\n\n\n\n\n> Documentation/technical/api-error-handling.txt | 75 ++++++++++++++++++++++++++\n> 1 file changed, 75 insertions(+)\n> create mode 100644 Documentation/technical/api-error-handling.txt\n>\n> diff --git a/Documentation/technical/api-error-handling.txt\n> b/Documentation/technical/api-error-handling.txt\n> new file mode 100644\n> index 0000000..fc68db1\n> --- /dev/null\n> +++ b/Documentation/technical/api-error-handling.txt\n> @@ -0,0 +1,75 @@\n> +Error reporting in git\n> +======================\n> +\n> +`die`, `usage`, `error`, and `warning` report errors of various\n> +kinds.\n> +\n> +- `die` is for fatal application errors. It prints a message to\n> + the user and exits with status 128.\n> +\n> +- `usage` is for errors in command line usage. After printing its\n> + message, it exits with status 129. (See also `usage_with_options`\n> + in the link:api-parse-options.html[parse-options API].)\n> +\n> +- `error` is for non-fatal library errors. It prints a message\n> + to the user and returns -1 for convenience in signaling the error\n> + to the caller.\n> +\n> +- `warning` is for reporting situations that probably should not\n> + occur but which the user (and Git) can continue to work around\n> + without running into too many problems. Like `error`, it\n> + returns -1 after reporting the situation to the caller.\n> +\n> +Customizable error handlers\n> +---------------------------\n> +\n> +The default behavior of `die` and `error` is to write a message to\n> +stderr and then exit or return as appropriate. This behavior can be\n> +overridden using `set_die_routine` and `set_error_routine`. For\n> +example, \"git daemon\" uses set_die_routine to write the reason `die`\n> +was called to syslog before exiting.\n> +\n> +Library errors\n> +--------------\n> +\n> +Functions return a negative integer on error. Details beyond that\n> +vary from function to function:\n> +\n> +- Some functions return -1 for all errors. Others return a more\n> + specific value depending on how the caller might want to react\n> + to the error.\n> +\n> +- Some functions report the error to stderr with `error`,\n> + while others leave that for the caller to do.\n> +\n> +- errno is not meaningful on return from most functions (except\n> + for thin wrappers for system calls).\n> +\n> +Check the function's API documentation to be sure.\n> +\n> +Caller-handled errors\n> +---------------------\n> +\n> +An increasing number of functions take a parameter 'struct strbuf *err'.\n> +On error, such functions append a message about what went wrong to the\n> +'err' strbuf. The message is meant to be complete enough to be passed\n> +to `die` or `error` as-is. For example:\n> +\n> +\tif (ref_transaction_commit(transaction, &err))\n> +\t\tdie(\"%s\", err.buf);\n> +\n> +The 'err' parameter will be untouched if no error occured, so multiple\n> +function calls can be chained:\n> +\n> +\tt = ref_transaction_begin(&err);\n> +\tif (!t ||\n> +\t ref_transaction_update(t, \"HEAD\", ..., &err) ||\n> +\t ret_transaction_commit(t, &err))\n> +\t\tdie(\"%s\", err.buf);\n> +\n> +The 'err' parameter must be a pointer to a valid strbuf. To silence\n> +a message, pass a strbuf that is explicitly ignored:\n> +\n> +\tif (thing_that_can_fail_in_an_ignorable_way(..., &err))\n> +\t\t/* This failure is okay. */\n> +\t\tstrbuf_reset(&err);\n"},{"id":"253133","messageId":"20141204233738.GE16345@google.com","threadId":"37988","inReplyTo":"xmqqy4qnq9m2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-04T23:37:38Z","receivedAt":"2014-12-04T23:37:38Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJunio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> Here's a draft for documentation on that.\n>\n> Thanks; looks reasonable; even if the discussion between you and\n> Peff took us to a slightly different direction than what you\n> described here, the earlier description of long established practice\n> is a welcome addition.\n\nThanks.\n\nCan you say more? I didn't think the discussion had reached a\nconclusion yet.\n\nUnless there's a strong reason to do otherwise, I prefer to stick with\nthe strbufs in this series for now. I prefer strbuf for this purpose\nover char[1024]. And switching later doesn't seem hard.\n\nBut if others prefer char[1024], I can update the existing code that\nuses strbuf from the transaction API to use that and reroll this\nseries with that convention. A part of me dislikes that but I'll go\nalong.\n\nJonathan\n"},{"id":"253134","messageId":"20141204234147.GF16345@google.com","threadId":"37988","inReplyTo":"xmqqy4qnq9m2.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-12-04T23:41:47Z","receivedAt":"2014-12-04T23:41:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n> Jonathan Nieder writes:\n\n>> Here's a draft for documentation on that.\n>\n> Thanks; looks reasonable; even if the discussion between you and\n> Peff took us to a slightly different direction than what you\n> described here, the earlier description of long established practice\n> is a welcome addition.\n\nI think I see what I misunderstood. Do you mean \"even if we settle on\na different API, this documentation of what you started with should be\neasy to adapt and will make life easier\"?\n\nIn other words, did you mean to say that things are still up in the\nair (which I agree with) or that the project has already settled on a\ndifferent direction (which I do not)?\n\nSorry for the confusion,\nJonathan\n"},{"id":"253136","messageId":"20141204234432.GA29953@peff.net","threadId":"37988","inReplyTo":"20141204234147.GF16345@google.com","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-04T23:44:33Z","receivedAt":"2014-12-04T23:44:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 04, 2014 at 03:41:47PM -0800, Jonathan Nieder wrote:\n\n> Junio C Hamano wrote:\n> > Jonathan Nieder writes:\n> \n> >> Here's a draft for documentation on that.\n> >\n> > Thanks; looks reasonable; even if the discussion between you and\n> > Peff took us to a slightly different direction than what you\n> > described here, the earlier description of long established practice\n> > is a welcome addition.\n> \n> I think I see what I misunderstood. Do you mean \"even if we settle on\n> a different API, this documentation of what you started with should be\n> easy to adapt and will make life easier\"?\n> \n> In other words, did you mean to say that things are still up in the\n> air (which I agree with) or that the project has already settled on a\n> different direction (which I do not)?\n\nFWIW, that is how I read it (up in the air), and where I thought our\ndiscussion was.\n\n-Peff\n"},{"id":"253139","messageId":"CAPc5daW3+8xjG3z3WgOMfqzWJUiPdcN1-FVgVc0fAjH7tgCa4A@mail.gmail.com","threadId":"37988","inReplyTo":"20141204234432.GA29953@peff.net","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-04T23:52:45Z","receivedAt":"2014-12-04T23:52:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Yeah, that is what I meant. The earlier part will not go to waste no matter\nwhat happens to the discussion.\n\nI am not a fan of char[1024], if only because our error message may have\nto mention things whose length is not under our control, e.g. a filename\nin the working tree, but I do share your concern that \"strbuf\"-approach\ncalls for more boilerplate. I offhand do not have a magic silver bullet for\nit, though.\n\nOn Thu, Dec 4, 2014 at 3:44 PM, Jeff King wrote:\n> On Thu, Dec 04, 2014 at 03:41:47PM -0800, Jonathan Nieder wrote:\n>\n>> Junio C Hamano wrote:\n>> > Jonathan Nieder writes:\n>>\n>> >> Here's a draft for documentation on that.\n>> >\n>> > Thanks; looks reasonable; even if the discussion between you and\n>> > Peff took us to a slightly different direction than what you\n>> > described here, the earlier description of long established practice\n>> > is a welcome addition.\n>>\n>> I think I see what I misunderstood. Do you mean \"even if we settle on\n>> a different API, this documentation of what you started with should be\n>> easy to adapt and will make life easier\"?\n>>\n>> In other words, did you mean to say that things are still up in the\n>> air (which I agree with) or that the project has already settled on a\n>> different direction (which I do not)?\n>\n> FWIW, that is how I read it (up in the air), and where I thought our\n> discussion was.\n>\n> -Peff\n"},{"id":"253142","messageId":"20141205000128.GA30048@peff.net","threadId":"37988","inReplyTo":"CAPc5daW3+8xjG3z3WgOMfqzWJUiPdcN1-FVgVc0fAjH7tgCa4A@mail.gmail.com","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-05T00:01:28Z","receivedAt":"2014-12-05T00:01:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Dec 04, 2014 at 03:52:45PM -0800, Junio C Hamano wrote:\n\n> Yeah, that is what I meant. The earlier part will not go to waste no matter\n> what happens to the discussion.\n> \n> I am not a fan of char[1024], if only because our error message may have\n> to mention things whose length is not under our control, e.g. a filename\n> in the working tree, but I do share your concern that \"strbuf\"-approach\n> calls for more boilerplate. I offhand do not have a magic silver bullet for\n> it, though.\n\nThe only downside I can think of is that we may truncate the message in\nexceptional circumstances. But is it really any less helpful to say:\n\n error: unable to open file: some-incredibly-long-filename-aaaaaa...\n\nthan printing out an extra 100 lines of \"a\"? And I mean the \"...\"\nliterally. I think mkerror() should indicate the truncation with a\n\"...\", just so that it is clear to the user. It should almost never\nhappen, but when it does, it can be helpful to show the user that yes,\nwe know we are truncating the message, and it is not that git truncated\nyour filename during the operation.\n\nIs this truncation really a concern, and/or is there some other downside\nI'm not thinking of?\n\n-Peff\n"},{"id":"253172","messageId":"xmqqfvcuq8nu.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141205000128.GA30048@peff.net","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-05T18:00:05Z","receivedAt":"2014-12-05T18:00:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> The only downside I can think of is that we may truncate the message in\n> exceptional circumstances. But is it really any less helpful to say:\n>\n> error: unable to open file: some-incredibly-long-filename-aaaaaa...\n>\n> than printing out an extra 100 lines of \"a\"? And I mean the \"...\"\n> literally. I think mkerror() should indicate the truncation with a\n> \"...\", just so that it is clear to the user. It should almost never\n> happen, but when it does, it can be helpful to show the user that yes,\n> we know we are truncating the message, and it is not that git truncated\n> your filename during the operation.\n>\n> Is this truncation really a concern, and/or is there some other downside\n> I'm not thinking of?\n\nI am more worried about variable length part pushing the information\nthat is given later out to the right, e.g. \"error: missing file '%s'\nprevents us from doing X\". Chomping to [1024] is not a good\nstrategy for that kind of message; abbreviating %s to /path/name/...\n(again, with literally \"...\") would be.\n"},{"id":"253275","messageId":"20141207100755.GB22230@peff.net","threadId":"37988","inReplyTo":"xmqqfvcuq8nu.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-07T10:07:55Z","receivedAt":"2014-12-07T10:07:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > The only downside I can think of is that we may truncate the message in\n> > exceptional circumstances. But is it really any less helpful to say:\n> >\n> > error: unable to open file: some-incredibly-long-filename-aaaaaa...\n> >\n> > than printing out an extra 100 lines of \"a\"? And I mean the \"...\"\n> > literally. I think mkerror() should indicate the truncation with a\n> > \"...\", just so that it is clear to the user. It should almost never\n> > happen, but when it does, it can be helpful to show the user that yes,\n> > we know we are truncating the message, and it is not that git truncated\n> > your filename during the operation.\n> >\n> > Is this truncation really a concern, and/or is there some other downside\n> > I'm not thinking of?\n> \n> I am more worried about variable length part pushing the information\n> that is given later out to the right, e.g. \"error: missing file '%s'\n> prevents us from doing X\". Chomping to [1024] is not a good\n> strategy for that kind of message; abbreviating %s to /path/name/...\n> (again, with literally \"...\") would be.\n\nTrue. Unfortunately I do not think there is an easy way to truncate the\nexpanded strings used by placeholders. The two approaches I could think\nof are:\n\n 1. Loop over the va_list and tweak any pointers we find. Except that\n loop means we actually need to loop over the _format string_, since\n that is the only thing which tells us which placeholders are strings.\n But I am not even sure there is a portable way to munge a va_list,\n as opposed to just reading it.\n\n 2. Transparently rewrite '%s' in the format string to '%.128s' or\n similar. This also requires iterating over the format string, but\n it's fairly straightforward. Code is below, but it's not _quite_\n right. We would want to conditionally add \"...\" based on whether or\n not the particular item was truncated. But we cannot know when\n munging the format string if that will happen or not (we know\n _something_ will be truncated, but not which string).\n\nI don't think you can do it right without reimplementing vsnprintf\nyourself. Which is obviously a terrible idea.\n\nI still think truncation is going to be a non-issue in practice, and I'd\nrather deal with that potential fallout than have to deal with memory\nmanagement in every error handler that uses this technique. But I don't\nhave much more to say on the matter, so if you disagree, I guess that's\nthat.\n\nUnless we can do something clever with a set of global error strbufs or\nsomething (i.e., that expand as needed, but the caller does not have to\nfree themselves, as they will get recycled eventually). That has its own\ncorner cases, though.\n\nAnyway, the truncating-fmt code is below, for fun.\n\n-Peff\n\nstatic int abbrev_fmt(char *buf, int len, int abbrev, const char *fmt, va_list ap)\n{\n\tva_list cp;\n\tint got;\n\n\tva_copy(cp, ap);\n\tgot = vsnprintf(buf, len, fmt, ap);\n\tif (got >= len) {\n\t\tstruct strbuf munged = STRBUF_INIT;\n\n\t\twhile (*fmt) {\n\t\t\tchar ch = *fmt++;\n\t\t\tstrbuf_addch(&munged, ch);\n\t\t\tif (ch == '%') {\n\t\t\t\tif (*fmt == 's') {\n\t\t\t\t\tstrbuf_addf(&munged, \".%ds...\", abbrev);\n\t\t\t\t\tfmt++;\n\t\t\t\t} else /* skips past %%-quoting */\n\t\t\t\t\tstrbuf_addch(&munged, *fmt);\n\t\t\t}\n\t\t}\n\n\t\tgot = vsnprintf(buf, len, munged.buf, cp);\n\t\tstrbuf_release(&munged);\n\t}\n\treturn got;\n}\n"},{"id":"253425","messageId":"xmqqk320mzo7.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20141207100755.GB22230@peff.net","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-09T18:43:52Z","receivedAt":"2014-12-09T18:43:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote:\n>\n>> I am more worried about variable length part pushing the information\n>> that is given later out to the right, e.g. \"error: missing file '%s'\n>> prevents us from doing X\". Chomping to [1024] is not a good\n>> strategy for that kind of message; abbreviating %s to /path/name/...\n>> (again, with literally \"...\") would be.\n>\n> True. Unfortunately I do not think there is an easy way to truncate the\n> expanded strings used by placeholders...\n> ...\n> Unless we can do something clever with a set of global error strbufs or\n> something (i.e., that expand as needed, but the caller does not have to\n> free themselves, as they will get recycled eventually). That has its own\n> corner cases, though.\n\nI do share your concern that \"strbuf\"-approach calls for more\nboilerplate leading to unmaintainable code, but I offhand do not\nhave a magic silver bullet for it. globals are indeed tempting, but\nI'd have to say that what Jonathan has may probably be the least bad\nof the possibilities.\n\nChomping also has complications when we have to deal with user\npayload (e.g. pathname in UTF-8), which we may want avoid having to\nworry about.\n"},{"id":"253427","messageId":"20141209184944.GA31158@peff.net","threadId":"37988","inReplyTo":"xmqqk320mzo7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-09T18:49:45Z","receivedAt":"2014-12-09T18:49:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 09, 2014 at 10:43:52AM -0800, Junio C Hamano wrote:\n\n> > Unless we can do something clever with a set of global error strbufs or\n> > something (i.e., that expand as needed, but the caller does not have to\n> > free themselves, as they will get recycled eventually). That has its own\n> > corner cases, though.\n> \n> I do share your concern that \"strbuf\"-approach calls for more\n> boilerplate leading to unmaintainable code, but I offhand do not\n> have a magic silver bullet for it. globals are indeed tempting, but\n> I'd have to say that what Jonathan has may probably be the least bad\n> of the possibilities.\n\nOK. I'm not sure I agree that it is the least bad, but I don't think\nit's worth arguing over more. Let's go with it, and you can note my\nobjection in the captain's log. :)\n\n-Peff\n"},{"id":"253510","messageId":"54887CB7.4000603@alum.mit.edu","threadId":"37988","inReplyTo":"20141204075920.GA27142@peff.net","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-12-10T17:02:47Z","receivedAt":"2014-12-10T17:02:47Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 12/04/2014 08:59 AM, Jeff King wrote:\n> On Wed, Dec 03, 2014 at 01:38:58PM -0800, Jonathan Nieder wrote:\n>> The allocation of a variable-sized buffer is a small overhead that I\n>> don't mind incurring on error. In the non-error case, the caller\n>> doesn't actually have to free the buffer, and if they choose to, the\n>> overhead incurred is that of free(NULL)'.\n> \n> I don't care at all about overhead. I care about extra work on the part\n> of the caller to avoid a leak. It turns:\n> \n> if (some_func(fd, &err))\n> \treturn error(\"%s\", err.msg);\n> \n> into:\n> \n> if (some_func(fd, &err)) {\n> \terror(\"%s\", err.buf);\n> \tstrbuf_release(&err);\n> \treturn -1;\n> }\n\nWhat if we go in the direction not of less infrastructure, but a little\nbit more? Like\n\n\tstruct result {\n\t\tint code;\n\t\tstruct strbuf msg;\n\t};\n\n\tint report_errors(struct result *result)\n\t{\n\t\tint code = result->code;\n\t\tif (code) {\n\t\t\terror(result->msg.buf);\n\t\t}\n\t\tresult->code = 0;\n\t\tstrbuf_release(result->msg);\n\t\treturn code; /* or alternatively (code ? -1 : 0) */\n\t}\n\n\tint report_warnings(struct result *result)\n\t{\n\t\t...\n\t}\n\n\tint report_with_prefix(struct result *result, const char *fmt, ...)\n\t{\n\t\t...\n\t}\n\nThen a caller could look pretty much like before:\n\n\tstruct result result = RESULT_INIT;\n\n\tif (some_func(fd, &result))\n\t\treturn report_errors(&result);\n\nOther callers might not even bother to check the return value of the\nfunction, relying instead on result.code via process_error():\n\n\tchar *ptr = some_func(fd, &result);\n\tif (report_errors(&result))\n\t\treturn -1;\n\nIf the result code is considered superfluous, we could use naked strbufs\nand use msg.len as the indicator that there was an error.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"253526","messageId":"xmqqfvcngwjh.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"54887CB7.4000603@alum.mit.edu","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-10T19:00:18Z","receivedAt":"2014-12-10T19:00:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> What if we go in the direction not of less infrastructure, but a little\n> bit more? Like\n>\n> \tstruct result {\n> \t\tint code;\n> \t\tstruct strbuf msg;\n> \t};\n>\n> \tint report_errors(struct result *result)\n> \t{\n> \t\tint code = result->code;\n> \t\tif (code) {\n> \t\t\terror(result->msg.buf);\n> \t\t}\n> \t\tresult->code = 0;\n> \t\tstrbuf_release(result->msg);\n> \t\treturn code; /* or alternatively (code ? -1 : 0) */\n> \t}\n>\n> \tint report_warnings(struct result *result)\n> \t{\n> \t\t...\n> \t}\n>\n> \tint report_with_prefix(struct result *result, const char *fmt, ...)\n> \t{\n> \t\t...\n> \t}\n>\n> Then a caller could look pretty much like before:\n>\n> \tstruct result result = RESULT_INIT;\n>\n> \tif (some_func(fd, &result))\n> \t\treturn report_errors(&result);\n>\n> Other callers might not even bother to check the return value of the\n> function, relying instead on result.code via process_error():\n>\n> \tchar *ptr = some_func(fd, &result);\n> \tif (report_errors(&result))\n> \t\treturn -1;\n>\n> If the result code is considered superfluous, we could use naked strbufs\n> and use msg.len as the indicator that there was an error.\n\nTwo potential issues are:\n\n - Callers that ignore errors need to actively ignore errors with\n strbuf_release(&result.msg);\n\n - Callers have to remember that once the report_errors() function\n is called on a \"struct result\", the struct loses its information.\n\nNeither is insurmountable, but the latter might turn out to be\ncumbersome to work around in some codepaths.\n\nOther than that, I think it is OK.\n\nAnother alternative may be to have the reporting storage on the side\nof the callee, and have callers that are interested in errors to\nsupply a place to store a pointer to it, i.e.\n\n\tint some_func(..., struct result **errors) {\n \tstatic struct result mine;\n\n\t\tclear_result(&mine);\n\t\t... do its thing ...\n if (... error detected ...) {\n mine.code = E_SOMEFUNC;\n strbuf_addstr(&mine.msg, \"some_func: foobared\");\n\t\t}\n\n\t\tif (errors)\n\t\t\t*errors = &mine;\n return mine.code;\n\t}\n\nAnd a caller would do this instead:\n\n\tstruct result *result = NULL;\n\n if (some_func(fd, &result))\n\t\treturn report_errors(result);\n\nwhere report_errors() and friends will only peek but not clear the\nresult reporting storage. The clear_result() helper would trivially\nbe:\n\n\tvoid clear_result(struct result *result) {\n\t\tresult->code = 0;\n strbuf_release(&result->msg);\n\t}\n"},{"id":"253527","messageId":"20141210191434.GA23273@peff.net","threadId":"37988","inReplyTo":"xmqqfvcngwjh.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-12-10T19:14:34Z","receivedAt":"2014-12-10T19:14:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Dec 10, 2014 at 11:00:18AM -0800, Junio C Hamano wrote:\n\n> Two potential issues are:\n> \n> - Callers that ignore errors need to actively ignore errors with\n> strbuf_release(&result.msg);\n\nThat was my first thought, too. If you want to do anything besides\nreport_error, you have to deal with the strbuf. But I'd guess that they\noften fall into one of two cases:\n\n 1. You are just propagating the error to your caller. In which case\n it is not _your_ result struct in the first place, and you do not\n need to care about deallocating it either way. I.e.:\n\n int some_func(..., struct result *err)\n\t{\n\t\tif (some_other_func(..., err))\n\t\t\treturn -1;\n\t\t...\n\t}\n\n 2. You want to ignore the error. I think anybody taking a result\n struct (or a strbuf, or whatever) should accept NULL as \"do not\n bother giving me your message\". And the convenience wrappers\n should handle that (I think the mkerror example I sent earlier\n did), so callees can just do:\n\n return mkerror(err, \"whatever: %s\", ...);\n\nThe remainder could strbuf_release manually, but there would hopefully\nnot be many of them.\n\nI think I could live with something like that.\n\n> - Callers have to remember that once the report_errors() function\n> is called on a \"struct result\", the struct loses its information.\n> \n> Neither is insurmountable, but the latter might turn out to be\n> cumbersome to work around in some codepaths.\n\nI suspect the message is not that interesting after calling\nreport_errors(). The \"code\" flag could remain, as it does not require\ndeallocation.\n\n> Another alternative may be to have the reporting storage on the side\n> of the callee, and have callers that are interested in errors to\n> supply a place to store a pointer to it, i.e.\n> \n> \tint some_func(..., struct result **errors) {\n> \tstatic struct result mine;\n\nThis makes some_func not reentrant. Which is a hazard both for threaded\ncode, but also for functions which want to do:\n\n if (some_func(foo, &err_one)) {\n /* didn't work? Try an alternative. */\n\t if (!some_func(bar, &err_two))\n\t ....\n\nand expect err_one to contain anything useful.\n"},{"id":"256036","messageId":"xmqqd25epw24.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"xmqqk320mzo7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-12T23:08:35Z","receivedAt":"2015-02-12T23:08:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jeff King writes:\n>\n>> On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote:\n>>\n>>> I am more worried about variable length part pushing the information\n>>> that is given later out to the right, e.g. \"error: missing file '%s'\n>>> prevents us from doing X\". Chomping to [1024] is not a good\n>>> strategy for that kind of message; abbreviating %s to /path/name/...\n>>> (again, with literally \"...\") would be.\n\nI have this one in my pile of Undecided topics:\n\n * jn/doc-api-errors (2014-12-04) 1 commit\n - doc: document error handling functions and conventions\n\n For discussion.\n What's the status of this one????\n\nI think we all agree that the early part of the new documentation\ntext is good, but the last section that proposes to store more\ndetailed errors in caller supplied strbuf in textual form was\ncontroversial (and I have not convinced myself it is a good idea\nyet).\n\nI could chuck the last section and then start merging the remainder\nto 'next' to salvage the \"obviously good bits\". Or do people want\nto hash its last section a bit more?\n"},{"id":"256199","messageId":"54E3632F.20907@alum.mit.edu","threadId":"37988","inReplyTo":"xmqqd25epw24.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2015-02-17T15:50:07Z","receivedAt":"2015-02-17T15:50:07Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 02/13/2015 12:08 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Jeff King writes:\n>>\n>>> On Fri, Dec 05, 2014 at 10:00:05AM -0800, Junio C Hamano wrote:\n>>>\n>>>> I am more worried about variable length part pushing the information\n>>>> that is given later out to the right, e.g. \"error: missing file '%s'\n>>>> prevents us from doing X\". Chomping to [1024] is not a good\n>>>> strategy for that kind of message; abbreviating %s to /path/name/...\n>>>> (again, with literally \"...\") would be.\n> \n> I have this one in my pile of Undecided topics:\n> \n> * jn/doc-api-errors (2014-12-04) 1 commit\n> - doc: document error handling functions and conventions\n> \n> For discussion.\n> What's the status of this one????\n> \n> I think we all agree that the early part of the new documentation\n> text is good, but the last section that proposes to store more\n> detailed errors in caller supplied strbuf in textual form was\n> controversial (and I have not convinced myself it is a good idea\n> yet).\n> \n> I could chuck the last section and then start merging the remainder\n> to 'next' to salvage the \"obviously good bits\". Or do people want\n> to hash its last section a bit more?\n\nWhether or not we decide on a different error-handling convention in the\nfuture, it is a fact of life that a good bit of code already uses the\n\"strbuf\" convention documented by Jonathan's patch. So I think it is OK\nto merge it as is. If we change the preferred convention in the future,\none part of the change will be to update this file.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\n"},{"id":"256201","messageId":"xmqq1tlo4jbf.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"54E3632F.20907@alum.mit.edu","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-17T16:03:00Z","receivedAt":"2015-02-17T16:03:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>> I think we all agree that the early part of the new documentation\n>> text is good, but the last section that proposes to store more\n>> detailed errors in caller supplied strbuf in textual form was\n>> controversial (and I have not convinced myself it is a good idea\n>> yet).\n>> \n>> I could chuck the last section and then start merging the remainder\n>> to 'next' to salvage the \"obviously good bits\". Or do people want\n>> to hash its last section a bit more?\n>\n> Whether or not we decide on a different error-handling convention in the\n> future, it is a fact of life that a good bit of code already uses the\n> \"strbuf\" convention documented by Jonathan's patch. So I think it is OK\n> to merge it as is. If we change the preferred convention in the future,\n> one part of the change will be to update this file.\n\nI wasn't sure about \"a good bit of code already\", but that settles\nit. Let's take it as-is and see how the code evolves.\n\nThanks.\n"},{"id":"256202","messageId":"20150217160524.GA11186@peff.net","threadId":"37988","inReplyTo":"xmqq1tlo4jbf.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-02-17T16:05:24Z","receivedAt":"2015-02-17T16:05:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 17, 2015 at 08:03:00AM -0800, Junio C Hamano wrote:\n\n> > Whether or not we decide on a different error-handling convention in the\n> > future, it is a fact of life that a good bit of code already uses the\n> > \"strbuf\" convention documented by Jonathan's patch. So I think it is OK\n> > to merge it as is. If we change the preferred convention in the future,\n> > one part of the change will be to update this file.\n> \n> I wasn't sure about \"a good bit of code already\", but that settles\n> it. Let's take it as-is and see how the code evolves.\n\nI'm not convinced myself after a quick grep. But it's certainly\nnon-zero, and I think I would rather have the technique documented than\nnot, so I withdraw my earlier complaints.\n\n-Peff\n"},{"id":"256245","messageId":"xmqqr3toyx47.fsf@gitster.dls.corp.google.com","threadId":"37988","inReplyTo":"20150217160524.GA11186@peff.net","subject":"Re: [PATCH/RFC] doc: document error handling functions and conventions (Re: [PATCH 03/14] copy_fd: pass error message back through a strbuf)","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-02-17T22:46:48Z","receivedAt":"2015-02-17T22:46:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Feb 17, 2015 at 08:03:00AM -0800, Junio C Hamano wrote:\n>\n>> > Whether or not we decide on a different error-handling convention in the\n>> > future, it is a fact of life that a good bit of code already uses the\n>> > \"strbuf\" convention documented by Jonathan's patch. So I think it is OK\n>> > to merge it as is. If we change the preferred convention in the future,\n>> > one part of the change will be to update this file.\n>> \n>> I wasn't sure about \"a good bit of code already\", but that settles\n>> it. Let's take it as-is and see how the code evolves.\n>\n> I'm not convinced myself after a quick grep. But it's certainly\n> non-zero, and I think I would rather have the technique documented than\n> not, so I withdraw my earlier complaints.\n\nOK.\n"}]}