Fast git status via a file system watcher

fsmonitor.c new file +14 -0

+       index += sizeof(uint32_t);
+
+       istate->fsmonitor_dirty = ewah_new();
+       ret = ewah_read_mmap(istate->fsmonitor_dirty, index, ewah_size);
+       if (ret != ewah_size) {
+               ewah_free(istate->fsmonitor_dirty);
+               istate->fsmonitor_dirty = NULL;
+               return error("failed to parse ewah bitmap reading fsmonitor index extension");
+       }
+
+       return 0;
+}
+
+void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)

fsmonitor.c new file +4 -0

+       strbuf_add(sb, &tm, sizeof(uint64_t));
+       fixup = sb->len;
+       strbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */
+

fsmonitor.c new file +11 -0

+       memcpy(sb->buf + fixup, &ewah_size, sizeof(uint32_t));
+}
 
+/*
+ * Call the query-fsmonitor hook passing the time of the last saved results.
+ */
+static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
+{
+       struct child_process cp = CHILD_PROCESS_INIT;
+       char ver[64];
+       char date[64];
+       const char *argv[4];

fsmonitor.c new file +23 -0

+       cp.out = -1;
+
+       return capture_command(&cp, query_result, 1024);
+}
+
+static void mark_file_dirty(struct index_state *istate, const char *name)
+{
+       struct untracked_cache_dir *dir;
+       int pos;
+
+       /* find it in the index and mark that entry as dirty */
+       pos = index_name_pos(istate, name, strlen(name));
+       if (pos >= 0) {
+               if (!(istate->cache[pos]->ce_flags & CE_FSMONITOR_DIRTY)) {
+                       istate->cache[pos]->ce_flags |= CE_FSMONITOR_DIRTY;
+                       istate->cache_changed |= FSMONITOR_CHANGED;
+               }
+       }
+
+       /*
+        * Find the corresponding directory in the untracked cache
+        * and mark it as invalid
+        */

fsmonitor.c new file +29 -0

+}
+
+void refresh_by_fsmonitor(struct index_state *istate)
+{
+       static int has_run_once = 0;
+       struct strbuf query_result = STRBUF_INIT;
+       int query_success = 0;
+       size_t bol = 0; /* beginning of line */
+       uint64_t last_update;
+       char *buf, *entry;
+       int i;
+
+       if (!core_fsmonitor || has_run_once)
+               return;
+       has_run_once = 1;
+
+       /*
+        * This could be racy so save the date/time now and the hook
+        * should be inclusive to ensure we don't miss potential changes.
+        */
+       last_update = getnanotime();
+
+       /*
+        * If we have a last update time, call query-monitor for the set of
+        * changes since that time.
+        */
+       if (istate->fsmonitor_last_update) {
+               query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,
+                       istate->fsmonitor_last_update, &query_result);

fsmonitor.c new file +7 -0

+               /* Mark all clean entries up-to-date */
+               for (i = 0; i < istate->cache_nr; i++) {
+                       struct cache_entry *ce = istate->cache[i];
+                       if (ce_stage(ce) || (ce->ce_flags & CE_FSMONITOR_DIRTY))
+                               continue;
+                       ce_mark_uptodate(ce);
+               }

fsmonitor.c line 22

Quoted 12 lines · Ben Peart

fsmonitor.c new file +12 -0

+int read_fsmonitor_extension(struct index_state *istate, const void *data,
+       unsigned long sz)
+{
+       const char *index = data;
+       uint32_t hdr_version;
+       uint32_t ewah_size;
+       int ret;
+
+       if (sz < sizeof(uint32_t) + sizeof(uint64_t) + sizeof(uint32_t))
+               return error("corrupt fsmonitor extension (too short)");
+
+       hdr_version = get_be32(index);

Here we use get_be32(), ...

fsmonitor.c line 27

Quoted 5 lines

fsmonitor.c new file +5 -0

+       index += sizeof(uint32_t);
+       if (hdr_version != INDEX_EXTENSION_VERSION)
+               return error("bad fsmonitor version %d", hdr_version);
+
+       istate->fsmonitor_last_update = get_be64(index);

...get_be64(), ...

fsmonitor.c line 30

Quoted 3 lines

fsmonitor.c new file +3 -0

+       index += sizeof(uint64_t);
+
+       ewah_size = get_be32(index);

... and get_be32 again, ...

Quoted 14 lines

fsmonitor.c new file +14 -0

+       index += sizeof(uint32_t);
+
+       istate->fsmonitor_dirty = ewah_new();
+       ret = ewah_read_mmap(istate->fsmonitor_dirty, index, ewah_size);
+       if (ret != ewah_size) {
+               ewah_free(istate->fsmonitor_dirty);
+               istate->fsmonitor_dirty = NULL;
+               return error("failed to parse ewah bitmap reading fsmonitor index extension");
+       }
+
+       return 0;
+}
+
+void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)

fsmonitor.c line 54

Quoted 10 lines

fsmonitor.c new file +10 -0

+{
+       uint32_t hdr_version;
+       uint64_t tm;
+       struct ewah_bitmap *bitmap;
+       int i;
+       uint32_t ewah_start;
+       uint32_t ewah_size = 0;
+       int fixup = 0;
+
+       hdr_version = htonl(INDEX_EXTENSION_VERSION);

... but here we use htonl() instead of put_be32(), ...

fsmonitor.c line 57

Quoted 3 lines

fsmonitor.c new file +3 -0

+       strbuf_add(sb, &hdr_version, sizeof(uint32_t));
+
+       tm = htonll((uint64_t)istate->fsmonitor_last_update);

... htonll(), ...

Quoted 4 lines

fsmonitor.c new file +4 -0

+       strbuf_add(sb, &tm, sizeof(uint64_t));
+       fixup = sb->len;
+       strbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */
+

fsmonitor.c line 71

Quoted 10 lines

fsmonitor.c new file +10 -0

+       ewah_start = sb->len;
+       bitmap = ewah_new();
+       for (i = 0; i < istate->cache_nr; i++)
+               if (istate->cache[i]->ce_flags & CE_FSMONITOR_DIRTY)
+                       ewah_set(bitmap, i);
+       ewah_serialize_strbuf(bitmap, sb);
+       ewah_free(bitmap);
+
+       /* fix up size field */
+       ewah_size = htonl(sb->len - ewah_start);

... and htonl() again.

It would be more consistent (and perhaps more correct) to use put_beXX() functions, instead of the htonl() and htonll() functions.

Quoted 17 lines

fsmonitor.c new file +15 -0

+       memcpy(sb->buf + fixup, &ewah_size, sizeof(uint32_t));
+}
 
+/*
+ * Call the query-fsmonitor hook passing the time of the last saved results.
+ */
+static int query_fsmonitor(int version, uint64_t last_update, struct strbuf *query_result)
+{
+       struct child_process cp = CHILD_PROCESS_INIT;
+       char ver[64];
+       char date[64];
+       const char *argv[4];
+
+       if (!(argv[0] = find_hook("query-fsmonitor")))
+               return -1;
+

fsmonitor.c line 155

Quoted 6 lines

fsmonitor.c new file +6 -0

+       snprintf(ver, sizeof(version), "%d", version);
+       snprintf(date, sizeof(date), "%" PRIuMAX, (uintmax_t)last_update);
+       argv[1] = ver;
+       argv[2] = date;
+       argv[3] = NULL;
+       cp.argv = argv;

Maybe it would be nicer using the argv_array_pushX() functions.

Quoted 29 lines

fsmonitor.c new file +28 -0

+       cp.out = -1;
+
+       return capture_command(&cp, query_result, 1024);
+}
+
+static void mark_file_dirty(struct index_state *istate, const char *name)
+{
+       struct untracked_cache_dir *dir;
+       int pos;
+
+       /* find it in the index and mark that entry as dirty */
+       pos = index_name_pos(istate, name, strlen(name));
+       if (pos >= 0) {
+               if (!(istate->cache[pos]->ce_flags & CE_FSMONITOR_DIRTY)) {
+                       istate->cache[pos]->ce_flags |= CE_FSMONITOR_DIRTY;
+                       istate->cache_changed |= FSMONITOR_CHANGED;
+               }
+       }
+
+       /*
+        * Find the corresponding directory in the untracked cache
+        * and mark it as invalid
+        */
+       if (!istate->untracked || !istate->untracked->root)
+               return;
+
+       dir = find_untracked_cache_dir(istate->untracked, istate->untracked->root, name);
+       if (dir) {

fsmonitor.c line 188

Quoted 5 lines

fsmonitor.c new file +5 -0

+               if (dir->valid) {
+                       dir->valid = 0;
+                       istate->cache_changed |= FSMONITOR_CHANGED;
+               }
+       }

The code above is quite similar as what is in mark_fsmonitor_dirty(), so I wonder if a refactoring is possible.

Quoted 30 lines

fsmonitor.c new file +29 -0

+}
+
+void refresh_by_fsmonitor(struct index_state *istate)
+{
+       static int has_run_once = 0;
+       struct strbuf query_result = STRBUF_INIT;
+       int query_success = 0;
+       size_t bol = 0; /* beginning of line */
+       uint64_t last_update;
+       char *buf, *entry;
+       int i;
+
+       if (!core_fsmonitor || has_run_once)
+               return;
+       has_run_once = 1;
+
+       /*
+        * This could be racy so save the date/time now and the hook
+        * should be inclusive to ensure we don't miss potential changes.
+        */
+       last_update = getnanotime();
+
+       /*
+        * If we have a last update time, call query-monitor for the set of
+        * changes since that time.
+        */
+       if (istate->fsmonitor_last_update) {
+               query_success = !query_fsmonitor(HOOK_INTERFACE_VERSION,
+                       istate->fsmonitor_last_update, &query_result);

fsmonitor.c line 227

Quoted 10 lines

fsmonitor.c new file +10 -0

+               trace_performance_since(last_update, "query-fsmonitor");
+       }
+
+       if (query_success) {
+               /* Mark all entries returned by the monitor as dirty */
+               buf = entry = query_result.buf;
+               for (i = 0; i < query_result.len; i++) {
+                       if (buf[i] != '\0')
+                               continue;
+                       mark_file_dirty(istate, buf + bol);

It looks like bol is always equal to i here...

fsmonitor.c line 231

Quoted 4 lines

fsmonitor.c new file +4 -0

+                       bol = i + 1;
+               }
+               if (bol < query_result.len)
+                       mark_file_dirty(istate, buf + bol);

... and here too. As it is not used below, I wonder if you really need the bol variable.

Quoted 8 lines

fsmonitor.c new file +7 -0

+               /* Mark all clean entries up-to-date */
+               for (i = 0; i < istate->cache_nr; i++) {
+                       struct cache_entry *ce = istate->cache[i];
+                       if (ce_stage(ce) || (ce->ce_flags & CE_FSMONITOR_DIRTY))
+                               continue;
+                       ce_mark_uptodate(ce);
+               }

t/t7519-status-fsmonitor.sh new file +4 -0

+# fsmonitor works correctly with or without the untracked cache
+# but if it is available, we'll turn it on to ensure we test that
+# codepath as well.
+

Quoted 4 lines · Ben Peart

t/t7519-status-fsmonitor.sh new file +4 -0

+# fsmonitor works correctly with or without the untracked cache
+# but if it is available, we'll turn it on to ensure we test that
+# codepath as well.
+

t/t7519-status-fsmonitor.sh line 44

Quoted 10 lines

t/t7519-status-fsmonitor.sh new file +10 -0

+test_lazy_prereq UNTRACKED_CACHE '
+       { git update-index --test-untracked-cache; ret=$?; } &&
+       test $ret -ne 1
+'
+
+if test_have_prereq UNTRACKED_CACHE; then
+       git config core.untrackedcache true
+else
+       git config core.untrackedcache false
+fi

I wonder if it would be better to just do something like:

=====================

test_expect_success 'setup' '

         ....

'

uc_values="false"
test_have_prereq UNTRACKED_CACHE && uc_values="false true"

for uc_val in $uc_values
do

     test_expect_success "setup untracked cache to $uc_val" '
          git config core.untrackedcache $uc_val
     '

     test_expect_success 'refresh_index() invalidates fsmonitor cache' '
           ...
     '

     test_expect_success "status doesn't detect unreported modifications" '
           ...
     '

...

done

=====================

Add a test utility (test-drop-caches) that enables dropping the file system cache on Windows.

Add a perf test (p7519-fsmonitor.sh) for fsmonitor.

Signed-off-by Ben Peart <[email protected]>

Signed-off-by Ævar Arnfjörð Bjarmason <[email protected]>

---
  Makefile                    |   1 +
  t/helper/test-drop-caches.c | 107 +++++++++++++++++++++++++++++
  t/perf/p7519-fsmonitor.sh   | 161 ++++++++++++++++++++++++++++++++++++++++++++
  3 files changed, 269 insertions(+)

create mode 100644 t/helper/test-drop-caches.c
create mode 100755 t/perf/p7519-fsmonitor.sh

Makefile +1 -0

index 992dd58801..893947839f 100644
@@ -648,6 +648,7 @@ TEST_PROGRAMS_NEED_X += test-subprocess
  TEST_PROGRAMS_NEED_X += test-svn-fe
  TEST_PROGRAMS_NEED_X += test-urlmatch-normalization
  TEST_PROGRAMS_NEED_X += test-wildmatch
+TEST_PROGRAMS_NEED_X += test-drop-caches
 
  TEST_PROGRAMS = $(patsubst %,t/helper/%$X,$(TEST_PROGRAMS_NEED_X))
 

t/helper/test-drop-caches.c new file +107 -0

new file mode 100644
index 0000000000..80830d920b
@@ -0,0 +1,107 @@
+#include "git-compat-util.h"
+#include <stdio.h>
+
+typedef DWORD NTSTATUS;
+
+#ifdef GIT_WINDOWS_NATIVE
+#include <tchar.h>
+
+#define STATUS_SUCCESS			(0x00000000L)
+#define STATUS_PRIVILEGE_NOT_HELD	(0xC0000061L)
+
+typedef enum _SYSTEM_INFORMATION_CLASS {
+	SystemMemoryListInformation = 80, // 80, q: SYSTEM_MEMORY_LIST_INFORMATION; s: SYSTEM_MEMORY_LIST_COMMAND (requires SeProfileSingleProcessPrivilege)
+} SYSTEM_INFORMATION_CLASS;
+
+// private
+typedef enum _SYSTEM_MEMORY_LIST_COMMAND
+{
+	MemoryCaptureAccessedBits,
+	MemoryCaptureAndResetAccessedBits,
+	MemoryEmptyWorkingSets,
+	MemoryFlushModifiedList,
+	MemoryPurgeStandbyList,
+	MemoryPurgeLowPriorityStandbyList,
+	MemoryCommandMax
+} SYSTEM_MEMORY_LIST_COMMAND;
+
+BOOL GetPrivilege(HANDLE TokenHandle, LPCSTR lpName, int flags)
+{
+	BOOL bResult;
+	DWORD dwBufferLength;
+	LUID luid;
+	TOKEN_PRIVILEGES tpPreviousState;
+	TOKEN_PRIVILEGES tpNewState;
+
+	dwBufferLength = 16;
+	bResult = LookupPrivilegeValueA(0, lpName, &luid);
+	if (bResult)
+	{
+		tpNewState.PrivilegeCount = 1;
+		tpNewState.Privileges[0].Luid = luid;
+		tpNewState.Privileges[0].Attributes = 0;
+		bResult = AdjustTokenPrivileges(TokenHandle, 0, &tpNewState, (DWORD)((LPBYTE)&(tpNewState.Privileges[1]) - (LPBYTE)&tpNewState), &tpPreviousState, &dwBufferLength);
+		if (bResult)
+		{
+			tpPreviousState.PrivilegeCount = 1;
+			tpPreviousState.Privileges[0].Luid = luid;
+			tpPreviousState.Privileges[0].Attributes = flags != 0 ? 2 : 0;
+			bResult = AdjustTokenPrivileges(TokenHandle, 0, &tpPreviousState, dwBufferLength, 0, 0);
+		}
+	}
+	return bResult;
+}
+#endif
+
+int cmd_main(int argc, const char **argv)
+{
+	NTSTATUS status = 1;
+#ifdef GIT_WINDOWS_NATIVE
+	HANDLE hProcess = GetCurrentProcess();
+	HANDLE hToken;
+	if (!OpenProcessToken(hProcess, TOKEN_QUERY | TOKEN_ADJUST_PRIVILEGES, &hToken))
+	{
+		_ftprintf(stderr, _T("Can't open current process token\n"));
+		return 1;
+	}
+
+	if (!GetPrivilege(hToken, "SeProfileSingleProcessPrivilege", 1))
+	{
+		_ftprintf(stderr, _T("Can't get SeProfileSingleProcessPrivilege\n"));
+		return 1;
+	}
+
+	CloseHandle(hToken);
+
+	HMODULE ntdll = LoadLibrary(_T("ntdll.dll"));
+	if (!ntdll)
+	{
+		_ftprintf(stderr, _T("Can't load ntdll.dll, wrong Windows version?\n"));
+		return 1;
+	}
+
+	NTSTATUS(WINAPI *NtSetSystemInformation)(INT, PVOID, ULONG) = (NTSTATUS(WINAPI *)(INT, PVOID, ULONG))GetProcAddress(ntdll, "NtSetSystemInformation");
+	if (!NtSetSystemInformation)
+	{
+		_ftprintf(stderr, _T("Can't get function addresses, wrong Windows version?\n"));
+		return 1;
+	}
+
+	SYSTEM_MEMORY_LIST_COMMAND command = MemoryPurgeStandbyList;
+	status = NtSetSystemInformation(
+		SystemMemoryListInformation,
+		&command,
+		sizeof(SYSTEM_MEMORY_LIST_COMMAND)
+	);
+	if (status == STATUS_PRIVILEGE_NOT_HELD)
+	{
+		_ftprintf(stderr, _T("Insufficient privileges to execute the memory list command"));
+	}
+	else if (status != STATUS_SUCCESS)
+	{
+		_ftprintf(stderr, _T("Unable to execute the memory list command %lX"), status);
+	}
+#endif
+
+	return status;
+}

t/perf/p7519-fsmonitor.sh new file +161 -0

new file mode 100755
index 0000000000..e41905cb9b
@@ -0,0 +1,161 @@
+#!/bin/sh
+
+test_description="Test core.fsmonitor"
+
+. ./perf-lib.sh
+
+# This has to be run with GIT_PERF_REPEAT_COUNT=1 to generate valid results.
+# Otherwise the caching that happens for the nth run will negate the validity
+# of the comparisons.
+if [ "$GIT_PERF_REPEAT_COUNT" -ne 1 ]
+then
+	echo "warning: This test must be run with GIT_PERF_REPEAT_COUNT=1 to generate valid results." >&2
+	echo "warning: Setting GIT_PERF_REPEAT_COUNT=1" >&2
+	GIT_PERF_REPEAT_COUNT=1
+fi
+
+test_perf_large_repo
+test_checkout_worktree
+
+# Convert unix style paths to what Watchman expects
+case "$(uname -s)" in
+MINGW*|MSYS_NT*)
+  GIT_WORK_TREE="$(cygpath -aw "$PWD" | sed 's,\\,/,g')"
+  ;;
+*)
+  GIT_WORK_TREE="$PWD"
+  ;;
+esac
+
+# The big win for using fsmonitor is the elimination of the need to scan
+# the working directory looking for changed files and untracked files. If
+# the file information is all cached in RAM, the benefits are reduced.
+
+flush_disk_cache () {
+	case "$(uname -s)" in
+	MINGW*|MSYS_NT*)
+	  sync && test-drop-caches
+	  ;;
+	*)
+	  sudo sync && echo 3 | sudo tee /proc/sys/vm/drop_caches
+	  ;;
+	esac
+
+}
+
+test_lazy_prereq UNTRACKED_CACHE '
+	{ git update-index --test-untracked-cache; ret=$?; } &&
+	test $ret -ne 1
+'
+
+test_expect_success "setup" '
+	# Maybe set untrackedCache & splitIndex depending on the environment
+	if test -n "$GIT_PERF_7519_UNTRACKED_CACHE"
+	then
+		git config core.untrackedCache "$GIT_PERF_7519_UNTRACKED_CACHE"
+	else
+		if test_have_prereq UNTRACKED_CACHE
+		then
+			git config core.untrackedCache true
+		else
+			git config core.untrackedCache false
+		fi
+	fi &&
+
+	if test -n "$GIT_PERF_7519_SPLIT_INDEX"
+	then
+		git config core.splitIndex "$GIT_PERF_7519_SPLIT_INDEX"
+	fi &&
+
+	# Hook scaffolding
+	mkdir .git/hooks &&
+	cp ../../../templates/hooks--query-fsmonitor.sample .git/hooks/query-fsmonitor &&
+
+	# have Watchman monitor the test folder
+	watchman watch "$GIT_WORK_TREE" &&
+	watchman watch-list | grep -q -F "$GIT_WORK_TREE"
+'
+
+# Worst case without fsmonitor
+test_expect_success "clear fs cache" '
+	git config core.fsmonitor false &&
+	flush_disk_cache
+'
+test_perf "status (fsmonitor=false, cold fs cache)" '
+	git status
+'
+
+# Best case without fsmonitor
+test_perf "status (fsmonitor=false, warm fs cache)" '
+	git status
+'
+
+# Let's see if -uno & -uall make any difference
+test_expect_success "clear fs cache" '
+	flush_disk_cache
+'
+test_perf "status -uno (fsmonitor=false, cold fs cache)" '
+	git status -uno
+'
+
+test_expect_success "clear fs cache" '
+	flush_disk_cache
+'
+test_perf "status -uall (fsmonitor=false, cold fs cache)" '
+	git status -uall
+'
+
+# The first run with core.fsmonitor=true has to do a normal scan and write
+# out the index extension.
+test_expect_success "populate extension" '
+	# core.preloadIndex defeats the benefits of core.fsMonitor as it
+	# calls lstat for the index entries. Turn it off as _not_ doing
+	# the work is faster than doing the work across multiple threads.
+	git config core.fsmonitor true &&
+	git config core.preloadIndex false &&
+	git status
+'
+
+# Worst case with fsmonitor
+test_expect_success "shutdown fsmonitor, clear fs cache" '
+	watchman shutdown-server &&
+	flush_disk_cache
+'
+test_perf "status (fsmonitor=true, cold fs cache, cold fsmonitor)" '
+	git status
+'
+
+# Best case with fsmonitor
+test_perf "status (fsmonitor=true, warm fs cache, warm fsmonitor)" '
+	git status
+'
+
+# Best improved with fsmonitor (compare to worst case without fsmonitor)
+test_expect_success "clear fs cache" '
+	flush_disk_cache
+'
+test_perf "status (fsmonitor=true, cold fs cache, warm fsmonitor)" '
+	git status
+'
+
+# Let's see if -uno & -uall make any difference
+test_expect_success "clear fs cache" '
+	flush_disk_cache
+'
+test_perf "status -uno (fsmonitor=true, cold fs cache)" '
+	git status -uno
+'
+
+test_expect_success "clear fs cache" '
+	flush_disk_cache
+'
+test_perf "status -uall (fsmonitor=true, cold fs cache)" '
+	git status -uall
+'
+
+test_expect_success "cleanup" '
+	watchman watch-del "$GIT_WORK_TREE" &&
+	watchman shutdown-server
+'
+
+test_done

t/helper/test-drop-caches.c new file +4 -0

+} SYSTEM_MEMORY_LIST_COMMAND;
+
+BOOL GetPrivilege(HANDLE TokenHandle, LPCSTR lpName, int flags)
+{

t/helper/test-drop-caches.c new file +34 -0

+		tpNewState.PrivilegeCount = 1;
+		tpNewState.Privileges[0].Luid = luid;
+		tpNewState.Privileges[0].Attributes = 0;
+		bResult = AdjustTokenPrivileges(TokenHandle, 0, &tpNewState, (DWORD)((LPBYTE)&(tpNewState.Privileges[1]) - (LPBYTE)&tpNewState), &tpPreviousState, &dwBufferLength);
+		if (bResult)
+		{
+			tpPreviousState.PrivilegeCount = 1;
+			tpPreviousState.Privileges[0].Luid = luid;
+			tpPreviousState.Privileges[0].Attributes = flags != 0 ? 2 : 0;
+			bResult = AdjustTokenPrivileges(TokenHandle, 0, &tpPreviousState, dwBufferLength, 0, 0);
+		}
+	}
+	return bResult;
+}
+#endif
+
+int cmd_main(int argc, const char **argv)
+{
+	NTSTATUS status = 1;
+#ifdef GIT_WINDOWS_NATIVE
+	HANDLE hProcess = GetCurrentProcess();
+	HANDLE hToken;
+	if (!OpenProcessToken(hProcess, TOKEN_QUERY | TOKEN_ADJUST_PRIVILEGES, &hToken))
+	{
+		_ftprintf(stderr, _T("Can't open current process token\n"));
+		return 1;
+	}
+
+	if (!GetPrivilege(hToken, "SeProfileSingleProcessPrivilege", 1))
+	{
+		_ftprintf(stderr, _T("Can't get SeProfileSingleProcessPrivilege\n"));
+		return 1;
+	}
+

t/helper/test-drop-caches.c new file +14 -0

+	if (!NtSetSystemInformation)
+	{
+		_ftprintf(stderr, _T("Can't get function addresses, wrong Windows version?\n"));
+		return 1;
+	}
+
+	SYSTEM_MEMORY_LIST_COMMAND command = MemoryPurgeStandbyList;
+	status = NtSetSystemInformation(
+		SystemMemoryListInformation,
+		&command,
+		sizeof(SYSTEM_MEMORY_LIST_COMMAND)
+	);
+	if (status == STATUS_PRIVILEGE_NOT_HELD)
+	{

t/perf/p7519-fsmonitor.sh new file +1 -0

new file mode 100755
index 0000000000..e41905cb9b
@@ -0,0 +1,161 @@
+#!/bin/sh

t/helper/test-drop-caches.c line 2

Quoted 8 lines · Ben Peart

t/helper/test-drop-caches.c new file +2 -0

new file mode 100644
index 0000000000..80830d920b
@@ -0,0 +1,107 @@
+#include "git-compat-util.h"
+#include <stdio.h>

I thought compat-util should already include stdio?

t/helper/test-drop-caches.c line 4

Quoted 2 lines

t/helper/test-drop-caches.c new file +2 -0

+
+typedef DWORD NTSTATUS;

Is this safe to have outside "#ifdef GIT_WINDOWS_NATIVE"?

t/helper/test-drop-caches.c line 13

Quoted 10 lines

t/helper/test-drop-caches.c new file +9 -0

+
+#ifdef GIT_WINDOWS_NATIVE
+#include <tchar.h>
+
+#define STATUS_SUCCESS			(0x00000000L)
+#define STATUS_PRIVILEGE_NOT_HELD	(0xC0000061L)
+
+typedef enum _SYSTEM_INFORMATION_CLASS {
+	SystemMemoryListInformation = 80, // 80, q: SYSTEM_MEMORY_LIST_INFORMATION; s: SYSTEM_MEMORY_LIST_COMMAND (requires SeProfileSingleProcessPrivilege)

I would have said "Please avoid // comment in this codebase unless we know we only use the compilers that grok it". This particular one may be OK, as it is inside GIT_WINDOWS_NATIVE and I assume everybody on that platform uses recent GCC (or VStudio groks it I guess)?

t/helper/test-drop-caches.c line 18

Quoted 5 lines

t/helper/test-drop-caches.c new file +5 -0

+} SYSTEM_INFORMATION_CLASS;
+
+// private
+typedef enum _SYSTEM_MEMORY_LIST_COMMAND
+{

Style: '{' comes at the end of the previous line, with a single SP immediately before it, unless it is the beginning of the function body.

What you did for _SYSTEM_INFORMATION_CLASS above is correct.

t/helper/test-drop-caches.c line 25

Quoted 7 lines

t/helper/test-drop-caches.c new file +7 -0

+	MemoryCaptureAccessedBits,
+	MemoryCaptureAndResetAccessedBits,
+	MemoryEmptyWorkingSets,
+	MemoryFlushModifiedList,
+	MemoryPurgeStandbyList,
+	MemoryPurgeLowPriorityStandbyList,
+	MemoryCommandMax

Style: avoid CamelCase.

So status is initialized to 1 and anybody without GIT_WINDOWS_NATIVE unconditionally get exit(1)?

Given that 'status' is the return value of this function that returns 'int', I wonder if we need NTSTATUS type here.

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

Quoted 8 lines

t/perf/p7519-fsmonitor.sh new file +1 -0

new file mode 100755
index 0000000000..e41905cb9b
@@ -0,0 +1,161 @@
+#!/bin/sh

t/perf/p7519-fsmonitor.sh line 11

Quoted 10 lines

t/perf/p7519-fsmonitor.sh new file +10 -0

+
+test_description="Test core.fsmonitor"
+
+. ./perf-lib.sh
+
+# This has to be run with GIT_PERF_REPEAT_COUNT=1 to generate valid results.
+# Otherwise the caching that happens for the nth run will negate the validity
+# of the comparisons.
+if [ "$GIT_PERF_REPEAT_COUNT" -ne 1 ]
+then

Style:

if test "$GIT_PERF_REPEAT_COUNT" -ne 1
then

t/perf/p7519-fsmonitor.sh line 72

Quoted 6 lines

t/perf/p7519-fsmonitor.sh new file +6 -0

+ ...
+test_expect_success "setup" '
+...
+	# Hook scaffolding
+	mkdir .git/hooks &&
+	cp ../../../templates/hooks--query-fsmonitor.sample .git/hooks/query-fsmonitor &&

Does this assume $TRASH_DIRECTORY must be in $TEST_DIRECTORY/perf/? Shouldn't t/perf/p[0-9][0-9][0-9][0-9]-*.sh scripts be capable of running with the --root=<ramdisk> option? Perhaps take the copy relative to $TEST_DIRECTORY?

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

Quoted 8 lines · Ben Peart

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, with sections protected by "#ifdef LINUX", "#ifdef WINDOWS", etc. That way, the platform detection and switching between running this program and echoing something into /proc filesystem performed in p7519 can be removed (this test-helper program will be responsible to implement that logic instead). When p7519 wants to drop the filesystem cache, regardless of the platform, it can call this test-helper without having to know how the filesystem cache is dropped.

I do not have strong preference either way, but I have a slight suspicion that the "another approach" above may give us a better result. For one thing, the test-helper can be reused in new perf scripts people will write in the future.

Thanks.

Quoted 11 lines · Junio C Hamano

Quoted 8 lines · Ben Peart

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

Quoted 8 lines

If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, ...

Quoted 12 lines · Ben Peart

Quoted 11 lines · Junio C Hamano

Quoted 8 lines · Ben Peart

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

Quoted 8 lines

Quoted 8 lines

If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, ...

... and having said all that, I think it is perfectly fine to do such a clean-up long after the series gets more exposure to wider audiences as a follow-up patch. Let's get the primary part that affects people's everyday use of Git right and then worry about the test details later.

A quick show of hands to the list audiences. How many of you guys actually tried this series on 'pu' and checked to see its performance (and correctness ;-) characteristics?

Do you folks like it? Rather not have such complexity in the core part of the system? A good first step to start adding more performance improvements? No opinion?

-----Original Message-----
From: Junio C Hamano [mailto:[email protected]] On Behalf Of Junio C Hamano
Sent: Friday, July 7, 2017 2:35 PM
To: Ben Peart <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; David Turner <[email protected]>; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v5 7/7] fsmonitor: add a performance test

Quoted 12 lines · Ben Peart

Quoted 11 lines · Junio C Hamano

Quoted 8 lines · Ben Peart

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

Quoted 8 lines

Quoted 8 lines

If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, ...

... and having said all that, I think it is perfectly fine to do such a clean-up long after the series gets more exposure to wider audiences as a follow-up patch. Let's get the primary part that affects people's everyday use of Git right and then worry about the test details later.

A quick show of hands to the list audiences. How many of you guys actually tried this series on 'pu' and checked to see its performance (and correctness ;-) characteristics?

Do you folks like it? Rather not have such complexity in the core part of the system? A good first step to start adding more performance improvements? No opinion?

Quoted 12 lines · Ben Peart

Quoted 11 lines · Junio C Hamano

Quoted 8 lines · Ben Peart

Quoted 4 lines

Having said all that, I think you are using this ONLY on windows; perhaps it is better to drop #ifdef GIT_WINDOWS_NATIVE from all of the above and arrange Makefile to build test-drop-cache only on that platform, or something?

I didn't find any other examples of Windows only tools. I'll update the #ifdef to properly dump the file system cache on Linux as well and only error out on other platforms.

Quoted 8 lines

Quoted 8 lines

If this will become Windows-only, then I have no problem with platform specfic typedef ;-) I have no problem with CamelCase, either, as that follows the local convention on the platform (similar to those in compat/* that are only for Windows).

Having said all that.

Another approach is to build this helper on all platforms, ...

... and having said all that, I think it is perfectly fine to do such a clean-up long after the series gets more exposure to wider audiences as a follow-up patch. Let's get the primary part that affects people's everyday use of Git right and then worry about the test details later.

A quick show of hands to the list audiences. How many of you guys actually tried this series on 'pu' and checked to see its performance (and correctness ;-) characteristics?

Do you folks like it? Rather not have such complexity in the core part of the system? A good first step to start adding more performance improvements? No opinion?

Changes from V4 include:

Quoted 1 line · Ben Peart

Changes from V4 include:

...

I took a look at this patch series except the last patch ([PATCH v5 7/7] fsmonitor: add a performance test) as Junio reviewed it already, and had only a few comments on patches 3/7 and 4/7.

I am still not convinced by the discussions following v2
(http://public-inbox.org/git/[email protected]/)
about using a hook instead of for example a "core.fsmonitorcommand".

I think using a hook is not necessary and might not be a good match for later optimizations. For example people might want to use a library or some OS specific system calls to do what the hook does.

AEvar previously reported some not so great performance numbers on some big Booking.com boxes with a big monorepo and it seems that using taskset for example to make sure that the hook is run on the same CPU improves these numbers significantly. So avoiding to run a separate process can be important in some cases.

Quoted 17 lines · Christian Couder

Quoted 1 line · Ben Peart

Changes from V4 include:

...

I took a look at this patch series except the last patch ([PATCH v5 7/7] fsmonitor: add a performance test) as Junio reviewed it already, and had only a few comments on patches 3/7 and 4/7.

I am still not convinced by the discussions following v2
(http://public-inbox.org/git/[email protected]/)

about using a hook instead of for example a "core.fsmonitorcommand".

I think using a hook is not necessary and might not be a good match for later optimizations. For example people might want to use a library or some OS specific system calls to do what the hook does.

Quoted 6 lines

AEvar previously reported some not so great performance numbers on some big Booking.com boxes with a big monorepo and it seems that using taskset for example to make sure that the hook is run on the same CPU improves these numbers significantly. So avoiding to run a separate process can be important in some cases.

Using a hook is the only pattern I've seen in git that provides a way to enable OS specific calls. I used a hook so that different file monitoring services could be plugged in depending on the OS or tools available. The Watchman integration script was mostly intended as a sample that could be used where Watchman is available and works well.

I had not heard about the taskset issues you mention above. If there is something else that can be done to make this work better, please let me know the details.

If I'm understanding you correctly, you are suggesting that someone should be able to configure a setting (core.fsmonitorcommand) that gives a custom command line that would be run instead of running the query-fsmonitor hook.

I'm not entirely sure how that should work. There are command line options that need to be passed (currently the interface version as well as the current clock in nanoseconds). How would those passed when using the custom command?

Is it OK to just append them to the given command line? Does there need to be some substitution token to indicate where they should be inserted (ie "mycustomcommand --custom --options %version% --more-options %timestamp%"). Are there any other tokens that should be supported (ie PID or processor mask?).

Is there a design pattern already used somewhere in git that I can follow or is this all blazing a new trail?

Thanks for continuing to look into this. Feedback is good!

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Friday, September 15, 2017 3:21 PM
To: [email protected]
Cc: David Turner <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

config.c +3 -0

+int git_config_get_fsmonitor(void)
+{
+	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))

fsmonitor.c new file +4 -0

+void add_fsmonitor(struct index_state *istate) {
+	int i;
+
+	if (!istate->fsmonitor_last_update) {

+/*
+ * Clear the given cache entries CE_FSMONITOR_VALID bit and invalidate

Quoted 9 lines

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Friday, September 15, 2017 3:21 PM
To: [email protected]
Cc: David Turner <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

config.c int git_config_get_max_percent_split_change(void) · line 2180

Quoted 13 lines

config.c +13 -0

+int git_config_get_fsmonitor(void)
+{
+	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
+		core_fsmonitor = getenv("GIT_FSMONITOR_TEST");
+
+	if (core_fsmonitor && !*core_fsmonitor)
+		core_fsmonitor = NULL;
+
+	if (core_fsmonitor)
+		return 1;
+
+	return 0;
+}

This functions return values are backwards relative to the rest of the git_config_* functions.

[snip]

+> /*
+> * With fsmonitor, we can trust the untracked cache's valid field.
+> */

[snip]

fsmonitor.c line 24

Quoted 3 lines

fsmonitor.c new file +3 -0

+int read_fsmonitor_extension(struct index_state *istate, const void *data,
+	unsigned long sz)
+{

If git_config_get_fsmonitor returns 0, fsmonitor_dirty will leak.

[snip]

fsmonitor.c line 179

Quoted 1 line

fsmonitor.c new file +1 -0

+	/* a fsmonitor process can return '*' to indicate all entries are invalid */

That's not documented in your documentation. Also, I'm not sure I like it: what
if I have a file whose name starts with '*'? Yeah, that would be silly, but this indicates the need for the protocol to have some sort of signaling mechanism that's out-of-band Maybe have some key\0value\0 pairs and then \0\0 and then the list of files? Or, if you want to keep it really simple, allow an entry of '/' (which is an invalid filename) to mean 'all'.

Quoted 4 lines

fsmonitor.c new file +4 -0

+void add_fsmonitor(struct index_state *istate) {
+	int i;
+
+	if (!istate->fsmonitor_last_update) {

[snip]

fsmonitor.c line 219

Quoted 1 line

fsmonitor.c new file +1 -0

+		/* reset the untracked cache */

Is this really necessary? Shouldn't the untracked cache be in a correct state already?

Quoted 2 lines

+/*
+ * Clear the given cache entries CE_FSMONITOR_VALID bit and invalidate

Nit: "s/entries/entry's/".

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Monday, September 18, 2017 9:07 AM
To: David Turner <[email protected]>; 'Ben Peart' <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

Thanks for taking the time to review/provide feedback!

Quoted 10 lines · David Turner

Quoted 9 lines

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Friday, September 15, 2017 3:21 PM
To: [email protected]
Cc: David Turner <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

Quoted 15 lines

config.c int git_config_get_max_percent_split_change(void) · line 2180

Quoted 13 lines

config.c +13 -0

+int git_config_get_fsmonitor(void)
+{
+	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))
+		core_fsmonitor = getenv("GIT_FSMONITOR_TEST");
+
+	if (core_fsmonitor && !*core_fsmonitor)
+		core_fsmonitor = NULL;
+
+	if (core_fsmonitor)
+		return 1;
+
+	return 0;
+}

This functions return values are backwards relative to the rest of the

git_config_* functions.

I'm confused. If core.fsmonitor is configured, it returns 1. If it is not configured, it returns 0. I don't make use of the -1 /* default value */ option as I didn't see any use/value in this case. What is backwards?

Quoted 6 lines

[snip]

+> /*
+> * With fsmonitor, we can trust the untracked cache's valid field.
+> */

Did you intend to make a comment here?

Quoted 39 lines

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Monday, September 18, 2017 9:07 AM
To: David Turner <[email protected]>; 'Ben Peart' <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

Thanks for taking the time to review/provide feedback!

Quoted 25 lines · David Turner

Quoted 13 lines

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Friday, September 15, 2017 3:21 PM
To: [email protected]
Cc: David Turner <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v6 04/12] fsmonitor: teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

config.c +3 -0

+int git_config_get_fsmonitor(void)
+{
+	if (git_config_get_pathname("core.fsmonitor", &core_fsmonitor))

config.c int git_config_get_max_percent_split_change(void) · line 2180

Quoted 10 lines

config.c +10 -0

+		core_fsmonitor = getenv("GIT_FSMONITOR_TEST");
+
+	if (core_fsmonitor && !*core_fsmonitor)
+		core_fsmonitor = NULL;
+
+	if (core_fsmonitor)
+		return 1;
+
+	return 0;
+}

This functions return values are backwards relative to the rest of the

Quoted 5 lines

git_config_* functions.

I'm confused. If core.fsmonitor is configured, it returns 1. If it is not configured, it returns 0. I don't make use of the -1 /* default value */ option as I didn't see any use/value in this case. What is backwards?

The other git_config_* functions return 1 for error and 0 for success.

Quoted 8 lines

Quoted 7 lines

[snip]

+> /*
+> * With fsmonitor, we can trust the untracked cache's valid field.
+> */

Quoted 1 line

Did you intend to make a comment here?

Sorry. I was going to make a comment that I didn't see how that could work since we weren't touching the untracked cache here, but then I saw the bit
further down. I'm still not sure it works (see comment on 10/12), but at
least it could in theory work.

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Friday, September 15, 2017 3:21 PM
To: [email protected]
Cc: David Turner <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v6 05/12] fsmonitor: add documentation for the fsmonitor extension.

This includes the core.fsmonitor setting, the query-fsmonitor hook, and the fsmonitor index extension.

Signed-off-by Ben Peart <[email protected]>

---
 Documentation/config.txt                 |  6 ++++++
 Documentation/githooks.txt               | 23 +++++++++++++++++++++++
 Documentation/technical/index-format.txt | 19 +++++++++++++++++++
 3 files changed, 48 insertions(+)

Documentation/config.txt index renamed from Documentation/config.txt +0 -0

dc4e3f58a2..c196007a27 100644

Documentation/config.txt +2 -0

+The paths should be relative to the root of the working directory and
+be separated by a single NUL.

Documentation/config.txt +4 -0

+  - 32-bit bitmap size: the size of the CE_FSMONITOR_VALID bitmap.
+
+  - An ewah bitmap, the n-th bit indicates whether the n-th index entry
+    is not CE_FSMONITOR_VALID.

Quoted 30 lines

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Friday, September 15, 2017 3:21 PM
To: [email protected]
Cc: David Turner <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v6 05/12] fsmonitor: add documentation for the fsmonitor extension.

This includes the core.fsmonitor setting, the query-fsmonitor hook, and the fsmonitor index extension.

Signed-off-by Ben Peart <[email protected]>

---
  Documentation/config.txt                 |  6 ++++++
  Documentation/githooks.txt               | 23 +++++++++++++++++++++++
  Documentation/technical/index-format.txt | 19 +++++++++++++++++++
  3 files changed, 48 insertions(+)

Documentation/config.txt index renamed from Documentation/config.txt +0 -0

dc4e3f58a2..c196007a27 100644

Documentation/config.txt +0 -0

@@ -413,6 +413,12 @@ core.protectNTFS::
  	8.3 "short" names.
  	Defaults to `true` on Windows, and `false` elsewhere.
 

Documentation/config.txt core.protectNTFS:: · line 420

Quoted 5 lines

Documentation/config.txt +5 -0

+core.fsmonitor::
+	If set, the value of this variable is used as a command which
+	will identify all files that may have changed since the
+	requested date/time. This information is used to speed up git by
+	avoiding unnecessary processing of files that have not changed.

I'm confused here. You have a file called "fsmonitor-watchman", which seems to discuss the protocol for core.fsmonitor scripts in general, and you have this documentation, which does not link to that file. Can you clarify this?

<snip>

Documentation/config.txt

Quoted 3 lines

Documentation/config.txt +3 -0

+The hook should output to stdout the list of all files in the working
+directory that may have changed since the requested time.  The logic
+should be inclusive so that it does not miss any potential changes.

+"It is OK to include files which have not actually changed. Newly-created and deleted files should also be included. When files are renamed, both the old and the new name should be included."

Also, please discuss case sensitivity issues (e.g. on OS X).

Quoted 2 lines

Documentation/config.txt +2 -0

+The paths should be relative to the root of the working directory and
+be separated by a single NUL.

<snip>

Documentation/config.txt

Quoted 5 lines

Documentation/config.txt +5 -0

+  - 32-bit version number: the current supported version is 1.
+
+  - 64-bit time: the extension data reflects all changes through the given
+	time which is stored as the nanoseconds elapsed since midnight,
+	January 1, 1970.

Nit: Please specify signed or unsigned for these. (I expect to be getting out of cryosleep around 2262, and I want to know if my old git repos will keep working...)

Quoted 4 lines

Documentation/config.txt +4 -0

+  - 32-bit bitmap size: the size of the CE_FSMONITOR_VALID bitmap.
+
+  - An ewah bitmap, the n-th bit indicates whether the n-th index entry
+    is not CE_FSMONITOR_VALID.

+[[fsmonitor-watchman]]
+fsmonitor-watchman
+~~~~~~~~~~~~~~~

Quoted 3 lines · Ben Peart

+[[fsmonitor-watchman]]
+fsmonitor-watchman
+~~~~~~~~~~~~~~~

I've queued a mini squash on top to make sure the ~~~~ line aligns with the length of the string above it by adding three ~'s here.

-----Original Message-----
From: Ben Peart [mailto:[email protected]]
Sent: Friday, September 15, 2017 3:21 PM
To: [email protected]
Cc: David Turner <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: [PATCH v6 06/12] ls-files: Add support in ls-files to display the fsmonitor valid bit

Add a new command line option (-f) to ls-files to have it use lowercase letters for 'fsmonitor valid' files

t/helper/test-dump-fsmonitor.c new file +5 -0

new file mode 100644
index 0000000000..482d749bb9
@@ -0,0 +1,21 @@
+#include "cache.h"
+
+int cmd_main(int ac, const char **av)
+{
+	struct index_state *istate = &the_index;

Quoted 17 lines · Ben Peart

t/helper/test-dump-fsmonitor.c new file +11 -0

new file mode 100644
index 0000000000..482d749bb9
@@ -0,0 +1,21 @@
+#include "cache.h"
+
+int cmd_main(int ac, const char **av)
+{
+	struct index_state *istate = &the_index;
+	int i;
+
+	setup_git_directory();
+	if (do_read_index(istate, get_index_file(), 0) < 0)
+		die("unable to read index file");
+	if (!istate->fsmonitor_last_update) {

t/helper/test-dump-fsmonitor.c line 15

Quoted 4 lines

t/helper/test-dump-fsmonitor.c new file +4 -0

+		printf("no fsmonitor\n");
+		return 0;
+	}
+	printf("fsmonitor last update %"PRIuMAX"\n", istate->fsmonitor_last_update);

After pushing this out and had Travis complain, I queued a squash on top of this to cast the argument to (uintmax_t), like you did in an earlier step (I think it was [PATCH 04/12]).

Quoted 20 lines · Junio C Hamano

Quoted 18 lines · Ben Peart

t/helper/test-dump-fsmonitor.c new file +12 -0

new file mode 100644
index 0000000000..482d749bb9
@@ -0,0 +1,21 @@
+#include "cache.h"
+
+int cmd_main(int ac, const char **av)
+{
+    struct index_state *istate = &the_index;
+    int i;
+
+    setup_git_directory();
+    if (do_read_index(istate, get_index_file(), 0) < 0)
+        die("unable to read index file");
+    if (!istate->fsmonitor_last_update) {
+        printf("no fsmonitor\n");

Quoted 9 lines

Quoted 4 lines

t/helper/test-dump-fsmonitor.c new file +3 -0

+        return 0;
+    }
+    printf("fsmonitor last update %"PRIuMAX"\n",

istate->fsmonitor_last_update);

After pushing this out and had Travis complain, I queued a squash on top of this to cast the argument to (uintmax_t), like you did in an earlier step (I think it was [PATCH 04/12]).

Thanks. I'll update this to cast it as (uint64_t) as that is what get/put_be64 use.  As far as I can tell they both map to the same thing (unsigned long long) so there isn't functional difference.

-----Original Message-----
From: Torsten Bögershausen [mailto:[email protected]]
Sent: Monday, September 18, 2017 11:43 AM
To: Ben Peart <[email protected]>; Junio C Hamano <[email protected]>; Ben Peart <[email protected]>
Cc: [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v6 08/12] fsmonitor: add a test tool to dump the index extension

Quoted 32 lines · Ben Peart

Quoted 28 lines · Junio C Hamano

Quoted 20 lines · Ben Peart

(quoted diff) +0 -0

b/t/helper/test-dump-fsmonitor.c new file mode 100644 index 0000000000..482d749bb9

t/helper/test-dump-fsmonitor.c new file +14 -0

@@ -0,0 +1,21 @@
+#include "cache.h"
+
+int cmd_main(int ac, const char **av) {
+    struct index_state *istate = &the_index;
+    int i;
+
+    setup_git_directory();
+    if (do_read_index(istate, get_index_file(), 0) < 0)
+        die("unable to read index file");
+    if (!istate->fsmonitor_last_update) {
+        printf("no fsmonitor\n");
+        return 0;
+    }
+    printf("fsmonitor last update %"PRIuMAX"\n",

Quoted 1 line

istate->fsmonitor_last_update);

After pushing this out and had Travis complain, I queued a squash on top of this to cast the argument to (uintmax_t), like you did in an earlier step (I think it was [PATCH 04/12]).

Quoted 3 lines

Thanks. I'll update this to cast it as (uint64_t) as that is what get/put_be64 use.  As far as I can tell they both map to the same thing (unsigned long long) so there isn't functional difference.

(Just to double-check): This is the way to print "PRIuMAX" correctly (on all platforms):

printf("fsmonitor last update %"PRIuMAX"\n", (uintmax_t)istate-

Quoted 1 line

fsmonitor_last_update);

Should I just make the variable type itself uintmax_t and then just skip the cast altogether? I went with uint64_t because that is what getnanotime returned.

-----Original Message-----
From: Torsten Bögershausen [mailto:[email protected]]
Sent: Tuesday, September 19, 2017 10:16 AM
To: Ben Peart <[email protected]>
Cc: Ben Peart <[email protected]>; Junio C Hamano <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]; [email protected]
Subject: Re: [PATCH v6 08/12] fsmonitor: add a test tool to dump the index extension

Quoted 5 lines

Should I just make the variable type itself uintmax_t and then just skip the cast altogether? I went with uint64_t because that is what getnanotime returned.

That is a bit of taste question (or answer)

Typically you declare the variables in the type you need, and this is uint64_t.

Let's step back a bit:
To print e.g a variable of type uint32_t, you use PRIu32 in the format string, like this:

fprintf(stderr, "Total %"PRIu32" (delta %"PRIu32"),",....

In theory (it is in the later specs, and it exists on many platforms), there is a PRIu64 as well.

We don't seem to use it in Git, probably because uintmax_t is (more) portable and understood by all platforms which support Git. (And beside that, on most platforms uintmax_t is 64 bit).

So my suggestion would be to keep uint64_t and cast the variable into uintmax_t whenever it is printed.