* [PATCH v2 1/2] fsmonitor: option to allow fsmonitor to run against network-mounted repos
2022-08-11 15:57 ` [PATCH v2 0/2] Option to allow fsmonitor to run against repos on network file systems Eric DeCosta via GitGitGadget
@ 2022-08-11 15:57 ` Eric DeCosta via GitGitGadget
2022-08-11 15:57 ` [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior Eric DeCosta via GitGitGadget
2022-08-11 18:32 ` [PATCH v3] fsmonitor: option to allow fsmonitor to run against network-mounted repos Eric DeCosta via GitGitGadget
2 siblings, 0 replies; 20+ messages in thread
From: Eric DeCosta via GitGitGadget @ 2022-08-11 15:57 UTC (permalink / raw)
To: git; +Cc: Eric DeCosta, Eric DeCosta
From: Eric DeCosta <edecosta@mathworks.com>
Though perhaps not common, there are uses cases where users have large,
network-mounted repos. Having the ability to run fsmonitor against
network paths would benefit those users.
Most modern Samba-based filers have the necessary support to enable
fsmonitor on network-mounted repos. As a first step towards enabling
fsmonitor to work against network-mounted repos, introduce a
configuration option, 'fsmonitor.allowRemote'. Setting this option to
true will override the default behavior (erroring-out) when a
network-mounted repo is detected by fsmonitor.
Additionally, as part of this first step, monitoring of network-mounted
repos will be restricted to those mounted over SMB regardless of the
value of 'fsmonitor.allowRemote' until more extensive testing can be
performed.
Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
---
compat/fsmonitor/fsm-settings-win32.c | 59 ++++++++++++++++++++++++++-
1 file changed, 58 insertions(+), 1 deletion(-)
diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
index 907655720bb..d120e4710cf 100644
--- a/compat/fsmonitor/fsm-settings-win32.c
+++ b/compat/fsmonitor/fsm-settings-win32.c
@@ -24,6 +24,58 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
return FSMONITOR_REASON_OK;
}
+/*
+ * Check if monitoring remote working directories is allowed.
+ *
+ * By default monitoring remote working directories is not allowed,
+ * but users may override this behavior in enviroments where they
+ * have proper support.
+*/
+static enum fsmonitor_reason check_allow_remote(struct repository *r)
+{
+ int allow;
+
+ if (repo_config_get_bool(r, "fsmonitor.allowremote", &allow) || !allow)
+ return FSMONITOR_REASON_REMOTE;
+
+ return FSMONITOR_REASON_OK;
+}
+
+/*
+ * Check if the remote working directory is mounted via SMB
+ *
+ * For now, remote working directories are only supported via SMB mounts
+*/
+static enum fsmonitor_reason check_smb(wchar_t *wpath)
+{
+ HANDLE h;
+ FILE_REMOTE_PROTOCOL_INFO proto_info;
+
+ h = CreateFileW(wpath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING,
+ FILE_FLAG_BACKUP_SEMANTICS, NULL);
+
+ if (h == INVALID_HANDLE_VALUE) {
+ error(_("[GLE %ld] unable to open for read '%ls'"),
+ GetLastError(), wpath);
+ return FSMONITOR_REASON_ERROR;
+ }
+
+ if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
+ &proto_info, sizeof(proto_info))) {
+ error(_("[GLE %ld] unable to get protocol information for '%ls'"),
+ GetLastError(), wpath);
+ CloseHandle(h);
+ return FSMONITOR_REASON_ERROR;
+ }
+
+ CloseHandle(h);
+
+ if (proto_info.Protocol == WNNC_NET_SMB)
+ return FSMONITOR_REASON_OK;
+
+ return FSMONITOR_REASON_ERROR;
+}
+
/*
* Remote working directories are problematic for FSMonitor.
*
@@ -76,6 +128,7 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
*/
static enum fsmonitor_reason check_remote(struct repository *r)
{
+ enum fsmonitor_reason reason;
wchar_t wpath[MAX_PATH];
wchar_t wfullpath[MAX_PATH];
size_t wlen;
@@ -115,7 +168,11 @@ static enum fsmonitor_reason check_remote(struct repository *r)
trace_printf_key(&trace_fsmonitor,
"check_remote('%s') true",
r->worktree);
- return FSMONITOR_REASON_REMOTE;
+
+ reason = check_smb(wfullpath);
+ if (reason != FSMONITOR_REASON_OK)
+ return reason;
+ return check_allow_remote(r);
}
return FSMONITOR_REASON_OK;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior
2022-08-11 15:57 ` [PATCH v2 0/2] Option to allow fsmonitor to run against repos on network file systems Eric DeCosta via GitGitGadget
2022-08-11 15:57 ` [PATCH v2 1/2] fsmonitor: option to allow fsmonitor to run against network-mounted repos Eric DeCosta via GitGitGadget
@ 2022-08-11 15:57 ` Eric DeCosta via GitGitGadget
2022-08-11 16:53 ` Junio C Hamano
2022-08-11 18:32 ` [PATCH v3] fsmonitor: option to allow fsmonitor to run against network-mounted repos Eric DeCosta via GitGitGadget
2 siblings, 1 reply; 20+ messages in thread
From: Eric DeCosta via GitGitGadget @ 2022-08-11 15:57 UTC (permalink / raw)
To: git; +Cc: Eric DeCosta, Eric DeCosta
From: Eric DeCosta <edecosta@mathworks.com>
Reworked the logic around fsmonitor.allowRemote such that if
allowRemote is set it will determine if monitoring the remote
worktree is allowed.
Get remote protocoal information; if this fails report an error else
print it if tracing is enabled.
Fixed fomratting issues.
Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
---
compat/fsmonitor/fsm-settings-win32.c | 57 ++++++++++++++++-----------
1 file changed, 33 insertions(+), 24 deletions(-)
diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
index d120e4710cf..32c0695c6c1 100644
--- a/compat/fsmonitor/fsm-settings-win32.c
+++ b/compat/fsmonitor/fsm-settings-win32.c
@@ -27,53 +27,55 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
/*
* Check if monitoring remote working directories is allowed.
*
- * By default monitoring remote working directories is not allowed,
- * but users may override this behavior in enviroments where they
- * have proper support.
-*/
-static enum fsmonitor_reason check_allow_remote(struct repository *r)
+ * By default, monitoring remote working directories is
+ * disabled unless on a network filesystem that is known to
+ * behave well. Users may override this behavior in enviroments where
+ * they have proper support.
+ */
+static int check_config_allowremote(struct repository *r)
{
int allow;
- if (repo_config_get_bool(r, "fsmonitor.allowremote", &allow) || !allow)
- return FSMONITOR_REASON_REMOTE;
+ if (!repo_config_get_bool(r, "fsmonitor.allowremote", &allow))
+ return allow;
- return FSMONITOR_REASON_OK;
+ return -1; /* fsmonitor.allowremote not set */
}
/*
- * Check if the remote working directory is mounted via SMB
+ * Check remote working directory protocol.
*
- * For now, remote working directories are only supported via SMB mounts
-*/
-static enum fsmonitor_reason check_smb(wchar_t *wpath)
+ * Error if client machine cannot get remote protocol information.
+ */
+static void check_remote_protocol(wchar_t *wpath)
{
HANDLE h;
FILE_REMOTE_PROTOCOL_INFO proto_info;
h = CreateFileW(wpath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING,
- FILE_FLAG_BACKUP_SEMANTICS, NULL);
+ FILE_FLAG_BACKUP_SEMANTICS, NULL);
if (h == INVALID_HANDLE_VALUE) {
error(_("[GLE %ld] unable to open for read '%ls'"),
GetLastError(), wpath);
- return FSMONITOR_REASON_ERROR;
+ return;
}
if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
- &proto_info, sizeof(proto_info))) {
+ &proto_info, sizeof(proto_info))) {
error(_("[GLE %ld] unable to get protocol information for '%ls'"),
GetLastError(), wpath);
CloseHandle(h);
- return FSMONITOR_REASON_ERROR;
+ return;
}
CloseHandle(h);
- if (proto_info.Protocol == WNNC_NET_SMB)
- return FSMONITOR_REASON_OK;
+ trace_printf_key(&trace_fsmonitor,
+ "check_remote_protocol('%ls') remote protocol %#8.8lx",
+ wpath, proto_info.Protocol);
- return FSMONITOR_REASON_ERROR;
+ return;
}
/*
@@ -128,7 +130,6 @@ static enum fsmonitor_reason check_smb(wchar_t *wpath)
*/
static enum fsmonitor_reason check_remote(struct repository *r)
{
- enum fsmonitor_reason reason;
wchar_t wpath[MAX_PATH];
wchar_t wfullpath[MAX_PATH];
size_t wlen;
@@ -169,10 +170,18 @@ static enum fsmonitor_reason check_remote(struct repository *r)
"check_remote('%s') true",
r->worktree);
- reason = check_smb(wfullpath);
- if (reason != FSMONITOR_REASON_OK)
- return reason;
- return check_allow_remote(r);
+ check_remote_protocol(wfullpath);
+
+ switch (check_config_allowremote(r)) {
+ case 0: /* config overrides and disables */
+ return FSMONITOR_REASON_REMOTE;
+ case 1: /* config overrides and enables */
+ return FSMONITOR_REASON_OK;
+ default:
+ break; /* config has no opinion */
+ }
+
+ return FSMONITOR_REASON_REMOTE;
}
return FSMONITOR_REASON_OK;
--
gitgitgadget
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior
2022-08-11 15:57 ` [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior Eric DeCosta via GitGitGadget
@ 2022-08-11 16:53 ` Junio C Hamano
2022-08-11 17:49 ` Eric D
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2022-08-11 16:53 UTC (permalink / raw)
To: Eric DeCosta via GitGitGadget; +Cc: git, Eric DeCosta
"Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Eric DeCosta <edecosta@mathworks.com>
>
> Reworked the logic around fsmonitor.allowRemote such that if
> allowRemote is set it will determine if monitoring the remote
> worktree is allowed.
>
> Get remote protocoal information; if this fails report an error else
> print it if tracing is enabled.
>
> Fixed fomratting issues.
The end result (i.e. HEAD^{tree} of the branch you developed these
two patches on) may be good (I haven't checked), but it is not how
we fix problems in an earlier attempt in this project by keeping the
faulty commit(s) on the bottom and piling "oops, that was wrong, and
here is a fix-up" commit(s) on top.
Once you are happy with the end result, use "rebase -i" to clean-up
the history leading to that end result. The goal is to pretend as
if you were a perfect human, more perfect than your actual self, who
came up with an ideal patch without making mistakes that need to be
corrected with "fix-up" commits. In this particular case, you'd
most likely want to end up with a single commit, so squashing them
together and fixing up the log message might be all you need to do.
When you work on a more elaborate topic, you may also want to split
or reorder original commits to present a logical progression towards
the end result. "rebase -i" is a good tool to help you do so.
I am not a user of GitGitGadget myself, but if I recall correctly,
you should be able to force-push the result of such a clean-up to
update the pull-request, to trigger a new iteration to be sent to
the list.
Thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior
2022-08-11 16:53 ` Junio C Hamano
@ 2022-08-11 17:49 ` Eric D
2022-08-11 17:53 ` Junio C Hamano
0 siblings, 1 reply; 20+ messages in thread
From: Eric D @ 2022-08-11 17:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric DeCosta via GitGitGadget, git, Eric DeCosta
Well, needless to say I wasn't expecting GitGitGadget to do what it
did.I had squashed things down to just two commits and forced-pushed
the second commit thinking that just the relevant stuff from the
second commit would show up in the next patch. Obviously that didn't
happen. Sorry about that.
I can certainly squash it down to just one commit and force-push that.
On Thu, Aug 11, 2022 at 1:17 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Eric DeCosta <edecosta@mathworks.com>
> >
> > Reworked the logic around fsmonitor.allowRemote such that if
> > allowRemote is set it will determine if monitoring the remote
> > worktree is allowed.
> >
> > Get remote protocoal information; if this fails report an error else
> > print it if tracing is enabled.
> >
> > Fixed fomratting issues.
>
> The end result (i.e. HEAD^{tree} of the branch you developed these
> two patches on) may be good (I haven't checked), but it is not how
> we fix problems in an earlier attempt in this project by keeping the
> faulty commit(s) on the bottom and piling "oops, that was wrong, and
> here is a fix-up" commit(s) on top.
>
> Once you are happy with the end result, use "rebase -i" to clean-up
> the history leading to that end result. The goal is to pretend as
> if you were a perfect human, more perfect than your actual self, who
> came up with an ideal patch without making mistakes that need to be
> corrected with "fix-up" commits. In this particular case, you'd
> most likely want to end up with a single commit, so squashing them
> together and fixing up the log message might be all you need to do.
> When you work on a more elaborate topic, you may also want to split
> or reorder original commits to present a logical progression towards
> the end result. "rebase -i" is a good tool to help you do so.
>
> I am not a user of GitGitGadget myself, but if I recall correctly,
> you should be able to force-push the result of such a clean-up to
> update the pull-request, to trigger a new iteration to be sent to
> the list.
>
> Thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior
2022-08-11 17:49 ` Eric D
@ 2022-08-11 17:53 ` Junio C Hamano
2022-08-11 17:58 ` Eric D
0 siblings, 1 reply; 20+ messages in thread
From: Junio C Hamano @ 2022-08-11 17:53 UTC (permalink / raw)
To: Eric D; +Cc: Eric DeCosta via GitGitGadget, git, Eric DeCosta
Eric D <eric.decosta@gmail.com> writes:
> Well, needless to say I wasn't expecting GitGitGadget to do what it
> did.I had squashed things down to just two commits and forced-pushed
> the second commit thinking that just the relevant stuff from the
> second commit would show up in the next patch. Obviously that didn't
> happen. Sorry about that.
Oh, sorry to hear that. If your ideal "logical progression" needs
two commits, then please do present the series that way. What GGG
sent out was apparently not that (i.e. the same one from v1 with
full of fix-ups for it in 2/2).
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior
2022-08-11 17:53 ` Junio C Hamano
@ 2022-08-11 17:58 ` Eric D
0 siblings, 0 replies; 20+ messages in thread
From: Eric D @ 2022-08-11 17:58 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Eric DeCosta via GitGitGadget, git, Eric DeCosta
Given that, in the end, the change is rather small and involves just
one file, having it be just one commit is fine. Perhaps my next lesson
to learn is to generate and send the patch sets myself, but that will
be for another time.
Thank you for all your patience, it makes a total noob like me feel welcome.
On Thu, Aug 11, 2022 at 1:53 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Eric D <eric.decosta@gmail.com> writes:
>
> > Well, needless to say I wasn't expecting GitGitGadget to do what it
> > did.I had squashed things down to just two commits and forced-pushed
> > the second commit thinking that just the relevant stuff from the
> > second commit would show up in the next patch. Obviously that didn't
> > happen. Sorry about that.
>
> Oh, sorry to hear that. If your ideal "logical progression" needs
> two commits, then please do present the series that way. What GGG
> sent out was apparently not that (i.e. the same one from v1 with
> full of fix-ups for it in 2/2).
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v3] fsmonitor: option to allow fsmonitor to run against network-mounted repos
2022-08-11 15:57 ` [PATCH v2 0/2] Option to allow fsmonitor to run against repos on network file systems Eric DeCosta via GitGitGadget
2022-08-11 15:57 ` [PATCH v2 1/2] fsmonitor: option to allow fsmonitor to run against network-mounted repos Eric DeCosta via GitGitGadget
2022-08-11 15:57 ` [PATCH v2 2/2] fsmonitor.allowRemote now overrides default behavior Eric DeCosta via GitGitGadget
@ 2022-08-11 18:32 ` Eric DeCosta via GitGitGadget
2022-08-11 19:33 ` Junio C Hamano
2022-08-11 23:57 ` [PATCH v4] " Eric DeCosta via GitGitGadget
2 siblings, 2 replies; 20+ messages in thread
From: Eric DeCosta via GitGitGadget @ 2022-08-11 18:32 UTC (permalink / raw)
To: git; +Cc: Eric DeCosta, Eric DeCosta
From: Eric DeCosta <edecosta@mathworks.com>
Though perhaps not common, there are uses cases where users have large,
network-mounted repos. Having the ability to run fsmonitor against
network paths would benefit those users.
Most modern Samba-based filers have the necessary support to enable
fsmonitor on network-mounted repos. As a first step towards enabling
fsmonitor to work against network-mounted repos, introduce a
configuration option, 'fsmonitor.allowRemote'. Setting this option to
true will override the default behavior (erroring-out) when a
network-mounted repo is detected by fsmonitor.
Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
---
Option to allow fsmonitor to run against repos on network file systems
cc: Eric D eric.decosta@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1317%2Fedecosta-mw%2Fmaster-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1317/edecosta-mw/master-v3
Pull-Request: https://github.com/gitgitgadget/git/pull/1317
Range-diff vs v2:
1: 7e67ce8c944 ! 1: 6c5f176cbee fsmonitor: option to allow fsmonitor to run against network-mounted repos
@@ Commit message
true will override the default behavior (erroring-out) when a
network-mounted repo is detected by fsmonitor.
- Additionally, as part of this first step, monitoring of network-mounted
- repos will be restricted to those mounted over SMB regardless of the
- value of 'fsmonitor.allowRemote' until more extensive testing can be
- performed.
-
Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
## compat/fsmonitor/fsm-settings-win32.c ##
@@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
+/*
+ * Check if monitoring remote working directories is allowed.
+ *
-+ * By default monitoring remote working directories is not allowed,
-+ * but users may override this behavior in enviroments where they
-+ * have proper support.
-+*/
-+static enum fsmonitor_reason check_allow_remote(struct repository *r)
++ * By default, monitoring remote working directories is
++ * disabled unless on a network filesystem that is known to
++ * behave well. Users may override this behavior in enviroments where
++ * they have proper support.
++ */
++static int check_config_allowremote(struct repository *r)
+{
+ int allow;
+
-+ if (repo_config_get_bool(r, "fsmonitor.allowremote", &allow) || !allow)
-+ return FSMONITOR_REASON_REMOTE;
++ if (!repo_config_get_bool(r, "fsmonitor.allowremote", &allow))
++ return allow;
+
-+ return FSMONITOR_REASON_OK;
++ return -1; /* fsmonitor.allowremote not set */
+}
+
+/*
-+ * Check if the remote working directory is mounted via SMB
++ * Check remote working directory protocol.
+ *
-+ * For now, remote working directories are only supported via SMB mounts
-+*/
-+static enum fsmonitor_reason check_smb(wchar_t *wpath)
++ * Error if client machine cannot get remote protocol information.
++ */
++static void check_remote_protocol(wchar_t *wpath)
+{
+ HANDLE h;
+ FILE_REMOTE_PROTOCOL_INFO proto_info;
+
+ h = CreateFileW(wpath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING,
-+ FILE_FLAG_BACKUP_SEMANTICS, NULL);
++ FILE_FLAG_BACKUP_SEMANTICS, NULL);
+
+ if (h == INVALID_HANDLE_VALUE) {
+ error(_("[GLE %ld] unable to open for read '%ls'"),
+ GetLastError(), wpath);
-+ return FSMONITOR_REASON_ERROR;
++ return;
+ }
+
+ if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
-+ &proto_info, sizeof(proto_info))) {
++ &proto_info, sizeof(proto_info))) {
+ error(_("[GLE %ld] unable to get protocol information for '%ls'"),
+ GetLastError(), wpath);
+ CloseHandle(h);
-+ return FSMONITOR_REASON_ERROR;
++ return;
+ }
+
+ CloseHandle(h);
+
-+ if (proto_info.Protocol == WNNC_NET_SMB)
-+ return FSMONITOR_REASON_OK;
++ trace_printf_key(&trace_fsmonitor,
++ "check_remote_protocol('%ls') remote protocol %#8.8lx",
++ wpath, proto_info.Protocol);
+
-+ return FSMONITOR_REASON_ERROR;
++ return;
+}
+
/*
* Remote working directories are problematic for FSMonitor.
*
-@@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4git(struct repository *r)
- */
- static enum fsmonitor_reason check_remote(struct repository *r)
- {
-+ enum fsmonitor_reason reason;
- wchar_t wpath[MAX_PATH];
- wchar_t wfullpath[MAX_PATH];
- size_t wlen;
@@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_remote(struct repository *r)
trace_printf_key(&trace_fsmonitor,
"check_remote('%s') true",
r->worktree);
-- return FSMONITOR_REASON_REMOTE;
+
-+ reason = check_smb(wfullpath);
-+ if (reason != FSMONITOR_REASON_OK)
-+ return reason;
-+ return check_allow_remote(r);
++ check_remote_protocol(wfullpath);
++
++ switch (check_config_allowremote(r)) {
++ case 0: /* config overrides and disables */
++ return FSMONITOR_REASON_REMOTE;
++ case 1: /* config overrides and enables */
++ return FSMONITOR_REASON_OK;
++ default:
++ break; /* config has no opinion */
++ }
++
+ return FSMONITOR_REASON_REMOTE;
}
- return FSMONITOR_REASON_OK;
2: 7a071c9e6be < -: ----------- fsmonitor.allowRemote now overrides default behavior
compat/fsmonitor/fsm-settings-win32.c | 66 +++++++++++++++++++++++++++
1 file changed, 66 insertions(+)
diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
index 907655720bb..32c0695c6c1 100644
--- a/compat/fsmonitor/fsm-settings-win32.c
+++ b/compat/fsmonitor/fsm-settings-win32.c
@@ -24,6 +24,60 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
return FSMONITOR_REASON_OK;
}
+/*
+ * Check if monitoring remote working directories is allowed.
+ *
+ * By default, monitoring remote working directories is
+ * disabled unless on a network filesystem that is known to
+ * behave well. Users may override this behavior in enviroments where
+ * they have proper support.
+ */
+static int check_config_allowremote(struct repository *r)
+{
+ int allow;
+
+ if (!repo_config_get_bool(r, "fsmonitor.allowremote", &allow))
+ return allow;
+
+ return -1; /* fsmonitor.allowremote not set */
+}
+
+/*
+ * Check remote working directory protocol.
+ *
+ * Error if client machine cannot get remote protocol information.
+ */
+static void check_remote_protocol(wchar_t *wpath)
+{
+ HANDLE h;
+ FILE_REMOTE_PROTOCOL_INFO proto_info;
+
+ h = CreateFileW(wpath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING,
+ FILE_FLAG_BACKUP_SEMANTICS, NULL);
+
+ if (h == INVALID_HANDLE_VALUE) {
+ error(_("[GLE %ld] unable to open for read '%ls'"),
+ GetLastError(), wpath);
+ return;
+ }
+
+ if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
+ &proto_info, sizeof(proto_info))) {
+ error(_("[GLE %ld] unable to get protocol information for '%ls'"),
+ GetLastError(), wpath);
+ CloseHandle(h);
+ return;
+ }
+
+ CloseHandle(h);
+
+ trace_printf_key(&trace_fsmonitor,
+ "check_remote_protocol('%ls') remote protocol %#8.8lx",
+ wpath, proto_info.Protocol);
+
+ return;
+}
+
/*
* Remote working directories are problematic for FSMonitor.
*
@@ -115,6 +169,18 @@ static enum fsmonitor_reason check_remote(struct repository *r)
trace_printf_key(&trace_fsmonitor,
"check_remote('%s') true",
r->worktree);
+
+ check_remote_protocol(wfullpath);
+
+ switch (check_config_allowremote(r)) {
+ case 0: /* config overrides and disables */
+ return FSMONITOR_REASON_REMOTE;
+ case 1: /* config overrides and enables */
+ return FSMONITOR_REASON_OK;
+ default:
+ break; /* config has no opinion */
+ }
+
return FSMONITOR_REASON_REMOTE;
}
base-commit: c50926e1f48891e2671e1830dbcd2912a4563450
--
gitgitgadget
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v3] fsmonitor: option to allow fsmonitor to run against network-mounted repos
2022-08-11 18:32 ` [PATCH v3] fsmonitor: option to allow fsmonitor to run against network-mounted repos Eric DeCosta via GitGitGadget
@ 2022-08-11 19:33 ` Junio C Hamano
2022-08-11 23:57 ` [PATCH v4] " Eric DeCosta via GitGitGadget
1 sibling, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2022-08-11 19:33 UTC (permalink / raw)
To: Eric DeCosta via GitGitGadget; +Cc: git, Eric DeCosta
"Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Eric DeCosta <edecosta@mathworks.com>
>
> Though perhaps not common, there are uses cases where users have large,
> network-mounted repos. Having the ability to run fsmonitor against
> network paths would benefit those users.
>
> Most modern Samba-based filers have the necessary support to enable
> fsmonitor on network-mounted repos. As a first step towards enabling
> fsmonitor to work against network-mounted repos, introduce a
> configuration option, 'fsmonitor.allowRemote'. Setting this option to
> true will override the default behavior (erroring-out) when a
> network-mounted repo is detected by fsmonitor.
>
> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> ---
> ...
> compat/fsmonitor/fsm-settings-win32.c | 66 +++++++++++++++++++++++++++
> 1 file changed, 66 insertions(+)
>
> diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
> index 907655720bb..32c0695c6c1 100644
> --- a/compat/fsmonitor/fsm-settings-win32.c
> +++ b/compat/fsmonitor/fsm-settings-win32.c
> @@ -24,6 +24,60 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
> return FSMONITOR_REASON_OK;
> }
>
> +/*
> + * Check if monitoring remote working directories is allowed.
> + *
> + * By default, monitoring remote working directories is
> + * disabled unless on a network filesystem that is known to
> + * behave well. Users may override this behavior in enviroments where
> + * they have proper support.
> + */
After applying this patch, "unless on a network filesystem ..." part
is not exactly in effect yet; we could say that we start with no
known-to-behave-well network filesystems, but we can then update the
above comment when we start to know of at least one good one.
> +/*
> + * Check remote working directory protocol.
> + *
> + * Error if client machine cannot get remote protocol information.
> + */
Good, but void means that the caller of this function does not know
when we detected an error. Perhaps return -1 on error, return 0 on
"not error", so that we can return 1 when we learn to recognize
"known to behave well" network filesystem to tell the caller?
That is,
> +static void check_remote_protocol(wchar_t *wpath)
"void" -> "int"
> +{
> + HANDLE h;
> + FILE_REMOTE_PROTOCOL_INFO proto_info;
> +
> + h = CreateFileW(wpath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING,
> + FILE_FLAG_BACKUP_SEMANTICS, NULL);
> +
> + if (h == INVALID_HANDLE_VALUE) {
> + error(_("[GLE %ld] unable to open for read '%ls'"),
> + GetLastError(), wpath);
> + return;
"return" -> "return -1"
> + }
> +
> + if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
> + &proto_info, sizeof(proto_info))) {
> + error(_("[GLE %ld] unable to get protocol information for '%ls'"),
> + GetLastError(), wpath);
> + CloseHandle(h);
> + return;
"return" -> "return -1"
> + }
> +
> + CloseHandle(h);
> +
> + trace_printf_key(&trace_fsmonitor,
> + "check_remote_protocol('%ls') remote protocol %#8.8lx",
> + wpath, proto_info.Protocol);
> +
> + return;
"return" -> "return 0" (or "-1")
> +}
> +
> /*
> * Remote working directories are problematic for FSMonitor.
> *
> @@ -115,6 +169,18 @@ static enum fsmonitor_reason check_remote(struct repository *r)
> trace_printf_key(&trace_fsmonitor,
> "check_remote('%s') true",
> r->worktree);
> +
> + check_remote_protocol(wfullpath);
And here
ret = check_remote_protocol(wfullpath);
if (ret < 0)
/* definitely an error */
return FSMONITOR_REASON_ERROR;
and then we can fall thru the non-error case below. We'd of course
need to declare "int ret" at the beginning of the function.
> + switch (check_config_allowremote(r)) {
> + case 0: /* config overrides and disables */
> + return FSMONITOR_REASON_REMOTE;
> + case 1: /* config overrides and enables */
> + return FSMONITOR_REASON_OK;
> + default:
> + break; /* config has no opinion */
> + }
> +
> return FSMONITOR_REASON_REMOTE;
> }
In the future, when this "first step" graduates to the upcoming
release, we may want to have a follow-up enhancement patch that
changes the code like so:
* we recognize ones like SMB in check_remote_protocol() as "known
to be good", and return 1 from there
* after the "switch" above determies that the configuration file
does not have any opinion, instead of unconditionally returning
REASON_REMOTE to refuse the request, pay attention to "ret", e.g.
something like
- return FSMONITOR_REASON_REMOTE;
+ if (!ret)
+ return FSMONITOR_REASON_REMOTE;
+ else /* known to be good ones */
+ return FSMONITOR_REASON_OK;
When we do so, we'd resurrect the "unless on a network filesystem
that is known to behave well" comment. What this last part does is
exactly that.
Thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v4] fsmonitor: option to allow fsmonitor to run against network-mounted repos
2022-08-11 18:32 ` [PATCH v3] fsmonitor: option to allow fsmonitor to run against network-mounted repos Eric DeCosta via GitGitGadget
2022-08-11 19:33 ` Junio C Hamano
@ 2022-08-11 23:57 ` Eric DeCosta via GitGitGadget
2022-08-12 18:23 ` Junio C Hamano
2022-08-15 16:01 ` Jeff Hostetler
1 sibling, 2 replies; 20+ messages in thread
From: Eric DeCosta via GitGitGadget @ 2022-08-11 23:57 UTC (permalink / raw)
To: git; +Cc: Eric DeCosta, Eric DeCosta
From: Eric DeCosta <edecosta@mathworks.com>
Though perhaps not common, there are uses cases where users have large,
network-mounted repos. Having the ability to run fsmonitor against
network paths would benefit those users.
Most modern Samba-based filers have the necessary support to enable
fsmonitor on network-mounted repos. As a first step towards enabling
fsmonitor to work against network-mounted repos, introduce a
configuration option, 'fsmonitor.allowRemote'. Setting this option to
true will override the default behavior (erroring-out) when a
network-mounted repo is detected by fsmonitor.
Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
---
Option to allow fsmonitor to run against repos on network file systems
cc: Eric D eric.decosta@gmail.com
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1317%2Fedecosta-mw%2Fmaster-v4
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1317/edecosta-mw/master-v4
Pull-Request: https://github.com/gitgitgadget/git/pull/1317
Range-diff vs v3:
1: 6c5f176cbee ! 1: 058dc400c8a fsmonitor: option to allow fsmonitor to run against network-mounted repos
@@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
+ * Check if monitoring remote working directories is allowed.
+ *
+ * By default, monitoring remote working directories is
-+ * disabled unless on a network filesystem that is known to
-+ * behave well. Users may override this behavior in enviroments where
++ * disabled. Users may override this behavior in enviroments where
+ * they have proper support.
+ */
+static int check_config_allowremote(struct repository *r)
@@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
+ *
+ * Error if client machine cannot get remote protocol information.
+ */
-+static void check_remote_protocol(wchar_t *wpath)
++static int check_remote_protocol(wchar_t *wpath)
+{
+ HANDLE h;
+ FILE_REMOTE_PROTOCOL_INFO proto_info;
@@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
+ if (h == INVALID_HANDLE_VALUE) {
+ error(_("[GLE %ld] unable to open for read '%ls'"),
+ GetLastError(), wpath);
-+ return;
++ return -1;
+ }
+
+ if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
@@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
+ error(_("[GLE %ld] unable to get protocol information for '%ls'"),
+ GetLastError(), wpath);
+ CloseHandle(h);
-+ return;
++ return -1;
+ }
+
+ CloseHandle(h);
@@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4gi
+ "check_remote_protocol('%ls') remote protocol %#8.8lx",
+ wpath, proto_info.Protocol);
+
-+ return;
++ return 0;
+}
+
/*
* Remote working directories are problematic for FSMonitor.
*
+@@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_vfs4git(struct repository *r)
+ */
+ static enum fsmonitor_reason check_remote(struct repository *r)
+ {
++ int ret;
+ wchar_t wpath[MAX_PATH];
+ wchar_t wfullpath[MAX_PATH];
+ size_t wlen;
@@ compat/fsmonitor/fsm-settings-win32.c: static enum fsmonitor_reason check_remote(struct repository *r)
trace_printf_key(&trace_fsmonitor,
"check_remote('%s') true",
r->worktree);
+
-+ check_remote_protocol(wfullpath);
++ ret = check_remote_protocol(wfullpath);
++ if (ret < 0)
++ return FSMONITOR_REASON_ERROR;
+
+ switch (check_config_allowremote(r)) {
+ case 0: /* config overrides and disables */
compat/fsmonitor/fsm-settings-win32.c | 68 +++++++++++++++++++++++++++
1 file changed, 68 insertions(+)
diff --git a/compat/fsmonitor/fsm-settings-win32.c b/compat/fsmonitor/fsm-settings-win32.c
index 907655720bb..e5ec5b0a9f7 100644
--- a/compat/fsmonitor/fsm-settings-win32.c
+++ b/compat/fsmonitor/fsm-settings-win32.c
@@ -24,6 +24,59 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
return FSMONITOR_REASON_OK;
}
+/*
+ * Check if monitoring remote working directories is allowed.
+ *
+ * By default, monitoring remote working directories is
+ * disabled. Users may override this behavior in enviroments where
+ * they have proper support.
+ */
+static int check_config_allowremote(struct repository *r)
+{
+ int allow;
+
+ if (!repo_config_get_bool(r, "fsmonitor.allowremote", &allow))
+ return allow;
+
+ return -1; /* fsmonitor.allowremote not set */
+}
+
+/*
+ * Check remote working directory protocol.
+ *
+ * Error if client machine cannot get remote protocol information.
+ */
+static int check_remote_protocol(wchar_t *wpath)
+{
+ HANDLE h;
+ FILE_REMOTE_PROTOCOL_INFO proto_info;
+
+ h = CreateFileW(wpath, GENERIC_READ, FILE_SHARE_READ, NULL, OPEN_EXISTING,
+ FILE_FLAG_BACKUP_SEMANTICS, NULL);
+
+ if (h == INVALID_HANDLE_VALUE) {
+ error(_("[GLE %ld] unable to open for read '%ls'"),
+ GetLastError(), wpath);
+ return -1;
+ }
+
+ if (!GetFileInformationByHandleEx(h, FileRemoteProtocolInfo,
+ &proto_info, sizeof(proto_info))) {
+ error(_("[GLE %ld] unable to get protocol information for '%ls'"),
+ GetLastError(), wpath);
+ CloseHandle(h);
+ return -1;
+ }
+
+ CloseHandle(h);
+
+ trace_printf_key(&trace_fsmonitor,
+ "check_remote_protocol('%ls') remote protocol %#8.8lx",
+ wpath, proto_info.Protocol);
+
+ return 0;
+}
+
/*
* Remote working directories are problematic for FSMonitor.
*
@@ -76,6 +129,7 @@ static enum fsmonitor_reason check_vfs4git(struct repository *r)
*/
static enum fsmonitor_reason check_remote(struct repository *r)
{
+ int ret;
wchar_t wpath[MAX_PATH];
wchar_t wfullpath[MAX_PATH];
size_t wlen;
@@ -115,6 +169,20 @@ static enum fsmonitor_reason check_remote(struct repository *r)
trace_printf_key(&trace_fsmonitor,
"check_remote('%s') true",
r->worktree);
+
+ ret = check_remote_protocol(wfullpath);
+ if (ret < 0)
+ return FSMONITOR_REASON_ERROR;
+
+ switch (check_config_allowremote(r)) {
+ case 0: /* config overrides and disables */
+ return FSMONITOR_REASON_REMOTE;
+ case 1: /* config overrides and enables */
+ return FSMONITOR_REASON_OK;
+ default:
+ break; /* config has no opinion */
+ }
+
return FSMONITOR_REASON_REMOTE;
}
base-commit: c50926e1f48891e2671e1830dbcd2912a4563450
--
gitgitgadget
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v4] fsmonitor: option to allow fsmonitor to run against network-mounted repos
2022-08-11 23:57 ` [PATCH v4] " Eric DeCosta via GitGitGadget
@ 2022-08-12 18:23 ` Junio C Hamano
2022-08-15 16:01 ` Jeff Hostetler
1 sibling, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2022-08-12 18:23 UTC (permalink / raw)
To: Eric DeCosta via GitGitGadget; +Cc: git, Eric DeCosta
"Eric DeCosta via GitGitGadget" <gitgitgadget@gmail.com> writes:
> From: Eric DeCosta <edecosta@mathworks.com>
>
> Though perhaps not common, there are uses cases where users have large,
"uses cases" -> "use cases", probably.
> network-mounted repos. Having the ability to run fsmonitor against
> network paths would benefit those users.
>
> Most modern Samba-based filers have the necessary support to enable
> fsmonitor on network-mounted repos. As a first step towards enabling
> fsmonitor to work against network-mounted repos, introduce a
> configuration option, 'fsmonitor.allowRemote'. Setting this option to
> true will override the default behavior (erroring-out) when a
> network-mounted repo is detected by fsmonitor.
>
> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> ---
Will make the above typofix while queuing.
Thanks.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4] fsmonitor: option to allow fsmonitor to run against network-mounted repos
2022-08-11 23:57 ` [PATCH v4] " Eric DeCosta via GitGitGadget
2022-08-12 18:23 ` Junio C Hamano
@ 2022-08-15 16:01 ` Jeff Hostetler
2022-08-15 17:33 ` Junio C Hamano
1 sibling, 1 reply; 20+ messages in thread
From: Jeff Hostetler @ 2022-08-15 16:01 UTC (permalink / raw)
To: Eric DeCosta via GitGitGadget, git; +Cc: Eric DeCosta
On 8/11/22 7:57 PM, Eric DeCosta via GitGitGadget wrote:
> From: Eric DeCosta <edecosta@mathworks.com>
>
> Though perhaps not common, there are uses cases where users have large,
> network-mounted repos. Having the ability to run fsmonitor against
> network paths would benefit those users.
>
> Most modern Samba-based filers have the necessary support to enable
> fsmonitor on network-mounted repos. As a first step towards enabling
> fsmonitor to work against network-mounted repos, introduce a
> configuration option, 'fsmonitor.allowRemote'. Setting this option to
> true will override the default behavior (erroring-out) when a
> network-mounted repo is detected by fsmonitor.
V4 LGTM. Thanks for your persistence and attention to detail here.
Jeff
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v4] fsmonitor: option to allow fsmonitor to run against network-mounted repos
2022-08-15 16:01 ` Jeff Hostetler
@ 2022-08-15 17:33 ` Junio C Hamano
0 siblings, 0 replies; 20+ messages in thread
From: Junio C Hamano @ 2022-08-15 17:33 UTC (permalink / raw)
To: Jeff Hostetler; +Cc: Eric DeCosta via GitGitGadget, git, Eric DeCosta
Jeff Hostetler <git@jeffhostetler.com> writes:
> On 8/11/22 7:57 PM, Eric DeCosta via GitGitGadget wrote:
>> From: Eric DeCosta <edecosta@mathworks.com>
>> Though perhaps not common, there are uses cases where users have
>> large,
>> network-mounted repos. Having the ability to run fsmonitor against
>> network paths would benefit those users.
>> Most modern Samba-based filers have the necessary support to enable
>> fsmonitor on network-mounted repos. As a first step towards enabling
>> fsmonitor to work against network-mounted repos, introduce a
>> configuration option, 'fsmonitor.allowRemote'. Setting this option to
>> true will override the default behavior (erroring-out) when a
>> network-mounted repo is detected by fsmonitor.
>
> V4 LGTM. Thanks for your persistence and attention to detail here.
Thanks, both. Queued.
^ permalink raw reply [flat|nested] 20+ messages in thread