Re: [PATCH v5 02/30] fsmonitor-ipc: create client routines for git-fsmonitor--daemon
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Feb 17, 2022, 16:13 UTC
- Message-ID
- <nycvar.QRO.7.76.6.2202171710270.348@tvgsbejvaqbjf.bet>
- In-Reply-To
- <365964b766400654a9978092f8fe5cfaa525fd3b.1644612979.git.gitgitgadget@gmail.com>
Hi Jeff,
On Fri, 11 Feb 2022, Jeff Hostetler via GitGitGadget wrote:
Show 7 quoted lines
> From: Jeff Hostetler <jeffhost@microsoft.com> > > Create fsmonitor_ipc__*() client routines to spawn the built-in file > system monitor daemon and send it an IPC request using the `Simple > IPC` API. > > Stub in empty fsmonitor_ipc__*() functions for unsupported platforms.
This looks very straight-forward, thanks to your design of the Simple IPC.
Just one thing:
Show 53 quoted lines
> diff --git a/fsmonitor-ipc.c b/fsmonitor-ipc.c
> new file mode 100644
> index 00000000000..01c8c25bf50
> --- /dev/null
> +++ b/fsmonitor-ipc.c
> @@ -0,0 +1,171 @@
> +#include "cache.h"
> +#include "fsmonitor.h"
> +#include "simple-ipc.h"
> +#include "fsmonitor-ipc.h"
> +#include "run-command.h"
> +#include "strbuf.h"
> +#include "trace2.h"
> +
> +#ifdef HAVE_FSMONITOR_DAEMON_BACKEND
> +
> [...]
> +
> +#else
> +
> +/*
> + * A trivial implementation of the fsmonitor_ipc__ API for unsupported
> + * platforms.
> + */
> +
> +int fsmonitor_ipc__is_supported(void)
> +{
> + return 0;
> +}
> +
> +const char *fsmonitor_ipc__get_path(void)
> +{
> + return NULL;
> +}
> +
> +enum ipc_active_state fsmonitor_ipc__get_state(void)
> +{
> + return IPC_STATE__OTHER_ERROR;
> +}
> +
> +int fsmonitor_ipc__send_query(const char *since_token,
> + struct strbuf *answer)
> +{
> + return -1;
> +}
> +
> +int fsmonitor_ipc__send_command(const char *command,
> + struct strbuf *answer)
> +{
> + return -1;
> +}
> +
> +#endifNot a big deal (read: I won't mind if you leave the code as-is), but I usually find it easier to read the shorter, more trivial arm of conditional if/else arms first. In this instance, the dummy implementation for platforms that are not (yet) supported looks more trivial to me.
Thanks, Dscho