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

From: Jacob Keller <jacob.keller@gmail.com>
Date: Apr 13, 2017, 18:34 UTC
Message-ID: <CA+P7+xpFVcj=Ssu6mjPs8km-GEsyhzbeVdb0ENH4bdzdqXOTsA@mail.gmail.com>
In-Reply-To: <CAGZ79kYi4X2cE8uF3cEwh5YndoSL6S2GUK-KYSYibqr+O2qG6g@mail.gmail.com>

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