Subject: Re: bug#15425: mktemp - conflicting options affected by ENV

From: Pádraig Brady <[email-masked]>
Date: Sat, 21 Sep 2013 03:44:05 +0100

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.
>From 53e0ce82ddd47e0d166742426c91d9b8473b8006 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?P=C3=A1draig=20Brady?= <[email-masked]>
Date: Sat, 21 Sep 2013 03:33:12 +0100
Subject: [PATCH] mktemp: disallow deprecated -t with --tempdir

* doc/coreutils.texi (mktemp invocation): Clarify that _all_ error
messages are suppressed with the -q option.
* src/mktemp.c (usage): Synchronize the -p option description with
the logic and info docs.  I.E. that -p is just an alias of --tmpdir.
(main): Disallow the deprecated -t option in combination with --tmpdir,
which would result in -t getting precedence.
* tests/misc/mktemp.pl: Add a test case.
* NEWS: Mention the change in behavior.
Fixes http://bugs.gnu.org/154245
---
 NEWS                 |    2 ++
 doc/coreutils.texi   |    5 +++--
 src/mktemp.c         |   21 +++++++++++----------
 tests/misc/mktemp.pl |    5 +++++
 4 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/NEWS b/NEWS
index d26722d..4855475 100644
--- a/NEWS
+++ b/NEWS
@@ -65,6 +65,8 @@ GNU coreutils NEWS                                    -*- outline -*-
   dd status=none now suppresses all non fatal diagnostic messages,
   not just the transfer counts.
 
+  mktemp now disallows using the deprecated -t option along with --tmpdir.
+
   stdbuf now requires at least one buffering mode option to be specified,
   as per the documented interface.
 
diff --git a/doc/coreutils.texi b/doc/coreutils.texi
index 21216b4..f2eba72 100644
--- a/doc/coreutils.texi
+++ b/doc/coreutils.texi
@@ -13421,8 +13421,9 @@ umask is more restrictive.
 @itemx --quiet
 @opindex -q
 @opindex --quiet
-Suppress diagnostics about failure to create a file or directory.  The
-exit status will still reflect whether a file was created.
+Suppress all error messages, including command option syntax errors,
+and failure to create a file or directory.
+The exit status will still reflect whether a file was created.
 
 @item -u
 @itemx --dry-run
diff --git a/src/mktemp.c b/src/mktemp.c
index 44845c3..bb0e8c1 100644
--- a/src/mktemp.c
+++ b/src/mktemp.c
@@ -43,7 +43,6 @@ static const char *default_template = "tmp.XXXXXXXXXX";
 enum
 {
   SUFFIX_OPTION = CHAR_MAX + 1,
-  TMPDIR_OPTION
 };
 
 static struct option const longopts[] =
@@ -52,7 +51,7 @@ static struct option const longopts[] =
   {"quiet", no_argument, NULL, 'q'},
   {"dry-run", no_argument, NULL, 'u'},
   {"suffix", required_argument, NULL, SUFFIX_OPTION},
-  {"tmpdir", optional_argument, NULL, TMPDIR_OPTION},
+  {"tmpdir", optional_argument, NULL, 'p'},
   {GETOPT_HELP_OPTION_DECL},
   {GETOPT_VERSION_OPTION_DECL},
   {NULL, 0, NULL, 0}
@@ -78,14 +77,14 @@ Files are created u+rw, and directories u+rwx, minus umask restrictions.\n\
       fputs (_("\
   -d, --directory     create a directory, not a file\n\
   -u, --dry-run       do not create anything; merely print a name (unsafe)\n\
-  -q, --quiet         suppress diagnostics about file/dir-creation failure\n\
+  -q, --quiet         suppress all error messages\n\
 "), stdout);
       fputs (_("\
       --suffix=SUFF   append SUFF to TEMPLATE; SUFF must not contain a slash.\n\
                         This option is implied if TEMPLATE does not end in X\n\
 "), stdout);
       fputs (_("\
-      --tmpdir[=DIR]  interpret TEMPLATE relative to DIR; if DIR is not\n\
+  -p DIR, --tmpdir[=DIR]  interpret TEMPLATE relative to DIR; if DIR is not\n\
                         specified, use $TMPDIR if set, else /tmp.  With\n\
                         this option, TEMPLATE must not be an absolute name;\n\
                         unlike with -t, TEMPLATE may contain slashes, but\n\
@@ -93,7 +92,6 @@ Files are created u+rw, and directories u+rwx, minus umask restrictions.\n\
 "), stdout);
       fputs ("\n", stdout);
       fputs (_("\
-  -p DIR              use DIR as a prefix; implies -t [deprecated]\n\
   -t                  interpret TEMPLATE as a single file name component,\n\
                         relative to a directory: $TMPDIR, if set; else the\n\
                         directory specified via -p; else /tmp [deprecated]\n\
@@ -158,6 +156,7 @@ main (int argc, char **argv)
   char *suffix = NULL;
   bool use_dest_dir = false;
   bool deprecated_t_option = false;
+  bool tmpdir_option = false;
   bool create_directory = false;
   bool dry_run = false;
   int status = EXIT_SUCCESS;
@@ -183,6 +182,7 @@ main (int argc, char **argv)
         case 'p':
           dest_dir_arg = optarg;
           use_dest_dir = true;
+          tmpdir_option = true;
           break;
         case 'q':
           suppress_stderr = true;
@@ -195,11 +195,6 @@ main (int argc, char **argv)
           dry_run = true;
           break;
 
-        case TMPDIR_OPTION:
-          use_dest_dir = true;
-          dest_dir_arg = optarg;
-          break;
-
         case SUFFIX_OPTION:
           suffix = optarg;
           break;
@@ -223,6 +218,12 @@ main (int argc, char **argv)
                _("failed to redirect stderr to /dev/null"));
     }
 
+  if (deprecated_t_option && tmpdir_option)
+    {
+      error (0, 0, _("the deprecated -t option is not allowed with --tmpdir"));
+      usage (EXIT_FAILURE);
+    }
+
   n_args = argc - optind;
   if (2 <= n_args)
     {
diff --git a/tests/misc/mktemp.pl b/tests/misc/mktemp.pl
index b15b669..f9baac3 100755
--- a/tests/misc/mktemp.pl
+++ b/tests/misc/mktemp.pl
@@ -64,6 +64,11 @@ my @Tests =
       {ERR=>"$prog: too few X's in template 'foo.XX'\n"}],
      ['too-few-xq', '-q foo.XX', {EXIT => 1} ],
 
+     ['t-and-tmpdir', '-t --tmpdir',
+      {ERR=>"$prog: the deprecated -t option is not allowed with --tmpdir\n"
+       . "Try '$prog --help' for more information.\n"}, {EXIT => 1} ],
+     ['t-and-tmpdir-q', '-q -t --tmpdir', {EXIT => 1} ],
+
      ['1f', 'bar.XXXX', {OUT => "bar.ZZZZ\n"},
       {OUT_SUBST => 's,\.....$,.ZZZZ,'},
       {POST => sub { my ($f) = @_; defined $f or return; chomp $f;
-- 
1.7.7.6