All of lore.kernel.org
 help / color / mirror / Atom feed
From: Phillip Wood <phillip.wood123@gmail.com>
To: "Eric Sunshine" <sunshine@sunshineco.com>,
	"Lénaïc Huard" <lenaic@lhuard.fr>
Cc: "Git List" <git@vger.kernel.org>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Derrick Stolee" <dstolee@microsoft.com>,
	"Đoàn Trần Công Danh" <congdanhqx@gmail.com>,
	"Felipe Contreras" <felipe.contreras@gmail.com>,
	"Martin Ågren" <martin.agren@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	"brian m . carlson" <sandals@crustytoothpaste.net>,
	"Johannes Schindelin" <Johannes.Schindelin@gmx.de>,
	"Jeff King" <peff@peff.net>
Subject: Re: [PATCH v6 2/3] maintenance: `git maintenance run` learned `--scheduler=<scheduler>`
Date: Thu, 17 Jun 2021 15:26:59 +0100	[thread overview]
Message-ID: <80d6050a-71d9-1278-e68f-91c3a1ca52e4@gmail.com> (raw)
In-Reply-To: <CAPig+cSLi7aN=6ahrHwy4fO-7JMBN3pmzfpWe5ZXOcC9j4+e+g@mail.gmail.com>

On 14/06/2021 05:36, Eric Sunshine wrote:
> On Sat, Jun 12, 2021 at 12:51 PM Lénaïc Huard <lenaic@lhuard.fr> wrote:
>> Depending on the system, different schedulers can be used to schedule
>> the hourly, daily and weekly executions of `git maintenance run`:
>> * `launchctl` for MacOS,
>> * `schtasks` for Windows and
>> * `crontab` for everything else.
>>
>> `git maintenance run` now has an option to let the end-user explicitly
>> choose which scheduler he wants to use:
>> `--scheduler=auto|crontab|launchctl|schtasks`.
>>
>> When `git maintenance start --scheduler=XXX` is run, it not only
>> registers `git maintenance run` tasks in the scheduler XXX, it also
>> removes the `git maintenance run` tasks from all the other schedulers to
>> ensure we cannot have two schedulers launching concurrent identical
>> tasks.
>>
>> The default value is `auto` which chooses a suitable scheduler for the
>> system.
>>
>> `git maintenance stop` doesn't have any `--scheduler` parameter because
>> this command will try to remove the `git maintenance run` tasks from all
>> the available schedulers.
>>
>> Signed-off-by: Lénaïc Huard <lenaic@lhuard.fr>
> 
> 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. The material in the cover letter and individual commit
> messages was helpful in understanding the nuances of the changes, and
> the series seems pretty well complete at this point. (If, however, you
> do happen to re-roll for some reason, please consider using the
> --range-diff option of git-format-patch as an aid to reviewers.)
> 
> I did leave a number of comments below regarding possible improvements
> to the code and documentation, however, they're probably mostly
> subjective and don't necessarily warrant a re-roll; I'd have no
> problem seeing this accepted as-is without the suggestions applied.
> (They can always be applied later on if someone considers them
> important enough.)
>
> I do, though, have one question (below) about is_crontab_available()
> for which I could not figure out the answer.

I think that is a bug

>> [...]
>> diff --git a/builtin/gc.c b/builtin/gc.c
>> @@ -1529,6 +1529,59 @@ static const char *get_frequency(enum schedule_priority schedule)
>> +static int get_schedule_cmd(const char **cmd, int *is_available)
>> +{
>> +       char *item;
>> +       char *testing = xstrdup_or_null(getenv("GIT_TEST_MAINT_SCHEDULER"));
>> +
>> +       if (!testing)
>> +               return 0;
>> +
>> +       if (is_available)
>> +               *is_available = 0;
>> +
>> +       for (item = testing;;) {
>> +               char *sep;
>> +               char *end_item = strchr(item, ',');
>> +               if (end_item)
>> +                       *end_item = '\0';
>> +
>> +               sep = strchr(item, ':');
>> +               if (!sep)
>> +                       die("GIT_TEST_MAINT_SCHEDULER unparseable: %s", testing);
>> +               *sep = '\0';
>> +
>> +               if (!strcmp(*cmd, item)) {
>> +                       *cmd = sep + 1;
>> +                       if (is_available)
>> +                               *is_available = 1;
>> +                       UNLEAK(testing);
>> +                       return 1;
>> +               }
>> +
>> +               if (!end_item)
>> +                       break;
>> +               item = end_item + 1;
>> +       }
>> +
>> +       free(testing);
>> +       return 1;
>> +}
> 
> I ended up studying this implementation several times since I had to
> come back to it repeatedly after reading calling code in order to (I
> hope) fully understand all the different conditions represented by its
> three distinct return values (the function return value, and the
> values returned in **cmd and **is_available). That it required several
> readings might warrant a comment block explaining what the function
> does and what the various return conditions mean. As a bonus, an
> explanation of the value of GIT_TEST_MAINT_SCHEDULER -- a
> comma-separated list of colon-delimited tuples, and what those tuples
> represent -- could be helpful.

I agree documenting GIT_TEST_MAINT_SCHEDULER would be useful

>> +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;
>      }
> 
>> +static int is_crontab_available(void)
>> +{
>> +       const char *cmd = "crontab";
>> +       int is_available;
>> +       struct child_process child = CHILD_PROCESS_INIT;
>> +
>> +       if (get_schedule_cmd(&cmd, &is_available) && !is_available)
>> +               return 0;
>> +
>> +       strvec_split(&child.args, cmd);
>> +       strvec_push(&child.args, "-l");
>> +       child.no_stdin = 1;
>> +       child.no_stdout = 1;
>> +       child.no_stderr = 1;
>> +       child.silent_exec_failure = 1;
>> +
>> +       if (start_command(&child))
>> +               return 0;
>> +       /* Ignore exit code, as an empty crontab will return error. */
>> +       finish_command(&child);
>> +       return 1;
>>   }
> 
> If I understand get_schedule_cmd() correctly, it will always return
> true if GIT_TEST_MAINT_SCHEDULER is present in the environment,
> however, it will only set `is_available` to true if
> GIT_TEST_MAINT_SCHEDULER contains a matching entry for `cmd` (which in
> this case is "crontab"). Assuming this understanding is correct, then
> I'm having trouble understanding why this:
> 
>      if (get_schedule_cmd(&cmd, &is_available) && !is_available)
>          return 0;
> 
> isn't instead written like this:
> 
>      if (get_schedule_cmd(&cmd, &is_available))
>          return is_available;
> 
> That is, why doesn't is_crontab_available() trust the result of
> get_schedule_cmd(), instead going ahead and trying to invoke `crontab`
> itself? Am I missing something which makes the `!is_available` case
> special?

I agree, I think we should be returning is_available irrespective of its 
value if get_schedule_cmd() returns true. This is what 
is_systemd_timer_available() does in the next patch. Well spotted.

Best Wishes

Phillip

>> +static void resolve_auto_scheduler(enum scheduler *scheduler)
>> +{
>> +       if (*scheduler != SCHEDULER_AUTO)
>> +               return;
>> +
>>   #if defined(__APPLE__)
>> +       *scheduler = SCHEDULER_LAUNCHCTL;
>> +       return;
>> +
>>   #elif defined(GIT_WINDOWS_NATIVE)
>> +       *scheduler = SCHEDULER_SCHTASKS;
>> +       return;
>> +
>>   #else
>> +       *scheduler = SCHEDULER_CRON;
>> +       return;
>>   #endif
>> +}
> 
> (See above for a way to simplify this implementation.)
> 
> Is there a strong reason which I'm missing that this function alters
> its argument rather than simply returning the resolved scheduler?
> 
>      static enum scheduler resolve_scheduler(enum scheduler x) {...}
> 
> Or is it just personal preference?
>
> (Minor: I took the liberty of shortening the function name since it
> doesn't feel like the longer name adds much value.)
> 
>> +static int maintenance_start(int argc, const char **argv, const char *prefix)
>>   {
>> +       struct maintenance_start_opts opts = { 0 };
>> +       struct option options[] = {
>> +               OPT_CALLBACK_F(
>> +                       0, "scheduler", &opts.scheduler, N_("scheduler"),
>> +                       N_("scheduler to use to trigger git maintenance run"),
> 
> Dropping "to use" would make this more concise without losing clarity:
> 
>      "scheduler to trigger git maintenance run"
> 
>> +                       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 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.)
> 

  parent reply	other threads:[~2021-06-17 14:27 UTC|newest]

Thread overview: 138+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-01 14:52 [PATCH] maintenance: use systemd timers on Linux Lénaïc Huard
2021-05-01 20:02 ` brian m. carlson
2021-05-02  5:28 ` Bagas Sanjaya
2021-05-02  6:49   ` Eric Sunshine
2021-05-02  6:45 ` Eric Sunshine
2021-05-02 14:10   ` Phillip Wood
2021-05-05 12:19     ` Đoàn Trần Công Danh
2021-05-05 14:57       ` Phillip Wood
2021-05-05 12:01   ` Ævar Arnfjörð Bjarmason
2021-05-09 22:34     ` Lénaïc Huard
2021-05-10 13:03       ` Ævar Arnfjörð Bjarmason
2021-05-02 11:12 ` Bagas Sanjaya
2021-05-03 12:04 ` Derrick Stolee
2021-05-09 21:32 ` [PATCH v2 0/1] " Lénaïc Huard
2021-05-09 21:32   ` [PATCH v2 1/1] " Lénaïc Huard
2021-05-10  1:20     ` Đoàn Trần Công Danh
2021-05-10  2:48       ` Eric Sunshine
2021-05-10  6:25         ` Junio C Hamano
2021-05-12  0:29           ` Đoàn Trần Công Danh
2021-05-12  6:59             ` Felipe Contreras
2021-05-12 13:26               ` Phillip Wood
2021-05-12 13:38             ` Phillip Wood
2021-05-12 15:41               ` Đoàn Trần Công Danh
2021-05-10 18:03     ` Phillip Wood
2021-05-10 18:25       ` Eric Sunshine
2021-05-10 20:09         ` Phillip Wood
2021-05-10 20:52           ` Eric Sunshine
2021-06-08 14:55       ` Lénaïc Huard
2021-05-10 19:15     ` Martin Ågren
2021-05-11 14:50     ` Phillip Wood
2021-05-11 17:31     ` Derrick Stolee
2021-05-20 22:13   ` [PATCH v3 0/4] " Lénaïc Huard
2021-05-20 22:13     ` [PATCH v3 1/4] cache.h: rename "xdg_config_home" to "xdg_config_home_git" Lénaïc Huard
2021-05-20 23:44       ` Đoàn Trần Công Danh
2021-05-20 22:13     ` [PATCH v3 2/4] maintenance: introduce ENABLE/DISABLE for code clarity Lénaïc Huard
2021-05-20 22:13     ` [PATCH v3 3/4] maintenance: `git maintenance run` learned `--scheduler=<scheduler>` Lénaïc Huard
2021-05-21  9:52       ` Bagas Sanjaya
2021-05-20 22:13     ` [PATCH v3 4/4] maintenance: optionally use systemd timers on Linux Lénaïc Huard
2021-05-21  9:59       ` Bagas Sanjaya
2021-05-21 16:59         ` Derrick Stolee
2021-05-22  6:57           ` Johannes Schindelin
2021-05-23 18:36             ` Felipe Contreras
2021-05-23 23:27               ` brian m. carlson
2021-05-24  1:18                 ` Felipe Contreras
2021-05-24  7:03                 ` Ævar Arnfjörð Bjarmason
2021-05-24 15:51                   ` Junio C Hamano
2021-05-25  1:50                     ` Johannes Schindelin
2021-05-25 11:13                       ` Felipe Contreras
2021-05-26 10:29                       ` CoC, inclusivity etc. (was "Re: [...] systemd timers on Linux") Ævar Arnfjörð Bjarmason
2021-05-26 16:05                         ` Felipe Contreras
2021-05-27 14:24                         ` Jeff King
2021-05-27 17:30                           ` Felipe Contreras
2021-05-27 23:58                           ` Junio C Hamano
2021-05-28 14:44                           ` Phillip Susi
2021-05-30 21:58                             ` Jeff King
2021-05-24 17:52                   ` [PATCH v3 4/4] maintenance: optionally use systemd timers on Linux Felipe Contreras
2021-05-24  7:15     ` [PATCH v4 0/4] add support for " Lénaïc Huard
2021-05-24  7:15       ` [PATCH v4 1/4] cache.h: Introduce a generic "xdg_config_home_for(…)" function Lénaïc Huard
2021-05-24  9:33         ` Phillip Wood
2021-05-24 12:23           ` Đoàn Trần Công Danh
2021-05-24  7:15       ` [PATCH v4 2/4] maintenance: introduce ENABLE/DISABLE for code clarity Lénaïc Huard
2021-05-24  9:41         ` Phillip Wood
2021-05-24 12:36           ` Đoàn Trần Công Danh
2021-05-25  7:18             ` Lénaïc Huard
2021-05-25  8:02               ` Junio C Hamano
2021-05-24  9:47         ` Ævar Arnfjörð Bjarmason
2021-05-24  7:15       ` [PATCH v4 3/4] maintenance: `git maintenance run` learned `--scheduler=<scheduler>` Lénaïc Huard
2021-05-24 10:12         ` Phillip Wood
2021-05-30  6:39           ` Lénaïc Huard
2021-05-30 10:16             ` Phillip Wood
2021-05-24  7:15       ` [PATCH v4 4/4] maintenance: add support for systemd timers on Linux Lénaïc Huard
2021-05-24  9:55         ` Ævar Arnfjörð Bjarmason
2021-05-24 16:39           ` Eric Sunshine
2021-05-24 18:08         ` Felipe Contreras
2021-05-26 10:26         ` Phillip Wood
2021-05-24  9:04       ` [PATCH v4 0/4] " Junio C Hamano
2021-06-08 13:39       ` [PATCH v5 0/3] " Lénaïc Huard
2021-06-08 13:39         ` [PATCH v5 1/3] cache.h: Introduce a generic "xdg_config_home_for(…)" function Lénaïc Huard
2021-06-08 13:39         ` [PATCH v5 2/3] maintenance: `git maintenance run` learned `--scheduler=<scheduler>` Lénaïc Huard
2021-06-08 13:40         ` [PATCH v5 3/3] maintenance: add support for systemd timers on Linux Lénaïc Huard
2021-06-09  9:34           ` Jeff King
2021-06-09 15:01           ` Phillip Wood
2021-06-09  0:21         ` [PATCH v5 0/3] " Junio C Hamano
2021-06-09 14:54         ` Phillip Wood
2021-06-12 16:50         ` [PATCH v6 0/3] maintenance: " Lénaïc Huard
2021-06-12 16:50           ` [PATCH v6 1/3] cache.h: Introduce a generic "xdg_config_home_for(…)" function Lénaïc Huard
2021-06-12 16:50           ` [PATCH v6 2/3] maintenance: `git maintenance run` learned `--scheduler=<scheduler>` Lénaïc Huard
2021-06-14  4:36             ` Eric Sunshine
2021-06-16 18:12               ` Derrick Stolee
2021-06-17  4:11                 ` Eric Sunshine
2021-06-17 14:26               ` Phillip Wood [this message]
2021-07-02 15:04               ` Lénaïc Huard
2021-06-12 16:50           ` [PATCH v6 3/3] maintenance: add support for systemd timers on Linux Lénaïc Huard
2021-07-02 14:25           ` [PATCH v7 0/3] " Lénaïc Huard
2021-07-02 14:25             ` [PATCH v7 1/3] cache.h: Introduce a generic "xdg_config_home_for(…)" function Lénaïc Huard
2021-07-02 14:25             ` [PATCH v7 2/3] maintenance: `git maintenance run` learned `--scheduler=<scheduler>` Lénaïc Huard
2021-07-06 19:56               ` Ævar Arnfjörð Bjarmason
2021-07-06 20:52                 ` Junio C Hamano
2021-07-13  0:15                   ` Jeff King
2021-07-13  2:22                     ` Eric Sunshine
2021-07-13  3:56                       ` Jeff King
2021-07-13  5:17                         ` Eric Sunshine
2021-07-13  7:04                       ` Bagas Sanjaya
2021-07-06 21:18                 ` Felipe Contreras
2021-08-23 20:06                 ` Lénaïc Huard
2021-08-23 22:30                   ` Junio C Hamano
2021-07-02 14:25             ` [PATCH v7 3/3] maintenance: add support for systemd timers on Linux Lénaïc Huard
2021-07-06 20:03               ` Ævar Arnfjörð Bjarmason
2021-07-02 18:18             ` [PATCH v7 0/3] " Junio C Hamano
2021-07-06 13:18             ` Phillip Wood
2021-08-23 20:40             ` [PATCH v8 " Lénaïc Huard
2021-08-23 20:40               ` [PATCH v8 1/3] cache.h: Introduce a generic "xdg_config_home_for(…)" function Lénaïc Huard
2021-08-23 20:40               ` [PATCH v8 2/3] maintenance: `git maintenance run` learned `--scheduler=<scheduler>` Lénaïc Huard
2021-08-24 17:45                 ` Derrick Stolee
2021-08-23 20:40               ` [PATCH v8 3/3] maintenance: add support for systemd timers on Linux Lénaïc Huard
2021-08-24 17:45                 ` Derrick Stolee
2021-08-24 17:47               ` [PATCH v8 0/3] " Derrick Stolee
2021-08-27 21:02               ` [PATCH v9 " Lénaïc Huard
2021-08-27 21:02                 ` [PATCH v9 1/3] cache.h: Introduce a generic "xdg_config_home_for(…)" function Lénaïc Huard
2021-08-27 21:02                 ` [PATCH v9 2/3] maintenance: `git maintenance run` learned `--scheduler=<scheduler>` Lénaïc Huard
2021-08-27 23:54                   ` Ramsay Jones
2021-08-27 21:02                 ` [PATCH v9 3/3] maintenance: add support for systemd timers on Linux Lénaïc Huard
2021-09-04 20:54                 ` [PATCH v10 0/3] " Lénaïc Huard
2021-09-04 20:54                   ` [PATCH v10 1/3] cache.h: Introduce a generic "xdg_config_home_for(…)" function Lénaïc Huard
2021-09-04 20:54                   ` [PATCH v10 2/3] maintenance: `git maintenance run` learned `--scheduler=<scheduler>` Lénaïc Huard
2021-09-04 20:55                   ` [PATCH v10 3/3] maintenance: add support for systemd timers on Linux Lénaïc Huard
2021-09-07 16:48                   ` [PATCH v10 0/3] " Derrick Stolee
2021-09-08 11:44                     ` Derrick Stolee
2021-09-09  5:52                       ` Lénaïc Huard
2021-09-09 19:55                         ` Derrick Stolee
2021-09-27 12:50                           ` Ævar Arnfjörð Bjarmason
2021-09-27 21:44                             ` Lénaïc Huard
2021-08-17 17:22         ` [PATCH v5 0/3] " Derrick Stolee
2021-08-17 19:43           ` Phillip Wood
2021-08-17 20:29             ` Derrick Stolee
2021-08-18  5:56           ` Lénaïc Huard
2021-08-18 13:28             ` Derrick Stolee
2021-08-18 18:23               ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=80d6050a-71d9-1278-e68f-91c3a1ca52e4@gmail.com \
    --to=phillip.wood123@gmail.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=avarab@gmail.com \
    --cc=bagasdotme@gmail.com \
    --cc=congdanhqx@gmail.com \
    --cc=dstolee@microsoft.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=lenaic@lhuard.fr \
    --cc=martin.agren@gmail.com \
    --cc=peff@peff.net \
    --cc=phillip.wood@dunelm.org.uk \
    --cc=sandals@crustytoothpaste.net \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.