coreutils icon indicating copy to clipboard operation
coreutils copied to clipboard

`touch no-file/` fails with a sub optimal error

Open sylvestre opened this issue 4 years ago • 18 comments

$ touch no-file/
touch: cannot touch 'no-file/': Other

GNU:

$ touch no-file/
touch: setting times of 'no-file/': No such file or directory

We should probably mix the two and have

touch: cannot touch 'no-file/': No such file or directory

The code is in: https://github.com/uutils/coreutils/blob/1bb23cdf73f22a10d534293cb36d8960930fbac3/src/uu/touch/src/touch.rs#L88

@miDeb Any idea how to implement that? thanks :)

sylvestre avatar Sep 06 '21 21:09 sylvestre

The current uutils behaviour is:

touch: cannot touch 'no-file/': Is a directory

tertsdiepraam avatar Jan 19 '22 10:01 tertsdiepraam

not great either :)

sylvestre avatar Jan 19 '22 11:01 sylvestre

May I give this a shot?

goodhoko avatar Feb 09 '22 18:02 goodhoko

Yeah, go ahead!

tertsdiepraam avatar Feb 09 '22 19:02 tertsdiepraam

I checked out main and ran cargo run touch no-file/ and got

touch: cannot touch 'no-file/': No such file or directory

It seems this issue has been already addressed in https://github.com/uutils/coreutils/pull/2408. The code that handled this was later removed in https://github.com/uutils/coreutils/commit/0cfaaeceda67947f98c938b69c6c7d2f2064cc83#diff-9565d8fad7b29837b3b468a61a809dc187fb351a72421ffe4b7c4879a8445b27R85 but the test from the PR is still in place and is passing.

@tertsdiepraam

The current uutils behaviour is: touch: cannot touch 'no-file/': Is a directory

How (platform, version of uutils) are you getting this behaviour? I couldn't reproduce it (macos, built from main).

goodhoko avatar Feb 10 '22 07:02 goodhoko

Maybe it depends on whether or not no-file/ exists as a directory? I.e. before and after mkdir no-file

tertsdiepraam avatar Feb 10 '22 08:02 tertsdiepraam

@tertsdiepraam I did try these scenarios and they all seem to behave as expected:

$ touch non-existent
# no output, creates a file named "non-existent"

$ touch non-existent/
touch: cannot touch 'non-existent/': No such file or directory

$ touch existing-dir
# no output, touches the dir

$ touch existing-dir/
# no output, touches the dir

these behaviors are consistent across my default touch implementation (BSD), GNU's and uutils'. Except the slight wording differences: BSD's leaves out the cannot touch part and GNU's replaces it with setting times of as described in the issue description.

Maybe it depends on whether or not no-file/ exists as a directory? I.e. before and after mkdir no-file

The case when no-file actually exists as directory doesn't and AFAIK shouldn't produce any error at all.

Anyway, the uutils' implementation seems to behave just this issue proposes. Am I missing something?

goodhoko avatar Feb 10 '22 08:02 goodhoko

Very curious, I'm still getting the same result, but maybe it's me who's missing something :) I'm on Linux and on the main branch.

❯ cargo run -- no-file/
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `/home/terts/Programming/coreutils/target/debug/touch no-file/`
/home/terts/Programming/coreutils/target/debug/touch: cannot touch 'no-file/': Is a directory

The case when no-file actually exists as directory doesn't and AFAIK shouldn't produce any error at all.

Oh yeah, that's true. Thanks for pointing that out!

tertsdiepraam avatar Feb 10 '22 08:02 tertsdiepraam

Very curious, I'm still getting the same result, but maybe it's me who's missing something :) I'm on Linux and on the main branch.

Curious indeed! Seems like maybe the platform is the culprit here. Gimme some more time to try on linux as well. And also to find out where exactly these messages originate.

goodhoko avatar Feb 11 '22 13:02 goodhoko

The issue appears to originate in this block. The Error returned by File::create is OS-dependent, and so is the message.

On Linux, printing the error gives Os { code: 21, kind: IsADirectory, message: "Is a directory" } On MacOS, it gives Os { code: 2, kind: NotFound, message: "No such file or directory" } on Windows, it gives Os { code: 123, kind: InvalidFilename, message: "The filename, directory name, or volume label syntax is incorrect." }

The last part of the error message on each platform appears to be just the message field of the Error (I'm guessing generated by map_err_context, but I'm not 100% sure).

chipbuster avatar May 23 '22 14:05 chipbuster

Thanks for investigating this!

(I'm guessing generated by map_err_context, but I'm not 100% sure).

Yes that is correct

tertsdiepraam avatar May 23 '22 15:05 tertsdiepraam

It seems to me that there might need to be some custom error message rules around this, since modifying the implementation of map_err_context feel like too broad of a change for this.

I'm thinking something like take the error from line 119 and then write a function that interprets it based on the OS and outputs the appropriate error message. Would that be an acceptable solution?

(Also, I'm not tied to writing this---if @goodhoko still wants to work on this, I would be happy to let them).

chipbuster avatar May 23 '22 19:05 chipbuster

I checked GNU to see what they do and where the discrepancy comes from. They do the following:

  1. Open (or create) the file (fd_reopen) and store the error into open_errno
  2. Set the time (fdutimensat) and store the error into utime_errno
  3. If utime_errno != 0 show the error
  4. Else if open_errno != 0 show the error

Hence, if a non-existent directory is given, the creation fails (with Is a directory on Linux) and then the setting of the time fails (with No such file or directory). That last error is then shown and the second ignored. I think this is quite strange behaviour and I might actually prefer Is a directory, though neither option is great. I like Is a directory because it gives the reason why touch hello succeeds, but touch hello/ fails if both don't exist. So maybe we should just leave it like it is?

I agree that we shouldn't change map_err_context for this change and only write a small workaround if we still want this. Modifying the error messages seems finicky, but might be a good option.

@sylvestre, what is your opinion here?

tertsdiepraam avatar May 23 '22 20:05 tertsdiepraam

Two quick things to note:

  • On Windows, the error message is going to be something like touch: cannot touch 'no-such-file/': The filename, directory name, or volume label syntax is incorrect., which seems pretty confusing.
  • On MacOS, the error is touch: cannot touch 'no-such-file/': No such file or directory, both if you try to touch a non-existent directory and if you try to touch a file in a non-existent directory (e.g. touch no-such-dir/ and touch no-such-dir/no-such-file). On Linux/Windows, the error message is different in these two cases.

I don't know to what extend cross-platform consistency in error messaging is a goal here, but to get MacOS to behave like the other two platforms would require an additional check upon first getting this error.

chipbuster avatar Jun 04 '22 02:06 chipbuster

(Also, I'm not tied to writing this---if @goodhoko still wants to work on this, I would be happy to let them).

Sorry for ghosting this. 🙈 Life dragged me somewhere else. Feel free to take over this.

goodhoko avatar Jul 08 '22 17:07 goodhoko

Hello, I would like to work on this.

@chipbuster Could you tell me what error message has to be shown for an OS? I was thinking of no such file or directory for Mac, Linux & BSD targets.

I'm not sure for Windows as their default error message is confusing.

shanmukhateja avatar Jul 16 '22 06:07 shanmukhateja

@chipbuster Could you tell me what error message has to be shown for an OS? I was thinking of no such file or directory for Mac, Linux & BSD targets.

Unfortunately, you're asking the wrong person. My only contribution to this project has been identifying the source of the error strings--my opinion on what the right message should be matters less than yours, if you end up implementing this, and much less than anyone with some actual involvement with this project

chipbuster avatar Jul 18 '22 18:07 chipbuster

Shouldn't this issue be closed due to this PR?

TornaxO7 avatar Aug 23 '22 22:08 TornaxO7

It is still there (linux, debian):

$ coreutils touch no-file/
touch: cannot touch 'no-file/': Is a directory
$ coreutils --help | grep multi
coreutils 0.0.25 (multi-call binary)

apatrushev avatar Mar 28 '24 21:03 apatrushev