{"thread":{"id":"47080","subject":"[PATCH 1/2] sequencer: factor out rewrite_file()","startedAt":"2017-10-31T09:54:40Z","lastAt":"2017-12-25T10:29:04Z","messageCount":35,"participants":["René Scharfe","Kevin Daudt","Simon Ruderich","Johannes Schindelin","Jeff King","Junio C Hamano","Johannes Sixt","Randall S. Becker"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"331467","messageId":"6150c80b-cb0e-06d4-63a7-a4f4a9107ab2@web.de","threadId":"47080","inReplyTo":null,"subject":"[PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-10-31T09:54:21Z","receivedAt":"2017-10-31T09:54:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Reduce code duplication by extracting a function for rewriting an\nexisting file.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n sequencer.c | 46 +++++++++++++++++-----------------------------\n 1 file changed, 17 insertions(+), 29 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f2a10cc4f2..17360eb38a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2665,6 +2665,20 @@ int check_todo_list(void)\n \treturn res;\n }\n \n+static int rewrite_file(const char *path, const char *buf, size_t len)\n+{\n+\tint rc = 0;\n+\tint fd = open(path, O_WRONLY);\n+\tif (fd < 0)\n+\t\treturn error_errno(_(\"could not open '%s' for writing\"), path);\n+\tif (write_in_full(fd, buf, len) < 0)\n+\t\trc = error_errno(_(\"could not write to '%s'\"), path);\n+\tif (!rc && ftruncate(fd, len) < 0)\n+\t\trc = error_errno(_(\"could not truncate '%s'\"), path);\n+\tclose(fd);\n+\treturn rc;\n+}\n+\n /* skip picking commits whose parents are unchanged */\n int skip_unnecessary_picks(void)\n {\n@@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)\n \t\t}\n \t\tclose(fd);\n \n-\t\tfd = open(rebase_path_todo(), O_WRONLY, 0666);\n-\t\tif (fd < 0) {\n-\t\t\terror_errno(_(\"could not open '%s' for writing\"),\n-\t\t\t\t    rebase_path_todo());\n+\t\tif (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,\n+\t\t\t\t todo_list.buf.len - offset) < 0) {\n \t\t\ttodo_list_release(&todo_list);\n \t\t\treturn -1;\n \t\t}\n-\t\tif (write_in_full(fd, todo_list.buf.buf + offset,\n-\t\t\t\ttodo_list.buf.len - offset) < 0) {\n-\t\t\terror_errno(_(\"could not write to '%s'\"),\n-\t\t\t\t    rebase_path_todo());\n-\t\t\tclose(fd);\n-\t\t\ttodo_list_release(&todo_list);\n-\t\t\treturn -1;\n-\t\t}\n-\t\tif (ftruncate(fd, todo_list.buf.len - offset) < 0) {\n-\t\t\terror_errno(_(\"could not truncate '%s'\"),\n-\t\t\t\t    rebase_path_todo());\n-\t\t\ttodo_list_release(&todo_list);\n-\t\t\tclose(fd);\n-\t\t\treturn -1;\n-\t\t}\n-\t\tclose(fd);\n \n \t\ttodo_list.current = i;\n \t\tif (is_fixup(peek_command(&todo_list, 0)))\n@@ -2944,15 +2940,7 @@ int rearrange_squash(void)\n \t\t\t}\n \t\t}\n \n-\t\tfd = open(todo_file, O_WRONLY);\n-\t\tif (fd < 0)\n-\t\t\tres = error_errno(_(\"could not open '%s'\"), todo_file);\n-\t\telse if (write(fd, buf.buf, buf.len) < 0)\n-\t\t\tres = error_errno(_(\"could not write to '%s'\"), todo_file);\n-\t\telse if (ftruncate(fd, buf.len) < 0)\n-\t\t\tres = error_errno(_(\"could not truncate '%s'\"),\n-\t\t\t\t\t   todo_file);\n-\t\tclose(fd);\n+\t\tres = rewrite_file(todo_file, buf.buf, buf.len);\n \t\tstrbuf_release(&buf);\n \t}\n \n-- \n2.15.0\n"},{"id":"331468","messageId":"6b8e2a79-302e-7e69-00bd-f4643d5195af@web.de","threadId":"47080","inReplyTo":"6150c80b-cb0e-06d4-63a7-a4f4a9107ab2@web.de","subject":"[PATCH 2/2] sequencer: use O_TRUNC to truncate files","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-10-31T09:58:16Z","receivedAt":"2017-10-31T10:05:41Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Cut off any previous content of the file to be rewritten by passing the\nflag O_TRUNC to open(2) instead of calling ftruncate(2) at the end.\nThat's easier and shorter.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n sequencer.c | 4 +---\n 1 file changed, 1 insertion(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 17360eb38a..f93b60f615 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2668,13 +2668,11 @@ int check_todo_list(void)\n static int rewrite_file(const char *path, const char *buf, size_t len)\n {\n \tint rc = 0;\n-\tint fd = open(path, O_WRONLY);\n+\tint fd = open(path, O_WRONLY | O_TRUNC);\n \tif (fd < 0)\n \t\treturn error_errno(_(\"could not open '%s' for writing\"), path);\n \tif (write_in_full(fd, buf, len) < 0)\n \t\trc = error_errno(_(\"could not write to '%s'\"), path);\n-\tif (!rc && ftruncate(fd, len) < 0)\n-\t\trc = error_errno(_(\"could not truncate '%s'\"), path);\n \tclose(fd);\n \treturn rc;\n }\n-- \n2.15.0\n"},{"id":"331478","messageId":"20171031163418.GB19161@alpha.vpn.ikke.info","threadId":"47080","inReplyTo":"6b8e2a79-302e-7e69-00bd-f4643d5195af@web.de","subject":"Re: [PATCH 2/2] sequencer: use O_TRUNC to truncate files","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-10-31T16:34:18Z","receivedAt":"2017-10-31T16:34:24Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Oct 31, 2017 at 10:58:16AM +0100, René Scharfe wrote:\n> Cut off any previous content of the file to be rewritten by passing the\n> flag O_TRUNC to open(2) instead of calling ftruncate(2) at the end.\n> That's easier and shorter.\n> \n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  sequencer.c | 4 +---\n>  1 file changed, 1 insertion(+), 3 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index 17360eb38a..f93b60f615 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2668,13 +2668,11 @@ int check_todo_list(void)\n>  static int rewrite_file(const char *path, const char *buf, size_t len)\n>  {\n>  \tint rc = 0;\n> -\tint fd = open(path, O_WRONLY);\n> +\tint fd = open(path, O_WRONLY | O_TRUNC);\n>  \tif (fd < 0)\n>  \t\treturn error_errno(_(\"could not open '%s' for writing\"), path);\n>  \tif (write_in_full(fd, buf, len) < 0)\n>  \t\trc = error_errno(_(\"could not write to '%s'\"), path);\n> -\tif (!rc && ftruncate(fd, len) < 0)\n> -\t\trc = error_errno(_(\"could not truncate '%s'\"), path);\n>  \tclose(fd);\n>  \treturn rc;\n>  }\n> -- \n> 2.15.0\n\nMakes sense\n"},{"id":"331483","messageId":"20171031163357.GA19161@alpha.vpn.ikke.info","threadId":"47080","inReplyTo":"6150c80b-cb0e-06d4-63a7-a4f4a9107ab2@web.de","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-10-31T16:33:57Z","receivedAt":"2017-10-31T17:12:00Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:\n> Reduce code duplication by extracting a function for rewriting an\n> existing file.\n> \n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  sequencer.c | 46 +++++++++++++++++-----------------------------\n>  1 file changed, 17 insertions(+), 29 deletions(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index f2a10cc4f2..17360eb38a 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2665,6 +2665,20 @@ int check_todo_list(void)\n>  \treturn res;\n>  }\n>  \n> +static int rewrite_file(const char *path, const char *buf, size_t len)\n> +{\n> +\tint rc = 0;\n> +\tint fd = open(path, O_WRONLY);\n> +\tif (fd < 0)\n> +\t\treturn error_errno(_(\"could not open '%s' for writing\"), path);\n> +\tif (write_in_full(fd, buf, len) < 0)\n> +\t\trc = error_errno(_(\"could not write to '%s'\"), path);\n> +\tif (!rc && ftruncate(fd, len) < 0)\n> +\t\trc = error_errno(_(\"could not truncate '%s'\"), path);\n> +\tclose(fd);\n> +\treturn rc;\n> +}\n> +\n>  /* skip picking commits whose parents are unchanged */\n>  int skip_unnecessary_picks(void)\n>  {\n> @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)\n>  \t\t}\n>  \t\tclose(fd);\n>  \n> -\t\tfd = open(rebase_path_todo(), O_WRONLY, 0666);\n> -\t\tif (fd < 0) {\n> -\t\t\terror_errno(_(\"could not open '%s' for writing\"),\n> -\t\t\t\t    rebase_path_todo());\n> +\t\tif (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,\n> +\t\t\t\t todo_list.buf.len - offset) < 0) {\n>  \t\t\ttodo_list_release(&todo_list);\n>  \t\t\treturn -1;\n>  \t\t}\n> -\t\tif (write_in_full(fd, todo_list.buf.buf + offset,\n> -\t\t\t\ttodo_list.buf.len - offset) < 0) {\n> -\t\t\terror_errno(_(\"could not write to '%s'\"),\n> -\t\t\t\t    rebase_path_todo());\n> -\t\t\tclose(fd);\n> -\t\t\ttodo_list_release(&todo_list);\n\nIs this missing on purpose in the new situation?\n\n> -\t\t\treturn -1;\n> -\t\t}\n> -\t\tif (ftruncate(fd, todo_list.buf.len - offset) < 0) {\n> -\t\t\terror_errno(_(\"could not truncate '%s'\"),\n> -\t\t\t\t    rebase_path_todo());\n> -\t\t\ttodo_list_release(&todo_list);\n> -\t\t\tclose(fd);\n> -\t\t\treturn -1;\n> -\t\t}\n> -\t\tclose(fd);\n>  \n>  \t\ttodo_list.current = i;\n>  \t\tif (is_fixup(peek_command(&todo_list, 0)))\n> @@ -2944,15 +2940,7 @@ int rearrange_squash(void)\n>  \t\t\t}\n>  \t\t}\n>  \n> -\t\tfd = open(todo_file, O_WRONLY);\n> -\t\tif (fd < 0)\n> -\t\t\tres = error_errno(_(\"could not open '%s'\"), todo_file);\n> -\t\telse if (write(fd, buf.buf, buf.len) < 0)\n> -\t\t\tres = error_errno(_(\"could not write to '%s'\"), todo_file);\n> -\t\telse if (ftruncate(fd, buf.len) < 0)\n> -\t\t\tres = error_errno(_(\"could not truncate '%s'\"),\n> -\t\t\t\t\t   todo_file);\n> -\t\tclose(fd);\n> +\t\tres = rewrite_file(todo_file, buf.buf, buf.len);\n>  \t\tstrbuf_release(&buf);\n>  \t}\n>  \n> -- \n> 2.15.0\n\nExcept for that question, it looks good to me (as a beginner), it makes\nthe code better readable.\n"},{"id":"331544","messageId":"20171101060608.GA1076@alpha.vpn.ikke.info","threadId":"47080","inReplyTo":"20171031163357.GA19161@alpha.vpn.ikke.info","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Kevin Daudt","fromEmail":"me@ikke.info","sentAt":"2017-11-01T06:06:08Z","receivedAt":"2017-11-01T06:06:14Z","isPatch":true,"sender":{"key":"me@ikke.info","avatar":"https://avatars.githubusercontent.com/u/135698?v=4"},"body":"On Tue, Oct 31, 2017 at 05:33:57PM +0100, Kevin Daudt wrote:\n> On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:\n> > Reduce code duplication by extracting a function for rewriting an\n> > existing file.\n> > \n> > Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> > ---\n> >  sequencer.c | 46 +++++++++++++++++-----------------------------\n> >  1 file changed, 17 insertions(+), 29 deletions(-)\n> > \n> > diff --git a/sequencer.c b/sequencer.c > > index f2a10cc4f2..17360eb38a 100644\n> > --- a/sequencer.c\n> > +++ b/sequencer.c\n> > @@ -2665,6 +2665,20 @@ int check_todo_list(void)\n> >  \treturn res;\n> >  }\n> >  \n> > +static int rewrite_file(const char *path, const char *buf, size_t len)\n> > +{\n> > +\tint rc = 0;\n> > +\tint fd = open(path, O_WRONLY);\n> > +\tif (fd < 0)\n> > +\t\treturn error_errno(_(\"could not open '%s' for writing\"), path);\n> > +\tif (write_in_full(fd, buf, len) < 0)\n> > +\t\trc = error_errno(_(\"could not write to '%s'\"), path);\n> > +\tif (!rc && ftruncate(fd, len) < 0)\n> > +\t\trc = error_errno(_(\"could not truncate '%s'\"), path);\n> > +\tclose(fd);\n> > +\treturn rc;\n> > +}\n> > +\n> >  /* skip picking commits whose parents are unchanged */\n> >  int skip_unnecessary_picks(void)\n> >  {\n> > @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)\n> >  \t\t}\n> >  \t\tclose(fd);\n> >  \n> > -\t\tfd = open(rebase_path_todo(), O_WRONLY, 0666);\n> > -\t\tif (fd < 0) {\n> > -\t\t\terror_errno(_(\"could not open '%s' for writing\"),\n> > -\t\t\t\t    rebase_path_todo());\n> > +\t\tif (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,\n> > +\t\t\t\t todo_list.buf.len - offset) < 0) {\n> >  \t\t\ttodo_list_release(&todo_list);\n> >  \t\t\treturn -1;\n> >  \t\t}\n> > -\t\tif (write_in_full(fd, todo_list.buf.buf + offset,\n> > -\t\t\t\ttodo_list.buf.len - offset) < 0) {\n> > -\t\t\terror_errno(_(\"could not write to '%s'\"),\n> > -\t\t\t\t    rebase_path_todo());\n> > -\t\t\tclose(fd);\n> > -\t\t\ttodo_list_release(&todo_list);\n> \n> Is this missing on purpose in the new situation?\n>\n\nI wasn't looking at the context, only the changed lines. After reading\nit again, it's clear that nothing is missing (the freeing of todo_list).\n\nKevin\n"},{"id":"331558","messageId":"20171101110715.e4s7td2weisog4wt@ruderich.org","threadId":"47080","inReplyTo":"6150c80b-cb0e-06d4-63a7-a4f4a9107ab2@web.de","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-11-01T11:10:50Z","receivedAt":"2017-11-01T11:10:58Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:\n> +static int rewrite_file(const char *path, const char *buf, size_t len)\n> +{\n> +\tint rc = 0;\n> +\tint fd = open(path, O_WRONLY);\n> +\tif (fd < 0)\n> +\t\treturn error_errno(_(\"could not open '%s' for writing\"), path);\n> +\tif (write_in_full(fd, buf, len) < 0)\n> +\t\trc = error_errno(_(\"could not write to '%s'\"), path);\n> +\tif (!rc && ftruncate(fd, len) < 0)\n> +\t\trc = error_errno(_(\"could not truncate '%s'\"), path);\n> +\tclose(fd);\n\nWe might want to check the return value of close() as some file\nsystems report write errors only on close. But I'm not sure how\nthe rest of Git's code-base handles this.\n\n> +\treturn rc;\n> +}\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"331565","messageId":"22afeefa-cdd5-cd32-0a7c-6bad4de79f05@web.de","threadId":"47080","inReplyTo":"20171101110715.e4s7td2weisog4wt@ruderich.org","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-11-01T13:00:11Z","receivedAt":"2017-11-01T13:00:25Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.11.2017 um 12:10 schrieb Simon Ruderich:\n> On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:\n>> +static int rewrite_file(const char *path, const char *buf, size_t len)\n>> +{\n>> +\tint rc = 0;\n>> +\tint fd = open(path, O_WRONLY);\n>> +\tif (fd < 0)\n>> +\t\treturn error_errno(_(\"could not open '%s' for writing\"), path);\n>> +\tif (write_in_full(fd, buf, len) < 0)\n>> +\t\trc = error_errno(_(\"could not write to '%s'\"), path);\n>> +\tif (!rc && ftruncate(fd, len) < 0)\n>> +\t\trc = error_errno(_(\"could not truncate '%s'\"), path);\n>> +\tclose(fd);\n> \n> We might want to check the return value of close() as some file\n> systems report write errors only on close. But I'm not sure how\n> the rest of Git's code-base handles this.\n\nMost calls are not checked, but that doesn't necessarily mean they need\nto (or should) stay that way.  The Linux man-page of close(2) spends\nmultiple paragraphs recommending to check its return value..  Care to\nsend a follow-up patch?\n\nRené\n"},{"id":"331567","messageId":"32c515d01d4257c1532004d0bf21b2c330f6b81b.1509547231.git.simon@ruderich.org","threadId":"47080","inReplyTo":"22afeefa-cdd5-cd32-0a7c-6bad4de79f05@web.de","subject":"[PATCH 1/2] wrapper.c: consistently quote filenames in error messages","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-11-01T14:44:44Z","receivedAt":"2017-11-01T14:44:51Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"All other error messages in the file use quotes around the file name.\n\nThis change removes two translations as \"could not write to '%s'\" and\n\"could not close '%s'\" are already translated and these two are the only\noccurrences without quotes.\n\nSigned-off-by: Simon Ruderich <simon@ruderich.org>\n---\n wrapper.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 61aba0b5c..d20356a77 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -569,7 +569,7 @@ static int warn_if_unremovable(const char *op, const char *file, int rc)\n \tif (!rc || errno == ENOENT)\n \t\treturn 0;\n \terr = errno;\n-\twarning_errno(\"unable to %s %s\", op, file);\n+\twarning_errno(\"unable to %s '%s'\", op, file);\n \terrno = err;\n \treturn rc;\n }\n@@ -583,7 +583,7 @@ int unlink_or_msg(const char *file, struct strbuf *err)\n \tif (!rc || errno == ENOENT)\n \t\treturn 0;\n \n-\tstrbuf_addf(err, \"unable to unlink %s: %s\",\n+\tstrbuf_addf(err, \"unable to unlink '%s': %s\",\n \t\t    file, strerror(errno));\n \treturn -1;\n }\n@@ -653,9 +653,9 @@ 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) < 0)\n-\t\tdie_errno(_(\"could not write to %s\"), path);\n+\t\tdie_errno(_(\"could not write to '%s'\"), path);\n \tif (close(fd))\n-\t\tdie_errno(_(\"could not close %s\"), path);\n+\t\tdie_errno(_(\"could not close '%s'\"), path);\n }\n \n void write_file(const char *path, const char *fmt, ...)\n-- \n2.15.0\n\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"331568","messageId":"06c33d3cfa35c0524ede2970ee3169d6c62eb5c1.1509547231.git.simon@ruderich.org","threadId":"47080","inReplyTo":"22afeefa-cdd5-cd32-0a7c-6bad4de79f05@web.de","subject":"[PATCH 2/2] sequencer.c: check return value of close() in rewrite_file()","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-11-01T14:45:42Z","receivedAt":"2017-11-01T14:45:48Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"Not checking close(2) can hide errors as not all errors are reported\nduring the write(2).\n\nSigned-off-by: Simon Ruderich <simon@ruderich.org>\n---\n\nOn Wed, Nov 01, 2017 at 02:00:11PM +0100, René Scharfe wrote:\n> Most calls are not checked, but that doesn't necessarily mean they need\n> to (or should) stay that way.  The Linux man-page of close(2) spends\n> multiple paragraphs recommending to check its return value..  Care to\n> send a follow-up patch?\n\nHello,\n\nSure, here is it.\n\nRegards\nSimon\n\n sequencer.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex f93b60f61..e0cc2f777 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -2673,7 +2673,8 @@ static int rewrite_file(const char *path, const char *buf, size_t len)\n \t\treturn error_errno(_(\"could not open '%s' for writing\"), path);\n \tif (write_in_full(fd, buf, len) < 0)\n \t\trc = error_errno(_(\"could not write to '%s'\"), path);\n-\tclose(fd);\n+\tif (close(fd) && !rc)\n+\t\trc = error_errno(_(\"could not close '%s'\"), path);\n \treturn rc;\n }\n \n-- \n2.15.0\n\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"331569","messageId":"alpine.DEB.2.21.1.1711011631490.6482@virtualbox","threadId":"47080","inReplyTo":"6150c80b-cb0e-06d4-63a7-a4f4a9107ab2@web.de","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-01T15:33:29Z","receivedAt":"2017-11-01T15:33:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Tue, 31 Oct 2017, René Scharfe wrote:\n\n> Reduce code duplication by extracting a function for rewriting an\n> existing file.\n\nFine by me. Thanks,\nDscho"},{"id":"331570","messageId":"alpine.DEB.2.21.1.1711011633520.6482@virtualbox","threadId":"47080","inReplyTo":"6b8e2a79-302e-7e69-00bd-f4643d5195af@web.de","subject":"Re: [PATCH 2/2] sequencer: use O_TRUNC to truncate files","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-01T15:34:10Z","receivedAt":"2017-11-01T15:34:21Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi René,\n\nOn Tue, 31 Oct 2017, René Scharfe wrote:\n\n> Cut off any previous content of the file to be rewritten by passing the\n> flag O_TRUNC to open(2) instead of calling ftruncate(2) at the end.\n> That's easier and shorter.\n\nSure.\n\nThanks,\nDscho"},{"id":"331578","messageId":"44fa927f-d4bc-9396-0a50-bc2366d4fadc@web.de","threadId":"47080","inReplyTo":"06c33d3cfa35c0524ede2970ee3169d6c62eb5c1.1509547231.git.simon@ruderich.org","subject":"Re: [PATCH 2/2] sequencer.c: check return value of close() in rewrite_file()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-11-01T17:09:49Z","receivedAt":"2017-11-01T17:10:03Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 01.11.2017 um 15:45 schrieb Simon Ruderich:\n> Not checking close(2) can hide errors as not all errors are reported\n> during the write(2).\n> \n> Signed-off-by: Simon Ruderich <simon@ruderich.org>\n> ---\n> \n> On Wed, Nov 01, 2017 at 02:00:11PM +0100, René Scharfe wrote:\n>> Most calls are not checked, but that doesn't necessarily mean they need\n>> to (or should) stay that way.  The Linux man-page of close(2) spends\n>> multiple paragraphs recommending to check its return value..  Care to\n>> send a follow-up patch?\n> \n> Hello,\n> \n> Sure, here is it.\n> \n> Regards\n> Simon\n> \n>   sequencer.c | 3 ++-\n>   1 file changed, 2 insertions(+), 1 deletion(-)\n> \n> diff --git a/sequencer.c b/sequencer.c\n> index f93b60f61..e0cc2f777 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -2673,7 +2673,8 @@ static int rewrite_file(const char *path, const char *buf, size_t len)\n>   \t\treturn error_errno(_(\"could not open '%s' for writing\"), path);\n>   \tif (write_in_full(fd, buf, len) < 0)\n>   \t\trc = error_errno(_(\"could not write to '%s'\"), path);\n> -\tclose(fd);\n> +\tif (close(fd) && !rc)\n> +\t\trc = error_errno(_(\"could not close '%s'\"), path);\n>   \treturn rc;\n>   }\n>   \n> \n\nLooks good to me, thank you!\n\nRené\n"},{"id":"331595","messageId":"20171101194732.fn4n46wppl35e2z2@sigill.intra.peff.net","threadId":"47080","inReplyTo":"6150c80b-cb0e-06d4-63a7-a4f4a9107ab2@web.de","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-01T19:47:32Z","receivedAt":"2017-11-01T19:47:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:\n\n> Reduce code duplication by extracting a function for rewriting an\n> existing file.\n\nThese patches look like an improvement on their own, but I wonder if we\nshouldn't just be using the existing write_file_buf() for this?\n\nCompared to your new function:\n\n> +static int rewrite_file(const char *path, const char *buf, size_t len)\n> +{\n> +\tint rc = 0;\n> +\tint fd = open(path, O_WRONLY);\n> +\tif (fd < 0)\n> +\t\treturn error_errno(_(\"could not open '%s' for writing\"), path);\n> +\tif (write_in_full(fd, buf, len) < 0)\n> +\t\trc = error_errno(_(\"could not write to '%s'\"), path);\n> +\tif (!rc && ftruncate(fd, len) < 0)\n> +\t\trc = error_errno(_(\"could not truncate '%s'\"), path);\n> +\tclose(fd);\n> +\treturn rc;\n> +}\n\n  - write_file_buf() uses O_TRUNC instead of ftruncate (but you end up\n    there in your second patch)\n\n  - it uses O_CREAT, which I think would be OK (we do not expect to\n    create the file, but it would work fine when the file does exist).\n\n  - it calls die() rather than returning an error. Looking at the\n    callsites, I'm inclined to say that would be fine. Failing to write\n    to the todo file is essentially a fatal error for sequencer code.\n\n-Peff\n"},{"id":"331615","messageId":"alpine.DEB.2.21.1.1711012240500.6482@virtualbox","threadId":"47080","inReplyTo":"20171101194732.fn4n46wppl35e2z2@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-01T21:46:14Z","receivedAt":"2017-11-01T21:46:32Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff,\n\nOn Wed, 1 Nov 2017, Jeff King wrote:\n\n> On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:\n> \n> > Reduce code duplication by extracting a function for rewriting an\n> > existing file.\n> \n> These patches look like an improvement on their own, but I wonder if we\n> shouldn't just be using the existing write_file_buf() for this?\n> \n> Compared to your new function:\n> \n> > +static int rewrite_file(const char *path, const char *buf, size_t len)\n> > +{\n> > +\tint rc = 0;\n> > +\tint fd = open(path, O_WRONLY);\n> > +\tif (fd < 0)\n> > +\t\treturn error_errno(_(\"could not open '%s' for writing\"), path);\n> > +\tif (write_in_full(fd, buf, len) < 0)\n> > +\t\trc = error_errno(_(\"could not write to '%s'\"), path);\n> > +\tif (!rc && ftruncate(fd, len) < 0)\n> > +\t\trc = error_errno(_(\"could not truncate '%s'\"), path);\n> > +\tclose(fd);\n> > +\treturn rc;\n> > +}\n> \n>   - write_file_buf() uses O_TRUNC instead of ftruncate (but you end up\n>     there in your second patch)\n> \n>   - it uses O_CREAT, which I think would be OK (we do not expect to\n>     create the file, but it would work fine when the file does exist).\n> \n>   - it calls die() rather than returning an error. Looking at the\n>     callsites, I'm inclined to say that would be fine. Failing to write\n>     to the todo file is essentially a fatal error for sequencer code.\n\nI spent substantial time on making the sequencer code libified (it was far\nfrom it). That die() call may look okay now, but it is not at all okay if\nwe want to make Git's source code cleaner and more reusable. And I want\nto.\n\nSo my suggestion is to clean up write_file_buf() first, to stop behaving\nlike a drunk lemming, and to return an error value already, and only then\nuse it in sequencer.c.\n\nCiao,\nDscho\n\nP.S.: The existing callers of write_file_buf() don't care because they are\nbuiltins, and for some reason we deem it okay for code in builtins to\nsimply die() deep in the call chains, without any way for callers to give\nadvice how to get out of the current mess."},{"id":"331621","messageId":"20171101221618.4ioog7jlp7n2nd53@sigill.intra.peff.net","threadId":"47080","inReplyTo":"alpine.DEB.2.21.1.1711012240500.6482@virtualbox","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-01T22:16:18Z","receivedAt":"2017-11-01T22:16:24Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote:\n\n> >   - it calls die() rather than returning an error. Looking at the\n> >     callsites, I'm inclined to say that would be fine. Failing to write\n> >     to the todo file is essentially a fatal error for sequencer code.\n> \n> I spent substantial time on making the sequencer code libified (it was far\n> from it). That die() call may look okay now, but it is not at all okay if\n> we want to make Git's source code cleaner and more reusable. And I want\n> to.\n> \n> So my suggestion is to clean up write_file_buf() first, to stop behaving\n> like a drunk lemming, and to return an error value already, and only then\n> use it in sequencer.c.\n\nThat would be fine with me, too.\n\n-Peff\n"},{"id":"331642","messageId":"xmqqvaitqon6.fsf@gitster.mtv.corp.google.com","threadId":"47080","inReplyTo":"32c515d01d4257c1532004d0bf21b2c330f6b81b.1509547231.git.simon@ruderich.org","subject":"Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-02T04:40:29Z","receivedAt":"2017-11-02T04:40:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Ruderich <simon@ruderich.org> writes:\n\n> All other error messages in the file use quotes around the file name.\n>\n> This change removes two translations as \"could not write to '%s'\" and\n> \"could not close '%s'\" are already translated and these two are the only\n> occurrences without quotes.\n>\n> Signed-off-by: Simon Ruderich <simon@ruderich.org>\n> ---\n>  wrapper.c | 8 ++++----\n>  1 file changed, 4 insertions(+), 4 deletions(-)\n\nThis patch is incomplete without adjusting a handful of tests to\nexpect the updated messages, no?\n"},{"id":"331644","messageId":"xmqqlgjpqmyj.fsf@gitster.mtv.corp.google.com","threadId":"47080","inReplyTo":"xmqqvaitqon6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-02T05:16:52Z","receivedAt":"2017-11-02T05:17:06Z","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> Simon Ruderich <simon@ruderich.org> writes:\n>\n>> All other error messages in the file use quotes around the file name.\n>>\n>> This change removes two translations as \"could not write to '%s'\" and\n>> \"could not close '%s'\" are already translated and these two are the only\n>> occurrences without quotes.\n>>\n>> Signed-off-by: Simon Ruderich <simon@ruderich.org>\n>> ---\n>>  wrapper.c | 8 ++++----\n>>  1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> This patch is incomplete without adjusting a handful of tests to\n> expect the updated messages, no?\n\nI'll squash these in while queuing, but there might be more that I\ndidn't notice.\n\nThansk.\n\n t/t3600-rm.sh | 2 +-\n t/t7001-mv.sh | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t3600-rm.sh b/t/t3600-rm.sh\nindex f8568f8841..ab5500db44 100755\n--- a/t/t3600-rm.sh\n+++ b/t/t3600-rm.sh\n@@ -688,7 +688,7 @@ test_expect_success 'checking out a commit after submodule removal needs manual\n \tgit submodule update &&\n \tgit checkout -q HEAD^ &&\n \tgit checkout -q master 2>actual &&\n-\ttest_i18ngrep \"^warning: unable to rmdir submod:\" actual &&\n+\ttest_i18ngrep \"^warning: unable to rmdir '\\''submod'\\'':\" actual &&\n \tgit status -s submod >actual &&\n \techo \"?? submod/\" >expected &&\n \ttest_cmp expected actual &&\ndiff --git a/t/t7001-mv.sh b/t/t7001-mv.sh\nindex f5929c46f3..6e5031f56f 100755\n--- a/t/t7001-mv.sh\n+++ b/t/t7001-mv.sh\n@@ -452,7 +452,7 @@ test_expect_success 'checking out a commit before submodule moved needs manual u\n \tgit mv sub sub2 &&\n \tgit commit -m \"moved sub to sub2\" &&\n \tgit checkout -q HEAD^ 2>actual &&\n-\ttest_i18ngrep \"^warning: unable to rmdir sub2:\" actual &&\n+\ttest_i18ngrep \"^warning: unable to rmdir '\\''sub2'\\'':\" actual &&\n \tgit status -s sub2 >actual &&\n \techo \"?? sub2/\" >expected &&\n \ttest_cmp expected actual &&\n"},{"id":"331665","messageId":"20171102102031.v266n6nf6vftglu6@ruderich.org","threadId":"47080","inReplyTo":"xmqqlgjpqmyj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-11-02T10:20:31Z","receivedAt":"2017-11-02T10:20:41Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Thu, Nov 02, 2017 at 02:16:52PM +0900, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>> This patch is incomplete without adjusting a handful of tests to\n>> expect the updated messages, no?\n>\n> I'll squash these in while queuing, but there might be more that I\n> didn't notice.\n\nSorry, didn't think about the tests.\n\nI've re-checked and I think those are the only affected tests.\nThe test suite passes with your squashed changes.\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"331756","messageId":"xmqq4lqcqgjf.fsf@gitster.mtv.corp.google.com","threadId":"47080","inReplyTo":"20171102102031.v266n6nf6vftglu6@ruderich.org","subject":"Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-03T01:47:48Z","receivedAt":"2017-11-03T01:47:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Ruderich <simon@ruderich.org> writes:\n\n> On Thu, Nov 02, 2017 at 02:16:52PM +0900, Junio C Hamano wrote:\n>> Junio C Hamano writes:\n>>> This patch is incomplete without adjusting a handful of tests to\n>>> expect the updated messages, no?\n>>\n>> I'll squash these in while queuing, but there might be more that I\n>> didn't notice.\n>\n> Sorry, didn't think about the tests.\n\nHeh, tests are not something you need to think about, if you always\nrun them after making changes.\n\n> I've re-checked and I think those are the only affected tests.\n> The test suite passes with your squashed changes.\n\nOK.  Thanks.\n"},{"id":"331769","messageId":"20171103103248.4p45r4klojk5cf2g@ruderich.org","threadId":"47080","inReplyTo":"20171101221618.4ioog7jlp7n2nd53@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-11-03T10:32:48Z","receivedAt":"2017-11-03T10:32:55Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Wed, Nov 01, 2017 at 06:16:18PM -0400, Jeff King wrote:\n> On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote:\n>> I spent substantial time on making the sequencer code libified (it was far\n>> from it). That die() call may look okay now, but it is not at all okay if\n>> we want to make Git's source code cleaner and more reusable. And I want\n>> to.\n>>\n>> So my suggestion is to clean up write_file_buf() first, to stop behaving\n>> like a drunk lemming, and to return an error value already, and only then\n>> use it in sequencer.c.\n>\n> That would be fine with me, too.\n\nI tried looking into this by adding a new write_file_buf_gently()\n(or maybe renaming write_file_buf to write_file_buf_or_die) and\nusing it from write_file_buf() but I don't know the proper way to\nhandle the error-case in write_file_buf(). Just calling\ndie(\"write_file_buf\") feels ugly, as the real error was already\nprinted on screen by error_errno() and I didn't find any function\nto just exit without writing a message (which still respects\ndie_routine). Suggestions welcome.\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"331779","messageId":"xmqqpo8zpjdj.fsf@gitster.mtv.corp.google.com","threadId":"47080","inReplyTo":"20171103103248.4p45r4klojk5cf2g@ruderich.org","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-03T13:44:08Z","receivedAt":"2017-11-03T13:44:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Simon Ruderich <simon@ruderich.org> writes:\n\n> I tried looking into this by adding a new write_file_buf_gently()\n> (or maybe renaming write_file_buf to write_file_buf_or_die) and\n> using it from write_file_buf() but I don't know the proper way to\n> handle the error-case in write_file_buf(). Just calling\n> die(\"write_file_buf\") feels ugly, as the real error was already\n> printed on screen by error_errno() and I didn't find any function\n> to just exit without writing a message (which still respects\n> die_routine). Suggestions welcome.\n\nHow about *not* printing the error at the place where you notice the\nerror, and instead return an error code to the caller to be noticed\nwhich dies with an error message?\n"},{"id":"331782","messageId":"alpine.DEB.2.21.1.1711031425420.6482@virtualbox","threadId":"47080","inReplyTo":"20171103103248.4p45r4klojk5cf2g@ruderich.org","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2017-11-03T14:46:44Z","receivedAt":"2017-11-03T14:47:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Simon,\n\nOn Fri, 3 Nov 2017, Simon Ruderich wrote:\n\n> On Wed, Nov 01, 2017 at 06:16:18PM -0400, Jeff King wrote:\n> > On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote:\n> >> I spent substantial time on making the sequencer code libified (it was far\n> >> from it). That die() call may look okay now, but it is not at all okay if\n> >> we want to make Git's source code cleaner and more reusable. And I want\n> >> to.\n> >>\n> >> So my suggestion is to clean up write_file_buf() first, to stop behaving\n> >> like a drunk lemming, and to return an error value already, and only then\n> >> use it in sequencer.c.\n> >\n> > That would be fine with me, too.\n> \n> I tried looking into this by adding a new write_file_buf_gently()\n> (or maybe renaming write_file_buf to write_file_buf_or_die) and\n> using it from write_file_buf() but I don't know the proper way to\n> handle the error-case in write_file_buf(). Just calling\n> die(\"write_file_buf\") feels ugly, as the real error was already\n> printed on screen by error_errno() and I didn't find any function\n> to just exit without writing a message (which still respects\n> die_routine). Suggestions welcome.\n\nIn my ideal world, we could use all those fancy refactoring tools that are\ncurrently en vogue and simply turn *all* error()/error_errno() calls into\ncontext-aware functions that can be told to die() right away, or to return\nthe error in an error buffer, depending hwhat the caller (or the call\nchain, really) wants.\n\nThis is quite a bit more object-oriented than Git's code base, though, and\nbesides, I am not aware of any refactoring tool that would make this\npainless (it's not just a matter of adding a parameter, you also have to\npass it through all of the call chain, something you get for free when\nworking with an object-oriented language).\n\nSo what I did in the sequencer when faced with the same conundrum was to\nsimply return -1 if the function I called returned a negative value. The\ntop-level builtin (in that case, `rebase--helper`) simply returns !!ret as\nexit code (so that `-1` gets translated into the exit code `1`).\n\nBTW I would not use the `_or_die()` convention, as it suggests that that\nfunction will *always* die() in the error case. Instead, what I would\nfollow is the `, int die_on_error` pattern e.g. of `real_pathdup()`, and\nsimply *add* that parameter to the signature (and changing the return\nvalue to `int`).\n\nCiao,\nDscho\n"},{"id":"331799","messageId":"20171103185732.jr6a5wo2bmvh4onb@sigill.intra.peff.net","threadId":"47080","inReplyTo":"alpine.DEB.2.21.1.1711031425420.6482@virtualbox","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-03T18:57:33Z","receivedAt":"2017-11-03T18:57:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 03, 2017 at 03:46:44PM +0100, Johannes Schindelin wrote:\n\n> > I tried looking into this by adding a new write_file_buf_gently()\n> > (or maybe renaming write_file_buf to write_file_buf_or_die) and\n> > using it from write_file_buf() but I don't know the proper way to\n> > handle the error-case in write_file_buf(). Just calling\n> > die(\"write_file_buf\") feels ugly, as the real error was already\n> > printed on screen by error_errno() and I didn't find any function\n> > to just exit without writing a message (which still respects\n> > die_routine). Suggestions welcome.\n> \n> In my ideal world, we could use all those fancy refactoring tools that are\n> currently en vogue and simply turn *all* error()/error_errno() calls into\n> context-aware functions that can be told to die() right away, or to return\n> the error in an error buffer, depending hwhat the caller (or the call\n> chain, really) wants.\n> \n> This is quite a bit more object-oriented than Git's code base, though, and\n> besides, I am not aware of any refactoring tool that would make this\n> painless (it's not just a matter of adding a parameter, you also have to\n> pass it through all of the call chain, something you get for free when\n> working with an object-oriented language).\n\nFWIW, I sketched this out a bit here:\n\n  https://public-inbox.org/git/20160928085841.aoisson3fnuke47q@sigill.intra.peff.net/\n\nAnd you can see the patches I played with while writing that here:\n\n  https://github.com/peff/git/compare/cb5918aa0d50f50e83787f65c2ddc3dcb10159fe...4d61927e66dcfdbdb6cc6c88ec4018e2142e826c\n\n(but note they don't quite compile, as some of the conversions are\nhalf-done; it was really just to get a sense of the flavor of the\nthing).\n\nOne of the complaints was that it makes it harder to see when we are\ncalling die() (because it's now happening via an error callback). That\nmaybe confusing for users, but it may also affect generated code since\nthe code paths that hit the NORETURN function are obscured.\n\nBut we could stop short of adding error_die, and just have error_silent,\nerror_warn, and error_print (and callers can turn error_print into a\ndie() themselves).\n\n-Peff\n"},{"id":"331801","messageId":"20171103191309.sth4zjokgcupvk2e@sigill.intra.peff.net","threadId":"47080","inReplyTo":"xmqqpo8zpjdj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-03T19:13:10Z","receivedAt":"2017-11-03T19:13:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 03, 2017 at 10:44:08PM +0900, Junio C Hamano wrote:\n\n> Simon Ruderich <simon@ruderich.org> writes:\n> \n> > I tried looking into this by adding a new write_file_buf_gently()\n> > (or maybe renaming write_file_buf to write_file_buf_or_die) and\n> > using it from write_file_buf() but I don't know the proper way to\n> > handle the error-case in write_file_buf(). Just calling\n> > die(\"write_file_buf\") feels ugly, as the real error was already\n> > printed on screen by error_errno() and I didn't find any function\n> > to just exit without writing a message (which still respects\n> > die_routine). Suggestions welcome.\n> \n> How about *not* printing the error at the place where you notice the\n> error, and instead return an error code to the caller to be noticed\n> which dies with an error message?\n\nThat ends up giving less-specific errors. It might be an OK tradeoff\nhere.\n\nI think we've been gravitating towards error strbufs, which would make\nit something like:\n\ndiff --git a/wrapper.c b/wrapper.c\nindex 61aba0b5c1..08eb5d1cb8 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -649,13 +649,34 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)\n \treturn len;\n }\n \n+int write_file_buf_gently(const char *path, const char *buf, size_t len,\n+\t\t\t  struct strbuf *err)\n+{\n+\tint fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n+\tif (fd < 0) {\n+\t\tstrbuf_addf(err, _(\"could not open '%s' for writing: %s\"),\n+\t\t\t    path, strerror(errno));\n+\t\treturn -1;\n+\t}\n+\tif (write_in_full(fd, buf, len) < 0) {\n+\t\tstrbuf_addf(err, _(\"could not write to %s: %s\"),\n+\t\t\t    path, strerror(errno));\n+\t\tclose(fd);\n+\t\treturn -1;\n+\t}\n+\tif (close(fd)) {\n+\t\tstrbuf_addf(err, _(\"could not close %s: %s\"),\n+\t\t\t    path, strerror(errno));\n+\t\treturn -1;\n+\t}\n+\treturn 0;\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) < 0)\n-\t\tdie_errno(_(\"could not write to %s\"), path);\n-\tif (close(fd))\n-\t\tdie_errno(_(\"could not close %s\"), path);\n+\tstruct strbuf err = STRBUF_INIT;\n+\tif (write_file_buf_gently(path, buf, len, &err) < 0)\n+\t\tdie(\"%s\", err.buf);\n }\n \n void write_file(const char *path, const char *fmt, ...)\n\n\nI'm not excited that the amount of error-handling code is now double the\namount of code that actually does something useful. Maybe this function\nsimply isn't large/complex enough to merit flexible error handling, and\nwe should simply go with René's original near-duplicate.\n\nOTOH, if we went all-in on flexible error handling contexts, you could\nimagine this function becoming:\n\n  void write_file_buf(const char *path, const char *buf, size_t len,\n                      struct error_context *err)\n  {\n\tint fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n\tif (fd < 0)\n\t\treturn -1;\n\tif (write_in_full(fd, buf, len, err) < 0)\n\t\treturn -1;\n\tif (xclose(fd, err) < 0)\n\t\treturn -1;\n\treturn 0;\n  }\n\nKind of gross, in that we're adding a layer on top of all system calls.\nBut if used consistently, it makes error-reporting a lot more pleasant,\nand makes all of our \"whoops, we forgot to save errno\" bugs go away.\n\n-Peff\n"},{"id":"331818","messageId":"b074d6fa-8778-ea0d-d53b-7cc35bc4264a@web.de","threadId":"47080","inReplyTo":"20171103191309.sth4zjokgcupvk2e@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-11-04T09:05:43Z","receivedAt":"2017-11-04T09:06:04Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 03.11.2017 um 20:13 schrieb Jeff King:\n> On Fri, Nov 03, 2017 at 10:44:08PM +0900, Junio C Hamano wrote:\n> \n>> Simon Ruderich <simon@ruderich.org> writes:\n>>\n>>> I tried looking into this by adding a new write_file_buf_gently()\n>>> (or maybe renaming write_file_buf to write_file_buf_or_die) and\n>>> using it from write_file_buf() but I don't know the proper way to\n>>> handle the error-case in write_file_buf(). Just calling\n>>> die(\"write_file_buf\") feels ugly, as the real error was already\n>>> printed on screen by error_errno() and I didn't find any function\n>>> to just exit without writing a message (which still respects\n>>> die_routine). Suggestions welcome.\n>>\n>> How about *not* printing the error at the place where you notice the\n>> error, and instead return an error code to the caller to be noticed\n>> which dies with an error message?\n> \n> That ends up giving less-specific errors.\n\nNot necessarily.  Function could return different codes for different\nerrors, e.g. -1 for an open(2) error and -2 for a write(2) error, and\nthe caller could use that to select the message to show.\n\nBasically all of the messages in wrapper.c consist of some text mixed\nwith the affected path path and a strerror(3) string, so they're\ncompatible in that way.  A single function (get_path_error_format()?)\ncould thus be used and callers would be able to combine its result with\ndie(), error(), or warning().\n\nRené\n"},{"id":"331821","messageId":"20171104093537.glucmbljnjlw3htq@sigill.intra.peff.net","threadId":"47080","inReplyTo":"b074d6fa-8778-ea0d-d53b-7cc35bc4264a@web.de","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-04T09:35:38Z","receivedAt":"2017-11-04T09:35:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 04, 2017 at 10:05:43AM +0100, René Scharfe wrote:\n\n> >> How about *not* printing the error at the place where you notice the\n> >> error, and instead return an error code to the caller to be noticed\n> >> which dies with an error message?\n> > \n> > That ends up giving less-specific errors.\n> \n> Not necessarily.  Function could return different codes for different\n> errors, e.g. -1 for an open(2) error and -2 for a write(2) error, and\n> the caller could use that to select the message to show.\n> \n> Basically all of the messages in wrapper.c consist of some text mixed\n> with the affected path path and a strerror(3) string, so they're\n> compatible in that way.  A single function (get_path_error_format()?)\n> could thus be used and callers would be able to combine its result with\n> die(), error(), or warning().\n\nI think we've had this discussion before, a while ago. Yes, returning an\ninteger error code is nice because you don't have pass in an extra\nparameter. But I think there are two pitfalls:\n\n  1. Integers may not be descriptive enough to cover all cases, which is\n     how we ended up with the strbuf-passing strategy in the ref code.\n     Certainly you could add an integer for every possible bespoke\n     message, but then I'm not sure it's buying that much over having\n     the function simply fill in a strbuf.\n\n  2. For complex functions there may be multiple errors that need to\n     stack. I think the refs code has cases like this, where a syscall\n     fails, which causes a fundamental ref operation to fail, which\n     causes a higher-level operation to fail. It's only the caller of\n     the higher-level operation that knows how to report the error.\n\nCertainly an integer error code would work for _this_ function, but I'd\nrather see us grow towards consistent error handling.\n\n-Peff\n"},{"id":"331829","messageId":"20171104183643.akaazwswysphzuoq@ruderich.org","threadId":"47080","inReplyTo":"20171103191309.sth4zjokgcupvk2e@sigill.intra.peff.net","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-11-04T18:36:43Z","receivedAt":"2017-11-04T18:36:50Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Fri, Nov 03, 2017 at 03:13:10PM -0400, Jeff King wrote:\n> I think we've been gravitating towards error strbufs, which would make\n> it something like:\n\nI like this approach to store the error in a separate variable\nand let the caller handle it. This provides proper error messages\nand is cleaner than printing the error on the error site (what\nerror_errno does).\n\nHowever I wouldn't use strbuf directly and instead add a new\nstruct error which provides a small set of helper functions.\nUsing a separate type also makes it clear to the reader that is\nnot a normal string and is more extendable in the future.\n\n> I'm not excited that the amount of error-handling code is now double the\n> amount of code that actually does something useful. Maybe this function\n> simply isn't large/complex enough to merit flexible error handling, and\n> we should simply go with René's original near-duplicate.\n\nA separate struct (and helper functions) would help in this case\nand could look like this, which is almost equal (in code size) to\nthe original solution using error_errno:\n\n    int write_file_buf_gently2(const char *path, const char *buf, size_t len, struct error *err)\n    {\n            int rc = 0;\n            int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n            if (fd < 0)\n                    return error_addf_errno(err, _(\"could not open '%s' for writing\"), path);\n            if (write_in_full(fd, buf, len) < 0)\n                    rc = error_addf_errno(err, _(\"could not write to '%s'\"), path);\n            if (close(fd) && !rc)\n                    rc = error_addf_errno(err, _(\"could not close '%s'\"), path);\n            return rc;\n    }\n\n(I didn't touch write_in_full here, but it could also take the\nerr and then the code would get a little shorter, however would\nlose the \"path\" information, but see below.)\n\nAnd in the caller:\n\n    void write_file_buf(const char *path, const char *buf, size_t len)\n    {\n            struct error err = ERROR_INIT;\n            if (write_file_buf_gently2(path, buf, len, &err) < 0)\n                    error_die(&err);\n    }\n\nFor now struct error just contains the strbuf, but one could add\nthe call location (by using a macro for error_addf_errno) or the\noriginal errno or more information in the future.\n\nerror_addf_errno() could also prepend the error the buffer so\nthat the caller can add more information if necessary and we get\nsomething like: \"failed to write file 'foo': write failed: errno\ntext\" in the write_file_buf case (the first error string is from\nwrite_file_buf_gently2, the second from write_in_full). However\nI'm not sure how well this works with translations.\n\nWe could also store the error condition in the error struct and\ndon't use the return value to indicate and error like this:\n\n    void write_file_buf(const char *path, const char *buf, size_t len)\n    {\n            struct error err = ERROR_INIT;\n            write_file_buf_gently2(path, buf, len, &err);\n            if (err.error)\n                    error_die(&err);\n    }\n\n> OTOH, if we went all-in on flexible error handling contexts, you could\n> imagine this function becoming:\n>\n>   void write_file_buf(const char *path, const char *buf, size_t len,\n>                       struct error_context *err)\n>   {\n> \tint fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n> \tif (fd < 0)\n> \t\treturn -1;\n> \tif (write_in_full(fd, buf, len, err) < 0)\n> \t\treturn -1;\n> \tif (xclose(fd, err) < 0)\n> \t\treturn -1;\n> \treturn 0;\n>   }\n\nThis looks interesting as well, but it misses the feature of\ncustom error messages which is really useful.\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"331838","messageId":"20171105020700.2p4nguemzdrwiila@sigill.intra.peff.net","threadId":"47080","inReplyTo":"20171104183643.akaazwswysphzuoq@ruderich.org","subject":"Re: [PATCH 1/2] sequencer: factor out rewrite_file()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-05T02:07:00Z","receivedAt":"2017-11-05T02:07:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 04, 2017 at 07:36:43PM +0100, Simon Ruderich wrote:\n\n> On Fri, Nov 03, 2017 at 03:13:10PM -0400, Jeff King wrote:\n> > I think we've been gravitating towards error strbufs, which would make\n> > it something like:\n> \n> I like this approach to store the error in a separate variable\n> and let the caller handle it. This provides proper error messages\n> and is cleaner than printing the error on the error site (what\n> error_errno does).\n> \n> However I wouldn't use strbuf directly and instead add a new\n> struct error which provides a small set of helper functions.\n> Using a separate type also makes it clear to the reader that is\n> not a normal string and is more extendable in the future.\n\nYes, I think what you've written here (and below) is quite close to the\nerror_context patches I linked elsewhere in the thread. In other\nwords, I think it's a sane approach.\n\n> We could also store the error condition in the error struct and\n> don't use the return value to indicate and error like this:\n> \n>     void write_file_buf(const char *path, const char *buf, size_t len)\n>     {\n>             struct error err = ERROR_INIT;\n>             write_file_buf_gently2(path, buf, len, &err);\n>             if (err.error)\n>                     error_die(&err);\n>     }\n\nI agree it might be nice for the error context to have a positive \"there\nwas an error\" flag. It's probably worth making it redundant with the\nreturn code, though, so callers can use whichever style is most\nconvenient for them.\n\n> > OTOH, if we went all-in on flexible error handling contexts, you could\n> > imagine this function becoming:\n> >\n> >   void write_file_buf(const char *path, const char *buf, size_t len,\n> >                       struct error_context *err)\n> >   {\n> > \tint fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666);\n> > \tif (fd < 0)\n> > \t\treturn -1;\n> > \tif (write_in_full(fd, buf, len, err) < 0)\n> > \t\treturn -1;\n> > \tif (xclose(fd, err) < 0)\n> > \t\treturn -1;\n> > \treturn 0;\n> >   }\n> \n> This looks interesting as well, but it misses the feature of\n> custom error messages which is really useful.\n\nRight, I didn't think that example through. The functions after the\nopen() don't have enough information to make a good message.\n\n-Peff\n"},{"id":"331928","messageId":"20171106161315.dmftp6ktk6bu7cah@ruderich.org","threadId":"47080","inReplyTo":"20171105020700.2p4nguemzdrwiila@sigill.intra.peff.net","subject":"Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-11-06T16:13:15Z","receivedAt":"2017-11-06T16:13:23Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:\n> Yes, I think what you've written here (and below) is quite close to the\n> error_context patches I linked elsewhere in the thread. In other\n> words, I think it's a sane approach.\n\nIn contrast to error_context I'd like to keep all exiting\nbehavior (die, ignore, etc.) in the hand of the caller and not\nuse any callbacks as that makes the control flow much harder to\nfollow.\n\n> I agree it might be nice for the error context to have a positive \"there\n> was an error\" flag. It's probably worth making it redundant with the\n> return code, though, so callers can use whichever style is most\n> convenient for them.\n\nAgreed.\n\nRegarding the API, should it be allowed to pass NULL as error\npointer to request no additional error handling or should the\nerror functions panic on NULL? Allowing NULL makes partial\nconversions possible (e.g. for write_in_full) where old callers\njust pass NULL and check the return values and converted callers\ncan use the error struct.\n\nHow should translations get handled? Appending \": %s\" for\nstrerror(errno) might be problematic. Same goes for \"outer\nmessage: inner message\" where the helper function just inserts \":\n\" between the messages. Is _(\"%s: %s\") (with appropriate\ntranslator comments) enough to handle these cases?\n\nSuggestions how to name the struct and the corresponding\nfunctions? My initial idea was struct error and to use error_ as\nprefix, but I'm not sure if struct error is too broad and may\nintroduce conflicts with system headers. Also error_ is a little\nlong and could be shorted to just err_ but I don't know if that's\nclear enough. The error_ prefix doesn't conflict with many git\nfunctions, but there are some in usage.c (error_errno, error,\nerror_routine).\n\nAnd as general question, is this approach to error handling\nsomething we should pursue or are there objections? If there's\nconsensus that this might be a good idea I'll look into\nconverting some parts of the git code (maybe refs.c) to see how\nit pans out.\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"332656","messageId":"20171116103648.5toqzejnuztuphm7@ruderich.org","threadId":"47080","inReplyTo":"20171106161315.dmftp6ktk6bu7cah@ruderich.org","subject":"Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())","fromName":"Simon Ruderich","fromEmail":"simon@ruderich.org","sentAt":"2017-11-16T10:36:49Z","receivedAt":"2017-11-16T10:36:56Z","isPatch":true,"sender":{"key":"simon@ruderich.org","avatar":"https://avatars.githubusercontent.com/u/390994?v=4"},"body":"On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote:\n> On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:\n>> Yes, I think what you've written here (and below) is quite close to the\n>> error_context patches I linked elsewhere in the thread. In other\n>> words, I think it's a sane approach.\n>\n> In contrast to error_context I'd like to keep all exiting\n> behavior (die, ignore, etc.) in the hand of the caller and not\n> use any callbacks as that makes the control flow much harder to\n> follow.\n>\n>> I agree it might be nice for the error context to have a positive \"there\n>> was an error\" flag. It's probably worth making it redundant with the\n>> return code, though, so callers can use whichever style is most\n>> convenient for them.\n>\n> Agreed.\n>\n> Regarding the API, should it be allowed to pass NULL as error\n> pointer to request no additional error handling or should the\n> error functions panic on NULL? Allowing NULL makes partial\n> conversions possible (e.g. for write_in_full) where old callers\n> just pass NULL and check the return values and converted callers\n> can use the error struct.\n>\n> How should translations get handled? Appending \": %s\" for\n> strerror(errno) might be problematic. Same goes for \"outer\n> message: inner message\" where the helper function just inserts \":\n> \" between the messages. Is _(\"%s: %s\") (with appropriate\n> translator comments) enough to handle these cases?\n>\n> Suggestions how to name the struct and the corresponding\n> functions? My initial idea was struct error and to use error_ as\n> prefix, but I'm not sure if struct error is too broad and may\n> introduce conflicts with system headers. Also error_ is a little\n> long and could be shorted to just err_ but I don't know if that's\n> clear enough. The error_ prefix doesn't conflict with many git\n> functions, but there are some in usage.c (error_errno, error,\n> error_routine).\n>\n> And as general question, is this approach to error handling\n> something we should pursue or are there objections? If there's\n> consensus that this might be a good idea I'll look into\n> converting some parts of the git code (maybe refs.c) to see how\n> it pans out.\n\nAny comments?\n\nRegards\nSimon\n-- \n+ privacy is necessary\n+ using gnupg http://gnupg.org\n+ public key id: 0x92FEFDB7E44C32F9\n"},{"id":"332781","messageId":"20171117223345.s3ihubgda3qdb2j6@sigill.intra.peff.net","threadId":"47080","inReplyTo":"20171106161315.dmftp6ktk6bu7cah@ruderich.org","subject":"Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-17T22:33:46Z","receivedAt":"2017-11-17T22:33:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote:\n\n> On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:\n> > Yes, I think what you've written here (and below) is quite close to the\n> > error_context patches I linked elsewhere in the thread. In other\n> > words, I think it's a sane approach.\n> \n> In contrast to error_context I'd like to keep all exiting\n> behavior (die, ignore, etc.) in the hand of the caller and not\n> use any callbacks as that makes the control flow much harder to\n> follow.\n\nYeah, I have mixed feelings on that. I think it does make the control\nflow less clear. At the same time, what I found was that handlers like\ndie/ignore/warn were the thing that gave the most reduction in\ncomplexity in the callers.\n\n> Regarding the API, should it be allowed to pass NULL as error\n> pointer to request no additional error handling or should the\n> error functions panic on NULL? Allowing NULL makes partial\n> conversions possible (e.g. for write_in_full) where old callers\n> just pass NULL and check the return values and converted callers\n> can use the error struct.\n\nI think it's probably better to be explicit, and pass some \"noop\" error\nhandling struct. We'll have to be adding parameters to functions to\nhandle this anyway, so I don't think there's much opportunity for having\nNULL as a fallback for partial conversions.\n\n> How should translations get handled? Appending \": %s\" for\n> strerror(errno) might be problematic. Same goes for \"outer\n> message: inner message\" where the helper function just inserts \":\n> \" between the messages. Is _(\"%s: %s\") (with appropriate\n> translator comments) enough to handle these cases?\n\nI don't have a real opinion, not having done much translation myself. I\nwill say that the existing die_errno(), error_errno(), etc just use \"%s:\n%s\", without even allowing for translation (see fmt_with_err in\nusage.c). I'm sure that probably sucks for RTL languages, but I think\nit would be fine to punt on it for now.\n\n> Suggestions how to name the struct and the corresponding\n> functions? My initial idea was struct error and to use error_ as\n> prefix, but I'm not sure if struct error is too broad and may\n> introduce conflicts with system headers. Also error_ is a little\n> long and could be shorted to just err_ but I don't know if that's\n> clear enough. The error_ prefix doesn't conflict with many git\n> functions, but there are some in usage.c (error_errno, error,\n> error_routine).\n\nIn my experiments[1] I called the types error_*, and then generally used\n\"err\" as a local variable when necessary. Variants on that seem fine to\nme, but yeah, you have to avoid conflicting with error(). We _could_\nrename that, but it would be a pretty invasive patch.\n\n> And as general question, is this approach to error handling\n> something we should pursue or are there objections? If there's\n> consensus that this might be a good idea I'll look into\n> converting some parts of the git code (maybe refs.c) to see how\n> it pans out.\n\nI dunno. I kind of like the idea, but if the only error context is one\nthat adds to strbufs, I don't know that it's buying us much over the\nstatus quo (which is passing around strbufs). It's a little more\nexplicit, I guess.\n\nOther list regulars besides me seem mostly quiet on the subject.\n\n-Peff\n\n[1] This is the jk/error-context-wip branch of https://github.com/peff/git.\n    I can't remember if I mentioned that before.\n"},{"id":"332794","messageId":"c50ac174-15bd-60bc-490c-d231e3eb501d@kdbg.org","threadId":"47080","inReplyTo":"20171117223345.s3ihubgda3qdb2j6@sigill.intra.peff.net","subject":"Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-11-18T09:01:45Z","receivedAt":"2017-11-18T09:01:54Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 17.11.2017 um 23:33 schrieb Jeff King:\n> On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote:\n>> On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:\n>>> Yes, I think what you've written here (and below) is quite close to the\n>>> error_context patches I linked elsewhere in the thread. In other\n>>> words, I think it's a sane approach.\n>>\n>> In contrast to error_context I'd like to keep all exiting\n>> behavior (die, ignore, etc.) in the hand of the caller and not\n>> use any callbacks as that makes the control flow much harder to\n>> follow.\n> \n> Yeah, I have mixed feelings on that. I think it does make the control\n> flow less clear. At the same time, what I found was that handlers like\n> die/ignore/warn were the thing that gave the most reduction in\n> complexity in the callers.\n\nWould you not consider switching over to C++? With exceptions, you get \nthe error context without cluttering the API. (Did I mention that \nlibrarification would become a breeze? Do not die in library routines: \nnot a problem anymore, just catch the exception. die_on_error \nparameters? Not needed anymore. Not to mention that resource leaks would \nbe much, MUCH simpler to treat.)\n\n-- Hannes\n"},{"id":"335260","messageId":"20171224145427.GG23648@sigill.intra.peff.net","threadId":"47080","inReplyTo":"c50ac174-15bd-60bc-490c-d231e3eb501d@kdbg.org","subject":"Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-12-24T14:54:28Z","receivedAt":"2017-12-24T14:57:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote:\n\n> > Yeah, I have mixed feelings on that. I think it does make the control\n> > flow less clear. At the same time, what I found was that handlers like\n> > die/ignore/warn were the thing that gave the most reduction in\n> > complexity in the callers.\n> \n> Would you not consider switching over to C++? With exceptions, you get the\n> error context without cluttering the API. (Did I mention that\n> librarification would become a breeze? Do not die in library routines: not a\n> problem anymore, just catch the exception. die_on_error parameters? Not\n> needed anymore. Not to mention that resource leaks would be much, MUCH\n> simpler to treat.)\n\nI threw this email on my todo pile since I was traveling when it came,\nbut I think it deserves a response (albeit quite late).\n\nIt's been a long while since I've done any serious C++, but I did really\nlike the RAII pattern coupled with exceptions. That said, I think it's\ndangerous to do it half-way, and especially to retrofit an existing code\nbase. It introduces a whole new control-flow pattern that is invisible\nto the existing code, so you're going to get leaks and variables in\nunexpected states whenever you see an exception.\n\nI also suspect there'd be a fair bit of in converting the existing code\nto something that actually compiles as C++.\n\nSo if we were starting the project from scratch and thinking about using\nC++ with RAII and exceptions, sure, that's something I'd entertain[1]\n(and maybe even Linus has softened on his opinion of C++ these days ;) ).\nBut at this point, it doesn't seem like the tradeoff for switching is\nthere.\n\n-Peff\n\n[1] I'd also consider Rust, though I'm not too experienced with it\n    myself.\n"},{"id":"335261","messageId":"000c01d37cce$49bd0d30$dd372790$@nexbridge.com","threadId":"47080","inReplyTo":"20171224145427.GG23648@sigill.intra.peff.net","subject":"RE: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())","fromName":"Randall S. Becker","fromEmail":"rsbecker@nexbridge.com","sentAt":"2017-12-24T15:45:50Z","receivedAt":"2017-12-24T15:46:51Z","isPatch":true,"sender":{"key":"randall.becker@nexbridge.ca","avatar":"https://avatars.githubusercontent.com/u/28956764?v=4"},"body":"On December 24, 2017 9:54 AM, Jeff King wrote:\n> Subject: Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor\n> out rewrite_file())\n> \n> On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote:\n> \n> > > Yeah, I have mixed feelings on that. I think it does make the\n> > > control flow less clear. At the same time, what I found was that\n> > > handlers like die/ignore/warn were the thing that gave the most\n> > > reduction in complexity in the callers.\n> >\n> > Would you not consider switching over to C++? With exceptions, you get\n> > the error context without cluttering the API. (Did I mention that\n> > librarification would become a breeze? Do not die in library routines:\n> > not a problem anymore, just catch the exception. die_on_error\n> > parameters? Not needed anymore. Not to mention that resource leaks\n> > would be much, MUCH simpler to treat.)\n> \n> I threw this email on my todo pile since I was traveling when it came, but I\n> think it deserves a response (albeit quite late).\n> \n> It's been a long while since I've done any serious C++, but I did really like the\n> RAII pattern coupled with exceptions. That said, I think it's dangerous to do it\n> half-way, and especially to retrofit an existing code base. It introduces a\n> whole new control-flow pattern that is invisible to the existing code, so\n> you're going to get leaks and variables in unexpected states whenever you\n> see an exception.\n> \n> I also suspect there'd be a fair bit of in converting the existing code to\n> something that actually compiles as C++.\n> \n> So if we were starting the project from scratch and thinking about using\n> C++ with RAII and exceptions, sure, that's something I'd entertain[1]\n> (and maybe even Linus has softened on his opinion of C++ these days ;) ).\n> But at this point, it doesn't seem like the tradeoff for switching is there.\n\nWhile I'm a huge fan of OO, you really need a solid justification for going there, and a good study of your target audience for Open Source C++. My comments are based on porting experience outside of Linux/Windows:\n\n1. Conversion to C++ just to pick up exceptions is a lot like \"One does not simply walk to Mordor\", as Peff hinted at above.\n2. Moving to C++ generally involves a **complete** redesign. While Command Patterns (and and...)  may be very helpful in one level, the current git code base is very procedural in nature.\n3. From a porting perspective, applications written in with C++ generally (there are exceptions) are significantly harder than C. The POSIX APIs are older and more broadly supported/emulated than what is available in C++. Once you start getting into \"my favourite C++ library is...\", or \"version2 or version3\", or smart pointers vs. scope allocation, things get pretty argumentative. It is (arguably) much easier to disable a section of code that won't function on a platform in C without having to rework an OO model, making subsequent merges pretty much impossible and the port unsustainable. That is unless the overall design really factors in platform differences right into the OO model from the beginning of incubation.\n4. I really hate making these points because I am an OO \"fanspert\", just not when doing portable code. Even in java, which is more port-stable than C++ (arguably, but in my experience), you tend to hit platform library differences than can invalidate ports.\n\nMy take is \"oh my please don't go there\" for the git project, for a component that has become/is becoming required core infrastructure for so many platforms.\n\nWith Respect,\nRandall\n\n-- Brief whoami: NonStop&UNIX developer since approximately UNIX(421664400)/NonStop(211288444200000000)\n-- In my real life, I talk too much.\n\n\n\n"},{"id":"335280","messageId":"16262419-3ce9-13e2-6dbc-2ffcad8327f6@kdbg.org","threadId":"47080","inReplyTo":"20171224145427.GG23648@sigill.intra.peff.net","subject":"Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2017-12-25T10:28:54Z","receivedAt":"2017-12-25T10:29:04Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 24.12.2017 um 15:54 schrieb Jeff King:\n> On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote:\n> \n>>> Yeah, I have mixed feelings on that. I think it does make the control\n>>> flow less clear. At the same time, what I found was that handlers like\n>>> die/ignore/warn were the thing that gave the most reduction in\n>>> complexity in the callers.\n>>\n>> Would you not consider switching over to C++? With exceptions, you get the\n>> error context without cluttering the API. (Did I mention that\n>> librarification would become a breeze? Do not die in library routines: not a\n>> problem anymore, just catch the exception. die_on_error parameters? Not\n>> needed anymore. Not to mention that resource leaks would be much, MUCH\n>> simpler to treat.)\n> \n> I threw this email on my todo pile since I was traveling when it came,\n> but I think it deserves a response (albeit quite late).\n> \n> It's been a long while since I've done any serious C++, but I did really\n> like the RAII pattern coupled with exceptions. That said, I think it's\n> dangerous to do it half-way, and especially to retrofit an existing code\n> base. It introduces a whole new control-flow pattern that is invisible\n> to the existing code, so you're going to get leaks and variables in\n> unexpected states whenever you see an exception.\n> \n> I also suspect there'd be a fair bit of in converting the existing code\n> to something that actually compiles as C++.\n\nI think I mentioned that I had a version that passed the test suite. \nIt's not pure C++ as it required -fpermissive due to the many implicit \nvoid*-to-pointer-to-object conversions (which are disallowed in C++). \nAnd, yes, a fair bit of conversion was required on top of that. ;)\n\n> So if we were starting the project from scratch and thinking about using\n> C++ with RAII and exceptions, sure, that's something I'd entertain[1]\n> (and maybe even Linus has softened on his opinion of C++ these days ;) ).\n> But at this point, it doesn't seem like the tradeoff for switching is\n> there.\n\nFair enough. I do agree that the tradeoff is not there, in particular, \nwhen the major players are more fluent in C than in modern C++.\n\nThere is just my usual rant: Why do we have look for resource leaks \nduring review when we could have leak-free code by design? (But Dscho \nscored a point[*] some time ago: \"For every fool-proof system invented, \nsomebody invents a better fool.\")\n\n[*] \nhttps://public-inbox.org/git/alpine.DEB.2.20.1704281334060.3480@virtualbox/\n"}]}