From: "Lénaïc Huard" <lenaic@lhuard.fr>
To: "Phillip Wood" <phillip.wood123@gmail.com>,
"Đoàn Trần Công Danh" <congdanhqx@gmail.com>
Cc: git@vger.kernel.org, "Junio C Hamano" <gitster@pobox.com>,
"Derrick Stolee" <dstolee@microsoft.com>,
"Eric Sunshine" <sunshine@sunshineco.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>
Subject: Re: [PATCH v4 2/4] maintenance: introduce ENABLE/DISABLE for code clarity
Date: Tue, 25 May 2021 09:18:09 +0200 [thread overview]
Message-ID: <9164987.SKD4A0viWJ@coruscant.lhuard.fr> (raw)
In-Reply-To: <YKudvp0m3m+Yi1+m@danh.dev>
Le lundi 24 mai 2021, 14:36:14 CEST Đoàn Trần Công Danh a écrit :
> On 2021-05-24 10:41:18+0100, Phillip Wood <phillip.wood123@gmail.com> wrote:
> > Hi Lénaïc
> >
> > On 24/05/2021 08:15, Lénaïc Huard wrote:
> > > The first parameter of `XXX_update_schedule` and alike functions is a
> > > boolean specifying if the tasks should be scheduled or unscheduled.
> > >
> > > Using an `enum` with `ENABLE` and `DISABLE` values can make the code
> > > clearer.
> >
> > I'm sorry to say that I'm not sure this does make the code clearer overall
> > - I wish I'd spoken up when Danh suggested it.
> > While
> >
> > launchctl_boot_plist(DISABLE, filename, cmd)
> >
> > is arguably clearer than
> >
> > launchctl_boot_plist(0, filename, cmd)
> >
> > we end up with bizarre tests like
> >
> > if (enabled == ENABLED)
> >
> > rather than
> >
> > if (enabled)
> >
> > and in the next patch we have
> >
> > (enable == ENABLE && (opts->scheduler == i)) ?
> >
> > ENABLE : DISABLE;
> >
> > rather than
> >
> > enable && opts->scheduler == i
> >
> > Also looking at the next patch it seems as this one is missing some
> > conversions in maintenance_start() as it is still calling
> > update_background_schedule() with an integer rather than the new enum.
>
> Yes, in this form, I also think the change looks bizarre.
> And, it's entirely my fault.
>
> I also agree with Ævar that 0 and 1 is meant well for off/on.
>
> However, I still think
>
> launchctl_boot_plist(0, filename, cmd)
>
> would require some degree on code navigation to figure out what would
> that LoC does.
>
> I'm thinking about rename the function. But, it would trigger a forever
> bikeshedding, which shouldn't be a blocker for this series.
>
> > I'd be happy to see this being dropped I'm afraid
>
> So, let's drop this patch and start a new conversation when the dust
> settled.
Hi,
I think the reason why the code looks worse is because I used an enum and I
didn’t want to make any assumption about how the enum members would be
evaluated in a boolean context.
Do you think it would make sense to drop the enum type, to revert all logic
changes (Use `if (enabled)` back instead of `switch`, etc.), and to define the
following constants :
static const int DISABLE = 0;
static const int ENABLE = 1;
so that we can keep function invocation in the form of
`launchctl_boot_plist(DISABLE, filename, cmd)` ?
next prev parent reply other threads:[~2021-05-25 7:18 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 [this message]
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
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=9164987.SKD4A0viWJ@coruscant.lhuard.fr \
--to=lenaic@lhuard.fr \
--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=martin.agren@gmail.com \
--cc=phillip.wood123@gmail.com \
--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.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).