On Wed, Apr 12, 2017 at 9:58 AM, Brandon Williams <bmw...@google.com> wrote:
> On 04/11, Jacob Keller wrote:
>> From: Jacob Keller <jacob.kel...@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.kel...@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
