Re: [PATCH v2 2/7] maintenance: store the "last run" time in config

[Derrick Stolee <stolee@gmail.com>]

On 8/25/2020 5:52 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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