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

Duy Nguyen· Mar 1, 2016, 09:53 UTC · lore

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

Jeff King· Mar 1, 2016, 09:55 UTC · re: Duy Nguyen · lore

On Tue, Mar 01, 2016 at 04:53:30PM +0700, Duy Nguyen wrote:

Show 7 quoted lines

> > 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?

Yes, that was what I meant by "available everywhere". Library code cannot rely on it right now, as only builtins set it up (even though external programs may call setup_git_directory()).

-Peff

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