Limit the allowed epoch timestamps

[lanker]

The code for parsing epoch timestamps when displaying tasks only supports values between year 1980 and 9999. Previous to this change, it was possible to set e.g., the due timestamp to a value outside of these limits, which would make it impossible to later on show the task.

With this change, we only allow setting values within the same limits used by the code for displaying tasks.

Fixes https://github.com/GothenburgBitFactory/taskwarrior/issues/3444

[lanker]

This PR is depending on https://github.com/GothenburgBitFactory/libshared/pull/83. Not sure how you handle bumping the version of libshared that is used in Taskwarrior...?

[djmitche]

Let's wait until that PR is merged, and then include a change to the submodule version in this PR.

That will also affect https://github.com/GothenburgBitFactory/taskwarrior/issues/3623 but I think there's no problem in rolling both of those at once.

[lanker]

Just some additional info if anyone ends up here in the future:

Looking at the commit and PR history, I couldn't find any reason for the lower limit when parsing timestamps (the upper limit was introduced from this issue), so my initial approach was to remove it completely. However, it seems like more strings than potential epoch timestamps are sent to Datetime::parse_epoch.

For example when doing task 3 info, Datetime::parse_epoch is called with the value 3, and must return false for it to work. So somehow, we need to differentiate between numbers being a timestamp and numbers being something else, like a task ID. I think that's why there's a lower limit. I guess it would've been nice to only trying to parse strings that we know should be timestamps, but I'm sure there's a good reason for it to work as it does.

(So there might be problems if a task ID would be > 315532800, but let's hope no one ends up there... :))

[djmitche]

OK, the libshared update has been merged into Taskwarrior, if you'd like to rebase this PR.

[djmitche]

If you're ready for a review on this, please add me and/or @smemsh as a reviewer!

[lanker]

If you're ready for a review on this, please add me and/or @smemsh as a reviewer!

I don't think I have enough permissions to be able to add reviewers. According to the documentation: "To assign a reviewer to a pull request, you will need write access to the repository.".

So please add yourself and anyone else that should have a look at it, thanks!

[smemsh]

Have you considered just using Datetime::parse_epoch() itself

But since you're using those defines, and it's just a simple bounds check, it seems valid enough in any case to do it like your current patch already does.

So, the code looks sane to me @djmitche ...

[lanker]

Have you considered just using Datetime::parse_epoch() itself and checking the return value? Since the range check is already in there. This way it wouldn't have to be duplicated in both taskwarrior and libshared. Just an idea...

That would actually be a better solution, so I took a brief look at it, not much luck though... Firstly, Datetime::parse_epoch() is private to Datetime, and I'm not comfortable enough with the code (or C++ in general) to start changing visibilities. Secondly, Datetime::parse_epoch() takes a Pig as argument. To create a Pig you need a string, I couldn't find any string that would work for us here.

I think it could've been nice to have a bool Datetime::check_epoch(time_t epoch) that could be used both here and in libshared, but might be a bit overkill for just a bounds check.