{"thread":{"id":"59469","subject":"[PATCH] fsmonitor: handle differences between Windows named pipe functions","startedAt":"2023-03-24T17:15:38Z","lastAt":"2023-05-15T21:51:06Z","messageCount":12,"participants":["Eric DeCosta via GitGitGadget","Johannes Schindelin","Jeff Hostetler","Junio C Hamano","Eric DeCosta"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"474101","messageId":"pull.1503.git.1679678090412.gitgitgadget@gmail.com","threadId":"59469","inReplyTo":null,"subject":"[PATCH] fsmonitor: handle differences between Windows named pipe functions","fromName":"Eric DeCosta via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-03-24T17:14:50Z","receivedAt":"2023-03-24T17:15:38Z","isPatch":true,"sender":{"key":"edecosta@mathworks.com","avatar":"https://avatars.githubusercontent.com/u/67609563?v=4"},"body":"From: Eric DeCosta \n\nCreateNamedPipeW is perfectly happy accepting pipe names with seemingly\nembedded escape charcters (e.g. \\b), WaitNamedPipeW is not and incorrectly\nreturns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully created\nwith CreateNamedPipeW, exists.\n\nFor example, this network path is problemmatic:\n\\\\batfs-sb29-cifs\\vmgr\\sbs29\\my_git_repo\n\nIn order to work around this issue, rather than using the path to the\nworktree directly as the name of the pipe, instead use the hash of the\nworktree path.\n\nSigned-off-by: Eric DeCosta \n---\n fsmonitor: handle differences between Windows named pipe functions\n \n CreateNamedPipeW is perfectly happy accepting pipe names with embedded\n escape charcters (e.g. \\b), WaitNamedPipeW is not and incorrectly\n returns ERROR_FILE_NOT_FOUND when clearly a named pipe with the given\n name exists.\n \n For example, this path is problemmatic:\n \\batfs-sb29-cifs\\vmgr\\sbs29\\my_git_repo\n \n In order to work around this issue, rather than using the path to the\n worktree directly as the name of the pipe, instead use the hash of the\n worktree path.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1503%2Fedecosta-mw%2Ffsmonitor_windows-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1503/edecosta-mw/fsmonitor_windows-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/1503\n\n compat/simple-ipc/ipc-win32.c | 33 +++++++++++++++++----------------\n 1 file changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\nindex 20ea7b65e0b..867590abd10 100644\n--- a/compat/simple-ipc/ipc-win32.c\n+++ b/compat/simple-ipc/ipc-win32.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"hex.h\"\n #include \"simple-ipc.h\"\n #include \"strbuf.h\"\n #include \"pkt-line.h\"\n@@ -17,27 +18,27 @@\n static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc)\n {\n \tint off = 0;\n-\tstruct strbuf realpath = STRBUF_INIT;\n-\n-\tif (!strbuf_realpath(&realpath, path, 0))\n-\t\treturn -1;\n+\tint ret = 0;\n+\tgit_SHA_CTX sha1ctx;\n+\tstruct strbuf real_path = STRBUF_INIT;\n+\tstruct strbuf pipe_name = STRBUF_INIT;\n+\tunsigned char hash[GIT_MAX_RAWSZ];\n \n-\toff = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n-\tif (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)\n+\tif (!strbuf_realpath(&real_path, path, 0))\n \t\treturn -1;\n \n-\t/* Handle drive prefix */\n-\tif (wpath[off] && wpath[off + 1] == L':') {\n-\t\twpath[off + 1] = L'_';\n-\t\toff += 2;\n-\t}\n+\tgit_SHA1_Init(&sha1ctx);\n+\tgit_SHA1_Update(&sha1ctx, real_path.buf, real_path.len);\n+\tgit_SHA1_Final(hash, &sha1ctx);\n+\tstrbuf_release(&real_path);\n \n-\tfor (; wpath[off]; off++)\n-\t\tif (wpath[off] == L'/')\n-\t\t\twpath[off] = L'\\\\';\n+\tstrbuf_addf(&pipe_name, \"git-fsmonitor-%s\", hash_to_hex(hash));\n+\toff = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n+\tif (xutftowcs(wpath + off, pipe_name.buf, alloc - off) < 0)\n+\t\tret = -1;\n \n-\tstrbuf_release(&realpath);\n-\treturn 0;\n+\tstrbuf_release(&pipe_name);\n+\treturn ret;\n }\n \n static enum ipc_active_state get_active_state(wchar_t *pipe_path)\n\nbase-commit: 27d43aaaf50ef0ae014b88bba294f93658016a2e\n-- \ngitgitgadget\n"},{"id":"474232","messageId":"e48e768a-19f3-386a-9bda-8fa8681d1a6c@gmx.de","threadId":"59469","inReplyTo":"pull.1503.git.1679678090412.gitgitgadget@gmail.com","subject":"Re: [PATCH] fsmonitor: handle differences between Windows named pipe functions","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2023-03-27T11:37:03Z","receivedAt":"2023-03-27T11:37:16Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Eric,\n\nOn Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote:\n\n> From: Eric DeCosta \n>\n> CreateNamedPipeW is perfectly happy accepting pipe names with seemingly\n> embedded escape charcters (e.g. \\b), WaitNamedPipeW is not and incorrectly\n> returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully created\n> with CreateNamedPipeW, exists.\n>\n> For example, this network path is problemmatic:\n> \\\\batfs-sb29-cifs\\vmgr\\sbs29\\my_git_repo\n>\n> In order to work around this issue, rather than using the path to the\n> worktree directly as the name of the pipe, instead use the hash of the\n> worktree path.\n\nThis is a rather large deviation from the other platforms, and it has an\nunwanted side effect: Git for Windows' installer currently enumerates the\nnamed pipes to figure out which FSMonitor instances need to be stopped\nbefore upgrading. It has to do that because it would otherwise be unable\nto overwrite the Git executable. And it needs to know the paths [*1*] so\nthat it can stop the FSMonitors gracefully (as opposed to terminating them\nand risk interrupting them while they serve a reply to a Git client).\n\nA much less intrusive change (that would not break Git for Windows'\ninstaller) would be to replace backslashes by forward slashes in the path.\n\nPlease do that instead.\n\nCiao,\nJohannes\n\nFootnote *1*: If you think that the Git for Windows installer could simply\nenumerate the process IDs of the FSMonitor instances and then look for\ntheir working directories: That is not a viable option. Not only does the\nWindows-based FSMonitor specifically switch to the parent directory (to\navoid blocking the removal of a Git directory merely by running the\nprocess in said directory), even worse: there is no officially-sanctioned\nway to query a running process' current working directory (the only way I\nknow of involves injecting a remote thread! Which will of course risk\nbeing labeled as malware by current anti-malware solutions).\n"},{"id":"474237","messageId":"b9cf67e4-22a7-2ff0-8310-9223bea10d6d@jeffhostetler.com","threadId":"59469","inReplyTo":"e48e768a-19f3-386a-9bda-8fa8681d1a6c@gmx.de","subject":"Re: [PATCH] fsmonitor: handle differences between Windows named pipe functions","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-03-27T15:02:16Z","receivedAt":"2023-03-27T15:03:32Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 3/27/23 7:37 AM, Johannes Schindelin wrote:\n> Hi Eric,\n> \n> On Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote:\n> \n>> From: Eric DeCosta \n>>\n>> CreateNamedPipeW is perfectly happy accepting pipe names with seemingly\n>> embedded escape charcters (e.g. \\b), WaitNamedPipeW is not and incorrectly\n>> returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully created\n>> with CreateNamedPipeW, exists.\n>>\n>> For example, this network path is problemmatic:\n>> \\\\batfs-sb29-cifs\\vmgr\\sbs29\\my_git_repo\n>>\n>> In order to work around this issue, rather than using the path to the\n>> worktree directly as the name of the pipe, instead use the hash of the\n>> worktree path.\n> \n> This is a rather large deviation from the other platforms, and it has an\n> unwanted side effect: Git for Windows' installer currently enumerates the\n> named pipes to figure out which FSMonitor instances need to be stopped\n> before upgrading. It has to do that because it would otherwise be unable\n> to overwrite the Git executable. And it needs to know the paths [*1*] so\n> that it can stop the FSMonitors gracefully (as opposed to terminating them\n> and risk interrupting them while they serve a reply to a Git client).\n> \n> A much less intrusive change (that would not break Git for Windows'\n> installer) would be to replace backslashes by forward slashes in the path.\n> \n> Please do that instead.\n> \n> Ciao,\n> Johannes\n> \n> Footnote *1*: If you think that the Git for Windows installer could simply\n> enumerate the process IDs of the FSMonitor instances and then look for\n> their working directories: That is not a viable option. Not only does the\n> Windows-based FSMonitor specifically switch to the parent directory (to\n> avoid blocking the removal of a Git directory merely by running the\n> process in said directory), even worse: there is no officially-sanctioned\n> way to query a running process' current working directory (the only way I\n> know of involves injecting a remote thread! Which will of course risk\n> being labeled as malware by current anti-malware solutions).\n\nAgreed. Please use forward slashes.\n\nThanks,\nJeff\n\n\n"},{"id":"474239","messageId":"xmqqtty6ch38.fsf@gitster.g","threadId":"59469","inReplyTo":"b9cf67e4-22a7-2ff0-8310-9223bea10d6d@jeffhostetler.com","subject":"Re: [PATCH] fsmonitor: handle differences between Windows named pipe functions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2023-03-27T16:08:27Z","receivedAt":"2023-03-27T16:08:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n>> This is a rather large deviation from the other platforms, and it\n>> has an\n>> unwanted side effect: Git for Windows' installer currently enumerates the\n> ...\n> Agreed. Please use forward slashes.\n\nThanks, both.\n\nI'll mark the topic to be expecting a reroll, then.\n\n\n"},{"id":"474948","messageId":"BL0PR05MB55713A00DEB97375BDC6D1DAD9919@BL0PR05MB5571.namprd05.prod.outlook.com","threadId":"59469","inReplyTo":"b9cf67e4-22a7-2ff0-8310-9223bea10d6d@jeffhostetler.com","subject":"RE: [PATCH] fsmonitor: handle differences between Windows named pipe functions","fromName":"Eric DeCosta","fromEmail":"edecosta@mathworks.com","sentAt":"2023-04-06T19:08:50Z","receivedAt":"2023-04-06T19:10:48Z","isPatch":true,"sender":{"key":"edecosta@mathworks.com","avatar":"https://avatars.githubusercontent.com/u/67609563?v=4"},"body":"\n\n> -----Original Message-----\n> From: Jeff Hostetler \n> Sent: Monday, March 27, 2023 11:02 AM\n> To: Johannes Schindelin ; Eric DeCosta via\n> GitGitGadget \n> Cc: git@vger.kernel.org; Eric DeCosta \n> Subject: Re: [PATCH] fsmonitor: handle differences between Windows\n> named pipe functions\n> \n> \n> \n> On 3/27/23 7:37 AM, Johannes Schindelin wrote:\n> > Hi Eric,\n> >\n> > On Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote:\n> >\n> >> From: Eric DeCosta \n> >>\n> >> CreateNamedPipeW is perfectly happy accepting pipe names with\n> >> seemingly embedded escape charcters (e.g. \\b), WaitNamedPipeW is not\n> >> and incorrectly returns ERROR_FILE_NOT_FOUND when clearly a named\n> >> pipe, succesfully created with CreateNamedPipeW, exists.\n> >>\n> >> For example, this network path is problemmatic:\n> >> \\\\batfs-sb29-cifs\\vmgr\\sbs29\\my_git_repo\n> >>\n> >> In order to work around this issue, rather than using the path to the\n> >> worktree directly as the name of the pipe, instead use the hash of\n> >> the worktree path.\n> >\n> > This is a rather large deviation from the other platforms, and it has\n> > an unwanted side effect: Git for Windows' installer currently\n> > enumerates the named pipes to figure out which FSMonitor instances\n> > need to be stopped before upgrading. It has to do that because it\n> > would otherwise be unable to overwrite the Git executable. And it\n> > needs to know the paths [*1*] so that it can stop the FSMonitors\n> > gracefully (as opposed to terminating them and risk interrupting them\n> while they serve a reply to a Git client).\n> >\n> > A much less intrusive change (that would not break Git for Windows'\n> > installer) would be to replace backslashes by forward slashes in the path.\n> >\n> > Please do that instead.\n> >\n> > Ciao,\n> > Johannes\n> >\n> > Footnote *1*: If you think that the Git for Windows installer could\n> > simply enumerate the process IDs of the FSMonitor instances and then\n> > look for their working directories: That is not a viable option. Not\n> > only does the Windows-based FSMonitor specifically switch to the\n> > parent directory (to avoid blocking the removal of a Git directory\n> > merely by running the process in said directory), even worse: there is\n> > no officially-sanctioned way to query a running process' current\n> > working directory (the only way I know of involves injecting a remote\n> > thread! Which will of course risk being labeled as malware by current anti-\n> malware solutions).\n> \n> Agreed. Please use forward slashes.\n> \n> Thanks,\n> Jeff\n> \n\nI have misdiagnosed the problem. Here are my most recent findings:\n\nThe 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\n\nRegardless 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\n\nIf 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.\n\n-Eric\n"},{"id":"475019","messageId":"dcd66846-8992-7016-c711-09397384c18c@jeffhostetler.com","threadId":"59469","inReplyTo":"BL0PR05MB55713A00DEB97375BDC6D1DAD9919@BL0PR05MB5571.namprd05.prod.outlook.com","subject":"Re: [PATCH] fsmonitor: handle differences between Windows named pipe functions","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-04-07T20:55:38Z","receivedAt":"2023-04-07T20:55:44Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 4/6/23 3:08 PM, Eric DeCosta wrote:\n> \n> \n>> -----Original Message-----\n>> From: Jeff Hostetler \n>> Sent: Monday, March 27, 2023 11:02 AM\n>> To: Johannes Schindelin ; Eric DeCosta via\n>> GitGitGadget \n>> Cc: git@vger.kernel.org; Eric DeCosta \n>> Subject: Re: [PATCH] fsmonitor: handle differences between Windows\n>> named pipe functions\n>>\n>>\n>>\n>> On 3/27/23 7:37 AM, Johannes Schindelin wrote:\n>>> Hi Eric,\n>>>\n>>> On Fri, 24 Mar 2023, Eric DeCosta via GitGitGadget wrote:\n>>>\n>>>> From: Eric DeCosta \n>>>>\n>>>> CreateNamedPipeW is perfectly happy accepting pipe names with\n>>>> seemingly embedded escape charcters (e.g. \\b), WaitNamedPipeW is not\n>>>> and incorrectly returns ERROR_FILE_NOT_FOUND when clearly a named\n>>>> pipe, succesfully created with CreateNamedPipeW, exists.\n>>>>\n>>>> For example, this network path is problemmatic:\n>>>> \\\\batfs-sb29-cifs\\vmgr\\sbs29\\my_git_repo\n>>>>\n>>>> In order to work around this issue, rather than using the path to the\n>>>> worktree directly as the name of the pipe, instead use the hash of\n>>>> the worktree path.\n>>>\n>>> This is a rather large deviation from the other platforms, and it has\n>>> an unwanted side effect: Git for Windows' installer currently\n>>> enumerates the named pipes to figure out which FSMonitor instances\n>>> need to be stopped before upgrading. It has to do that because it\n>>> would otherwise be unable to overwrite the Git executable. And it\n>>> needs to know the paths [*1*] so that it can stop the FSMonitors\n>>> gracefully (as opposed to terminating them and risk interrupting them\n>> while they serve a reply to a Git client).\n>>>\n>>> A much less intrusive change (that would not break Git for Windows'\n>>> installer) would be to replace backslashes by forward slashes in the path.\n>>>\n>>> Please do that instead.\n>>>\n>>> Ciao,\n>>> Johannes\n>>>\n>>> Footnote *1*: If you think that the Git for Windows installer could\n>>> simply enumerate the process IDs of the FSMonitor instances and then\n>>> look for their working directories: That is not a viable option. Not\n>>> only does the Windows-based FSMonitor specifically switch to the\n>>> parent directory (to avoid blocking the removal of a Git directory\n>>> merely by running the process in said directory), even worse: there is\n>>> no officially-sanctioned way to query a running process' current\n>>> working directory (the only way I know of involves injecting a remote\n>>> thread! Which will of course risk being labeled as malware by current anti-\n>> malware solutions).\n>>\n>> Agreed. Please use forward slashes.\n>>\n>> Thanks,\n>> Jeff\n>>\n> \n> I have misdiagnosed the problem. Here are my most recent findings:\n> \n> 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\n> \n> 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\n> \n> 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.\n> \n> -Eric\n\nThe named pipe file system (NPFS) is a little \"special\". It is a flat\nnamespace and not hierarchical and not subject to the usual Win32\nand/or NTFS limitations/quirks (such as restricted characters or legacy\nfilename suffixes). It is a single level dictionary, in a sense.\n\nThe local form is \"\\\\.\\pipe\\\" and according to [1], the only\nrestriction is that portion may not contain backslashes[1],\nbut I'm seeing lots of named pipes of the form \"\\\\.\\pipe\\Winsock2\\...\"\non my Windows 10 system, so that restriction may have been lifted\nsince the documentation was last updated.\n\n[1] \nhttps://learn.microsoft.com/en-us/windows/win32/api/namedpipeapi/nf-namedpipeapi-createnamedpipew\n\nForward slashes (and now it seems backslashes) are not directory\nseparators -- they are just another character in the allowed char[256].\nWe tend to think of them as directory separators, but that is an\nillusion. For example, in a CMD prompt:\n\n dir \\\\.\\pipe\\\\ /b\n\nshows a simple list of all the named pipes on the system, including\nsome \"Winsock2\\CatalogChangeListener...\" ones. However, any attempt\nto list the contents of the \"Winsock2\" directory:\n\n dir \\\\.\\pipe\\\\Winsock2\\\\ /b\n\nfails with a file not found error.\n\nHowever, a simple wildcard lists them:\n\n dir \\\\.\\pipe\\\\Winsock2* /b\n\n\n From PowerShell, we can see a complete list of pipes with:\n\n (get-childitem \\\\.\\pipe\\).FullName\n\nBut we get a path does not exist with:\n\n (get-childitem \\\\.\\pipe\\Winsock2\\).FullName\n\nHowever \"get-childitem\" is confused and reports \"Winsock2\" as\na directory multiple times, each with one item, when we do:\n\n (get-childitem \\\\.\\pipe)\n\n\n(BTW There's also the \"Pipelist\" tool from SysInternals that shows\nthem as a simple list of names (some with the embedded backslashes).\n\n\nIn [1], it also says that CreateNamedPipeW() can only create a local\n\"\\\\.\\pipe\\\" pipe, so I wonder if CreateNamedPipeW() is\nsilently prefixing \"\\\\.\\pipe\\\" if necessary... I haven't had time\nto try this.\n\n\nThen WaitNamedPipeW() and/or CreateFileW() allows fully general\n\"\\\\\\\\\", so the OS cannot do any implicit\nfixup -- and these calls actually try to access the (intended)\nnetwork file.\n\n\nI'm guessing here that this is the problem you've found.\nIf that is the case, we need to think about how to fix it\nmainly because of what Johannes said about the installer\nneeding to properly shutdown running daemons during an upgrade.\nOr rather, we will need to coordinate with the GFW installer.\n\nPlease let me know if any of this makes sense.\n\nThanks,\nJeff\n\n\n\n"},{"id":"475076","messageId":"pull.1503.v2.git.1681155963011.gitgitgadget@gmail.com","threadId":"59469","inReplyTo":"pull.1503.git.1679678090412.gitgitgadget@gmail.com","subject":"[PATCH v2] fsmonitor: handle differences between Windows named pipe functions","fromName":"Eric DeCosta via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2023-04-10T19:46:02Z","receivedAt":"2023-04-10T19:46:11Z","isPatch":true,"sender":{"key":"edecosta@mathworks.com","avatar":"https://avatars.githubusercontent.com/u/67609563?v=4"},"body":"From: Eric DeCosta \n\nThe two functions involved with creating and checking for the existance of\nthe local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW appear\nto handle names with leading slashes or backslashes a bit differently.\n\nIf a repo resolves to a remote file system with the UNC path of\n//some-server/some-dir/some-repo, CreateNamedPipeW accepts this name and\ncreates this named pipe: \\\\.\\pipe\\some-server\\some-dir\\some-repo\n\nHowever, when the same UNC path is passed to WaitNamedPipeW, it fails with\nERROR_FILE_NOT_FOUND.\n\nSkipping the leading slash for UNC paths works for both CreateNamedPipeW and\nWaitNamedPipeW. Resulting in a named pipe with the same name as above that\nWaitNamedPipeW is able to correctly find.\n\nSigned-off-by: Eric DeCosta \n---\n fsmonitor: handle differences between Windows named pipe functions\n \n The two functions involved with creating and checking for the existance\n of the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW\n appear to handle names with leading slashes or backslashes a bit\n differently.\n \n If a repo resolves to a remote file system with the UNC path of\n //some-server/some-dir/some-repo, CreateNamedPipeW accepts this name and\n creates this named pipe: .\\pipe\\some-server\\some-dir\\some-repo\n \n However, when the same UNC path is passed to WaitNamedPipeW, it fails\n with ERROR_FILE_NOT_FOUND.\n \n Skipping the leading slash for UNC paths works for both CreateNamedPipeW\n and WaitNamedPipeW. Resulting in a named pipe with the same name as\n above that WaitNamedPipeW is able to correctly find.\n \n v1 differs from v0:\n \n * Abandon hashing repo path in favor of simpler solution where the\n leading slash is simply dropped for UNC paths\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-1503%2Fedecosta-mw%2Ffsmonitor_windows-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1503/edecosta-mw/fsmonitor_windows-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/1503\n\nRange-diff vs v1:\n\n 1: c5307928cd7 ! 1: b41f9bd2e96 fsmonitor: handle differences between Windows named pipe functions\n @@ Metadata\n ## Commit message ##\n fsmonitor: handle differences between Windows named pipe functions\n \n - CreateNamedPipeW is perfectly happy accepting pipe names with seemingly\n - embedded escape charcters (e.g. \\b), WaitNamedPipeW is not and incorrectly\n - returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully created\n - with CreateNamedPipeW, exists.\n + The two functions involved with creating and checking for the existance of\n + the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW appear\n + to handle names with leading slashes or backslashes a bit differently.\n \n - For example, this network path is problemmatic:\n - \\\\batfs-sb29-cifs\\vmgr\\sbs29\\my_git_repo\n + If a repo resolves to a remote file system with the UNC path of\n + //some-server/some-dir/some-repo, CreateNamedPipeW accepts this name and\n + creates this named pipe: \\\\.\\pipe\\some-server\\some-dir\\some-repo\n \n - In order to work around this issue, rather than using the path to the\n - worktree directly as the name of the pipe, instead use the hash of the\n - worktree path.\n + However, when the same UNC path is passed to WaitNamedPipeW, it fails with\n + ERROR_FILE_NOT_FOUND.\n +\n + Skipping the leading slash for UNC paths works for both CreateNamedPipeW and\n + WaitNamedPipeW. Resulting in a named pipe with the same name as above that\n + WaitNamedPipeW is able to correctly find.\n \n Signed-off-by: Eric DeCosta \n \n ## compat/simple-ipc/ipc-win32.c ##\n -@@\n - #include \"cache.h\"\n -+#include \"hex.h\"\n - #include \"simple-ipc.h\"\n - #include \"strbuf.h\"\n - #include \"pkt-line.h\"\n @@\n static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc)\n {\n \tint off = 0;\n --\tstruct strbuf realpath = STRBUF_INIT;\n --\n --\tif (!strbuf_realpath(&realpath, path, 0))\n --\t\treturn -1;\n -+\tint ret = 0;\n -+\tgit_SHA_CTX sha1ctx;\n -+\tstruct strbuf real_path = STRBUF_INIT;\n -+\tstruct strbuf pipe_name = STRBUF_INIT;\n -+\tunsigned char hash[GIT_MAX_RAWSZ];\n ++\tint real_off = 0;\n + \tstruct strbuf realpath = STRBUF_INIT;\n \n --\toff = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n --\tif (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)\n -+\tif (!strbuf_realpath(&real_path, path, 0))\n + \tif (!strbuf_realpath(&realpath, path, 0))\n \t\treturn -1;\n \n --\t/* Handle drive prefix */\n --\tif (wpath[off] && wpath[off + 1] == L':') {\n --\t\twpath[off + 1] = L'_';\n --\t\toff += 2;\n --\t}\n -+\tgit_SHA1_Init(&sha1ctx);\n -+\tgit_SHA1_Update(&sha1ctx, real_path.buf, real_path.len);\n -+\tgit_SHA1_Final(hash, &sha1ctx);\n -+\tstrbuf_release(&real_path);\n - \n --\tfor (; wpath[off]; off++)\n --\t\tif (wpath[off] == L'/')\n --\t\t\twpath[off] = L'\\\\';\n -+\tstrbuf_addf(&pipe_name, \"git-fsmonitor-%s\", hash_to_hex(hash));\n -+\toff = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n -+\tif (xutftowcs(wpath + off, pipe_name.buf, alloc - off) < 0)\n -+\t\tret = -1;\n - \n --\tstrbuf_release(&realpath);\n --\treturn 0;\n -+\tstrbuf_release(&pipe_name);\n -+\treturn ret;\n - }\n ++\t/* UNC Path, skip leading slash */\n ++\tif (realpath.buf[0] == '/' && realpath.buf[1] == '/')\n ++\t\treal_off = 1;\n ++\n + \toff = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n +-\tif (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)\n ++\tif (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)\n + \t\treturn -1;\n \n - static enum ipc_active_state get_active_state(wchar_t *pipe_path)\n + \t/* Handle drive prefix */\n\n\n compat/simple-ipc/ipc-win32.c | 7 ++++++-\n 1 file changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\nindex 997f6144344..632b12e1ab5 100644\n--- a/compat/simple-ipc/ipc-win32.c\n+++ b/compat/simple-ipc/ipc-win32.c\n@@ -19,13 +19,18 @@\n static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t alloc)\n {\n \tint off = 0;\n+\tint real_off = 0;\n \tstruct strbuf realpath = STRBUF_INIT;\n \n \tif (!strbuf_realpath(&realpath, path, 0))\n \t\treturn -1;\n \n+\t/* UNC Path, skip leading slash */\n+\tif (realpath.buf[0] == '/' && realpath.buf[1] == '/')\n+\t\treal_off = 1;\n+\n \toff = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n-\tif (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)\n+\tif (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)\n \t\treturn -1;\n \n \t/* Handle drive prefix */\n\nbase-commit: f285f68a132109c234d93490671c00218066ace9\n-- \ngitgitgadget\n"},{"id":"475852","messageId":"BL0PR05MB557134157C3D08D982DEB338D9619@BL0PR05MB5571.namprd05.prod.outlook.com","threadId":"59469","inReplyTo":"pull.1503.v2.git.1681155963011.gitgitgadget@gmail.com","subject":"RE: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions","fromName":"Eric DeCosta","fromEmail":"edecosta@mathworks.com","sentAt":"2023-04-22T20:00:51Z","receivedAt":"2023-04-22T20:02:57Z","isPatch":true,"sender":{"key":"edecosta@mathworks.com","avatar":"https://avatars.githubusercontent.com/u/67609563?v=4"},"body":"\n> -----Original Message-----\n> From: Eric DeCosta via GitGitGadget \n> Sent: Monday, April 10, 2023 3:46 PM\n> To: git@vger.kernel.org\n> Cc: Johannes Schindelin ; Jeff Hostetler\n> ; Eric DeCosta ; Eric\n> DeCosta \n> Subject: [PATCH v2] fsmonitor: handle differences between Windows named\n> pipe functions\n> \n> From: Eric DeCosta \n> \n> The two functions involved with creating and checking for the existance of\n> the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW\n> appear to handle names with leading slashes or backslashes a bit differently.\n> \n> If a repo resolves to a remote file system with the UNC path of //some-\n> server/some-dir/some-repo, CreateNamedPipeW accepts this name and\n> creates this named pipe: \\\\.\\pipe\\some-server\\some-dir\\some-repo\n> \n> However, when the same UNC path is passed to WaitNamedPipeW, it fails\n> with ERROR_FILE_NOT_FOUND.\n> \n> Skipping the leading slash for UNC paths works for both CreateNamedPipeW\n> and WaitNamedPipeW. Resulting in a named pipe with the same name as\n> above that WaitNamedPipeW is able to correctly find.\n> \n> Signed-off-by: Eric DeCosta \n> ---\n> fsmonitor: handle differences between Windows named pipe functions\n> \n> The two functions involved with creating and checking for the existance of\n> the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW\n> appear to handle names with leading slashes or backslashes a bit differently.\n> \n> If a repo resolves to a remote file system with the UNC path of //some-\n> server/some-dir/some-repo, CreateNamedPipeW accepts this name and\n> creates this named pipe: .\\pipe\\some-server\\some-dir\\some-repo\n> \n> However, when the same UNC path is passed to WaitNamedPipeW, it fails\n> with ERROR_FILE_NOT_FOUND.\n> \n> Skipping the leading slash for UNC paths works for both CreateNamedPipeW\n> and WaitNamedPipeW. Resulting in a named pipe with the same name as\n> above that WaitNamedPipeW is able to correctly find.\n> \n> v1 differs from v0:\n> \n> * Abandon hashing repo path in favor of simpler solution where the leading\n> slash is simply dropped for UNC paths\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-\n> 1503%2Fedecosta-mw%2Ffsmonitor_windows-v2 us.mimecast.com/s/WUMKCqxoXGIDMzRYtE7lUM?domain=github.com>\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git us.mimecast.com/s/meLwCrkpNJSpB16QhjvzbL?domain=github.com> pr-\n> 1503/edecosta-mw/fsmonitor_windows-v2\n> Pull-Request: https://github.com/gitgitgadget/git/pull/1503 us.mimecast.com/s/0_e0Cv2w7NFGk24wH5_RA3?domain=github.com>\n> \n> Range-diff vs v1:\n> \n> 1: c5307928cd7 ! 1: b41f9bd2e96 fsmonitor: handle differences between\n> Windows named pipe functions @@ Metadata ## Commit message ##\n> fsmonitor: handle differences between Windows named pipe functions\n> \n> - CreateNamedPipeW is perfectly happy accepting pipe names with\n> seemingly\n> - embedded escape charcters (e.g. \\b), WaitNamedPipeW is not and\n> incorrectly\n> - returns ERROR_FILE_NOT_FOUND when clearly a named pipe, succesfully\n> created\n> - with CreateNamedPipeW, exists.\n> + The two functions involved with creating and checking for the\n> + existance of the local fsmonitor named pipe, CratedNamedPipeW and\n> + WaitNamedPipeW appear to handle names with leading slashes or\n> backslashes a bit differently.\n> \n> - For example, this network path is problemmatic:\n> - \\\\batfs-sb29-cifs\\vmgr\\sbs29\\my_git_repo\n> + If a repo resolves to a remote file system with the UNC path of\n> + //some-server/some-dir/some-repo, CreateNamedPipeW accepts this\n> name\n> + and creates this named pipe: \\\\.\\pipe\\some-server\\some-dir\\some-repo\n> \n> - In order to work around this issue, rather than using the path to the\n> - worktree directly as the name of the pipe, instead use the hash of the\n> - worktree path.\n> + However, when the same UNC path is passed to WaitNamedPipeW, it fails\n> + with ERROR_FILE_NOT_FOUND.\n> +\n> + Skipping the leading slash for UNC paths works for both\n> + CreateNamedPipeW and WaitNamedPipeW. Resulting in a named pipe with\n> + the same name as above that WaitNamedPipeW is able to correctly find.\n> \n> Signed-off-by: Eric DeCosta \n> \n> ## compat/simple-ipc/ipc-win32.c ##\n> -@@\n> - #include \"cache.h\"\n> -+#include \"hex.h\"\n> - #include \"simple-ipc.h\"\n> - #include \"strbuf.h\"\n> - #include \"pkt-line.h\"\n> @@\n> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t\n> alloc) { int off = 0;\n> -- struct strbuf realpath = STRBUF_INIT;\n> --\n> -- if (!strbuf_realpath(&realpath, path, 0))\n> -- return -1;\n> -+ int ret = 0;\n> -+ git_SHA_CTX sha1ctx;\n> -+ struct strbuf real_path = STRBUF_INIT; struct strbuf pipe_name =\n> -+ STRBUF_INIT; unsigned char hash[GIT_MAX_RAWSZ];\n> ++ int real_off = 0;\n> + struct strbuf realpath = STRBUF_INIT;\n> \n> -- off = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n> -- if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)\n> -+ if (!strbuf_realpath(&real_path, path, 0))\n> + if (!strbuf_realpath(&realpath, path, 0))\n> return -1;\n> \n> -- /* Handle drive prefix */\n> -- if (wpath[off] && wpath[off + 1] == L':') {\n> -- wpath[off + 1] = L'_';\n> -- off += 2;\n> -- }\n> -+ git_SHA1_Init(&sha1ctx);\n> -+ git_SHA1_Update(&sha1ctx, real_path.buf, real_path.len);\n> -+ git_SHA1_Final(hash, &sha1ctx); strbuf_release(&real_path);\n> -\n> -- for (; wpath[off]; off++)\n> -- if (wpath[off] == L'/')\n> -- wpath[off] = L'\\\\';\n> -+ strbuf_addf(&pipe_name, \"git-fsmonitor-%s\", hash_to_hex(hash)); off =\n> -+ swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\"); if (xutftowcs(wpath + off,\n> -+ pipe_name.buf, alloc - off) < 0) ret = -1;\n> -\n> -- strbuf_release(&realpath);\n> -- return 0;\n> -+ strbuf_release(&pipe_name);\n> -+ return ret;\n> - }\n> ++ /* UNC Path, skip leading slash */\n> ++ if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;\n> ++\n> + off = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n> +- if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)\n> ++ if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)\n> + return -1;\n> \n> - static enum ipc_active_state get_active_state(wchar_t *pipe_path)\n> + /* Handle drive prefix */\n> \n> \n> compat/simple-ipc/ipc-win32.c | 7 ++++++-\n> 1 file changed, 6 insertions(+), 1 deletion(-)\n> \n> diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\n> index 997f6144344..632b12e1ab5 100644\n> --- a/compat/simple-ipc/ipc-win32.c\n> +++ b/compat/simple-ipc/ipc-win32.c\n> @@ -19,13 +19,18 @@\n> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t\n> alloc) { int off = 0;\n> + int real_off = 0;\n> struct strbuf realpath = STRBUF_INIT;\n> \n> if (!strbuf_realpath(&realpath, path, 0)) return -1;\n> \n> + /* UNC Path, skip leading slash */\n> + if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;\n> +\n> off = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n> - if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)\n> + if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)\n> return -1;\n> \n> /* Handle drive prefix */\n> \n> base-commit: f285f68a132109c234d93490671c00218066ace9\n> --\n> gitgitgadget\n\nAre there any other thoughts about this?\n\nI believe that this is the simplest change possible that will ensure that\nfsmonitor correctly handles network repos.\n\n-Eric\n"},{"id":"476143","messageId":"48d44c06-34ba-0a1f-dd0d-7d66bd8dfcb9@jeffhostetler.com","threadId":"59469","inReplyTo":"BL0PR05MB557134157C3D08D982DEB338D9619@BL0PR05MB5571.namprd05.prod.outlook.com","subject":"Re: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-04-26T20:33:42Z","receivedAt":"2023-04-26T20:33:47Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 4/22/23 4:00 PM, Eric DeCosta wrote:\n> \n>> -----Original Message-----\n>> From: Eric DeCosta via GitGitGadget \n>> Sent: Monday, April 10, 2023 3:46 PM\n>> To: git@vger.kernel.org\n>> Cc: Johannes Schindelin ; Jeff Hostetler\n>> ; Eric DeCosta ; Eric\n>> DeCosta \n>> Subject: [PATCH v2] fsmonitor: handle differences between Windows named\n>> pipe functions\n>>\n>> From: Eric DeCosta \n>>\n>> The two functions involved with creating and checking for the existance of\n>> the local fsmonitor named pipe, CratedNamedPipeW and WaitNamedPipeW\n>> appear to handle names with leading slashes or backslashes a bit differently.\n>>\n>> If a repo resolves to a remote file system with the UNC path of //some-\n>> server/some-dir/some-repo, CreateNamedPipeW accepts this name and\n>> creates this named pipe: \\\\.\\pipe\\some-server\\some-dir\\some-repo\n>>\n>> However, when the same UNC path is passed to WaitNamedPipeW, it fails\n>> with ERROR_FILE_NOT_FOUND.\n>>\n>> Skipping the leading slash for UNC paths works for both CreateNamedPipeW\n>> and WaitNamedPipeW. Resulting in a named pipe with the same name as\n>> above that WaitNamedPipeW is able to correctly find.\n>>\n>> Signed-off-by: Eric DeCosta \n[...]\n>>\n>>\n>> compat/simple-ipc/ipc-win32.c | 7 ++++++-\n>> 1 file changed, 6 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-win32.c\n>> index 997f6144344..632b12e1ab5 100644\n>> --- a/compat/simple-ipc/ipc-win32.c\n>> +++ b/compat/simple-ipc/ipc-win32.c\n>> @@ -19,13 +19,18 @@\n>> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t\n>> alloc) { int off = 0;\n>> + int real_off = 0;\n>> struct strbuf realpath = STRBUF_INIT;\n>>\n>> if (!strbuf_realpath(&realpath, path, 0)) return -1;\n>>\n>> + /* UNC Path, skip leading slash */\n>> + if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;\n>> +\n>> off = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n>> - if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)\n>> + if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)\n>> return -1;\n\nI haven't had a chance to test this, but this does look\nlike a minimal solution for the pathname confusion in the\nMSFT APIs.\n\nDo you need to test for realpath.buf[0] and [1] being a forward OR\na backslash ?\n\nShould we set real_off to 2 rather than 1 because we already\nappended a trailing backslash in the swprintf() ?\n\nYou should run one of those NPFS directory listing tools to\nconfirm the exact spelling of the pipe matches your expectation\nhere. Yes, if both functions now work, we should be good, but\nit would be good to confirm your real_off choice, right?\n\nIf would be good to (at least interactively) test that the\ngit-for-windows installer can find the path and stop the daemon\non an upgrade or uninstall. See Johannes' earlier point.\n\nWe should state somewhere that we are running the fsmonitor\ndaemon locally and it is watching a remote file system.\n\nYou should run a few stress tests to ensure that the\nMAX_RDCW_BUF_FALLBACK throttling works and that the daemon\ndoesn't fall behind on a very busy remote file system. (There\nare SMB/CIFS wire protocol limits. See the source.) (I did\ntest this between the combination of systems that I had, but\nYMMV.)\n\nDuring the stress test, it would also be good to test that\nIO generated by a client process on your local machine to the\nremote file system is reported, but also that random IO from\nremote processes on the remote system are seen in the event\nstream. Again, I tested the combinations of machines that I\nhad available at the time.\n\nHope this helps,\nJeff\n\n\n>>\n>> /* Handle drive prefix */\n>>\n>> base-commit: f285f68a132109c234d93490671c00218066ace9\n>> --\n>> gitgitgadget\n> \n> Are there any other thoughts about this?\n> \n> I believe that this is the simplest change possible that will ensure that\n> fsmonitor correctly handles network repos.\n> \n> -Eric\n"},{"id":"476205","messageId":"dfd693ce-a9dc-6a8c-d16a-0573bc37b93e@jeffhostetler.com","threadId":"59469","inReplyTo":"48d44c06-34ba-0a1f-dd0d-7d66bd8dfcb9@jeffhostetler.com","subject":"Re: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-04-27T13:45:07Z","receivedAt":"2023-04-27T13:45:13Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 4/26/23 4:33 PM, Jeff Hostetler wrote:\n...\n>>>\n>>> + /* UNC Path, skip leading slash */\n>>> + if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;\n>>> +\n>>> off = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n>>> - if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)\n>>> + if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)\n>>> return -1;\n> \n> I haven't had a chance to test this, but this does look\n> like a minimal solution for the pathname confusion in the\n> MSFT APIs.\n> \n> Do you need to test for realpath.buf[0] and [1] being a forward OR\n> a backslash ?\n> \n> Should we set real_off to 2 rather than 1 because we already\n> appended a trailing backslash in the swprintf() ?\n\nOn second thought, you might actually need the second slash\nin //./pipe//server/mount/whatever so that the GfW installer\ncan find remote repo path to CWD and stop the daemon.\n\nTesting will tell here.\n\nJeff\n\n> \n> You should run one of those NPFS directory listing tools to\n> confirm the exact spelling of the pipe matches your expectation\n> here.  Yes, if both functions now work, we should be good, but\n> it would be good to confirm your real_off choice, right?\n> \n> If would be good to (at least interactively) test that the\n> git-for-windows installer can find the path and stop the daemon\n> on an upgrade or uninstall.  See Johannes' earlier point.\n> \n> We should state somewhere that we are running the fsmonitor\n> daemon locally and it is watching a remote file system.\n> \n> You should run a few stress tests to ensure that the\n> MAX_RDCW_BUF_FALLBACK throttling works and that the daemon\n> doesn't fall behind on a very busy remote file system.  (There\n> are SMB/CIFS wire protocol limits.  See the source.)  (I did\n> test this between the combination of systems that I had, but\n> YMMV.)\n> \n> During the stress test, it would also be good to test that\n> IO generated by a client process on your local machine to the\n> remote file system is reported, but also that random IO from\n> remote processes on the remote system are seen in the event\n> stream.  Again, I tested the combinations of machines that I\n> had available at the time.\n> \n> Hope this helps,\n> Jeff\n> \n> \n>>>\n>>> /* Handle drive prefix */\n>>>\n>>> base-commit: f285f68a132109c234d93490671c00218066ace9\n>>> -- \n>>> gitgitgadget\n>>\n>> Are there any other thoughts about this?\n>>\n>> I believe that this is the simplest change possible that will ensure that\n>> fsmonitor correctly handles network repos.\n>>\n>> -Eric\n"},{"id":"476808","messageId":"BL0PR05MB5571DD75B080F2790FC82534D9719@BL0PR05MB5571.namprd05.prod.outlook.com","threadId":"59469","inReplyTo":"48d44c06-34ba-0a1f-dd0d-7d66bd8dfcb9@jeffhostetler.com","subject":"RE: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions","fromName":"Eric DeCosta","fromEmail":"edecosta@mathworks.com","sentAt":"2023-05-08T20:30:09Z","receivedAt":"2023-05-08T20:31:03Z","isPatch":true,"sender":{"key":"edecosta@mathworks.com","avatar":"https://avatars.githubusercontent.com/u/67609563?v=4"},"body":"\n> -----Original Message-----\n> From: Jeff Hostetler \n> Sent: Wednesday, April 26, 2023 4:34 PM\n> To: Eric DeCosta ; Eric DeCosta via GitGitGadget\n> ; git@vger.kernel.org\n> Cc: Johannes Schindelin \n> Subject: Re: [PATCH v2] fsmonitor: handle differences between Windows\n> named pipe functions\n> \n> \n> \n> On 4/22/23 4:00 PM, Eric DeCosta wrote:\n> >\n> >> -----Original Message-----\n> >> From: Eric DeCosta via GitGitGadget \n> >> Sent: Monday, April 10, 2023 3:46 PM\n> >> To: git@vger.kernel.org\n> >> Cc: Johannes Schindelin ; Jeff Hostetler\n> >> ; Eric DeCosta ; Eric\n> >> DeCosta \n> >> Subject: [PATCH v2] fsmonitor: handle differences between Windows\n> named\n> >> pipe functions\n> >>\n> >> From: Eric DeCosta \n> >>\n> >> The two functions involved with creating and checking for the existance of\n> >> the local fsmonitor named pipe, CratedNamedPipeW and\n> WaitNamedPipeW\n> >> appear to handle names with leading slashes or backslashes a bit\n> differently.\n> >>\n> >> If a repo resolves to a remote file system with the UNC path of //some-\n> >> server/some-dir/some-repo, CreateNamedPipeW accepts this name and\n> >> creates this named pipe: \\\\.\\pipe\\some-server\\some-dir\\some-repo\n> >>\n> >> However, when the same UNC path is passed to WaitNamedPipeW, it fails\n> >> with ERROR_FILE_NOT_FOUND.\n> >>\n> >> Skipping the leading slash for UNC paths works for both\n> CreateNamedPipeW\n> >> and WaitNamedPipeW. Resulting in a named pipe with the same name as\n> >> above that WaitNamedPipeW is able to correctly find.\n> >>\n> >> Signed-off-by: Eric DeCosta \n> [...]\n> >>\n> >>\n> >> compat/simple-ipc/ipc-win32.c | 7 ++++++-\n> >> 1 file changed, 6 insertions(+), 1 deletion(-)\n> >>\n> >> diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-\n> win32.c\n> >> index 997f6144344..632b12e1ab5 100644\n> >> --- a/compat/simple-ipc/ipc-win32.c\n> >> +++ b/compat/simple-ipc/ipc-win32.c\n> >> @@ -19,13 +19,18 @@\n> >> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t\n> >> alloc) { int off = 0;\n> >> + int real_off = 0;\n> >> struct strbuf realpath = STRBUF_INIT;\n> >>\n> >> if (!strbuf_realpath(&realpath, path, 0)) return -1;\n> >>\n> >> + /* UNC Path, skip leading slash */\n> >> + if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;\n> >> +\n> >> off = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n> >> - if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)\n> >> + if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)\n> >> return -1;\n> \n> I haven't had a chance to test this, but this does look\n> like a minimal solution for the pathname confusion in the\n> MSFT APIs.\n> \n> Do you need to test for realpath.buf[0] and [1] being a forward OR\n> a backslash ?\n> \n> Should we set real_off to 2 rather than 1 because we already\n> appended a trailing backslash in the swprintf() ?\n> \nAttempts to add additional backslashes after \\\\.\\pipe\\\\ are apparently\nignored. The name of the local pipe always ends up looking like this:\n\nfor UNC paths:\n \\\\.\\pipe\\\\some-server\\some-dir\\some-repo\n \nand for local paths:\n \\\\.\\pipe\\\\C_\\some-dir\\some-repo\n \nThus for a UNC path of //some-server/some-dir/some-repo the simplest thing to\ndo is to skip the first slash.\n\n> You should run one of those NPFS directory listing tools to\n> confirm the exact spelling of the pipe matches your expectation\n> here. Yes, if both functions now work, we should be good, but\n> it would be good to confirm your real_off choice, right?\n> \nI have used both PowerShell and Process Explorer and see similar results.\n\n> If would be good to (at least interactively) test that the\n> git-for-windows installer can find the path and stop the daemon\n> on an upgrade or uninstall. See Johannes' earlier point.\n> \nIn regards to the GfW installer, if the daemon is running against\na network mounted repo it reports the following:\n\nCould not stop FSMonitor daemon in some-server\\some-dir\\some-repo\n(output: , errors: fatal cannot change to 'some-server\\some-dir\\some-repo':\nNo such file or directory)\n\nIt looks like the installer may have to do something like:\nlook for \"_\" immediately after \"\\\\.\\pipe\\\\\" and if it does not\nfind it, assume a UNC path.\n\n> We should state somewhere that we are running the fsmonitor\n> daemon locally and it is watching a remote file system.\n> \n> You should run a few stress tests to ensure that the\n> MAX_RDCW_BUF_FALLBACK throttling works and that the daemon\n> doesn't fall behind on a very busy remote file system. (There\n> are SMB/CIFS wire protocol limits. See the source.) (I did\n> test this between the combination of systems that I had, but\n> YMMV.)\n> \n> During the stress test, it would also be good to test that\n> IO generated by a client process on your local machine to the\n> remote file system is reported, but also that random IO from\n> remote processes on the remote system are seen in the event\n> stream. Again, I tested the combinations of machines that I\n> had available at the time.\n> \nI hear what you are saying, however reporting that information increases\nthe scope of this change. As this stands right now the advertised feature\nof allowing fsmonitor to work on network-mounted sandboxes for Windows\nis not working as expected.\n\n-Eric\n\n> Hope this helps,\n> Jeff\n> \n> \n> >>\n> >> /* Handle drive prefix */\n> >>\n> >> base-commit: f285f68a132109c234d93490671c00218066ace9\n> >> --\n> >> gitgitgadget\n> >\n> > Are there any other thoughts about this?\n> >\n> > I believe that this is the simplest change possible that will ensure that\n> > fsmonitor correctly handles network repos.\n> >\n> > -Eric\n\n"},{"id":"477325","messageId":"9cb47e80-6000-8d1b-3132-35f1b4566e5e@jeffhostetler.com","threadId":"59469","inReplyTo":"BL0PR05MB5571DD75B080F2790FC82534D9719@BL0PR05MB5571.namprd05.prod.outlook.com","subject":"Re: [PATCH v2] fsmonitor: handle differences between Windows named pipe functions","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2023-05-15T21:50:59Z","receivedAt":"2023-05-15T21:51:06Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\nSorry this got lost in my inbox.\n\nOn 5/8/23 4:30 PM, Eric DeCosta wrote:\n> \n>> -----Original Message-----\n>> From: Jeff Hostetler \n>> Sent: Wednesday, April 26, 2023 4:34 PM\n>> To: Eric DeCosta ; Eric DeCosta via GitGitGadget\n>> ; git@vger.kernel.org\n>> Cc: Johannes Schindelin \n>> Subject: Re: [PATCH v2] fsmonitor: handle differences between Windows\n>> named pipe functions\n>>\n>>\n>>\n>> On 4/22/23 4:00 PM, Eric DeCosta wrote:\n>>>\n>>>> -----Original Message-----\n>>>> From: Eric DeCosta via GitGitGadget \n>>>> Sent: Monday, April 10, 2023 3:46 PM\n>>>> To: git@vger.kernel.org\n>>>> Cc: Johannes Schindelin ; Jeff Hostetler\n>>>> ; Eric DeCosta ; Eric\n>>>> DeCosta \n>>>> Subject: [PATCH v2] fsmonitor: handle differences between Windows\n>> named\n>>>> pipe functions\n>>>>\n>>>> From: Eric DeCosta \n>>>>\n>>>> The two functions involved with creating and checking for the existance of\n>>>> the local fsmonitor named pipe, CratedNamedPipeW and\n>> WaitNamedPipeW\n>>>> appear to handle names with leading slashes or backslashes a bit\n>> differently.\n>>>>\n>>>> If a repo resolves to a remote file system with the UNC path of //some-\n>>>> server/some-dir/some-repo, CreateNamedPipeW accepts this name and\n>>>> creates this named pipe: \\\\.\\pipe\\some-server\\some-dir\\some-repo\n>>>>\n>>>> However, when the same UNC path is passed to WaitNamedPipeW, it fails\n>>>> with ERROR_FILE_NOT_FOUND.\n>>>>\n>>>> Skipping the leading slash for UNC paths works for both\n>> CreateNamedPipeW\n>>>> and WaitNamedPipeW. Resulting in a named pipe with the same name as\n>>>> above that WaitNamedPipeW is able to correctly find.\n>>>>\n>>>> Signed-off-by: Eric DeCosta \n>> [...]\n>>>>\n>>>>\n>>>> compat/simple-ipc/ipc-win32.c | 7 ++++++-\n>>>> 1 file changed, 6 insertions(+), 1 deletion(-)\n>>>>\n>>>> diff --git a/compat/simple-ipc/ipc-win32.c b/compat/simple-ipc/ipc-\n>> win32.c\n>>>> index 997f6144344..632b12e1ab5 100644\n>>>> --- a/compat/simple-ipc/ipc-win32.c\n>>>> +++ b/compat/simple-ipc/ipc-win32.c\n>>>> @@ -19,13 +19,18 @@\n>>>> static int initialize_pipe_name(const char *path, wchar_t *wpath, size_t\n>>>> alloc) { int off = 0;\n>>>> + int real_off = 0;\n>>>> struct strbuf realpath = STRBUF_INIT;\n>>>>\n>>>> if (!strbuf_realpath(&realpath, path, 0)) return -1;\n>>>>\n>>>> + /* UNC Path, skip leading slash */\n>>>> + if (realpath.buf[0] == '/' && realpath.buf[1] == '/') real_off = 1;\n>>>> +\n>>>> off = swprintf(wpath, alloc, L\"\\\\\\\\.\\\\pipe\\\\\");\n>>>> - if (xutftowcs(wpath + off, realpath.buf, alloc - off) < 0)\n>>>> + if (xutftowcs(wpath + off, realpath.buf + real_off, alloc - off) < 0)\n>>>> return -1;\n>>\n>> I haven't had a chance to test this, but this does look\n>> like a minimal solution for the pathname confusion in the\n>> MSFT APIs.\n>>\n>> Do you need to test for realpath.buf[0] and [1] being a forward OR\n>> a backslash ?\n>>\n>> Should we set real_off to 2 rather than 1 because we already\n>> appended a trailing backslash in the swprintf() ?\n>>\n> Attempts to add additional backslashes after \\\\.\\pipe\\\\ are apparently\n> ignored. The name of the local pipe always ends up looking like this:\n> \n> for UNC paths:\n> \\\\.\\pipe\\\\some-server\\some-dir\\some-repo\n> \n> and for local paths:\n> \\\\.\\pipe\\\\C_\\some-dir\\some-repo\n> \n> Thus for a UNC path of //some-server/some-dir/some-repo the simplest thing to\n> do is to skip the first slash.\n\nOk thanks. Just being paranoid...\n\n> \n>> You should run one of those NPFS directory listing tools to\n>> confirm the exact spelling of the pipe matches your expectation\n>> here. Yes, if both functions now work, we should be good, but\n>> it would be good to confirm your real_off choice, right?\n>>\n> I have used both PowerShell and Process Explorer and see similar results.\n> \n\ngood. thanks.\n\n>> If would be good to (at least interactively) test that the\n>> git-for-windows installer can find the path and stop the daemon\n>> on an upgrade or uninstall. See Johannes' earlier point.\n>>\n> In regards to the GfW installer, if the daemon is running against\n> a network mounted repo it reports the following:\n> \n> Could not stop FSMonitor daemon in some-server\\some-dir\\some-repo\n> (output: , errors: fatal cannot change to 'some-server\\some-dir\\some-repo':\n> No such file or directory)\n> \n> It looks like the installer may have to do something like:\n> look for \"_\" immediately after \"\\\\.\\pipe\\\\\" and if it does not\n> find it, assume a UNC path.\n> \n\nok. perhaps you could log an issue on github.com/git-for-windows/git.git\nto describe this.\n\n>> We should state somewhere that we are running the fsmonitor\n>> daemon locally and it is watching a remote file system.\n>>\n>> You should run a few stress tests to ensure that the\n>> MAX_RDCW_BUF_FALLBACK throttling works and that the daemon\n>> doesn't fall behind on a very busy remote file system. (There\n>> are SMB/CIFS wire protocol limits. See the source.) (I did\n>> test this between the combination of systems that I had, but\n>> YMMV.)\n>>\n>> During the stress test, it would also be good to test that\n>> IO generated by a client process on your local machine to the\n>> remote file system is reported, but also that random IO from\n>> remote processes on the remote system are seen in the event\n>> stream. Again, I tested the combinations of machines that I\n>> had available at the time.\n>>\n> I hear what you are saying, however reporting that information increases\n> the scope of this change. As this stands right now the advertised feature\n> of allowing fsmonitor to work on network-mounted sandboxes for Windows\n> is not working as expected.\n\nunderstood. my only concern was that remotely mounted file systems\ndidn't get a lot of testing. it worked, but i didn't have resources\nto really stress it. but then again, it if falls behind it will\nforce a resync, so you *shouldn't* get a wrong answer.\n\nThanks for all your work (and patience with me) on this.\nJeff\n\n\n> \n> -Eric\n> \n>> Hope this helps,\n>> Jeff\n>>\n>>\n>>>>\n>>>> /* Handle drive prefix */\n>>>>\n>>>> base-commit: f285f68a132109c234d93490671c00218066ace9\n>>>> --\n>>>> gitgitgadget\n>>>\n>>> Are there any other thoughts about this?\n>>>\n>>> I believe that this is the simplest change possible that will ensure that\n>>> fsmonitor correctly handles network repos.\n>>>\n>>> -Eric\n> \n"}]}