[PATCH] sequencer.c: unify error messages

[PATCH] sequencer.c: unify error messages

[Ralf Thielow]

When ftruncate() in rearrange_squash() fails, we write
that we couldn't finish the operation on the todo file.
It is more accurate to write that we couldn't truncate
as we do in other calls of ftruncate().

While at there, remove a full stop in another error message.

Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e258bb646..b0e6459a5 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2948,9 +2948,9 @@ int rearrange_squash(void)
 		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 read '%s'."), todo_file);
+			res = error_errno(_("could not read '%s'"), todo_file);
 		else if (ftruncate(fd, buf.len) < 0)
-			res = error_errno(_("could not finish '%s'"),
+			res = error_errno(_("could not truncate '%s'"),
 					   todo_file);
 		close(fd);
 		strbuf_release(&buf);
-- 
2.15.0.rc0.296.g7b26d72

Re: [PATCH] sequencer.c: unify error messages

[René Scharfe]

Am 13.10.2017 um 19:51 schrieb Ralf Thielow:
> When ftruncate() in rearrange_squash() fails, we write
> that we couldn't finish the operation on the todo file.
> It is more accurate to write that we couldn't truncate
> as we do in other calls of ftruncate().

Would it make sense to factor out rewriting the to-do file to avoid
code duplication in the first place?

> While at there, remove a full stop in another error message.
> 
> Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
> ---
>   sequencer.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index e258bb646..b0e6459a5 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2948,9 +2948,9 @@ int rearrange_squash(void)
>   		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 read '%s'."), todo_file);
> +			res = error_errno(_("could not read '%s'"), todo_file);
                                                       ^^^^
That should read "write", right?

>   		else if (ftruncate(fd, buf.len) < 0)
> -			res = error_errno(_("could not finish '%s'"),
> +			res = error_errno(_("could not truncate '%s'"),
>   					   todo_file);

Hmm, why call ftruncate(2) instead of opening the file with O_TRUNC?

>   		close(fd);
>   		strbuf_release(&buf);
> 

Re: [PATCH] sequencer.c: unify error messages

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 434 bytes --]

Hi Ralf,

On Fri, 13 Oct 2017, Ralf Thielow wrote:

> When ftruncate() in rearrange_squash() fails, we write
> that we couldn't finish the operation on the todo file.
> It is more accurate to write that we couldn't truncate
> as we do in other calls of ftruncate().
> 
> While at there, remove a full stop in another error message.

Thanks, looks good (maybe s/read/write/ as pointed out by René, though?).

Ciao,
Dscho

Re: [PATCH] sequencer.c: unify error messages

[Ralf Thielow]

2017-10-13 23:12 GMT+02:00 René Scharfe <l.s.r@web.de>:
> Am 13.10.2017 um 19:51 schrieb Ralf Thielow:
>> When ftruncate() in rearrange_squash() fails, we write
>> that we couldn't finish the operation on the todo file.
>> It is more accurate to write that we couldn't truncate
>> as we do in other calls of ftruncate().
>
> Would it make sense to factor out rewriting the to-do file to avoid
> code duplication in the first place?
>
>> While at there, remove a full stop in another error message.
>>
>> Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
>> ---
>>   sequencer.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/sequencer.c b/sequencer.c
>> index e258bb646..b0e6459a5 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -2948,9 +2948,9 @@ int rearrange_squash(void)
>>               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 read '%s'."), todo_file);
>> +                     res = error_errno(_("could not read '%s'"), todo_file);
>                                                        ^^^^
> That should read "write", right?
>

Sure. I'll send a new version of this patch to fix the messages. Maybe
someone else picks up the other things. Thanks.

>>               else if (ftruncate(fd, buf.len) < 0)
>> -                     res = error_errno(_("could not finish '%s'"),
>> +                     res = error_errno(_("could not truncate '%s'"),
>>                                          todo_file);
>
> Hmm, why call ftruncate(2) instead of opening the file with O_TRUNC?
>
>>               close(fd);
>>               strbuf_release(&buf);
>>

[PATCH v2] sequencer.c: fix and unify error messages in rearrange_squash()

[Ralf Thielow]

When the write opertion fails, we write that we could
not read. Change the error message to match the operation
and remove the full stop at the end.

When ftruncate() fails, we write that we couldn't finish
the operation on the todo file. It is more accurate to write
that we couldn't truncate as we do in other calls of ftruncate().

Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
---
 sequencer.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index e258bb646..75f5356f6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -2948,9 +2948,9 @@ int rearrange_squash(void)
 		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 read '%s'."), todo_file);
+			res = error_errno(_("could not write '%s'"), todo_file);
 		else if (ftruncate(fd, buf.len) < 0)
-			res = error_errno(_("could not finish '%s'"),
+			res = error_errno(_("could not truncate '%s'"),
 					   todo_file);
 		close(fd);
 		strbuf_release(&buf);
-- 
2.15.0.rc0.296.g7b26d72

Re: [PATCH v2] sequencer.c: fix and unify error messages in rearrange_squash()

[Junio C Hamano]

Ralf Thielow <ralf.thielow@gmail.com> writes:

> When the write opertion fails, we write that we could
> not read. Change the error message to match the operation
> and remove the full stop at the end.
>
> When ftruncate() fails, we write that we couldn't finish
> the operation on the todo file. It is more accurate to write
> that we couldn't truncate as we do in other calls of ftruncate().

Wouldn't it be more accurate to say we couldn't ftruncate, though?

>
> Signed-off-by: Ralf Thielow <ralf.thielow@gmail.com>
> ---
>  sequencer.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index e258bb646..75f5356f6 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2948,9 +2948,9 @@ int rearrange_squash(void)
>  		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 read '%s'."), todo_file);
> +			res = error_errno(_("could not write '%s'"), todo_file);
>  		else if (ftruncate(fd, buf.len) < 0)
> -			res = error_errno(_("could not finish '%s'"),
> +			res = error_errno(_("could not truncate '%s'"),
>  					   todo_file);
>  		close(fd);
>  		strbuf_release(&buf);

Re: [PATCH v2] sequencer.c: fix and unify error messages in rearrange_squash()

[Johannes Schindelin]

Hi Junio,

On Mon, 16 Oct 2017, Junio C Hamano wrote:

> Ralf Thielow <ralf.thielow@gmail.com> writes:
> 
> > When the write opertion fails, we write that we could
> > not read. Change the error message to match the operation
> > and remove the full stop at the end.
> >
> > When ftruncate() fails, we write that we couldn't finish
> > the operation on the todo file. It is more accurate to write
> > that we couldn't truncate as we do in other calls of ftruncate().
> 
> Wouldn't it be more accurate to say we couldn't ftruncate, though?

This is an end-user facing error message, right? Should we not let users
who are happily oblivious of POSIX nomenclature remain happily oblivious?

In other words, I would be finer with "truncate" than with "ftruncate...
wait, huh? Is that even English?"

Ciao,
Dscho

Re: [PATCH v2] sequencer.c: fix and unify error messages in rearrange_squash()

[Philip Oakley]

From: "Johannes Schindelin" <Johannes.Schindelin@gmx.de>
> On Mon, 16 Oct 2017, Junio C Hamano wrote:
>
>> Ralf Thielow <ralf.thielow@gmail.com> writes:
>>
>> > When the write opertion fails, we write that we could
>> > not read. Change the error message to match the operation
>> > and remove the full stop at the end.
>> >
>> > When ftruncate() fails, we write that we couldn't finish
>> > the operation on the todo file. It is more accurate to write
>> > that we couldn't truncate as we do in other calls of ftruncate().
>>
>> Wouldn't it be more accurate to say we couldn't ftruncate, though?
>
> This is an end-user facing error message, right? Should we not let users
> who are happily oblivious of POSIX nomenclature remain happily oblivious?
>
> In other words, I would be finer with "truncate" than with "ftruncate...
> wait, huh? Is that even English?"
>
Hi, 'Truncate' is real English, but it is not that common in normal usage.

My dictionary suggests that it means 'cut off at the tip' such as a 
truncated cone. However the thesaurus is far more relaxed about the common 
idioms that truncate at the tail such as: clip, crop, cut short, trim, 
abbreviate, curtail, etc.

So perhaps "could not trim '%s'".

--
Philip

Re: [PATCH v2] sequencer.c: fix and unify error messages in rearrange_squash()

[Junio C Hamano]

"Philip Oakley" <philipoakley@iee.org> writes:

> Hi, 'Truncate' is real English, but it is not that common in normal usage.
>
> My dictionary suggests that it means 'cut off at the tip' such as a
> truncated cone. However the thesaurus is far more relaxed about the
> common idioms that truncate at the tail such as: clip, crop, cut
> short, trim, abbreviate, curtail, etc.
>
> So perhaps "could not trim '%s'".

Truncate is fine, as there is already another instance that barfs
with "cannot truncate" upon an error from ftruncate(), and the patch
merely matches the two error messages.