Re: [PATCH v15 1/6] fsmonitor: refactor filesystem checks to common interface

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Tue, Oct 04 2022, Eric DeCosta via GitGitGadget wrote:

> 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.

> +	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?

> +
> +	/*
> +	 * 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.

> +
> +	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" ?

> +
> +	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: