mktemp -t foo.XXXX should create in TMPDIR #4832
Conversation
There was a problem hiding this comment.
I think the PR is incomplete because mktemp -t -p ~/projects/playground foo.XXXX doesn't work as expected if $TMPDIR is not set: it creates the file in /tmp instead of ~/projects/playground.
From the GNU mktemp documentation:
-tinterpret TEMPLATE as a single file name component,
relative to a directory: $TMPDIR, if set; else the
directory specified via -p; else /tmp [deprecated]
|
GNU testsuite comparison: |
|
sure but afaik, i didn't regress this issue :) |
|
Ah no, it's not a regression, I just didn't test this variant in my first review :) |
|
GNU testsuite comparison: |
|
GNU testsuite comparison: |
src/uu/mktemp/src/mktemp.rs
Outdated
| // Ignore TMPDIR when it's set to "." | ||
| if tmpdir_env == "." { |
There was a problem hiding this comment.
Why do you ignore .? GNU mktemp accepts it:
$ TMPDIR=. mktemp -t -p dir_b foo.XXXX
./foo.6Nai
src/uu/mktemp/src/mktemp.rs
Outdated
| @@ -191,7 +191,22 @@ impl Options { | |||
| (tmpdir, template.to_string()) | |||
| } | |||
| Some(template) => { | |||
| let tmpdir = matches.get_one::<String>(OPT_TMPDIR).map(String::from); | |||
| let tmpdir = if let Ok(tmpdir_env) = env::var("TMPDIR") { | |||
There was a problem hiding this comment.
I think you need an additional condition here. It currently breaks the GNU tests because GNU mktemp ignores TMPDIR in the following case:
$ TMPDIR=dir_a mktemp foo.XXXX
foo.fgPS
|
this is why i would have prefer to land the previous commit as i didn't introduce a regression ;) |
|
I removed the last patch of the stack It doesn't fix |
|
Sorry, I misunderstood what you meant with "regression" :| |
|
GNU testsuite comparison: |
|
yeah, sorry :) i use that word every day at Mozilla :) By regression, i meant that this current PR is still an improvement over the current main. Even if it isn't perfect as you noticed :) |
closes: #4821