On 8/20/2020 10:51 AM, Đoàn Trần Công Danh wrote:
> On 2020-08-19 17:16:44+0000, Derrick Stolee via GitGitGadget <gitgitgadget@xxxxxxxxx> wrote:
>> +static void fill_schedule_info(struct maintenance_task *task,
>> + const char *config_name,
>> + timestamp_t schedule_delay)
>> +{
>> + timestamp_t now = approxidate("now");
>
> I see this pattern in both previous patch and this patch,
> should we create a helper (if not exist) to get current timestamp
> instead, parsing "now" every now and then is not a good idea, in my
> very opinionated opinion.
Parsing "now" is not that much work, and it is done only once per
maintenance task. To make a helper that avoids these string comparisons
(specifically to avoid iterating through the "special" array in date.c)
is unlikely to be worth the effort and code duplication.
If you mean it would be good to use a macro here, then that would be
easy:
#define approxidate_now() approxidate("now")
One important thing for using this over time(NULL) is that we really
want this to work with GIT_TEST_DATE_NOW.
>> + char *value = NULL;
>> + struct strbuf last_run = STRBUF_INIT;
>> + int64_t previous_run;
>> +
>> + strbuf_addf(&last_run, "maintenance.%s.lastrun", task->name);
>> +
>> + if (git_config_get_string(last_run.buf, &value))
>> + task->scheduled = 1;
>> + else {
>> + previous_run = git_config_int64(last_run.buf, value);
>> + if (now >= previous_run + schedule_delay)
>> + task->scheduled = 1;
>> + }
>> +
>> + free(value);
>> + strbuf_release(&last_run);
>> +}
>> +
>> static void initialize_task_config(void)
>> {
>> int i;
>> @@ -1359,6 +1387,7 @@ static void initialize_task_config(void)
>>
>> for (i = 0; i < TASK__COUNT; i++) {
>> int config_value;
>> + char *config_str;
>>
>> strbuf_setlen(&config_name, 0);
>> strbuf_addf(&config_name, "maintenance.%s.enabled",
>> @@ -1366,6 +1395,20 @@ static void initialize_task_config(void)
>>
>> if (!git_config_get_bool(config_name.buf, &config_value))
>> tasks[i].enabled = config_value;
>> +
>> + strbuf_setlen(&config_name, 0);
>
> It looks like we have a simple and better named alias for this:
>
> strbuf_reset(&config_name)
>
> _reset has 400+ occurences in this code base, compare to 20 of _setlen
Makes sense. Thanks.
-Stolee
