From: Junio C Hamano <gitster@pobox.com>
To: "Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, "Eric Sunshine" <sunshine@sunshineco.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Eric DeCosta" <edecosta@mathworks.com>
Subject: Re: [PATCH v2 07/12] fsmonitor: prepare to share code between Mac OS and Linux
Date: Fri, 14 Oct 2022 16:46:17 -0700 [thread overview]
Message-ID: <xmqqmt9y55py.fsf@gitster.g> (raw)
In-Reply-To: <295beb89ab10f001724fb64fa55944d05ee29fc7.1665783945.git.gitgitgadget@gmail.com> (Eric DeCosta via GitGitGadget's message of "Fri, 14 Oct 2022 21:45:39 +0000")
"Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes:
> ifdef FSMONITOR_DAEMON_BACKEND
> COMPAT_CFLAGS += -DHAVE_FSMONITOR_DAEMON_BACKEND
> + ifdef FSMONITOR_DAEMON_COMMON
> + COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_COMMON).o
> + else
> + COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_BACKEND).o
> + endif
> COMPAT_OBJS += compat/fsmonitor/fsm-listen-$(FSMONITOR_DAEMON_BACKEND).o
> COMPAT_OBJS += compat/fsmonitor/fsm-health-$(FSMONITOR_DAEMON_BACKEND).o
> - COMPAT_OBJS += compat/fsmonitor/fsm-ipc-$(FSMONITOR_DAEMON_BACKEND).o
> endif
>
> ifdef FSMONITOR_OS_SETTINGS
> COMPAT_CFLAGS += -DHAVE_FSMONITOR_OS_SETTINGS
> + ifdef FSMONITOR_DAEMON_COMMON
> + COMPAT_OBJS += compat/fsmonitor/fsm-settings-$(FSMONITOR_DAEMON_COMMON).o
> + endif
> COMPAT_OBJS += compat/fsmonitor/fsm-settings-$(FSMONITOR_OS_SETTINGS).o
> COMPAT_OBJS += compat/fsmonitor/fsm-path-utils-$(FSMONITOR_OS_SETTINGS).o
> endif
Ugly.
One overrides backend with common, while the other one doesn't.
That asymmetry alone should stop us and wonder if there is something
fishy in the approach that can be improved. It makes it look like
the word "common" means something quite different between the -ipc
and the -settings world.
I suspect that in both, you should not expose "unix" to this part of
the Makefile. Linux and macOS occasionally being similar in some
places does not have to be exposed here. INstead you can use
backend "linux" and "macos", whose C sources may include from a
separate C source file whose name may contain "unix". That would
allow you to get rid of FSMONITOR_DAEMON_COMMON in a cleaner way.
next prev parent reply other threads:[~2022-10-14 23:46 UTC|newest]
Thread overview: 89+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-09 14:37 [PATCH 00/12] fsmonitor: Implement fsmonitor for Linux Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 01/12] fsmonitor: refactor filesystem checks to common interface Eric DeCosta via GitGitGadget
2022-10-18 12:29 ` Ævar Arnfjörð Bjarmason
2022-10-09 14:37 ` [PATCH 02/12] fsmonitor: relocate socket file if .git directory is remote Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 03/12] fsmonitor: avoid socket location check if using hook Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 04/12] fsmonitor: deal with synthetic firmlinks on macOS Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 05/12] fsmonitor: check for compatability before communicating with fsmonitor Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 06/12] fsmonitor: add documentation for allowRemote and socketDir options Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 07/12] fsmonitor: prepare to share code between Mac OS and Linux Eric DeCosta via GitGitGadget
2022-10-09 22:36 ` Junio C Hamano
2022-10-10 16:23 ` Jeff Hostetler
2022-10-09 14:37 ` [PATCH 08/12] fsmonitor: determine if filesystem is local or remote Eric DeCosta via GitGitGadget
2022-10-10 10:04 ` Ævar Arnfjörð Bjarmason
2022-10-14 19:38 ` Eric DeCosta
2022-10-09 14:37 ` [PATCH 09/12] fsmonitor: implement filesystem change listener for Linux Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 10/12] fsmonitor: enable fsmonitor " Eric DeCosta via GitGitGadget
2022-10-09 14:37 ` [PATCH 11/12] fsmonitor: test updates Eric DeCosta via GitGitGadget
2022-10-18 12:42 ` Ævar Arnfjörð Bjarmason
2022-10-09 14:37 ` [PATCH 12/12] fsmonitor: update doc for Linux Eric DeCosta via GitGitGadget
2022-10-18 12:43 ` Ævar Arnfjörð Bjarmason
2022-10-09 22:24 ` [PATCH 00/12] fsmonitor: Implement fsmonitor " Junio C Hamano
2022-10-10 0:19 ` Eric Sunshine
2022-10-10 21:08 ` Junio C Hamano
2022-10-14 21:45 ` [PATCH v2 " Eric DeCosta via GitGitGadget
2022-10-14 21:45 ` [PATCH v2 01/12] fsmonitor: refactor filesystem checks to common interface Eric DeCosta via GitGitGadget
2022-10-14 21:45 ` [PATCH v2 02/12] fsmonitor: relocate socket file if .git directory is remote Eric DeCosta via GitGitGadget
2022-10-18 12:12 ` Ævar Arnfjörð Bjarmason
2022-10-14 21:45 ` [PATCH v2 03/12] fsmonitor: avoid socket location check if using hook Eric DeCosta via GitGitGadget
2022-10-14 21:45 ` [PATCH v2 04/12] fsmonitor: deal with synthetic firmlinks on macOS Eric DeCosta via GitGitGadget
2022-10-14 21:45 ` [PATCH v2 05/12] fsmonitor: check for compatability before communicating with fsmonitor Eric DeCosta via GitGitGadget
2022-10-14 21:45 ` [PATCH v2 06/12] fsmonitor: add documentation for allowRemote and socketDir options Eric DeCosta via GitGitGadget
2022-10-14 21:45 ` [PATCH v2 07/12] fsmonitor: prepare to share code between Mac OS and Linux Eric DeCosta via GitGitGadget
2022-10-14 23:46 ` Junio C Hamano [this message]
2022-10-17 21:30 ` Eric DeCosta
2022-10-18 6:31 ` Junio C Hamano
2022-10-14 21:45 ` [PATCH v2 08/12] fsmonitor: determine if filesystem is local or remote Eric DeCosta via GitGitGadget
2022-10-14 21:45 ` [PATCH v2 09/12] fsmonitor: implement filesystem change listener for Linux Eric DeCosta via GitGitGadget
2022-10-18 11:59 ` Ævar Arnfjörð Bjarmason
2022-10-14 21:45 ` [PATCH v2 10/12] fsmonitor: enable fsmonitor " Eric DeCosta via GitGitGadget
2022-10-14 21:45 ` [PATCH v2 11/12] fsmonitor: test updates Eric DeCosta via GitGitGadget
2022-10-14 21:45 ` [PATCH v2 12/12] fsmonitor: update doc for Linux Eric DeCosta via GitGitGadget
2022-10-14 23:32 ` [PATCH v2 00/12] fsmonitor: Implement fsmonitor " Junio C Hamano
2022-10-17 21:32 ` Eric DeCosta
2022-10-17 22:22 ` Junio C Hamano
2022-10-18 14:02 ` Johannes Schindelin
2022-10-17 22:14 ` Glen Choo
2022-10-18 4:17 ` Junio C Hamano
2022-10-18 17:03 ` Glen Choo
2022-10-18 17:11 ` Junio C Hamano
2022-10-19 1:19 ` Ævar Arnfjörð Bjarmason
2022-10-19 2:28 ` Eric Sunshine
2022-10-19 16:58 ` Junio C Hamano
2022-10-19 19:11 ` Ævar Arnfjörð Bjarmason
2022-10-19 20:14 ` Eric Sunshine
2022-10-19 1:04 ` Ævar Arnfjörð Bjarmason
2022-10-19 16:33 ` Junio C Hamano
2022-10-20 16:13 ` Junio C Hamano
2022-10-20 16:37 ` Eric Sunshine
2022-10-20 17:01 ` Junio C Hamano
2022-10-20 17:54 ` Eric Sunshine
2022-10-20 15:43 ` Junio C Hamano
2022-10-20 22:01 ` Junio C Hamano
2022-11-16 23:23 ` [PATCH v3 0/6] " Eric DeCosta via GitGitGadget
2022-11-16 23:23 ` [PATCH v3 1/6] fsmonitor: prepare to share code between Mac OS and Linux Eric DeCosta via GitGitGadget
2022-11-16 23:23 ` [PATCH v3 2/6] fsmonitor: determine if filesystem is local or remote Eric DeCosta via GitGitGadget
2022-11-16 23:23 ` [PATCH v3 3/6] fsmonitor: implement filesystem change listener for Linux Eric DeCosta via GitGitGadget
2022-11-16 23:23 ` [PATCH v3 4/6] fsmonitor: enable fsmonitor " Eric DeCosta via GitGitGadget
2022-11-16 23:23 ` [PATCH v3 5/6] fsmonitor: test updates Eric DeCosta via GitGitGadget
2022-11-16 23:23 ` [PATCH v3 6/6] fsmonitor: update doc for Linux Eric DeCosta via GitGitGadget
2022-11-16 23:27 ` [PATCH v3 0/6] fsmonitor: Implement fsmonitor " Taylor Blau
2022-11-23 19:00 ` [PATCH v4 " Eric DeCosta via GitGitGadget
2022-11-23 19:00 ` [PATCH v4 1/6] fsmonitor: prepare to share code between Mac OS and Linux Eric DeCosta via GitGitGadget
2022-11-23 19:00 ` [PATCH v4 2/6] fsmonitor: determine if filesystem is local or remote Eric DeCosta via GitGitGadget
2022-11-25 7:31 ` Junio C Hamano
2022-12-12 10:24 ` Ævar Arnfjörð Bjarmason
2022-11-23 19:00 ` [PATCH v4 3/6] fsmonitor: implement filesystem change listener for Linux Eric DeCosta via GitGitGadget
2022-12-12 10:42 ` Ævar Arnfjörð Bjarmason
2022-11-23 19:00 ` [PATCH v4 4/6] fsmonitor: enable fsmonitor " Eric DeCosta via GitGitGadget
2022-11-23 19:00 ` [PATCH v4 5/6] fsmonitor: test updates Eric DeCosta via GitGitGadget
2022-11-23 19:00 ` [PATCH v4 6/6] fsmonitor: update doc for Linux Eric DeCosta via GitGitGadget
2022-12-12 21:57 ` [PATCH v5 0/6] fsmonitor: Implement fsmonitor " Eric DeCosta via GitGitGadget
2022-12-12 21:58 ` [PATCH v5 1/6] fsmonitor: prepare to share code between Mac OS and Linux Eric DeCosta via GitGitGadget
2022-12-12 21:58 ` [PATCH v5 2/6] fsmonitor: determine if filesystem is local or remote Eric DeCosta via GitGitGadget
2022-12-12 21:58 ` [PATCH v5 3/6] fsmonitor: implement filesystem change listener for Linux Eric DeCosta via GitGitGadget
2022-12-12 21:58 ` [PATCH v5 4/6] fsmonitor: enable fsmonitor " Eric DeCosta via GitGitGadget
2022-12-12 21:58 ` [PATCH v5 5/6] fsmonitor: test updates Eric DeCosta via GitGitGadget
2022-12-12 21:58 ` [PATCH v5 6/6] fsmonitor: update doc for Linux Eric DeCosta via GitGitGadget
2023-04-12 5:42 ` [PATCH v5 0/6] fsmonitor: Implement fsmonitor " Junio C Hamano
2022-10-18 12:47 ` [PATCH 00/12] " Ævar Arnfjörð Bjarmason
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=xmqqmt9y55py.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=avarab@gmail.com \
--cc=edecosta@mathworks.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--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 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).