* [PATCH 1/2] sequencer: factor out rewrite_file() @ 2017-10-31 9:54 René Scharfe 2017-10-31 9:58 ` [PATCH 2/2] sequencer: use O_TRUNC to truncate files René Scharfe ` (4 more replies) 0 siblings, 5 replies; 35+ messages in thread From: René Scharfe @ 2017-10-31 9:54 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano, Ralf Thielow, Johannes Schindelin Reduce code duplication by extracting a function for rewriting an existing file. Signed-off-by: Rene Scharfe <l.s.r@web.de> --- sequencer.c | 46 +++++++++++++++++----------------------------- 1 file changed, 17 insertions(+), 29 deletions(-) diff --git a/sequencer.c b/sequencer.c index f2a10cc4f2..17360eb38a 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2665,6 +2665,20 @@ int check_todo_list(void) return res; } +static int rewrite_file(const char *path, const char *buf, size_t len) +{ + int rc = 0; + int fd = open(path, O_WRONLY); + if (fd < 0) + return error_errno(_("could not open '%s' for writing"), path); + if (write_in_full(fd, buf, len) < 0) + rc = error_errno(_("could not write to '%s'"), path); + if (!rc && ftruncate(fd, len) < 0) + rc = error_errno(_("could not truncate '%s'"), path); + close(fd); + return rc; +} + /* skip picking commits whose parents are unchanged */ int skip_unnecessary_picks(void) { @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void) } close(fd); - fd = open(rebase_path_todo(), O_WRONLY, 0666); - if (fd < 0) { - error_errno(_("could not open '%s' for writing"), - rebase_path_todo()); + if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset, + todo_list.buf.len - offset) < 0) { todo_list_release(&todo_list); return -1; } - if (write_in_full(fd, todo_list.buf.buf + offset, - todo_list.buf.len - offset) < 0) { - error_errno(_("could not write to '%s'"), - rebase_path_todo()); - close(fd); - todo_list_release(&todo_list); - return -1; - } - if (ftruncate(fd, todo_list.buf.len - offset) < 0) { - error_errno(_("could not truncate '%s'"), - rebase_path_todo()); - todo_list_release(&todo_list); - close(fd); - return -1; - } - close(fd); todo_list.current = i; if (is_fixup(peek_command(&todo_list, 0))) @@ -2944,15 +2940,7 @@ int rearrange_squash(void) } } - fd = open(todo_file, O_WRONLY); - if (fd < 0) - res = error_errno(_("could not open '%s'"), todo_file); - else if (write(fd, buf.buf, buf.len) < 0) - res = error_errno(_("could not write to '%s'"), todo_file); - else if (ftruncate(fd, buf.len) < 0) - res = error_errno(_("could not truncate '%s'"), - todo_file); - close(fd); + res = rewrite_file(todo_file, buf.buf, buf.len); strbuf_release(&buf); } -- 2.15.0 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* [PATCH 2/2] sequencer: use O_TRUNC to truncate files 2017-10-31 9:54 [PATCH 1/2] sequencer: factor out rewrite_file() René Scharfe @ 2017-10-31 9:58 ` René Scharfe 2017-10-31 16:34 ` Kevin Daudt 2017-11-01 15:34 ` Johannes Schindelin 2017-10-31 16:33 ` [PATCH 1/2] sequencer: factor out rewrite_file() Kevin Daudt ` (3 subsequent siblings) 4 siblings, 2 replies; 35+ messages in thread From: René Scharfe @ 2017-10-31 9:58 UTC (permalink / raw) To: Git List; +Cc: Junio C Hamano, Ralf Thielow, Johannes Schindelin Cut off any previous content of the file to be rewritten by passing the flag O_TRUNC to open(2) instead of calling ftruncate(2) at the end. That's easier and shorter. Signed-off-by: Rene Scharfe <l.s.r@web.de> --- sequencer.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/sequencer.c b/sequencer.c index 17360eb38a..f93b60f615 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2668,13 +2668,11 @@ int check_todo_list(void) static int rewrite_file(const char *path, const char *buf, size_t len) { int rc = 0; - int fd = open(path, O_WRONLY); + int fd = open(path, O_WRONLY | O_TRUNC); if (fd < 0) return error_errno(_("could not open '%s' for writing"), path); if (write_in_full(fd, buf, len) < 0) rc = error_errno(_("could not write to '%s'"), path); - if (!rc && ftruncate(fd, len) < 0) - rc = error_errno(_("could not truncate '%s'"), path); close(fd); return rc; } -- 2.15.0 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] sequencer: use O_TRUNC to truncate files 2017-10-31 9:58 ` [PATCH 2/2] sequencer: use O_TRUNC to truncate files René Scharfe @ 2017-10-31 16:34 ` Kevin Daudt 2017-11-01 15:34 ` Johannes Schindelin 1 sibling, 0 replies; 35+ messages in thread From: Kevin Daudt @ 2017-10-31 16:34 UTC (permalink / raw) To: René Scharfe Cc: Git List, Junio C Hamano, Ralf Thielow, Johannes Schindelin On Tue, Oct 31, 2017 at 10:58:16AM +0100, René Scharfe wrote: > Cut off any previous content of the file to be rewritten by passing the > flag O_TRUNC to open(2) instead of calling ftruncate(2) at the end. > That's easier and shorter. > > Signed-off-by: Rene Scharfe <l.s.r@web.de> > --- > sequencer.c | 4 +--- > 1 file changed, 1 insertion(+), 3 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index 17360eb38a..f93b60f615 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2668,13 +2668,11 @@ int check_todo_list(void) > static int rewrite_file(const char *path, const char *buf, size_t len) > { > int rc = 0; > - int fd = open(path, O_WRONLY); > + int fd = open(path, O_WRONLY | O_TRUNC); > if (fd < 0) > return error_errno(_("could not open '%s' for writing"), path); > if (write_in_full(fd, buf, len) < 0) > rc = error_errno(_("could not write to '%s'"), path); > - if (!rc && ftruncate(fd, len) < 0) > - rc = error_errno(_("could not truncate '%s'"), path); > close(fd); > return rc; > } > -- > 2.15.0 Makes sense ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] sequencer: use O_TRUNC to truncate files 2017-10-31 9:58 ` [PATCH 2/2] sequencer: use O_TRUNC to truncate files René Scharfe 2017-10-31 16:34 ` Kevin Daudt @ 2017-11-01 15:34 ` Johannes Schindelin 1 sibling, 0 replies; 35+ messages in thread From: Johannes Schindelin @ 2017-11-01 15:34 UTC (permalink / raw) To: René Scharfe; +Cc: Git List, Junio C Hamano, Ralf Thielow [-- Attachment #1: Type: text/plain, Size: 259 bytes --] Hi René, On Tue, 31 Oct 2017, René Scharfe wrote: > Cut off any previous content of the file to be rewritten by passing the > flag O_TRUNC to open(2) instead of calling ftruncate(2) at the end. > That's easier and shorter. Sure. Thanks, Dscho ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-10-31 9:54 [PATCH 1/2] sequencer: factor out rewrite_file() René Scharfe 2017-10-31 9:58 ` [PATCH 2/2] sequencer: use O_TRUNC to truncate files René Scharfe @ 2017-10-31 16:33 ` Kevin Daudt 2017-11-01 6:06 ` Kevin Daudt 2017-11-01 11:10 ` Simon Ruderich ` (2 subsequent siblings) 4 siblings, 1 reply; 35+ messages in thread From: Kevin Daudt @ 2017-10-31 16:33 UTC (permalink / raw) To: René Scharfe On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote: > Reduce code duplication by extracting a function for rewriting an > existing file. > > Signed-off-by: Rene Scharfe <l.s.r@web.de> > --- > sequencer.c | 46 +++++++++++++++++----------------------------- > 1 file changed, 17 insertions(+), 29 deletions(-) > > diff --git a/sequencer.c b/sequencer.c > index f2a10cc4f2..17360eb38a 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2665,6 +2665,20 @@ int check_todo_list(void) > return res; > } > > +static int rewrite_file(const char *path, const char *buf, size_t len) > +{ > + int rc = 0; > + int fd = open(path, O_WRONLY); > + if (fd < 0) > + return error_errno(_("could not open '%s' for writing"), path); > + if (write_in_full(fd, buf, len) < 0) > + rc = error_errno(_("could not write to '%s'"), path); > + if (!rc && ftruncate(fd, len) < 0) > + rc = error_errno(_("could not truncate '%s'"), path); > + close(fd); > + return rc; > +} > + > /* skip picking commits whose parents are unchanged */ > int skip_unnecessary_picks(void) > { > @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void) > } > close(fd); > > - fd = open(rebase_path_todo(), O_WRONLY, 0666); > - if (fd < 0) { > - error_errno(_("could not open '%s' for writing"), > - rebase_path_todo()); > + if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset, > + todo_list.buf.len - offset) < 0) { > todo_list_release(&todo_list); > return -1; > } > - if (write_in_full(fd, todo_list.buf.buf + offset, > - todo_list.buf.len - offset) < 0) { > - error_errno(_("could not write to '%s'"), > - rebase_path_todo()); > - close(fd); > - todo_list_release(&todo_list); Is this missing on purpose in the new situation? > - return -1; > - } > - if (ftruncate(fd, todo_list.buf.len - offset) < 0) { > - error_errno(_("could not truncate '%s'"), > - rebase_path_todo()); > - todo_list_release(&todo_list); > - close(fd); > - return -1; > - } > - close(fd); > > todo_list.current = i; > if (is_fixup(peek_command(&todo_list, 0))) > @@ -2944,15 +2940,7 @@ int rearrange_squash(void) > } > } > > - fd = open(todo_file, O_WRONLY); > - if (fd < 0) > - res = error_errno(_("could not open '%s'"), todo_file); > - else if (write(fd, buf.buf, buf.len) < 0) > - res = error_errno(_("could not write to '%s'"), todo_file); > - else if (ftruncate(fd, buf.len) < 0) > - res = error_errno(_("could not truncate '%s'"), > - todo_file); > - close(fd); > + res = rewrite_file(todo_file, buf.buf, buf.len); > strbuf_release(&buf); > } > > -- > 2.15.0 Except for that question, it looks good to me (as a beginner), it makes the code better readable. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-10-31 16:33 ` [PATCH 1/2] sequencer: factor out rewrite_file() Kevin Daudt @ 2017-11-01 6:06 ` Kevin Daudt 0 siblings, 0 replies; 35+ messages in thread From: Kevin Daudt @ 2017-11-01 6:06 UTC (permalink / raw) To: git On Tue, Oct 31, 2017 at 05:33:57PM +0100, Kevin Daudt wrote: > On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote: > > Reduce code duplication by extracting a function for rewriting an > > existing file. > > > > Signed-off-by: Rene Scharfe <l.s.r@web.de> > > --- > > sequencer.c | 46 +++++++++++++++++----------------------------- > > 1 file changed, 17 insertions(+), 29 deletions(-) > > > > diff --git a/sequencer.c b/sequencer.c > > index f2a10cc4f2..17360eb38a 100644 > > --- a/sequencer.c > > +++ b/sequencer.c > > @@ -2665,6 +2665,20 @@ int check_todo_list(void) > > return res; > > } > > > > +static int rewrite_file(const char *path, const char *buf, size_t len) > > +{ > > + int rc = 0; > > + int fd = open(path, O_WRONLY); > > + if (fd < 0) > > + return error_errno(_("could not open '%s' for writing"), path); > > + if (write_in_full(fd, buf, len) < 0) > > + rc = error_errno(_("could not write to '%s'"), path); > > + if (!rc && ftruncate(fd, len) < 0) > > + rc = error_errno(_("could not truncate '%s'"), path); > > + close(fd); > > + return rc; > > +} > > + > > /* skip picking commits whose parents are unchanged */ > > int skip_unnecessary_picks(void) > > { > > @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void) > > } > > close(fd); > > > > - fd = open(rebase_path_todo(), O_WRONLY, 0666); > > - if (fd < 0) { > > - error_errno(_("could not open '%s' for writing"), > > - rebase_path_todo()); > > + if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset, > > + todo_list.buf.len - offset) < 0) { > > todo_list_release(&todo_list); > > return -1; > > } > > - if (write_in_full(fd, todo_list.buf.buf + offset, > > - todo_list.buf.len - offset) < 0) { > > - error_errno(_("could not write to '%s'"), > > - rebase_path_todo()); > > - close(fd); > > - todo_list_release(&todo_list); > > Is this missing on purpose in the new situation? > I wasn't looking at the context, only the changed lines. After reading it again, it's clear that nothing is missing (the freeing of todo_list). Kevin ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-10-31 9:54 [PATCH 1/2] sequencer: factor out rewrite_file() René Scharfe 2017-10-31 9:58 ` [PATCH 2/2] sequencer: use O_TRUNC to truncate files René Scharfe 2017-10-31 16:33 ` [PATCH 1/2] sequencer: factor out rewrite_file() Kevin Daudt @ 2017-11-01 11:10 ` Simon Ruderich 2017-11-01 13:00 ` René Scharfe 2017-11-01 15:33 ` [PATCH 1/2] sequencer: factor out rewrite_file() Johannes Schindelin 2017-11-01 19:47 ` Jeff King 4 siblings, 1 reply; 35+ messages in thread From: Simon Ruderich @ 2017-11-01 11:10 UTC (permalink / raw) To: René Scharfe Cc: Git List, Junio C Hamano, Ralf Thielow, Johannes Schindelin On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote: > +static int rewrite_file(const char *path, const char *buf, size_t len) > +{ > + int rc = 0; > + int fd = open(path, O_WRONLY); > + if (fd < 0) > + return error_errno(_("could not open '%s' for writing"), path); > + if (write_in_full(fd, buf, len) < 0) > + rc = error_errno(_("could not write to '%s'"), path); > + if (!rc && ftruncate(fd, len) < 0) > + rc = error_errno(_("could not truncate '%s'"), path); > + close(fd); We might want to check the return value of close() as some file systems report write errors only on close. But I'm not sure how the rest of Git's code-base handles this. > + return rc; > +} Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-01 11:10 ` Simon Ruderich @ 2017-11-01 13:00 ` René Scharfe 2017-11-01 14:44 ` [PATCH 1/2] wrapper.c: consistently quote filenames in error messages Simon Ruderich 2017-11-01 14:45 ` [PATCH 2/2] sequencer.c: check return value of close() in rewrite_file() Simon Ruderich 0 siblings, 2 replies; 35+ messages in thread From: René Scharfe @ 2017-11-01 13:00 UTC (permalink / raw) To: Simon Ruderich Cc: Git List, Junio C Hamano, Ralf Thielow, Johannes Schindelin Am 01.11.2017 um 12:10 schrieb Simon Ruderich: > On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote: >> +static int rewrite_file(const char *path, const char *buf, size_t len) >> +{ >> + int rc = 0; >> + int fd = open(path, O_WRONLY); >> + if (fd < 0) >> + return error_errno(_("could not open '%s' for writing"), path); >> + if (write_in_full(fd, buf, len) < 0) >> + rc = error_errno(_("could not write to '%s'"), path); >> + if (!rc && ftruncate(fd, len) < 0) >> + rc = error_errno(_("could not truncate '%s'"), path); >> + close(fd); > > We might want to check the return value of close() as some file > systems report write errors only on close. But I'm not sure how > the rest of Git's code-base handles this. Most calls are not checked, but that doesn't necessarily mean they need to (or should) stay that way. The Linux man-page of close(2) spends multiple paragraphs recommending to check its return value.. Care to send a follow-up patch? René ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/2] wrapper.c: consistently quote filenames in error messages 2017-11-01 13:00 ` René Scharfe @ 2017-11-01 14:44 ` Simon Ruderich 2017-11-02 4:40 ` Junio C Hamano 2017-11-01 14:45 ` [PATCH 2/2] sequencer.c: check return value of close() in rewrite_file() Simon Ruderich 1 sibling, 1 reply; 35+ messages in thread From: Simon Ruderich @ 2017-11-01 14:44 UTC (permalink / raw) To: René Scharfe Cc: Git List, Junio C Hamano, Ralf Thielow, Johannes Schindelin All other error messages in the file use quotes around the file name. This change removes two translations as "could not write to '%s'" and "could not close '%s'" are already translated and these two are the only occurrences without quotes. Signed-off-by: Simon Ruderich <simon@ruderich.org> --- wrapper.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/wrapper.c b/wrapper.c index 61aba0b5c..d20356a77 100644 --- a/wrapper.c +++ b/wrapper.c @@ -569,7 +569,7 @@ static int warn_if_unremovable(const char *op, const char *file, int rc) if (!rc || errno == ENOENT) return 0; err = errno; - warning_errno("unable to %s %s", op, file); + warning_errno("unable to %s '%s'", op, file); errno = err; return rc; } @@ -583,7 +583,7 @@ int unlink_or_msg(const char *file, struct strbuf *err) if (!rc || errno == ENOENT) return 0; - strbuf_addf(err, "unable to unlink %s: %s", + strbuf_addf(err, "unable to unlink '%s': %s", file, strerror(errno)); return -1; } @@ -653,9 +653,9 @@ void write_file_buf(const char *path, const char *buf, size_t len) { int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666); if (write_in_full(fd, buf, len) < 0) - die_errno(_("could not write to %s"), path); + die_errno(_("could not write to '%s'"), path); if (close(fd)) - die_errno(_("could not close %s"), path); + die_errno(_("could not close '%s'"), path); } void write_file(const char *path, const char *fmt, ...) -- 2.15.0 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages 2017-11-01 14:44 ` [PATCH 1/2] wrapper.c: consistently quote filenames in error messages Simon Ruderich @ 2017-11-02 4:40 ` Junio C Hamano 2017-11-02 5:16 ` Junio C Hamano 0 siblings, 1 reply; 35+ messages in thread From: Junio C Hamano @ 2017-11-02 4:40 UTC (permalink / raw) To: Simon Ruderich Cc: René Scharfe, Git List, Ralf Thielow, Johannes Schindelin Simon Ruderich <simon@ruderich.org> writes: > All other error messages in the file use quotes around the file name. > > This change removes two translations as "could not write to '%s'" and > "could not close '%s'" are already translated and these two are the only > occurrences without quotes. > > Signed-off-by: Simon Ruderich <simon@ruderich.org> > --- > wrapper.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) This patch is incomplete without adjusting a handful of tests to expect the updated messages, no? ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages 2017-11-02 4:40 ` Junio C Hamano @ 2017-11-02 5:16 ` Junio C Hamano 2017-11-02 10:20 ` Simon Ruderich 0 siblings, 1 reply; 35+ messages in thread From: Junio C Hamano @ 2017-11-02 5:16 UTC (permalink / raw) To: Simon Ruderich Cc: René Scharfe, Git List, Ralf Thielow, Johannes Schindelin Junio C Hamano <gitster@pobox.com> writes: > Simon Ruderich <simon@ruderich.org> writes: > >> All other error messages in the file use quotes around the file name. >> >> This change removes two translations as "could not write to '%s'" and >> "could not close '%s'" are already translated and these two are the only >> occurrences without quotes. >> >> Signed-off-by: Simon Ruderich <simon@ruderich.org> >> --- >> wrapper.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) > > This patch is incomplete without adjusting a handful of tests to > expect the updated messages, no? I'll squash these in while queuing, but there might be more that I didn't notice. Thansk. t/t3600-rm.sh | 2 +- t/t7001-mv.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh index f8568f8841..ab5500db44 100755 --- a/t/t3600-rm.sh +++ b/t/t3600-rm.sh @@ -688,7 +688,7 @@ test_expect_success 'checking out a commit after submodule removal needs manual git submodule update && git checkout -q HEAD^ && git checkout -q master 2>actual && - test_i18ngrep "^warning: unable to rmdir submod:" actual && + test_i18ngrep "^warning: unable to rmdir '\''submod'\'':" actual && git status -s submod >actual && echo "?? submod/" >expected && test_cmp expected actual && diff --git a/t/t7001-mv.sh b/t/t7001-mv.sh index f5929c46f3..6e5031f56f 100755 --- a/t/t7001-mv.sh +++ b/t/t7001-mv.sh @@ -452,7 +452,7 @@ test_expect_success 'checking out a commit before submodule moved needs manual u git mv sub sub2 && git commit -m "moved sub to sub2" && git checkout -q HEAD^ 2>actual && - test_i18ngrep "^warning: unable to rmdir sub2:" actual && + test_i18ngrep "^warning: unable to rmdir '\''sub2'\'':" actual && git status -s sub2 >actual && echo "?? sub2/" >expected && test_cmp expected actual && ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages 2017-11-02 5:16 ` Junio C Hamano @ 2017-11-02 10:20 ` Simon Ruderich 2017-11-03 1:47 ` Junio C Hamano 0 siblings, 1 reply; 35+ messages in thread From: Simon Ruderich @ 2017-11-02 10:20 UTC (permalink / raw) To: Junio C Hamano Cc: René Scharfe, Git List, Ralf Thielow, Johannes Schindelin On Thu, Nov 02, 2017 at 02:16:52PM +0900, Junio C Hamano wrote: > Junio C Hamano writes: >> This patch is incomplete without adjusting a handful of tests to >> expect the updated messages, no? > > I'll squash these in while queuing, but there might be more that I > didn't notice. Sorry, didn't think about the tests. I've re-checked and I think those are the only affected tests. The test suite passes with your squashed changes. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages 2017-11-02 10:20 ` Simon Ruderich @ 2017-11-03 1:47 ` Junio C Hamano 0 siblings, 0 replies; 35+ messages in thread From: Junio C Hamano @ 2017-11-03 1:47 UTC (permalink / raw) To: Simon Ruderich Cc: René Scharfe, Git List, Ralf Thielow, Johannes Schindelin Simon Ruderich <simon@ruderich.org> writes: > On Thu, Nov 02, 2017 at 02:16:52PM +0900, Junio C Hamano wrote: >> Junio C Hamano writes: >>> This patch is incomplete without adjusting a handful of tests to >>> expect the updated messages, no? >> >> I'll squash these in while queuing, but there might be more that I >> didn't notice. > > Sorry, didn't think about the tests. Heh, tests are not something you need to think about, if you always run them after making changes. > I've re-checked and I think those are the only affected tests. > The test suite passes with your squashed changes. OK. Thanks. ^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 2/2] sequencer.c: check return value of close() in rewrite_file() 2017-11-01 13:00 ` René Scharfe 2017-11-01 14:44 ` [PATCH 1/2] wrapper.c: consistently quote filenames in error messages Simon Ruderich @ 2017-11-01 14:45 ` Simon Ruderich 2017-11-01 17:09 ` René Scharfe 1 sibling, 1 reply; 35+ messages in thread From: Simon Ruderich @ 2017-11-01 14:45 UTC (permalink / raw) To: René Scharfe Cc: Git List, Junio C Hamano, Ralf Thielow, Johannes Schindelin Not checking close(2) can hide errors as not all errors are reported during the write(2). Signed-off-by: Simon Ruderich <simon@ruderich.org> --- On Wed, Nov 01, 2017 at 02:00:11PM +0100, René Scharfe wrote: > Most calls are not checked, but that doesn't necessarily mean they need > to (or should) stay that way. The Linux man-page of close(2) spends > multiple paragraphs recommending to check its return value.. Care to > send a follow-up patch? Hello, Sure, here is it. Regards Simon sequencer.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/sequencer.c b/sequencer.c index f93b60f61..e0cc2f777 100644 --- a/sequencer.c +++ b/sequencer.c @@ -2673,7 +2673,8 @@ static int rewrite_file(const char *path, const char *buf, size_t len) return error_errno(_("could not open '%s' for writing"), path); if (write_in_full(fd, buf, len) < 0) rc = error_errno(_("could not write to '%s'"), path); - close(fd); + if (close(fd) && !rc) + rc = error_errno(_("could not close '%s'"), path); return rc; } -- 2.15.0 -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 2/2] sequencer.c: check return value of close() in rewrite_file() 2017-11-01 14:45 ` [PATCH 2/2] sequencer.c: check return value of close() in rewrite_file() Simon Ruderich @ 2017-11-01 17:09 ` René Scharfe 0 siblings, 0 replies; 35+ messages in thread From: René Scharfe @ 2017-11-01 17:09 UTC (permalink / raw) To: Simon Ruderich Cc: Git List, Junio C Hamano, Ralf Thielow, Johannes Schindelin Am 01.11.2017 um 15:45 schrieb Simon Ruderich: > Not checking close(2) can hide errors as not all errors are reported > during the write(2). > > Signed-off-by: Simon Ruderich <simon@ruderich.org> > --- > > On Wed, Nov 01, 2017 at 02:00:11PM +0100, René Scharfe wrote: >> Most calls are not checked, but that doesn't necessarily mean they need >> to (or should) stay that way. The Linux man-page of close(2) spends >> multiple paragraphs recommending to check its return value.. Care to >> send a follow-up patch? > > Hello, > > Sure, here is it. > > Regards > Simon > > sequencer.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/sequencer.c b/sequencer.c > index f93b60f61..e0cc2f777 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -2673,7 +2673,8 @@ static int rewrite_file(const char *path, const char *buf, size_t len) > return error_errno(_("could not open '%s' for writing"), path); > if (write_in_full(fd, buf, len) < 0) > rc = error_errno(_("could not write to '%s'"), path); > - close(fd); > + if (close(fd) && !rc) > + rc = error_errno(_("could not close '%s'"), path); > return rc; > } > > Looks good to me, thank you! René ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-10-31 9:54 [PATCH 1/2] sequencer: factor out rewrite_file() René Scharfe ` (2 preceding siblings ...) 2017-11-01 11:10 ` Simon Ruderich @ 2017-11-01 15:33 ` Johannes Schindelin 2017-11-01 19:47 ` Jeff King 4 siblings, 0 replies; 35+ messages in thread From: Johannes Schindelin @ 2017-11-01 15:33 UTC (permalink / raw) To: René Scharfe; +Cc: Git List, Junio C Hamano, Ralf Thielow [-- Attachment #1: Type: text/plain, Size: 173 bytes --] Hi René, On Tue, 31 Oct 2017, René Scharfe wrote: > Reduce code duplication by extracting a function for rewriting an > existing file. Fine by me. Thanks, Dscho ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-10-31 9:54 [PATCH 1/2] sequencer: factor out rewrite_file() René Scharfe ` (3 preceding siblings ...) 2017-11-01 15:33 ` [PATCH 1/2] sequencer: factor out rewrite_file() Johannes Schindelin @ 2017-11-01 19:47 ` Jeff King 2017-11-01 21:46 ` Johannes Schindelin 4 siblings, 1 reply; 35+ messages in thread From: Jeff King @ 2017-11-01 19:47 UTC (permalink / raw) To: René Scharfe Cc: Git List, Junio C Hamano, Ralf Thielow, Johannes Schindelin On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote: > Reduce code duplication by extracting a function for rewriting an > existing file. These patches look like an improvement on their own, but I wonder if we shouldn't just be using the existing write_file_buf() for this? Compared to your new function: > +static int rewrite_file(const char *path, const char *buf, size_t len) > +{ > + int rc = 0; > + int fd = open(path, O_WRONLY); > + if (fd < 0) > + return error_errno(_("could not open '%s' for writing"), path); > + if (write_in_full(fd, buf, len) < 0) > + rc = error_errno(_("could not write to '%s'"), path); > + if (!rc && ftruncate(fd, len) < 0) > + rc = error_errno(_("could not truncate '%s'"), path); > + close(fd); > + return rc; > +} - write_file_buf() uses O_TRUNC instead of ftruncate (but you end up there in your second patch) - it uses O_CREAT, which I think would be OK (we do not expect to create the file, but it would work fine when the file does exist). - it calls die() rather than returning an error. Looking at the callsites, I'm inclined to say that would be fine. Failing to write to the todo file is essentially a fatal error for sequencer code. -Peff ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-01 19:47 ` Jeff King @ 2017-11-01 21:46 ` Johannes Schindelin 2017-11-01 22:16 ` Jeff King 0 siblings, 1 reply; 35+ messages in thread From: Johannes Schindelin @ 2017-11-01 21:46 UTC (permalink / raw) To: Jeff King; +Cc: René Scharfe, Git List, Junio C Hamano, Ralf Thielow [-- Attachment #1: Type: text/plain, Size: 2059 bytes --] Hi Peff, On Wed, 1 Nov 2017, Jeff King wrote: > On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote: > > > Reduce code duplication by extracting a function for rewriting an > > existing file. > > These patches look like an improvement on their own, but I wonder if we > shouldn't just be using the existing write_file_buf() for this? > > Compared to your new function: > > > +static int rewrite_file(const char *path, const char *buf, size_t len) > > +{ > > + int rc = 0; > > + int fd = open(path, O_WRONLY); > > + if (fd < 0) > > + return error_errno(_("could not open '%s' for writing"), path); > > + if (write_in_full(fd, buf, len) < 0) > > + rc = error_errno(_("could not write to '%s'"), path); > > + if (!rc && ftruncate(fd, len) < 0) > > + rc = error_errno(_("could not truncate '%s'"), path); > > + close(fd); > > + return rc; > > +} > > - write_file_buf() uses O_TRUNC instead of ftruncate (but you end up > there in your second patch) > > - it uses O_CREAT, which I think would be OK (we do not expect to > create the file, but it would work fine when the file does exist). > > - it calls die() rather than returning an error. Looking at the > callsites, I'm inclined to say that would be fine. Failing to write > to the todo file is essentially a fatal error for sequencer code. I spent substantial time on making the sequencer code libified (it was far from it). That die() call may look okay now, but it is not at all okay if we want to make Git's source code cleaner and more reusable. And I want to. So my suggestion is to clean up write_file_buf() first, to stop behaving like a drunk lemming, and to return an error value already, and only then use it in sequencer.c. Ciao, Dscho P.S.: The existing callers of write_file_buf() don't care because they are builtins, and for some reason we deem it okay for code in builtins to simply die() deep in the call chains, without any way for callers to give advice how to get out of the current mess. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-01 21:46 ` Johannes Schindelin @ 2017-11-01 22:16 ` Jeff King 2017-11-03 10:32 ` Simon Ruderich 0 siblings, 1 reply; 35+ messages in thread From: Jeff King @ 2017-11-01 22:16 UTC (permalink / raw) To: Johannes Schindelin Cc: René Scharfe, Git List, Junio C Hamano, Ralf Thielow On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote: > > - it calls die() rather than returning an error. Looking at the > > callsites, I'm inclined to say that would be fine. Failing to write > > to the todo file is essentially a fatal error for sequencer code. > > I spent substantial time on making the sequencer code libified (it was far > from it). That die() call may look okay now, but it is not at all okay if > we want to make Git's source code cleaner and more reusable. And I want > to. > > So my suggestion is to clean up write_file_buf() first, to stop behaving > like a drunk lemming, and to return an error value already, and only then > use it in sequencer.c. That would be fine with me, too. -Peff ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-01 22:16 ` Jeff King @ 2017-11-03 10:32 ` Simon Ruderich 2017-11-03 13:44 ` Junio C Hamano 2017-11-03 14:46 ` [PATCH 1/2] sequencer: factor out rewrite_file() Johannes Schindelin 0 siblings, 2 replies; 35+ messages in thread From: Simon Ruderich @ 2017-11-03 10:32 UTC (permalink / raw) To: Jeff King Cc: Johannes Schindelin, René Scharfe, Git List, Junio C Hamano, Ralf Thielow On Wed, Nov 01, 2017 at 06:16:18PM -0400, Jeff King wrote: > On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote: >> I spent substantial time on making the sequencer code libified (it was far >> from it). That die() call may look okay now, but it is not at all okay if >> we want to make Git's source code cleaner and more reusable. And I want >> to. >> >> So my suggestion is to clean up write_file_buf() first, to stop behaving >> like a drunk lemming, and to return an error value already, and only then >> use it in sequencer.c. > > That would be fine with me, too. I tried looking into this by adding a new write_file_buf_gently() (or maybe renaming write_file_buf to write_file_buf_or_die) and using it from write_file_buf() but I don't know the proper way to handle the error-case in write_file_buf(). Just calling die("write_file_buf") feels ugly, as the real error was already printed on screen by error_errno() and I didn't find any function to just exit without writing a message (which still respects die_routine). Suggestions welcome. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-03 10:32 ` Simon Ruderich @ 2017-11-03 13:44 ` Junio C Hamano 2017-11-03 19:13 ` Jeff King 2017-11-03 14:46 ` [PATCH 1/2] sequencer: factor out rewrite_file() Johannes Schindelin 1 sibling, 1 reply; 35+ messages in thread From: Junio C Hamano @ 2017-11-03 13:44 UTC (permalink / raw) To: Simon Ruderich Cc: Jeff King, Johannes Schindelin, René Scharfe, Git List, Ralf Thielow Simon Ruderich <simon@ruderich.org> writes: > I tried looking into this by adding a new write_file_buf_gently() > (or maybe renaming write_file_buf to write_file_buf_or_die) and > using it from write_file_buf() but I don't know the proper way to > handle the error-case in write_file_buf(). Just calling > die("write_file_buf") feels ugly, as the real error was already > printed on screen by error_errno() and I didn't find any function > to just exit without writing a message (which still respects > die_routine). Suggestions welcome. How about *not* printing the error at the place where you notice the error, and instead return an error code to the caller to be noticed which dies with an error message? ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-03 13:44 ` Junio C Hamano @ 2017-11-03 19:13 ` Jeff King 2017-11-04 9:05 ` René Scharfe 2017-11-04 18:36 ` Simon Ruderich 0 siblings, 2 replies; 35+ messages in thread From: Jeff King @ 2017-11-03 19:13 UTC (permalink / raw) To: Junio C Hamano Cc: Simon Ruderich, Johannes Schindelin, René Scharfe, Git List, Ralf Thielow On Fri, Nov 03, 2017 at 10:44:08PM +0900, Junio C Hamano wrote: > Simon Ruderich <simon@ruderich.org> writes: > > > I tried looking into this by adding a new write_file_buf_gently() > > (or maybe renaming write_file_buf to write_file_buf_or_die) and > > using it from write_file_buf() but I don't know the proper way to > > handle the error-case in write_file_buf(). Just calling > > die("write_file_buf") feels ugly, as the real error was already > > printed on screen by error_errno() and I didn't find any function > > to just exit without writing a message (which still respects > > die_routine). Suggestions welcome. > > How about *not* printing the error at the place where you notice the > error, and instead return an error code to the caller to be noticed > which dies with an error message? That ends up giving less-specific errors. It might be an OK tradeoff here. I think we've been gravitating towards error strbufs, which would make it something like: diff --git a/wrapper.c b/wrapper.c index 61aba0b5c1..08eb5d1cb8 100644 --- a/wrapper.c +++ b/wrapper.c @@ -649,13 +649,34 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...) return len; } +int write_file_buf_gently(const char *path, const char *buf, size_t len, + struct strbuf *err) +{ + int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666); + if (fd < 0) { + strbuf_addf(err, _("could not open '%s' for writing: %s"), + path, strerror(errno)); + return -1; + } + if (write_in_full(fd, buf, len) < 0) { + strbuf_addf(err, _("could not write to %s: %s"), + path, strerror(errno)); + close(fd); + return -1; + } + if (close(fd)) { + strbuf_addf(err, _("could not close %s: %s"), + path, strerror(errno)); + return -1; + } + return 0; +} + void write_file_buf(const char *path, const char *buf, size_t len) { - int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666); - if (write_in_full(fd, buf, len) < 0) - die_errno(_("could not write to %s"), path); - if (close(fd)) - die_errno(_("could not close %s"), path); + struct strbuf err = STRBUF_INIT; + if (write_file_buf_gently(path, buf, len, &err) < 0) + die("%s", err.buf); } void write_file(const char *path, const char *fmt, ...) I'm not excited that the amount of error-handling code is now double the amount of code that actually does something useful. Maybe this function simply isn't large/complex enough to merit flexible error handling, and we should simply go with René's original near-duplicate. OTOH, if we went all-in on flexible error handling contexts, you could imagine this function becoming: void write_file_buf(const char *path, const char *buf, size_t len, struct error_context *err) { int fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666); if (fd < 0) return -1; if (write_in_full(fd, buf, len, err) < 0) return -1; if (xclose(fd, err) < 0) return -1; return 0; } Kind of gross, in that we're adding a layer on top of all system calls. But if used consistently, it makes error-reporting a lot more pleasant, and makes all of our "whoops, we forgot to save errno" bugs go away. -Peff ^ permalink raw reply related [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-03 19:13 ` Jeff King @ 2017-11-04 9:05 ` René Scharfe 2017-11-04 9:35 ` Jeff King 2017-11-04 18:36 ` Simon Ruderich 1 sibling, 1 reply; 35+ messages in thread From: René Scharfe @ 2017-11-04 9:05 UTC (permalink / raw) To: Jeff King, Junio C Hamano Cc: Simon Ruderich, Johannes Schindelin, Git List, Ralf Thielow Am 03.11.2017 um 20:13 schrieb Jeff King: > On Fri, Nov 03, 2017 at 10:44:08PM +0900, Junio C Hamano wrote: > >> Simon Ruderich <simon@ruderich.org> writes: >> >>> I tried looking into this by adding a new write_file_buf_gently() >>> (or maybe renaming write_file_buf to write_file_buf_or_die) and >>> using it from write_file_buf() but I don't know the proper way to >>> handle the error-case in write_file_buf(). Just calling >>> die("write_file_buf") feels ugly, as the real error was already >>> printed on screen by error_errno() and I didn't find any function >>> to just exit without writing a message (which still respects >>> die_routine). Suggestions welcome. >> >> How about *not* printing the error at the place where you notice the >> error, and instead return an error code to the caller to be noticed >> which dies with an error message? > > That ends up giving less-specific errors. Not necessarily. Function could return different codes for different errors, e.g. -1 for an open(2) error and -2 for a write(2) error, and the caller could use that to select the message to show. Basically all of the messages in wrapper.c consist of some text mixed with the affected path path and a strerror(3) string, so they're compatible in that way. A single function (get_path_error_format()?) could thus be used and callers would be able to combine its result with die(), error(), or warning(). René ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-04 9:05 ` René Scharfe @ 2017-11-04 9:35 ` Jeff King 0 siblings, 0 replies; 35+ messages in thread From: Jeff King @ 2017-11-04 9:35 UTC (permalink / raw) To: René Scharfe Cc: Junio C Hamano, Simon Ruderich, Johannes Schindelin, Git List, Ralf Thielow On Sat, Nov 04, 2017 at 10:05:43AM +0100, René Scharfe wrote: > >> How about *not* printing the error at the place where you notice the > >> error, and instead return an error code to the caller to be noticed > >> which dies with an error message? > > > > That ends up giving less-specific errors. > > Not necessarily. Function could return different codes for different > errors, e.g. -1 for an open(2) error and -2 for a write(2) error, and > the caller could use that to select the message to show. > > Basically all of the messages in wrapper.c consist of some text mixed > with the affected path path and a strerror(3) string, so they're > compatible in that way. A single function (get_path_error_format()?) > could thus be used and callers would be able to combine its result with > die(), error(), or warning(). I think we've had this discussion before, a while ago. Yes, returning an integer error code is nice because you don't have pass in an extra parameter. But I think there are two pitfalls: 1. Integers may not be descriptive enough to cover all cases, which is how we ended up with the strbuf-passing strategy in the ref code. Certainly you could add an integer for every possible bespoke message, but then I'm not sure it's buying that much over having the function simply fill in a strbuf. 2. For complex functions there may be multiple errors that need to stack. I think the refs code has cases like this, where a syscall fails, which causes a fundamental ref operation to fail, which causes a higher-level operation to fail. It's only the caller of the higher-level operation that knows how to report the error. Certainly an integer error code would work for _this_ function, but I'd rather see us grow towards consistent error handling. -Peff ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-03 19:13 ` Jeff King 2017-11-04 9:05 ` René Scharfe @ 2017-11-04 18:36 ` Simon Ruderich 2017-11-05 2:07 ` Jeff King 1 sibling, 1 reply; 35+ messages in thread From: Simon Ruderich @ 2017-11-04 18:36 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Johannes Schindelin, René Scharfe, Git List, Ralf Thielow On Fri, Nov 03, 2017 at 03:13:10PM -0400, Jeff King wrote: > I think we've been gravitating towards error strbufs, which would make > it something like: I like this approach to store the error in a separate variable and let the caller handle it. This provides proper error messages and is cleaner than printing the error on the error site (what error_errno does). However I wouldn't use strbuf directly and instead add a new struct error which provides a small set of helper functions. Using a separate type also makes it clear to the reader that is not a normal string and is more extendable in the future. > I'm not excited that the amount of error-handling code is now double the > amount of code that actually does something useful. Maybe this function > simply isn't large/complex enough to merit flexible error handling, and > we should simply go with René's original near-duplicate. A separate struct (and helper functions) would help in this case and could look like this, which is almost equal (in code size) to the original solution using error_errno: int write_file_buf_gently2(const char *path, const char *buf, size_t len, struct error *err) { int rc = 0; int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666); if (fd < 0) return error_addf_errno(err, _("could not open '%s' for writing"), path); if (write_in_full(fd, buf, len) < 0) rc = error_addf_errno(err, _("could not write to '%s'"), path); if (close(fd) && !rc) rc = error_addf_errno(err, _("could not close '%s'"), path); return rc; } (I didn't touch write_in_full here, but it could also take the err and then the code would get a little shorter, however would lose the "path" information, but see below.) And in the caller: void write_file_buf(const char *path, const char *buf, size_t len) { struct error err = ERROR_INIT; if (write_file_buf_gently2(path, buf, len, &err) < 0) error_die(&err); } For now struct error just contains the strbuf, but one could add the call location (by using a macro for error_addf_errno) or the original errno or more information in the future. error_addf_errno() could also prepend the error the buffer so that the caller can add more information if necessary and we get something like: "failed to write file 'foo': write failed: errno text" in the write_file_buf case (the first error string is from write_file_buf_gently2, the second from write_in_full). However I'm not sure how well this works with translations. We could also store the error condition in the error struct and don't use the return value to indicate and error like this: void write_file_buf(const char *path, const char *buf, size_t len) { struct error err = ERROR_INIT; write_file_buf_gently2(path, buf, len, &err); if (err.error) error_die(&err); } > OTOH, if we went all-in on flexible error handling contexts, you could > imagine this function becoming: > > void write_file_buf(const char *path, const char *buf, size_t len, > struct error_context *err) > { > int fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666); > if (fd < 0) > return -1; > if (write_in_full(fd, buf, len, err) < 0) > return -1; > if (xclose(fd, err) < 0) > return -1; > return 0; > } This looks interesting as well, but it misses the feature of custom error messages which is really useful. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-04 18:36 ` Simon Ruderich @ 2017-11-05 2:07 ` Jeff King 2017-11-06 16:13 ` Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) Simon Ruderich 0 siblings, 1 reply; 35+ messages in thread From: Jeff King @ 2017-11-05 2:07 UTC (permalink / raw) To: Simon Ruderich Cc: Junio C Hamano, Johannes Schindelin, René Scharfe, Git List, Ralf Thielow On Sat, Nov 04, 2017 at 07:36:43PM +0100, Simon Ruderich wrote: > On Fri, Nov 03, 2017 at 03:13:10PM -0400, Jeff King wrote: > > I think we've been gravitating towards error strbufs, which would make > > it something like: > > I like this approach to store the error in a separate variable > and let the caller handle it. This provides proper error messages > and is cleaner than printing the error on the error site (what > error_errno does). > > However I wouldn't use strbuf directly and instead add a new > struct error which provides a small set of helper functions. > Using a separate type also makes it clear to the reader that is > not a normal string and is more extendable in the future. Yes, I think what you've written here (and below) is quite close to the error_context patches I linked elsewhere in the thread. In other words, I think it's a sane approach. > We could also store the error condition in the error struct and > don't use the return value to indicate and error like this: > > void write_file_buf(const char *path, const char *buf, size_t len) > { > struct error err = ERROR_INIT; > write_file_buf_gently2(path, buf, len, &err); > if (err.error) > error_die(&err); > } I agree it might be nice for the error context to have a positive "there was an error" flag. It's probably worth making it redundant with the return code, though, so callers can use whichever style is most convenient for them. > > OTOH, if we went all-in on flexible error handling contexts, you could > > imagine this function becoming: > > > > void write_file_buf(const char *path, const char *buf, size_t len, > > struct error_context *err) > > { > > int fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666); > > if (fd < 0) > > return -1; > > if (write_in_full(fd, buf, len, err) < 0) > > return -1; > > if (xclose(fd, err) < 0) > > return -1; > > return 0; > > } > > This looks interesting as well, but it misses the feature of > custom error messages which is really useful. Right, I didn't think that example through. The functions after the open() don't have enough information to make a good message. -Peff ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) 2017-11-05 2:07 ` Jeff King @ 2017-11-06 16:13 ` Simon Ruderich 2017-11-16 10:36 ` Simon Ruderich 2017-11-17 22:33 ` Jeff King 0 siblings, 2 replies; 35+ messages in thread From: Simon Ruderich @ 2017-11-06 16:13 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Johannes Schindelin, René Scharfe, Git List, Ralf Thielow On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote: > Yes, I think what you've written here (and below) is quite close to the > error_context patches I linked elsewhere in the thread. In other > words, I think it's a sane approach. In contrast to error_context I'd like to keep all exiting behavior (die, ignore, etc.) in the hand of the caller and not use any callbacks as that makes the control flow much harder to follow. > I agree it might be nice for the error context to have a positive "there > was an error" flag. It's probably worth making it redundant with the > return code, though, so callers can use whichever style is most > convenient for them. Agreed. Regarding the API, should it be allowed to pass NULL as error pointer to request no additional error handling or should the error functions panic on NULL? Allowing NULL makes partial conversions possible (e.g. for write_in_full) where old callers just pass NULL and check the return values and converted callers can use the error struct. How should translations get handled? Appending ": %s" for strerror(errno) might be problematic. Same goes for "outer message: inner message" where the helper function just inserts ": " between the messages. Is _("%s: %s") (with appropriate translator comments) enough to handle these cases? Suggestions how to name the struct and the corresponding functions? My initial idea was struct error and to use error_ as prefix, but I'm not sure if struct error is too broad and may introduce conflicts with system headers. Also error_ is a little long and could be shorted to just err_ but I don't know if that's clear enough. The error_ prefix doesn't conflict with many git functions, but there are some in usage.c (error_errno, error, error_routine). And as general question, is this approach to error handling something we should pursue or are there objections? If there's consensus that this might be a good idea I'll look into converting some parts of the git code (maybe refs.c) to see how it pans out. Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) 2017-11-06 16:13 ` Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) Simon Ruderich @ 2017-11-16 10:36 ` Simon Ruderich 2017-11-17 22:33 ` Jeff King 1 sibling, 0 replies; 35+ messages in thread From: Simon Ruderich @ 2017-11-16 10:36 UTC (permalink / raw) To: Jeff King Cc: Junio C Hamano, Johannes Schindelin, René Scharfe, Git List, Ralf Thielow On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote: > On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote: >> Yes, I think what you've written here (and below) is quite close to the >> error_context patches I linked elsewhere in the thread. In other >> words, I think it's a sane approach. > > In contrast to error_context I'd like to keep all exiting > behavior (die, ignore, etc.) in the hand of the caller and not > use any callbacks as that makes the control flow much harder to > follow. > >> I agree it might be nice for the error context to have a positive "there >> was an error" flag. It's probably worth making it redundant with the >> return code, though, so callers can use whichever style is most >> convenient for them. > > Agreed. > > Regarding the API, should it be allowed to pass NULL as error > pointer to request no additional error handling or should the > error functions panic on NULL? Allowing NULL makes partial > conversions possible (e.g. for write_in_full) where old callers > just pass NULL and check the return values and converted callers > can use the error struct. > > How should translations get handled? Appending ": %s" for > strerror(errno) might be problematic. Same goes for "outer > message: inner message" where the helper function just inserts ": > " between the messages. Is _("%s: %s") (with appropriate > translator comments) enough to handle these cases? > > Suggestions how to name the struct and the corresponding > functions? My initial idea was struct error and to use error_ as > prefix, but I'm not sure if struct error is too broad and may > introduce conflicts with system headers. Also error_ is a little > long and could be shorted to just err_ but I don't know if that's > clear enough. The error_ prefix doesn't conflict with many git > functions, but there are some in usage.c (error_errno, error, > error_routine). > > And as general question, is this approach to error handling > something we should pursue or are there objections? If there's > consensus that this might be a good idea I'll look into > converting some parts of the git code (maybe refs.c) to see how > it pans out. Any comments? Regards Simon -- + privacy is necessary + using gnupg http://gnupg.org + public key id: 0x92FEFDB7E44C32F9 ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) 2017-11-06 16:13 ` Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) Simon Ruderich 2017-11-16 10:36 ` Simon Ruderich @ 2017-11-17 22:33 ` Jeff King 2017-11-18 9:01 ` Johannes Sixt 1 sibling, 1 reply; 35+ messages in thread From: Jeff King @ 2017-11-17 22:33 UTC (permalink / raw) To: Simon Ruderich Cc: Junio C Hamano, Johannes Schindelin, René Scharfe, Git List, Ralf Thielow On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote: > On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote: > > Yes, I think what you've written here (and below) is quite close to the > > error_context patches I linked elsewhere in the thread. In other > > words, I think it's a sane approach. > > In contrast to error_context I'd like to keep all exiting > behavior (die, ignore, etc.) in the hand of the caller and not > use any callbacks as that makes the control flow much harder to > follow. Yeah, I have mixed feelings on that. I think it does make the control flow less clear. At the same time, what I found was that handlers like die/ignore/warn were the thing that gave the most reduction in complexity in the callers. > Regarding the API, should it be allowed to pass NULL as error > pointer to request no additional error handling or should the > error functions panic on NULL? Allowing NULL makes partial > conversions possible (e.g. for write_in_full) where old callers > just pass NULL and check the return values and converted callers > can use the error struct. I think it's probably better to be explicit, and pass some "noop" error handling struct. We'll have to be adding parameters to functions to handle this anyway, so I don't think there's much opportunity for having NULL as a fallback for partial conversions. > How should translations get handled? Appending ": %s" for > strerror(errno) might be problematic. Same goes for "outer > message: inner message" where the helper function just inserts ": > " between the messages. Is _("%s: %s") (with appropriate > translator comments) enough to handle these cases? I don't have a real opinion, not having done much translation myself. I will say that the existing die_errno(), error_errno(), etc just use "%s: %s", without even allowing for translation (see fmt_with_err in usage.c). I'm sure that probably sucks for RTL languages, but I think it would be fine to punt on it for now. > Suggestions how to name the struct and the corresponding > functions? My initial idea was struct error and to use error_ as > prefix, but I'm not sure if struct error is too broad and may > introduce conflicts with system headers. Also error_ is a little > long and could be shorted to just err_ but I don't know if that's > clear enough. The error_ prefix doesn't conflict with many git > functions, but there are some in usage.c (error_errno, error, > error_routine). In my experiments[1] I called the types error_*, and then generally used "err" as a local variable when necessary. Variants on that seem fine to me, but yeah, you have to avoid conflicting with error(). We _could_ rename that, but it would be a pretty invasive patch. > And as general question, is this approach to error handling > something we should pursue or are there objections? If there's > consensus that this might be a good idea I'll look into > converting some parts of the git code (maybe refs.c) to see how > it pans out. I dunno. I kind of like the idea, but if the only error context is one that adds to strbufs, I don't know that it's buying us much over the status quo (which is passing around strbufs). It's a little more explicit, I guess. Other list regulars besides me seem mostly quiet on the subject. -Peff [1] This is the jk/error-context-wip branch of https://github.com/peff/git. I can't remember if I mentioned that before. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) 2017-11-17 22:33 ` Jeff King @ 2017-11-18 9:01 ` Johannes Sixt 2017-12-24 14:54 ` Jeff King 0 siblings, 1 reply; 35+ messages in thread From: Johannes Sixt @ 2017-11-18 9:01 UTC (permalink / raw) To: Jeff King, Simon Ruderich Cc: Junio C Hamano, Johannes Schindelin, René Scharfe, Git List, Ralf Thielow Am 17.11.2017 um 23:33 schrieb Jeff King: > On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote: >> On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote: >>> Yes, I think what you've written here (and below) is quite close to the >>> error_context patches I linked elsewhere in the thread. In other >>> words, I think it's a sane approach. >> >> In contrast to error_context I'd like to keep all exiting >> behavior (die, ignore, etc.) in the hand of the caller and not >> use any callbacks as that makes the control flow much harder to >> follow. > > Yeah, I have mixed feelings on that. I think it does make the control > flow less clear. At the same time, what I found was that handlers like > die/ignore/warn were the thing that gave the most reduction in > complexity in the callers. Would you not consider switching over to C++? With exceptions, you get the error context without cluttering the API. (Did I mention that librarification would become a breeze? Do not die in library routines: not a problem anymore, just catch the exception. die_on_error parameters? Not needed anymore. Not to mention that resource leaks would be much, MUCH simpler to treat.) -- Hannes ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) 2017-11-18 9:01 ` Johannes Sixt @ 2017-12-24 14:54 ` Jeff King 2017-12-24 15:45 ` Randall S. Becker 2017-12-25 10:28 ` Johannes Sixt 0 siblings, 2 replies; 35+ messages in thread From: Jeff King @ 2017-12-24 14:54 UTC (permalink / raw) To: Johannes Sixt Cc: Simon Ruderich, Junio C Hamano, Johannes Schindelin, René Scharfe, Git List, Ralf Thielow On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote: > > Yeah, I have mixed feelings on that. I think it does make the control > > flow less clear. At the same time, what I found was that handlers like > > die/ignore/warn were the thing that gave the most reduction in > > complexity in the callers. > > Would you not consider switching over to C++? With exceptions, you get the > error context without cluttering the API. (Did I mention that > librarification would become a breeze? Do not die in library routines: not a > problem anymore, just catch the exception. die_on_error parameters? Not > needed anymore. Not to mention that resource leaks would be much, MUCH > simpler to treat.) I threw this email on my todo pile since I was traveling when it came, but I think it deserves a response (albeit quite late). It's been a long while since I've done any serious C++, but I did really like the RAII pattern coupled with exceptions. That said, I think it's dangerous to do it half-way, and especially to retrofit an existing code base. It introduces a whole new control-flow pattern that is invisible to the existing code, so you're going to get leaks and variables in unexpected states whenever you see an exception. I also suspect there'd be a fair bit of in converting the existing code to something that actually compiles as C++. So if we were starting the project from scratch and thinking about using C++ with RAII and exceptions, sure, that's something I'd entertain[1] (and maybe even Linus has softened on his opinion of C++ these days ;) ). But at this point, it doesn't seem like the tradeoff for switching is there. -Peff [1] I'd also consider Rust, though I'm not too experienced with it myself. ^ permalink raw reply [flat|nested] 35+ messages in thread
* RE: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) 2017-12-24 14:54 ` Jeff King @ 2017-12-24 15:45 ` Randall S. Becker 2017-12-25 10:28 ` Johannes Sixt 1 sibling, 0 replies; 35+ messages in thread From: Randall S. Becker @ 2017-12-24 15:45 UTC (permalink / raw) To: 'Jeff King', 'Johannes Sixt' Cc: 'Simon Ruderich', 'Junio C Hamano', 'Johannes Schindelin', 'René Scharfe', 'Git List', 'Ralf Thielow' On December 24, 2017 9:54 AM, Jeff King wrote: > Subject: Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor > out rewrite_file()) > > On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote: > > > > Yeah, I have mixed feelings on that. I think it does make the > > > control flow less clear. At the same time, what I found was that > > > handlers like die/ignore/warn were the thing that gave the most > > > reduction in complexity in the callers. > > > > Would you not consider switching over to C++? With exceptions, you get > > the error context without cluttering the API. (Did I mention that > > librarification would become a breeze? Do not die in library routines: > > not a problem anymore, just catch the exception. die_on_error > > parameters? Not needed anymore. Not to mention that resource leaks > > would be much, MUCH simpler to treat.) > > I threw this email on my todo pile since I was traveling when it came, but I > think it deserves a response (albeit quite late). > > It's been a long while since I've done any serious C++, but I did really like the > RAII pattern coupled with exceptions. That said, I think it's dangerous to do it > half-way, and especially to retrofit an existing code base. It introduces a > whole new control-flow pattern that is invisible to the existing code, so > you're going to get leaks and variables in unexpected states whenever you > see an exception. > > I also suspect there'd be a fair bit of in converting the existing code to > something that actually compiles as C++. > > So if we were starting the project from scratch and thinking about using > C++ with RAII and exceptions, sure, that's something I'd entertain[1] > (and maybe even Linus has softened on his opinion of C++ these days ;) ). > But at this point, it doesn't seem like the tradeoff for switching is there. While 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: 1. Conversion to C++ just to pick up exceptions is a lot like "One does not simply walk to Mordor", as Peff hinted at above. 2. 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. 3. 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. 4. 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. My 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. With Respect, Randall -- Brief whoami: NonStop&UNIX developer since approximately UNIX(421664400)/NonStop(211288444200000000) -- In my real life, I talk too much. ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) 2017-12-24 14:54 ` Jeff King 2017-12-24 15:45 ` Randall S. Becker @ 2017-12-25 10:28 ` Johannes Sixt 1 sibling, 0 replies; 35+ messages in thread From: Johannes Sixt @ 2017-12-25 10:28 UTC (permalink / raw) To: Jeff King Cc: Simon Ruderich, Junio C Hamano, Johannes Schindelin, René Scharfe, Git List, Ralf Thielow Am 24.12.2017 um 15:54 schrieb Jeff King: > On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote: > >>> Yeah, I have mixed feelings on that. I think it does make the control >>> flow less clear. At the same time, what I found was that handlers like >>> die/ignore/warn were the thing that gave the most reduction in >>> complexity in the callers. >> >> Would you not consider switching over to C++? With exceptions, you get the >> error context without cluttering the API. (Did I mention that >> librarification would become a breeze? Do not die in library routines: not a >> problem anymore, just catch the exception. die_on_error parameters? Not >> needed anymore. Not to mention that resource leaks would be much, MUCH >> simpler to treat.) > > I threw this email on my todo pile since I was traveling when it came, > but I think it deserves a response (albeit quite late). > > It's been a long while since I've done any serious C++, but I did really > like the RAII pattern coupled with exceptions. That said, I think it's > dangerous to do it half-way, and especially to retrofit an existing code > base. It introduces a whole new control-flow pattern that is invisible > to the existing code, so you're going to get leaks and variables in > unexpected states whenever you see an exception. > > I also suspect there'd be a fair bit of in converting the existing code > to something that actually compiles as C++. I think I mentioned that I had a version that passed the test suite. It's not pure C++ as it required -fpermissive due to the many implicit void*-to-pointer-to-object conversions (which are disallowed in C++). And, yes, a fair bit of conversion was required on top of that. ;) > So if we were starting the project from scratch and thinking about using > C++ with RAII and exceptions, sure, that's something I'd entertain[1] > (and maybe even Linus has softened on his opinion of C++ these days ;) ). > But at this point, it doesn't seem like the tradeoff for switching is > there. Fair enough. I do agree that the tradeoff is not there, in particular, when the major players are more fluent in C than in modern C++. There is just my usual rant: Why do we have look for resource leaks during review when we could have leak-free code by design? (But Dscho scored a point[*] some time ago: "For every fool-proof system invented, somebody invents a better fool.") [*] https://public-inbox.org/git/alpine.DEB.2.20.1704281334060.3480@virtualbox/ ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-03 10:32 ` Simon Ruderich 2017-11-03 13:44 ` Junio C Hamano @ 2017-11-03 14:46 ` Johannes Schindelin 2017-11-03 18:57 ` Jeff King 1 sibling, 1 reply; 35+ messages in thread From: Johannes Schindelin @ 2017-11-03 14:46 UTC (permalink / raw) To: Simon Ruderich Cc: Jeff King, René Scharfe, Git List, Junio C Hamano, Ralf Thielow Hi Simon, On Fri, 3 Nov 2017, Simon Ruderich wrote: > On Wed, Nov 01, 2017 at 06:16:18PM -0400, Jeff King wrote: > > On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote: > >> I spent substantial time on making the sequencer code libified (it was far > >> from it). That die() call may look okay now, but it is not at all okay if > >> we want to make Git's source code cleaner and more reusable. And I want > >> to. > >> > >> So my suggestion is to clean up write_file_buf() first, to stop behaving > >> like a drunk lemming, and to return an error value already, and only then > >> use it in sequencer.c. > > > > That would be fine with me, too. > > I tried looking into this by adding a new write_file_buf_gently() > (or maybe renaming write_file_buf to write_file_buf_or_die) and > using it from write_file_buf() but I don't know the proper way to > handle the error-case in write_file_buf(). Just calling > die("write_file_buf") feels ugly, as the real error was already > printed on screen by error_errno() and I didn't find any function > to just exit without writing a message (which still respects > die_routine). Suggestions welcome. In my ideal world, we could use all those fancy refactoring tools that are currently en vogue and simply turn *all* error()/error_errno() calls into context-aware functions that can be told to die() right away, or to return the error in an error buffer, depending hwhat the caller (or the call chain, really) wants. This is quite a bit more object-oriented than Git's code base, though, and besides, I am not aware of any refactoring tool that would make this painless (it's not just a matter of adding a parameter, you also have to pass it through all of the call chain, something you get for free when working with an object-oriented language). So what I did in the sequencer when faced with the same conundrum was to simply return -1 if the function I called returned a negative value. The top-level builtin (in that case, `rebase--helper`) simply returns !!ret as exit code (so that `-1` gets translated into the exit code `1`). BTW I would not use the `_or_die()` convention, as it suggests that that function will *always* die() in the error case. Instead, what I would follow is the `, int die_on_error` pattern e.g. of `real_pathdup()`, and simply *add* that parameter to the signature (and changing the return value to `int`). Ciao, Dscho ^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 1/2] sequencer: factor out rewrite_file() 2017-11-03 14:46 ` [PATCH 1/2] sequencer: factor out rewrite_file() Johannes Schindelin @ 2017-11-03 18:57 ` Jeff King 0 siblings, 0 replies; 35+ messages in thread From: Jeff King @ 2017-11-03 18:57 UTC (permalink / raw) To: Johannes Schindelin Cc: Simon Ruderich, René Scharfe, Git List, Junio C Hamano, Ralf Thielow On Fri, Nov 03, 2017 at 03:46:44PM +0100, Johannes Schindelin wrote: > > I tried looking into this by adding a new write_file_buf_gently() > > (or maybe renaming write_file_buf to write_file_buf_or_die) and > > using it from write_file_buf() but I don't know the proper way to > > handle the error-case in write_file_buf(). Just calling > > die("write_file_buf") feels ugly, as the real error was already > > printed on screen by error_errno() and I didn't find any function > > to just exit without writing a message (which still respects > > die_routine). Suggestions welcome. > > In my ideal world, we could use all those fancy refactoring tools that are > currently en vogue and simply turn *all* error()/error_errno() calls into > context-aware functions that can be told to die() right away, or to return > the error in an error buffer, depending hwhat the caller (or the call > chain, really) wants. > > This is quite a bit more object-oriented than Git's code base, though, and > besides, I am not aware of any refactoring tool that would make this > painless (it's not just a matter of adding a parameter, you also have to > pass it through all of the call chain, something you get for free when > working with an object-oriented language). FWIW, I sketched this out a bit here: https://public-inbox.org/git/20160928085841.aoisson3fnuke47q@sigill.intra.peff.net/ And you can see the patches I played with while writing that here: https://github.com/peff/git/compare/cb5918aa0d50f50e83787f65c2ddc3dcb10159fe...4d61927e66dcfdbdb6cc6c88ec4018e2142e826c (but note they don't quite compile, as some of the conversions are half-done; it was really just to get a sense of the flavor of the thing). One of the complaints was that it makes it harder to see when we are calling die() (because it's now happening via an error callback). That maybe confusing for users, but it may also affect generated code since the code paths that hit the NORETURN function are obscured. But we could stop short of adding error_die, and just have error_silent, error_warn, and error_print (and callers can turn error_print into a die() themselves). -Peff ^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2017-12-25 10:29 UTC | newest] Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-10-31 9:54 [PATCH 1/2] sequencer: factor out rewrite_file() René Scharfe 2017-10-31 9:58 ` [PATCH 2/2] sequencer: use O_TRUNC to truncate files René Scharfe 2017-10-31 16:34 ` Kevin Daudt 2017-11-01 15:34 ` Johannes Schindelin 2017-10-31 16:33 ` [PATCH 1/2] sequencer: factor out rewrite_file() Kevin Daudt 2017-11-01 6:06 ` Kevin Daudt 2017-11-01 11:10 ` Simon Ruderich 2017-11-01 13:00 ` René Scharfe 2017-11-01 14:44 ` [PATCH 1/2] wrapper.c: consistently quote filenames in error messages Simon Ruderich 2017-11-02 4:40 ` Junio C Hamano 2017-11-02 5:16 ` Junio C Hamano 2017-11-02 10:20 ` Simon Ruderich 2017-11-03 1:47 ` Junio C Hamano 2017-11-01 14:45 ` [PATCH 2/2] sequencer.c: check return value of close() in rewrite_file() Simon Ruderich 2017-11-01 17:09 ` René Scharfe 2017-11-01 15:33 ` [PATCH 1/2] sequencer: factor out rewrite_file() Johannes Schindelin 2017-11-01 19:47 ` Jeff King 2017-11-01 21:46 ` Johannes Schindelin 2017-11-01 22:16 ` Jeff King 2017-11-03 10:32 ` Simon Ruderich 2017-11-03 13:44 ` Junio C Hamano 2017-11-03 19:13 ` Jeff King 2017-11-04 9:05 ` René Scharfe 2017-11-04 9:35 ` Jeff King 2017-11-04 18:36 ` Simon Ruderich 2017-11-05 2:07 ` Jeff King 2017-11-06 16:13 ` Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file()) Simon Ruderich 2017-11-16 10:36 ` Simon Ruderich 2017-11-17 22:33 ` Jeff King 2017-11-18 9:01 ` Johannes Sixt 2017-12-24 14:54 ` Jeff King 2017-12-24 15:45 ` Randall S. Becker 2017-12-25 10:28 ` Johannes Sixt 2017-11-03 14:46 ` [PATCH 1/2] sequencer: factor out rewrite_file() Johannes Schindelin 2017-11-03 18:57 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.