Re: [PATCH v2 4/4] maintenance: use Windows scheduled tasks

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Nov 11, 2020, 08:59 UTC
Message-ID: <CAPig+cRRQc=RNd=zGFvi2yiLD6PoLLBJQtFxkZ+QaVVA9MP1DA@mail.gmail.com>
In-Reply-To: <84eb44de31f04b2a94f57ee11d70be81f5bbeee2.1604520368.git.gitgitgadget@gmail.com>

On Wed, Nov 4, 2020 at 3:06 PM Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com> wrote:

Show 12 quoted lines

> Git's background maintenance uses cron by default, but this is not
> available on Windows. Instead, integrate with Task Scheduler.
> [...]
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
> diff --git a/builtin/gc.c b/builtin/gc.c
> @@ -1698,6 +1698,187 @@ static int platform_update_schedule(int run_maintenance, int fd)
> +static int schedule_task(const char *exec_path, enum schedule_priority schedule)
> +{
> +       xmlpath =  xstrfmt("%s/schedule-%s.xml",
> +                          the_repository->objects->odb->path,
> +                          frequency);

Am I reading correctly that it is writing this throwaway XML file into the Git object directory? Would writing to a temporary directory make more sense? (Not worth a re-roll.)

> +       xmlfp = fopen(xmlpath, "w");
> +       if (!xmlfp)
> +               die(_("failed to open '%s'"), xmlpath);

Could use xfopen() as mentioned previously.

Show 7 quoted lines

> diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
> @@ -442,6 +442,36 @@ test_expect_success MACOS_MAINTENANCE 'start and stop macOS maintenance' '
> +test_expect_success MINGW 'start and stop Windows maintenance' '
> +       for frequency in hourly daily weekly
> +       do
> +               printf "/create /tn Git Maintenance (%s) /f /xml .git/objects/schedule-%s.xml\n" \
> +                       $frequency $frequency

Nit: You lost the `|| return 1` which was present in the previous
version. True, it's very unlikely that `printf` could fail, but having
the `|| return 1` there makes it easier for the reader's eye to glide
over the code without having to worry about whether it is handling
error conditions correctly, thus reduces cognitive load.

> +       done >expect &&

Rather than a loop, you could just use:

    printf "/create ... (%s) /f /xml ...schedule-%s.xml\n" \
        hourly hourly daily daily weekly weekly >expect &&

though it's subjective as to whether that is an improvement.