[PATCH 1/2] sequencer: factor out rewrite_file()

[PATCH 1/2] sequencer: factor out rewrite_file()

[René Scharfe]

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

[PATCH 2/2] sequencer: use O_TRUNC to truncate files

[René Scharfe]

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

Re: [PATCH 2/2] sequencer: use O_TRUNC to truncate files

[Kevin Daudt]

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

Re: [PATCH 2/2] sequencer: use O_TRUNC to truncate files

[Johannes Schindelin]

[-- 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

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Kevin Daudt]

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.

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Kevin Daudt]

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

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[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.

> +	return rc;
> +}

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[René Scharfe]

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é

[PATCH 1/2] wrapper.c: consistently quote filenames in error messages

[Simon Ruderich]

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

Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages

[Junio C Hamano]

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?

Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages

[Junio C Hamano]

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 &&

Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages

[Simon Ruderich]

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

Re: [PATCH 1/2] wrapper.c: consistently quote filenames in error messages

[Junio C Hamano]

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.

[PATCH 2/2] sequencer.c: check return value of close() in rewrite_file()

[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;
 }
 
-- 
2.15.0

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

Re: [PATCH 2/2] sequencer.c: check return value of close() in rewrite_file()

[René Scharfe]

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é

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Johannes Schindelin]

[-- 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

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Jeff King]

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

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Johannes Schindelin]

[-- 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.

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Jeff King]

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

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Simon Ruderich]

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

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Junio C Hamano]

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?

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[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. 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

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[René Scharfe]

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é

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Jeff King]

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

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Simon Ruderich]

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

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Jeff King]

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

Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())

[Simon Ruderich]

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

Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())

[Simon Ruderich]

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

Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())

[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.

> 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.

Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())

[Johannes Sixt]

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

Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())

[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++.

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.

RE: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())

[Randall S. Becker]

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.

Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())

[Johannes Sixt]

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/

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Johannes Schindelin]

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

Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Jeff King]

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