{"thread":{"id":"51950","subject":"git-grep in sparse checkout","startedAt":"2019-10-01T13:06:44Z","lastAt":"2019-10-02T16:46:38Z","messageCount":9,"participants":["Matheus Tavares Bernardino","Bert Wesarg","Elijah Newren","Derrick Stolee","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"383203","messageId":"CAHd-oW7e5qCuxZLBeVDq+Th3E+E4+P8=WzJfK8WcG2yz=n_nag@mail.gmail.com","threadId":"51950","inReplyTo":null,"subject":"git-grep in sparse checkout","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-10-01T13:06:31Z","receivedAt":"2019-10-01T13:06:44Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"Hi,\n\nDuring Git Summit it was mentioned that git-grep searches outside\nsparsity pattern which is not aligned with user expectation. I took a\nquick look at it and it seems the reason is\nbuiltin/grep.c:grep_cache() (which also greps worktree) will grep the\nobject store when a given index entry has the CE_SKIP_WORKTREE bit\nturned on.\n\nFrom what I understand, this bit is used exactly for sparse checkouts\n(as described in Documentation/technical/index-format.txt[1]). But\nshould we perhaps ignore it in git-grep to have the expected behavior?\nI'll be happy to send the patch if so, but I wanted to check with you\nfirst.\n\nGrepping with --cached or in given trees objects would still grep\noutside the cone, but that is what one would expect, right? Or should\nwe filter out what is outside of the cone for cached grep as well?\n\nThanks,\nMatheus\n\n[1]: https://github.com/git/git/blob/master/Documentation/technical/index-format.txt#L101\n"},{"id":"383204","messageId":"CAKPyHN3G-j6p8YZmk+07Sb3tL9vua_R-hJ=W81O7vCYr07AkxA@mail.gmail.com","threadId":"51950","inReplyTo":"CAHd-oW7e5qCuxZLBeVDq+Th3E+E4+P8=WzJfK8WcG2yz=n_nag@mail.gmail.com","subject":"Re: git-grep in sparse checkout","fromName":"Bert Wesarg","fromEmail":"bert.wesarg@googlemail.com","sentAt":"2019-10-01T13:30:45Z","receivedAt":"2019-10-01T13:30:59Z","isPatch":false,"sender":{"key":"bert.wesarg@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/111934?v=4"},"body":"Hi,\n\nOn Tue, Oct 1, 2019 at 3:06 PM Matheus Tavares Bernardino\n wrote:\n>\n> Hi,\n>\n> During Git Summit it was mentioned that git-grep searches outside\n> sparsity pattern which is not aligned with user expectation. I took a\n> quick look at it and it seems the reason is\n> builtin/grep.c:grep_cache() (which also greps worktree) will grep the\n> object store when a given index entry has the CE_SKIP_WORKTREE bit\n> turned on.\n>\n\nI also had once this problem and found that out and wrote a patch. I\nwas just about to send this patch out.\n\nBtw, ls-files should also learn to skip worktree files.\n\nStay tuned.\n\nBert\n\n> From what I understand, this bit is used exactly for sparse checkouts\n> (as described in Documentation/technical/index-format.txt[1]). But\n> should we perhaps ignore it in git-grep to have the expected behavior?\n> I'll be happy to send the patch if so, but I wanted to check with you\n> first.\n>\n> Grepping with --cached or in given trees objects would still grep\n> outside the cone, but that is what one would expect, right? Or should\n> we filter out what is outside of the cone for cached grep as well?\n>\n> Thanks,\n> Matheus\n>\n> [1]: https://github.com/git/git/blob/master/Documentation/technical/index-format.txt#L101\n"},{"id":"383215","messageId":"CABPp-BGuFhDwWZBRaD3nA8ui46wor-4=Ha1G1oApsfF8KNpfGQ@mail.gmail.com","threadId":"51950","inReplyTo":"CAKPyHN3G-j6p8YZmk+07Sb3tL9vua_R-hJ=W81O7vCYr07AkxA@mail.gmail.com","subject":"Re: git-grep in sparse checkout","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-10-01T16:12:26Z","receivedAt":"2019-10-01T16:12:44Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Oct 1, 2019 at 6:30 AM Bert Wesarg wrote:\n>\n> Hi,\n>\n> On Tue, Oct 1, 2019 at 3:06 PM Matheus Tavares Bernardino\n> wrote:\n> >\n> > Hi,\n> >\n> > During Git Summit it was mentioned that git-grep searches outside\n> > sparsity pattern which is not aligned with user expectation. I took a\n> > quick look at it and it seems the reason is\n> > builtin/grep.c:grep_cache() (which also greps worktree) will grep the\n> > object store when a given index entry has the CE_SKIP_WORKTREE bit\n> > turned on.\n> >\n>\n> I also had once this problem and found that out and wrote a patch. I\n> was just about to send this patch out.\n>\n> Btw, ls-files should also learn to skip worktree files.\n>\n> Stay tuned.\n\nI too have a small patch for just grep without --cached or revisions\n(it's only a few lines), but it's very incomplete as that is the only\nusecase it handles. For the usecases I'm closest too, what users have\nreported they want is essentially a miniature repo where they work on\nstuff they care about and ignore the rest. As such, the desired\nfunctionality for these users is:\n\n* git grep, by default, should only search within the sparsity pattern\n* git grep --cached and git grep $REVISION should also only search\nwithin the sparsity pattern\n* git diff $REV1 $REV2, git diff $REV1, etc., should by default only\nsearch within sparsity patterns\n* git log should by default only show commits modifying files matching\nthe sparsity patterns\n* for all of these, there should be some kind of\n--ignore-sparsity-patterns flag to allow searching outside the\nsparsity pattern\n* other commands (archive, bisect, clean?, gitk, shortlog, blame,\nfsck?, etc.) likely need to pay attention to sparsity patterns as\nwell, but there are some special cases:\n\n* merge, cherry-pick, and rebase (anything touching the merge\nmachinery) will need to expand the size of the non-sparse worktree if\nthere are files outside the sparsity patterns with conflicts. (Though\nmerge should do a better job of not expanding the non-sparse worktree\nwhen files can cleanly be resolved.)\n* ls-files has a -t option which can be used to differentiate which\nentries in the index are skip-worktree (S) and which are not. As\nsuch, use of that flag should probably imply\n--ignore-sparsity-patterns\n* fast-export and format-patch are not about viewing history but about\nexporting it, and limiting to sparsity patterns would result in the\ncreation of an incompatible history. As such, they should error out\nwithout a --ignore-sparsity-patterns when invoked from a repository\nthat has a sparse checkout.\n* We may want to augment status with additional information to remind\nusers they are in a sparse checkout\n* New worktrees, by default, should copy the sparsity-patterns of the\nworktree they were created from (much like a new shell inherits the\ncurrent working directory of it's parent process)\n\nI should note here that Stolee wasn't so sure about having 'log' only\nshow commits that touched files within the sparse patterns, so we may\nneed some kind of config setting and have a good usability story for\nwhat each of the settings means and usecases in order to guide how to\nhandle weird cases better.\n\nAlso, as if that weren't enough, there are two more challenges too:\n1) As pointed out by Dscho in the contributor summit, we want\nintersection of pathspecs specified by the user and those in the\nsparsity patterns; e.g. if the user says `git diff $REV -- '*.c' `, we\nwant to show them a diff against $REV of all .c files that are within\ntheir sparsity patterns.\n2) We have two different kinds of path patterns, one for .gitignore\nand sparse-checkout, and the other for command-line pathspecs. See\nhttps://public-inbox.org/git/xmqq4l1qpiaw.fsf@gitster-ct.c.googlers.com/.\nThese differences might make the implementation more difficult, and\nmaking the two types of path patterns have more overlap might be a\nnecessary first step.\n\nHowever, the \"work with a miniature repo\" probably makes the VFS for\nGit and partial clone stuff easier -- we don't have to worry about the\nincessant need to download more blobs after the initial partial clone\nbecause git commands by default would avoid requesting them. It also\nwould work quite nicely with a partial index -- we could have a\ndirectory entry in the index and marked as skip-worktree and avoid\nhaving all the paths under it show up in the index. That would\naccelerate many operations within git.\n\n\n\nI'd love to work on this, but I've got plenty of other things on my\nplate at the moment, so I probably won't get time for it at least\nuntil the middle of next year. But I thought I'd send out what I view\nas the bigger picture. Also, this is very much still idea stage; the\ncontributor summit refined some of the ideas and there may be more\nrefinement as more people in the list chime in.\n\n\nHope that helps,\nElijah\n"},{"id":"383225","messageId":"f3101c8f-9709-5bcb-35f0-39ffaf8aa809@gmail.com","threadId":"51950","inReplyTo":"CAHd-oW7e5qCuxZLBeVDq+Th3E+E4+P8=WzJfK8WcG2yz=n_nag@mail.gmail.com","subject":"Re: git-grep in sparse checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-10-01T18:29:00Z","receivedAt":"2019-10-01T18:29:04Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/1/2019 9:06 AM, Matheus Tavares Bernardino wrote:\n> Hi,\n> \n> During Git Summit it was mentioned that git-grep searches outside\n> sparsity pattern which is not aligned with user expectation. I took a\n> quick look at it and it seems the reason is\n> builtin/grep.c:grep_cache() (which also greps worktree) will grep the\n> object store when a given index entry has the CE_SKIP_WORKTREE bit\n> turned on.\n> \n> From what I understand, this bit is used exactly for sparse checkouts\n> (as described in Documentation/technical/index-format.txt[1]). But\n> should we perhaps ignore it in git-grep to have the expected behavior?\n> I'll be happy to send the patch if so, but I wanted to check with you\n> first.\n\nIs that the expected behavior? In a sparse-checkout, wouldn't you _want_\nGit to report things outside the cone? You can already use external tools\nto search for things in the sparse cone: they are on disk. You need \"git\ngrep\" for the objects reachable from the current tree but not already\non disk.\n\nI respect the goal to minimize the work \"git grep\" is doing, especially\nin a sparse-checkout + partial-clone world, where we wouldn't expect to\nhave the blobs locally and this search would cause many blob downloads.\nI just want to truly examine if this is the right behavior.\n\nAt minimum, I would expect a new option to have \"git grep\" go back to\nthe old behavior, so users who really want a tree-wide search can have\none.\n\nThanks,\n-Stolee\n"},{"id":"383235","messageId":"CAHd-oW6XUFO2mG_5kiqsTYuh998j95Sg_6zBBE4sXTtCJ3dPDA@mail.gmail.com","threadId":"51950","inReplyTo":"f3101c8f-9709-5bcb-35f0-39ffaf8aa809@gmail.com","subject":"Re: git-grep in sparse checkout","fromName":"Matheus Tavares Bernardino","fromEmail":"matheus.bernardino@usp.br","sentAt":"2019-10-02T00:06:15Z","receivedAt":"2019-10-02T00:06:31Z","isPatch":false,"sender":{"key":"matheus.tavb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/12701583?v=4"},"body":"On Tue, Oct 1, 2019 at 3:29 PM Derrick Stolee wrote:\n>\n> On 10/1/2019 9:06 AM, Matheus Tavares Bernardino wrote:\n> > Hi,\n> >\n> > During Git Summit it was mentioned that git-grep searches outside\n> > sparsity pattern which is not aligned with user expectation. I took a\n> > quick look at it and it seems the reason is\n> > builtin/grep.c:grep_cache() (which also greps worktree) will grep the\n> > object store when a given index entry has the CE_SKIP_WORKTREE bit\n> > turned on.\n> >\n> > From what I understand, this bit is used exactly for sparse checkouts\n> > (as described in Documentation/technical/index-format.txt[1]). But\n> > should we perhaps ignore it in git-grep to have the expected behavior?\n> > I'll be happy to send the patch if so, but I wanted to check with you\n> > first.\n>\n> Is that the expected behavior? In a sparse-checkout, wouldn't you _want_\n> Git to report things outside the cone? You can already use external tools\n> to search for things in the sparse cone: they are on disk. You need \"git\n> grep\" for the objects reachable from the current tree but not already\n> on disk.\n\nHmm, we can use external tools to search in the sparse cone, but even\nin this circumstance, I think we need git-grep for some usecases.\ngit-grep on disk can exclude files not being tracked and access other\ngit functionalities such as --textconv to read .gitattributes and\nconvert the files before grepping. So maybe people would want to grep\njust the cone but also have these other options that would be\nunavailable through external tools?\n\n(Users could already simulate such behavior giving the sparse clone\npatterns as pathspecs to git-grep, but that can get complicated for\nmore complex sparse patterns.)\n\n> I respect the goal to minimize the work \"git grep\" is doing, especially\n> in a sparse-checkout + partial-clone world, where we wouldn't expect to\n> have the blobs locally and this search would cause many blob downloads.\n> I just want to truly examine if this is the right behavior.\n>\n> At minimum, I would expect a new option to have \"git grep\" go back to\n> the old behavior, so users who really want a tree-wide search can have\n> one.\n\nYes, I totally agree that this behavior change should come with such\nan option. Maybe --ignore-sparsity-patterns, as Elijah suggested.\n\n> Thanks,\n> -Stolee\n"},{"id":"383251","messageId":"xmqq36gbfz62.fsf@gitster-ct.c.googlers.com","threadId":"51950","inReplyTo":"f3101c8f-9709-5bcb-35f0-39ffaf8aa809@gmail.com","subject":"Re: git-grep in sparse checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-02T06:18:29Z","receivedAt":"2019-10-02T06:18:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> Is that the expected behavior? In a sparse-checkout, wouldn't you _want_\n> Git to report things outside the cone?\n\nThat should be optional, I would think. When you declare \"by\ndefault, this the subset of the project I am interested in\", we\nshould honor it, I would think.\n\n> At minimum, I would expect a new option to have \"git grep\" go back to\n> the old behavior, so users who really want a tree-wide search can have\n> one.\n\nYeah, a bugfix to honor SKIP_WORKTREE bit, followed by a new feature\nto ignore it, would be pretty sensible way to go.\n\n"},{"id":"383252","messageId":"xmqqy2y3ejwe.fsf@gitster-ct.c.googlers.com","threadId":"51950","inReplyTo":"CABPp-BGuFhDwWZBRaD3nA8ui46wor-4=Ha1G1oApsfF8KNpfGQ@mail.gmail.com","subject":"Re: git-grep in sparse checkout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-10-02T06:33:37Z","receivedAt":"2019-10-02T06:33:45Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Elijah Newren writes:\n\n> * other commands (archive, bisect, clean?, gitk, shortlog, blame,\n> fsck?, etc.) likely need to pay attention to sparsity patterns as\n> well, but there are some special cases:\n\n\"git archive\" falls into the same class as fast-(im|ex)port; it\nshould ignore the sparse cone by default. I suspect you threw\n\"fsck\" as a joke, but I do not think it should pay attention to the\nsparse cone, either (besides, most of the time in fsck the objects\nsubject to checking do not know all the paths that reach them).\n\n> * merge, cherry-pick, and rebase (anything touching the merge\n> machinery) will need to expand the size of the non-sparse worktree if\n> there are files outside the sparsity patterns with conflicts. (Though\n> merge should do a better job of not expanding the non-sparse worktree\n> when files can cleanly be resolved.)\n\nI think the important point is what is done to the result of\noperation. Result of these operations that create new commits are\nmeant to be consumed by other people, who may not share your\ndefinition of sparse cone. And such a command (i.e. those whose\nresults are consumed by others who may have different sparse cone)\nmust be full-tree by default.\n\n> * fast-export and format-patch are not about viewing history but about\n> exporting it, and limiting to sparsity patterns would result in the\n> creation of an incompatible history.\n\nI agree with the conclusion; see above.\n\n> * New worktrees, by default, should copy the sparsity-patterns of the\n> worktree they were created from (much like a new shell inherits the\n> current working directory of it's parent process)\n\nSorry, but I do not share this view at all.\n\nIn my mental model, \"worktree new\" is to attach a brand-new worktree\nto a bare repository that underlies the existing worktree I happen\nto be in, and that existing worktree that I happen to type \"worktree\nnew\" in is no more or no less special than other worktrees.\n\nThe above isn't to say that I'd veto your \"a new worktree inherits\ntraits from an existing worktree that 'git worktree add' was invoked\nin\" idea. I am just saying that I have a problem with that mode of\noperation and mental model being the default.\n\n\n"},{"id":"383271","messageId":"48ebe9a4-23a8-8679-f380-195d7fc93d60@gmail.com","threadId":"51950","inReplyTo":"xmqq36gbfz62.fsf@gitster-ct.c.googlers.com","subject":"Re: git-grep in sparse checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-10-02T12:09:13Z","receivedAt":"2019-10-02T12:09:17Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 10/2/2019 2:18 AM, Junio C Hamano wrote:\n> Derrick Stolee writes:\n> \n>> Is that the expected behavior? In a sparse-checkout, wouldn't you _want_\n>> Git to report things outside the cone?\n> \n> That should be optional, I would think. When you declare \"by\n> default, this the subset of the project I am interested in\", we\n> should honor it, I would think.\n> \n>> At minimum, I would expect a new option to have \"git grep\" go back to\n>> the old behavior, so users who really want a tree-wide search can have\n>> one.\n> \n> Yeah, a bugfix to honor SKIP_WORKTREE bit, followed by a new feature\n> to ignore it, would be pretty sensible way to go.\n\nThanks for everyone's responses here. I am satisfied with this direction.\n\n-Stolee\n\n"},{"id":"383286","messageId":"CABPp-BE6w_GJ6+N0PJBpJh=pguM85izUYCfFy=AoE53OiifAUg@mail.gmail.com","threadId":"51950","inReplyTo":"xmqqy2y3ejwe.fsf@gitster-ct.c.googlers.com","subject":"Re: git-grep in sparse checkout","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-10-02T16:46:23Z","receivedAt":"2019-10-02T16:46:38Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Tue, Oct 1, 2019 at 11:33 PM Junio C Hamano wrote:\n>\n> Elijah Newren writes:\n>\n> > * other commands (archive, bisect, clean?, gitk, shortlog, blame,\n> > fsck?, etc.) likely need to pay attention to sparsity patterns as\n> > well, but there are some special cases:\n>\n> \"git archive\" falls into the same class as fast-(im|ex)port; it\n> should ignore the sparse cone by default. I suspect you threw\n> \"fsck\" as a joke, but I do not think it should pay attention to the\n> sparse cone, either (besides, most of the time in fsck the objects\n> subject to checking do not know all the paths that reach them).\n\narchive in the same category as fast-(im|ex)port makes sense. I'm not\nsure if \"ignore the sparse cone\" by default makes sense or if it\nshould be a case where we error out if --ignore-sparsity-patterns\nisn't specified, especially if history is also sparse.\n\nIn terms of fsck, I agree that if history is dense and the worktree is\nsparse that you want to walk all history. I was thinking further\nalong the lines when partial clones and sparse checkouts are combined\nso that history is also sparse. In cases where a partial clone is in\nuse, rather than download everything in order to walk it, wouldn't it\nmake more sense to have fsck walk over the bits that are already\ndownloaded? I don't really know how that'd all work, but it seems\nthat if fsck walked over all history it'd be treated as a\nuseless/dangerous command by those who are doing partial clones\nbecause the repo is just too big.\n\n> > * merge, cherry-pick, and rebase (anything touching the merge\n> > machinery) will need to expand the size of the non-sparse worktree if\n> > there are files outside the sparsity patterns with conflicts. (Though\n> > merge should do a better job of not expanding the non-sparse worktree\n> > when files can cleanly be resolved.)\n>\n> I think the important point is what is done to the result of\n> operation. Result of these operations that create new commits are\n> meant to be consumed by other people, who may not share your\n> definition of sparse cone. And such a command (i.e. those whose\n> results are consumed by others who may have different sparse cone)\n> must be full-tree by default.\n>\n> > * fast-export and format-patch are not about viewing history but about\n> > exporting it, and limiting to sparsity patterns would result in the\n> > creation of an incompatible history.\n>\n> I agree with the conclusion; see above.\n>\n> > * New worktrees, by default, should copy the sparsity-patterns of the\n> > worktree they were created from (much like a new shell inherits the\n> > current working directory of it's parent process)\n>\n> Sorry, but I do not share this view at all.\n>\n> In my mental model, \"worktree new\" is to attach a brand-new worktree\n> to a bare repository that underlies the existing worktree I happen\n> to be in, and that existing worktree that I happen to type \"worktree\n> new\" in is no more or no less special than other worktrees.\n>\n> The above isn't to say that I'd veto your \"a new worktree inherits\n> traits from an existing worktree that 'git worktree add' was invoked\n> in\" idea. I am just saying that I have a problem with that mode of\n> operation and mental model being the default.\n\nIf worktrees are the only area we disagree on, then I'll happily take\nthe stuff we agree on and can overlook this piece.\n\nBut, perhaps some further explaining on worktrees might help us reach\nsome middle ground. If worktrees are dense by default and folks have\nnot only sparse checkouts but sparse history, then creating a new\nworktree would suddenly mandate downloading a lot more of history --\nwhich could be prohibitively expensive, forcing people to instead have\nN clones without any shared history. That may be fine (I tend to not\nbe a heavy worktree user, I just support some users who are), but is\nit the route we want to push people with big repos towards?\n\n\nThanks for the feedback on the ideas,\nElijah\n"}]}