[PATCH] fsmonitor: handle differences between Windows named pipe functions

[PATCH] fsmonitor: handle differences between Windows named pipe functions

[Eric DeCosta via GitGitGadget]

From: Eric DeCosta <edecosta@mathworks.com>

CreateNamedPipeW is perfectly happy accepting pipe names with seemingly
embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly
returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully created
with CreateNamedPipeW, exists.

For example, this network path is problemmatic:
\\batfs-sb29-cifs\vmgr\sbs29\my_git_repo

In order to work around this issue, rather than using the path to the
worktree directly as the name of the pipe, instead use the hash of the
worktree path.

Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
---
    fsmonitor: handle differences between Windows named pipe functions
    
    CreateNamedPipeW is perfectly happy accepting pipe names with embedded
    escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly
    returns ERROR_FILE_NOT_FOUND when clearly a named pipe with the given
    name exists.
    
    For example, this path is problemmatic:
    \batfs-sb29-cifs\vmgr\sbs29\my_git_repo
    
    In order to work around this issue, rather than using the path to the
    worktree directly as the name of the pipe, instead use the hash of the
    worktree path.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1503%2Fedecosta-mw%2Ffsmonitor_windows-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1503/edecosta-mw/fsmonitor_windows-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/1503

 compat/simple-ipc/ipc-win32.c | 33 +++++++++++++++++----------------
 1 file changed, 17 insertions(+), 16 deletions(-)

diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index 20ea7b65e0b..867590abd10 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -1,4 +1,5 @@
 #include "cache.h"
+#include "hex.h"
 #include "simple-ipc.h"
 #include "strbuf.h"
 #include "pkt-line.h"
@@ -17,27 +18,27 @@
 static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc)
 {
 	int off = 0;
-	struct strbuf realpath = STRBUF_INIT;
-
-	if (!strbuf_realpath(&realpath, path, 0))
-		return -1;
+	int ret = 0;
+	git_SHA_CTX sha1ctx;
+	struct strbuf real_path = STRBUF_INIT;
+	struct strbuf pipe_name = STRBUF_INIT;
+	unsigned char hash[GIT_MAX_RAWSZ];
 
-	off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
-	if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
+	if (!strbuf_realpath(&real_path, path, 0))
 		return -1;
 
-	/* Handle drive prefix */
-	if (wpath[off] && wpath[off + 1] == L':') {
-		wpath[off + 1] = L'_';
-		off += 2;
-	}
+	git_SHA1_Init(&sha1ctx);
+	git_SHA1_Update(&sha1ctx, real_path.buf, real_path.len);
+	git_SHA1_Final(hash, &sha1ctx);
+	strbuf_release(&real_path);
 
-	for (; wpath[off]; off++)
-		if (wpath[off] == L'/')
-			wpath[off] = L'\\';
+	strbuf_addf(&pipe_name, "git-fsmonitor-%s", hash_to_hex(hash));
+	off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
+	if (xutftowcs(wpath + off, pipe_name.buf, alloc - off) < 0)
+		ret = -1;
 
-	strbuf_release(&realpath);
-	return 0;
+	strbuf_release(&pipe_name);
+	return ret;
 }
 
 static enum ipc_active_state get_active_state(wchar_t *pipe_path)

base-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e
-- 
gitgitgadget

Re: [PATCH] fsmonitor: handle differences between Windows named pipe functions

[Johannes Schindelin]

Hi Eric,

On Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote:

> From: Eric DeCosta <edecosta@mathworks.com>
>
> CreateNamedPipeW is perfectly happy accepting pipe names with seemingly
> embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly
> returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully created
> with CreateNamedPipeW, exists.
>
> For example, this network path is problemmatic:
> \\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
>
> In order to work around this issue, rather than using the path to the
> worktree directly as the name of the pipe, instead use the hash of the
> worktree path.

This is a rather large deviation from the other platforms, and it has an
unwanted side effect: Git for Windows' installer currently enumerates the
named pipes to figure out which FSMonitor instances need to be stopped
before upgrading. It has to do that because it would otherwise be unable
to overwrite the Git executable. And it needs to know the paths [*1*] so
that it can stop the FSMonitors gracefully (as opposed to terminating them
and risk interrupting them while they serve a reply to a Git client).

A much less intrusive change (that would not break Git for Windows'
installer) would be to replace backslashes by forward slashes in the path.

Please do that instead.

Ciao,
Johannes

Footnote *1*: If you think that the Git for Windows installer could simply
enumerate the process IDs of the FSMonitor instances and then look for
their working directories: That is not a viable option. Not only does the
Windows-based FSMonitor specifically switch to the parent directory (to
avoid blocking the removal of a Git directory merely by running the
process in said directory), even worse: there is no officially-sanctioned
way to query a running process' current working directory (the only way I
know of involves injecting a remote thread! Which will of course risk
being labeled as malware by current anti-malware solutions).

Re: [PATCH] fsmonitor: handle differences between Windows named pipe functions

[Jeff Hostetler]

On 3/27/23 7:37 AM, Johannes Schindelin wrote:
> Hi Eric,
> 
> On Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote:
> 
>> From: Eric DeCosta <edecosta@mathworks.com>
>>
>> CreateNamedPipeW is perfectly happy accepting pipe names with seemingly
>> embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly
>> returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully created
>> with CreateNamedPipeW, exists.
>>
>> For example, this network path is problemmatic:
>> \\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
>>
>> In order to work around this issue, rather than using the path to the
>> worktree directly as the name of the pipe, instead use the hash of the
>> worktree path.
> 
> This is a rather large deviation from the other platforms, and it has an
> unwanted side effect: Git for Windows' installer currently enumerates the
> named pipes to figure out which FSMonitor instances need to be stopped
> before upgrading. It has to do that because it would otherwise be unable
> to overwrite the Git executable. And it needs to know the paths [*1*] so
> that it can stop the FSMonitors gracefully (as opposed to terminating them
> and risk interrupting them while they serve a reply to a Git client).
> 
> A much less intrusive change (that would not break Git for Windows'
> installer) would be to replace backslashes by forward slashes in the path.
> 
> Please do that instead.
> 
> Ciao,
> Johannes
> 
> Footnote *1*: If you think that the Git for Windows installer could simply
> enumerate the process IDs of the FSMonitor instances and then look for
> their working directories: That is not a viable option. Not only does the
> Windows-based FSMonitor specifically switch to the parent directory (to
> avoid blocking the removal of a Git directory merely by running the
> process in said directory), even worse: there is no officially-sanctioned
> way to query a running process' current working directory (the only way I
> know of involves injecting a remote thread! Which will of course risk
> being labeled as malware by current anti-malware solutions).

Agreed. Please use forward slashes.

Thanks,
Jeff

Re: [PATCH] fsmonitor: handle differences between Windows named pipe functions

[Junio C Hamano]

Jeff Hostetler <git@jeffhostetler.com> writes:

>> This is a rather large deviation from the other platforms, and it
>> has an
>> unwanted side effect: Git for Windows' installer currently enumerates the
> ...
> Agreed. Please use forward slashes.

Thanks, both.

I'll mark the topic to be expecting a reroll, then.

RE: [PATCH] fsmonitor: handle differences between Windows named pipe functions

[Eric DeCosta]

> -----Original Message-----
> From: Jeff Hostetler <git@jeffhostetler.com>
> Sent: Monday, March 27, 2023 11:02 AM
> To: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Eric DeCosta via
> GitGitGadget <gitgitgadget@gmail.com>
> Cc: git@vger.kernel.org; Eric DeCosta <edecosta@mathworks.com>
> Subject: Re: [PATCH] fsmonitor: handle differences between Windows
> named pipe functions
> 
> 
> 
> On 3/27/23 7:37 AM, Johannes Schindelin wrote:
> > Hi Eric,
> >
> > On Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote:
> >
> >> From: Eric DeCosta <edecosta@mathworks.com>
> >>
> >> CreateNamedPipeW is perfectly happy accepting pipe names with
> >> seemingly embedded escape charcters (e.g. \b), WaitNamedPipeW is not
> >> and incorrectly returns ERROR_FILE_NOT_FOUND when clearly a named
> >> pipe, succesfully created with CreateNamedPipeW, exists.
> >>
> >> For example, this network path is problemmatic:
> >> \\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
> >>
> >> In order to work around this issue, rather than using the path to the
> >> worktree directly as the name of the pipe, instead use the hash of
> >> the worktree path.
> >
> > This is a rather large deviation from the other platforms, and it has
> > an unwanted side effect: Git for Windows' installer currently
> > enumerates the named pipes to figure out which FSMonitor instances
> > need to be stopped before upgrading. It has to do that because it
> > would otherwise be unable to overwrite the Git executable. And it
> > needs to know the paths [*1*] so that it can stop the FSMonitors
> > gracefully (as opposed to terminating them and risk interrupting them
> while they serve a reply to a Git client).
> >
> > A much less intrusive change (that would not break Git for Windows'
> > installer) would be to replace backslashes by forward slashes in the path.
> >
> > Please do that instead.
> >
> > Ciao,
> > Johannes
> >
> > Footnote *1*: If you think that the Git for Windows installer could
> > simply enumerate the process IDs of the FSMonitor instances and then
> > look for their working directories: That is not a viable option. Not
> > only does the Windows-based FSMonitor specifically switch to the
> > parent directory (to avoid blocking the removal of a Git directory
> > merely by running the process in said directory), even worse: there is
> > no officially-sanctioned way to query a running process' current
> > working directory (the only way I know of involves injecting a remote
> > thread! Which will of course risk being labeled as malware by current anti-
> malware solutions).
> 
> Agreed. Please use forward slashes.
> 
> Thanks,
> Jeff
> 

I have misdiagnosed the problem. Here are my most recent findings:

The problem is the leading double-slashes for repos that resolve to remote filesystems. i.e. if S:\myrepo resolves to \\some-server\some-dir\myrepo then the path passed to initialize_pipe_name is //some-server/some-dir/myrepo

Regardless of what type or how many slashes appear after \\.\pipe\ the pipe name, as reported from PowerShell, is always \\.\\pipe\\some-server\some-dir\myrepo and WaitNamedPipeW returns ERROR_FILE_NOT_FOUND

If I skip over the first leading slash an use /some-server/some-dir/myrepo I get the same pipe name as before, WaitNamedPipeW is happy and commands like git fsmonitor--daemon status correctly report that the daemon is watching the repo.

-Eric

Re: [PATCH] fsmonitor: handle differences between Windows named pipe functions

[Jeff Hostetler]

On 4/6/23 3:08 PM, Eric DeCosta wrote:
> 
> 
>> -----Original Message-----
>> From: Jeff Hostetler <git@jeffhostetler.com>
>> Sent: Monday, March 27, 2023 11:02 AM
>> To: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Eric DeCosta via
>> GitGitGadget <gitgitgadget@gmail.com>
>> Cc: git@vger.kernel.org; Eric DeCosta <edecosta@mathworks.com>
>> Subject: Re: [PATCH] fsmonitor: handle differences between Windows
>> named pipe functions
>>
>>
>>
>> On 3/27/23 7:37 AM, Johannes Schindelin wrote:
>>> Hi Eric,
>>>
>>> On Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote:
>>>
>>>> From: Eric DeCosta <edecosta@mathworks.com>
>>>>
>>>> CreateNamedPipeW is perfectly happy accepting pipe names with
>>>> seemingly embedded escape charcters (e.g. \b), WaitNamedPipeW is not
>>>> and incorrectly returns ERROR_FILE_NOT_FOUND when clearly a named
>>>> pipe, succesfully created with CreateNamedPipeW, exists.
>>>>
>>>> For example, this network path is problemmatic:
>>>> \\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
>>>>
>>>> In order to work around this issue, rather than using the path to the
>>>> worktree directly as the name of the pipe, instead use the hash of
>>>> the worktree path.
>>>
>>> This is a rather large deviation from the other platforms, and it has
>>> an unwanted side effect: Git for Windows' installer currently
>>> enumerates the named pipes to figure out which FSMonitor instances
>>> need to be stopped before upgrading. It has to do that because it
>>> would otherwise be unable to overwrite the Git executable. And it
>>> needs to know the paths [*1*] so that it can stop the FSMonitors
>>> gracefully (as opposed to terminating them and risk interrupting them
>> while they serve a reply to a Git client).
>>>
>>> A much less intrusive change (that would not break Git for Windows'
>>> installer) would be to replace backslashes by forward slashes in the path.
>>>
>>> Please do that instead.
>>>
>>> Ciao,
>>> Johannes
>>>
>>> Footnote *1*: If you think that the Git for Windows installer could
>>> simply enumerate the process IDs of the FSMonitor instances and then
>>> look for their working directories: That is not a viable option. Not
>>> only does the Windows-based FSMonitor specifically switch to the
>>> parent directory (to avoid blocking the removal of a Git directory
>>> merely by running the process in said directory), even worse: there is
>>> no officially-sanctioned way to query a running process' current
>>> working directory (the only way I know of involves injecting a remote
>>> thread! Which will of course risk being labeled as malware by current anti-
>> malware solutions).
>>
>> Agreed. Please use forward slashes.
>>
>> Thanks,
>> Jeff
>>
> 
> I have misdiagnosed the problem. Here are my most recent findings:
> 
> The problem is the leading double-slashes for repos that resolve to remote filesystems. i.e. if S:\myrepo resolves to \\some-server\some-dir\myrepo then the path passed to initialize_pipe_name is //some-server/some-dir/myrepo
> 
> Regardless of what type or how many slashes appear after \\.\pipe\ the pipe name, as reported from PowerShell, is always \\.\\pipe\\some-server\some-dir\myrepo and WaitNamedPipeW returns ERROR_FILE_NOT_FOUND
> 
> If I skip over the first leading slash an use /some-server/some-dir/myrepo I get the same pipe name as before, WaitNamedPipeW is happy and commands like git fsmonitor--daemon status correctly report that the daemon is watching the repo.
> 
> -Eric

The named pipe file system (NPFS) is a little "special".  It is a flat
namespace and not hierarchical and not subject to the usual Win32
and/or NTFS limitations/quirks (such as restricted characters or legacy
filename suffixes).  It is a single level dictionary, in a sense.

The local form is "\\.\pipe\<name>" and according to [1], the only
restriction is that <name> portion may not contain backslashes[1],
but I'm seeing lots of named pipes of the form "\\.\pipe\Winsock2\..."
on my Windows 10 system, so that restriction may have been lifted
since the documentation was last updated.

[1] 
https://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createnamedpipew

Forward slashes (and now it seems backslashes) are not directory
separators -- they are just another character in the allowed char[256].
We tend to think of them as directory separators, but that is an
illusion.  For example, in a CMD prompt:

     dir \\.\pipe\\ /b

shows a simple list of all the named pipes on the system, including
some "Winsock2\CatalogChangeListener..." ones.  However, any attempt
to list the contents of the "Winsock2" directory:

     dir \\.\pipe\\Winsock2\\ /b

fails with a file not found error.

However, a simple wildcard lists them:

     dir \\.\pipe\\Winsock2* /b


 From PowerShell, we can see a complete list of pipes with:

     (get-childitem \\.\pipe\).FullName

But we get a path does not exist with:

     (get-childitem \\.\pipe\Winsock2\).FullName

However "get-childitem" is confused and reports "Winsock2" as
a directory multiple times, each with one item, when we do:

     (get-childitem \\.\pipe)


(BTW There's also the "Pipelist" tool from SysInternals that shows
them as a simple list of names (some with the embedded backslashes).


In [1], it also says that CreateNamedPipeW() can only create a local
"\\.\pipe\<something>" pipe, so I wonder if CreateNamedPipeW() is
silently prefixing "\\.\pipe\" if necessary...  I haven't had time
to try this.


Then WaitNamedPipeW() and/or CreateFileW() allows fully general
"\\<host>\<share>\<pathnames>", so the OS cannot do any implicit
fixup -- and these calls actually try to access the (intended)
network file.


I'm guessing here that this is the problem you've found.
If that is the case, we need to think about how to fix it
mainly because of what Johannes said about the installer
needing to properly shutdown running daemons during an upgrade.
Or rather, we will need to coordinate with the GFW installer.

Please let me know if any of this makes sense.

Thanks,
Jeff

[PATCH v2] fsmonitor: handle differences between Windows named pipe functions

[Eric DeCosta via GitGitGadget]

From: Eric DeCosta <edecosta@mathworks.com>

The two functions involved with creating and checking for the existance of
the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW appear
to handle names with leading slashes or backslashes a bit differently.

If a repo resolves to a remote file system with the UNC path of
//some-server/some-dir/some-repo, CreateNamedPipeW accepts this name and
creates this named pipe: \\.\pipe\some-server\some-dir\some-repo

However, when the same UNC path is passed to WaitNamedPipeW, it fails with
ERROR_FILE_NOT_FOUND.

Skipping the leading slash for UNC paths works for both CreateNamedPipeW and
WaitNamedPipeW. Resulting in a named pipe with the same name as above that
WaitNamedPipeW is able to correctly find.

Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
---
    fsmonitor: handle differences between Windows named pipe functions
    
    The two functions involved with creating and checking for the existance
    of the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW
    appear to handle names with leading slashes or backslashes a bit
    differently.
    
    If a repo resolves to a remote file system with the UNC path of
    //some-server/some-dir/some-repo, CreateNamedPipeW accepts this name and
    creates this named pipe: .\pipe\some-server\some-dir\some-repo
    
    However, when the same UNC path is passed to WaitNamedPipeW, it fails
    with ERROR_FILE_NOT_FOUND.
    
    Skipping the leading slash for UNC paths works for both CreateNamedPipeW
    and WaitNamedPipeW. Resulting in a named pipe with the same name as
    above that WaitNamedPipeW is able to correctly find.
    
    v1 differs from v0:
    
     * Abandon hashing repo path in favor of simpler solution where the
       leading slash is simply dropped for UNC paths

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1503%2Fedecosta-mw%2Ffsmonitor_windows-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1503/edecosta-mw/fsmonitor_windows-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1503

Range-diff vs v1:

 1:  c5307928cd7 ! 1:  b41f9bd2e96 fsmonitor: handle differences between Windows named pipe functions
     @@ Metadata
       ## Commit message ##
          fsmonitor: handle differences between Windows named pipe functions
      
     -    CreateNamedPipeW is perfectly happy accepting pipe names with seemingly
     -    embedded escape charcters (e.g. \b), WaitNamedPipeW is not and incorrectly
     -    returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully created
     -    with CreateNamedPipeW, exists.
     +    The two functions involved with creating and checking for the existance of
     +    the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW appear
     +    to handle names with leading slashes or backslashes a bit differently.
      
     -    For example, this network path is problemmatic:
     -    \\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
     +    If a repo resolves to a remote file system with the UNC path of
     +    //some-server/some-dir/some-repo, CreateNamedPipeW accepts this name and
     +    creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
      
     -    In order to work around this issue, rather than using the path to the
     -    worktree directly as the name of the pipe, instead use the hash of the
     -    worktree path.
     +    However, when the same UNC path is passed to WaitNamedPipeW, it fails with
     +    ERROR_FILE_NOT_FOUND.
     +
     +    Skipping the leading slash for UNC paths works for both CreateNamedPipeW and
     +    WaitNamedPipeW. Resulting in a named pipe with the same name as above that
     +    WaitNamedPipeW is able to correctly find.
      
          Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
      
       ## compat/simple-ipc/ipc-win32.c ##
     -@@
     - #include "cache.h"
     -+#include "hex.h"
     - #include "simple-ipc.h"
     - #include "strbuf.h"
     - #include "pkt-line.h"
      @@
       static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc)
       {
       	int off = 0;
     --	struct strbuf realpath = STRBUF_INIT;
     --
     --	if (!strbuf_realpath(&realpath, path, 0))
     --		return -1;
     -+	int ret = 0;
     -+	git_SHA_CTX sha1ctx;
     -+	struct strbuf real_path = STRBUF_INIT;
     -+	struct strbuf pipe_name = STRBUF_INIT;
     -+	unsigned char hash[GIT_MAX_RAWSZ];
     ++	int real_off = 0;
     + 	struct strbuf realpath = STRBUF_INIT;
       
     --	off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
     --	if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
     -+	if (!strbuf_realpath(&real_path, path, 0))
     + 	if (!strbuf_realpath(&realpath, path, 0))
       		return -1;
       
     --	/* Handle drive prefix */
     --	if (wpath[off] && wpath[off + 1] == L':') {
     --		wpath[off + 1] = L'_';
     --		off += 2;
     --	}
     -+	git_SHA1_Init(&sha1ctx);
     -+	git_SHA1_Update(&sha1ctx, real_path.buf, real_path.len);
     -+	git_SHA1_Final(hash, &sha1ctx);
     -+	strbuf_release(&real_path);
     - 
     --	for (; wpath[off]; off++)
     --		if (wpath[off] == L'/')
     --			wpath[off] = L'\\';
     -+	strbuf_addf(&pipe_name, "git-fsmonitor-%s", hash_to_hex(hash));
     -+	off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
     -+	if (xutftowcs(wpath + off, pipe_name.buf, alloc - off) < 0)
     -+		ret = -1;
     - 
     --	strbuf_release(&realpath);
     --	return 0;
     -+	strbuf_release(&pipe_name);
     -+	return ret;
     - }
     ++	/* UNC Path, skip leading slash */
     ++	if (realpath.buf[0] == '/' && realpath.buf[1] == '/')
     ++		real_off = 1;
     ++
     + 	off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
     +-	if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
     ++	if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)
     + 		return -1;
       
     - static enum ipc_active_state get_active_state(wchar_t *pipe_path)
     + 	/* Handle drive prefix */


 compat/simple-ipc/ipc-win32.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
index 997f6144344..632b12e1ab5 100644
--- a/compat/simple-ipc/ipc-win32.c
+++ b/compat/simple-ipc/ipc-win32.c
@@ -19,13 +19,18 @@
 static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc)
 {
 	int off = 0;
+	int real_off = 0;
 	struct strbuf realpath = STRBUF_INIT;
 
 	if (!strbuf_realpath(&realpath, path, 0))
 		return -1;
 
+	/* UNC Path, skip leading slash */
+	if (realpath.buf[0] == '/' && realpath.buf[1] == '/')
+		real_off = 1;
+
 	off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
-	if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
+	if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)
 		return -1;
 
 	/* Handle drive prefix */

base-commit: f285f68a132109c234d93490671c00218066ace9
-- 
gitgitgadget

RE: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions

[Eric DeCosta]

> -----Original Message-----
> From: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>
> Sent: Monday, April 10, 2023 3:46 PM
> To: git@vger.kernel.org
> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Jeff Hostetler
> <git@jeffhostetler.com>; Eric DeCosta <edecosta@mathworks.com>; Eric
> DeCosta <edecosta@mathworks.com>
> Subject: [PATCH v2] fsmonitor: handle differences between Windows named
> pipe functions
> 
> From: Eric DeCosta <edecosta@mathworks.com>
> 
> The two functions involved with creating and checking for the existance of
> the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW
> appear to handle names with leading slashes or backslashes a bit differently.
> 
> If a repo resolves to a remote file system with the UNC path of //some-
> server/some-dir/some-repo, CreateNamedPipeW accepts this name and
> creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
> 
> However, when the same UNC path is passed to WaitNamedPipeW, it fails
> with ERROR_FILE_NOT_FOUND.
> 
> Skipping the leading slash for UNC paths works for both CreateNamedPipeW
> and WaitNamedPipeW. Resulting in a named pipe with the same name as
> above that WaitNamedPipeW is able to correctly find.
> 
> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> ---
> fsmonitor: handle differences between Windows named pipe functions
> 
> The two functions involved with creating and checking for the existance of
> the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW
> appear to handle names with leading slashes or backslashes a bit differently.
> 
> If a repo resolves to a remote file system with the UNC path of //some-
> server/some-dir/some-repo, CreateNamedPipeW accepts this name and
> creates this named pipe: .\pipe\some-server\some-dir\some-repo
> 
> However, when the same UNC path is passed to WaitNamedPipeW, it fails
> with ERROR_FILE_NOT_FOUND.
> 
> Skipping the leading slash for UNC paths works for both CreateNamedPipeW
> and WaitNamedPipeW. Resulting in a named pipe with the same name as
> above that WaitNamedPipeW is able to correctly find.
> 
> v1 differs from v0:
> 
> * Abandon hashing repo path in favor of simpler solution where the leading
> slash is simply dropped for UNC paths
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-
> 1503%2Fedecosta-mw%2Ffsmonitor_windows-v2 <https://protect-
> us.mimecast.com/s/WUMKCqxoXGIDMzRYtE7lUM?domain=github.com>
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git <https://protect-
> us.mimecast.com/s/meLwCrkpNJSpB16QhjvzbL?domain=github.com>  pr-
> 1503/edecosta-mw/fsmonitor_windows-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/1503 <https://protect-
> us.mimecast.com/s/0_e0Cv2w7NFGk24wH5_RA3?domain=github.com>
> 
> Range-diff vs v1:
> 
> 1: c5307928cd7 ! 1: b41f9bd2e96 fsmonitor: handle differences between
> Windows named pipe functions @@ Metadata ## Commit message ##
> fsmonitor: handle differences between Windows named pipe functions
> 
> - CreateNamedPipeW is perfectly happy accepting pipe names with
> seemingly
> - embedded escape charcters (e.g. \b), WaitNamedPipeW is not and
> incorrectly
> - returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully
> created
> - with CreateNamedPipeW, exists.
> + The two functions involved with creating and checking for the
> + existance of the local fsmonitor named pipe, CratedNamedPipeW and
> + WaitNamedPipeW appear to handle names with leading slashes or
> backslashes a bit differently.
> 
> - For example, this network path is problemmatic:
> - \\batfs-sb29-cifs\vmgr\sbs29\my_git_repo
> + If a repo resolves to a remote file system with the UNC path of
> + //some-server/some-dir/some-repo, CreateNamedPipeW accepts this
> name
> + and creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
> 
> - In order to work around this issue, rather than using the path to the
> - worktree directly as the name of the pipe, instead use the hash of the
> - worktree path.
> + However, when the same UNC path is passed to WaitNamedPipeW, it fails
> + with ERROR_FILE_NOT_FOUND.
> +
> + Skipping the leading slash for UNC paths works for both
> + CreateNamedPipeW and WaitNamedPipeW. Resulting in a named pipe with
> + the same name as above that WaitNamedPipeW is able to correctly find.
> 
> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> 
> ## compat/simple-ipc/ipc-win32.c ##
> -@@
> - #include "cache.h"
> -+#include "hex.h"
> - #include "simple-ipc.h"
> - #include "strbuf.h"
> - #include "pkt-line.h"
> @@
> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t
> alloc) { int off = 0;
> -- struct strbuf realpath = STRBUF_INIT;
> --
> -- if (!strbuf_realpath(&realpath, path, 0))
> -- return -1;
> -+ int ret = 0;
> -+ git_SHA_CTX sha1ctx;
> -+ struct strbuf real_path = STRBUF_INIT; struct strbuf pipe_name =
> -+ STRBUF_INIT; unsigned char hash[GIT_MAX_RAWSZ];
> ++ int real_off = 0;
> + struct strbuf realpath = STRBUF_INIT;
> 
> -- off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
> -- if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
> -+ if (!strbuf_realpath(&real_path, path, 0))
> + if (!strbuf_realpath(&realpath, path, 0))
> return -1;
> 
> -- /* Handle drive prefix */
> -- if (wpath[off] && wpath[off + 1] == L':') {
> -- wpath[off + 1] = L'_';
> -- off += 2;
> -- }
> -+ git_SHA1_Init(&sha1ctx);
> -+ git_SHA1_Update(&sha1ctx, real_path.buf, real_path.len);
> -+ git_SHA1_Final(hash, &sha1ctx); strbuf_release(&real_path);
> -
> -- for (; wpath[off]; off++)
> -- if (wpath[off] == L'/')
> -- wpath[off] = L'\\';
> -+ strbuf_addf(&pipe_name, "git-fsmonitor-%s", hash_to_hex(hash)); off =
> -+ swprintf(wpath, alloc, L"\\\\.\\pipe\\"); if (xutftowcs(wpath + off,
> -+ pipe_name.buf, alloc - off) < 0) ret = -1;
> -
> -- strbuf_release(&realpath);
> -- return 0;
> -+ strbuf_release(&pipe_name);
> -+ return ret;
> - }
> ++ /* UNC Path, skip leading slash */
> ++ if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;
> ++
> + off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
> +- if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
> ++ if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)
> + return -1;
> 
> - static enum ipc_active_state get_active_state(wchar_t *pipe_path)
> + /* Handle drive prefix */
> 
> 
> compat/simple-ipc/ipc-win32.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
> index 997f6144344..632b12e1ab5 100644
> --- a/compat/simple-ipc/ipc-win32.c
> +++ b/compat/simple-ipc/ipc-win32.c
> @@ -19,13 +19,18 @@
> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t
> alloc) { int off = 0;
> + int real_off = 0;
> struct strbuf realpath = STRBUF_INIT;
> 
> if (!strbuf_realpath(&realpath, path, 0)) return -1;
> 
> + /* UNC Path, skip leading slash */
> + if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;
> +
> off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
> - if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
> + if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)
> return -1;
> 
> /* Handle drive prefix */
> 
> base-commit: f285f68a132109c234d93490671c00218066ace9
> --
> gitgitgadget

Are there any other thoughts about this?

I believe that this is the simplest change possible that will ensure that
fsmonitor correctly handles network repos.

-Eric

Re: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions

[Jeff Hostetler]

On 4/22/23 4:00 PM, Eric DeCosta wrote:
> 
>> -----Original Message-----
>> From: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>
>> Sent: Monday, April 10, 2023 3:46 PM
>> To: git@vger.kernel.org
>> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Jeff Hostetler
>> <git@jeffhostetler.com>; Eric DeCosta <edecosta@mathworks.com>; Eric
>> DeCosta <edecosta@mathworks.com>
>> Subject: [PATCH v2] fsmonitor: handle differences between Windows named
>> pipe functions
>>
>> From: Eric DeCosta <edecosta@mathworks.com>
>>
>> The two functions involved with creating and checking for the existance of
>> the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW
>> appear to handle names with leading slashes or backslashes a bit differently.
>>
>> If a repo resolves to a remote file system with the UNC path of //some-
>> server/some-dir/some-repo, CreateNamedPipeW accepts this name and
>> creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
>>
>> However, when the same UNC path is passed to WaitNamedPipeW, it fails
>> with ERROR_FILE_NOT_FOUND.
>>
>> Skipping the leading slash for UNC paths works for both CreateNamedPipeW
>> and WaitNamedPipeW. Resulting in a named pipe with the same name as
>> above that WaitNamedPipeW is able to correctly find.
>>
>> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
[...]
>>
>>
>> compat/simple-ipc/ipc-win32.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c
>> index 997f6144344..632b12e1ab5 100644
>> --- a/compat/simple-ipc/ipc-win32.c
>> +++ b/compat/simple-ipc/ipc-win32.c
>> @@ -19,13 +19,18 @@
>> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t
>> alloc) { int off = 0;
>> + int real_off = 0;
>> struct strbuf realpath = STRBUF_INIT;
>>
>> if (!strbuf_realpath(&realpath, path, 0)) return -1;
>>
>> + /* UNC Path, skip leading slash */
>> + if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;
>> +
>> off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
>> - if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
>> + if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)
>> return -1;

I haven't had a chance to test this, but this does look
like a minimal solution for the pathname confusion in the
MSFT APIs.

Do you need to test for realpath.buf[0] and [1] being a forward OR
a backslash ?

Should we set real_off to 2 rather than 1 because we already
appended a trailing backslash in the swprintf() ?

You should run one of those NPFS directory listing tools to
confirm the exact spelling of the pipe matches your expectation
here.  Yes, if both functions now work, we should be good, but
it would be good to confirm your real_off choice, right?

If would be good to (at least interactively) test that the
git-for-windows installer can find the path and stop the daemon
on an upgrade or uninstall.  See Johannes' earlier point.

We should state somewhere that we are running the fsmonitor
daemon locally and it is watching a remote file system.

You should run a few stress tests to ensure that the
MAX_RDCW_BUF_FALLBACK throttling works and that the daemon
doesn't fall behind on a very busy remote file system.  (There
are SMB/CIFS wire protocol limits.  See the source.)  (I did
test this between the combination of systems that I had, but
YMMV.)

During the stress test, it would also be good to test that
IO generated by a client process on your local machine to the
remote file system is reported, but also that random IO from
remote processes on the remote system are seen in the event
stream.  Again, I tested the combinations of machines that I
had available at the time.

Hope this helps,
Jeff


>>
>> /* Handle drive prefix */
>>
>> base-commit: f285f68a132109c234d93490671c00218066ace9
>> --
>> gitgitgadget
> 
> Are there any other thoughts about this?
> 
> I believe that this is the simplest change possible that will ensure that
> fsmonitor correctly handles network repos.
> 
> -Eric

Re: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions

[Jeff Hostetler]

On 4/26/23 4:33 PM, Jeff Hostetler wrote:
...
>>>
>>> + /* UNC Path, skip leading slash */
>>> + if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;
>>> +
>>> off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
>>> - if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
>>> + if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)
>>> return -1;
> 
> I haven't had a chance to test this, but this does look
> like a minimal solution for the pathname confusion in the
> MSFT APIs.
> 
> Do you need to test for realpath.buf[0] and [1] being a forward OR
> a backslash ?
> 
> Should we set real_off to 2 rather than 1 because we already
> appended a trailing backslash in the swprintf() ?

On second thought, you might actually need the second slash
in //./pipe//server/mount/whatever so that the GfW installer
can find remote repo path to CWD and stop the daemon.

Testing will tell here.

Jeff

> 
> You should run one of those NPFS directory listing tools to
> confirm the exact spelling of the pipe matches your expectation
> here.  Yes, if both functions now work, we should be good, but
> it would be good to confirm your real_off choice, right?
> 
> If would be good to (at least interactively) test that the
> git-for-windows installer can find the path and stop the daemon
> on an upgrade or uninstall.  See Johannes' earlier point.
> 
> We should state somewhere that we are running the fsmonitor
> daemon locally and it is watching a remote file system.
> 
> You should run a few stress tests to ensure that the
> MAX_RDCW_BUF_FALLBACK throttling works and that the daemon
> doesn't fall behind on a very busy remote file system.  (There
> are SMB/CIFS wire protocol limits.  See the source.)  (I did
> test this between the combination of systems that I had, but
> YMMV.)
> 
> During the stress test, it would also be good to test that
> IO generated by a client process on your local machine to the
> remote file system is reported, but also that random IO from
> remote processes on the remote system are seen in the event
> stream.  Again, I tested the combinations of machines that I
> had available at the time.
> 
> Hope this helps,
> Jeff
> 
> 
>>>
>>> /* Handle drive prefix */
>>>
>>> base-commit: f285f68a132109c234d93490671c00218066ace9
>>> -- 
>>> gitgitgadget
>>
>> Are there any other thoughts about this?
>>
>> I believe that this is the simplest change possible that will ensure that
>> fsmonitor correctly handles network repos.
>>
>> -Eric

RE: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions

[Eric DeCosta]

> -----Original Message-----
> From: Jeff Hostetler <git@jeffhostetler.com>
> Sent: Wednesday, April 26, 2023 4:34 PM
> To: Eric DeCosta <edecosta@mathworks.com>; Eric DeCosta via GitGitGadget
> <gitgitgadget@gmail.com>; git@vger.kernel.org
> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
> Subject: Re: [PATCH v2] fsmonitor: handle differences between Windows
> named pipe functions
> 
> 
> 
> On 4/22/23 4:00 PM, Eric DeCosta wrote:
> >
> >> -----Original Message-----
> >> From: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>
> >> Sent: Monday, April 10, 2023 3:46 PM
> >> To: git@vger.kernel.org
> >> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Jeff Hostetler
> >> <git@jeffhostetler.com>; Eric DeCosta <edecosta@mathworks.com>; Eric
> >> DeCosta <edecosta@mathworks.com>
> >> Subject: [PATCH v2] fsmonitor: handle differences between Windows
> named
> >> pipe functions
> >>
> >> From: Eric DeCosta <edecosta@mathworks.com>
> >>
> >> The two functions involved with creating and checking for the existance of
> >> the local fsmonitor named pipe, CratedNamedPipeW and
> WaitNamedPipeW
> >> appear to handle names with leading slashes or backslashes a bit
> differently.
> >>
> >> If a repo resolves to a remote file system with the UNC path of //some-
> >> server/some-dir/some-repo, CreateNamedPipeW accepts this name and
> >> creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
> >>
> >> However, when the same UNC path is passed to WaitNamedPipeW, it fails
> >> with ERROR_FILE_NOT_FOUND.
> >>
> >> Skipping the leading slash for UNC paths works for both
> CreateNamedPipeW
> >> and WaitNamedPipeW. Resulting in a named pipe with the same name as
> >> above that WaitNamedPipeW is able to correctly find.
> >>
> >> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
> [...]
> >>
> >>
> >> compat/simple-ipc/ipc-win32.c | 7 ++++++-
> >> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-
> win32.c
> >> index 997f6144344..632b12e1ab5 100644
> >> --- a/compat/simple-ipc/ipc-win32.c
> >> +++ b/compat/simple-ipc/ipc-win32.c
> >> @@ -19,13 +19,18 @@
> >> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t
> >> alloc) { int off = 0;
> >> + int real_off = 0;
> >> struct strbuf realpath = STRBUF_INIT;
> >>
> >> if (!strbuf_realpath(&realpath, path, 0)) return -1;
> >>
> >> + /* UNC Path, skip leading slash */
> >> + if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;
> >> +
> >> off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
> >> - if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
> >> + if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)
> >> return -1;
> 
> I haven't had a chance to test this, but this does look
> like a minimal solution for the pathname confusion in the
> MSFT APIs.
> 
> Do you need to test for realpath.buf[0] and [1] being a forward OR
> a backslash ?
> 
> Should we set real_off to 2 rather than 1 because we already
> appended a trailing backslash in the swprintf() ?
> 
Attempts to add additional backslashes after \\.\pipe\\ are apparently
ignored. The name of the local pipe always ends up looking like this:

for UNC paths:
  \\.\pipe\\some-server\some-dir\some-repo
  
and for local paths:
 \\.\pipe\\C_\some-dir\some-repo
  
Thus for a UNC path of //some-server/some-dir/some-repo the simplest thing to
do is to skip the first slash.

> You should run one of those NPFS directory listing tools to
> confirm the exact spelling of the pipe matches your expectation
> here.  Yes, if both functions now work, we should be good, but
> it would be good to confirm your real_off choice, right?
> 
I have used both PowerShell and Process Explorer and see similar results.

> If would be good to (at least interactively) test that the
> git-for-windows installer can find the path and stop the daemon
> on an upgrade or uninstall.  See Johannes' earlier point.
> 
In regards to the GfW installer, if the daemon is running against
a network mounted repo it reports the following:

Could not stop FSMonitor daemon in some-server\some-dir\some-repo
(output: , errors: fatal cannot change to 'some-server\some-dir\some-repo':
No such file or directory)

It looks like the installer may have to do something like:
look for "<drive letter>_" immediately after "\\.\pipe\\" and if it does not
find it, assume a UNC path.

> We should state somewhere that we are running the fsmonitor
> daemon locally and it is watching a remote file system.
> 
> You should run a few stress tests to ensure that the
> MAX_RDCW_BUF_FALLBACK throttling works and that the daemon
> doesn't fall behind on a very busy remote file system.  (There
> are SMB/CIFS wire protocol limits.  See the source.)  (I did
> test this between the combination of systems that I had, but
> YMMV.)
> 
> During the stress test, it would also be good to test that
> IO generated by a client process on your local machine to the
> remote file system is reported, but also that random IO from
> remote processes on the remote system are seen in the event
> stream.  Again, I tested the combinations of machines that I
> had available at the time.
> 
I hear what you are saying, however reporting that information increases
the scope of this change. As this stands right now the advertised feature
of allowing fsmonitor to work on network-mounted sandboxes for Windows
is not working as expected.

-Eric

> Hope this helps,
> Jeff
> 
> 
> >>
> >> /* Handle drive prefix */
> >>
> >> base-commit: f285f68a132109c234d93490671c00218066ace9
> >> --
> >> gitgitgadget
> >
> > Are there any other thoughts about this?
> >
> > I believe that this is the simplest change possible that will ensure that
> > fsmonitor correctly handles network repos.
> >
> > -Eric

Re: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions

[Jeff Hostetler]

Sorry this got lost in my inbox.

On 5/8/23 4:30 PM, Eric DeCosta wrote:
> 
>> -----Original Message-----
>> From: Jeff Hostetler <git@jeffhostetler.com>
>> Sent: Wednesday, April 26, 2023 4:34 PM
>> To: Eric DeCosta <edecosta@mathworks.com>; Eric DeCosta via GitGitGadget
>> <gitgitgadget@gmail.com>; git@vger.kernel.org
>> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>
>> Subject: Re: [PATCH v2] fsmonitor: handle differences between Windows
>> named pipe functions
>>
>>
>>
>> On 4/22/23 4:00 PM, Eric DeCosta wrote:
>>>
>>>> -----Original Message-----
>>>> From: Eric DeCosta via GitGitGadget <gitgitgadget@gmail.com>
>>>> Sent: Monday, April 10, 2023 3:46 PM
>>>> To: git@vger.kernel.org
>>>> Cc: Johannes Schindelin <Johannes.Schindelin@gmx.de>; Jeff Hostetler
>>>> <git@jeffhostetler.com>; Eric DeCosta <edecosta@mathworks.com>; Eric
>>>> DeCosta <edecosta@mathworks.com>
>>>> Subject: [PATCH v2] fsmonitor: handle differences between Windows
>> named
>>>> pipe functions
>>>>
>>>> From: Eric DeCosta <edecosta@mathworks.com>
>>>>
>>>> The two functions involved with creating and checking for the existance of
>>>> the local fsmonitor named pipe, CratedNamedPipeW and
>> WaitNamedPipeW
>>>> appear to handle names with leading slashes or backslashes a bit
>> differently.
>>>>
>>>> If a repo resolves to a remote file system with the UNC path of //some-
>>>> server/some-dir/some-repo, CreateNamedPipeW accepts this name and
>>>> creates this named pipe: \\.\pipe\some-server\some-dir\some-repo
>>>>
>>>> However, when the same UNC path is passed to WaitNamedPipeW, it fails
>>>> with ERROR_FILE_NOT_FOUND.
>>>>
>>>> Skipping the leading slash for UNC paths works for both
>> CreateNamedPipeW
>>>> and WaitNamedPipeW. Resulting in a named pipe with the same name as
>>>> above that WaitNamedPipeW is able to correctly find.
>>>>
>>>> Signed-off-by: Eric DeCosta <edecosta@mathworks.com>
>> [...]
>>>>
>>>>
>>>> compat/simple-ipc/ipc-win32.c | 7 ++++++-
>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-
>> win32.c
>>>> index 997f6144344..632b12e1ab5 100644
>>>> --- a/compat/simple-ipc/ipc-win32.c
>>>> +++ b/compat/simple-ipc/ipc-win32.c
>>>> @@ -19,13 +19,18 @@
>>>> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t
>>>> alloc) { int off = 0;
>>>> + int real_off = 0;
>>>> struct strbuf realpath = STRBUF_INIT;
>>>>
>>>> if (!strbuf_realpath(&realpath, path, 0)) return -1;
>>>>
>>>> + /* UNC Path, skip leading slash */
>>>> + if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;
>>>> +
>>>> off = swprintf(wpath, alloc, L"\\\\.\\pipe\\");
>>>> - if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)
>>>> + if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)
>>>> return -1;
>>
>> I haven't had a chance to test this, but this does look
>> like a minimal solution for the pathname confusion in the
>> MSFT APIs.
>>
>> Do you need to test for realpath.buf[0] and [1] being a forward OR
>> a backslash ?
>>
>> Should we set real_off to 2 rather than 1 because we already
>> appended a trailing backslash in the swprintf() ?
>>
> Attempts to add additional backslashes after \\.\pipe\\ are apparently
> ignored. The name of the local pipe always ends up looking like this:
> 
> for UNC paths:
>    \\.\pipe\\some-server\some-dir\some-repo
>    
> and for local paths:
>   \\.\pipe\\C_\some-dir\some-repo
>    
> Thus for a UNC path of //some-server/some-dir/some-repo the simplest thing to
> do is to skip the first slash.

Ok thanks. Just being paranoid...

> 
>> You should run one of those NPFS directory listing tools to
>> confirm the exact spelling of the pipe matches your expectation
>> here.  Yes, if both functions now work, we should be good, but
>> it would be good to confirm your real_off choice, right?
>>
> I have used both PowerShell and Process Explorer and see similar results.
> 

good. thanks.

>> If would be good to (at least interactively) test that the
>> git-for-windows installer can find the path and stop the daemon
>> on an upgrade or uninstall.  See Johannes' earlier point.
>>
> In regards to the GfW installer, if the daemon is running against
> a network mounted repo it reports the following:
> 
> Could not stop FSMonitor daemon in some-server\some-dir\some-repo
> (output: , errors: fatal cannot change to 'some-server\some-dir\some-repo':
> No such file or directory)
> 
> It looks like the installer may have to do something like:
> look for "<drive letter>_" immediately after "\\.\pipe\\" and if it does not
> find it, assume a UNC path.
> 

ok. perhaps you could log an issue on github.com/git-for-windows/git.git
to describe this.

>> We should state somewhere that we are running the fsmonitor
>> daemon locally and it is watching a remote file system.
>>
>> You should run a few stress tests to ensure that the
>> MAX_RDCW_BUF_FALLBACK throttling works and that the daemon
>> doesn't fall behind on a very busy remote file system.  (There
>> are SMB/CIFS wire protocol limits.  See the source.)  (I did
>> test this between the combination of systems that I had, but
>> YMMV.)
>>
>> During the stress test, it would also be good to test that
>> IO generated by a client process on your local machine to the
>> remote file system is reported, but also that random IO from
>> remote processes on the remote system are seen in the event
>> stream.  Again, I tested the combinations of machines that I
>> had available at the time.
>>
> I hear what you are saying, however reporting that information increases
> the scope of this change. As this stands right now the advertised feature
> of allowing fsmonitor to work on network-mounted sandboxes for Windows
> is not working as expected.

understood.  my only concern was that remotely mounted file systems
didn't get a lot of testing.  it worked, but i didn't have resources
to really stress it.  but then again, it if falls behind it will
force a resync, so you *shouldn't* get a wrong answer.

Thanks for all your work (and patience with me) on this.
Jeff


> 
> -Eric
> 
>> Hope this helps,
>> Jeff
>>
>>
>>>>
>>>> /* Handle drive prefix */
>>>>
>>>> base-commit: f285f68a132109c234d93490671c00218066ace9
>>>> --
>>>> gitgitgadget
>>>
>>> Are there any other thoughts about this?
>>>
>>> I believe that this is the simplest change possible that will ensure that
>>> fsmonitor correctly handles network repos.
>>>
>>> -Eric
>