Re: [PATCH 3/7] maintenance: add --scheduled option and config
- From
Đoàn Trần Công Danh <congdanhqx@gmail.com>
- Date
- Aug 20, 2020, 14:51 UTC
- Message-ID
- <20200820145127.GC31084@danh.dev>
- In-Reply-To
- <4473c93b118a0e0cdb205d1758aaaa2d8bf5139a.1597857412.git.gitgitgadget@gmail.com>
On 2020-08-19 17:16:44+0000, Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 5 quoted lines
> +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.
Show 35 quoted lines
> + 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
-- Danh