GNU bug report logs - #15425
mktemp - conflicting options affected by ENV

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 15425 in the body.
You can then email your comments to 15425 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Assaf Gordon <assafgordon <at> gmail.com>
To: bug-coreutils <at> gnu.org
Subject: mktemp - conflicting options affected by ENV
Date: Fri, 20 Sep 2013 10:50:25 -0600

Hello,

Not sure if this is a bug or just unintended side-effect, but it caused some scripts here to fail and took a while to troubleshoot:
It basically boils down to the interplay between the deprecated "-t" "-p" and "$TMPDIR" and "--tmpdir".

===
##
## Problem 1: "-t" and TMPDIR (if non-empty) overrides "--tmpdir"
##
$ TMPDIR= mktemp -u --tmpdir=. -t XXXXXX
./og9G5s
$ TMPDIR=/foo/ mktemp -u --tmpdir=. -t XXXXXX
/foo/AAWcOl

##
## Problem 2: if TMPDIR is empty, "--tmpdir" overrides "-p", despite having "-t"
##
$ TMPDIR=/foo/ mktemp -u -p /bar/ --tmpdir=. -t XXXXXX
/foo/tHXcrq
$ TMPDIR= mktemp -u -p /bar/ --tmpdir=. -t XXXXXX
./OfWXSS
## I'd expect the above to use "/bar/OfWXSS", to be consistent with TMPDIR overriding "--tmpdir".
===

I realize "-t" and "-p" are deprecated, and it's best not to meddle with them -
but for consistency, perhaps consider having the new "--tmpdir" always take precedence over "-t" and "-p" ?
or print a warning to STDERR when "-t" and "--tmpdir" are mixed?

Regards,
 -gordon

Message #8 received at 15425 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Assaf Gordon <assafgordon <at> gmail.com>
Cc: 15425 <at> debbugs.gnu.org
Subject: Re: bug#15425: mktemp - conflicting options affected by ENV
Date: Sat, 21 Sep 2013 01:14:23 +0100

On 09/20/2013 05:50 PM, Assaf Gordon wrote:
> Hello,
> 
> Not sure if this is a bug or just unintended side-effect, but it caused some scripts here to fail and took a while to troubleshoot:
> It basically boils down to the interplay between the deprecated "-t" "-p" and "$TMPDIR" and "--tmpdir".
> 
> ===
> ##
> ## Problem 1: "-t" and TMPDIR (if non-empty) overrides "--tmpdir"
> ##
> $ TMPDIR= mktemp -u --tmpdir=. -t XXXXXX
> ./og9G5s
> $ TMPDIR=/foo/ mktemp -u --tmpdir=. -t XXXXXX
> /foo/AAWcOl

Yes this is confusing, and _not_ order dependent:

$ TMPDIR=/foo/ mktemp -t -u --tmpdir=. XXXXXX
/foo/AAWcOl

> 
> ##
> ## Problem 2: if TMPDIR is empty, "--tmpdir" overrides "-p", despite having "-t"
> ##
> $ TMPDIR=/foo/ mktemp -u -p /bar/ --tmpdir=. -t XXXXXX
> /foo/tHXcrq
> $ TMPDIR= mktemp -u -p /bar/ --tmpdir=. -t XXXXXX
> ./OfWXSS
> ## I'd expect the above to use "/bar/OfWXSS", to be consistent with TMPDIR overriding "--tmpdir".
> ===

Also confusing, and order dependent too:

$ mktemp -p /aaa -u --tmpdir=. asdf/XXXX
./asdf/wdIT
$ mktemp -u --tmpdir=. -p /aaa asdf/XXXX
/aaa/asdf/3Jc7

> I realize "-t" and "-p" are deprecated, and it's best not to meddle with them -
> but for consistency, perhaps consider having the new "--tmpdir" always take precedence over "-t" and "-p" ?
> or print a warning to STDERR when "-t" and "--tmpdir" are mixed?

Yes I agree that --tmpdir should override -pt no matter what the order.
In fact there is no reason to use -pt with --tmpdir and since the former
are deprecated, I suggest we just disallow that combination with an error.

Patch coming up.

thanks!
Pádraig.

Message #11 received at 15425 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Assaf Gordon <assafgordon <at> gmail.com>
Cc: 15425 <at> debbugs.gnu.org
Subject: Re: bug#15425: mktemp - conflicting options affected by ENV
Date: Sat, 21 Sep 2013 01:40:27 +0100

On 09/21/2013 01:14 AM, Pádraig Brady wrote:
> On 09/20/2013 05:50 PM, Assaf Gordon wrote:
>> Hello,
>>
>> Not sure if this is a bug or just unintended side-effect, but it caused some scripts here to fail and took a while to troubleshoot:
>> It basically boils down to the interplay between the deprecated "-t" "-p" and "$TMPDIR" and "--tmpdir".
>>
>> ===
>> ##
>> ## Problem 1: "-t" and TMPDIR (if non-empty) overrides "--tmpdir"
>> ##
>> $ TMPDIR= mktemp -u --tmpdir=. -t XXXXXX
>> ./og9G5s
>> $ TMPDIR=/foo/ mktemp -u --tmpdir=. -t XXXXXX
>> /foo/AAWcOl
> 
> Yes this is confusing, and _not_ order dependent:
> 
> $ TMPDIR=/foo/ mktemp -t -u --tmpdir=. XXXXXX
> /foo/AAWcOl
> 
>>
>> ##
>> ## Problem 2: if TMPDIR is empty, "--tmpdir" overrides "-p", despite having "-t"
>> ##
>> $ TMPDIR=/foo/ mktemp -u -p /bar/ --tmpdir=. -t XXXXXX
>> /foo/tHXcrq
>> $ TMPDIR= mktemp -u -p /bar/ --tmpdir=. -t XXXXXX
>> ./OfWXSS
>> ## I'd expect the above to use "/bar/OfWXSS", to be consistent with TMPDIR overriding "--tmpdir".
>> ===
> 
> Also confusing, and order dependent too:
> 
> $ mktemp -p /aaa -u --tmpdir=. asdf/XXXX
> ./asdf/wdIT
> $ mktemp -u --tmpdir=. -p /aaa asdf/XXXX
> /aaa/asdf/3Jc7
> 
>> I realize "-t" and "-p" are deprecated, and it's best not to meddle with them -
>> but for consistency, perhaps consider having the new "--tmpdir" always take precedence over "-t" and "-p" ?
>> or print a warning to STDERR when "-t" and "--tmpdir" are mixed?
> 
> Yes I agree that --tmpdir should override -pt no matter what the order.
> In fact there is no reason to use -pt with --tmpdir and since the former
> are deprecated, I suggest we just disallow that combination with an error.
> 
> Patch coming up.

I also see the --help says that -p implies -t
AFAICS -t in the context of -p just means to interpret template as a single file without dirs,
however the example below shows that -t is not in fact implied with -p

$ mktemp -u -p dir1 dir2/XXXXXX
dir1/dir2/qQBdsf
$ mktemp -u -t -p dir1 dir2/XXXXXX
src/mktemp: invalid template, ‘dir2/XXXXXX’, contains directory separator

So rather than adjusting any functionality of these deprecated options,
I'll just remove the --help text saying that -t is implied.

thanks,
Pádraig.

Message #16 received at 15425-done <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Assaf Gordon <assafgordon <at> gmail.com>
Cc: 15425-done <at> debbugs.gnu.org
Subject: Re: bug#15425: mktemp - conflicting options affected by ENV
Date: Sat, 21 Sep 2013 03:44:05 +0100

[Message part 1 (text/plain, inline)]

On 09/21/2013 01:40 AM, Pádraig Brady wrote:
> On 09/21/2013 01:14 AM, Pádraig Brady wrote:
>> On 09/20/2013 05:50 PM, Assaf Gordon wrote:
>>> Hello,
>>>
>>> Not sure if this is a bug or just unintended side-effect, but it caused some scripts here to fail and took a while to troubleshoot:
>>> It basically boils down to the interplay between the deprecated "-t" "-p" and "$TMPDIR" and "--tmpdir".
>>>
>>> ===
>>> ##
>>> ## Problem 1: "-t" and TMPDIR (if non-empty) overrides "--tmpdir"
>>> ##
>>> $ TMPDIR= mktemp -u --tmpdir=. -t XXXXXX
>>> ./og9G5s
>>> $ TMPDIR=/foo/ mktemp -u --tmpdir=. -t XXXXXX
>>> /foo/AAWcOl
>>
>> Yes this is confusing, and _not_ order dependent:
>>
>> $ TMPDIR=/foo/ mktemp -t -u --tmpdir=. XXXXXX
>> /foo/AAWcOl
>>
>>>
>>> ##
>>> ## Problem 2: if TMPDIR is empty, "--tmpdir" overrides "-p", despite having "-t"
>>> ##
>>> $ TMPDIR=/foo/ mktemp -u -p /bar/ --tmpdir=. -t XXXXXX
>>> /foo/tHXcrq
>>> $ TMPDIR= mktemp -u -p /bar/ --tmpdir=. -t XXXXXX
>>> ./OfWXSS
>>> ## I'd expect the above to use "/bar/OfWXSS", to be consistent with TMPDIR overriding "--tmpdir".
>>> ===
>>
>> Also confusing, and order dependent too:
>>
>> $ mktemp -p /aaa -u --tmpdir=. asdf/XXXX
>> ./asdf/wdIT
>> $ mktemp -u --tmpdir=. -p /aaa asdf/XXXX
>> /aaa/asdf/3Jc7
>>
>>> I realize "-t" and "-p" are deprecated, and it's best not to meddle with them -
>>> but for consistency, perhaps consider having the new "--tmpdir" always take precedence over "-t" and "-p" ?
>>> or print a warning to STDERR when "-t" and "--tmpdir" are mixed?
>>
>> Yes I agree that --tmpdir should override -pt no matter what the order.
>> In fact there is no reason to use -pt with --tmpdir and since the former
>> are deprecated, I suggest we just disallow that combination with an error.
>>
>> Patch coming up.
> 
> I also see the --help says that -p implies -t
> AFAICS -t in the context of -p just means to interpret template as a single file without dirs,
> however the example below shows that -t is not in fact implied with -p
> 
> $ mktemp -u -p dir1 dir2/XXXXXX
> dir1/dir2/qQBdsf
> $ mktemp -u -t -p dir1 dir2/XXXXXX
> src/mktemp: invalid template, ‘dir2/XXXXXX’, contains directory separator
> 
> So rather than adjusting any functionality of these deprecated options,
> I'll just remove the --help text saying that -t is implied.

Which would make -p = --tmpdir, which is what the code has done
since being introduced to coreutils 6 years ago, and what the
texinfo has documented for the last 4 years.  So I've just aligned
the --help to say -p is the short form of --tmpdir.

Hopefully the attached addresses all these issues.

thanks,
Pádraig.

[mktemp-pt.patch (text/x-patch, attachment)]

Message #19 received at 15425 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: 15425 <at> debbugs.gnu.org, assafgordon <at> gmail.com, 
 Eric Blake <eblake <at> redhat.com>
Subject: Re: bug#15425: mktemp - conflicting options affected by ENV
Date: Wed, 25 Sep 2013 10:32:32 +0100

On 09/21/2013 03:44 AM, Pádraig Brady wrote:
> On 09/21/2013 01:40 AM, Pádraig Brady wrote:
>> On 09/21/2013 01:14 AM, Pádraig Brady wrote:
>>> On 09/20/2013 05:50 PM, Assaf Gordon wrote:
>>>> Hello,
>>>>
>>>> Not sure if this is a bug or just unintended side-effect, but it caused some scripts here to fail and took a while to troubleshoot:
>>>> It basically boils down to the interplay between the deprecated "-t" "-p" and "$TMPDIR" and "--tmpdir".
>>>>
>>>> ===
>>>> ##
>>>> ## Problem 1: "-t" and TMPDIR (if non-empty) overrides "--tmpdir"
>>>> ##
>>>> $ TMPDIR= mktemp -u --tmpdir=. -t XXXXXX
>>>> ./og9G5s
>>>> $ TMPDIR=/foo/ mktemp -u --tmpdir=. -t XXXXXX
>>>> /foo/AAWcOl
>>>
>>> Yes this is confusing, and _not_ order dependent:
>>>
>>> $ TMPDIR=/foo/ mktemp -t -u --tmpdir=. XXXXXX
>>> /foo/AAWcOl
>>>
>>>>
>>>> ##
>>>> ## Problem 2: if TMPDIR is empty, "--tmpdir" overrides "-p", despite having "-t"
>>>> ##
>>>> $ TMPDIR=/foo/ mktemp -u -p /bar/ --tmpdir=. -t XXXXXX
>>>> /foo/tHXcrq
>>>> $ TMPDIR= mktemp -u -p /bar/ --tmpdir=. -t XXXXXX
>>>> ./OfWXSS
>>>> ## I'd expect the above to use "/bar/OfWXSS", to be consistent with TMPDIR overriding "--tmpdir".
>>>> ===
>>>
>>> Also confusing, and order dependent too:
>>>
>>> $ mktemp -p /aaa -u --tmpdir=. asdf/XXXX
>>> ./asdf/wdIT
>>> $ mktemp -u --tmpdir=. -p /aaa asdf/XXXX
>>> /aaa/asdf/3Jc7
>>>
>>>> I realize "-t" and "-p" are deprecated, and it's best not to meddle with them -
>>>> but for consistency, perhaps consider having the new "--tmpdir" always take precedence over "-t" and "-p" ?
>>>> or print a warning to STDERR when "-t" and "--tmpdir" are mixed?
>>>
>>> Yes I agree that --tmpdir should override -pt no matter what the order.
>>> In fact there is no reason to use -pt with --tmpdir and since the former
>>> are deprecated, I suggest we just disallow that combination with an error.
>>>
>>> Patch coming up.
>>
>> I also see the --help says that -p implies -t
>> AFAICS -t in the context of -p just means to interpret template as a single file without dirs,
>> however the example below shows that -t is not in fact implied with -p
>>
>> $ mktemp -u -p dir1 dir2/XXXXXX
>> dir1/dir2/qQBdsf
>> $ mktemp -u -t -p dir1 dir2/XXXXXX
>> src/mktemp: invalid template, ‘dir2/XXXXXX’, contains directory separator
>>
>> So rather than adjusting any functionality of these deprecated options,
>> I'll just remove the --help text saying that -t is implied.
> 
> Which would make -p = --tmpdir, which is what the code has done
> since being introduced to coreutils 6 years ago, and what the
> texinfo has documented for the last 4 years.  So I've just aligned
> the --help to say -p is the short form of --tmpdir.
> 
> Hopefully the attached addresses all these issues.

I'll push this soon, unless there are objections
to the above "undeprecation" of -p.

thanks,
Pádraig.

Message #22 received at 15425 <at> debbugs.gnu.org (full text, mbox):

From: Jim Meyering <jim <at> meyering.net>
To: Pádraig Brady <P <at> draigbrady.com>
Cc: 15425 <at> debbugs.gnu.org, assafgordon <at> gmail.com,
 Eric Blake <eblake <at> redhat.com>
Subject: Re: bug#15425: mktemp - conflicting options affected by ENV
Date: Wed, 25 Sep 2013 21:36:27 -0700

> I'll push this soon, unless there are objections
> to the above "undeprecation" of -p.

Go for it. That seems best.  Thank you.

Message #25 received at 15425 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: Jim Meyering <jim <at> meyering.net>
Cc: 15425 <at> debbugs.gnu.org, assafgordon <at> gmail.com,
 Eric Blake <eblake <at> redhat.com>
Subject: Re: bug#15425: mktemp - conflicting options affected by ENV
Date: Thu, 26 Sep 2013 13:59:54 +0100

[Message part 1 (text/plain, inline)]

On 09/26/2013 05:36 AM, Jim Meyering wrote:
>> I'll push this soon, unless there are objections
>> to the above "undeprecation" of -p.
> 
> Go for it. That seems best.  Thank you.

That -p = --tmpdir aspect is OK.

But on consideration, erroring on -p,-t is too harsh.
Really -t is just a mode to operate in,
i.e. enforce no '/' in template, and give precedence
to TMPDIR over --tmpdir
Hopefully the attached documentation only patch
suffices to clear things up.

BTW I noticed this variation which could be
used to generate passwords or something:

 $ mktemp -u -t -p '' XXXXXXXX
 L5awccB1

However without -t, '/tmp/' is inserted.
I'm inclined to change to not output '/tmp/' here?

 $ mktemp -u -p '' XXXXXXXX
 /tmp/L5awccB1

thanks,
Pádraig.

[mktemp-pt.patch (text/x-patch, attachment)]

Message #28 received at 15425 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: 15425 <at> debbugs.gnu.org
Subject: Re: bug#15425: mktemp - conflicting options affected by ENV
Date: Sun, 06 Oct 2013 18:10:06 +0100

[Message part 1 (text/plain, inline)]

On 09/26/2013 01:59 PM, Pádraig Brady wrote:
> On 09/26/2013 05:36 AM, Jim Meyering wrote:
>>> I'll push this soon, unless there are objections
>>> to the above "undeprecation" of -p.
>>
>> Go for it. That seems best.  Thank you.
> 
> That -p = --tmpdir aspect is OK.
> 
> But on consideration, erroring on -p,-t is too harsh.
> Really -t is just a mode to operate in,
> i.e. enforce no '/' in template, and give precedence
> to TMPDIR over --tmpdir
> Hopefully the attached documentation only patch
> suffices to clear things up.
> 
> BTW I noticed this variation which could be
> used to generate passwords or something:
> 
>  $ mktemp -u -t -p '' XXXXXXXX
>  L5awccB1
> 
> However without -t, '/tmp/' is inserted.
> I'm inclined to change to not output '/tmp/' here?
> 
>  $ mktemp -u -p '' XXXXXXXX
>  /tmp/L5awccB1

Actually the best way to generate random chars like the above
is to avoid -p entirely. So I've made -t consistent and
output /tmp/... in the above case.

Also I matched the -q option up with the documentation.
The documented functionality made sense, which was to
suppress only I/O errors, but the existing code suppressed
almost all errors, including most usage errors.
So I fixed that up in a second patch.

Both updated patches are attached,
and already pushed.

thanks,
Pádraig.

[mktemp-match-docs.patch (text/x-patch, attachment)]

Message #31 received at 15425 <at> debbugs.gnu.org (full text, mbox):

From: Pádraig Brady <P <at> draigBrady.com>
To: 15425 <at> debbugs.gnu.org
Subject: Re: bug#15425: mktemp - conflicting options affected by ENV
Date: Mon, 07 Oct 2013 12:22:30 +0100

On 10/06/2013 06:10 PM, Pádraig Brady wrote:
> Also I matched the -q option up with the documentation.
> The documented functionality made sense, which was to
> suppress only I/O errors, but the existing code suppressed
> almost all errors, including most usage errors.
> So I fixed that up in a second patch.

I was caught out by inconsistent exit() handling in this area,
which required this follow up commit.

commit cba81e7b84f27385e254cddd065bb16c98a5d045
Author: Pádraig Brady <P <at> draigBrady.com>
Date:   Mon Oct 7 11:59:53 2013 +0100

    mktemp: fix incorrect exit status from previous commit

    * src/mktemp.c (main): Use an exit() strategy consistent with the
    previous clauses dealing with optional error messages to ensure
    we exit with the correct status in all cases.
    Prompted by the continuous integration build failure at:
    http://hydra.nixos.org/build/6412979

diff --git a/src/mktemp.c b/src/mktemp.c
index 05530a3..074676f 100644
--- a/src/mktemp.c
+++ b/src/mktemp.c
@@ -335,7 +335,8 @@ main (int argc, char **argv)
           int saved_errno = errno;
           remove (dest_name);
           if (!suppress_file_err)
-            error (EXIT_FAILURE, saved_errno, _("write error"));
+            error (0, saved_errno, _("write error"));
+          status = EXIT_FAILURE;
         }
     }

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.