{"thread":{"id":"39970","subject":"Confused about sparse vs untracked-cache","startedAt":"2015-07-30T02:32:07Z","lastAt":"2015-07-31T11:28:58Z","messageCount":5,"participants":["David Turner","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"267116","messageId":"1438223527.18134.53.camel@twopensource.com","threadId":"39970","inReplyTo":null,"subject":"Confused about sparse vs untracked-cache","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-30T02:32:07Z","receivedAt":"2015-07-30T02:32:07Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"I'm looking at dir.c, and there's a bit I'm confused about:\n\nprep_exclude() says:\n\t\t /*\n\t\t * .. and .gitignore does not exist before\n\t\t * (i.e. null exclude_sha1 and skip_worktree is\n\t\t * not set). Then we can skip loading .gitignore,\n\t\t * which would result in ENOENT anyway.\n\t\t * skip_worktree is taken care in read_directory()\n\t\t */\n\t\t !is_null_sha1(untracked->exclude_sha1))) {\n\nThat \"skip_worktree is taken care in read_directory()\" appears to be\nreferring to this bit of validate_untracked_cache():\n\t/*\n\t * An optimization in prep_exclude() does not play well with\n\t * CE_SKIP_WORKTREE. It's a rare case anyway, if a single\n\t * entry has that bit set, disable the whole untracked cache.\n\t */\n\tfor (i = 0; i < active_nr; i++)\n\t\tif (ce_skip_worktree(active_cache[i]))\n\t\t\treturn NULL;\n------------\nI'm confused about why skip_worktree needs to be unset. When I comment\nout the second snippet, all the tests still pass. What was the reason\nbehind that condition? Is it really necessary?\n"},{"id":"267138","messageId":"CACsJy8C_PUq4htCSF=qUJ0HNWf-E4RGXJ2FuzbRhSRhLN2vrRg@mail.gmail.com","threadId":"39970","inReplyTo":"1438223527.18134.53.camel@twopensource.com","subject":"Re: Confused about sparse vs untracked-cache","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-07-30T14:09:03Z","receivedAt":"2015-07-30T14:09:03Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Jul 30, 2015 at 9:32 AM, David Turner wrote:\n> I'm looking at dir.c, and there's a bit I'm confused about:\n>\n> prep_exclude() says:\n> /*\n> * .. and .gitignore does not exist before\n> * (i.e. null exclude_sha1 and skip_worktree is\n> * not set). Then we can skip loading .gitignore,\n> * which would result in ENOENT anyway.\n> * skip_worktree is taken care in read_directory()\n> */\n> !is_null_sha1(untracked->exclude_sha1))) {\n>\n> That \"skip_worktree is taken care in read_directory()\" appears to be\n> referring to this bit of validate_untracked_cache():\n> /*\n> * An optimization in prep_exclude() does not play well with\n> * CE_SKIP_WORKTREE. It's a rare case anyway, if a single\n> * entry has that bit set, disable the whole untracked cache.\n> */\n> for (i = 0; i < active_nr; i++)\n> if (ce_skip_worktree(active_cache[i]))\n> return NULL;\n> ------------\n> I'm confused about why skip_worktree needs to be unset. When I comment\n> out the second snippet, all the tests still pass. What was the reason\n> behind that condition? Is it really necessary?\n\nThis code is added in 27b099a (untracked cache: don't open\nnon-existent .gitignore - 2015-03-08) so it's about non-existent\n.gitignore files. We have two cases: .gitignore does not exist, which\nwe want to avoid opening it, and .gitignore does not exist, but it's\nin the index and is skip-worktree'd. We would want to call\nadd_excludes() anyway in the second case.\n\nI think I followed that train of thought when I wrote this and to\navoid trouble, I just left skip-worktree case of out. But that\nce_skip_worktree() check in read_directory() is probably too strong.\nAll we need is disable the cache only when there's an .gitignore file\nbeing skip-worktree'd. If we do that and make sure all .gitignore\nfiles are not skip-worktree'd, the two can work toghether.\n\nBack to the problem. The question is, is\nis_null_sha1(untracked->exclude_sha1) enough to satisfy both cases? If\nso, we don't have to disable the cache in the presence of\nskip-worktree. I haven't stared at this code again long enough to be\nconfident, but I think we may be alright. exclude_sha1 should reflect\nthe true, effective .gitignore content, wherever it's from. So in\nskip-worktree case, is_null_sha1(exclude_sha1) should only be true\nwhen the entry does not exist in both worktree and index. There will\nbe an unnecessary open() in this case before the index version is\nused, but that's probably ok.\n\nNo I don't think the tests cover skip-worktree + untracked cache\ncombination, so yeah it would pass.\n-- \nDuy\n"},{"id":"267181","messageId":"1438299008.18134.59.camel@twopensource.com","threadId":"39970","inReplyTo":"CACsJy8C_PUq4htCSF=qUJ0HNWf-E4RGXJ2FuzbRhSRhLN2vrRg@mail.gmail.com","subject":"Re: Confused about sparse vs untracked-cache","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-30T23:30:08Z","receivedAt":"2015-07-30T23:30:08Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Thu, 2015-07-30 at 21:09 +0700, Duy Nguyen wrote:\n> On Thu, Jul 30, 2015 at 9:32 AM, David Turner wrote:\n> > I'm looking at dir.c, and there's a bit I'm confused about:\n> >\n> > prep_exclude() says:\n> > /*\n> > * .. and .gitignore does not exist before\n> > * (i.e. null exclude_sha1 and skip_worktree is\n> > * not set). Then we can skip loading .gitignore,\n> > * which would result in ENOENT anyway.\n> > * skip_worktree is taken care in read_directory()\n> > */\n> > !is_null_sha1(untracked->exclude_sha1))) {\n> >\n> > That \"skip_worktree is taken care in read_directory()\" appears to be\n> > referring to this bit of validate_untracked_cache():\n> > /*\n> > * An optimization in prep_exclude() does not play well with\n> > * CE_SKIP_WORKTREE. It's a rare case anyway, if a single\n> > * entry has that bit set, disable the whole untracked cache.\n> > */\n> > for (i = 0; i < active_nr; i++)\n> > if (ce_skip_worktree(active_cache[i]))\n> > return NULL;\n> > ------------\n> > I'm confused about why skip_worktree needs to be unset. When I comment\n> > out the second snippet, all the tests still pass. What was the reason\n> > behind that condition? Is it really necessary?\n> \n> This code is added in 27b099a (untracked cache: don't open\n> non-existent .gitignore - 2015-03-08) so it's about non-existent\n> .gitignore files. We have two cases: .gitignore does not exist, which\n> we want to avoid opening it, and .gitignore does not exist, but it's\n> in the index and is skip-worktree'd. We would want to call\n> add_excludes() anyway in the second case.\n> \n> I think I followed that train of thought when I wrote this and to\n> avoid trouble, I just left skip-worktree case of out. But that\n> ce_skip_worktree() check in read_directory() is probably too strong.\n> All we need is disable the cache only when there's an .gitignore file\n> being skip-worktree'd. If we do that and make sure all .gitignore\n> files are not skip-worktree'd, the two can work toghether.\n> \n> Back to the problem. The question is, is\n> is_null_sha1(untracked->exclude_sha1) enough to satisfy both cases? If\n> so, we don't have to disable the cache in the presence of\n> skip-worktree. I haven't stared at this code again long enough to be\n> confident, but I think we may be alright. exclude_sha1 should reflect\n> the true, effective .gitignore content, wherever it's from. So in\n> skip-worktree case, is_null_sha1(exclude_sha1) should only be true\n> when the entry does not exist in both worktree and index. There will\n> be an unnecessary open() in this case before the index version is\n> used, but that's probably ok.\n> \n> No I don't think the tests cover skip-worktree + untracked cache\n> combination, so yeah it would pass.\n\nI went to add some tests for this, but I ran into what appears to be an\nindependent bug in the untracked cache code. \n\nThe call to lookup_untracked in treat_directory() passes dirname, but\ndirname is the full name of the directory. lookup_untracked is looking\nfor the base name -- that is, the code needs to subtract out the length\nof not just the parent dir, but all ancestor directories. So it ends up\ncreating an entry named e.g. foo/bar inside foo (instead of just finding\nan existing 'bar', or adding one called 'bar')\n\nI wasn't really sure what the right way to fix this is, as I just\nstarted looking into this code, but I thought I should report it.\n\nMy test case for this was having two levels of subdirectory e.g.\nfoo/bar/some-file. \n\nLet me know if you need more details.\n"},{"id":"267185","messageId":"1438319612.18134.60.camel@twopensource.com","threadId":"39970","inReplyTo":"1438299008.18134.59.camel@twopensource.com","subject":"Re: Confused about sparse vs untracked-cache","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-31T05:13:32Z","receivedAt":"2015-07-31T05:13:32Z","isPatch":false,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Thu, 2015-07-30 at 19:30 -0400, David Turner wrote:\n> On Thu, 2015-07-30 at 21:09 +0700, Duy Nguyen wrote:\n> > On Thu, Jul 30, 2015 at 9:32 AM, David Turner wrote:\n> > > I'm looking at dir.c, and there's a bit I'm confused about:\n> > >\n> > > prep_exclude() says:\n> > > /*\n> > > * .. and .gitignore does not exist before\n> > > * (i.e. null exclude_sha1 and skip_worktree is\n> > > * not set). Then we can skip loading .gitignore,\n> > > * which would result in ENOENT anyway.\n> > > * skip_worktree is taken care in read_directory()\n> > > */\n> > > !is_null_sha1(untracked->exclude_sha1))) {\n> > >\n> > > That \"skip_worktree is taken care in read_directory()\" appears to be\n> > > referring to this bit of validate_untracked_cache():\n> > > /*\n> > > * An optimization in prep_exclude() does not play well with\n> > > * CE_SKIP_WORKTREE. It's a rare case anyway, if a single\n> > > * entry has that bit set, disable the whole untracked cache.\n> > > */\n> > > for (i = 0; i < active_nr; i++)\n> > > if (ce_skip_worktree(active_cache[i]))\n> > > return NULL;\n> > > ------------\n> > > I'm confused about why skip_worktree needs to be unset. When I comment\n> > > out the second snippet, all the tests still pass. What was the reason\n> > > behind that condition? Is it really necessary?\n> > \n> > This code is added in 27b099a (untracked cache: don't open\n> > non-existent .gitignore - 2015-03-08) so it's about non-existent\n> > .gitignore files. We have two cases: .gitignore does not exist, which\n> > we want to avoid opening it, and .gitignore does not exist, but it's\n> > in the index and is skip-worktree'd. We would want to call\n> > add_excludes() anyway in the second case.\n> > \n> > I think I followed that train of thought when I wrote this and to\n> > avoid trouble, I just left skip-worktree case of out. But that\n> > ce_skip_worktree() check in read_directory() is probably too strong.\n> > All we need is disable the cache only when there's an .gitignore file\n> > being skip-worktree'd. If we do that and make sure all .gitignore\n> > files are not skip-worktree'd, the two can work toghether.\n> > \n> > Back to the problem. The question is, is\n> > is_null_sha1(untracked->exclude_sha1) enough to satisfy both cases? If\n> > so, we don't have to disable the cache in the presence of\n> > skip-worktree. I haven't stared at this code again long enough to be\n> > confident, but I think we may be alright. exclude_sha1 should reflect\n> > the true, effective .gitignore content, wherever it's from. So in\n> > skip-worktree case, is_null_sha1(exclude_sha1) should only be true\n> > when the entry does not exist in both worktree and index. There will\n> > be an unnecessary open() in this case before the index version is\n> > used, but that's probably ok.\n> > \n> > No I don't think the tests cover skip-worktree + untracked cache\n> > combination, so yeah it would pass.\n> \n> I went to add some tests for this, but I ran into what appears to be an\n> independent bug in the untracked cache code. \n> \n> The call to lookup_untracked in treat_directory() passes dirname, but\n> dirname is the full name of the directory. lookup_untracked is looking\n> for the base name -- that is, the code needs to subtract out the length\n> of not just the parent dir, but all ancestor directories. So it ends up\n> creating an entry named e.g. foo/bar inside foo (instead of just finding\n> an existing 'bar', or adding one called 'bar')\n> \n> I wasn't really sure what the right way to fix this is, as I just\n> started looking into this code, but I thought I should report it.\n> \n> My test case for this was having two levels of subdirectory e.g.\n> foo/bar/some-file. \n> \n> Let me know if you need more details.\n\nI should mention that other than that, skip-worktree + untracked cache\nseems to work fine.\n"},{"id":"267197","messageId":"CACsJy8A9qcw+jCa+F5nOS2CMWMdnAVXwAkWxHiNdSyiMWFqWiA@mail.gmail.com","threadId":"39970","inReplyTo":"1438319612.18134.60.camel@twopensource.com","subject":"Re: Confused about sparse vs untracked-cache","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2015-07-31T11:28:58Z","receivedAt":"2015-07-31T11:28:58Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Jul 31, 2015 at 12:13 PM, David Turner wrote:\n> I should mention that other than that, skip-worktree + untracked cache\n> seems to work fine.\n\nPlease go ahead and make a patch to allow it. I'll spend some more\ntime looking at this code. But I think it'll be fine.\n-- \nDuy\n"}]}