{"thread":{"id":"42796","subject":"[PATCH] am: ignore return value of write_file()","startedAt":"2016-07-07T20:02:34Z","lastAt":"2016-07-10T10:54:00Z","messageCount":27,"participants":["René Scharfe","Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"290976","messageId":"577EB546.1090007@web.de","threadId":"42796","inReplyTo":null,"subject":"[PATCH] am: ignore return value of write_file()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-07-07T20:02:14Z","receivedAt":"2016-07-07T20:02:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"write_file() either returns 0 or dies, so there is no point in checking\nits return value.  The callers of the wrappers write_state_text(),\nwrite_state_count() and write_state_bool() consequently already ignore\ntheir return values.  Stop pretenting we care and make them void.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n builtin/am.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d5da5fe..339e360 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -184,22 +184,22 @@ static inline const char *am_path(const struct am_state *state, const char *path\n /**\n  * For convenience to call write_file()\n  */\n-static int write_state_text(const struct am_state *state,\n-\t\t\t    const char *name, const char *string)\n+static void write_state_text(const struct am_state *state,\n+\t\t\t     const char *name, const char *string)\n {\n-\treturn write_file(am_path(state, name), \"%s\", string);\n+\twrite_file(am_path(state, name), \"%s\", string);\n }\n \n-static int write_state_count(const struct am_state *state,\n-\t\t\t     const char *name, int value)\n+static void write_state_count(const struct am_state *state,\n+\t\t\t      const char *name, int value)\n {\n-\treturn write_file(am_path(state, name), \"%d\", value);\n+\twrite_file(am_path(state, name), \"%d\", value);\n }\n \n-static int write_state_bool(const struct am_state *state,\n-\t\t\t    const char *name, int value)\n+static void write_state_bool(const struct am_state *state,\n+\t\t\t     const char *name, int value)\n {\n-\treturn write_state_text(state, name, value ? \"t\" : \"f\");\n+\twrite_state_text(state, name, value ? \"t\" : \"f\");\n }\n \n /**\n-- \n2.9.0\n\n"},{"id":"290980","messageId":"20160707203157.GA11804@sigill.intra.peff.net","threadId":"42796","inReplyTo":"577EB546.1090007@web.de","subject":"Re: [PATCH] am: ignore return value of write_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-07T20:31:58Z","receivedAt":"2016-07-07T20:32:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 07, 2016 at 10:02:14PM +0200, René Scharfe wrote:\n\n> write_file() either returns 0 or dies, so there is no point in checking\n> its return value.  The callers of the wrappers write_state_text(),\n> write_state_count() and write_state_bool() consequently already ignore\n> their return values.  Stop pretenting we care and make them void.\n\nMakes sense. Originally it took \"fatal\" as a parameter, but it was split\ninto two functions in 12d6ce1 (write_file(): drop \"fatal\" parameter,\n2015-08-24). The return value on the non-gentle version could have been\ndropped at that point.\n\nArguably we could get rid of the gentle form entirely, as below. The\ndiffstat is certainly pleasing, but maybe we would eventually want it\nfor another caller. I dunno. I won't be offended if we drop this as\nchurn.\n\n-- >8 --\nSubject: [PATCH] write_file: drop \"gently\" form\n\nWe have two forms of write_file(): one that dies, and one\nthat returns an error. However, the latter has only a single\ncaller, which immediately dies anyway (after producing a\nmessage that is not really any more informative than\nwrite_file's generic die(), and arguably worse because it\ndoes not give the actual filename).\n\nLet's convert that site to use the non-gentle form. At that\npoint the gentle form has no callers, and we can simplify\nthe implementation of write_file.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAs a fun aside, this patch was generated using \"--patience\",\nwhich gives a much closer to result to what I actually\nchanged than Myers diff (not meaningful to the patch, but\nI'm just always on the lookout for cases where the\nalgorithms produce meaningfully different results).\n\n builtin/branch.c |  5 +----\n cache.h          |  3 +--\n wrapper.c        | 56 ++++++++++++--------------------------------------------\n 3 files changed, 14 insertions(+), 50 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 2ecde53..15232c4 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -618,10 +618,7 @@ static int edit_branch_description(const char *branch_name)\n \t\t    \"  %s\\n\"\n \t\t    \"Lines starting with '%c' will be stripped.\\n\",\n \t\t    branch_name, comment_line_char);\n-\tif (write_file_gently(git_path(edit_description), \"%s\", buf.buf)) {\n-\t\tstrbuf_release(&buf);\n-\t\treturn error_errno(_(\"could not write branch description template\"));\n-\t}\n+\twrite_file(git_path(edit_description), \"%s\", buf.buf);\n \tstrbuf_reset(&buf);\n \tif (launch_editor(git_path(edit_description), &buf, NULL)) {\n \t\tstrbuf_release(&buf);\ndiff --git a/cache.h b/cache.h\nindex f1dc289..3f6c53f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1745,8 +1745,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n \treturn write_in_full(fd, str, strlen(str));\n }\n \n-extern int write_file(const char *path, const char *fmt, ...);\n-extern int write_file_gently(const char *path, const char *fmt, ...);\n+extern void write_file(const char *path, const char *fmt, ...);\n \n /* pager.c */\n extern void setup_pager(void);\ndiff --git a/wrapper.c b/wrapper.c\nindex 5dc4e15..0349441 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -640,56 +640,24 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)\n \treturn len;\n }\n \n-static int write_file_v(const char *path, int fatal,\n-\t\t\tconst char *fmt, va_list params)\n+void write_file(const char *path, const char *fmt, ...)\n {\n+\tva_list params;\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);\n-\tif (fd < 0) {\n-\t\tif (fatal)\n-\t\t\tdie_errno(_(\"could not open %s for writing\"), path);\n-\t\treturn -1;\n-\t}\n+\tif (fd < 0)\n+\t\tdie_errno(_(\"could not open %s for writing\"), path);\n+\n+\tva_start(params, fmt);\n \tstrbuf_vaddf(&sb, fmt, params);\n+\tva_end(params);\n+\n \tstrbuf_complete_line(&sb);\n-\tif (write_in_full(fd, sb.buf, sb.len) != sb.len) {\n-\t\tint err = errno;\n-\t\tclose(fd);\n-\t\tstrbuf_release(&sb);\n-\t\terrno = err;\n-\t\tif (fatal)\n-\t\t\tdie_errno(_(\"could not write to %s\"), path);\n-\t\treturn -1;\n-\t}\n+\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n+\t\tdie_errno(_(\"could not write to %s\"), path);\n \tstrbuf_release(&sb);\n-\tif (close(fd)) {\n-\t\tif (fatal)\n-\t\t\tdie_errno(_(\"could not close %s\"), path);\n-\t\treturn -1;\n-\t}\n-\treturn 0;\n-}\n-\n-int write_file(const char *path, const char *fmt, ...)\n-{\n-\tint status;\n-\tva_list params;\n-\n-\tva_start(params, fmt);\n-\tstatus = write_file_v(path, 1, fmt, params);\n-\tva_end(params);\n-\treturn status;\n-}\n-\n-int write_file_gently(const char *path, const char *fmt, ...)\n-{\n-\tint status;\n-\tva_list params;\n-\n-\tva_start(params, fmt);\n-\tstatus = write_file_v(path, 0, fmt, params);\n-\tva_end(params);\n-\treturn status;\n+\tif (close(fd))\n+\t\tdie_errno(_(\"could not close %s\"), path);\n }\n \n void sleep_millisec(int millisec)\n-- \n2.9.0.393.g704e522\n\n"},{"id":"291005","messageId":"alpine.DEB.2.20.1607080830430.6426@virtualbox","threadId":"42796","inReplyTo":"577EB546.1090007@web.de","subject":"Re: [PATCH] am: ignore return value of write_file()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-08T06:33:55Z","receivedAt":"2016-07-08T06:34:15Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Thu, 7 Jul 2016, René Scharfe wrote:\n\n> write_file() either returns 0 or dies, so there is no point in checking\n> its return value.\n\nThe question is whether it makes sense for write_file() to die(). It is a\nlibrary function and not every caller can be happy with that function to\nexit the program when some file could not be written, without a chance to\ntell the user what to do about the situation.\n\nIf write_file() was defined in builtin/am.c, as a static function, I would\ngrudgingly acquiesce, but it is not.\n\nIMO it would be better to fix write_file() to *not* die() but return\nerror() instead.\n\nCiao,\nDscho"},{"id":"291006","messageId":"alpine.DEB.2.20.1607080834340.6426@virtualbox","threadId":"42796","inReplyTo":"20160707203157.GA11804@sigill.intra.peff.net","subject":"Re: [PATCH] am: ignore return value of write_file()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-08T06:37:35Z","receivedAt":"2016-07-08T06:37:49Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Thu, 7 Jul 2016, Jeff King wrote:\n\n> We have two forms of write_file(): one that dies, and one\n> that returns an error. However, the latter has only a single\n> caller, which immediately dies anyway (after producing a\n> message that is not really any more informative than\n> write_file's generic die(), and arguably worse because it\n> does not give the actual filename).\n\nThis is more an illustration of unnecessarily duplicated code, isn't it?\nThere are *tons* of instances in Git's code where writing to a file is\nimplemented separately (and differently).\n\nIt would make tons of sense to consolidate all of these instances,\nmethinks. The diffstat should look *very* pleasing.\n\nCiao,\nDscho\n"},{"id":"291008","messageId":"20160708065650.GA10664@sigill.intra.peff.net","threadId":"42796","inReplyTo":"alpine.DEB.2.20.1607080834340.6426@virtualbox","subject":"Re: [PATCH] am: ignore return value of write_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T06:56:51Z","receivedAt":"2016-07-08T06:57:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2016 at 08:37:35AM +0200, Johannes Schindelin wrote:\n\n> > We have two forms of write_file(): one that dies, and one\n> > that returns an error. However, the latter has only a single\n> > caller, which immediately dies anyway (after producing a\n> > message that is not really any more informative than\n> > write_file's generic die(), and arguably worse because it\n> > does not give the actual filename).\n> \n> This is more an illustration of unnecessarily duplicated code, isn't it?\n> There are *tons* of instances in Git's code where writing to a file is\n> implemented separately (and differently).\n> \n> It would make tons of sense to consolidate all of these instances,\n> methinks. The diffstat should look *very* pleasing.\n\nI grepped for O_WRONLY, and there are fewer instances than I would have\nthought. Most of the obvious write_file() candidates are in the merge\ncode, which is probably why you saw so many of them. :)\n\nI started at converting a few sites, but it's actually a little awkward\nbecause they all have strbufs (with a ptr/len combo that _could_ contain\nNULs, but probably doesn't), and write_file() wants to take a format\nstring.\n\nI think we can clean that up, though. I'll hopefully have a series in a\nfew minutes.\n\n-Peff\n"},{"id":"291013","messageId":"20160708090400.GA26594@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708065650.GA10664@sigill.intra.peff.net","subject":"[PATCH 0/8] write_file cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:04:01Z","receivedAt":"2016-07-08T09:04:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2016 at 02:56:50AM -0400, Jeff King wrote:\n\n> > This is more an illustration of unnecessarily duplicated code, isn't it?\n> > There are *tons* of instances in Git's code where writing to a file is\n> > implemented separately (and differently).\n> > \n> > It would make tons of sense to consolidate all of these instances,\n> > methinks. The diffstat should look *very* pleasing.\n> \n> I grepped for O_WRONLY, and there are fewer instances than I would have\n> thought. Most of the obvious write_file() candidates are in the merge\n> code, which is probably why you saw so many of them. :)\n> \n> I started at converting a few sites, but it's actually a little awkward\n> because they all have strbufs (with a ptr/len combo that _could_ contain\n> NULs, but probably doesn't), and write_file() wants to take a format\n> string.\n> \n> I think we can clean that up, though. I'll hopefully have a series in a\n> few minutes.\n\nHere it is. There actually weren't that many spots to clean up, as quite\na few of them have a \"twist\" where they want to do something clever,\nlike open the file and feed the descriptor to a sub-function, or open\nwith funny things like O_EXCL.\n\nBut still, the diffstat is pleasing:\n\n builtin/am.c     | 25 +++++++----------\n builtin/branch.c |  5 +---\n builtin/config.c |  2 +-\n builtin/merge.c  | 45 ++++--------------------------\n cache.h          | 17 ++++++++++--\n wrapper.c        | 52 ++++++++---------------------------\n 6 files changed, 44 insertions(+), 102 deletions(-)\n\nand that even includes adding some function documentation.\n\nThe most interesting thing is that I also found a real bug, albeit a\nfairly minor one. I floated that up to the front of the series.\n\n  [1/8]: config: fix bogus fd check when setting up default config\n  [2/8]: am: ignore return value of write_file()\n  [3/8]: branch: use non-gentle write_file for branch description\n  [4/8]: write_file: drop \"gently\" form\n  [5/8]: write_file: use xopen\n  [6/8]: write_file: add pointer+len variant\n  [7/8]: write_file: add format attribute\n  [8/8]: use write_file_buf where applicable\n\n-Peff\n"},{"id":"291014","messageId":"20160708090649.GA10152@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708090400.GA26594@sigill.intra.peff.net","subject":"[PATCH 1/8] config: fix bogus fd check when setting up default config","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:06:50Z","receivedAt":"2016-07-08T09:07:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Since 9830534 (config --global --edit: create a template\nfile if needed, 2014-07-25), an edit of the global config\nfile will try to open() it with O_EXCL, and wants to handle\nthree cases:\n\n  1. We succeeded; the user has no config file, and we\n     should fill in the default template.\n\n  2. We got EEXIST; they have a file already, proceed as usual.\n\n  3. We got another error; we should complain.\n\nHowever, the check for case 1 does \"if (fd)\", which will\ngenerally _always_ be true (except for the oddball case that\nsomehow our stdin got closed and opening really did give us\na new descriptor 0).\n\nSo in the EEXIST case, we tried to write the default config\nanyway! Fortunately, this turns out to be a noop, since we\njust end up writing to and closing \"-1\", which does nothing.\n\nBut in case 3, we would fail to notice any other errors, and\njust silently continue (given that we don't actually notice\nwrite errors for the template either, it's probably not that\nbig a deal; we're about to spawn the editor, so it would\nnotice any problems. But the code is clearly _trying_ to hit\ncover this case and failing).\n\nWe can fix it easily by using \"fd >= 0\" for case 1.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI was looking at this to see whether it could be converted to\nwrite_file(). However, the O_EXCL and the error-handling make things\ntoo tricky.\n\n builtin/config.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/config.c b/builtin/config.c\nindex 1d7c6ef..a991a53 100644\n--- a/builtin/config.c\n+++ b/builtin/config.c\n@@ -604,7 +604,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)\n \t\t\t\t      given_config_source.file : git_path(\"config\"));\n \t\tif (use_global_config) {\n \t\t\tint fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);\n-\t\t\tif (fd) {\n+\t\t\tif (fd >= 0) {\n \t\t\t\tchar *content = default_user_config();\n \t\t\t\twrite_str_in_full(fd, content);\n \t\t\t\tfree(content);\n-- \n2.9.0.393.g704e522\n\n"},{"id":"291015","messageId":"20160708090805.GB10152@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708090400.GA26594@sigill.intra.peff.net","subject":"[PATCH 2/8] am: ignore return value of write_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:08:05Z","receivedAt":"2016-07-08T09:08:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: René Scharfe <l.s.r@web.de>\n\nwrite_file() either returns 0 or dies, so there is no point in checking\nits return value.  The callers of the wrappers write_state_text(),\nwrite_state_count() and write_state_bool() consequently already ignore\ntheir return values.  Stop pretending we care and make them void.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\nSigned-off-by: Jeff King <peff@peff.net>\n---\nI included this just to make it clear that I am building on top. You\ncan also just apply on top of the existing branch (though note that I\nfixed a typo s/pretenting/pretending/ in the commit message).\n\n builtin/am.c | 18 +++++++++---------\n 1 file changed, 9 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d5da5fe..339e360 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -184,22 +184,22 @@ static inline const char *am_path(const struct am_state *state, const char *path\n /**\n  * For convenience to call write_file()\n  */\n-static int write_state_text(const struct am_state *state,\n-\t\t\t    const char *name, const char *string)\n+static void write_state_text(const struct am_state *state,\n+\t\t\t     const char *name, const char *string)\n {\n-\treturn write_file(am_path(state, name), \"%s\", string);\n+\twrite_file(am_path(state, name), \"%s\", string);\n }\n \n-static int write_state_count(const struct am_state *state,\n-\t\t\t     const char *name, int value)\n+static void write_state_count(const struct am_state *state,\n+\t\t\t      const char *name, int value)\n {\n-\treturn write_file(am_path(state, name), \"%d\", value);\n+\twrite_file(am_path(state, name), \"%d\", value);\n }\n \n-static int write_state_bool(const struct am_state *state,\n-\t\t\t    const char *name, int value)\n+static void write_state_bool(const struct am_state *state,\n+\t\t\t     const char *name, int value)\n {\n-\treturn write_state_text(state, name, value ? \"t\" : \"f\");\n+\twrite_state_text(state, name, value ? \"t\" : \"f\");\n }\n \n /**\n-- \n2.9.0.393.g704e522\n\n"},{"id":"291016","messageId":"20160708090854.GC10152@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708090400.GA26594@sigill.intra.peff.net","subject":"[PATCH 3/8] branch: use non-gentle write_file for branch description","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:08:54Z","receivedAt":"2016-07-08T09:09:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We use write_file_gently() to do this job currently.\nHowever, if we see an error, we simply complain via\nerror_errno() and then end up exiting with an error code.\n\nBy switching to the non-gentle form, the function will die\nfor us, with a better error. It is more specific about which\nsyscall caused the error, and that mentions the\nactual filename we're trying to write.\n\nOur exit code for the error case does switch from \"1\" to\n\"128\", but that's OK; it wasn't a meaningful documented code\n(and in fact it was odd that it was a different exit code\nthan most other error conditions).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/branch.c | 5 +----\n 1 file changed, 1 insertion(+), 4 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 2ecde53..15232c4 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -618,10 +618,7 @@ static int edit_branch_description(const char *branch_name)\n \t\t    \"  %s\\n\"\n \t\t    \"Lines starting with '%c' will be stripped.\\n\",\n \t\t    branch_name, comment_line_char);\n-\tif (write_file_gently(git_path(edit_description), \"%s\", buf.buf)) {\n-\t\tstrbuf_release(&buf);\n-\t\treturn error_errno(_(\"could not write branch description template\"));\n-\t}\n+\twrite_file(git_path(edit_description), \"%s\", buf.buf);\n \tstrbuf_reset(&buf);\n \tif (launch_editor(git_path(edit_description), &buf, NULL)) {\n \t\tstrbuf_release(&buf);\n-- \n2.9.0.393.g704e522\n\n"},{"id":"291017","messageId":"20160708090934.GD10152@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708090400.GA26594@sigill.intra.peff.net","subject":"[PATCH 4/8] write_file: drop \"gently\" form","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:09:34Z","receivedAt":"2016-07-08T09:09:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are no callers left of write_file_gently(). Let's drop\nit, as it doesn't seem likely for new callers to be added\n(since its inception, the only callers who wanted the gentle\nform generally just died immediately themselves, and have\nsince been converted).\n\nWhile we're there, let's also drop the \"int\" return from\nwrite_file, as it is never meaningful (in the non-gentle\nform, we always either die or return 0).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h   |  3 +--\n wrapper.c | 54 +++++++++++-------------------------------------------\n 2 files changed, 12 insertions(+), 45 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex f1dc289..3f6c53f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1745,8 +1745,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n \treturn write_in_full(fd, str, strlen(str));\n }\n \n-extern int write_file(const char *path, const char *fmt, ...);\n-extern int write_file_gently(const char *path, const char *fmt, ...);\n+extern void write_file(const char *path, const char *fmt, ...);\n \n /* pager.c */\n extern void setup_pager(void);\ndiff --git a/wrapper.c b/wrapper.c\nindex 5dc4e15..0349441 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -640,56 +640,24 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)\n \treturn len;\n }\n \n-static int write_file_v(const char *path, int fatal,\n-\t\t\tconst char *fmt, va_list params)\n+void write_file(const char *path, const char *fmt, ...)\n {\n+\tva_list params;\n \tstruct strbuf sb = STRBUF_INIT;\n \tint fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);\n-\tif (fd < 0) {\n-\t\tif (fatal)\n-\t\t\tdie_errno(_(\"could not open %s for writing\"), path);\n-\t\treturn -1;\n-\t}\n-\tstrbuf_vaddf(&sb, fmt, params);\n-\tstrbuf_complete_line(&sb);\n-\tif (write_in_full(fd, sb.buf, sb.len) != sb.len) {\n-\t\tint err = errno;\n-\t\tclose(fd);\n-\t\tstrbuf_release(&sb);\n-\t\terrno = err;\n-\t\tif (fatal)\n-\t\t\tdie_errno(_(\"could not write to %s\"), path);\n-\t\treturn -1;\n-\t}\n-\tstrbuf_release(&sb);\n-\tif (close(fd)) {\n-\t\tif (fatal)\n-\t\t\tdie_errno(_(\"could not close %s\"), path);\n-\t\treturn -1;\n-\t}\n-\treturn 0;\n-}\n-\n-int write_file(const char *path, const char *fmt, ...)\n-{\n-\tint status;\n-\tva_list params;\n+\tif (fd < 0)\n+\t\tdie_errno(_(\"could not open %s for writing\"), path);\n \n \tva_start(params, fmt);\n-\tstatus = write_file_v(path, 1, fmt, params);\n+\tstrbuf_vaddf(&sb, fmt, params);\n \tva_end(params);\n-\treturn status;\n-}\n-\n-int write_file_gently(const char *path, const char *fmt, ...)\n-{\n-\tint status;\n-\tva_list params;\n \n-\tva_start(params, fmt);\n-\tstatus = write_file_v(path, 0, fmt, params);\n-\tva_end(params);\n-\treturn status;\n+\tstrbuf_complete_line(&sb);\n+\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n+\t\tdie_errno(_(\"could not write to %s\"), path);\n+\tstrbuf_release(&sb);\n+\tif (close(fd))\n+\t\tdie_errno(_(\"could not close %s\"), path);\n }\n \n void sleep_millisec(int millisec)\n-- \n2.9.0.393.g704e522\n\n"},{"id":"291018","messageId":"20160708091007.GE10152@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708090400.GA26594@sigill.intra.peff.net","subject":"[PATCH 5/8] write_file: use xopen","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:10:08Z","receivedAt":"2016-07-08T09:10:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This simplifies the code a tiny bit, and provides consistent\nerror messages with other users of xopen().\n\nWhile we're here, let's also switch to using O_WRONLY. We\nknow we're only going to open/write/close the file, so\nthere's no point in asking for O_RDWR.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n wrapper.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 0349441..7c126b8 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -644,9 +644,7 @@ void write_file(const char *path, const char *fmt, ...)\n {\n \tva_list params;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);\n-\tif (fd < 0)\n-\t\tdie_errno(_(\"could not open %s for writing\"), path);\n+\tint fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n \n \tva_start(params, fmt);\n \tstrbuf_vaddf(&sb, fmt, params);\n-- \n2.9.0.393.g704e522\n\n"},{"id":"291019","messageId":"20160708091222.GF10152@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708090400.GA26594@sigill.intra.peff.net","subject":"[PATCH 6/8] write_file: add pointer+len variant","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:12:22Z","receivedAt":"2016-07-08T09:12:35Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are many callsites which could use write_file, but for\nwhich it is a little awkward because they have a strbuf or\nother pointer/len combo. Specifically:\n\n 1. write_file() takes a format string, so we have to use\n    \"%s\" or \"%.*s\", which are ugly.\n\n 2. Using any form of \"%s\" does not handle embedded NULs in\n    the output. That probably doesn't matter for our\n    call-sites, but it's nicer not to have to worry.\n\n 3. It's less efficient; we format into another strbuf\n    just to do the write. That's probably not measurably\n    slow for our uses, but it's simply inelegant.\n\nWe can fix this by providing a helper to write out the\nformatted buffer, and just calling it from write_file().\n\nNote that we don't do the usual \"complete with a newline\"\nthat write_file does. If the caller has their own buffer,\nthere's a reasonable chance they're doing something more\ncomplicated than a single line, and they can call\nstrbuf_complete_line() themselves.\n\nWe could go even further and add strbuf_write_file(), but it\ndoesn't save much:\n\n  -  write_file_buf(path, sb.buf, sb.len);\n  +  strbuf_write_file(&sb, path);\n\nIt would also be somewhat asymmetric with strbuf_read_file,\nwhich actually returns errors rather than dying (and the\nerror handling is most of the benefit of write_file() in the\nfirst place).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h   |  6 ++++++\n wrapper.c | 16 +++++++++++-----\n 2 files changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 3f6c53f..9d6ad4f 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1745,6 +1745,12 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n \treturn write_in_full(fd, str, strlen(str));\n }\n \n+/**\n+ * Open (and truncate) the file at path, write the contents of buf to it,\n+ * and close it. Dies if any errors are encountered.\n+ */\n+extern void write_file_buf(const char *path, const char *buf, size_t len);\n+\n extern void write_file(const char *path, const char *fmt, ...);\n \n /* pager.c */\ndiff --git a/wrapper.c b/wrapper.c\nindex 7c126b8..b827206 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -640,22 +640,28 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)\n \treturn len;\n }\n \n+void write_file_buf(const char *path, const char *buf, size_t len)\n+{\n+\tint fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n+\tif (write_in_full(fd, buf, len) != len)\n+\t\tdie_errno(_(\"could not write to %s\"), path);\n+\tif (close(fd))\n+\t\tdie_errno(_(\"could not close %s\"), path);\n+}\n+\n void write_file(const char *path, const char *fmt, ...)\n {\n \tva_list params;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tint fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n \n \tva_start(params, fmt);\n \tstrbuf_vaddf(&sb, fmt, params);\n \tva_end(params);\n \n \tstrbuf_complete_line(&sb);\n-\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n-\t\tdie_errno(_(\"could not write to %s\"), path);\n+\n+\twrite_file_buf(path, sb.buf, sb.len);\n \tstrbuf_release(&sb);\n-\tif (close(fd))\n-\t\tdie_errno(_(\"could not close %s\"), path);\n }\n \n void sleep_millisec(int millisec)\n-- \n2.9.0.393.g704e522\n\n"},{"id":"291020","messageId":"20160708091241.GG10152@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708090400.GA26594@sigill.intra.peff.net","subject":"[PATCH 7/8] write_file: add format attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:12:42Z","receivedAt":"2016-07-08T09:13:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"This gives us compile-time checking of our format strings,\nwhich is a good thing.\n\nI had also hoped it would help with confusing write_file()\nand write_file_buf(), since the former's \"...\" can make it\nmatch the signature of the latter. But given that the buffer\nfor write_file_buf() is generally not a string literal, the\ncompiler won't complain unless -Wformat-nonliteral is on,\nand that creates a ton of false positives elsewhere in the\ncode base.\n\nWhile we're there, let's also give the function a docstring,\nwhich it never had.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n cache.h | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 9d6ad4f..c6a282c 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1751,6 +1751,14 @@ static inline ssize_t write_str_in_full(int fd, const char *str)\n  */\n extern void write_file_buf(const char *path, const char *buf, size_t len);\n \n+/**\n+ * Like write_file_buf(), but format the contents into a buffer first.\n+ * Additionally, write_file() will append a newline if one is not already\n+ * present, making it convenient to write text files:\n+ *\n+ *   write_file(path, \"counter: %d\", ctr);\n+ */\n+__attribute__((format (printf, 2, 3)))\n extern void write_file(const char *path, const char *fmt, ...);\n \n /* pager.c */\n-- \n2.9.0.393.g704e522\n\n"},{"id":"291021","messageId":"20160708091255.GH10152@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708090400.GA26594@sigill.intra.peff.net","subject":"[PATCH 8/8] use write_file_buf where applicable","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:12:55Z","receivedAt":"2016-07-08T09:13:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"There are several places where we open a file, write some\ncontent from a strbuf, and close it. These can be simplified\nwith write_file_buf(). As a bonus, many of these did not\ncatch write problems at close() time.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/am.c    |  7 +------\n builtin/merge.c | 45 +++++----------------------------------------\n 2 files changed, 6 insertions(+), 46 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 339e360..3ac2448 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -403,13 +403,8 @@ static int read_commit_msg(struct am_state *state)\n  */\n static void write_commit_msg(const struct am_state *state)\n {\n-\tint fd;\n \tconst char *filename = am_path(state, \"final-commit\");\n-\n-\tfd = xopen(filename, O_WRONLY | O_CREAT, 0666);\n-\tif (write_in_full(fd, state->msg, state->msg_len) < 0)\n-\t\tdie_errno(_(\"could not write to %s\"), filename);\n-\tclose(fd);\n+\twrite_file_buf(filename, state->msg, state->msg_len);\n }\n \n /**\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex b555a1b..2e782db 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -336,15 +336,9 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead\n \tstruct rev_info rev;\n \tstruct strbuf out = STRBUF_INIT;\n \tstruct commit_list *j;\n-\tconst char *filename;\n-\tint fd;\n \tstruct pretty_print_context ctx = {0};\n \n \tprintf(_(\"Squash commit -- not updating HEAD\\n\"));\n-\tfilename = git_path_squash_msg();\n-\tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n-\tif (fd < 0)\n-\t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n \n \tinit_revisions(&rev, NULL);\n \trev.ignore_merges = 1;\n@@ -371,10 +365,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead\n \t\t\toid_to_hex(&commit->object.oid));\n \t\tpretty_print_commit(&ctx, commit, &out);\n \t}\n-\tif (write_in_full(fd, out.buf, out.len) != out.len)\n-\t\tdie_errno(_(\"Writing SQUASH_MSG\"));\n-\tif (close(fd))\n-\t\tdie_errno(_(\"Finishing SQUASH_MSG\"));\n+\twrite_file_buf(git_path_squash_msg(), out.buf, out.len);\n \tstrbuf_release(&out);\n }\n \n@@ -756,18 +747,6 @@ static void add_strategies(const char *string, unsigned attr)\n \n }\n \n-static void write_merge_msg(struct strbuf *msg)\n-{\n-\tconst char *filename = git_path_merge_msg();\n-\tint fd = open(filename, O_WRONLY | O_CREAT, 0666);\n-\tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open '%s' for writing\"),\n-\t\t\t  filename);\n-\tif (write_in_full(fd, msg->buf, msg->len) != msg->len)\n-\t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n-\tclose(fd);\n-}\n-\n static void read_merge_msg(struct strbuf *msg)\n {\n \tconst char *filename = git_path_merge_msg();\n@@ -801,7 +780,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)\n \tstrbuf_addch(&msg, '\\n');\n \tif (0 < option_edit)\n \t\tstrbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);\n-\twrite_merge_msg(&msg);\n+\twrite_file_buf(git_path_merge_msg(), msg.buf, msg.len);\n \tif (run_commit_hook(0 < option_edit, get_index_file(), \"prepare-commit-msg\",\n \t\t\t    git_path_merge_msg(), \"merge\", NULL))\n \t\tabort_commit(remoteheads, NULL);\n@@ -964,8 +943,6 @@ static int setup_with_upstream(const char ***argv)\n \n static void write_merge_state(struct commit_list *remoteheads)\n {\n-\tconst char *filename;\n-\tint fd;\n \tstruct commit_list *j;\n \tstruct strbuf buf = STRBUF_INIT;\n \n@@ -979,26 +956,14 @@ static void write_merge_state(struct commit_list *remoteheads)\n \t\t}\n \t\tstrbuf_addf(&buf, \"%s\\n\", oid_to_hex(oid));\n \t}\n-\tfilename = git_path_merge_head();\n-\tfd = open(filename, O_WRONLY | O_CREAT, 0666);\n-\tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n-\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n-\t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n-\tclose(fd);\n+\twrite_file_buf(git_path_merge_head(), buf.buf, buf.len);\n \tstrbuf_addch(&merge_msg, '\\n');\n-\twrite_merge_msg(&merge_msg);\n+\twrite_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);\n \n-\tfilename = git_path_merge_mode();\n-\tfd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n-\tif (fd < 0)\n-\t\tdie_errno(_(\"Could not open '%s' for writing\"), filename);\n \tstrbuf_reset(&buf);\n \tif (fast_forward == FF_NO)\n \t\tstrbuf_addf(&buf, \"no-ff\");\n-\tif (write_in_full(fd, buf.buf, buf.len) != buf.len)\n-\t\tdie_errno(_(\"Could not write to '%s'\"), filename);\n-\tclose(fd);\n+\twrite_file_buf(git_path_merge_mode(), buf.buf, buf.len);\n }\n \n static int default_edit_option(void)\n-- \n2.9.0.393.g704e522\n"},{"id":"291022","messageId":"20160708091653.GA17072@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708090400.GA26594@sigill.intra.peff.net","subject":"[PATCH 9/8] branch: use write_file_buf instead of write_file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:16:53Z","receivedAt":"2016-07-08T09:17:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"If we already have a strbuf, then using write_file_buf is a\nlittle nicer to read (no wondering whether \"%s\" will eat\nyour NULs), and it's more efficient (no extra formatting\nstep).\n\nWe don't care about the newline magic of write_file(), as we\nhave our own multi-line content.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nAlmost forgot this one. I had originally converted to write_file_buf\ndirectly, but later reshuffled the patches to make the refactoring more\nclear.\n\n builtin/branch.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 15232c4..1d41251 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -618,7 +618,7 @@ static int edit_branch_description(const char *branch_name)\n \t\t    \"  %s\\n\"\n \t\t    \"Lines starting with '%c' will be stripped.\\n\",\n \t\t    branch_name, comment_line_char);\n-\twrite_file(git_path(edit_description), \"%s\", buf.buf);\n+\twrite_file_buf(git_path(edit_description), buf.buf, buf.len);\n \tstrbuf_reset(&buf);\n \tif (launch_editor(git_path(edit_description), &buf, NULL)) {\n \t\tstrbuf_release(&buf);\n-- \n2.9.0.393.g704e522\n\n"},{"id":"291023","messageId":"20160708092510.GB17072@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708091241.GG10152@sigill.intra.peff.net","subject":"Re: [PATCH 7/8] write_file: add format attribute","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:25:10Z","receivedAt":"2016-07-08T09:25:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2016 at 05:12:41AM -0400, Jeff King wrote:\n\n> I had also hoped it would help with confusing write_file()\n> and write_file_buf(), since the former's \"...\" can make it\n> match the signature of the latter. But given that the buffer\n> for write_file_buf() is generally not a string literal, the\n> compiler won't complain unless -Wformat-nonliteral is on,\n> and that creates a ton of false positives elsewhere in the\n> code base.\n\nI poked around at the results of compiling with -Wformat-nonliteral, but\ngave up at trying to make it work. There are a number of clever uses of\nformats that would be hard to do otherwise. There are also several cases\nwhere a format string is used multiple times and we want to avoid\nrepeating it. But using #define doesn't work, because we want to be able\nto translate it.\n\nI did find a little bit of low-hanging fruit, though.\n\n  [1/2]: walker: let walker_say take arbitrary formats\n  [2/2]: avoid using sha1_to_hex output as printf format\n\nThese are totally independent of the main series, so they can be a\nseparate topic, go on top, or just get dropped entirely if it's not\nworth the trouble.\n\n-Peff\n"},{"id":"291024","messageId":"20160708092522.GA18263@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708092510.GB17072@sigill.intra.peff.net","subject":"[PATCH 1/2] walker: let walker_say take arbitrary formats","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:25:23Z","receivedAt":"2016-07-08T09:25:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We take a printf-style format and a single \"char *\"\nparameter, and the format must therefore have at most one\n\"%s\" in it. Besides being error-prone (and tickling\n-Wformat-nonliteral), this is unnecessarily restrictive. We\ncan just provide the usual varargs interface.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n walker.c | 10 +++++++---\n walker.h |  3 ++-\n 2 files changed, 9 insertions(+), 4 deletions(-)\n\ndiff --git a/walker.c b/walker.c\nindex d95b007..2c86e40 100644\n--- a/walker.c\n+++ b/walker.c\n@@ -9,10 +9,14 @@\n \n static unsigned char current_commit_sha1[20];\n \n-void walker_say(struct walker *walker, const char *fmt, const char *hex)\n+void walker_say(struct walker *walker, const char *fmt, ...)\n {\n-\tif (walker->get_verbosely)\n-\t\tfprintf(stderr, fmt, hex);\n+\tif (walker->get_verbosely) {\n+\t\tva_list ap;\n+\t\tva_start(ap, fmt);\n+\t\tvfprintf(stderr, fmt, ap);\n+\t\tva_end(ap);\n+\t}\n }\n \n static void report_missing(const struct object *obj)\ndiff --git a/walker.h b/walker.h\nindex 95e5765..a869013 100644\n--- a/walker.h\n+++ b/walker.h\n@@ -19,7 +19,8 @@ struct walker {\n };\n \n /* Report what we got under get_verbosely */\n-void walker_say(struct walker *walker, const char *, const char *);\n+__attribute__((format (printf, 2, 3)))\n+void walker_say(struct walker *walker, const char *fmt, ...);\n \n /* Load pull targets from stdin */\n int walker_targets_stdin(char ***target, const char ***write_ref);\n-- \n2.9.0.393.g704e522\n\n"},{"id":"291025","messageId":"20160708092526.GB18263@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708092510.GB17072@sigill.intra.peff.net","subject":"[PATCH 2/2] avoid using sha1_to_hex output as printf format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T09:25:26Z","receivedAt":"2016-07-08T09:25:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We know that it should not contain any percent-signs, but\nit's a good habit not to feed non-literals to printf\nformatters.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/worktree.c | 2 +-\n commit.c           | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex e866844..cce555c 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -262,7 +262,7 @@ static int add_worktree(const char *path, const char *refname,\n \t */\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/HEAD\", sb_repo.buf);\n-\twrite_file(sb.buf, sha1_to_hex(null_sha1));\n+\twrite_file(sb.buf, \"%s\", sha1_to_hex(null_sha1));\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/commondir\", sb_repo.buf);\n \twrite_file(sb.buf, \"../..\");\ndiff --git a/commit.c b/commit.c\nindex 3f4f371..9603379 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1623,7 +1623,7 @@ void print_commit_list(struct commit_list *list,\n {\n \tfor ( ; list; list = list->next) {\n \t\tconst char *format = list->next ? format_cur : format_last;\n-\t\tprintf(format, oid_to_hex(&list->item->object.oid));\n+\t\tprintf(format, \"%s\", oid_to_hex(&list->item->object.oid));\n \t}\n }\n \n-- \n2.9.0.393.g704e522\n"},{"id":"291027","messageId":"20160708103515.GA19705@sigill.intra.peff.net","threadId":"42796","inReplyTo":"20160708092526.GB18263@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] avoid using sha1_to_hex output as printf format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T10:35:15Z","receivedAt":"2016-07-08T10:35:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2016 at 05:25:26AM -0400, Jeff King wrote:\n\n> diff --git a/commit.c b/commit.c\n> index 3f4f371..9603379 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -1623,7 +1623,7 @@ void print_commit_list(struct commit_list *list,\n>  {\n>  \tfor ( ; list; list = list->next) {\n>  \t\tconst char *format = list->next ? format_cur : format_last;\n> -\t\tprintf(format, oid_to_hex(&list->item->object.oid));\n> +\t\tprintf(format, \"%s\", oid_to_hex(&list->item->object.oid));\n\nUrgh, this second hunk is clearly bogus. This is a -Wformat-nonliteral\nproblem, but not because of oid_to_hex(), but rather because of\n\"format\". :-/\n\nHere's a corrected patch. But as this has demonstrated the dangers of\nchurn, and as it doesn't really get us meaningfully closer to being able\nto use -Wformat-nonliteral, perhaps the best course of action is to just\ndrop it (I do think the \"walker_say\" patch has more inherent value as a\ncleanup, though).\n\n-- >8 --\nSubject: [PATCH] avoid using sha1_to_hex output as printf format\n\nWe know that it should not contain any percent-signs, but\nit's a good habit not to feed non-literals to printf\nformatters.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n builtin/worktree.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/builtin/worktree.c b/builtin/worktree.c\nindex e866844..cce555c 100644\n--- a/builtin/worktree.c\n+++ b/builtin/worktree.c\n@@ -262,7 +262,7 @@ static int add_worktree(const char *path, const char *refname,\n \t */\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/HEAD\", sb_repo.buf);\n-\twrite_file(sb.buf, sha1_to_hex(null_sha1));\n+\twrite_file(sb.buf, \"%s\", sha1_to_hex(null_sha1));\n \tstrbuf_reset(&sb);\n \tstrbuf_addf(&sb, \"%s/commondir\", sb_repo.buf);\n \twrite_file(sb.buf, \"../..\");\n-- \n2.9.0.393.g704e522\n\n"},{"id":"291038","messageId":"xmqqinwgm5aa.fsf@gitster.mtv.corp.google.com","threadId":"42796","inReplyTo":"20160708103515.GA19705@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] avoid using sha1_to_hex output as printf format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-08T17:02:53Z","receivedAt":"2016-07-08T17:03:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Fri, Jul 08, 2016 at 05:25:26AM -0400, Jeff King wrote:\n>\n>> diff --git a/commit.c b/commit.c\n>> index 3f4f371..9603379 100644\n>> --- a/commit.c\n>> +++ b/commit.c\n>> @@ -1623,7 +1623,7 @@ void print_commit_list(struct commit_list *list,\n>>  {\n>>  \tfor ( ; list; list = list->next) {\n>>  \t\tconst char *format = list->next ? format_cur : format_last;\n>> -\t\tprintf(format, oid_to_hex(&list->item->object.oid));\n>> +\t\tprintf(format, \"%s\", oid_to_hex(&list->item->object.oid));\n>\n> Urgh, this second hunk is clearly bogus. This is a -Wformat-nonliteral\n> problem, but not because of oid_to_hex(), but rather because of\n> \"format\". :-/\n>\n> Here's a corrected patch. But as this has demonstrated the dangers of\n> churn, and as it doesn't really get us meaningfully closer to being able\n> to use -Wformat-nonliteral, perhaps the best course of action is to just\n> drop it (I do think the \"walker_say\" patch has more inherent value as a\n> cleanup, though).\n\nHmm.  While both do look correct, and it is a no-brainer to take\nthis (corrected) patch, I am not sure how much we care about walkers\nthese days.\n\nAs to the hunk to commit.c that was dropped in this round, the only\ncaller of print_commit_list() is bisect.c, and it passes \"%s\\n\" to\nformat_cur and format_last, it seems, so that suggests a more\nobvious direction for cleaning things up, I would say.\n\n> -- >8 --\n> Subject: [PATCH] avoid using sha1_to_hex output as printf format\n>\n> We know that it should not contain any percent-signs, but\n> it's a good habit not to feed non-literals to printf\n> formatters.\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n> ---\n>  builtin/worktree.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/builtin/worktree.c b/builtin/worktree.c\n> index e866844..cce555c 100644\n> --- a/builtin/worktree.c\n> +++ b/builtin/worktree.c\n> @@ -262,7 +262,7 @@ static int add_worktree(const char *path, const char *refname,\n>  \t */\n>  \tstrbuf_reset(&sb);\n>  \tstrbuf_addf(&sb, \"%s/HEAD\", sb_repo.buf);\n> -\twrite_file(sb.buf, sha1_to_hex(null_sha1));\n> +\twrite_file(sb.buf, \"%s\", sha1_to_hex(null_sha1));\n>  \tstrbuf_reset(&sb);\n>  \tstrbuf_addf(&sb, \"%s/commondir\", sb_repo.buf);\n>  \twrite_file(sb.buf, \"../..\");\n"},{"id":"291039","messageId":"xmqqeg74m4zb.fsf@gitster.mtv.corp.google.com","threadId":"42796","inReplyTo":"xmqqinwgm5aa.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] avoid using sha1_to_hex output as printf format","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-08T17:09:28Z","receivedAt":"2016-07-08T17:09:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> As to the hunk to commit.c that was dropped in this round, the only\n> caller of print_commit_list() is bisect.c, and it passes \"%s\\n\" to\n> format_cur and format_last, it seems, so that suggests a more\n> obvious direction for cleaning things up, I would say.\n\nAnd the result is a pleasing diffstat.\n\n-- >8 --\nSubject: commit.c: remove print_commit_list()\n\nThe helper function tries to offer a way to conveniently show the\nlast one differently from others, presumably to allow you to say\nsomething like\n\n\tA, B, and C.\n\nwhile iterating over a list that has these three elements.\n\nHowever, there is only one caller, and it passes the same format\nstring \"%s\\n\" for both the last one and the other ones.  Retire the\nhelper function and update the caller with a simplified version.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n bisect.c |  5 ++++-\n commit.c | 10 ----------\n commit.h |  4 ----\n 3 files changed, 4 insertions(+), 15 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex dc13319..02f76f0 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -646,7 +646,10 @@ static void exit_if_skipped_commits(struct commit_list *tried,\n \n \tprintf(\"There are only 'skip'ped commits left to test.\\n\"\n \t       \"The first %s commit could be any of:\\n\", term_bad);\n-\tprint_commit_list(tried, \"%s\\n\", \"%s\\n\");\n+\n+\tfor ( ; tried; tried = tried->next)\n+\t\tprintf(\"%s\\n\", oid_to_hex(&tried->item->object.oid));\n+\n \tif (bad)\n \t\tprintf(\"%s\\n\", oid_to_hex(bad));\n \tprintf(\"We cannot bisect more!\\n\");\ndiff --git a/commit.c b/commit.c\nindex 3f4f371..bf27972 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -1617,16 +1617,6 @@ struct commit_list **commit_list_append(struct commit *commit,\n \treturn &new->next;\n }\n \n-void print_commit_list(struct commit_list *list,\n-\t\t       const char *format_cur,\n-\t\t       const char *format_last)\n-{\n-\tfor ( ; list; list = list->next) {\n-\t\tconst char *format = list->next ? format_cur : format_last;\n-\t\tprintf(format, oid_to_hex(&list->item->object.oid));\n-\t}\n-}\n-\n const char *find_commit_header(const char *msg, const char *key, size_t *out_len)\n {\n \tint key_len = strlen(key);\ndiff --git a/commit.h b/commit.h\nindex 78ed513..71693ce 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -376,10 +376,6 @@ extern int parse_signed_commit(const struct commit *commit,\n \t\t\t       struct strbuf *message, struct strbuf *signature);\n extern int remove_signature(struct strbuf *buf);\n \n-extern void print_commit_list(struct commit_list *list,\n-\t\t\t      const char *format_cur,\n-\t\t\t      const char *format_last);\n-\n /*\n  * Check the signature of the given commit. The result of the check is stored\n  * in sig->check_result, 'G' for a good signature, 'U' for a good signature\n"},{"id":"291048","messageId":"577FF48C.4030008@web.de","threadId":"42796","inReplyTo":"alpine.DEB.2.20.1607080830430.6426@virtualbox","subject":"Re: [PATCH] am: ignore return value of write_file()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-07-08T18:44:28Z","receivedAt":"2016-07-08T18:44:46Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Hi Dscho,\n\nAm 08.07.2016 um 08:33 schrieb Johannes Schindelin:\n> On Thu, 7 Jul 2016, René Scharfe wrote:\n>> write_file() either returns 0 or dies, so there is no point in checking\n>> its return value.\n>\n> The question is whether it makes sense for write_file() to die(). It is a\n> library function and not every caller can be happy with that function to\n> exit the program when some file could not be written, without a chance to\n> tell the user what to do about the situation.\n>\n> If write_file() was defined in builtin/am.c, as a static function, I would\n> grudgingly acquiesce, but it is not.\n>\n> IMO it would be better to fix write_file() to *not* die() but return\n> error() instead.\n\nthere is write_file_gently() for that purpose, but it's used only by a \nsingle caller that exits on failure after all, and in fact Peff's series \ndrops it.\n\nSo I think write_file() is fine, and it's rather a question of whether \nam should use write_file_gently() instead.  I don't see why, but perhaps \nthat's because it's Friday..\n\nRené\n"},{"id":"291049","messageId":"577FF48E.4060905@web.de","threadId":"42796","inReplyTo":"20160708090400.GA26594@sigill.intra.peff.net","subject":"Re: [PATCH 0/8] write_file cleanups","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2016-07-08T18:44:30Z","receivedAt":"2016-07-08T18:44:55Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 08.07.2016 um 11:04 schrieb Jeff King:\n> Here it is. There actually weren't that many spots to clean up, as quite\n> a few of them have a \"twist\" where they want to do something clever,\n> like open the file and feed the descriptor to a sub-function, or open\n> with funny things like O_EXCL.\n>\n> But still, the diffstat is pleasing:\n>\n>   builtin/am.c     | 25 +++++++----------\n>   builtin/branch.c |  5 +---\n>   builtin/config.c |  2 +-\n>   builtin/merge.c  | 45 ++++--------------------------\n>   cache.h          | 17 ++++++++++--\n>   wrapper.c        | 52 ++++++++---------------------------\n>   6 files changed, 44 insertions(+), 102 deletions(-)\n>\n> and that even includes adding some function documentation.\n\nHaha, feels like Candy Crush. :)\n\n(Sent a single small patch, lots of places get patched as if by magic.)\n\nThanks,\nRené\n"},{"id":"291058","messageId":"20160708214109.GA9820@sigill.intra.peff.net","threadId":"42796","inReplyTo":"xmqqeg74m4zb.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/2] avoid using sha1_to_hex output as printf format","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T21:41:09Z","receivedAt":"2016-07-08T21:41:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2016 at 10:09:28AM -0700, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > As to the hunk to commit.c that was dropped in this round, the only\n> > caller of print_commit_list() is bisect.c, and it passes \"%s\\n\" to\n> > format_cur and format_last, it seems, so that suggests a more\n> > obvious direction for cleaning things up, I would say.\n> \n> And the result is a pleasing diffstat.\n> \n> -- >8 --\n> Subject: commit.c: remove print_commit_list()\n\nYeah, I agree this is a nice diffstat.\n\nAs for the \"walker\" thing, I also agree that we don't really care much\nabout it these days.\n\nI really wish we could just kill it off; it would simplify the http code\nsignificantly (e.g., the entire reason we have the whole async\nmulti-fetch setup is to support it). But every time I think that,\nsomebody mentions on the list that they are still using it. :-/\n\nI do suspect we could kill off the http push-over-dav, though. It's less\nlikely to be useful than the fetch code.\n\n-Peff\n"},{"id":"291060","messageId":"20160708215114.GC9820@sigill.intra.peff.net","threadId":"42796","inReplyTo":"577FF48C.4030008@web.de","subject":"Re: [PATCH] am: ignore return value of write_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-08T21:51:14Z","receivedAt":"2016-07-08T21:51:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jul 08, 2016 at 08:44:28PM +0200, René Scharfe wrote:\n\n> > The question is whether it makes sense for write_file() to die(). It is a\n> > library function and not every caller can be happy with that function to\n> > exit the program when some file could not be written, without a chance to\n> > tell the user what to do about the situation.\n> > \n> > If write_file() was defined in builtin/am.c, as a static function, I would\n> > grudgingly acquiesce, but it is not.\n> > \n> > IMO it would be better to fix write_file() to *not* die() but return\n> > error() instead.\n> \n> there is write_file_gently() for that purpose, but it's used only by a\n> single caller that exits on failure after all, and in fact Peff's series\n> drops it.\n\nYeah, I always feel funny going in the opposite direction of\nlibification, as in this case. But having looked at the set of current\nand potential-to-convert callers, I couldn't find a single one which\nwould want the gentle behavior. Any site which doesn't die also wanted\nsomething else more complex (e.g., different open options).\n\nSo I think rather than loading down write_file() with options that will\nmake the simple callers harder to read, we are better off to keep its\nimplementation simple, and let people call its building blocks easily.\n\nAnd I think we already have that; in my final version it really is just\nxopen/write_in_full/close. So the non-simple sites can still make use of\nthose components.\n\n-Peff\n"},{"id":"291103","messageId":"alpine.DEB.2.20.1607091624120.6426@virtualbox","threadId":"42796","inReplyTo":"20160708065650.GA10664@sigill.intra.peff.net","subject":"Re: [PATCH] am: ignore return value of write_file()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-09T14:24:42Z","receivedAt":"2016-07-09T14:25:11Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Fri, 8 Jul 2016, Jeff King wrote:\n\n> I think we can clean that up, though. I'll hopefully have a series in a\n> few minutes.\n\nYou caught me at busy times... I'll review it tomorrow, promise!\n\nCiao,\nDscho\n"},{"id":"291129","messageId":"alpine.DEB.2.20.1607101251230.6426@virtualbox","threadId":"42796","inReplyTo":"alpine.DEB.2.20.1607091624120.6426@virtualbox","subject":"Re: [PATCH] am: ignore return value of write_file()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-10T10:53:20Z","receivedAt":"2016-07-10T10:54:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Sat, 9 Jul 2016, Johannes Schindelin wrote:\n\n> On Fri, 8 Jul 2016, Jeff King wrote:\n> \n> > I think we can clean that up, though. I'll hopefully have a series in a\n> > few minutes.\n> \n> You caught me at busy times... I'll review it tomorrow, promise!\n\nAnd so I did. Looks good to me, I was surprised that there were no\npossible callers is the non-builtin code. But I guess they all use the\nlockfile mechanism now.\n\nCiao,\nDscho\n"}]}