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

From: Stefan Beller <sbeller@google.com>
Date: Apr 13, 2017, 18:15 UTC
Message-ID: <CAGZ79kYi4X2cE8uF3cEwh5YndoSL6S2GUK-KYSYibqr+O2qG6g@mail.gmail.com>
In-Reply-To: <20170413171224.3537-2-jacob.e.keller@intel.com>

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