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