Skip to content

Add path mtime #9057

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

Merged
merged 7 commits into from
Jul 18, 2022
Merged

Add path mtime #9057

merged 7 commits into from
Jul 18, 2022

Conversation

faho
Copy link
Member

@faho faho commented Jul 7, 2022

This can be used to print the modification time, like stat with some
options.

The reason is that stat has caused us a number of portability
headaches:

  1. It's not available everywhere by default
  2. The versions are quite different

For instance, with GNU stat it's stat -c '%Y', with macOS it's stat -f %m.

With the --relative option, it prints the seconds since the modification time instead, so

path mtime -R $cache_file

is a direct replacement for

math (date +%s) - (path mtime -R $cache_file)

So now checking a cache file can be done just with builtins.

TODOs:

  • Changes to fish usage are reflected in user documentation/manpages.
  • Tests have been added
  • User-visible changes noted in CHANGELOG.rst

Some questions:

  • Is --relative an okay name? I chose it because it's not specific to mtime and might be usable for other commands as well
  • Do we need to explain that we pick the current time once?
  • Is the --quiet behavior okay? It returns 0 if there's a >0 mtime, but I don't know if that really happens?
  • What about portability?
  • An alternative I looked at was giving a way to get the current time as epoch-seconds, like a $epoch variable and then allowing people to do the math. I don't think that's suuuper useful
  • Is mtime the right command granularity or should we have a path info subcommand instead? I think subcommands are cheap and mtime is an important thing - the next one is probably path size

@faho faho added this to the fish 3.6.0 milestone Jul 7, 2022
@faho faho force-pushed the tips-fedora-mtime branch from 22df082 to c4fbf10 Compare July 7, 2022 20:08
@floam
Copy link
Member

floam commented Jul 8, 2022

This reminded me of #3589; I started a draft PR implementing that.

@zx8
Copy link

zx8 commented Jul 9, 2022

I think path info makes sense - what about atime and ctime? I feel it’s a bit clunky to add them as first-level subcommands; grouping them under info feels like the natural choice here.

@faho
Copy link
Member Author

faho commented Jul 9, 2022

I think path info makes sense - what about atime and ctime?

Both are much less useful and might not even be a thing - atime is often disabled.

So I don't want to expose them. If you know they exist and are useful on that specific system, you can use that system's tools.

@krobelus
Copy link
Contributor

krobelus commented Jul 9, 2022

  • Is --relative an okay name? I chose it because it's not specific to mtime and might be usable for other commands as well

I think it's good. An alternative is path mtime --elapsed but that doesn't parse.

  • Do we need to explain that we pick the current time once?

You mean as opposed to once for every argument? I think that's what I'd expect anyway

  • Is the --quiet behavior okay? It returns 0 if there's a >0 mtime, but I don't know if that really happens?

I expect an error exit code if, and only if wstat() failed for any one of the input paths.
(If we're being nice we can also print a message to stderr).

  • Is mtime the right command granularity or should we have a path info subcommand instead? I think subcommands are cheap and mtime is an important thing - the next one is probably path size

Not sure. I think path mtime is fine. If we plan to add more than 10 then we should definitely consider path info

@floam
Copy link
Member

floam commented Jul 9, 2022

Is the --quiet behavior okay? It returns 0 if there's a >0 mtime, but I don't know if that really happens?

An mtime of 0 is December 31, 1969 - less than zero is before that, so that seems kind of arbitrary. Albeit specifically 0 mtime is an annoying thing you run into on unix for myriad reasons that doesn't really have anything to do with the 70s and might be something you want to check for/against.

ctime, birth time do seem kind of useful potentially to me but I think @faho Is probably right, and besides, nothing this PR does prevents anybody from using stat(1) and the scope of builtin path isn't to be a full on stat builtin/replacement. I'd not be a fan of hiding mtime behind info even if there were also crime and/or atime.

Copy link
Member

@floam floam left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See how it's done in wutil.cpp: I don't think we really need new cmake checks.

@floam
Copy link
Member

floam commented Jul 9, 2022

I'd like this to incorporate the nsec component if we've got one.

@faho
Copy link
Member Author

faho commented Jul 9, 2022

I don't want to print anything we can't get in a cross-platform manner.

It's awkward to sometimes get a decimal point and sometimes not.

@floam
Copy link
Member

floam commented Jul 9, 2022

I'd just print it as a float regardless to avoid the confusion or the potential that someone treats it as an integer in the script; seeing1657401612.0 if the filesystem/OS can't do better seems safe.

What are the platforms/filesystems where we never can get those fields?

There's a big class of annoying things that happen when a file gets modified or whatever within the same second that something is checked and it's frustrating. Like, the integer timestamps in our history files have even had annoying consequences.

Since floating point math is so convenient in fish, it seems like a great spot not to go lowest-common-denominator.

@floam
Copy link
Member

floam commented Jul 9, 2022

Some research suggests that ext4 on Linux doesn't typically have like, actual nanosecond time granularity, but supposedly something like 0.01s is evidenced - that's fine in my book for mtime comparisons in a shellscript. Beats 1s.

@faho
Copy link
Member Author

faho commented Jul 9, 2022

Subsecond resolution is very very rarely going to matter, but figuring out if it is supported is going to be annoying for the user, and displaying a "." just makes it look worse. Let's just leave it out.

@floam
Copy link
Member

floam commented Jul 9, 2022

You would be displaying it here, but only sometimes.

Why? I was proposing just forcing printf to show the decimal point. This isn't for "display" anyhow really - it's going to be used in scripts - where it is to be displayed,, it ought to get formatted into an actual human-readable date format using date or something.

I think you're imagining this will only be used for that usage case of testing if a cache file is a week old.

@faho
Copy link
Member Author

faho commented Jul 9, 2022

Why? I was proposing just forcing printf to show the decimal point.

I missed that you had commented twice. So I deleted the comment and wrote a new one.

I think you're imagining this will only be used for that usage case of testing if a cache file is a week old.

That is the usecase, yes.

Note that the stat calls we used for this displayed seconds, and I don't believe that was an issue, or that subseconds would have helped in any way.

@floam
Copy link
Member

floam commented Jul 9, 2022

Maybe it'd be nicer to just accommodate that use case directly then, with UI closer to the thing find -mtime does where you're asking for files newer/older than n days (hours?).

@faho
Copy link
Member Author

faho commented Jul 10, 2022

That requires some form of parsing.

It is possible to add later, for now I would like to do the simple, targeted addition.

@krobelus
Copy link
Contributor

I would add the decimal point always. Is there any scenario that works better with integers?

@faho
Copy link
Member Author

faho commented Jul 10, 2022

Okay, let me expand on my idea:

Subsecond resolution is rarely helpful for this tool. That's because this is made to answer questions like "is this file older than a week", "a minute", "a day".

You can even use it to say "was this file modified after 08:30 last wednesday", but of course converting that to unix time is up to you, currently (some fiddling with date).

But for that, subsecond resolution isn't helpful. "Was this modified between 22:29:54.2 and 22:29:54.8" isn't a question someone has.

So subsecond resolution isn't useful, and adding the decimal point makes it look like we can do subsecond resolution when often we can't - systems that don't have st_mtim don't give us subsecond resolution, filesystems might not be able to (vfat can't), ...

And having a decimal point here makes it strictly harder to interoperate with other tools - does your date accept it? Does your find? stat?

My find, from GNU findutils, doesn't:

> find . -mtime +5.5
find: Ungültiges Argument +5.5 für »-mtime«.
# (that's "Invalid argument")

(because it expects the arguments to be localized - I would have to pass 5,5 with my de_DE.UTF-8 locale)

Subsecond resolution also exacerbates the problem with --relative and when we get the current time:

path mtime --relative foo bar baz

Having subsecond timestamps here is basically just a measure of how long path mtime takes.

So: In summary:

  1. It's not useful
  2. It looks like it promises it'll always be there when it's not
  3. It leads to issues with other tools
  4. It's awkward with --relative

The question that people do have is "is this file older than this other file", but that's #9058.

@floam
Copy link
Member

floam commented Jul 10, 2022

So subsecond resolution isn't useful

You're really limiting the scope of this new builtin to specific ways you plan to use this builtin, just throughout stuff like our completions.

Where things start to fall apart are where one starts getting the same mtime for different files that were not all created atomically at the same moment, or deltas between "now" and an mtime seem to return 0. The person who is doing something you did not think of will necessarily be the kind of person who will wish you had started this off at full precision.

It is possible to add later, for now I would like to do the simple, targeted addition.

It's not easy to add later - unless we will be just appending ".0" to everything we return from day 1, it may end up like our history timestamps where it's hard to change the output format later.

What's the rush?

@faho
Copy link
Member Author

faho commented Jul 10, 2022

Where things start to fall apart are where one starts getting the same mtime for different files that were not all created atomically at the same moment, or deltas between "now" and an mtime seem to return 0.

That looks like you would

  1. create/modify a file
  2. immediately ask how long ago it was modified

which... does not seem useful to me (and smells suspiciously like you want a polling mechanism instead - see inotifywait).

And even then, it would only do anything if your OS/filesystem combination offers subsecond mtime resolution, and would mysteriously fail otherwise.

What's the rush?

The "rush" is that I don't want to add subsecond resolution mtime because I do not believe it useful, and because I believe it has annoying limitations (not working everywhere, not working together with other applications, but only sometimes, ...), and because I believe adding ".0" sends the wrong signal.

That's the perfect misfeature - something that sounds "cool", but that's not really of any use, and that then ends up confusing and misleading.

But if someone, later on, has a proper usecase, we could find a way to handle it.

But for now I would quite like it if we could stop this discussion because it's about a minor feature that's distracting from the rest, and it doesn't seem to be leading anywhere. This PR has 18 comments, ~14 of which are about subsecond resolution, and I simply don't believe it deserves that much attention.

In general we have spent a bunch lately on useless bikesheds and I would really love to spend my energy elsewhere.

@faho faho force-pushed the tips-fedora-mtime branch 3 times, most recently from 9912a26 to 230e41c Compare July 11, 2022 19:22
@floam
Copy link
Member

floam commented Jul 17, 2022

make is an example of a utility that'd probably be harder to implement in shell script in fish if this part did not support sub-second resolution (or it'd carefully use find, test, stat, or ls.) We do that here: https://github.com/fish-shell/fish-shell/blob/master/tests/checks/check-all-fish-files.fish

@faho
Copy link
Member Author

faho commented Jul 18, 2022

You might notice that, what we did there, was to compare with a timestamp file. That can be done with #9058.

If we had subsecond resolution here and passed that timestamp to find it would break in a comma-using locale.

Even without subsecond resolution, the failure case is that you run check-all-fish-files within a second of modifying a file, which I don't even believe is possible via make test?

faho added 2 commits July 18, 2022 20:28
This can be used to print the modification time, like `stat` with some
options.

The reason is that `stat` has caused us a number of portability
headaches:

1. It's not available everywhere by default
2. The versions are quite different

For instance, with GNU stat it's `stat -c '%Y'`, with macOS it's `stat
-f %m`.

So now checking a cache file can be done just with builtins.
It's the new hotness. Still only second resolution, because

1. That's more likely to be supported everywhere
2. What are you gonna do with sub-second resolution in a shell script?
faho added 5 commits July 18, 2022 20:28
Posix *2008*. It's been 14 years and this still isn't implemented in a
certified UNIX.
Encapsulates some of the logic - in particular this removes the ifdeffery
This is kinda meaningless, but nothing better came to mind.

Also because it's so meaningless, we can change it later.
@faho faho force-pushed the tips-fedora-mtime branch from 230e41c to 2ba6bb3 Compare July 18, 2022 18:30
@faho faho merged commit 5dfb64b into fish-shell:master Jul 18, 2022
@faho
Copy link
Member Author

faho commented Jul 18, 2022

I've merged this now, without subsecond resolution. I apparently didn't manage to convince everyone, but that's the way it is.

We were starting to go in circles and so it seems more productive to move on.

@faho faho deleted the tips-fedora-mtime branch September 7, 2022 11:56
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants