Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree

[Johannes Schindelin]

Hi,

On Thu, 26 Oct 2017, Ben Peart wrote:

> On 10/26/2017 5:27 PM, Alex Vandiver wrote:
> > On Thu, 26 Oct 2017, Ben Peart wrote:
> > > On 10/25/2017 9:31 PM, Alex Vandiver wrote:
> > > > diff --git a/fsmonitor.c b/fsmonitor.c
> > > > index 7c1540c05..0d26ff34f 100644
> > > > --- a/fsmonitor.c
> > > > +++ b/fsmonitor.c
> > > > @@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t
> > > > last_update, struct strbuf *que
> > > > argv[3] = NULL;
> > > > cp.argv = argv;
> > > > cp.use_shell = 1;
> > > > +cp.dir = get_git_work_tree();
> > > >
> > >
> > > I'm not sure what would trigger this problem but I don't doubt that it is
> > > possible.  Better to be safe than sorry. This is a better/faster solution
> > > than
> > > you're previous patch.  Thank you!
> > 
> > See my response on the v1 of this series -- I'm curious how you're
> > _not_ seeing it, actually.  Can  you not replicate just by running
> > `git status` from differing parts of the working tree?
> >   - Alex
> > 
> 
> I saw your response but actually can't replicate it locally.  I've been
> running with Watchman integration on all my repos for months and my "watchman
> watch-list" command only shows the root of the various working directories -
> no subdirectories.

Indeed, I cannot replicate either. The thing is that "status" is marked
with GIT_SETUP in git.c:

https://github.com/git-for-windows/git/blob/v2.14.3.windows.1/git.c#L465

That means that the setup_git_directory() is run, which sets the current
working directory to the top-level directory.

So there is your explanation why neither Ben nor I saw this.

And I agree with Ben that it is safer to do it the way you suggested, just
in case that the call path comes from a Git command that was not marked
with GIT_SETUP.

Ciao,
Johannes

Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree

[Alex Vandiver]

On Thu, 26 Oct 2017, Ben Peart wrote:
> I saw your response but actually can't replicate it locally.  I've been
> running with Watchman integration on all my repos for months and my "watchman
> watch-list" command only shows the root of the various working directories -
> no subdirectories.

Weird.  I double-checked and I see the same behavior with watchman
4.9.0 as with 4.7.0 that I had been using previously.  I wonder if
something's different between `git` in `next` from wherever your
branch was based.
 - Alex

Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree

[Ben Peart]

On 10/26/2017 5:27 PM, Alex Vandiver wrote:

On Thu, 26 Oct 2017, Ben Peart wrote:

On 10/25/2017 9:31 PM, Alex Vandiver wrote:

diff --git a/fsmonitor.c b/fsmonitor.c
index 7c1540c05..0d26ff34f 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t
last_update, struct strbuf *que
argv[3] = NULL;
cp.argv = argv;
cp.use_shell = 1;
+cp.dir = get_git_work_tree();
   


I'm not sure what would trigger this problem but I don't doubt that it is
possible.  Better to be safe than sorry. This is a better/faster solution than
you're previous patch.  Thank you!


See my response on the v1 of this series -- I'm curious how you're
_not_ seeing it, actually.  Can  you not replicate just by running
`git status` from differing parts of the working tree?
  - Alex



I saw your response but actually can't replicate it locally.  I've been 
running with Watchman integration on all my repos for months and my 
"watchman watch-list" command only shows the root of the various working 
directories - no subdirectories.

Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree

[Alex Vandiver]

On Thu, 26 Oct 2017, Ben Peart wrote:
> On 10/25/2017 9:31 PM, Alex Vandiver wrote:
> > diff --git a/fsmonitor.c b/fsmonitor.c
> > index 7c1540c05..0d26ff34f 100644
> > --- a/fsmonitor.c
> > +++ b/fsmonitor.c
> > @@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t
> > last_update, struct strbuf *que
> > argv[3] = NULL;
> > cp.argv = argv;
> > cp.use_shell = 1;
> > +cp.dir = get_git_work_tree();
> >   
>
> I'm not sure what would trigger this problem but I don't doubt that it is
> possible.  Better to be safe than sorry. This is a better/faster solution than
> you're previous patch.  Thank you!

See my response on the v1 of this series -- I'm curious how you're
_not_ seeing it, actually.  Can  you not replicate just by running
`git status` from differing parts of the working tree?
 - Alex

Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree

[Ben Peart]

On 10/25/2017 9:31 PM, Alex Vandiver wrote:

The fsmonitor command inherits the PWD of its caller, which may be
anywhere in the working copy.  This makes is difficult for the
fsmonitor command to operate on the whole repository.  Specifically,
for the watchman integration, this causes each subdirectory to get its
own watch entry.

Set the CWD to the top of the working directory, for consistency.

Signed-off-by: Alex Vandiver 
---
  fsmonitor.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/fsmonitor.c b/fsmonitor.c
index 7c1540c05..0d26ff34f 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t 
last_update, struct strbuf *que
argv[3] = NULL;
cp.argv = argv;
cp.use_shell = 1;
+cp.dir = get_git_work_tree();
  


I'm not sure what would trigger this problem but I don't doubt that it 
is possible.  Better to be safe than sorry. This is a better/faster 
solution than you're previous patch.  Thank you!



return capture_command(, query_result, 1024);
  }

Re: [PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree

[Alex Vandiver]

On Wed, 25 Oct 2017, Alex Vandiver wrote:
> The fsmonitor command inherits the PWD of its caller, which may be
> anywhere in the working copy.  This makes is difficult for the
> fsmonitor command to operate on the whole repository.  Specifically,
> for the watchman integration, this causes each subdirectory to get its
> own watch entry.
> 
> Set the CWD to the top of the working directory, for consistency.
> 
> Signed-off-by: Alex Vandiver 
> ---
>  fsmonitor.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/fsmonitor.c b/fsmonitor.c
> index 7c1540c05..0d26ff34f 100644
> --- a/fsmonitor.c
> +++ b/fsmonitor.c
> @@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t 
> last_update, struct strbuf *que
>   argv[3] = NULL;
>   cp.argv = argv;
>   cp.use_shell = 1;
> +cp.dir = get_git_work_tree();

Looks like my editor swapped out a tab on me.  I'll hold off on
sending a revised version to collect any other comments.
 - Alex

[PATCH v2 1/4] fsmonitor: Set the PWD to the top of the working tree

[Alex Vandiver]

The fsmonitor command inherits the PWD of its caller, which may be
anywhere in the working copy.  This makes is difficult for the
fsmonitor command to operate on the whole repository.  Specifically,
for the watchman integration, this causes each subdirectory to get its
own watch entry.

Set the CWD to the top of the working directory, for consistency.

Signed-off-by: Alex Vandiver 
---
 fsmonitor.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fsmonitor.c b/fsmonitor.c
index 7c1540c05..0d26ff34f 100644
--- a/fsmonitor.c
+++ b/fsmonitor.c
@@ -121,6 +121,7 @@ static int query_fsmonitor(int version, uint64_t 
last_update, struct strbuf *que
argv[3] = NULL;
cp.argv = argv;
cp.use_shell = 1;
+cp.dir = get_git_work_tree();
 
return capture_command(, query_result, 1024);
 }
-- 
2.15.0.rc1.413.g76aedb451