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.
…
> > +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.
…
> > + 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.
