Re: [PATCH v6 2/3] maintenance: `git maintenance run` learned `--scheduler=<scheduler>`

Le lundi 14 juin 2021, 06:36:09 CEST Eric Sunshine a écrit :

Thanks. Unfortunately, I haven't been following this series too
closely since I reviewed v1, so I set aside time to review v6, which I
have now done.
…
I do, though, have one question (below) about is_crontab_available()
for which I could not figure out the answer.

Thank you very much for your review.
I’ve just submitted a new re-roll that should address the points you raised 
and in particular the `is_crontab_available` unexpected behavior.

…

quoted

+static int is_launchctl_available(void)
+{
+       const char *cmd = "launchctl";
+       int is_available;
+       if (get_schedule_cmd(&cmd, &is_available))
+               return is_available;
+
+#ifdef __APPLE__
+       return 1;
+#else
+       return 0;
+#endif
+}

On this project, we usually frown upon #if conditionals within
functions since the code often can become unreadable. The usage in
this function doesn't suffer from that problem, however,
resolve_auto_scheduler() is somewhat ugly. An alternative would be to
set up these values outside of all functions, perhaps like this:

    #ifdef __APPLE__
    #define MAINT_SCHEDULER SCHEDULER_LAUNCHCTL
    #elif GIT_WINDOWS_NATIVE
    #define MAINT_SCHEDULER SCHEDULER_SCHTASKS
    #else
    #define MAINT_SCHEDULER SCHEDULER_CRON
    #endif

and then:

    static int is_launchctl_available(void)
    {
        if (get_schedule_cmd(...))
            return is_available;
        return MAINT_SCHEDULER == SCHEDULER_LAUNCHCTL;
    }

    static void resolve_auto_scheduler(enum scheduler *scheduler)
    {
        if (*scheduler == SCHEDULER_AUTO)
            *scheduler = MAINT_SCHEDULER;
    }

This approach would unfortunately work only for the second patch of this 
series where a single scheduler is available on each platform.
With the third patch of this series, `resolve_auto_scheduler` doesn’t return a 
value that is fully determined at compilation time anymore.
On Linux, both `crontab` and `systemd-timers` are susceptible to be available 
and this is checked at runtime.
So, with the third patch of this series, it wouldn’t be possible anymore to 
define a single value for `MAINT_SCHEDULER` and to base 
`resolve_auto_scheduler` on it.

…

quoted

+                       PARSE_OPT_NONEG, maintenance_opt_scheduler),
+               OPT_END()
+       };

diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh

@@ -494,8 +494,21 @@ test_expect_success !MINGW 'register and unregister

with regex metacharacters' ' +test_expect_success 'start
--scheduler=<scheduler>' '
+       test_expect_code 129 git maintenance start --scheduler=foo 2>err
&&
+       test_i18ngrep "unrecognized --scheduler argument" err &&
+
+       test_expect_code 129 git maintenance start --no-scheduler 2>err &&
+       test_i18ngrep "unknown option" err &&
+
+       test_expect_code 128 \
+               env
GIT_TEST_MAINT_SCHEDULER="launchctl:true,schtasks:true" \ +              
git maintenance start --scheduler=crontab 2>err && +       test_i18ngrep
"fatal: crontab scheduler is not available" err +'

Why does this test care about the exact exit codes rather than simply
using test_must_fail() as is typically done elsewhere in the test
suite, especially since we're also checking the error message itself?
Am I missing some non-obvious property of the error codes?

I have no strong opinion on this.
I only mimicked the `help text` test that is at the top of the `t7900-
maintenance.sh` file as it was also testing invalid commands and checking the 
resulting error message.

I don't see `auto` being tested anywhere. Do we want such a test? (It
seems like it should be doable, though perhaps the complexity is too
high -- I haven't thought it through fully.)

My main problem with the `auto` is that a big part of its logic is determined 
at compilation time with `#if` statements based on the platform.
And it seems that the tests are designed so far to test the same things on all 
platforms.
A solution could be to completely get rid of the platform `#if` statements and 
to turn all the detection logic as runtime tests.
But it would mean that, for ex., the git binary for Linux would systematically 
check if the MacOS and Windows specific schedulers are available.

Cheers,
Lénaïc.