Re: mktemp: Clarify error message

[Scott Cheloha]

On Tue, Dec 26, 2017 at 12:31:02AM +0100, Klemens Nanni wrote:
> On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
> > I think this is a silly solution, and the documentation is clear
> > enough.
> The manual page certainly is clear enough but the current error message
> is logically wrong, as there are sufficient Xs *in* `XXs' but just
> not at the end of it, call it nitpicking if you will.
> 
> > How did this happen to you?  Show the place where it happened to you.
> > Would the text you propose actually have saved you 1 second of time
> > to help you realize what was wrong?  I don't think so.
> Just a typo really making me think "this could be clearer". So yes, I
> find telling this way actually saves time understanding the error, even
> if so little.

I think your error message is too complex.  It explains two possible
error cases in a single go.  This necessitates more thinking on the part
of the user.

Why not just acknowledge the two errors differently?

$ mktemp test.X
mktemp: insufficient number of Xs in template: test.X
$ mktemp test.XXC
mktemp: invalid template suffix: test.XXC
# mktemp test.XX
test.KJ9TG4

--
Scott Cheloha

Index: mktemp.1
===
RCS file: /cvs/src/usr.bin/mktemp/mktemp.1,v
retrieving revision 1.28
diff -u -p -r1.28 mktemp.1
--- mktemp.17 Aug 2013 06:19:36 -   1.28
+++ mktemp.126 Dec 2017 13:53:18 -
@@ -228,12 +228,17 @@ does not succeed and the
 .Fl q
 option was not specified:
 .Bl -tag -width indent
+.It Li "invalid template suffix"
+The specified
+.Ar template
+was suffixed with something other than
+.Ql X Ns s.
 .It Li "insufficient number of Xs in template"
 The specified
 .Ar template
 contained fewer than six
 .Ql X Ns s
-at the end.
+in its suffix.
 .It Li "template must not contain directory separators in -t mode"
 The
 .Ar template
Index: mktemp.c
===
RCS file: /cvs/src/usr.bin/mktemp/mktemp.c,v
retrieving revision 1.22
diff -u -p -r1.22 mktemp.c
--- mktemp.c9 Oct 2015 01:37:08 -   1.22
+++ mktemp.c26 Dec 2017 13:53:18 -
@@ -77,10 +77,10 @@ main(int argc, char *argv[])
}
 
len = strlen(template);
-   if (len < 6 || strcmp([len - 6], "XX")) {
-   fatalx("insufficient number of Xs in template `%s'",
-   template);
-   }
+   if (template[len - 1] != 'X')
+   fatalx("invalid template suffix: %s", template);
+   if (len < 6 || strcmp([len - 6], "XX"))
+   fatalx("insufficient number of Xs in template: %s", template);
if (tflag) {
if (strchr(template, '/')) {
fatalx("template must not contain directory "

Re: mktemp: Clarify error message

[Marc Espie]

On Tue, Dec 26, 2017 at 08:25:50AM +0100, Otto Moerbeek wrote:
> 
> This reminds me of the old IRIX compiler, that would cite complete
> parapgraphs of the C standard in error mesasges. Of course logically
> it was all correct, but it lead to long and unreadable error messages
> that filled up disks with build logs.

Actually, having at least the paragraph number in the standard would
often help a lot with the more arcane messages.

Re: mktemp: Clarify error message

[Theo de Raadt]

>On 12/25/17, Otto Moerbeek  wrote:
>> On Tue, Dec 26, 2017 at 12:31:02AM +0100, Klemens Nanni wrote:
>>
>>> On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
>>> > I think this is a silly solution, and the documentation is clear
>>> > enough.
>>> The manual page certainly is clear enough but the current error message
>>> is logically wrong, as there are sufficient Xs *in* `XXs' but just
>>> not at the end of it, call it nitpicking if you will.
>>>
>>> > How did this happen to you?  Show the place where it happened to you.
>>> > Would the text you propose actually have saved you 1 second of time
>>> > to help you realize what was wrong?  I don't think so.
>>> Just a typo really making me think "this could be clearer". So yes, I
>>> find telling this way actually saves time understanding the error, even
>>> if so little.
>>>
>>> > If you weren't familiar that the template has to be minimum 6 XX at
>>> > end of the string, then you hadn't achieved familiarity of the
>>> > subject matter yet.
>>> I agree that knowing one from the manual implies knowing the other as
>>> well, but it doesn't seem reason enough to keep the error message as is,
>>> hence the diff.
>>
>> I disagree. An error message does not need to document everything, An
>> erro message should short and clear enough together with the doumentation.
>
>all well and good, but let's not drop words and letters in pursuit of brevity.

this isn't a competition to see if your opinion matters

Re: mktemp: Clarify error message

[Theo de Raadt]

>On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
>> I think this is a silly solution, and the documentation is clear
>> enough.
>The manual page certainly is clear enough but the current error message
>is logically wrong, as there are sufficient Xs *in* `XXs' but just
>not at the end of it, call it nitpicking if you will.

Disagree.

It is logically correct according to the definition of a mktemp
template.  If you put X elsewhere it is not template material.

If the *programmer* didn't understand what a template was and screwed
it up, providing the *application user* with detailed information
isn't useful.  If the programmer is that user, the terse message is
enough to indicate "hey moron, go study"

And now, you want to come up with a 40 character snippet of text
which will inform the user of the software that the programmer
didn't follow the rules..

And let me guess, soon you'll find another fundamental concept easily
learned in the manual page, and want to sneak it into the error
message also?

Sorry, that's not how it works.  Short firm error messages
provide a strong hint that further study is needed.

Unix is a terse operating system.  It is considered a strength.

Re: mktemp: Clarify error message

[patrick keshishian]

On 12/25/17, Otto Moerbeek  wrote:
> On Tue, Dec 26, 2017 at 12:31:02AM +0100, Klemens Nanni wrote:
>
>> On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
>> > I think this is a silly solution, and the documentation is clear
>> > enough.
>> The manual page certainly is clear enough but the current error message
>> is logically wrong, as there are sufficient Xs *in* `XXs' but just
>> not at the end of it, call it nitpicking if you will.
>>
>> > How did this happen to you?  Show the place where it happened to you.
>> > Would the text you propose actually have saved you 1 second of time
>> > to help you realize what was wrong?  I don't think so.
>> Just a typo really making me think "this could be clearer". So yes, I
>> find telling this way actually saves time understanding the error, even
>> if so little.
>>
>> > If you weren't familiar that the template has to be minimum 6 XX at
>> > end of the string, then you hadn't achieved familiarity of the
>> > subject matter yet.
>> I agree that knowing one from the manual implies knowing the other as
>> well, but it doesn't seem reason enough to keep the error message as is,
>> hence the diff.
>
> I disagree. An error message does not need to document everything, An
> erro message should short and clear enough together with the doumentation.

all well and good, but let's not drop words and letters in pursuit of brevity.

-pk

> This reminds me of the old IRIX compiler, that would cite complete
> parapgraphs of the C standard in error mesasges. Of course logically
> it was all correct, but it lead to long and unreadable error messages
> that filled up disks with build logs.
>
>   -Otto
>
>
>
>

Re: mktemp: Clarify error message

[Otto Moerbeek]

On Tue, Dec 26, 2017 at 12:31:02AM +0100, Klemens Nanni wrote:

> On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
> > I think this is a silly solution, and the documentation is clear
> > enough.
> The manual page certainly is clear enough but the current error message
> is logically wrong, as there are sufficient Xs *in* `XXs' but just
> not at the end of it, call it nitpicking if you will.
> 
> > How did this happen to you?  Show the place where it happened to you.
> > Would the text you propose actually have saved you 1 second of time
> > to help you realize what was wrong?  I don't think so.
> Just a typo really making me think "this could be clearer". So yes, I
> find telling this way actually saves time understanding the error, even
> if so little.
> 
> > If you weren't familiar that the template has to be minimum 6 XX at
> > end of the string, then you hadn't achieved familiarity of the
> > subject matter yet.
> I agree that knowing one from the manual implies knowing the other as
> well, but it doesn't seem reason enough to keep the error message as is,
> hence the diff.

I disagree. An error message does not need to document everything, An
erro message should short and clear enough together with the doumentation.

This reminds me of the old IRIX compiler, that would cite complete
parapgraphs of the C standard in error mesasges. Of course logically
it was all correct, but it lead to long and unreadable error messages
that filled up disks with build logs.

-Otto

Re: mktemp: Clarify error message

[Klemens Nanni]

On Mon, Dec 25, 2017 at 03:57:00PM -0700, Theo de Raadt wrote:
> I think this is a silly solution, and the documentation is clear
> enough.
The manual page certainly is clear enough but the current error message
is logically wrong, as there are sufficient Xs *in* `XXs' but just
not at the end of it, call it nitpicking if you will.

> How did this happen to you?  Show the place where it happened to you.
> Would the text you propose actually have saved you 1 second of time
> to help you realize what was wrong?  I don't think so.
Just a typo really making me think "this could be clearer". So yes, I
find telling this way actually saves time understanding the error, even
if so little.

> If you weren't familiar that the template has to be minimum 6 XX at
> end of the string, then you hadn't achieved familiarity of the
> subject matter yet.
I agree that knowing one from the manual implies knowing the other as
well, but it doesn't seem reason enough to keep the error message as is,
hence the diff.

Re: mktemp: Clarify error message

[Theo de Raadt]

I think this is a silly solution, and the documentation is clear
enough.

How did this happen to you?  Show the place where it happened to you.
Would the text you propose actually have saved you 1 second of time
to help you realize what was wrong?  I don't think so.  If you
weren't familiar that the template has to be minimum 6 XX at
end of the string, then you hadn't achieved familiarity of the
subject matter yet.

>On Mon, Dec 25, 2017 at 08:36:07PM +, Stuart Henderson wrote:
>> On 2017/12/25 20:52, Klemens Nanni wrote:
>> > from mktemp(1):
>> > 
>> >The template may be any filename with at least six ‘Xs’ appended
>> >to it, for example /tmp/tfile.XX.
>> > 
>> > Now when a template contains but does not end in six Xs, the error
>> > message may imply errornous behaviour instead of bad usage:
>> > 
>> >$ mktemp XX
>> >oAQnQ5
>> >$ mktemp XXs
>> >mktemp: insufficient number of Xs in template `XXs'
>> > 
>> > I'd like to see a more precise error message here.
>> > 
>> > Feedback?
>> > 
>> > diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
>> > index 713b67fd105..c080d1d6474 100644
>> > --- a/usr.bin/mktemp/mktemp.c
>> > +++ b/usr.bin/mktemp/mktemp.c
>> > @@ -77,10 +77,9 @@ main(int argc, char *argv[])
>> >}
>> >  
>> >len = strlen(template);
>> > -  if (len < 6 || strcmp([len - 6], "XX")) {
>> > -  fatalx("insufficient number of Xs in template `%s'",
>> > -  template);
>> > -  }
>> > +  if (len < 6 || strcmp([len - 6], "XX"))
>> > +  fatalx("template must end in six or more Xs");
>> > +
>> >if (tflag) {
>> >if (strchr(template, '/')) {
>> >fatalx("template must not contain directory "
>> > 
>> 
>> Printing the actual template used makes it easier to track down
>> the problematic call.
>Fair enough, how about this?
>
>diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
>index 713b67fd105..96b6731ca90 100644
>--- a/usr.bin/mktemp/mktemp.c
>+++ b/usr.bin/mktemp/mktemp.c
>@@ -78,7 +78,7 @@ main(int argc, char *argv[])
> 
>   len = strlen(template);
>   if (len < 6 || strcmp([len - 6], "XX")) {
>-  fatalx("insufficient number of Xs in template `%s'",
>+  fatalx("insufficient number of Xs at end of template `%s'",
>   template);
>   }
>   if (tflag) {
>
>

Re: mktemp: Clarify error message

[Klemens Nanni]

On Mon, Dec 25, 2017 at 08:36:07PM +, Stuart Henderson wrote:
> On 2017/12/25 20:52, Klemens Nanni wrote:
> > from mktemp(1):
> > 
> > The template may be any filename with at least six ‘Xs’ appended
> > to it, for example /tmp/tfile.XX.
> > 
> > Now when a template contains but does not end in six Xs, the error
> > message may imply errornous behaviour instead of bad usage:
> > 
> > $ mktemp XX
> > oAQnQ5
> > $ mktemp XXs
> > mktemp: insufficient number of Xs in template `XXs'
> > 
> > I'd like to see a more precise error message here.
> > 
> > Feedback?
> > 
> > diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
> > index 713b67fd105..c080d1d6474 100644
> > --- a/usr.bin/mktemp/mktemp.c
> > +++ b/usr.bin/mktemp/mktemp.c
> > @@ -77,10 +77,9 @@ main(int argc, char *argv[])
> > }
> >  
> > len = strlen(template);
> > -   if (len < 6 || strcmp([len - 6], "XX")) {
> > -   fatalx("insufficient number of Xs in template `%s'",
> > -   template);
> > -   }
> > +   if (len < 6 || strcmp([len - 6], "XX"))
> > +   fatalx("template must end in six or more Xs");
> > +
> > if (tflag) {
> > if (strchr(template, '/')) {
> > fatalx("template must not contain directory "
> > 
> 
> Printing the actual template used makes it easier to track down
> the problematic call.
Fair enough, how about this?

diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
index 713b67fd105..96b6731ca90 100644
--- a/usr.bin/mktemp/mktemp.c
+++ b/usr.bin/mktemp/mktemp.c
@@ -78,7 +78,7 @@ main(int argc, char *argv[])
 
len = strlen(template);
if (len < 6 || strcmp([len - 6], "XX")) {
-   fatalx("insufficient number of Xs in template `%s'",
+   fatalx("insufficient number of Xs at end of template `%s'",
template);
}
if (tflag) {

Re: mktemp: Clarify error message

[Stuart Henderson]

On 2017/12/25 20:52, Klemens Nanni wrote:
> from mktemp(1):
> 
>   The template may be any filename with at least six ‘Xs’ appended
>   to it, for example /tmp/tfile.XX.
> 
> Now when a template contains but does not end in six Xs, the error
> message may imply errornous behaviour instead of bad usage:
> 
>   $ mktemp XX
>   oAQnQ5
>   $ mktemp XXs
>   mktemp: insufficient number of Xs in template `XXs'
> 
> I'd like to see a more precise error message here.
> 
> Feedback?
> 
> diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
> index 713b67fd105..c080d1d6474 100644
> --- a/usr.bin/mktemp/mktemp.c
> +++ b/usr.bin/mktemp/mktemp.c
> @@ -77,10 +77,9 @@ main(int argc, char *argv[])
>   }
>  
>   len = strlen(template);
> - if (len < 6 || strcmp([len - 6], "XX")) {
> - fatalx("insufficient number of Xs in template `%s'",
> - template);
> - }
> + if (len < 6 || strcmp([len - 6], "XX"))
> + fatalx("template must end in six or more Xs");
> +
>   if (tflag) {
>   if (strchr(template, '/')) {
>   fatalx("template must not contain directory "
> 

Printing the actual template used makes it easier to track down
the problematic call.

mktemp: Clarify error message

[Klemens Nanni]

from mktemp(1):

The template may be any filename with at least six ‘Xs’ appended
to it, for example /tmp/tfile.XX.

Now when a template contains but does not end in six Xs, the error
message may imply errornous behaviour instead of bad usage:

$ mktemp XX
oAQnQ5
$ mktemp XXs
mktemp: insufficient number of Xs in template `XXs'

I'd like to see a more precise error message here.

Feedback?

diff --git a/usr.bin/mktemp/mktemp.c b/usr.bin/mktemp/mktemp.c
index 713b67fd105..c080d1d6474 100644
--- a/usr.bin/mktemp/mktemp.c
+++ b/usr.bin/mktemp/mktemp.c
@@ -77,10 +77,9 @@ main(int argc, char *argv[])
}
 
len = strlen(template);
-   if (len < 6 || strcmp([len - 6], "XX")) {
-   fatalx("insufficient number of Xs in template `%s'",
-   template);
-   }
+   if (len < 6 || strcmp([len - 6], "XX"))
+   fatalx("template must end in six or more Xs");
+
if (tflag) {
if (strchr(template, '/')) {
fatalx("template must not contain directory "