Re: [PATCH] ls-files: properly prepare submodule environment

2017-04-12 Thread Jacob Keller 
On Wed, Apr 12, 2017 at 9:58 AM, Brandon Williams  wrote:
> On 04/11, Jacob Keller wrote:
>> From: Jacob Keller 
>>
>> Since commit e77aa336f116 ("ls-files: optionally recurse into
>> submodules", 2016-10-07) ls-files has known how to recurse into
>> submodules when displaying files.
>>
>> Unfortunately this code does not work as expected. Initially, it
>> produced results indicating that a --super-prefix can't be used from
>> a subdirectory:
>>
>>   fatal: can't use --super-prefix from a subdirectory
>>
>> After commit b58a68c1c187 ("setup: allow for prefix to be passed to git
>> commands", 2017-03-17) this behavior changed and resulted in repeated
>> calls of ls-files with an expanding super-prefix.
>>
>> This recursive expanding super-prefix appears to be the result of
>> ls-files acting on the super-project instead of on the submodule files.
>>
>> We can fix this by properly preparing the submodule environment when
>> setting up the submodule process. This ensures that the command we
>> execute properly reads the directory and ensures that we correctly
>> operate in the submodule instead of repeating oureslves on the
>> super-project contents forever.
>>
>> While we're here lets also add some tests to ensure that ls-files works
>> with recurse-submodules to catch these issues in the future.
>>
>> Signed-off-by: Jacob Keller 
>> ---
>> I found this fix on accident after looking at git-grep code and
>> comparing how ls-files instantiated the submodule. Can someone who knows
>> more about submodules explain the reasoning better for this fix?
>> Essentially we "recurse" into the submodule folder, but still operate as
>> if we're at the root of the project but with a new prefix. So then we
>> keep recursing into the submodule forever.
>
> The reason why this fix is required is that the env var GIT_DIR is set
> to be the parents gitdir.  When recursing the childprocess just uses the
> parents gitdir instead of its own causing it to recurse into itself
> again and again.  The child process's environment needs to have the
> GIT_DIR var cleared so that the child will do discovery and actually
> find its own gitdir.

Right. That makes sense, but that raises the question of how or where
this worked in the first place?

>
>>
>> I also added some tests here, and I *definitely* think this should be a
>> maintenance backport into any place which supports ls-files
>> --recurse-submodules, since as far as I can tell it is otherwise
>> useless.
>>
>> There were no tests for it, so I added some based on git-grep tests. I
>> did not try them against the broken setups, because of the endless
>> recursion.
>
> There are tests for ls-files --recurse-submodules in
> t3007-ls-files-recurse-submodules.sh, though it looks like this
> particular case (which triggers this bug) maybe didn't have any tests.
> I'm actually unsure of why the existing tests didn't catch this (I'm
> probably just bad at writing tests).

It seems to me like this would be a problem for any setup with
submodules, no? Or is it specific case for me? I have a submodule
within a submodule and I'm not setting GIT_DIR manually myself. I want
to isolate exactly what scenario fails here so that the commit
description can be accurate and we know for sure the test cases cover
it.

Thanks,
Jake

>
>
>>
>>  builtin/ls-files.c |   4 +
>>  t/t3080-ls-files-recurse-submodules.sh | 162 
>> +
>>  2 files changed, 166 insertions(+)
>>  create mode 100755 t/t3080-ls-files-recurse-submodules.sh
>>
>> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
>> index d449e46db551..e9b3546ca053 100644
>> --- a/builtin/ls-files.c
>> +++ b/builtin/ls-files.c
>> @@ -15,6 +15,7 @@
>>  #include "string-list.h"
>>  #include "pathspec.h"
>>  #include "run-command.h"
>> +#include "submodule.h"
>>
>>  static int abbrev;
>>  static int show_deleted;
>> @@ -203,6 +204,9 @@ static void show_gitlink(const struct cache_entry *ce)
>>   struct child_process cp = CHILD_PROCESS_INIT;
>>   int status;
>>
>> + prepare_submodule_repo_env(_array);
>> + argv_array_push(_array, GIT_DIR_ENVIRONMENT);
>
> Yes these lines need to be included in order to prepare the environment.
> Thanks for catching that.
>
>> +
>>   if (prefix_len)
>>   argv_array_pushf(_array, "%s=%s",
>>GIT_TOPLEVEL_PREFIX_ENVIRONMENT,
>> diff --git a/t/t3080-ls-files-recurse-submodules.sh 
>> b/t/t3080-ls-files-recurse-submodules.sh
>> new file mode 100755
>> index ..6788a8f09635
>> --- /dev/null
>> +++ b/t/t3080-ls-files-recurse-submodules.sh
>> @@ -0,0 +1,162 @@
>> +#!/bin/sh
>> +
>> +test_description='Test ls-files recurse-submodules feature
>> +
>> +This test verifies the recurse-submodules feature correctly lists files 
>> across
>> +submodules.
>> +'
>> +
>> +. ./test-lib.sh
>> +
>> +test_expect_success 'setup directory structure and

Re: [PATCH] ls-files: properly prepare submodule environment

2017-04-12 Thread Brandon Williams 
On 04/11, Jacob Keller wrote:
> From: Jacob Keller 
> 
> Since commit e77aa336f116 ("ls-files: optionally recurse into
> submodules", 2016-10-07) ls-files has known how to recurse into
> submodules when displaying files.
> 
> Unfortunately this code does not work as expected. Initially, it
> produced results indicating that a --super-prefix can't be used from
> a subdirectory:
> 
>   fatal: can't use --super-prefix from a subdirectory
> 
> After commit b58a68c1c187 ("setup: allow for prefix to be passed to git
> commands", 2017-03-17) this behavior changed and resulted in repeated
> calls of ls-files with an expanding super-prefix.
> 
> This recursive expanding super-prefix appears to be the result of
> ls-files acting on the super-project instead of on the submodule files.
> 
> We can fix this by properly preparing the submodule environment when
> setting up the submodule process. This ensures that the command we
> execute properly reads the directory and ensures that we correctly
> operate in the submodule instead of repeating oureslves on the
> super-project contents forever.
> 
> While we're here lets also add some tests to ensure that ls-files works
> with recurse-submodules to catch these issues in the future.
> 
> Signed-off-by: Jacob Keller 
> ---
> I found this fix on accident after looking at git-grep code and
> comparing how ls-files instantiated the submodule. Can someone who knows
> more about submodules explain the reasoning better for this fix?
> Essentially we "recurse" into the submodule folder, but still operate as
> if we're at the root of the project but with a new prefix. So then we
> keep recursing into the submodule forever.

The reason why this fix is required is that the env var GIT_DIR is set
to be the parents gitdir.  When recursing the childprocess just uses the
parents gitdir instead of its own causing it to recurse into itself
again and again.  The child process's environment needs to have the
GIT_DIR var cleared so that the child will do discovery and actually
find its own gitdir.

> 
> I also added some tests here, and I *definitely* think this should be a
> maintenance backport into any place which supports ls-files
> --recurse-submodules, since as far as I can tell it is otherwise
> useless.
> 
> There were no tests for it, so I added some based on git-grep tests. I
> did not try them against the broken setups, because of the endless
> recursion.

There are tests for ls-files --recurse-submodules in
t3007-ls-files-recurse-submodules.sh, though it looks like this
particular case (which triggers this bug) maybe didn't have any tests.
I'm actually unsure of why the existing tests didn't catch this (I'm
probably just bad at writing tests).


> 
>  builtin/ls-files.c |   4 +
>  t/t3080-ls-files-recurse-submodules.sh | 162 
> +
>  2 files changed, 166 insertions(+)
>  create mode 100755 t/t3080-ls-files-recurse-submodules.sh
> 
> diff --git a/builtin/ls-files.c b/builtin/ls-files.c
> index d449e46db551..e9b3546ca053 100644
> --- a/builtin/ls-files.c
> +++ b/builtin/ls-files.c
> @@ -15,6 +15,7 @@
>  #include "string-list.h"
>  #include "pathspec.h"
>  #include "run-command.h"
> +#include "submodule.h"
>  
>  static int abbrev;
>  static int show_deleted;
> @@ -203,6 +204,9 @@ static void show_gitlink(const struct cache_entry *ce)
>   struct child_process cp = CHILD_PROCESS_INIT;
>   int status;
>  
> + prepare_submodule_repo_env(_array);
> + argv_array_push(_array, GIT_DIR_ENVIRONMENT);

Yes these lines need to be included in order to prepare the environment.
Thanks for catching that.

> +
>   if (prefix_len)
>   argv_array_pushf(_array, "%s=%s",
>GIT_TOPLEVEL_PREFIX_ENVIRONMENT,
> diff --git a/t/t3080-ls-files-recurse-submodules.sh 
> b/t/t3080-ls-files-recurse-submodules.sh
> new file mode 100755
> index ..6788a8f09635
> --- /dev/null
> +++ b/t/t3080-ls-files-recurse-submodules.sh
> @@ -0,0 +1,162 @@
> +#!/bin/sh
> +
> +test_description='Test ls-files recurse-submodules feature
> +
> +This test verifies the recurse-submodules feature correctly lists files 
> across
> +submodules.
> +'
> +
> +. ./test-lib.sh
> +
> +test_expect_success 'setup directory structure and submodule' '
> + echo "foobar" >a &&
> + mkdir b &&
> + echo "bar" >b/b &&
> + git add a b &&
> + git commit -m "add a and b" &&
> + git init submodule &&
> + echo "foobar" >submodule/a &&
> + git -C submodule add a &&
> + git -C submodule commit -m "add a" &&
> + git submodule add ./submodule &&
> + git commit -m "added submodule"
> +'
> +
> +test_expect_success 'ls-files correctly lists files in a submodule' '
> + cat >expect <<-\EOF &&
> + .gitmodules
> + a
> + b/b
> + submodule/a
> + EOF
> +
> + git ls-files --recurse-submodules >actual &&
> + test_cmp expect actual

[PATCH] ls-files: properly prepare submodule environment

2017-04-11 Thread Jacob Keller 
From: Jacob Keller 

Since commit e77aa336f116 ("ls-files: optionally recurse into
submodules", 2016-10-07) ls-files has known how to recurse into
submodules when displaying files.

Unfortunately this code does not work as expected. Initially, it
produced results indicating that a --super-prefix can't be used from
a subdirectory:

  fatal: can't use --super-prefix from a subdirectory

After commit b58a68c1c187 ("setup: allow for prefix to be passed to git
commands", 2017-03-17) this behavior changed and resulted in repeated
calls of ls-files with an expanding super-prefix.

This recursive expanding super-prefix appears to be the result of
ls-files acting on the super-project instead of on the submodule files.

We can fix this by properly preparing the submodule environment when
setting up the submodule process. This ensures that the command we
execute properly reads the directory and ensures that we correctly
operate in the submodule instead of repeating oureslves on the
super-project contents forever.

While we're here lets also add some tests to ensure that ls-files works
with recurse-submodules to catch these issues in the future.

Signed-off-by: Jacob Keller 
---
I found this fix on accident after looking at git-grep code and
comparing how ls-files instantiated the submodule. Can someone who knows
more about submodules explain the reasoning better for this fix?
Essentially we "recurse" into the submodule folder, but still operate as
if we're at the root of the project but with a new prefix. So then we
keep recursing into the submodule forever.

I also added some tests here, and I *definitely* think this should be a
maintenance backport into any place which supports ls-files
--recurse-submodules, since as far as I can tell it is otherwise
useless.

There were no tests for it, so I added some based on git-grep tests. I
did not try them against the broken setups, because of the endless
recursion.

 builtin/ls-files.c |   4 +
 t/t3080-ls-files-recurse-submodules.sh | 162 +
 2 files changed, 166 insertions(+)
 create mode 100755 t/t3080-ls-files-recurse-submodules.sh

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index d449e46db551..e9b3546ca053 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -15,6 +15,7 @@
 #include "string-list.h"
 #include "pathspec.h"
 #include "run-command.h"
+#include "submodule.h"
 
 static int abbrev;
 static int show_deleted;
@@ -203,6 +204,9 @@ static void show_gitlink(const struct cache_entry *ce)
struct child_process cp = CHILD_PROCESS_INIT;
int status;
 
+   prepare_submodule_repo_env(_array);
+   argv_array_push(_array, GIT_DIR_ENVIRONMENT);
+
if (prefix_len)
argv_array_pushf(_array, "%s=%s",
 GIT_TOPLEVEL_PREFIX_ENVIRONMENT,
diff --git a/t/t3080-ls-files-recurse-submodules.sh 
b/t/t3080-ls-files-recurse-submodules.sh
new file mode 100755
index ..6788a8f09635
--- /dev/null
+++ b/t/t3080-ls-files-recurse-submodules.sh
@@ -0,0 +1,162 @@
+#!/bin/sh
+
+test_description='Test ls-files recurse-submodules feature
+
+This test verifies the recurse-submodules feature correctly lists files across
+submodules.
+'
+
+. ./test-lib.sh
+
+test_expect_success 'setup directory structure and submodule' '
+   echo "foobar" >a &&
+   mkdir b &&
+   echo "bar" >b/b &&
+   git add a b &&
+   git commit -m "add a and b" &&
+   git init submodule &&
+   echo "foobar" >submodule/a &&
+   git -C submodule add a &&
+   git -C submodule commit -m "add a" &&
+   git submodule add ./submodule &&
+   git commit -m "added submodule"
+'
+
+test_expect_success 'ls-files correctly lists files in a submodule' '
+   cat >expect <<-\EOF &&
+   .gitmodules
+   a
+   b/b
+   submodule/a
+   EOF
+
+   git ls-files --recurse-submodules >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'ls-files and basic pathspecs' '
+   cat >expect <<-\EOF &&
+   submodule/a
+   EOF
+
+   git ls-files --recurse-submodules -- submodule >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'ls-files and nested submodules' '
+   git init submodule/sub &&
+   echo "foobar" >submodule/sub/a &&
+   git -C submodule/sub add a &&
+   git -C submodule/sub commit -m "add a" &&
+   git -C submodule submodule add ./sub &&
+   git -C submodule add sub &&
+   git -C submodule commit -m "added sub" &&
+   git add submodule &&
+   git commit -m "updated submodule" &&
+
+   cat >expect <<-\EOF &&
+   .gitmodules
+   a
+   b/b
+   submodule/.gitmodules
+   submodule/a
+   submodule/sub/a
+   EOF
+
+   git ls-files --recurse-submodules >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'ls-files using relative path' '
+