fsmonitor: Stop reading from PWD, write fsmonitor+split index right

A couple further patches for the fsmonitor branch, which ideally I'd have noticed before my previous series landed.

In the first patch I believe I've found the underlying reason for the PWD confusion in the previous go-around -- but I'm not sure I'm wholly convinced about my solution to it. Specifically, it seems like this problem is likely more widespread than this one place, so adjusting it in the example hook may just be leaving the same dangerous edge for others to stumble across later.

- Alex

Though the process has chdir'd to the root of the working tree, the PWD environment variable is only guaranteed to be updated accordingly if a shell is involved -- which is not guaranteed to be the case. That is, if /usr/bin/perl is a binary, $ENV{PWD} is unchanged from whatever spawned git -- if /usr/bin/perl is a trivial shell wrapper to the real perl, $ENV{PWD} will have been updated to the root of the working copy.

Update to read from the Cwd module using the getcwd syscall, not the PWD environment variable. The Cygwin case is left unchanged, as it necessarily _does_ go through a shell.

---
 t/t7519/fsmonitor-watchman                 | 3 ++-
 templates/hooks--fsmonitor-watchman.sample | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

index a3e30bf54..5fe72cefa 100755
@@ -41,7 +41,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
 	$git_work_tree =~ s/[\r\n]+//g;
 	$git_work_tree =~ s,\\,/,g;
 } else {
-	$git_work_tree = $ENV{'PWD'};
+	require Cwd;
+	$git_work_tree = Cwd::cwd();
 }
 
 my $retry = 1;

index 9a082f278..ba6d88c5f 100755
@@ -40,7 +40,8 @@ if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
 	$git_work_tree =~ s/[\r\n]+//g;
 	$git_work_tree =~ s,\\,/,g;
 } else {
-	$git_work_tree = $ENV{'PWD'};
+	require Cwd;
+	$git_work_tree = Cwd::cwd();
 }
 
 my $retry = 1;

2.15.0.rc1.413.g76aedb451

Alex Vandiver <[email protected]> writes:

This comment is only so that I do not keep editing them while queuing..

Downcase "Read" (or any word after "<area>: " on the commit title).

Interesting observation. Why didn't anybody else notice it, I wonder?

Thanks, will queue.

I haven't tested the non Windows paths but the patch looks reasonable.

This inspired me to get someone more familiar with perl (thanks Johannes) to revisit this code for the Windows side as well. The logic for determining the git worktree when running on Windows is more complex than necessary. It also spawns multiple processes (uname and cygpath) which slows things down.

Simplify and speed up the process of finding the git worktree when running on Windows by keeping it in perl and avoiding spawning helper processes.

---

Notes:

    Base Ref:
    Web-Diff: https://github.com/benpeart/git/commit/20affe124b
    Checkout: git fetch https://github.com/benpeart/git fsmonitor_splitindex-v1 && git checkout 20affe124b

 t/t7519/fsmonitor-watchman                 | 13 +++----------
 templates/hooks--fsmonitor-watchman.sample | 13 +++----------
 2 files changed, 6 insertions(+), 20 deletions(-)

index 5fe72cefaf..5514edcf68 100755
@@ -29,17 +29,10 @@ if ($version == 1) {
 	    "Falling back to scanning...\n";
 }
 
-# Convert unix style paths to escaped Windows style paths when running
-# in Windows command prompt
-
-my $system = `uname -s`;
-$system =~ s/[\r\n]+//g;
 my $git_work_tree;
-
-if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
-	$git_work_tree = `cygpath -aw "\$PWD"`;
-	$git_work_tree =~ s/[\r\n]+//g;
-	$git_work_tree =~ s,\\,/,g;
+if ($^O =~ 'msys' || $^O =~ 'cygwin') {
+	$git_work_tree = Win32::GetCwd();
+	$git_work_tree =~ tr/\\/\//;
 } else {
 	require Cwd;
 	$git_work_tree = Cwd::cwd();

index ba6d88c5f8..e673bb3980 100755
@@ -28,17 +28,10 @@ if ($version == 1) {
 	    "Falling back to scanning...\n";
 }
 
-# Convert unix style paths to escaped Windows style paths when running
-# in Windows command prompt
-
-my $system = `uname -s`;
-$system =~ s/[\r\n]+//g;
 my $git_work_tree;
-
-if ($system =~ m/^MSYS_NT/ || $system =~ m/^MINGW/) {
-	$git_work_tree = `cygpath -aw "\$PWD"`;
-	$git_work_tree =~ s/[\r\n]+//g;
-	$git_work_tree =~ s,\\,/,g;
+if ($^O =~ 'msys' || $^O =~ 'cygwin') {
+	$git_work_tree = Win32::GetCwd();
+	$git_work_tree =~ tr/\\/\//;
 } else {
 	require Cwd;
 	$git_work_tree = Cwd::cwd();

base-commit: f9d9e50b62094689773dccc5f9493fa15e30d592

2.15.0.windows.1

I do not think the above line part of the proposed log message for this patch ;-) I guess I'll strip these earlier parts and leave only the last paragraph while queuing.

The patch looks reasonable ;-) Thanks.

ba1b9caca6 resolved the problem of the fsmonitor data being applied to the non-base index when reading; however, a similar problem exists when writing the index. Specifically, writing of the fsmonitor extension happens only after the work to split the index has been applied -- as such, the information in the index is only for the non-"base" index, and thus the extension information contains only partial data.

When saving, compute the ewah bitmap before the index is split, and store it in the fsmonitor_dirty field, mirroring the behavior that occurred during reading. fsmonitor_dirty is kept from being leaked by being freed when the extension data is written -- which always happens precisely once, no matter the split index configuration.

---
 fsmonitor.c                 | 20 ++++++++++++--------
 fsmonitor.h                 |  9 ++++++++-
 read-cache.c                |  3 +++
 t/t7519-status-fsmonitor.sh | 13 +++++++++++++
 4 files changed, 36 insertions(+), 9 deletions(-)

index f494a866d..0af7c4edb 100644
@@ -54,12 +54,19 @@ int read_fsmonitor_extension(struct index_state *istate, const void *data,
 	return 0;
 }
 
+void fill_fsmonitor_bitmap(struct index_state *istate)
+{
+	int i;
+	istate->fsmonitor_dirty = ewah_new();
+	for (i = 0; i < istate->cache_nr; i++)
+		if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
+			ewah_set(istate->fsmonitor_dirty, i);
+}
+
 void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 {
 	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;

@@ -73,12 +80,9 @@ void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate)
 	strbuf_add(sb, &ewah_size, sizeof(uint32_t)); /* we'll fix this up later */
 
 	ewah_start = sb->len;
-	bitmap = ewah_new();
-	for (i = 0; i < istate->cache_nr; i++)
-		if (!(istate->cache[i]->ce_flags & CE_FSMONITOR_VALID))
-			ewah_set(bitmap, i);
-	ewah_serialize_strbuf(bitmap, sb);
-	ewah_free(bitmap);
+	ewah_serialize_strbuf(istate->fsmonitor_dirty, sb);
+	ewah_free(istate->fsmonitor_dirty);
+	istate->fsmonitor_dirty = NULL;
 
 	/* fix up size field */
 	put_be32(&ewah_size, sb->len - ewah_start);

index 0de644e01..cd3cc0ccf 100644
@@ -10,7 +10,14 @@ extern struct trace_key trace_fsmonitor;
 extern int read_fsmonitor_extension(struct index_state *istate, const void *data, unsigned long sz);
 
 /*
- * Write the CE_FSMONITOR_VALID state into the fsmonitor index extension.
+ * Fill the fsmonitor_dirty ewah bits with their state from the index,
+ * before it is split during writing.
+ */
+extern void fill_fsmonitor_bitmap(struct index_state *istate);
+
+/*
+ * Write the CE_FSMONITOR_VALID state into the fsmonitor index
+ * extension.  Reads from the fsmonitor_dirty ewah in the index.
  */
 extern void write_fsmonitor_extension(struct strbuf *sb, struct index_state *istate);
 

index c18e5e623..7976d39d6 100644
@@ -2529,6 +2529,9 @@ int write_locked_index(struct index_state *istate, struct lock_file *lock,
 	int new_shared_index, ret;
 	struct split_index *si = istate->split_index;
 
+	if (istate->fsmonitor_last_update)
+		fill_fsmonitor_bitmap(istate);
+
 	if (!si || alternate_index_output ||
 	    (istate->cache_changed & ~EXTMASK)) {
 		if (si)

index c6df85af5..eb2d13bbc 100755
@@ -301,4 +301,17 @@ do
 	done
 done
 
+# test that splitting the index dosn't interfere
+test_expect_success 'splitting the index results in the same state' '
+	write_integration_script &&
+	dirty_repo &&
+	git update-index --fsmonitor  &&
+	git ls-files -f >expect &&
+	test-dump-fsmonitor >&2 && echo &&
+	git update-index --fsmonitor --split-index &&
+	test-dump-fsmonitor >&2 && echo &&
+	git ls-files -f >actual &&
+	test_cmp expect actual
+'
+
 test_done

2.15.0.rc1.413.g76aedb451

(from SubmittingPatches)

If you want to reference a previous commit in the history of a stable branch, use the format "abbreviated sha1 (subject, date)", with the subject enclosed in a pair of double-quotes, like this:

Commit f86a374 ("pack-bitmap.c: fix a memleak", 2015-03-30) noticed that ...

The "Copy commit summary" command of gitk can be used to obtain this format, or this invocation of "git show":

    git show -s --date=short --pretty='format:%h ("%s", %ad)' <commit>

So... what's the effect of not applying this change? Do we miss paths that are known to the watchman to have been modified and end up not adding them if we do "git add -u"? Or do we miss paths that are known to the watchman to be clean but mistakenly think are dirty, and spend unnecessary cycles? IOW, is this fixing a correctness issue, or a performance one?

The observation and the approach stated to fix both sounds sensible. I'll queue this too, awaiting for Ben's review.

Thanks.

The patch looks like a reasonable fix to make fsmonitor work correctly with split index. I also did manual testing to verify it was working as expected.

Thanks for adding this additional test case to ensure we don't have any regressions with the interactions between fsmonitor and split-index. While the test does correctly fail before the patch and pass after the patch, I had a question about the test-dump-fsmonitor lines.

Why do you redirect stdout to stderr and then and perform an "echo" afterwards? I don't understand what benefit that provides. I removed this logic and the test still passes so am confused as to what its purpose is.

Ah -- the "echo" was purely to clean up STDERR as I was running the test interactively. It serves no purpose, which is why it was hard to understand its benefit. :)

Apologies for missing this (and in not replying here earlier!). I'll send a commit that drops these.
- Alex