Sort By Change Time

[kmwenja]

Sort file list by file change time. Slightly dirty, no tests.

This probably doesn't follow contributing rules (PRs are for bugfixes according to the contributing guidelines) but I didn't know the best way to raise this. See details below.

---

Just recently run into lf (by seeing it on @LukeSmithxyz's site, thanks btw) and I was hooked by the very fact that it's written in Go so I decided to try it out.

Then I hit the most curious of walls; for some reason ranger was sorting files by time in a different way to lf. Soon after I realized I was using oc on ranger which sorts by change time while lf's only time sort was using os.FileInfo.ModTime (aka modification time). On a whim, I decided to see if I could implement change time sorting.

It's weird that go stdlib doesn't expose file change times so I sought out an answer (DDG to the rescue), first landing on an SO question and then to https://github.com/djherbis/times which at least attempts to be cross-platform. Some quick hacks later and the code below was born (BTW this was very quick, a testament to how organized this codebase is).

I'm presuming that I've probably overstepped by starting a PR directly, not to mention introducing a new lib and not writing tests. But I thought starting an issue while I already had working code would be a roundabout way of getting here and I wanted to see if the code could speak for itself.

Is this something that can get merged in and if so what would I need to do to clean it up to something merge-able?

[kmwenja]

I jumped the gun a bit, there seems to be instances where some files aren't added to the file list (I'm guessing the stat check is failing?). Looking into it.

[kmwenja]

On the latest commit, I'm falling back to ModTime if ChangeTime cannot be determined. That seems to fix the missing files issue. I'll keep using it to see if I ferret out any weird issues.

[gokcehan]

@kmwenja Sorry for late reply and thanks for contributing. It says in the wiki page that it is best to open an issue first only because I don't want to waste other people's time working on a patch that is not to be merged. So it is ok for me if it is ok for you too.

I am usually hesitant to add a new dependency but this one seems like a clean and stable library so I think it is ok. Looking at the code without trying, have you considered using HasChangeTime() instead of checking the err variable for the fallback? Also do you think we should also add atime sorting option as well? btime seems like a relatively rare option so I think we can leave it out for now.

[kmwenja]

Awesome! I've gone ahead and added access time sorting as well as updated the info types (it was previously still showing ModTime() on the ui).
It also turns out that HasChangeTime() is actually a guard for a panic on ChangeTime() so I've gone ahead and considered that as well.

[kmwenja]

Oh, almost forgot. I also rebased the commits I was working on so it's one single commit now.

[kmwenja]

Latest commits are just tiny cleanups. Should be good to look at now.

[gokcehan]

@kmwenja I just realized we are now doing an extra stat call for each file. We already have a stat call just above which should already have this information. Is it not possible to get the time information from that file info and get rid of the redundant stat call? Looking at the library code, I could not find a way to do so. We somehow need an exported version of the function func getTimespec(fi os.FileInfo) Timespec for this purpose. System calls can be somewhat expensive. It may be worthwhile to propose adding such an exported function to the times library.

Also currently we seem to be using os.Lstat and times.Stat so there is a mismatch there. I think we should better use times.Lstat instead though it is better to avoid the call altogether as mentioned above.

[kmwenja]

Good point, nice catch. There is actually an exported function here: https://godoc.org/github.com/djherbis/times#Get. Let me see what I can do with it.

[kmwenja]

@gokcehan Updated.

[kmwenja]

@gokcehan Some cautionary notes on the times' README:

Windows XP does not have ChangeTime so HasChangeTime = false, however Vista onward does have ChangeTime so Timespec.HasChangeTime() will only return false on those platforms when the syscall used to obtain them fails.
Also note, Get(FileInfo) will now only return values available in FileInfo.Sys(), this means Stat() is required to get ChangeTime on Windows

[gokcehan]

@kmwenja It seems like a confusing note and I'm not sure what it means. I hope it does not panic in windows.

[gokcehan]

@kmwenja Also thanks for the patch.

[kmwenja]

Woah! This was awesome. Thanks for letting me!