On 8/25/2020 5:52 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> I selected the timestamp before the task, as opposed to after the task,
>> for a couple reasons:
>>
>> 1. The time the task takes to execute should not contribute to the
>> interval between running the tasks.
>
> ... as long as the run time is sufficiently shorter than the
> interval, that is. If a task takes 10-30 minutes depending on how
> dirty the repository is, it does not make sense to even try to run
> it every 15 minutes.
Definitely. The lock on the object database from earlier prevents these
longer-than-anticipated tasks from stacking.
>> If a daily task takes 10 minutes
>> to run, then every day the execution will drift by at least 10
>> minutes.
>
> That is not incorrect per-se, but it does not tell us why drifting
> by 10 minutes is a bad thing.
True.
>> 2. If the task fails for some unforseen reason, it would be good to
>> indicate that we _attempted_ the task at a certain timestamp. This
>> will avoid spamming a repository that is in a bad state.
>
> Absolutely.
>
>> +static void update_last_run(struct maintenance_task *task)
>> +{
>> + timestamp_t now = approxidate("now");
>> + struct strbuf config = STRBUF_INIT;
>> + struct strbuf value = STRBUF_INIT;
>> + strbuf_addf(&config, "maintenance.%s.lastrun", task->name);
>> + strbuf_addf(&value, "%"PRItime"", now);
>
> So is this essentially meant as a human-unreadable opaque value,
> like we have in the commit object header lines? I do not have a
> strong opinion, but it would be nice to allow curious to casually
> read it. Perhaps "git config --type=timestamp maintenance.lastrun"
> can be taught to pretty print its value?
Good idea. I will think on this. Of course, we already have
--type=expiry-date, which does the opposite. Perhaps this config
value should be a human-readable date and then be parsed into a
timestamp in-process using git_config_expiry_date()?
I have mixed feelings on using that format, because it can store
both a fixed or relative datetime. The *.lastRun config really
wants a _fixed_ datetime, but the *.schedule config (in the next
patch) would want a _relative_ datetime. This also allows things
like "now" or "never", so it presents a lot of flexibility for
users. A nightmare to test, but perhaps that flexibility is
useful.
(Of course, in another thread you mentioned multiple `crontab`
lines, which might make this entire discussion irrelevant. I'll
follow up there.)
Thanks,
-Stolee
