[v2,2/2] fsmonitor.allowRemote now overrides default behavior

Commit Message

Eric DeCosta Aug. 11, 2022, 3:57 p.m. UTC

From: Eric DeCosta <[e-mail]>

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 <[e-mail]>
---
 compat/fsmonitor/fsm-settings-win32.c | 57 ++++++++++++++++-----------
 1 file changed, 33 insertions(+), 24 deletions(-)

Comments

Junio C Hamano Aug. 11, 2022, 4:53 p.m. UTC | #1

"Eric DeCosta via GitGitGadget" <[e-mail]> writes:

> From: Eric DeCosta <[e-mail]>
>
> 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.

Eric D Aug. 11, 2022, 5:49 p.m. UTC | #2

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 <[e-mail]> wrote:
>
> "Eric DeCosta via GitGitGadget" <[e-mail]> writes:
>
> > From: Eric DeCosta <[e-mail]>
> >
> > 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.

Junio C Hamano Aug. 11, 2022, 5:53 p.m. UTC | #3

Eric D <[e-mail]> 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).

Eric D Aug. 11, 2022, 5:58 p.m. UTC | #4

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 <[e-mail]> wrote:
>
> Eric D <[e-mail]> 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).
>

Patch

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;