{"thread":{"id":"41576","subject":"Re: [PATCH v7 01/33] setup: call setup_git_directory_gently before accessing refs","startedAt":"2016-03-01T09:53:30Z","lastAt":"2016-03-01T09:55:43Z","messageCount":2,"participants":["Duy Nguyen","Jeff King"],"isPatch":true,"patchVersion":7,"patchTotal":33},"messages":[{"id":"279943","messageId":"CACsJy8AiT0=juU0pqBfraCZyk-g9MgEuybOw-0JQyqm9VveUWQ@mail.gmail.com","threadId":"41576","inReplyTo":null,"subject":"Re: [PATCH v7 01/33] setup: call setup_git_directory_gently before accessing refs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-03-01T09:53:30Z","receivedAt":"2016-03-01T09:53:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Mar 1, 2016 at 3:35 PM, Jeff King <peff@peff.net> wrote:\n> On Mon, Feb 29, 2016 at 07:52:34PM -0500, David Turner wrote:\n>\n>> Usually, git calls some form of setup_git_directory at startup.  But\n>> sometimes, it doesn't.  Usually, that's OK because it's not really\n>> using the repository.  But in some cases, it is using the repo.  In\n>> those cases, either setup_git_directory_gently must be called, or the\n>> repository (e.g. the refs) must not be accessed.\n>\n> It's actually not just setup_git_directory(). We can also use\n> check_repository_format(), which is used by enter_repo() (and hence by\n> things like upload-pack). I think the rule really ought to be: if we\n> didn't have check_repository_format_gently() tell us we have a valid\n> repo, we should not access any repo elements (refs, objects, etc).\n\nAgreed.\n\nThere's also a lighter version of check_repo.. which is\nis_git_directory(). Most of the time we just want to answer the\nquestion \"is it a valid repository? support or not does not matter\".\nWe probably need more eyes on submodule case when this functino is\nused. For example in 25/33 [1] we check if a repo is non-bare (a\nvariant of is_git_directory) then we peek the config file inside.\nShould check_repository_format() be done in this case?\n\nYou know what, forget my question. The answer is yes. After writing\nall that, I remember that part of the config file may be moved away in\nthe next version of multiple worktrees [2]. We need proper repo\nvalidation before reading anything inside.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/287959\n[2] http://article.gmane.org/gmane.comp.version-control.git/284803\n\n> I started earlier today on a patch series to identify and fix these\n> cases independent of your series.\n\nYes this sounds like a separate problem, even though it's raised by lmdb topic.\n\n> The basic strategy was to adapt the\n> existing \"struct startup_info\" to be available everywhere, and have\n> relevant bits of code assert() on it, or even behave differently (e.g.,\n> if some library code should do different things in a repo versus not).\n\nstartup_info is NULL for external programs if I remember correctly, or\ndo you make it available to all of them too?\n-- \nDuy\n"},{"id":"279944","messageId":"20160301095542.GA4718@sigill.intra.peff.net","threadId":"41576","inReplyTo":"CACsJy8AiT0=juU0pqBfraCZyk-g9MgEuybOw-0JQyqm9VveUWQ@mail.gmail.com","subject":"Re: [PATCH v7 01/33] setup: call setup_git_directory_gently before accessing refs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-03-01T09:55:43Z","receivedAt":"2016-03-01T09:55:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 01, 2016 at 04:53:30PM +0700, Duy Nguyen wrote:\n\n> > The basic strategy was to adapt the\n> > existing \"struct startup_info\" to be available everywhere, and have\n> > relevant bits of code assert() on it, or even behave differently (e.g.,\n> > if some library code should do different things in a repo versus not).\n> \n> startup_info is NULL for external programs if I remember correctly, or\n> do you make it available to all of them too?\n\nYes, that was what I meant by \"available everywhere\". Library code\ncannot rely on it right now, as only builtins set it up (even though\nexternal programs may call setup_git_directory()).\n\n-Peff\n"}]}