Re: [PATCH v7 01/33] setup: call setup_git_directory_gently before accessing refs

From: Duy Nguyen <pclouds@gmail.com>
Date: Mar 1, 2016, 09:53 UTC
Message-ID: <CACsJy8AiT0=juU0pqBfraCZyk-g9MgEuybOw-0JQyqm9VveUWQ@mail.gmail.com>

On Tue, Mar 1, 2016 at 3:35 PM, Jeff King <peff@peff.net> wrote:

Show 13 quoted lines

> On Mon, Feb 29, 2016 at 07:52:34PM -0500, David Turner wrote:
>
>> Usually, git calls some form of setup_git_directory at startup.  But
>> sometimes, it doesn't.  Usually, that's OK because it's not really
>> using the repository.  But in some cases, it is using the repo.  In
>> those cases, either setup_git_directory_gently must be called, or the
>> repository (e.g. the refs) must not be accessed.
>
> It's actually not just setup_git_directory(). We can also use
> check_repository_format(), which is used by enter_repo() (and hence by
> things like upload-pack). I think the rule really ought to be: if we
> didn't have check_repository_format_gently() tell us we have a valid
> repo, we should not access any repo elements (refs, objects, etc).

Agreed.

There's also a lighter version of check_repo.. which is is_git_directory(). Most of the time we just want to answer the question "is it a valid repository? support or not does not matter". We probably need more eyes on submodule case when this functino is used. For example in 25/33 [1] we check if a repo is non-bare (a variant of is_git_directory) then we peek the config file inside. Should check_repository_format() be done in this case?

You know what, forget my question. The answer is yes. After writing all that, I remember that part of the config file may be moved away in the next version of multiple worktrees [2]. We need proper repo validation before reading anything inside.

[1] http://article.gmane.org/gmane.comp.version-control.git/287959 [2] http://article.gmane.org/gmane.comp.version-control.git/284803

> I started earlier today on a patch series to identify and fix these
> cases independent of your series.

Yes this sounds like a separate problem, even though it's raised by lmdb topic.

> The basic strategy was to adapt the
> existing "struct startup_info" to be available everywhere, and have
> relevant bits of code assert() on it, or even behave differently (e.g.,
> if some library code should do different things in a repo versus not).

startup_info is NULL for external programs if I remember correctly, or do you make it available to all of them too?

-- 
Duy