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

From: Derrick Stolee <stolee@gmail.com>
To: Eric Sunshine <sunshine@sunshineco.com>,
	Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: "Git List" <git@vger.kernel.org>,
	"Jonathan Nieder" <jrnieder@gmail.com>,
	"Jonathan Tan" <jonathantanmy@google.com>,
	"Son Luong Ngoc" <sluongng@gmail.com>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Derrick Stolee" <derrickstolee@github.com>,
	"Derrick Stolee" <dstolee@microsoft.com>
Subject: Re: [PATCH v2 4/4] maintenance: use Windows scheduled tasks
Date: Thu, 12 Nov 2020 08:56:45 -0500	[thread overview]
Message-ID: <d46826a5-0c30-3d71-26a0-f579d997c3a0@gmail.com> (raw)
In-Reply-To: <CAPig+cRRQc=RNd=zGFvi2yiLD6PoLLBJQtFxkZ+QaVVA9MP1DA@mail.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