{"thread":{"id":"55983","subject":"Untracked Cache and --untracked-files=all","startedAt":"2021-06-22T09:25:50Z","lastAt":"2021-06-23T06:42:28Z","messageCount":3,"participants":["Tao Klerks","Elijah Newren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"428187","messageId":"CAPMMpohp6+jW2C0ewfYEp3rrwbKSqGVa94LRgQDcKJvYmiANuA@mail.gmail.com","threadId":"55983","inReplyTo":null,"subject":"Untracked Cache and --untracked-files=all","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2021-06-22T09:25:36Z","receivedAt":"2021-06-22T09:25:50Z","isPatch":false,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"Hi folks,\n\nI'm hoping for a little help understanding *intent* around a\nparticular code comment in dir.c, and reaching out to the whole list\nbecause someone (Junio?) said they consider any mail that *doesn't*\ncopy the list to be spam anyway, and the original author of the\ncomment in question (Duy Nguyen) is no longer active on git.\n\nContext:\nI am trying to explore how to get \"--untracked-files=all\" to play nice\nwith the untracked cache, so that windows users using tooling that\nsets \"--untracked-files=all\" can benefit from the same\norders-of-magnitude git status performance improvements as commandline\nusers.\n\nThere is a \"naive\" approach to this (store the untracked cache in the\nindex file with whatever dir flags were specified/used in the\nrecursive walk, and ignore/rewrite that cached data every time the\nflags change), and there is presumably a more \"comprehensive\" approach\n(store all the information required in the untracked cache to be able\nto satisfy requests with either set of flags - even if this is a\nlittle more expensive on first run).\n\nThe main disadvantage of the \"naive\" approach is that is every time\nyou flip-flop between \"git status\" and \"git status -u\", the untracked\ncache is ignored, a recursive directory walk ensues, and the untracked\ncache is rewritten to the index file for the next time you rerun,\nhopefully with the same flags. However, I would think in most\nsituations flip-flopping will be less common - more commonly you're\nusing a tool or workflow that ends up running the same command(s)\nrepeatedly... At least, that's my thesis. I would put this \"store the\nuntracked cache every time dir flags change\" behavior behind a config\nswitch, anyway.\n\nThis \"naive\" approach *is* rather easy to achieve - you just need to\nrecreate a new \"untracked\" structure inside\ndir.c#validate_untracked_cache() if you find a mismatch of flags (and\nmake other small fixes to store the correct flags in that new\n\"untracked\" structure).\n\nThe one thing that sticks out after making these changes is a code\ncomment first introduced in 2015 by Duy Nguyen in ccad261f, explaining\n*why* we refuse to use the untracked cache with \"-uall\":\n> * See treat_directory(), case index_nonexistent. Without\n> * this flag, we may need to also cache .git file content\n> * for the resolve_gitlink_ref() call, which we don't.\n\nI've seen this comment many times over the past few months, and I've\npreviously always interpreted it as a \"correctness\" concern.\n\nLooking at the original code in question, however (as of ccad261f,\ndir.c#treat_directory(), \"resolve_gitlink_ref\" call), I don't\nunderstand how correctness could be impacted.\n\nFast-forward 6 years and all this code has been substantially\noverhauled by several folks over the years (most recently and majorly\nElijah Newren), and the \"resolve_gitlink_ref()\" call is long-gone.\n\nDoes this look familiar to anyone? Is there any remaining obvious\nreason to be wary of storing the untracked cache structure produced\nwith '-uall'?\n\nThanks,\nTao\n"},{"id":"428210","messageId":"CABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com","threadId":"55983","inReplyTo":"CAPMMpohp6+jW2C0ewfYEp3rrwbKSqGVa94LRgQDcKJvYmiANuA@mail.gmail.com","subject":"Re: Untracked Cache and --untracked-files=all","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-06-22T16:06:23Z","receivedAt":"2021-06-22T16:06:38Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi,\n\nOn Tue, Jun 22, 2021 at 2:25 AM Tao Klerks <tao@klerks.biz> wrote:\n>\n> Hi folks,\n>\n> I'm hoping for a little help understanding *intent* around a\n> particular code comment in dir.c, and reaching out to the whole list\n\nOh, boy, dir.c.  We should probably put a simple warning at the top of\nthe file, something like \"Abandon all hope, all ye who enter here\".\n\n> because someone (Junio?) said they consider any mail that *doesn't*\n> copy the list to be spam anyway, and the original author of the\n> comment in question (Duy Nguyen) is no longer active on git.\n>\n> Context:\n> I am trying to explore how to get \"--untracked-files=all\" to play nice\n> with the untracked cache, so that windows users using tooling that\n> sets \"--untracked-files=all\" can benefit from the same\n> orders-of-magnitude git status performance improvements as commandline\n> users.\n>\n> There is a \"naive\" approach to this (store the untracked cache in the\n> index file with whatever dir flags were specified/used in the\n> recursive walk, and ignore/rewrite that cached data every time the\n> flags change), and there is presumably a more \"comprehensive\" approach\n> (store all the information required in the untracked cache to be able\n> to satisfy requests with either set of flags - even if this is a\n> little more expensive on first run).\n>\n> The main disadvantage of the \"naive\" approach is that is every time\n> you flip-flop between \"git status\" and \"git status -u\", the untracked\n> cache is ignored, a recursive directory walk ensues, and the untracked\n> cache is rewritten to the index file for the next time you rerun,\n> hopefully with the same flags. However, I would think in most\n> situations flip-flopping will be less common - more commonly you're\n> using a tool or workflow that ends up running the same command(s)\n> repeatedly... At least, that's my thesis. I would put this \"store the\n> untracked cache every time dir flags change\" behavior behind a config\n> switch, anyway.\n>\n> This \"naive\" approach *is* rather easy to achieve - you just need to\n> recreate a new \"untracked\" structure inside\n> dir.c#validate_untracked_cache() if you find a mismatch of flags (and\n> make other small fixes to store the correct flags in that new\n> \"untracked\" structure).\n>\n> The one thing that sticks out after making these changes is a code\n> comment first introduced in 2015 by Duy Nguyen in ccad261f, explaining\n> *why* we refuse to use the untracked cache with \"-uall\":\n> > * See treat_directory(), case index_nonexistent. Without\n> > * this flag, we may need to also cache .git file content\n> > * for the resolve_gitlink_ref() call, which we don't.\n>\n> I've seen this comment many times over the past few months, and I've\n> previously always interpreted it as a \"correctness\" concern.\n>\n> Looking at the original code in question, however (as of ccad261f,\n> dir.c#treat_directory(), \"resolve_gitlink_ref\" call), I don't\n> understand how correctness could be impacted.\n>\n> Fast-forward 6 years and all this code has been substantially\n> overhauled by several folks over the years (most recently and majorly\n> Elijah Newren), and the \"resolve_gitlink_ref()\" call is long-gone.\n\nI didn't remember removing any resolve_gitlink_ref() calls, but a\nquick search (`git log -Sresolve_gitlink_ref -p -- dir.c`) suggests it\nwas removed by b22827045e0b (dir: do not traverse repositories with no\ncommits, 2019-04-09).  A brief look at that commit message certainly\nsuggests things have changed in a way that _might_ make the original\ncomment irrelevant.\n\n> Does this look familiar to anyone? Is there any remaining obvious\n> reason to be wary of storing the untracked cache structure produced\n> with '-uall'?\n\nUntracked cache is its own beast.  I fought with it a bunch, but it's\nbeen a while.  (See e.g.\nhttps://lore.kernel.org/git/CABPp-BEWhavmtDi-ahT+QMWtD6Fe-Ey7cn_82nbetWEQJL=hRA@mail.gmail.com/)\n\nI can say that untracked traversal had all sorts of weirdness and\nbreakage, which could have made untracked cache harder to make right\nand consistent particularly with -uall.  See for example 8d92fb292706\n(dir: replace exponential algorithm with a linear one, 2020-04-01) and\nperhaps aa6e1b21e5de (dir: avoid unnecessary traversal into ignored\ndirectory, 2021-05-12).  In particular, the exponential traversal\n(paths at depth N might be traversed 2^N times) from the former commit\ncombined with the fact that later traversals often returned a\ndifferent status for a path than the first traversal certainly could\nhave caused some additional weirdness for the untracked cache.\n\nIt may be that some combination of Kyle's removal of the\nresolve_gitlink_ref() and my removal of the exponential traversal of\nuntracked paths make your idea safe...but this is dir.c and\nuntracked-cache, so who really knows?\n\n\nNot sure if that helps, but I hope one or two pointers I provided are\nsomehow meaningful to you.\n\nElijah\n"},{"id":"428255","messageId":"CAPMMpojA7EVSuKo=Ej55H-Sk29LitEJscMuMbHHPfR7UiR4n6A@mail.gmail.com","threadId":"55983","inReplyTo":"CABPp-BFiwzzUgiTj_zu+vF5x20L0=1cf25cHwk7KZQj2YkVzXw@mail.gmail.com","subject":"Re: Untracked Cache and --untracked-files=all","fromName":"Tao Klerks","fromEmail":"tao@klerks.biz","sentAt":"2021-06-23T06:42:10Z","receivedAt":"2021-06-23T06:42:28Z","isPatch":false,"sender":{"key":"tao@klerks.biz","avatar":"https://avatars.githubusercontent.com/u/531704?v=4"},"body":"Hi Elijah,\n\nOn Tue, Jun 22, 2021 at 6:06 PM Elijah Newren <newren@gmail.com> wrote:\n> It may be that some combination of Kyle's removal of the\n> resolve_gitlink_ref() and my removal of the exponential traversal of\n> untracked paths make your idea safe...but this is dir.c and\n> untracked-cache, so who really knows?\n>\n> Not sure if that helps, but I hope one or two pointers I provided are\n> somehow meaningful to you.\n\nIt is, thank you! I had already looked at those commits you mention,\nand my impression is indeed that any -uall incompatibilities have been\naddressed as a result of the major restructurings and fixes.\n\nAfter this exchange I prepared a patch that seems to be \"right\" from\nmy perspective - it aligns the untracked cache flags with the config,\nso if your command-line \"-uXXX\" choice matches your config you get\nuntracked cache, if it does not you get it disabled for that run (ie,\nin windows under some circumstances, surprisingly bad performance).\n\nThis means people who want -uall to work well/performantly (because\nthey prefer it, or because their tooling requires it) can now have it\ndo so, simply by setting \"git config status.showuntrackedfiles all\".\n\nThis seems much better than a new temporary config, it is not subject\nto \"flip-flopping\" issues like my previously described \"naive\"\napproach was, and it seems much more achievable than substantially\nchanging the untracked cache structure & correspondingly adjusting the\nuntracked directory iteration process to support both -unormal and\n-uall simultaneously within a single cache structure.\n\nThe patch passes all (working) tests, including the t7603 untracked\ncache series I know you worked on, and seems to do exactly what I\nintend it to - but I'm new here, so there are probably many problems;\nhopefully this is interesting enough for people to take the time to\npoint them out.\n\nThe pull request in GitGitGadget is still requesting some form of\napproval before I can submit the patch to the mailing list:\nhttps://github.com/gitgitgadget/git/pull/985\n\nThanks,\nTao\n"}]}