[PATCH 0/2] fsmonitor: Stop reading from PWD, write fsmonitor+split index right

[PATCH 0/2] fsmonitor: Stop reading from PWD, write fsmonitor+split index right

[Alex Vandiver]

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

[PATCH 1/2] fsmonitor: Read from getcwd(), not the PWD environment variable

[Alex Vandiver]

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.

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
---
 t/t7519/fsmonitor-watchman                 | 3 ++-
 templates/hooks--fsmonitor-watchman.sample | 3 ++-
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index a3e30bf54..5fe72cefa 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -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;
diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample
index 9a082f278..ba6d88c5f 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -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

[PATCH 2/2] fsmonitor: Store fsmonitor bitmap before splitting index

[Alex Vandiver]

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.

Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
---
 fsmonitor.c                 | 20 ++++++++++++--------
 fsmonitor.h                 |  9 ++++++++-
 read-cache.c                |  3 +++
 t/t7519-status-fsmonitor.sh | 13 +++++++++++++
 4 files changed, 36 insertions(+), 9 deletions(-)

diff --git a/fsmonitor.c b/fsmonitor.c
index f494a866d..0af7c4edb 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -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);
diff --git a/fsmonitor.h b/fsmonitor.h
index 0de644e01..cd3cc0ccf 100644
--- a/fsmonitor.h
+++ b/fsmonitor.h
@@ -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);
 
diff --git a/read-cache.c b/read-cache.c
index c18e5e623..7976d39d6 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -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)
diff --git a/t/t7519-status-fsmonitor.sh b/t/t7519-status-fsmonitor.sh
index c6df85af5..eb2d13bbc 100755
--- a/t/t7519-status-fsmonitor.sh
+++ b/t/t7519-status-fsmonitor.sh
@@ -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

Re: [PATCH 2/2] fsmonitor: Store fsmonitor bitmap before splitting index

[Junio C Hamano]

Alex Vandiver <alexmv@dropbox.com> writes:

> ba1b9caca6 resolved the problem of the fsmonitor data being applied to

(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>


> 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.

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?

> 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.

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

Thanks.

Re: [PATCH 2/2] fsmonitor: Store fsmonitor bitmap before splitting index

[Ben Peart]

On 11/9/2017 2:58 PM, Alex Vandiver wrote:
> 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.
> 
> Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
> ---

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.


>   
> +# 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
> 

Re: [PATCH 2/2] fsmonitor: Store fsmonitor bitmap before splitting index

[Alex Vandiver]

On Mon, 13 Nov 2017, Ben Peart wrote:
> 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

> > +# 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

Re: [PATCH 1/2] fsmonitor: Read from getcwd(), not the PWD environment variable

[Junio C Hamano]

Alex Vandiver <alexmv@dropbox.com> writes:

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

> Subject: Re: [PATCH 1/2] fsmonitor: Read from getcwd(), not the PWD environment variable

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

> 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.

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

Thanks, will queue.

> Signed-off-by: Alex Vandiver <alexmv@dropbox.com>
> ---
>  t/t7519/fsmonitor-watchman                 | 3 ++-
>  templates/hooks--fsmonitor-watchman.sample | 3 ++-
>  2 files changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
> index a3e30bf54..5fe72cefa 100755
> --- a/t/t7519/fsmonitor-watchman
> +++ b/t/t7519/fsmonitor-watchman
> @@ -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;
> diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample
> index 9a082f278..ba6d88c5f 100755
> --- a/templates/hooks--fsmonitor-watchman.sample
> +++ b/templates/hooks--fsmonitor-watchman.sample
> @@ -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;

[PATCH v1] fsmonitor: simplify determining the git worktree under Windows

[Ben Peart]

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.

Signed-off-by: Ben Peart <benpeart@microsoft.com>
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---

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(-)

diff --git a/t/t7519/fsmonitor-watchman b/t/t7519/fsmonitor-watchman
index 5fe72cefaf..5514edcf68 100755
--- a/t/t7519/fsmonitor-watchman
+++ b/t/t7519/fsmonitor-watchman
@@ -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();
diff --git a/templates/hooks--fsmonitor-watchman.sample b/templates/hooks--fsmonitor-watchman.sample
index ba6d88c5f8..e673bb3980 100755
--- a/templates/hooks--fsmonitor-watchman.sample
+++ b/templates/hooks--fsmonitor-watchman.sample
@@ -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

Re: [PATCH v1] fsmonitor: simplify determining the git worktree under Windows

[Junio C Hamano]

Ben Peart <benpeart@microsoft.com> writes:

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

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.

>
> 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.
>
> Signed-off-by: Ben Peart <benpeart@microsoft.com>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---

The patch looks reasonable ;-)  Thanks.

> +if ($^O =~ 'msys' || $^O =~ 'cygwin') {
> +	$git_work_tree = Win32::GetCwd();
> +	$git_work_tree =~ tr/\\/\//;
>  } else {
>  	require Cwd;
>  	$git_work_tree = Cwd::cwd();