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

[Derrick Stolee <stolee@gmail.com>]

On 11/18/2020 2:15 AM, Eric Sunshine wrote:
> On Tue, Nov 17, 2020 at 4:13 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> [...]
>> Since the GIT_TEST_MAINT_SCHEDULER environment variable allows us to
>> specify 'schtasks' as the scheduler, we can test the Windows-specific
>> logic on a macOS platform. Thus, add a check that the XML file written
>> by Git is valid when xmllint exists on the system.
> 
> Nit: xmllint can be installed on Linux (and likely other platforms),
> as well, so it's not clear why this calls out macOS specially. More
> generally, it may not be important to call out xmllint at all in the
> commit message; it's just _one_ thing being checked by a test which is
> checking several other things not called out individually by the
> commit message. Anyhow, this is minor; not worth a re-roll.

Sorry, it should just say "other platforms"

>> diff --git a/builtin/gc.c b/builtin/gc.c
>> @@ -1671,6 +1671,167 @@ static int launchctl_update_schedule(int run_maintenance, int fd, const char *cm
>> +static int schtasks_schedule_task(const char *exec_path, enum schedule_priority schedule, const char *cmd)
>> +{
>> +       xmlpath =  xstrfmt("%s/schedule-%s.xml",
>> +                          the_repository->objects->odb->path,
>> +                          frequency);
> 
> I missed this in the earlier rounds since I wasn't paying close enough
> attention, but placing this XML file within the object database
> directory (.git/objects/) feels rather odd, even if it is just a
> temporary file. Using the .git/ directory itself might be better,
> perhaps like this:
> 
>     struct strbuf xmlpath = STRBUF_INIT;
>     strbuf_git_common_path(&xmlpath, the_repository,
>         "schtasks-%s.xml", frequency);
>     ...
>     strbuf_release(&xmlpath);

It does look odd, and in this case we could use the .git directory
instead. I specifically use the objects directory for the maintenance
lock in 'git maintenance run' to allow maintenance to run when
GIT_OBJECT_DIRECTORY points to an alternate, allowing us to maintain
object databases that don't have a full .git directory around them.

Thanks,
-Stolee