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

[Derrick Stolee <stolee@gmail.com>]

On 11/11/2020 3:59 AM, Eric Sunshine wrote:
> On Wed, Nov 4, 2020 at 3:06 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> 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.)

A temp directory is a good idea.

>> +       xmlfp = fopen(xmlpath, "w");
>> +       if (!xmlfp)
>> +               die(_("failed to open '%s'"), xmlpath);
> 
> Could use xfopen() as mentioned previously.
> 
>> 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.

It's sufficient, and avoids issues with deep tabbing and
chaining "|| return 1"".

Thanks,
-Stolee