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

[Brandon Williams <bmwill@google.com>]

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