Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

touch no-file/ fails with a sub optimal error #2639

Open
sylvestre opened this issue Sep 6, 2021 · 19 comments
Open

touch no-file/ fails with a sub optimal error #2639

sylvestre opened this issue Sep 6, 2021 · 19 comments
Labels

Comments

@sylvestre
Copy link
Sponsor Contributor

$ 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:

show!(e.map_err_context(|| format!("cannot touch '{}'", path.display())));

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

@tertsdiepraam
Copy link
Member

The current uutils behaviour is:

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

@sylvestre
Copy link
Sponsor Contributor Author

not great either :)

@tertsdiepraam tertsdiepraam added the good first issue For newcomers! label Jan 19, 2022
@goodhoko
Copy link

goodhoko commented Feb 9, 2022

May I give this a shot?

@tertsdiepraam
Copy link
Member

Yeah, go ahead!

@goodhoko
Copy link

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 #2408. The code that handled this was later removed in 0cfaaec#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).

@tertsdiepraam
Copy link
Member

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

@goodhoko
Copy link

@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?

@tertsdiepraam
Copy link
Member

tertsdiepraam commented Feb 10, 2022

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!

@goodhoko
Copy link

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.

@chipbuster
Copy link

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).

@tertsdiepraam
Copy link
Member

Thanks for investigating this!

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

Yes that is correct

@chipbuster
Copy link

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).

@tertsdiepraam
Copy link
Member

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?

@chipbuster
Copy link

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.

@goodhoko
Copy link

goodhoko commented Jul 8, 2022

(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.

@shanmukhateja
Copy link
Contributor

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.

@chipbuster
Copy link

@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

@TornaxO7
Copy link

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

@apatrushev
Copy link
Contributor

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)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

7 participants