mbox series

[v7,00/30] Builtin FSMonitor Part 3

Message ID pull.1143.v7.git.1653336765.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series Builtin FSMonitor Part 3 | expand

Message

Victoria Dye via GitGitGadget May 23, 2022, 8:12 p.m. UTC
Here is version 7 of part 3 of FSMonitor.

This version mainly addresses comments received on V6.

 1. A few typos.
 2. Cleanup of some wide-char string manipulations in Windows code.
 3. Fix some coding-style issues in the unit tests: don't capitalize test
    case description and single-quote the descriptions when possible.
 4. Add t/helper/test-hexdump.c and use that in t7527 rather than
    /usr/bin/od in the Unicode/UTF8 compatibility tests for macOS.
 5. Refactor the implicit daemon shutdown tests to retry a few times rather
    than a fixed sleep. This helps avoid races in the CI build runs.

Here is a range-diff against V6:

 1:  8b7c5f4e23 !  1:  26144c5865 fsm-listen-win32: handle shortnames
    @@ compat/fsmonitor/fsm-listen-win32.c: struct one_watch
     +   * clients.)
     +   */
     +  BOOL has_shortnames;
    -+  BOOL has_tilda;
    ++  BOOL has_tilde;
     +  wchar_t dotgit_shortname[16]; /* for 8.3 name */
      };
      
    @@ compat/fsmonitor/fsm-listen-win32.c: static int normalize_path_in_utf8(FILE_NOTI
     +{
     +  wchar_t buf_in[MAX_PATH + 1];
     +  wchar_t buf_out[MAX_PATH + 1];
    -+  wchar_t *last_slash = NULL;
    -+  wchar_t *last_bslash = NULL;
     +  wchar_t *last;
    ++  wchar_t *p;
     +
     +  /* build L"<wt-root-path>/.git" */
    -+  wcscpy(buf_in, watch->wpath_longname);
    -+  wcscpy(buf_in + watch->wpath_longname_len, L".git");
    ++  swprintf(buf_in, ARRAY_SIZE(buf_in) - 1, L"%s.git",
    ++           watch->wpath_longname);
     +
    -+  if (!GetShortPathNameW(buf_in, buf_out, MAX_PATH))
    ++  if (!GetShortPathNameW(buf_in, buf_out, ARRAY_SIZE(buf_out)))
     +          return;
     +
    -+  last_slash = wcsrchr(buf_out, L'/');
    -+  last_bslash = wcsrchr(buf_out, L'\\');
    -+  if (last_slash > last_bslash)
    -+          last = last_slash + 1;
    -+  else if (last_bslash)
    -+          last = last_bslash + 1;
    -+  else
    -+          last = buf_out;
    ++  /*
    ++   * Get the final filename component of the shortpath.
    ++   * We know that the path does not have a final slash.
    ++   */
    ++  for (last = p = buf_out; *p; p++)
    ++          if (*p == L'/' || *p == '\\')
    ++                  last = p + 1;
     +
     +  if (!wcscmp(last, L".git"))
     +          return;
    @@ compat/fsmonitor/fsm-listen-win32.c: static int normalize_path_in_utf8(FILE_NOTI
     +   * Lets test this.
     +   */
     +  if (wcschr(watch->dotgit_shortname, L'~'))
    -+          watch->has_tilda = 1;
    ++          watch->has_tilde = 1;
     +}
     +
     +enum get_relative_result {
    @@ compat/fsmonitor/fsm-listen-win32.c: static int normalize_path_in_utf8(FILE_NOTI
     +static enum get_relative_result get_relative_longname(
     +  struct one_watch *watch,
     +  const wchar_t *wpath, DWORD wpath_len,
    -+  wchar_t *wpath_longname)
    ++  wchar_t *wpath_longname, size_t bufsize_wpath_longname)
     +{
     +  wchar_t buf_in[2 * MAX_PATH + 1];
     +  wchar_t buf_out[MAX_PATH + 1];
     +  DWORD root_len;
    ++  DWORD out_len;
     +
    -+  /* Build L"<wt-root-path>/<event-rel-path>" */
    ++  /*
    ++   * Build L"<wt-root-path>/<event-rel-path>"
    ++   * Note that the <event-rel-path> might not be null terminated
    ++   * so we avoid swprintf() constructions.
    ++   */
     +  root_len = watch->wpath_longname_len;
    ++  if (root_len + wpath_len >= ARRAY_SIZE(buf_in)) {
    ++          /*
    ++           * This should not happen.  We cannot append the observed
    ++           * relative path onto the end of the worktree root path
    ++           * without overflowing the buffer.  Just give up.
    ++           */
    ++          return GRR_SHUTDOWN;
    ++  }
     +  wcsncpy(buf_in, watch->wpath_longname, root_len);
     +  wcsncpy(buf_in + root_len, wpath, wpath_len);
     +  buf_in[root_len + wpath_len] = 0;
     +
     +  /*
     +   * We don't actually know if the source pathname is a
    -+   * shortname or a longname.  This routine allows either to be
    -+   * given as input.
    ++   * shortname or a longname.  This Windows routine allows
    ++   * either to be given as input.
     +   */
    -+  if (!GetLongPathNameW(buf_in, buf_out, MAX_PATH)) {
    ++  out_len = GetLongPathNameW(buf_in, buf_out, ARRAY_SIZE(buf_out));
    ++  if (!out_len) {
     +          /*
     +           * The shortname to longname conversion can fail for
     +           * various reasons, for example if the file has been
    @@ compat/fsmonitor/fsm-listen-win32.c: static int normalize_path_in_utf8(FILE_NOTI
     +          return GRR_SHUTDOWN;
     +  }
     +
    ++  if (out_len - root_len >= bufsize_wpath_longname) {
    ++          /*
    ++           * This should not happen.  We cannot copy the root-relative
    ++           * portion of the path into the provided buffer without an
    ++           * overrun.  Just give up.
    ++           */
    ++          return GRR_SHUTDOWN;
    ++  }
    ++
     +  /* Return the worktree root-relative portion of the longname. */
     +
     +  wcscpy(wpath_longname, buf_out + root_len);
    @@ compat/fsmonitor/fsm-listen-win32.c: static struct one_watch *create_watch(struc
                return NULL;
        }
      
    -+  if (!GetLongPathNameW(wpath, wpath_longname, MAX_PATH)) {
    ++  len_longname = GetLongPathNameW(wpath, wpath_longname,
    ++                                  ARRAY_SIZE(wpath_longname));
    ++  if (!len_longname) {
     +          error(_("[GLE %ld] could not get longname of '%s'"),
     +                GetLastError(), path);
     +          CloseHandle(hDir);
     +          return NULL;
     +  }
     +
    -+  len_longname = wcslen(wpath_longname);
     +  if (wpath_longname[len_longname - 1] != L'/' &&
     +      wpath_longname[len_longname - 1] != L'\\') {
     +          wpath_longname[len_longname++] = L'/';
    @@ compat/fsmonitor/fsm-listen-win32.c: static int process_worktree_events(struct f
     -          case IS_INSIDE_DOT_GIT:
     -                  /* ignore everything inside of "<worktree>/.git/" */
     -                  break;
    -+                  if (watch->has_tilda && !wcschr(wpath, L'~')) {
    ++                  if (watch->has_tilde && !wcschr(wpath, L'~')) {
     +                          /*
    -+                           * Shortnames on this filesystem have tildas
    ++                           * Shortnames on this filesystem have tildes
     +                           * and the notification path does not have
     +                           * one, so we assume that it is a longname.
     +                           */
    @@ compat/fsmonitor/fsm-listen-win32.c: static int process_worktree_events(struct f
     -                                             "fsm-listen/dotgit",
     -                                             "removed");
     +                  grr = get_relative_longname(watch, wpath, wpath_len,
    -+                                              wpath_longname);
    ++                                              wpath_longname,
    ++                                              ARRAY_SIZE(wpath_longname));
     +                  switch (grr) {
     +                  case GRR_NO_CONVERSION_NEEDED: /* use info buffer as is */
     +                          break;
 2:  5b246bec24 !  2:  1bf2e36b6a t7527: test FSMonitor on repos with Unicode root paths
    @@ t/t7527-builtin-fsmonitor.sh: do
     +u_values="$u1 $u2"
     +for u in $u_values
     +do
    -+  test_expect_success "Unicode in repo root path: $u" '
    ++  test_expect_success "unicode in repo root path: $u" '
     +          test_when_finished "stop_daemon_delete_repo $u" &&
     +
     +          git init "$u" &&
 3:  8a474d6999 =  3:  4bca494bb2 t/helper/fsmonitor-client: create stress test
 4:  72b94acd5f !  4:  663deabc3f fsmonitor-settings: bare repos are incompatible with FSMonitor
    @@ fsmonitor-settings.h
     + */
     +enum fsmonitor_reason {
     +  FSMONITOR_REASON_UNTESTED = 0,
    -+  FSMONITOR_REASON_OK, /* no incompatibility or when disbled */
    ++  FSMONITOR_REASON_OK, /* no incompatibility or when disabled */
     +  FSMONITOR_REASON_BARE,
     +};
     +
 5:  2e225c3f4f =  5:  7cb0180a1e fsmonitor-settings: stub in Win32-specific incompatibility checking
 6:  e0d3bdf755 !  6:  9774faddc4 fsmonitor-settings: VFS for Git virtual repos are incompatible
    @@ fsmonitor-settings.c: char *fsm_settings__get_incompatible_msg(const struct repo
      ## fsmonitor-settings.h ##
     @@ fsmonitor-settings.h: enum fsmonitor_reason {
        FSMONITOR_REASON_UNTESTED = 0,
    -   FSMONITOR_REASON_OK, /* no incompatibility or when disbled */
    +   FSMONITOR_REASON_OK, /* no incompatibility or when disabled */
        FSMONITOR_REASON_BARE,
     +  FSMONITOR_REASON_VFS4GIT, /* VFS for Git virtualization */
      };
 7:  c50ed29a31 =  7:  f7ef7dcffc fsmonitor-settings: stub in macOS-specific incompatibility checking
 8:  1f5b772d42 !  8:  dc2dfd6793 fsmonitor-settings: remote repos on macOS are incompatible
    @@ fsmonitor-settings.c: char *fsm_settings__get_incompatible_msg(const struct repo
      ## fsmonitor-settings.h ##
     @@ fsmonitor-settings.h: enum fsmonitor_reason {
        FSMONITOR_REASON_UNTESTED = 0,
    -   FSMONITOR_REASON_OK, /* no incompatibility or when disbled */
    +   FSMONITOR_REASON_OK, /* no incompatibility or when disabled */
        FSMONITOR_REASON_BARE,
     +  FSMONITOR_REASON_ERROR, /* FS error probing for compatibility */
     +  FSMONITOR_REASON_REMOTE,
 9:  495e54049b =  9:  5627038aaa fsmonitor-settings: remote repos on Windows are incompatible
10:  4b52083698 = 10:  9a12cc78b5 fsmonitor-settings: NTFS and FAT32 on MacOS are incompatible
11:  d4a4263d37 = 11:  aaff000cec unpack-trees: initialize fsmonitor_has_run_once in o->result
12:  f4feb00ec2 = 12:  4f2b15d3d1 fsm-listen-darwin: ignore FSEvents caused by xattr changes on macOS
13:  dbb983fd9d = 13:  427dec412a fsmonitor--daemon: cd out of worktree root
14:  ae90b99ea9 = 14:  51b266b06e fsmonitor--daemon: prepare for adding health thread
15:  b6c5800095 = 15:  594e0ae243 fsmonitor--daemon: rename listener thread related variables
16:  32fc6ba743 = 16:  c2b5c02ed3 fsmonitor--daemon: stub in health thread
17:  77bc037481 = 17:  46a5ae2a63 fsm-health-win32: add polling framework to monitor daemon health
18:  b06edd995e = 18:  7cf1be5f8e fsm-health-win32: force shutdown daemon if worktree root moves
19:  1bd5f34624 = 19:  95cf1299d4 fsm-listen-darwin: shutdown daemon if worktree root is moved/renamed
20:  48af0813de = 20:  b020bfb456 fsmonitor: optimize processing of directory events
21:  a9b35e770f = 21:  d058d7e0c0 t7527: FSMonitor tests for directory moves
22:  26308936af = 22:  f5dac28681 t/perf/p7527: add perf test for builtin FSMonitor
23:  d0e25f6bac ! 23:  92f5c0d2c8 fsmonitor: never set CE_FSMONITOR_VALID on submodules
    @@ t/t7527-builtin-fsmonitor.sh: do
     +  git -C super/dir_1/dir_2/sub clean -d -f
     +}
     +
    -+test_expect_success "Submodule always visited" '
    ++test_expect_success 'submodule always visited' '
     +  test_when_finished "git -C super fsmonitor--daemon stop; \
     +                      rm -rf super; \
     +                      rm -rf sub" &&
24:  410dd2d292 ! 24:  40b80adbb3 t7527: test FSMonitor on case insensitive+preserving file system
    @@ Commit message
         Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
     
      ## t/t7527-builtin-fsmonitor.sh ##
    -@@ t/t7527-builtin-fsmonitor.sh: test_expect_success "Submodule always visited" '
    +@@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'submodule always visited' '
        my_match_and_clean
      '
      
25:  cd7c55b0d3 = 25:  b93f064269 fsmonitor: on macOS also emit NFC spelling for NFD pathname
 -:  ---------- > 26:  6f2e935f14 t/helper/hexdump: add helper to print hexdump of stdin
26:  8278f32c4d ! 27:  6a83086995 t/lib-unicode-nfc-nfd: helper prereqs for testing unicode nfc/nfd
    @@ t/lib-unicode-nfc-nfd.sh (new)
     +# and on APFS, NFC paths are preserved.  As we have established
     +# above, this is independent of "composition sensitivity".
     +#
    -+# 0000000 63 5f c3 a9
    -+#
    -+# (/usr/bin/od output contains different amount of whitespace
    -+# on different platforms, so we need the wildcards here.)
    -+#
     +test_lazy_prereq UNICODE_NFC_PRESERVED '
     +  mkdir c_${utf8_nfc} &&
    -+  ls | od -t x1 | grep "63 *5f *c3 *a9"
    ++  ls | test-tool hexdump | grep "63 5f c3 a9"
     +'
     +
     +# Is the spelling of an NFD pathname preserved on disk?
     +#
    -+# 0000000 64 5f 65 cc 81
    -+#
     +test_lazy_prereq UNICODE_NFD_PRESERVED '
     +  mkdir d_${utf8_nfd} &&
    -+  ls | od -t x1 | grep "64 *5f *65 *cc *81"
    ++  ls | test-tool hexdump | grep "64 5f 65 cc 81"
     +'
    -+  mkdir c_${utf8_nfc} &&
    -+  mkdir d_${utf8_nfd} &&
     +
     +# The following _DOUBLE_ forms are more for my curiosity,
     +# but there may be quirks lurking when there are multiple
    @@ t/lib-unicode-nfc-nfd.sh (new)
     +#
     +test_lazy_prereq UNICODE_DOUBLE_NFC_PRESERVED '
     +  mkdir c_${greek_nfc} &&
    -+  ls | od -t x1 | grep "63 *5f *e1 *bd *a7"
    ++  ls | test-tool hexdump | grep "63 5f e1 bd a7"
     +'
     +
     +# See if the NFD spelling appears on the disk.
     +#
     +test_lazy_prereq UNICODE_DOUBLE_NFD_PRESERVED '
     +  mkdir d_${greek_nfd2} &&
    -+  ls | od -t x1 | grep "64 *5f *cf *89 *cc *94 *cd *82"
    ++  ls | test-tool hexdump | grep "64 5f cf 89 cc 94 cd 82"
     +'
     +
     +# The following is for debugging. I found it useful when
27:  4efb3a4383 = 28:  f9a7869d20 t7527: test Unicode NFC/NFD handling on MacOS
28:  df1b4f3a80 ! 29:  9fc7c97092 fsmonitor--daemon: allow --super-prefix argument
    @@ git.c: static struct cmd_struct commands[] = {
        { "grep", cmd_grep, RUN_SETUP_GENTLY },
     
      ## t/t7527-builtin-fsmonitor.sh ##
    -@@ t/t7527-builtin-fsmonitor.sh: test_expect_success "Submodule always visited" '
    +@@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'submodule always visited' '
        my_match_and_clean
      '
      
    @@ t/t7527-builtin-fsmonitor.sh: test_expect_success "Submodule always visited" '
     +  tr '\047' Q <$1 | grep -e "$msg"
     +}
     +
    -+test_expect_success "Stray Submodule super-prefix warning" '
    ++test_expect_success "stray submodule super-prefix warning" '
     +  test_when_finished "rm -rf super; \
     +                      rm -rf sub;   \
     +                      rm super-sub.trace" &&
 -:  ---------- > 30:  ca833ecc7a t7527: improve implicit shutdown testing in fsmonitor--daemon


Jeff Hostetler (30):
  fsm-listen-win32: handle shortnames
  t7527: test FSMonitor on repos with Unicode root paths
  t/helper/fsmonitor-client: create stress test
  fsmonitor-settings: bare repos are incompatible with FSMonitor
  fsmonitor-settings: stub in Win32-specific incompatibility checking
  fsmonitor-settings: VFS for Git virtual repos are incompatible
  fsmonitor-settings: stub in macOS-specific incompatibility checking
  fsmonitor-settings: remote repos on macOS are incompatible
  fsmonitor-settings: remote repos on Windows are incompatible
  fsmonitor-settings: NTFS and FAT32 on MacOS are incompatible
  unpack-trees: initialize fsmonitor_has_run_once in o->result
  fsm-listen-darwin: ignore FSEvents caused by xattr changes on macOS
  fsmonitor--daemon: cd out of worktree root
  fsmonitor--daemon: prepare for adding health thread
  fsmonitor--daemon: rename listener thread related variables
  fsmonitor--daemon: stub in health thread
  fsm-health-win32: add polling framework to monitor daemon health
  fsm-health-win32: force shutdown daemon if worktree root moves
  fsm-listen-darwin: shutdown daemon if worktree root is moved/renamed
  fsmonitor: optimize processing of directory events
  t7527: FSMonitor tests for directory moves
  t/perf/p7527: add perf test for builtin FSMonitor
  fsmonitor: never set CE_FSMONITOR_VALID on submodules
  t7527: test FSMonitor on case insensitive+preserving file system
  fsmonitor: on macOS also emit NFC spelling for NFD pathname
  t/helper/hexdump: add helper to print hexdump of stdin
  t/lib-unicode-nfc-nfd: helper prereqs for testing unicode nfc/nfd
  t7527: test Unicode NFC/NFD handling on MacOS
  fsmonitor--daemon: allow --super-prefix argument
  t7527: improve implicit shutdown testing in fsmonitor--daemon

 Makefile                               |  20 +-
 builtin/fsmonitor--daemon.c            | 116 ++++++-
 builtin/update-index.c                 |  16 +
 compat/fsmonitor/fsm-health-darwin.c   |  24 ++
 compat/fsmonitor/fsm-health-win32.c    | 278 ++++++++++++++++
 compat/fsmonitor/fsm-health.h          |  47 +++
 compat/fsmonitor/fsm-listen-darwin.c   | 122 ++++++-
 compat/fsmonitor/fsm-listen-win32.c    | 436 ++++++++++++++++++++-----
 compat/fsmonitor/fsm-listen.h          |   2 +-
 compat/fsmonitor/fsm-settings-darwin.c |  89 +++++
 compat/fsmonitor/fsm-settings-win32.c  | 137 ++++++++
 config.mak.uname                       |   5 +
 contrib/buildsystems/CMakeLists.txt    |   8 +
 fsmonitor--daemon.h                    |  11 +-
 fsmonitor-settings.c                   | 167 ++++++++--
 fsmonitor-settings.h                   |  33 ++
 fsmonitor.c                            |  73 ++++-
 fsmonitor.h                            |  11 +
 git.c                                  |   2 +-
 t/helper/test-fsmonitor-client.c       | 106 ++++++
 t/helper/test-hexdump.c                |  24 ++
 t/helper/test-tool.c                   |   1 +
 t/helper/test-tool.h                   |   1 +
 t/lib-unicode-nfc-nfd.sh               | 158 +++++++++
 t/perf/p7527-builtin-fsmonitor.sh      | 257 +++++++++++++++
 t/t7519-status-fsmonitor.sh            |  32 ++
 t/t7527-builtin-fsmonitor.sh           | 401 ++++++++++++++++++++++-
 unpack-trees.c                         |   1 +
 28 files changed, 2429 insertions(+), 149 deletions(-)
 create mode 100644 compat/fsmonitor/fsm-health-darwin.c
 create mode 100644 compat/fsmonitor/fsm-health-win32.c
 create mode 100644 compat/fsmonitor/fsm-health.h
 create mode 100644 compat/fsmonitor/fsm-settings-darwin.c
 create mode 100644 compat/fsmonitor/fsm-settings-win32.c
 create mode 100644 t/helper/test-hexdump.c
 create mode 100755 t/lib-unicode-nfc-nfd.sh
 create mode 100755 t/perf/p7527-builtin-fsmonitor.sh


base-commit: 5eb696daba2fe108d4d9ba2ccf4b357447ef9946
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1143%2Fjeffhostetler%2Fbuiltin-fsmonitor-part3-v7
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1143/jeffhostetler/builtin-fsmonitor-part3-v7
Pull-Request: https://github.com/gitgitgadget/git/pull/1143

Range-diff vs v6:

  1:  8b7c5f4e234 !  1:  26144c58659 fsm-listen-win32: handle shortnames
     @@ compat/fsmonitor/fsm-listen-win32.c: struct one_watch
      +	 * clients.)
      +	 */
      +	BOOL has_shortnames;
     -+	BOOL has_tilda;
     ++	BOOL has_tilde;
      +	wchar_t dotgit_shortname[16]; /* for 8.3 name */
       };
       
     @@ compat/fsmonitor/fsm-listen-win32.c: normalize:
      +{
      +	wchar_t buf_in[MAX_PATH + 1];
      +	wchar_t buf_out[MAX_PATH + 1];
     -+	wchar_t *last_slash = NULL;
     -+	wchar_t *last_bslash = NULL;
      +	wchar_t *last;
     ++	wchar_t *p;
      +
      +	/* build L"<wt-root-path>/.git" */
     -+	wcscpy(buf_in, watch->wpath_longname);
     -+	wcscpy(buf_in + watch->wpath_longname_len, L".git");
     ++	swprintf(buf_in, ARRAY_SIZE(buf_in) - 1, L"%s.git",
     ++		 watch->wpath_longname);
      +
     -+	if (!GetShortPathNameW(buf_in, buf_out, MAX_PATH))
     ++	if (!GetShortPathNameW(buf_in, buf_out, ARRAY_SIZE(buf_out)))
      +		return;
      +
     -+	last_slash = wcsrchr(buf_out, L'/');
     -+	last_bslash = wcsrchr(buf_out, L'\\');
     -+	if (last_slash > last_bslash)
     -+		last = last_slash + 1;
     -+	else if (last_bslash)
     -+		last = last_bslash + 1;
     -+	else
     -+		last = buf_out;
     ++	/*
     ++	 * Get the final filename component of the shortpath.
     ++	 * We know that the path does not have a final slash.
     ++	 */
     ++	for (last = p = buf_out; *p; p++)
     ++		if (*p == L'/' || *p == '\\')
     ++			last = p + 1;
      +
      +	if (!wcscmp(last, L".git"))
      +		return;
     @@ compat/fsmonitor/fsm-listen-win32.c: normalize:
      +	 * Lets test this.
      +	 */
      +	if (wcschr(watch->dotgit_shortname, L'~'))
     -+		watch->has_tilda = 1;
     ++		watch->has_tilde = 1;
      +}
      +
      +enum get_relative_result {
     @@ compat/fsmonitor/fsm-listen-win32.c: normalize:
      +static enum get_relative_result get_relative_longname(
      +	struct one_watch *watch,
      +	const wchar_t *wpath, DWORD wpath_len,
     -+	wchar_t *wpath_longname)
     ++	wchar_t *wpath_longname, size_t bufsize_wpath_longname)
      +{
      +	wchar_t buf_in[2 * MAX_PATH + 1];
      +	wchar_t buf_out[MAX_PATH + 1];
      +	DWORD root_len;
     ++	DWORD out_len;
      +
     -+	/* Build L"<wt-root-path>/<event-rel-path>" */
     ++	/*
     ++	 * Build L"<wt-root-path>/<event-rel-path>"
     ++	 * Note that the <event-rel-path> might not be null terminated
     ++	 * so we avoid swprintf() constructions.
     ++	 */
      +	root_len = watch->wpath_longname_len;
     ++	if (root_len + wpath_len >= ARRAY_SIZE(buf_in)) {
     ++		/*
     ++		 * This should not happen.  We cannot append the observed
     ++		 * relative path onto the end of the worktree root path
     ++		 * without overflowing the buffer.  Just give up.
     ++		 */
     ++		return GRR_SHUTDOWN;
     ++	}
      +	wcsncpy(buf_in, watch->wpath_longname, root_len);
      +	wcsncpy(buf_in + root_len, wpath, wpath_len);
      +	buf_in[root_len + wpath_len] = 0;
      +
      +	/*
      +	 * We don't actually know if the source pathname is a
     -+	 * shortname or a longname.  This routine allows either to be
     -+	 * given as input.
     ++	 * shortname or a longname.  This Windows routine allows
     ++	 * either to be given as input.
      +	 */
     -+	if (!GetLongPathNameW(buf_in, buf_out, MAX_PATH)) {
     ++	out_len = GetLongPathNameW(buf_in, buf_out, ARRAY_SIZE(buf_out));
     ++	if (!out_len) {
      +		/*
      +		 * The shortname to longname conversion can fail for
      +		 * various reasons, for example if the file has been
     @@ compat/fsmonitor/fsm-listen-win32.c: normalize:
      +		return GRR_SHUTDOWN;
      +	}
      +
     ++	if (out_len - root_len >= bufsize_wpath_longname) {
     ++		/*
     ++		 * This should not happen.  We cannot copy the root-relative
     ++		 * portion of the path into the provided buffer without an
     ++		 * overrun.  Just give up.
     ++		 */
     ++		return GRR_SHUTDOWN;
     ++	}
     ++
      +	/* Return the worktree root-relative portion of the longname. */
      +
      +	wcscpy(wpath_longname, buf_out + root_len);
     @@ compat/fsmonitor/fsm-listen-win32.c: static struct one_watch *create_watch(struc
       		return NULL;
       	}
       
     -+	if (!GetLongPathNameW(wpath, wpath_longname, MAX_PATH)) {
     ++	len_longname = GetLongPathNameW(wpath, wpath_longname,
     ++					ARRAY_SIZE(wpath_longname));
     ++	if (!len_longname) {
      +		error(_("[GLE %ld] could not get longname of '%s'"),
      +		      GetLastError(), path);
      +		CloseHandle(hDir);
      +		return NULL;
      +	}
      +
     -+	len_longname = wcslen(wpath_longname);
      +	if (wpath_longname[len_longname - 1] != L'/' &&
      +	    wpath_longname[len_longname - 1] != L'\\') {
      +		wpath_longname[len_longname++] = L'/';
     @@ compat/fsmonitor/fsm-listen-win32.c: static int process_worktree_events(struct f
      -		case IS_INSIDE_DOT_GIT:
      -			/* ignore everything inside of "<worktree>/.git/" */
      -			break;
     -+			if (watch->has_tilda && !wcschr(wpath, L'~')) {
     ++			if (watch->has_tilde && !wcschr(wpath, L'~')) {
      +				/*
     -+				 * Shortnames on this filesystem have tildas
     ++				 * Shortnames on this filesystem have tildes
      +				 * and the notification path does not have
      +				 * one, so we assume that it is a longname.
      +				 */
     @@ compat/fsmonitor/fsm-listen-win32.c: static int process_worktree_events(struct f
      -						   "fsm-listen/dotgit",
      -						   "removed");
      +			grr = get_relative_longname(watch, wpath, wpath_len,
     -+						    wpath_longname);
     ++						    wpath_longname,
     ++						    ARRAY_SIZE(wpath_longname));
      +			switch (grr) {
      +			case GRR_NO_CONVERSION_NEEDED: /* use info buffer as is */
      +				break;
  2:  5b246bec247 !  2:  1bf2e36b6ad t7527: test FSMonitor on repos with Unicode root paths
     @@ t/t7527-builtin-fsmonitor.sh: do
      +u_values="$u1 $u2"
      +for u in $u_values
      +do
     -+	test_expect_success "Unicode in repo root path: $u" '
     ++	test_expect_success "unicode in repo root path: $u" '
      +		test_when_finished "stop_daemon_delete_repo $u" &&
      +
      +		git init "$u" &&
  3:  8a474d69999 =  3:  4bca494bb22 t/helper/fsmonitor-client: create stress test
  4:  72b94acd5fe !  4:  663deabc3f6 fsmonitor-settings: bare repos are incompatible with FSMonitor
     @@ fsmonitor-settings.h
      + */
      +enum fsmonitor_reason {
      +	FSMONITOR_REASON_UNTESTED = 0,
     -+	FSMONITOR_REASON_OK, /* no incompatibility or when disbled */
     ++	FSMONITOR_REASON_OK, /* no incompatibility or when disabled */
      +	FSMONITOR_REASON_BARE,
      +};
      +
  5:  2e225c3f4f2 =  5:  7cb0180a1ed fsmonitor-settings: stub in Win32-specific incompatibility checking
  6:  e0d3bdf7556 !  6:  9774faddc45 fsmonitor-settings: VFS for Git virtual repos are incompatible
     @@ fsmonitor-settings.c: char *fsm_settings__get_incompatible_msg(const struct repo
       ## fsmonitor-settings.h ##
      @@ fsmonitor-settings.h: enum fsmonitor_reason {
       	FSMONITOR_REASON_UNTESTED = 0,
     - 	FSMONITOR_REASON_OK, /* no incompatibility or when disbled */
     + 	FSMONITOR_REASON_OK, /* no incompatibility or when disabled */
       	FSMONITOR_REASON_BARE,
      +	FSMONITOR_REASON_VFS4GIT, /* VFS for Git virtualization */
       };
  7:  c50ed29a310 =  7:  f7ef7dcffc8 fsmonitor-settings: stub in macOS-specific incompatibility checking
  8:  1f5b772d42a !  8:  dc2dfd67931 fsmonitor-settings: remote repos on macOS are incompatible
     @@ fsmonitor-settings.c: char *fsm_settings__get_incompatible_msg(const struct repo
       ## fsmonitor-settings.h ##
      @@ fsmonitor-settings.h: enum fsmonitor_reason {
       	FSMONITOR_REASON_UNTESTED = 0,
     - 	FSMONITOR_REASON_OK, /* no incompatibility or when disbled */
     + 	FSMONITOR_REASON_OK, /* no incompatibility or when disabled */
       	FSMONITOR_REASON_BARE,
      +	FSMONITOR_REASON_ERROR, /* FS error probing for compatibility */
      +	FSMONITOR_REASON_REMOTE,
  9:  495e54049b4 =  9:  5627038aaa3 fsmonitor-settings: remote repos on Windows are incompatible
 10:  4b52083698c = 10:  9a12cc78b5d fsmonitor-settings: NTFS and FAT32 on MacOS are incompatible
 11:  d4a4263d379 = 11:  aaff000cecb unpack-trees: initialize fsmonitor_has_run_once in o->result
 12:  f4feb00ec2b = 12:  4f2b15d3d1f fsm-listen-darwin: ignore FSEvents caused by xattr changes on macOS
 13:  dbb983fd9d0 = 13:  427dec412a5 fsmonitor--daemon: cd out of worktree root
 14:  ae90b99ea9b = 14:  51b266b06e1 fsmonitor--daemon: prepare for adding health thread
 15:  b6c5800095f = 15:  594e0ae243d fsmonitor--daemon: rename listener thread related variables
 16:  32fc6ba7437 = 16:  c2b5c02ed38 fsmonitor--daemon: stub in health thread
 17:  77bc037481a = 17:  46a5ae2a635 fsm-health-win32: add polling framework to monitor daemon health
 18:  b06edd995ea = 18:  7cf1be5f8e2 fsm-health-win32: force shutdown daemon if worktree root moves
 19:  1bd5f346248 = 19:  95cf1299d44 fsm-listen-darwin: shutdown daemon if worktree root is moved/renamed
 20:  48af0813dec = 20:  b020bfb4568 fsmonitor: optimize processing of directory events
 21:  a9b35e770f3 = 21:  d058d7e0c08 t7527: FSMonitor tests for directory moves
 22:  26308936af9 = 22:  f5dac286812 t/perf/p7527: add perf test for builtin FSMonitor
 23:  d0e25f6bac6 ! 23:  92f5c0d2c8b fsmonitor: never set CE_FSMONITOR_VALID on submodules
     @@ t/t7527-builtin-fsmonitor.sh: do
      +	git -C super/dir_1/dir_2/sub clean -d -f
      +}
      +
     -+test_expect_success "Submodule always visited" '
     ++test_expect_success 'submodule always visited' '
      +	test_when_finished "git -C super fsmonitor--daemon stop; \
      +			    rm -rf super; \
      +			    rm -rf sub" &&
 24:  410dd2d2920 ! 24:  40b80adbb31 t7527: test FSMonitor on case insensitive+preserving file system
     @@ Commit message
          Signed-off-by: Jeff Hostetler <jeffhost@microsoft.com>
      
       ## t/t7527-builtin-fsmonitor.sh ##
     -@@ t/t7527-builtin-fsmonitor.sh: test_expect_success "Submodule always visited" '
     +@@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'submodule always visited' '
       	my_match_and_clean
       '
       
 25:  cd7c55b0d38 = 25:  b93f0642699 fsmonitor: on macOS also emit NFC spelling for NFD pathname
  -:  ----------- > 26:  6f2e935f148 t/helper/hexdump: add helper to print hexdump of stdin
 26:  8278f32c4d8 ! 27:  6a830869954 t/lib-unicode-nfc-nfd: helper prereqs for testing unicode nfc/nfd
     @@ t/lib-unicode-nfc-nfd.sh (new)
      +# and on APFS, NFC paths are preserved.  As we have established
      +# above, this is independent of "composition sensitivity".
      +#
     -+# 0000000 63 5f c3 a9
     -+#
     -+# (/usr/bin/od output contains different amount of whitespace
     -+# on different platforms, so we need the wildcards here.)
     -+#
      +test_lazy_prereq UNICODE_NFC_PRESERVED '
      +	mkdir c_${utf8_nfc} &&
     -+	ls | od -t x1 | grep "63 *5f *c3 *a9"
     ++	ls | test-tool hexdump | grep "63 5f c3 a9"
      +'
      +
      +# Is the spelling of an NFD pathname preserved on disk?
      +#
     -+# 0000000 64 5f 65 cc 81
     -+#
      +test_lazy_prereq UNICODE_NFD_PRESERVED '
      +	mkdir d_${utf8_nfd} &&
     -+	ls | od -t x1 | grep "64 *5f *65 *cc *81"
     ++	ls | test-tool hexdump | grep "64 5f 65 cc 81"
      +'
     -+	mkdir c_${utf8_nfc} &&
     -+	mkdir d_${utf8_nfd} &&
      +
      +# The following _DOUBLE_ forms are more for my curiosity,
      +# but there may be quirks lurking when there are multiple
     @@ t/lib-unicode-nfc-nfd.sh (new)
      +#
      +test_lazy_prereq UNICODE_DOUBLE_NFC_PRESERVED '
      +	mkdir c_${greek_nfc} &&
     -+	ls | od -t x1 | grep "63 *5f *e1 *bd *a7"
     ++	ls | test-tool hexdump | grep "63 5f e1 bd a7"
      +'
      +
      +# See if the NFD spelling appears on the disk.
      +#
      +test_lazy_prereq UNICODE_DOUBLE_NFD_PRESERVED '
      +	mkdir d_${greek_nfd2} &&
     -+	ls | od -t x1 | grep "64 *5f *cf *89 *cc *94 *cd *82"
     ++	ls | test-tool hexdump | grep "64 5f cf 89 cc 94 cd 82"
      +'
      +
      +# The following is for debugging. I found it useful when
 27:  4efb3a43838 = 28:  f9a7869d202 t7527: test Unicode NFC/NFD handling on MacOS
 28:  df1b4f3a80f ! 29:  9fc7c970929 fsmonitor--daemon: allow --super-prefix argument
     @@ git.c: static struct cmd_struct commands[] = {
       	{ "grep", cmd_grep, RUN_SETUP_GENTLY },
      
       ## t/t7527-builtin-fsmonitor.sh ##
     -@@ t/t7527-builtin-fsmonitor.sh: test_expect_success "Submodule always visited" '
     +@@ t/t7527-builtin-fsmonitor.sh: test_expect_success 'submodule always visited' '
       	my_match_and_clean
       '
       
     @@ t/t7527-builtin-fsmonitor.sh: test_expect_success "Submodule always visited" '
      +	tr '\047' Q <$1 | grep -e "$msg"
      +}
      +
     -+test_expect_success "Stray Submodule super-prefix warning" '
     ++test_expect_success "stray submodule super-prefix warning" '
      +	test_when_finished "rm -rf super; \
      +			    rm -rf sub;   \
      +			    rm super-sub.trace" &&
  -:  ----------- > 30:  ca833ecc7a1 t7527: improve implicit shutdown testing in fsmonitor--daemon

Comments

Johannes Schindelin May 24, 2022, noon UTC | #1
Hi Jeff,

On Mon, 23 May 2022, Jeff Hostetler via GitGitGadget wrote:

>  1:  8b7c5f4e23 !  1:  26144c5865 fsm-listen-win32: handle shortnames
>       [...]
>     @@ compat/fsmonitor/fsm-listen-win32.c: static int normalize_path_in_utf8(FILE_NOTI
>      +{
>      +  wchar_t buf_in[MAX_PATH + 1];
>      +  wchar_t buf_out[MAX_PATH + 1];
>     -+  wchar_t *last_slash = NULL;
>     -+  wchar_t *last_bslash = NULL;
>      +  wchar_t *last;
>     ++  wchar_t *p;
>      +
>      +  /* build L"<wt-root-path>/.git" */
>     -+  wcscpy(buf_in, watch->wpath_longname);
>     -+  wcscpy(buf_in + watch->wpath_longname_len, L".git");
>     ++  swprintf(buf_in, ARRAY_SIZE(buf_in) - 1, L"%s.git",
>     ++           watch->wpath_longname);
>      +
>     -+  if (!GetShortPathNameW(buf_in, buf_out, MAX_PATH))
>     ++  if (!GetShortPathNameW(buf_in, buf_out, ARRAY_SIZE(buf_out)))

Nice touch using `ARRAY_SIZE()` here!

The changes look good to me, from my side this is good to go.

Thank you so much!
Dscho
Jeff Hostetler May 24, 2022, 3:07 p.m. UTC | #2
On 5/24/22 8:00 AM, Johannes Schindelin wrote:
> Hi Jeff,
> 
> On Mon, 23 May 2022, Jeff Hostetler via GitGitGadget wrote:
> 
>>   1:  8b7c5f4e23 !  1:  26144c5865 fsm-listen-win32: handle shortnames
>>        [...]
>>      @@ compat/fsmonitor/fsm-listen-win32.c: static int normalize_path_in_utf8(FILE_NOTI
>>       +{
>>       +  wchar_t buf_in[MAX_PATH + 1];
>>       +  wchar_t buf_out[MAX_PATH + 1];
>>      -+  wchar_t *last_slash = NULL;
>>      -+  wchar_t *last_bslash = NULL;
>>       +  wchar_t *last;
>>      ++  wchar_t *p;
>>       +
>>       +  /* build L"<wt-root-path>/.git" */
>>      -+  wcscpy(buf_in, watch->wpath_longname);
>>      -+  wcscpy(buf_in + watch->wpath_longname_len, L".git");
>>      ++  swprintf(buf_in, ARRAY_SIZE(buf_in) - 1, L"%s.git",
>>      ++           watch->wpath_longname);
>>       +
>>      -+  if (!GetShortPathNameW(buf_in, buf_out, MAX_PATH))
>>      ++  if (!GetShortPathNameW(buf_in, buf_out, ARRAY_SIZE(buf_out)))
> 
> Nice touch using `ARRAY_SIZE()` here!

Thanks.  And hopefully it will make the GFW downstream MAX_LONG_PATH
fixups easier too.

> 
> The changes look good to me, from my side this is good to go.
> 
> Thank you so much!
> Dscho
> 

Thanks for all your reviews.
Jeff
Johannes Schindelin May 25, 2022, 10:23 a.m. UTC | #3
Hi Jeff,

On Tue, 24 May 2022, Jeff Hostetler wrote:

> On 5/24/22 8:00 AM, Johannes Schindelin wrote:
> >
> > On Mon, 23 May 2022, Jeff Hostetler via GitGitGadget wrote:
> >
> > >   1:  8b7c5f4e23 !  1:  26144c5865 fsm-listen-win32: handle shortnames
> > >        [...]
> > >      @@ compat/fsmonitor/fsm-listen-win32.c: static int
> > >      normalize_path_in_utf8(FILE_NOTI
> > >       +{
> > >       +  wchar_t buf_in[MAX_PATH + 1];
> > >       +  wchar_t buf_out[MAX_PATH + 1];
> > >      -+  wchar_t *last_slash = NULL;
> > >      -+  wchar_t *last_bslash = NULL;
> > >      +  wchar_t *last;
> > >      ++  wchar_t *p;
> > >       +
> > >       +  /* build L"<wt-root-path>/.git" */
> > >      -+  wcscpy(buf_in, watch->wpath_longname);
> > >      -+  wcscpy(buf_in + watch->wpath_longname_len, L".git");
> > >      ++  swprintf(buf_in, ARRAY_SIZE(buf_in) - 1, L"%s.git",
> > >      ++           watch->wpath_longname);
> > >      +
> > >      -+  if (!GetShortPathNameW(buf_in, buf_out, MAX_PATH))
> > >      ++  if (!GetShortPathNameW(buf_in, buf_out, ARRAY_SIZE(buf_out)))
> >
> > Nice touch using `ARRAY_SIZE()` here!
>
> Thanks.  And hopefully it will make the GFW downstream MAX_LONG_PATH
> fixups easier too.

As a matter of fact, it already did!

Thank you,
Dscho