Re: [PATCH v15 1/6] fsmonitor: refactor filesystem checks to common interface
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jan 30, 2023, 09:37 UTC
- Message-ID
- <230130.86sffss5s5.gmgdl@evledraar.gmail.com>
- In-Reply-To
- <ec49a74086dee4545a3bb7b24c8da22e77aef579.1664904751.git.gitgitgadget@gmail.com>
On Tue, Oct 04 2022, Eric DeCosta via GitGitGadget wrote:
Show 53 quoted lines
> From: Eric DeCosta <edecosta@mathworks.com>
>
> Provide a common interface for getting basic filesystem information
> including filesystem type and whether the filesystem is remote.
>
> Refactor existing code for getting basic filesystem info and detecting
> remote file systems to the new interface.
>
> Refactor filesystem checks to leverage new interface. For macOS,
> error-out if the Unix Domain socket (UDS) file is on a remote
> filesystem.
>
> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> ---
> Makefile | 1 +
> compat/fsmonitor/fsm-path-utils-darwin.c | 43 ++++++
> compat/fsmonitor/fsm-path-utils-win32.c | 128 +++++++++++++++++
> compat/fsmonitor/fsm-settings-darwin.c | 69 +++------
> compat/fsmonitor/fsm-settings-win32.c | 172 +----------------------
> contrib/buildsystems/CMakeLists.txt | 2 +
> fsmonitor-path-utils.h | 26 ++++
> fsmonitor-settings.c | 50 +++++++
> 8 files changed, 272 insertions(+), 219 deletions(-)
> create mode 100644 compat/fsmonitor/fsm-path-utils-darwin.c
> create mode 100644 compat/fsmonitor/fsm-path-utils-win32.c
> create mode 100644 fsmonitor-path-utils.h
>
> diff --git a/Makefile b/Makefile
> index cac3452edb9..ffab427ea5b 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2044,6 +2044,7 @@ endif
> ifdef FSMONITOR_OS_SETTINGS
> COMPAT_CFLAGS += -DHAVE_FSMONITOR_OS_SETTINGS
> COMPAT_OBJS += compat/fsmonitor/fsm-settings-$(FSMONITOR_OS_SETTINGS).o
> + COMPAT_OBJS += compat/fsmonitor/fsm-path-utils-$(FSMONITOR_OS_SETTINGS).o
> endif
>
> ifeq ($(TCLTK_PATH),)
> diff --git a/compat/fsmonitor/fsm-path-utils-darwin.c b/compat/fsmonitor/fsm-path-utils-darwin.c
> new file mode 100644
> index 00000000000..d46d7f13538
> --- /dev/null
> +++ b/compat/fsmonitor/fsm-path-utils-darwin.c
> @@ -0,0 +1,43 @@
> +#include "fsmonitor.h"
> +#include "fsmonitor-path-utils.h"
> +#include <sys/param.h>
> +#include <sys/mount.h>
> +
> +int fsmonitor__get_fs_info(const char *path, struct fs_info *fs_info)
> +{
> + struct statfs fs;Should have a \n here after variable decls.
Show 16 quoted lines
> + if (statfs(path, &fs) == -1) {
> + int saved_errno = errno;
> + trace_printf_key(&trace_fsmonitor, "statfs('%s') failed: %s",
> + path, strerror(saved_errno));
> + errno = saved_errno;
> + return -1;
> + }
> +
> + trace_printf_key(&trace_fsmonitor,
> + "statfs('%s') [type 0x%08x][flags 0x%08x] '%s'",
> + path, fs.f_type, fs.f_flags, fs.f_fstypename);
> +
> + if (!(fs.f_flags & MNT_LOCAL))
> + fs_info->is_remote = 1;
> + else
> + fs_info->is_remote = 0;Instead:
fs_info->is_remote = !(fs.f_flags & MNT_LOCAL)
?
> +int fsmonitor__is_fs_remote(const char *path)
> +{
> + struct fs_info fs;Same style issue as above.
> + if (fsmonitor__get_fs_info(path, &fs)) > + return -1; > + > + free(fs.typename);
Can we get a "free_fs_info()" function or something instead, this is one of N codepaths where we now peek into that struct. If we ever add another field that needs malloc'ing altering all callers will be painful.
> + if (xutftowcs_path(wpath, path) < 0) {
> + return -1;
> + }Maybe drop the braces here as this code is being modified-while-moved anyway?
Show 19 quoted lines
> +
> + /*
> + * GetDriveTypeW() requires a final slash. We assume that the
> + * worktree pathname points to an actual directory.
> + */
> + wlen = wcslen(wpath);
> + if (wpath[wlen - 1] != L'\\' && wpath[wlen - 1] != L'/') {
> + wpath[wlen++] = L'\\';
> + wpath[wlen] = 0;
> + }
> +
> + /*
> + * Normalize the path. If nothing else, this converts forward
> + * slashes to backslashes. This is essential to get GetDriveTypeW()
> + * correctly handle some UNC "\\server\share\..." paths.
> + */
> + if (!GetFullPathNameW(wpath, MAX_PATH, wfullpath, NULL)) {
> + return -1;
> + }Here you have added needless braces while moving this, let's not do that.
Show 10 quoted lines
> +
> + driveType = GetDriveTypeW(wfullpath);
> + trace_printf_key(&trace_fsmonitor,
> + "DriveType '%s' L'%ls' (%u)",
> + path, wfullpath, driveType);
> +
> + if (driveType == DRIVE_REMOTE) {
> + fs_info->is_remote = 1;
> + if (check_remote_protocol(wfullpath) < 0)
> + return -1;Maybe this code should be more careful not to modify the passed-in struct if we're returning an error?
I.e. do this "return -1" before assigning "is_remote = 1"?
> + } else {
> + fs_info->is_remote = 0;
> + }Maybe just at the start: "struct fs_info = { 0 }" and skip this "else" ?Show 14 quoted lines
> +
> + trace_printf_key(&trace_fsmonitor,
> + "'%s' is_remote: %d",
> + path, fs_info->is_remote);
> +
> + return 0;
> +}
> +
> +int fsmonitor__is_fs_remote(const char *path)
> +{
> + struct fs_info fs;
> + if (fsmonitor__get_fs_info(path, &fs))
> + return -1;
> + return fs.is_remote;I find this and the resulting "switch/case" caller rather un-idiomatic, i.e. you end up checking 1/0/default.
Why not in your check_remote() instead:
int is_remote;
if (fsmonitor__check_fs_remote(r->worktree, &is_remote)) return FSMONITOR_REASON_ERROR; else if (!is_remote) return FSMONITOR_REASON_OK; ...
Where the "..." is the "repo_config_get_bool()" etc. code I suggest below.
I.e. having an "is" function return non-boolean is somewhat confusing, better to write to a variable (which the config API you're using does).
> +#ifdef HAVE_FSMONITOR_OS_SETTINGS
> +static enum fsmonitor_reason check_remote(struct repository *r)
> +{
> + int allow_remote = -1; /* -1 unset, 0 not allowed, 1 allowed */Let's not init this to -1, and instead...
> + int is_remote = fsmonitor__is_fs_remote(r->worktree);
> +
> + switch (is_remote) {
> + case 0:The usual coding style is to not indent the "case" further than the "switch".
> + return FSMONITOR_REASON_OK; > + case 1: > + repo_config_get_bool(r, "fsmonitor.allowremote", &allow_remote); > + if (allow_remote < 1)
...continued from above: We can scope this variable to this case statement, as "case 1: { int v; ... }", but furthermore you don't need to init it to -1 and check this -1 case if you just check the return value of repo_config_get_bool(), which IMO is also more obvious:
int v;
if (!repo...(..., &v)) return v ? FSMONITOR_REASON_OK : FSMONITOR_REASON_REMOTE: else return FSMONITOR_REASON_REMOTE;
I.e. you clearly separate the cases where it's un-init'd by checking the return value.
Maybe better (but would result in more churn later if you ever want to change the default):
int v;
return !repo...(..., &v) && v ? FSMONITOR_REASON_OK : FSMONITOR_REASON_REMOTE: