threads / patch / 45672

patchls-files: properly prepare submodule environment

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

## tl;dr

33 messages between Apr 12, 2017 and Aug 1, 2017. Diffs are folded; open one to read it.

replies: 32people: 6as markdown or json

Jacob Keller· Apr 12, 2017, 00:39 UTC · lore
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
Show changes to 2 files +166 −0

builtin/ls-files.c, 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
Brandon Williams· Apr 12, 2017, 16:58 UTC · re: Jacob Keller · lore

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

On 04/11, Jacob Keller wrote:
Show 36 quoted lines
> 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.

Show 9 quoted lines
> 
> 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).

Show 24 quoted lines
> 
>  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.

Show 175 quoted lines
> +
>  	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
Jacob Keller· Apr 12, 2017, 22:45 UTC · re: Brandon Williams · lore

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

On Wed, Apr 12, 2017 at 9:58 AM, Brandon Williams <bmwill@google.com> wrote:
Show 44 quoted lines
> 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?

Show 16 quoted lines
>
>>
>> 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

Show 208 quoted lines
>
>
>>
>>  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
Jacob Keller· Apr 13, 2017, 17:12 UTC · re: Jacob Keller · lore

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

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(+)
Show changes to 2 files +5 −0

builtin/ls-files.c, t/t3007-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/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
Jacob Keller· Apr 13, 2017, 17:12 UTC · re: Jacob Keller · lore

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

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(-)
Show changes to 2 files +15 −1

builtin/ls-files.c, t/t3007-ls-files-recurse-submodules.sh

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
Brandon Williams· Apr 13, 2017, 18:10 UTC · re: Jacob Keller · lore

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

On 04/13, Jacob Keller wrote:
Show 19 quoted lines
> 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
Stefan Beller· Apr 13, 2017, 18:15 UTC · re: Jacob Keller · lore

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

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)
Show 5 quoted lines
> 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.
Show 12 quoted lines
> 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

Jacob Keller· Apr 13, 2017, 18:34 UTC · re: Stefan Beller · lore

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

On Thu, Apr 13, 2017 at 11:15 AM, Stefan Beller <sbeller@google.com> wrote:
Show 9 quoted lines
> 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.
Show 12 quoted lines
> 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.
Show 16 quoted lines
>> 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.

Show 7 quoted lines
> 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
Junio C Hamano· Apr 18, 2017, 02:03 UTC · re: Jacob Keller · lore

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

Jacob Keller <jacob.e.keller@intel.com> writes:
Show 19 quoted lines
> 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.

Jacob Keller· Apr 18, 2017, 07:42 UTC · re: Junio C Hamano · lore

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

On Mon, Apr 17, 2017 at 7:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 28 quoted lines
> 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

Stefan Beller· Apr 18, 2017, 18:41 UTC · re: Junio C Hamano · lore

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

On Mon, Apr 17, 2017 at 7:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 25 quoted lines
> 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

Brandon Williams· Apr 13, 2017, 18:03 UTC · re: Jacob Keller · lore

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

On 04/13, Jacob Keller wrote:
Show 31 quoted lines
> 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 :)

Show 47 quoted lines
> ---
> 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
Jacob Keller· Apr 13, 2017, 18:31 UTC · re: Brandon Williams · lore

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

On Thu, Apr 13, 2017 at 11:03 AM, Brandon Williams <bmwill@google.com> wrote:
Show 39 quoted lines
> 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.

Stefan Beller· Apr 13, 2017, 18:35 UTC · re: Jacob Keller · lore

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

On Thu, Apr 13, 2017 at 11:31 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
Show 5 quoted lines
> 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
Brandon Williams· Apr 13, 2017, 18:36 UTC · re: Stefan Beller · lore

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

On 04/13, Stefan Beller wrote:
Show 9 quoted lines
> 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
Brandon Williams· Apr 13, 2017, 18:57 UTC · re: Jacob Keller · lore

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

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(-)
Show changes to builtin/ls-files.c +6 −2
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
Stefan Beller· Apr 13, 2017, 19:05 UTC · re: Brandon Williams · lore

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

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

Jacob Keller· Apr 13, 2017, 19:12 UTC · re: Stefan Beller · lore

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

On Thu, Apr 13, 2017 at 12:05 PM, Stefan Beller <sbeller@google.com> wrote:
Show 10 quoted lines
> 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

Stefan Beller· Apr 13, 2017, 19:25 UTC · re: Jacob Keller · lore

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

On Thu, Apr 13, 2017 at 12:12 PM, Jacob Keller <jacob.keller@gmail.com> wrote:
Show 15 quoted lines
> 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
Jacob Keller· Apr 13, 2017, 19:30 UTC · re: Stefan Beller · lore

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

On Thu, Apr 13, 2017 at 12:25 PM, Stefan Beller <sbeller@google.com> wrote:
Show 35 quoted lines
> 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

Jacob Keller· Apr 14, 2017, 16:33 UTC · re: Stefan Beller · lore

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

On Thu, Apr 13, 2017 at 12:25 PM, Stefan Beller <sbeller@google.com> wrote:
Show 35 quoted lines
> 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

Stefan Beller· Apr 14, 2017, 17:02 UTC · re: Jacob Keller · lore

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

On Fri, Apr 14, 2017 at 9:33 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
Show 7 quoted lines
>
> 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

Jacob Keller· Apr 14, 2017, 23:49 UTC · re: Stefan Beller · lore

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

On Fri, Apr 14, 2017 at 10:02 AM, Stefan Beller <sbeller@google.com> wrote:
Show 15 quoted lines
> 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

Junio C Hamano· Apr 19, 2017, 01:03 UTC · re: Brandon Williams · lore

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

Brandon Williams <bmwill@google.com> writes:
Show 6 quoted lines
> 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?

paul@myitcv.io· May 12, 2017, 16:21 UTC · re: Junio C Hamano · lore

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

> ...

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
Brandon Williams· May 12, 2017, 17:26 UTC · re: paul@myitcv.io · lore

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

On 05/12, paul@myitcv.io wrote:
Show 17 quoted lines
> > ...
> 
> 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.

Show 12 quoted lines
> 
> 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:

Show changes to builtin/ls-files.c +4 −2
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
Paul Jolly· May 12, 2017, 18:12 UTC · re: Brandon Williams · lore

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

>> How can I help diagnose what's going on here?
<snip>
Show 5 quoted lines
> 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.
Brandon Williams· May 12, 2017, 18:19 UTC · re: Paul Jolly · lore

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

On 05/12, Paul Jolly wrote:
Show 19 quoted lines
> >> 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
Paul Jolly· Aug 1, 2017, 18:16 UTC · re: Brandon Williams · lore

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

>> 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
Brandon Williams· Aug 1, 2017, 18:18 UTC · lore

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

On 08/01, Paul Jolly wrote:
Show 13 quoted lines
> >
> > > 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
Paul Jolly· Aug 1, 2017, 18:19 UTC · re: Brandon Williams · lore

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

> 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
Brandon Williams· Aug 1, 2017, 18:20 UTC · re: Paul Jolly · lore

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

On 08/01, Paul Jolly wrote:
Show 7 quoted lines
> > 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
Junio C Hamano· Aug 1, 2017, 18:41 UTC · re: Brandon Williams · lore

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

Brandon Williams <bmwill@google.com> writes:
Show 10 quoted lines
> 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.

← back to recent threads