[PATCH] ls-files: properly prepare submodule environment

[PATCH] ls-files: properly prepare submodule environment

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

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 <jacob.keller@gmail.com>
---
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(&cp.env_array);
+	argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
+
 	if (prefix_len)
 		argv_array_pushf(&cp.env_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 000000000000..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' '
+	test_when_finished "rm -rf parent sub" &&
+	git init sub &&
+	echo "foobar" >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+
+	git init parent &&
+	echo "foobar" >parent/file &&
+	git -C parent add file &&
+	mkdir parent/src &&
+	echo "foobar" >parent/src/file2 &&
+	git -C parent add src/file2 &&
+	git -C parent submodule add ../sub &&
+	git -C parent commit -m "add files and submodule" &&
+
+	# From top works
+	cat >expect <<-\EOF &&
+	.gitmodules
+	file
+	src/file2
+	sub/file
+	EOF
+	git -C parent ls-files --recurse-submodules >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to top
+	cat >expect <<-\EOF &&
+	../.gitmodules
+	../file
+	file2
+	../sub/file
+	EOF
+	git -C parent/src ls-files --recurse-submodules .. >actual &&
+	test_cmp expect actual &&
+
+	# Relative path to submodule
+	cat >expect <<-\EOF &&
+	../sub/file
+	EOF
+	git -C parent/src ls-files --recurse-submodules ../sub >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'ls-files from a subdir' '
+	test_when_finished "rm -rf parent sub" &&
+	git init sub &&
+	echo "foobar" >sub/file &&
+	git -C sub add file &&
+	git -C sub commit -m "add file" &&
+
+	git init parent &&
+	mkdir parent/src &&
+	echo "foobar" >parent/src/file &&
+	git -C parent add src/file &&
+	git -C parent submodule add ../sub src/sub &&
+	git -C parent submodule add ../sub sub &&
+	git -C parent commit -m "add files and submodules" &&
+
+	# Verify grep from root works
+	cat >expect <<-\EOF &&
+	.gitmodules
+	src/file
+	src/sub/file
+	sub/file
+	EOF
+	git -C parent ls-files --recurse-submodules >actual &&
+	test_cmp expect actual &&
+
+	# Verify grep from a subdir works
+	cat >expect <<-\EOF &&
+	file
+	sub/file
+	EOF
+	git -C parent/src ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
+test_incompatible_with_recurse_submodules ()
+{
+	test_expect_success "--recurse-submodules and $1 are incompatible" "
+		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
+		test_i18ngrep -- '--recurse-submodules unsupported mode' actual
+	"
+}
+
+test_incompatible_with_recurse_submodules --deleted
+test_incompatible_with_recurse_submodules --others
+test_incompatible_with_recurse_submodules --unmerged
+test_incompatible_with_recurse_submodules --killed
+test_incompatible_with_recurse_submodules --modified
+
+test_done
-- 
2.12.2.776.gded3dc243c29.dirty

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

[Brandon Williams]

On 04/11, Jacob Keller wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
> 
> 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 <jacob.keller@gmail.com>
> ---
> 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(&cp.env_array);
> +	argv_array_push(&cp.env_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(&cp.env_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 000000000000..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' '
> +	test_when_finished "rm -rf parent sub" &&
> +	git init sub &&
> +	echo "foobar" >sub/file &&
> +	git -C sub add file &&
> +	git -C sub commit -m "add file" &&
> +
> +	git init parent &&
> +	echo "foobar" >parent/file &&
> +	git -C parent add file &&
> +	mkdir parent/src &&
> +	echo "foobar" >parent/src/file2 &&
> +	git -C parent add src/file2 &&
> +	git -C parent submodule add ../sub &&
> +	git -C parent commit -m "add files and submodule" &&
> +
> +	# From top works
> +	cat >expect <<-\EOF &&
> +	.gitmodules
> +	file
> +	src/file2
> +	sub/file
> +	EOF
> +	git -C parent ls-files --recurse-submodules >actual &&
> +	test_cmp expect actual &&
> +
> +	# Relative path to top
> +	cat >expect <<-\EOF &&
> +	../.gitmodules
> +	../file
> +	file2
> +	../sub/file
> +	EOF
> +	git -C parent/src ls-files --recurse-submodules .. >actual &&
> +	test_cmp expect actual &&
> +
> +	# Relative path to submodule
> +	cat >expect <<-\EOF &&
> +	../sub/file
> +	EOF
> +	git -C parent/src ls-files --recurse-submodules ../sub >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'ls-files from a subdir' '
> +	test_when_finished "rm -rf parent sub" &&
> +	git init sub &&
> +	echo "foobar" >sub/file &&
> +	git -C sub add file &&
> +	git -C sub commit -m "add file" &&
> +
> +	git init parent &&
> +	mkdir parent/src &&
> +	echo "foobar" >parent/src/file &&
> +	git -C parent add src/file &&
> +	git -C parent submodule add ../sub src/sub &&
> +	git -C parent submodule add ../sub sub &&
> +	git -C parent commit -m "add files and submodules" &&
> +
> +	# Verify grep from root works
> +	cat >expect <<-\EOF &&
> +	.gitmodules
> +	src/file
> +	src/sub/file
> +	sub/file
> +	EOF
> +	git -C parent ls-files --recurse-submodules >actual &&
> +	test_cmp expect actual &&
> +
> +	# Verify grep from a subdir works
> +	cat >expect <<-\EOF &&
> +	file
> +	sub/file
> +	EOF
> +	git -C parent/src ls-files --recurse-submodules >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_incompatible_with_recurse_submodules ()
> +{
> +	test_expect_success "--recurse-submodules and $1 are incompatible" "
> +		test_must_fail git ls-files --recurse-submodules $1 2>actual &&
> +		test_i18ngrep -- '--recurse-submodules unsupported mode' actual
> +	"
> +}
> +
> +test_incompatible_with_recurse_submodules --deleted
> +test_incompatible_with_recurse_submodules --others
> +test_incompatible_with_recurse_submodules --unmerged
> +test_incompatible_with_recurse_submodules --killed
> +test_incompatible_with_recurse_submodules --modified
> +
> +test_done
> -- 
> 2.12.2.776.gded3dc243c29.dirty
> 

-- 
Brandon Williams

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

[Jacob Keller]

On Wed, Apr 12, 2017 at 9:58 AM, Brandon Williams <bmwill@google.com> wrote:
> On 04/11, Jacob Keller wrote:
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> 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 <jacob.keller@gmail.com>
>> ---
>> 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(&cp.env_array);
>> +     argv_array_push(&cp.env_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(&cp.env_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 000000000000..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' '
>> +     test_when_finished "rm -rf parent sub" &&
>> +     git init sub &&
>> +     echo "foobar" >sub/file &&
>> +     git -C sub add file &&
>> +     git -C sub commit -m "add file" &&
>> +
>> +     git init parent &&
>> +     echo "foobar" >parent/file &&
>> +     git -C parent add file &&
>> +     mkdir parent/src &&
>> +     echo "foobar" >parent/src/file2 &&
>> +     git -C parent add src/file2 &&
>> +     git -C parent submodule add ../sub &&
>> +     git -C parent commit -m "add files and submodule" &&
>> +
>> +     # From top works
>> +     cat >expect <<-\EOF &&
>> +     .gitmodules
>> +     file
>> +     src/file2
>> +     sub/file
>> +     EOF
>> +     git -C parent ls-files --recurse-submodules >actual &&
>> +     test_cmp expect actual &&
>> +
>> +     # Relative path to top
>> +     cat >expect <<-\EOF &&
>> +     ../.gitmodules
>> +     ../file
>> +     file2
>> +     ../sub/file
>> +     EOF
>> +     git -C parent/src ls-files --recurse-submodules .. >actual &&
>> +     test_cmp expect actual &&
>> +
>> +     # Relative path to submodule
>> +     cat >expect <<-\EOF &&
>> +     ../sub/file
>> +     EOF
>> +     git -C parent/src ls-files --recurse-submodules ../sub >actual &&
>> +     test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'ls-files from a subdir' '
>> +     test_when_finished "rm -rf parent sub" &&
>> +     git init sub &&
>> +     echo "foobar" >sub/file &&
>> +     git -C sub add file &&
>> +     git -C sub commit -m "add file" &&
>> +
>> +     git init parent &&
>> +     mkdir parent/src &&
>> +     echo "foobar" >parent/src/file &&
>> +     git -C parent add src/file &&
>> +     git -C parent submodule add ../sub src/sub &&
>> +     git -C parent submodule add ../sub sub &&
>> +     git -C parent commit -m "add files and submodules" &&
>> +
>> +     # Verify grep from root works
>> +     cat >expect <<-\EOF &&
>> +     .gitmodules
>> +     src/file
>> +     src/sub/file
>> +     sub/file
>> +     EOF
>> +     git -C parent ls-files --recurse-submodules >actual &&
>> +     test_cmp expect actual &&
>> +
>> +     # Verify grep from a subdir works
>> +     cat >expect <<-\EOF &&
>> +     file
>> +     sub/file
>> +     EOF
>> +     git -C parent/src ls-files --recurse-submodules >actual &&
>> +     test_cmp expect actual
>> +'
>> +
>> +test_incompatible_with_recurse_submodules ()
>> +{
>> +     test_expect_success "--recurse-submodules and $1 are incompatible" "
>> +             test_must_fail git ls-files --recurse-submodules $1 2>actual &&
>> +             test_i18ngrep -- '--recurse-submodules unsupported mode' actual
>> +     "
>> +}
>> +
>> +test_incompatible_with_recurse_submodules --deleted
>> +test_incompatible_with_recurse_submodules --others
>> +test_incompatible_with_recurse_submodules --unmerged
>> +test_incompatible_with_recurse_submodules --killed
>> +test_incompatible_with_recurse_submodules --modified
>> +
>> +test_done
>> --
>> 2.12.2.776.gded3dc243c29.dirty
>>
>
> --
> Brandon Williams

[PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

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 fails for certain cases, including when nesting more
than one submodule, called from within a submodule that itself has
submodules, or when the GIT_DIR environemnt variable is set.

Prior to commit b58a68c1c187 ("setup: allow for prefix to be passed to
git commands", 2017-03-17) this resulted in an error indicating that
--prefix and --super-prefix were incompatible.

After this commit, instead, the process loops forever with a GIT_DIR set
to the parent and continuously reads the parent submodule files and
recursing forever.

Fix this by preparing the environment properly for submodules when
setting up the child process. This is similar to how other commands such
as grep behave.

This was not caught by the original tests because the scenario is
avoided if the submodules are created separately and not stored as the
standard method of putting the submodule git directory under
.git/modules/<name>. We can update the test to show the failure by the
addition of "git submodule absorbgitdirs" to the test case. However,
note that this new test would run forever without the necessary fix in
this patch.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
I updated and reworded the description so that the problem would be more
obvious. It doesn't occur always, but only when run nested with properly
absorbed gitdirs for the submodules. This explains the reason why the
test case had not caught the issue before.

 builtin/ls-files.c                     | 4 ++++
 t/t3007-ls-files-recurse-submodules.sh | 1 +
 2 files changed, 5 insertions(+)

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(&cp.env_array);
+	argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
+
 	if (prefix_len)
 		argv_array_pushf(&cp.env_array, "%s=%s",
 				 GIT_TOPLEVEL_PREFIX_ENVIRONMENT,
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index 4cf6ccf5a8ea..c8030dd3299a 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -77,6 +77,7 @@ test_expect_success 'ls-files recurses more than 1 level' '
 	git -C submodule/subsub commit -m "add d" &&
 	git -C submodule submodule add ./subsub &&
 	git -C submodule commit -m "added subsub" &&
+	git submodule absorbgitdirs &&
 	git ls-files --recurse-submodules >actual &&
 	test_cmp expect actual
 '
-- 
2.12.2.776.gded3dc243c29.dirty

[PATCH v2 2/2] ls-files: fix path used when recursing into submodules

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

Don't assume that the current working directory is the root of the
repository. Correctly generate the path for the recursing child
processes by building it from the work_tree() root instead. Otherwise if
we run ls-files using --git-dir or --work-tree it will not work
correctly as it attempts to change directory into a potentially invalid
location. Best case, it doesn't exist and we produce an error. Worst
case we cd into the wrong location and unknown behavior occurs.

Add a new test which highlights this possibility.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
I'm not sure that I'm convinced by this method of solving the problem as
I suspect it has some corner cases (what about when run inside a
subdirectory? It seems to work for me but I'm not sure...) Additionally,
it felt weird that there's no helper function for creating a toplevel
relative path.

 builtin/ls-files.c                     |  5 ++++-
 t/t3007-ls-files-recurse-submodules.sh | 11 +++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index e9b3546ca053..a6c70dbe9ec8 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -203,6 +203,7 @@ static void show_gitlink(const struct cache_entry *ce)
 {
 	struct child_process cp = CHILD_PROCESS_INIT;
 	int status;
+	char *dir;
 
 	prepare_submodule_repo_env(&cp.env_array);
 	argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
@@ -221,8 +222,10 @@ static void show_gitlink(const struct cache_entry *ce)
 	argv_array_pushv(&cp.args, submodule_options.argv);
 
 	cp.git_cmd = 1;
-	cp.dir = ce->name;
+	dir = mkpathdup("%s/%s", get_git_work_tree(), ce->name);
+	cp.dir = dir;
 	status = run_command(&cp);
+	free(dir);
 	if (status)
 		exit(status);
 }
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index c8030dd3299a..ebb956fd16cc 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -82,6 +82,17 @@ test_expect_success 'ls-files recurses more than 1 level' '
 	test_cmp expect actual
 '
 
+test_expect_success 'ls-files works with GIT_DIR' '
+	cat >expect <<-\EOF &&
+	.gitmodules
+	c
+	subsub/d
+	EOF
+
+	git --git-dir=submodule/.git ls-files --recurse-submodules >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success '--recurse-submodules and pathspecs setup' '
 	echo e >submodule/subsub/e.txt &&
 	git -C submodule/subsub add e.txt &&
-- 
2.12.2.776.gded3dc243c29.dirty

Re: [PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules

[Brandon Williams]

On 04/13, Jacob Keller wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
> 
> 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 fails for certain cases, including when nesting more
> than one submodule, called from within a submodule that itself has
> submodules, or when the GIT_DIR environemnt variable is set.
> 
> Prior to commit b58a68c1c187 ("setup: allow for prefix to be passed to
> git commands", 2017-03-17) this resulted in an error indicating that
> --prefix and --super-prefix were incompatible.
> 
> After this commit, instead, the process loops forever with a GIT_DIR set
> to the parent and continuously reads the parent submodule files and
> recursing forever.
> 
> Fix this by preparing the environment properly for submodules when
> setting up the child process. This is similar to how other commands such
> as grep behave.
> 
> This was not caught by the original tests because the scenario is
> avoided if the submodules are created separately and not stored as the
> standard method of putting the submodule git directory under
> .git/modules/<name>. We can update the test to show the failure by the
> addition of "git submodule absorbgitdirs" to the test case. However,
> note that this new test would run forever without the necessary fix in
> this patch.
> 
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>

This looks good to me.  Thanks again for catching this.  Dealing with
submodules definitely isn't easy (I seem to have made a lot of mistakes
that have been cropping up recently)...it would be easier if we didn't
have to spin out a process for each submodule but that's not the world
we live in today :)

> ---
> I updated and reworded the description so that the problem would be more
> obvious. It doesn't occur always, but only when run nested with properly
> absorbed gitdirs for the submodules. This explains the reason why the
> test case had not caught the issue before.
> 
>  builtin/ls-files.c                     | 4 ++++
>  t/t3007-ls-files-recurse-submodules.sh | 1 +
>  2 files changed, 5 insertions(+)
> 
> 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(&cp.env_array);
> +	argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
> +
>  	if (prefix_len)
>  		argv_array_pushf(&cp.env_array, "%s=%s",
>  				 GIT_TOPLEVEL_PREFIX_ENVIRONMENT,
> diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
> index 4cf6ccf5a8ea..c8030dd3299a 100755
> --- a/t/t3007-ls-files-recurse-submodules.sh
> +++ b/t/t3007-ls-files-recurse-submodules.sh
> @@ -77,6 +77,7 @@ test_expect_success 'ls-files recurses more than 1 level' '
>  	git -C submodule/subsub commit -m "add d" &&
>  	git -C submodule submodule add ./subsub &&
>  	git -C submodule commit -m "added subsub" &&
> +	git submodule absorbgitdirs &&
>  	git ls-files --recurse-submodules >actual &&
>  	test_cmp expect actual
>  '
> -- 
> 2.12.2.776.gded3dc243c29.dirty
> 

-- 
Brandon Williams

Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules

[Brandon Williams]

On 04/13, Jacob Keller wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
> 
> Don't assume that the current working directory is the root of the
> repository. Correctly generate the path for the recursing child
> processes by building it from the work_tree() root instead. Otherwise if
> we run ls-files using --git-dir or --work-tree it will not work
> correctly as it attempts to change directory into a potentially invalid
> location. Best case, it doesn't exist and we produce an error. Worst
> case we cd into the wrong location and unknown behavior occurs.
> 
> Add a new test which highlights this possibility.
> 
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> I'm not sure that I'm convinced by this method of solving the problem as
> I suspect it has some corner cases (what about when run inside a
> subdirectory? It seems to work for me but I'm not sure...) Additionally,
> it felt weird that there's no helper function for creating a toplevel
> relative path.

I never considered the case where you use --git-dir or --work-tree,
definitely an oversight on my part.  This change seems reasonable to me.

-- 
Brandon Williams

Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules

[Stefan Beller]

On Thu, Apr 13, 2017 at 10:12 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
> From: Jacob Keller <jacob.keller@gmail.com>
>
> Don't assume that the current working directory is the root of the
> repository.

1)  Oh! This bug might be hidden in other commands, too.
($ git grep cp.dir -- submodule.c)

2) But why?
Isn't that what most of setup.c is all about ? (discovery of the root of the
repository, staying there, and invoking the correct subcommand with a prefix)

> Correctly generate the path for the recursing child
> processes by building it from the work_tree() root instead. Otherwise if
> we run ls-files using --git-dir or --work-tree it will not work
> correctly as it attempts to change directory into a potentially invalid
> location.

Oh, I see. In that case the setup doesn't cd into the worktree.

> Best case, it doesn't exist and we produce an error. Worst
> case we cd into the wrong location and unknown behavior occurs.
>
> Add a new test which highlights this possibility.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> I'm not sure that I'm convinced by this method of solving the problem as
> I suspect it has some corner cases (what about when run inside a
> subdirectory? It seems to work for me but I'm not sure...) Additionally,
> it felt weird that there's no helper function for creating a toplevel
> relative path.

Do we want to run ls-files from the working tree or from the git dir?
For the git dir there would be git_pathdup_submodule.

We could introduce
  const char *get_submodule_work_tree(const char *submodule_path);
as a wrapper around
  mkpathdup("%s/%s", get_git_work_tree(), ce->name);

Code and test look fine in this patch,

Thanks,
Stefan

Re: [PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules

[Jacob Keller]

On Thu, Apr 13, 2017 at 11:03 AM, Brandon Williams <bmwill@google.com> wrote:
> On 04/13, Jacob Keller wrote:
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> 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 fails for certain cases, including when nesting more
>> than one submodule, called from within a submodule that itself has
>> submodules, or when the GIT_DIR environemnt variable is set.
>>
>> Prior to commit b58a68c1c187 ("setup: allow for prefix to be passed to
>> git commands", 2017-03-17) this resulted in an error indicating that
>> --prefix and --super-prefix were incompatible.
>>
>> After this commit, instead, the process loops forever with a GIT_DIR set
>> to the parent and continuously reads the parent submodule files and
>> recursing forever.
>>
>> Fix this by preparing the environment properly for submodules when
>> setting up the child process. This is similar to how other commands such
>> as grep behave.
>>
>> This was not caught by the original tests because the scenario is
>> avoided if the submodules are created separately and not stored as the
>> standard method of putting the submodule git directory under
>> .git/modules/<name>. We can update the test to show the failure by the
>> addition of "git submodule absorbgitdirs" to the test case. However,
>> note that this new test would run forever without the necessary fix in
>> this patch.
>>
>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
>
> This looks good to me.  Thanks again for catching this.  Dealing with
> submodules definitely isn't easy (I seem to have made a lot of mistakes
> that have been cropping up recently)...it would be easier if we didn't
> have to spin out a process for each submodule but that's not the world
> we live in today :)
>

Spinning out a process is one of the big downsides of working with
submodules in our code. Unfortunately, spinning out a process is also
one of the biggest ways we isolate submodules, and if we wanted to do
this "in-process" we would need to add an abstraction layer that lets
us handle submodules in-process in some clean way.

Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules

[Jacob Keller]

On Thu, Apr 13, 2017 at 11:15 AM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Apr 13, 2017 at 10:12 AM, Jacob Keller <jacob.e.keller@intel.com> wrote:
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Don't assume that the current working directory is the root of the
>> repository.
>
> 1)  Oh! This bug might be hidden in other commands, too.
> ($ git grep cp.dir -- submodule.c)
>

Almost certainly. I'm not sure how best to audit this.

> 2) But why?
> Isn't that what most of setup.c is all about ? (discovery of the root of the
> repository, staying there, and invoking the correct subcommand with a prefix)
>
>> Correctly generate the path for the recursing child
>> processes by building it from the work_tree() root instead. Otherwise if
>> we run ls-files using --git-dir or --work-tree it will not work
>> correctly as it attempts to change directory into a potentially invalid
>> location.
>
> Oh, I see. In that case the setup doesn't cd into the worktree.
>

Yea we aren't in the worktree when we thought we were.

>> Best case, it doesn't exist and we produce an error. Worst
>> case we cd into the wrong location and unknown behavior occurs.
>>
>> Add a new test which highlights this possibility.
>>
>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
>> ---
>> I'm not sure that I'm convinced by this method of solving the problem as
>> I suspect it has some corner cases (what about when run inside a
>> subdirectory? It seems to work for me but I'm not sure...) Additionally,
>> it felt weird that there's no helper function for creating a toplevel
>> relative path.
>
> Do we want to run ls-files from the working tree or from the git dir?
> For the git dir there would be git_pathdup_submodule.
>

Well prior to this code we're assuming we are in the worktree. I
wasn't actually certain if we should run from within the gitdir or the
worktree. Probably we actually want to be in the gitdir so that we can
work even if we're not checked out. Additionally, we probably need to
protect this check with a "is_submodule_initialized" unless ls-files
somehow ignores the submodule already in this case.. it didn't look
like it at a glance.

> We could introduce
>   const char *get_submodule_work_tree(const char *submodule_path);
> as a wrapper around
>   mkpathdup("%s/%s", get_git_work_tree(), ce->name);
>
> Code and test look fine in this patch,
>

Yea adding a helper function seems like a good thing.

Thanks,
Jake

> Thanks,
> Stefan

Re: [PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules

[Stefan Beller]

On Thu, Apr 13, 2017 at 11:31 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> Spinning out a process is one of the big downsides of working with
> submodules in our code. Unfortunately, spinning out a process is also
> one of the biggest ways we isolate submodules, and if we wanted to do
> this "in-process" we would need to add an abstraction layer that lets
> us handle submodules in-process in some clean way.

Yeah if we had less globals and a repo struct (class) which we could
operate on, that would be bring in the abstractions that we'd need.

Stefan

Re: [PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules

[Brandon Williams]

On 04/13, Stefan Beller wrote:
> On Thu, Apr 13, 2017 at 11:31 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
> > Spinning out a process is one of the big downsides of working with
> > submodules in our code. Unfortunately, spinning out a process is also
> > one of the biggest ways we isolate submodules, and if we wanted to do
> > this "in-process" we would need to add an abstraction layer that lets
> > us handle submodules in-process in some clean way.
> 
> Yeah if we had less globals and a repo struct (class) which we could
> operate on, that would be bring in the abstractions that we'd need.

Agreed, though we're probably pretty far from that becoming a reality.
One day though!

-- 
Brandon Williams

[PATCH 3/2] ls-files: only recurse on active submodules

[Brandon Williams]

Add in a check to see if a submodule is active before attempting to
recurse.  This prevents 'ls-files' from trying to operate on a submodule
which may not exist in the working directory.

Signed-off-by: Brandon Williams <bmwill@google.com>
---

After you mentioned possibly needing to check if a submodule is initialized, I
went back and noticed that there was indeed no check for it...So this patch
adds in the necessary check to see if a submodule is active or not before
attempting to recurse.

 builtin/ls-files.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index d449e46db..10ddc0306 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;
@@ -235,7 +236,8 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
 	if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
-	    submodule_path_match(&pathspec, name.buf, ps_matched)) {
+	    submodule_path_match(&pathspec, name.buf, ps_matched) &&
+	    is_submodule_initialized(ce->name)) {
 		show_gitlink(ce);
 	} else if (match_pathspec(&pathspec, name.buf, name.len,
 				  len, ps_matched,
@@ -604,8 +606,10 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
-	if (recurse_submodules)
+	if (recurse_submodules) {
+		gitmodules_config();
 		compile_submodule_options(argv, &dir, show_tag);
+	}
 
 	if (recurse_submodules &&
 	    (show_stage || show_deleted || show_others || show_unmerged ||
-- 
2.12.2.762.g0e3151a226-goog

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Stefan Beller]

On Thu, Apr 13, 2017 at 11:57 AM, Brandon Williams <bmwill@google.com> wrote:
> Add in a check to see if a submodule is active before attempting to
> recurse.  This prevents 'ls-files' from trying to operate on a submodule
> which may not exist in the working directory.

What would currently happen on recursing into non-active submodules?
Can we have a test for this?

Thanks,
Stefan

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Jacob Keller]

On Thu, Apr 13, 2017 at 12:05 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Apr 13, 2017 at 11:57 AM, Brandon Williams <bmwill@google.com> wrote:
>> Add in a check to see if a submodule is active before attempting to
>> recurse.  This prevents 'ls-files' from trying to operate on a submodule
>> which may not exist in the working directory.
>
> What would currently happen on recursing into non-active submodules?
> Can we have a test for this?
>
> Thanks,
> Stefan

We should be able to test for this. Is it possible that we can recurse
into a submodule as long as we have the clone in .git/modules/<name>
even if we don't have it checked out currently?

Thanks,
Jake

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Stefan Beller]

On Thu, Apr 13, 2017 at 12:12 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
> On Thu, Apr 13, 2017 at 12:05 PM, Stefan Beller <sbeller@google.com> wrote:
>> On Thu, Apr 13, 2017 at 11:57 AM, Brandon Williams <bmwill@google.com> wrote:
>>> Add in a check to see if a submodule is active before attempting to
>>> recurse.  This prevents 'ls-files' from trying to operate on a submodule
>>> which may not exist in the working directory.
>>
>> What would currently happen on recursing into non-active submodules?
>> Can we have a test for this?
>>
>> Thanks,
>> Stefan
>
> We should be able to test for this. Is it possible that we can recurse
> into a submodule as long as we have the clone in .git/modules/<name>
> even if we don't have it checked out currently?

Conceptually that should be possible, e.g.

    git ls-files --recurse-submodules <ancient ref>

where the ancient ref contained submodules that are not present any more.
In that case we would need to do

    struct strbuf sb = STRBUF_INIT;
    struct child_process = CHILD_PROCESS_INIT;
    struct submodule *sub = submodule_from_path( \
        <path as recorded in ancient tree>, <ancient ref>)
    strbuf_git_path(&sb, "modules/%s", sub->name);

    argv_array_pushl(&cp.args, "git", "ls-files", "--recurse", ...);
    cp.dir = sb.buf;
    run_command(&cp);

Stefan

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Jacob Keller]

On Thu, Apr 13, 2017 at 12:25 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Apr 13, 2017 at 12:12 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Thu, Apr 13, 2017 at 12:05 PM, Stefan Beller <sbeller@google.com> wrote:
>>> On Thu, Apr 13, 2017 at 11:57 AM, Brandon Williams <bmwill@google.com> wrote:
>>>> Add in a check to see if a submodule is active before attempting to
>>>> recurse.  This prevents 'ls-files' from trying to operate on a submodule
>>>> which may not exist in the working directory.
>>>
>>> What would currently happen on recursing into non-active submodules?
>>> Can we have a test for this?
>>>
>>> Thanks,
>>> Stefan
>>
>> We should be able to test for this. Is it possible that we can recurse
>> into a submodule as long as we have the clone in .git/modules/<name>
>> even if we don't have it checked out currently?
>
> Conceptually that should be possible, e.g.
>
>     git ls-files --recurse-submodules <ancient ref>
>
> where the ancient ref contained submodules that are not present any more.
> In that case we would need to do
>
>     struct strbuf sb = STRBUF_INIT;
>     struct child_process = CHILD_PROCESS_INIT;
>     struct submodule *sub = submodule_from_path( \
>         <path as recorded in ancient tree>, <ancient ref>)
>     strbuf_git_path(&sb, "modules/%s", sub->name);
>
>     argv_array_pushl(&cp.args, "git", "ls-files", "--recurse", ...);
>     cp.dir = sb.buf;
>     run_command(&cp);
>
> Stefan

This is probably the right flow to use then.

Thanks,
Jake

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Jacob Keller]

On Thu, Apr 13, 2017 at 12:25 PM, Stefan Beller <sbeller@google.com> wrote:
> On Thu, Apr 13, 2017 at 12:12 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
>> On Thu, Apr 13, 2017 at 12:05 PM, Stefan Beller <sbeller@google.com> wrote:
>>> On Thu, Apr 13, 2017 at 11:57 AM, Brandon Williams <bmwill@google.com> wrote:
>>>> Add in a check to see if a submodule is active before attempting to
>>>> recurse.  This prevents 'ls-files' from trying to operate on a submodule
>>>> which may not exist in the working directory.
>>>
>>> What would currently happen on recursing into non-active submodules?
>>> Can we have a test for this?
>>>
>>> Thanks,
>>> Stefan
>>
>> We should be able to test for this. Is it possible that we can recurse
>> into a submodule as long as we have the clone in .git/modules/<name>
>> even if we don't have it checked out currently?
>
> Conceptually that should be possible, e.g.
>
>     git ls-files --recurse-submodules <ancient ref>
>
> where the ancient ref contained submodules that are not present any more.
> In that case we would need to do
>
>     struct strbuf sb = STRBUF_INIT;
>     struct child_process = CHILD_PROCESS_INIT;
>     struct submodule *sub = submodule_from_path( \
>         <path as recorded in ancient tree>, <ancient ref>)
>     strbuf_git_path(&sb, "modules/%s", sub->name);
>
>     argv_array_pushl(&cp.args, "git", "ls-files", "--recurse", ...);
>     cp.dir = sb.buf;
>     run_command(&cp);
>
> Stefan

Never mind. git ls-files doesn't support showing files for a specific
ancient history. (I guess you'd use ls-tree for that?). I'm guessing
we want to run in the actual work-tree for ls-files here.

Does "is_submodule_initialized()" going to ensure that we only operate
on a submodule that's currently checked out?

Thanks,
Jake

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Stefan Beller]

On Fri, Apr 14, 2017 at 9:33 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>
> Never mind. git ls-files doesn't support showing files for a specific
> ancient history. (I guess you'd use ls-tree for that?). I'm guessing
> we want to run in the actual work-tree for ls-files here.
>
> Does "is_submodule_initialized()" going to ensure that we only operate
> on a submodule that's currently checked out?

I think for that we rather want to use is_submodule_populated.
And I think it would make sense as well to check for that instead
of the initialized state.

Thanks,
Stefan

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Jacob Keller]

On Fri, Apr 14, 2017 at 10:02 AM, Stefan Beller <sbeller@google.com> wrote:
> On Fri, Apr 14, 2017 at 9:33 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>>
>> Never mind. git ls-files doesn't support showing files for a specific
>> ancient history. (I guess you'd use ls-tree for that?). I'm guessing
>> we want to run in the actual work-tree for ls-files here.
>>
>> Does "is_submodule_initialized()" going to ensure that we only operate
>> on a submodule that's currently checked out?
>
> I think for that we rather want to use is_submodule_populated.
> And I think it would make sense as well to check for that instead
> of the initialized state.
>
> Thanks,
> Stefan

Right ok.

Thanks,
Jake

Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> From: Jacob Keller <jacob.keller@gmail.com>
>
> Don't assume that the current working directory is the root of the
> repository. Correctly generate the path for the recursing child
> processes by building it from the work_tree() root instead. Otherwise if
> we run ls-files using --git-dir or --work-tree it will not work
> correctly as it attempts to change directory into a potentially invalid
> location. Best case, it doesn't exist and we produce an error. Worst
> case we cd into the wrong location and unknown behavior occurs.
>
> Add a new test which highlights this possibility.
>
> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
> ---
> I'm not sure that I'm convinced by this method of solving the problem as
> I suspect it has some corner cases (what about when run inside a
> subdirectory? It seems to work for me but I'm not sure...) Additionally,
> it felt weird that there's no helper function for creating a toplevel
> relative path.

Is this a similar issue as discussed in a nearby thread e.g.

  <CACsJy8CLBY22j3EjR4PW3n+K6PWUzb-HCgxTVeCGpwtApZF-6g@mail.gmail.com>

I do think it is a bug that sometimes we do not go to the root of
the working tree when we know the repository is not a bare one.

Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules

[Jacob Keller]

On Mon, Apr 17, 2017 at 7:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Don't assume that the current working directory is the root of the
>> repository. Correctly generate the path for the recursing child
>> processes by building it from the work_tree() root instead. Otherwise if
>> we run ls-files using --git-dir or --work-tree it will not work
>> correctly as it attempts to change directory into a potentially invalid
>> location. Best case, it doesn't exist and we produce an error. Worst
>> case we cd into the wrong location and unknown behavior occurs.
>>
>> Add a new test which highlights this possibility.
>>
>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
>> ---
>> I'm not sure that I'm convinced by this method of solving the problem as
>> I suspect it has some corner cases (what about when run inside a
>> subdirectory? It seems to work for me but I'm not sure...) Additionally,
>> it felt weird that there's no helper function for creating a toplevel
>> relative path.
>
> Is this a similar issue as discussed in a nearby thread e.g.
>
>   <CACsJy8CLBY22j3EjR4PW3n+K6PWUzb-HCgxTVeCGpwtApZF-6g@mail.gmail.com>
>
> I do think it is a bug that sometimes we do not go to the root of
> the working tree when we know the repository is not a bare one.

Yes and no. I think that this would be a problem in both a bare and
non-bare repo ? I think the correct thing to do here is really to do
what we proposed, and properly lookup the full file name.

I do think it makes the most sense conceptually to always cd into the
top level directory of a non-bare repo though.

Thanks,
Jake

Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules

[Stefan Beller]

On Mon, Apr 17, 2017 at 7:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Don't assume that the current working directory is the root of the
>> repository. Correctly generate the path for the recursing child
>> processes by building it from the work_tree() root instead. Otherwise if
>> we run ls-files using --git-dir or --work-tree it will not work
>> correctly as it attempts to change directory into a potentially invalid
>> location. Best case, it doesn't exist and we produce an error. Worst
>> case we cd into the wrong location and unknown behavior occurs.
>>
>> Add a new test which highlights this possibility.
>>
>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
>> ---
>> I'm not sure that I'm convinced by this method of solving the problem as
>> I suspect it has some corner cases (what about when run inside a
>> subdirectory? It seems to work for me but I'm not sure...) Additionally,
>> it felt weird that there's no helper function for creating a toplevel
>> relative path.
>
> Is this a similar issue as discussed in a nearby thread e.g.
>
>   <CACsJy8CLBY22j3EjR4PW3n+K6PWUzb-HCgxTVeCGpwtApZF-6g@mail.gmail.com>

yes, it's the same broader issue.

>
> I do think it is a bug that sometimes we do not go to the root of
> the working tree when we know the repository is not a bare one.

ok, that would have been my understanding as well.

That nearby thread however highlights how it may be a user breaking if
we just fix the paths. The patch here doesn't cd to the root of the repo,
but constructs the correct path, which is correct. It may be considered
clumsy if we decide that cd'ing to the root of the repo is the correct bug fix
for the underlying issue.

Thanks,
Stefan

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> Add in a check to see if a submodule is active before attempting to
> recurse.  This prevents 'ls-files' from trying to operate on a submodule
> which may not exist in the working directory.
>
> Signed-off-by: Brandon Williams <bmwill@google.com>
> ---

Sounds like this is something we can demonstrate its effect with a
simple test in t/ directory?

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[paul]

> ...

I'm nowhere near up to speed with the entire history of this chain so
please excuse me if I've missed some detail somewhere.

As of b06d3643105c8758ed019125a4399cb7efdcce2c, the following command
"hangs" at near 100% CPU in a repo of mine:

git ls-files --recurse-submodules

The "hang" appears to be an explosion of subprocesses (snipped the
COMMAND detail from ps):

git ls-files --recurse-submodules
git --super-prefix=g/_vendor/src/github.com/ramya-rao-a/go-outline/ ls-files --recurse-submodules --
git --super-prefix=g/_vendor/src/github.com/ramya-rao-a/go-outline/g/_vendor/src/github.com/ramya-rao-a/go-outline/ ls-files --recurse-submodules --
....

In v2.11.0 I simply get a fatal error (this repo doesn't actually
contain any submodules):

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

I haven't yet been able to work out what's special about the
subdirectory g/_vendor/src/github.com/ramya-rao-a/go-outline/

Is this expected? (I'm guessing not)

How can I help diagnose what's going on here?

Thanks

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Brandon Williams]

On 05/12, paul@myitcv.io wrote:
> > ...
> 
> I'm nowhere near up to speed with the entire history of this chain so
> please excuse me if I've missed some detail somewhere.
> 
> As of b06d3643105c8758ed019125a4399cb7efdcce2c, the following command
> "hangs" at near 100% CPU in a repo of mine:
> 
> git ls-files --recurse-submodules
> 
> The "hang" appears to be an explosion of subprocesses (snipped the
> COMMAND detail from ps):
> 
> git ls-files --recurse-submodules
> git --super-prefix=g/_vendor/src/github.com/ramya-rao-a/go-outline/ ls-files --recurse-submodules --
> git --super-prefix=g/_vendor/src/github.com/ramya-rao-a/go-outline/g/_vendor/src/github.com/ramya-rao-a/go-outline/ ls-files --recurse-submodules --
> ....

Yeah the way the recursion works as of 2.13 is that a new process is
spawned for each submodule.  This really is kind of clunky and I'd like
to move to an implementation which doesn't require spawning a ton of
processes, so that's a goal of mine I'm trying to work towards.  This is
still a relatively new feature so I expect there's still a lot of bugs to
iron out.

> 
> In v2.11.0 I simply get a fatal error (this repo doesn't actually
> contain any submodules):
> 
> fatal: can't use --super-prefix from a subdirectory
> 
> I haven't yet been able to work out what's special about the
> subdirectory g/_vendor/src/github.com/ramya-rao-a/go-outline/
> 
> Is this expected? (I'm guessing not)
> 
> How can I help diagnose what's going on here?

Looking at this it appears to me that you have a submodule loop, either
directly or indirectly.  That is if you have a repo 'a' which contains a
submodule 'b' and repo 'b' happens to contain 'a' as a submodule you'd
find yourself with an infinite loop.  But that's what I'm guessing
based on that output you've shown me.

But you're saying that this repo doesn't actually contain submodules?
Hmm then I'm at a loss...unless!

The directory 'go-outline' is a place holder for a submodule so ls-files
spawns a process in that directory. But since its not populated, during
the gitdir discovery it falls back to the superproject's gitdir,
restarting the whole process and creating an infinite loop...

Welp that's a pretty terrible bug which stems from
missing a check to see if a submodule is initialized, and not explicitly
setting GIT_DIR=.git (theres cases where we don't want this but turns
out we probably should do that here).  Let me know if this fixes the
problem:


diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index a6c70dbe9..b0796fc10 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -206,7 +206,6 @@ static void show_gitlink(const struct cache_entry *ce)
 	char *dir;
 
 	prepare_submodule_repo_env(&cp.env_array);
-	argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
 
 	if (prefix_len)
 		argv_array_pushf(&cp.env_array, "%s=%s",
@@ -242,6 +241,7 @@ static void show_ce_entry(const char *tag, const struct cache_entry *ce)
 		die("git ls-files: internal error - cache entry not superset of prefix");
 
 	if (recurse_submodules && S_ISGITLINK(ce->ce_mode) &&
+	    is_submodule_initialized(ce->name) &&
 	    submodule_path_match(&pathspec, name.buf, ps_matched)) {
 		show_gitlink(ce);
 	} else if (match_pathspec(&pathspec, name.buf, name.len,
@@ -611,8 +611,10 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
 	if (require_work_tree && !is_inside_work_tree())
 		setup_work_tree();
 
-	if (recurse_submodules)
+	if (recurse_submodules) {
+		gitmodules_config();
 		compile_submodule_options(argv, &dir, show_tag);
+	}
 
 	if (recurse_submodules &&
 	    (show_stage || show_deleted || show_others || show_unmerged ||

-- 
Brandon Williams

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Paul Jolly]

>> How can I help diagnose what's going on here?

<snip>

> Welp that's a pretty terrible bug which stems from
> missing a check to see if a submodule is initialized, and not explicitly
> setting GIT_DIR=.git (theres cases where we don't want this but turns
> out we probably should do that here).  Let me know if this fixes the
> problem:

That's certainly fixed the fork explosion. Indeed seems to now work as
expected from my perspective...

And also answers the question that I was going to ask, namely whether
git ls-files --recurse-submodules should work on repos that do not contain
any submodules... to which I'd hoped the answer was "yes" (because as I
said it was a fatal error in v2.11.x, despite there being some output). The
behaviour I'm now seeing appears to affirm that, assuming it's as expected
from your perspective!

Thanks for the quick response.

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Brandon Williams]

On 05/12, Paul Jolly wrote:
> >> How can I help diagnose what's going on here?
> 
> <snip>
> 
> > Welp that's a pretty terrible bug which stems from
> > missing a check to see if a submodule is initialized, and not explicitly
> > setting GIT_DIR=.git (theres cases where we don't want this but turns
> > out we probably should do that here).  Let me know if this fixes the
> > problem:
> 
> That's certainly fixed the fork explosion. Indeed seems to now work as
> expected from my perspective...
> 
> And also answers the question that I was going to ask, namely whether
> git ls-files --recurse-submodules should work on repos that do not contain
> any submodules... to which I'd hoped the answer was "yes" (because as I
> said it was a fatal error in v2.11.x, despite there being some output). The
> behaviour I'm now seeing appears to affirm that, assuming it's as expected
> from your perspective!

Yep it should definitely work on repos with no submodules.  I'll put
this on my list of things to do so that I can come up with a more robust
solution to the problem (with tests).

> 
> Thanks for the quick response.

Course, let me know if you find anything else! :D

-- 
Brandon Williams

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Paul Jolly]

>> Thanks for the quick response.
>
> Course, let me know if you find anything else! :D

Brandon - doesn't look like this change has made it's way into master:

https://github.com/git/git/blob/master/builtin/ls-files.c

Is there a plan to merge it?

Thanks

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Brandon Williams]

On 08/01, Paul Jolly wrote:
> >
> > > Thanks for the quick response.
> >
> > Course, let me know if you find anything else! :D
> >
> 
> Brandon - doesn't look like this change has made it's way into master:
> 
> https://github.com/git/git/blob/master/builtin/ls-files.c
> 
> Is there a plan to merge it?
> 
> Thanks

It looks like it was merged to master.  This should be the relevant
commit: 188dce131 (ls-files: use repository object, 2017-06-22).

-- 
Brandon Williams

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Paul Jolly]

> It looks like it was merged to master.  This should be the relevant
> commit: 188dce131 (ls-files: use repository object, 2017-06-22).

I was just typing a response to my response. Apologies, I was testing
locally with the wrong compiled version of git.

Confirmed fixed for me in e2d9c4613 at least.

Thanks again

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Brandon Williams]

On 08/01, Paul Jolly wrote:
> > It looks like it was merged to master.  This should be the relevant
> > commit: 188dce131 (ls-files: use repository object, 2017-06-22).
> 
> I was just typing a response to my response. Apologies, I was testing
> locally with the wrong compiled version of git.
> 
> Confirmed fixed for me in e2d9c4613 at least.

Perfect!  Glad to hear that it fixed the problem.

-- 
Brandon Williams

Re: [PATCH 3/2] ls-files: only recurse on active submodules

[Junio C Hamano]

Brandon Williams <bmwill@google.com> writes:

> On 08/01, Paul Jolly wrote:
>> > It looks like it was merged to master.  This should be the relevant
>> > commit: 188dce131 (ls-files: use repository object, 2017-06-22).
>> 
>> I was just typing a response to my response. Apologies, I was testing
>> locally with the wrong compiled version of git.
>> 
>> Confirmed fixed for me in e2d9c4613 at least.
>
> Perfect!  Glad to hear that it fixed the problem.

Thanks, both, for tying potentially loose ends.