{"thread":{"id":"56545","subject":"Memory leak with sparse-checkout","startedAt":"2021-09-20T12:15:30Z","lastAt":"2021-09-22T19:37:09Z","messageCount":15,"participants":["Calbabreaker","Taylor Blau","Derrick Stolee"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"436409","messageId":"CAKRwm5a9PyqffEC5N__urSpNcZ-d5vz9GBM2Ei16eGS25B=-FQ@mail.gmail.com","threadId":"56545","inReplyTo":null,"subject":"Memory leak with sparse-checkout","fromName":"Calbabreaker","fromEmail":"calbabreaker@gmail.com","sentAt":"2021-09-20T12:15:14Z","receivedAt":"2021-09-20T12:15:30Z","isPatch":false,"sender":{"key":"calbabreaker@gmail.com","avatar":null},"body":"What did you do before the bug happened? (Steps to reproduce your issue)\n\nThis was ran:\n\ngit clone https://github.com/Calbabreaker/piano --sparse\ncd piano\ngit sparse-checkout add any_text\ngit checkout deploy-frontend\ngit sparse-checkout init --cone\ngit sparse-checkout add any_text\n\nBasically clone any repository with --sparse, add anything to\nsparse-checkout list\nthen checkout to another branch, init the sparse-checkout with --cone and add\nanything to it again.\n\nWhat did you expect to happen? (Expected behavior)\n\nNothing at all except for the sparse-checkout list to be updated.\n\nWhat happened instead? (Actual behavior)\n\nA massive memory leak using all the RAM with one of the cpu cores\nbeing at 100% usage.\n\n[System Info]\ngit version:\ngit version 2.33.0\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /bin/sh\nuname: Linux 5.10.63-1-MANJARO #1 SMP PREEMPT Wed Sep 8 14:13:59 UTC 2021 x86_64\ncompiler info: gnuc: 11.1\nlibc info: glibc: 2.33\n$SHELL (typically, interactive shell): /usr/bin/zsh\n"},{"id":"436434","messageId":"YUiuWSXO1P3JwerH@nand.local","threadId":"56545","inReplyTo":"CAKRwm5a9PyqffEC5N__urSpNcZ-d5vz9GBM2Ei16eGS25B=-FQ@mail.gmail.com","subject":"Re: Memory leak with sparse-checkout","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T15:52:57Z","receivedAt":"2021-09-20T15:53:00Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 09:45:14PM +0930, Calbabreaker wrote:\n> What did you do before the bug happened? (Steps to reproduce your issue)\n>\n> This was ran:\n>\n> git clone https://github.com/Calbabreaker/piano --sparse\n> cd piano\n> git sparse-checkout add any_text\n> git checkout deploy-frontend\n> git sparse-checkout init --cone\n> git sparse-checkout add any_text\n\nThanks for the reproduction. An even simpler one may be (inside of any\nrepository):\n\n git sparse-checkout init\n git sparse-checkout add dir\n git sparse-checkout init --cone\n git sparse-checkout add dir\n\nThe problem occurs because we keep existing entries when adding to the\nsparse-checkout list, and cone-mode patterns do not mix with\nnon cone-mode patterns.\n\nSo after the first init and \"add dir\", your sparse-checkout file looks\nlike:\n\n /*\n !/*/\n dir\n\nbut then when we convert to cone-mode and try and add \"dir\" (which in\ncone-mode we'll convert to \"/dir/\"), we run into trouble when adding the\nexisting \"dir\" entry. That's because add_patterns_cone_mode() calls\ninsert_recursive_pattern() on every entry in the existing list,\nincluding \"dir\".\n\nSo when we call insert_recursive_pattern() with any pattern list and\npath containing \"dir\", we first insert \"dir\" into the list, and then:\n\n char *slash = strrchr(e->pattern, '/');\n char *oldpattern = e->pattern;\n\n if (slash == e->pattern)\n break;\n // trim off a slash, repeat\n\nexcept slash is NULL because \"dir\" doesn't contain a slash. And that\nexplains the problem you're seeing, because (a) we'll stay in that while\nloop forever, and (b) because each iteration allocates memory to\naccommodate the new pattern, so we'll eventually run out of memory.\n\nThe wrong thing to do would be to handle this case by changing the\nconditional to \"if (!slash || slash == e->pattern)\", because we can't\nblindly carry forward some patterns which look like cone-mode patterns,\nsince together the list of sparse-checkout entries may not represent a\ncone.\n\n(An example here is if we added /foo/bar/baz/* without the corresponding\n/foo/, !/foo/*, and so on).\n\nSo I think the problem really is that we need to drop existing patterns\nwhen re-initializing the sparse-checkout in cone mode. We could try to\nrecognize that existing patterns may already constitute a cone (and/or\ncreate a cone that covers the existing patterns).\n\nBut I think the easiest thing (if a little unfriendly) would be to just\ndrop them and start afresh when re-initializing the sparse-checkout in\ncone mode.\n\nI'm adding Stolee to the CC to see if he thinks that would be sensible\nbehavior or not.\n\nThanks,\nTaylor\n"},{"id":"436439","messageId":"8a0ddd8e-b585-8f40-c4b1-0a51f11e6b84@gmail.com","threadId":"56545","inReplyTo":"YUiuWSXO1P3JwerH@nand.local","subject":"Re: Memory leak with sparse-checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-20T16:29:36Z","receivedAt":"2021-09-20T16:29:39Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/20/2021 11:52 AM, Taylor Blau wrote:\n> On Mon, Sep 20, 2021 at 09:45:14PM +0930, Calbabreaker wrote:\n>> What did you do before the bug happened? (Steps to reproduce your issue)\n>>\n>> This was ran:\n>>\n>> git clone https://github.com/Calbabreaker/piano --sparse\n>> cd piano\n>> git sparse-checkout add any_text\n>> git checkout deploy-frontend\n>> git sparse-checkout init --cone\n>> git sparse-checkout add any_text\n\nThank you for pointing this out.\n\nI'll point out that this was likely found because \"--sparse\" does not\ninitialize cone mode patterns, but you might have expected it to. This\nwill increase the priority of adding something like \"--sparse=cone\"\nto the 'git clone' options.\n\n> Thanks for the reproduction. An even simpler one may be (inside of any\n> repository):\n> \n> git sparse-checkout init\n> git sparse-checkout add dir\n> git sparse-checkout init --cone\n> git sparse-checkout add dir\n> \n> The problem occurs because we keep existing entries when adding to the\n> sparse-checkout list, and cone-mode patterns do not mix with\n> non cone-mode patterns.\n> \n> So after the first init and \"add dir\", your sparse-checkout file looks\n> like:\n> \n> /*\n> !/*/\n> dir\n> \n> but then when we convert to cone-mode and try and add \"dir\" (which in\n> cone-mode we'll convert to \"/dir/\"), we run into trouble when adding the\n> existing \"dir\" entry. That's because add_patterns_cone_mode() calls\n> insert_recursive_pattern() on every entry in the existing list,\n> including \"dir\".\n> \n> So when we call insert_recursive_pattern() with any pattern list and\n> path containing \"dir\", we first insert \"dir\" into the list, and then:\n> \n> char *slash = strrchr(e->pattern, '/');\n> char *oldpattern = e->pattern;\n> \n> if (slash == e->pattern)\n> break;\n> // trim off a slash, repeat\n> \n> except slash is NULL because \"dir\" doesn't contain a slash. And that\n> explains the problem you're seeing, because (a) we'll stay in that while\n> loop forever, and (b) because each iteration allocates memory to\n> accommodate the new pattern, so we'll eventually run out of memory.\n\nYikes! Thanks for digging into the details.\n\n> The wrong thing to do would be to handle this case by changing the\n> conditional to \"if (!slash || slash == e->pattern)\", because we can't\n> blindly carry forward some patterns which look like cone-mode patterns,\n> since together the list of sparse-checkout entries may not represent a\n> cone.\n> \n> (An example here is if we added /foo/bar/baz/* without the corresponding\n> /foo/, !/foo/*, and so on).\n> \n> So I think the problem really is that we need to drop existing patterns\n> when re-initializing the sparse-checkout in cone mode. We could try to\n> recognize that existing patterns may already constitute a cone (and/or\n> create a cone that covers the existing patterns).\n> \n> But I think the easiest thing (if a little unfriendly) would be to just\n> drop them and start afresh when re-initializing the sparse-checkout in\n> cone mode.\n\nThis isn't sufficient, as a user can modify their .git/info/sparse-checkout\nfile whenever they want, so we should fix this bug regardless. We could add\na \"Your existing patterns are not in cone mode\" error.\n\nIt might still be a good idea to let \"git sparse-checkout init --cone\"\noverwrite the sparse-checkout file _if the file is not already in cone\nmode_.\n\nThanks,\n-Stolee\n"},{"id":"436441","messageId":"YUi55/3L9nizTVyA@nand.local","threadId":"56545","inReplyTo":"8a0ddd8e-b585-8f40-c4b1-0a51f11e6b84@gmail.com","subject":"Re: Memory leak with sparse-checkout","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T16:42:15Z","receivedAt":"2021-09-20T16:42:19Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 12:29:36PM -0400, Derrick Stolee wrote:\n> > So I think the problem really is that we need to drop existing patterns\n> > when re-initializing the sparse-checkout in cone mode. We could try to\n> > recognize that existing patterns may already constitute a cone (and/or\n> > create a cone that covers the existing patterns).\n> >\n> > But I think the easiest thing (if a little unfriendly) would be to just\n> > drop them and start afresh when re-initializing the sparse-checkout in\n> > cone mode.\n>\n> This isn't sufficient, as a user can modify their .git/info/sparse-checkout\n> file whenever they want, so we should fix this bug regardless. We could add\n> a \"Your existing patterns are not in cone mode\" error.\n>\n> It might still be a good idea to let \"git sparse-checkout init --cone\"\n> overwrite the sparse-checkout file _if the file is not already in cone\n> mode_.\n\nI'm not sure how helpful such an error message might be to a user in\nthis scenario without extra information. After seeing just \"this isn't a\ncone\", it's not clear what they should do other than drop their\nsparse-checkout configuration and start over.\n\nIt would be nice to have an intermediate step between seeing realizing\nthat the existing patterns don't form a cone and dropping them.\n\nPerhaps we could include an error message and say something like:\n\n warning: your sparse-checkout patterns do not from a cone\n hint: to reinitialize your sparse-checkout configuration\n hint: try running:\n hint:\n hint: git sparse-checkout init --cone --reinitialize\n\nWhere `--reinitialize` means to drop existing patterns. I suppose it\ncould be the default when transitioning from non-cone to cone mode, but\nthat would defeat the purpose of the warning.\n\nWe would probably want to perform this check both during initialization,\nand when adding patterns in cone mode. It may also be worthwhile to\ncheck the validity of the cone before running 'list' or 'reapply', too.\n\nThanks,\nTaylor\n"},{"id":"436457","messageId":"b082f98b-eb49-7cc4-9f75-fe1ec480bd61@gmail.com","threadId":"56545","inReplyTo":"YUi55/3L9nizTVyA@nand.local","subject":"Re: Memory leak with sparse-checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-20T17:25:31Z","receivedAt":"2021-09-20T21:17:16Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/20/2021 12:42 PM, Taylor Blau wrote:\n> On Mon, Sep 20, 2021 at 12:29:36PM -0400, Derrick Stolee wrote:\n>>> So I think the problem really is that we need to drop existing patterns\n>>> when re-initializing the sparse-checkout in cone mode. We could try to\n>>> recognize that existing patterns may already constitute a cone (and/or\n>>> create a cone that covers the existing patterns).\n>>>\n>>> But I think the easiest thing (if a little unfriendly) would be to just\n>>> drop them and start afresh when re-initializing the sparse-checkout in\n>>> cone mode.\n>>\n>> This isn't sufficient, as a user can modify their .git/info/sparse-checkout\n>> file whenever they want, so we should fix this bug regardless. We could add\n>> a \"Your existing patterns are not in cone mode\" error.\n>>\n>> It might still be a good idea to let \"git sparse-checkout init --cone\"\n>> overwrite the sparse-checkout file _if the file is not already in cone\n>> mode_.\n> \n> I'm not sure how helpful such an error message might be to a user in\n> this scenario without extra information. After seeing just \"this isn't a\n> cone\", it's not clear what they should do other than drop their\n> sparse-checkout configuration and start over.\n> \n> It would be nice to have an intermediate step between seeing realizing\n> that the existing patterns don't form a cone and dropping them.\n> \n> Perhaps we could include an error message and say something like:\n> \n> warning: your sparse-checkout patterns do not from a cone\n> hint: to reinitialize your sparse-checkout configuration\n> hint: try running:\n> hint:\n> hint: git sparse-checkout init --cone --reinitialize\n> \n> Where `--reinitialize` means to drop existing patterns. I suppose it\n> could be the default when transitioning from non-cone to cone mode, but\n> that would defeat the purpose of the warning.\n\nI've got an initial set of commits in a GGG pull request [1], and I'm\nwaiting build validation as a double-check before sending them to the\nlist.\n\n[1] https://github.com/gitgitgadget/git/pull/1043\n\nThere, I think the best thing to do is have 'init --cone' overwrite the\nold, non-cone-mode patterns. This essentially makes your --reinitialize\nsuggestion on permanently in that case. Note that it doesn't reinitialize\nwhen the patterns are already in cone mode.\n\nNote that 'git sparse-checkout set' (with no other arguments) initializes\nthe sparse-checkout file with the default set of cone mode patterns, so\nwe have a way to do that now.\n \n> We would probably want to perform this check both during initialization,\n> and when adding patterns in cone mode. It may also be worthwhile to\n> check the validity of the cone before running 'list' or 'reapply', too.\n\n'list' definitely seems like a good idea, since it is expecting different\noutput than the literal patterns when cone mode is enabled.\n\n'reapply' should work in both cases, it will just use the old pattern\nmatching algorithm when in cone mode. It will present a warning when the\npatterns don't match as expected.\n\nOne thing that is very interesting about the example here is that a\npattern \"dir\" is not recognized as a non-cone-mode pattern, but it\nshould be. The fix is to look for a '/' at the start of the pattern and\nreject it otherwise. This is fixed in my series.\n\nThanks,\n-Stolee\n"},{"id":"436461","messageId":"b7ee5ff5-dfff-8d3f-36f6-b30daf2d71ec@gmail.com","threadId":"56545","inReplyTo":"b082f98b-eb49-7cc4-9f75-fe1ec480bd61@gmail.com","subject":"Re: Memory leak with sparse-checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-20T17:27:25Z","receivedAt":"2021-09-20T21:35:12Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/20/2021 1:25 PM, Derrick Stolee wrote:\n> On 9/20/2021 12:42 PM, Taylor Blau wrote:\n>> We would probably want to perform this check both during initialization,\n>> and when adding patterns in cone mode. It may also be worthwhile to\n>> check the validity of the cone before running 'list' or 'reapply', too.\n> \n> 'list' definitely seems like a good idea, since it is expecting different\n> output than the literal patterns when cone mode is enabled.\n\nI double-checked this to see how to fix this, and the 'list' subcommand\nalready notices that the patterns are not in cone mode and reverts its\nbehavior to writing all of the sparse-checkout file to stdout. It also\nwrites warnings over stderr before that.\n\nThere might not be anything pressing to do here.\n\nThanks,\n-Stolee\n"},{"id":"436491","messageId":"427c6d86-f123-035e-b0e6-4a21598ed111@gmail.com","threadId":"56545","inReplyTo":"YUjcMu7Z094eaFRA@nand.local","subject":"Re: Memory leak with sparse-checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-20T20:56:47Z","receivedAt":"2021-09-21T01:44:30Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/20/2021 3:08 PM, Taylor Blau wrote:\n> On Mon, Sep 20, 2021 at 01:27:25PM -0400, Derrick Stolee wrote:\n>> On 9/20/2021 1:25 PM, Derrick Stolee wrote:\n>>> On 9/20/2021 12:42 PM, Taylor Blau wrote:\n>>>> We would probably want to perform this check both during initialization,\n>>>> and when adding patterns in cone mode. It may also be worthwhile to\n>>>> check the validity of the cone before running 'list' or 'reapply', too.\n>>>\n>>> 'list' definitely seems like a good idea, since it is expecting different\n>>> output than the literal patterns when cone mode is enabled.\n>>\n>> I double-checked this to see how to fix this, and the 'list' subcommand\n>> already notices that the patterns are not in cone mode and reverts its\n>> behavior to writing all of the sparse-checkout file to stdout. It also\n>> writes warnings over stderr before that.\n>>\n>> There might not be anything pressing to do here.\n> \n> Hmm. I think we'd probably want the same behavior for init and any other\n> commands which could potentially overwrite the contents of the\n> sparse-checkout file.\n\nCould you elaborate on what you mean by \"the same behavior\"?\n\nDo you mean that \"git sparse-checkout add X\" should act as if cone mode\nis not enabled if the existing patterns are not cone-mode patterns?\n\nWhat exactly do you mean about \"init\" changing behavior here?\n \n> Those may already call list routines internally, in which case I agree\n> that this is already taken care of. But if not, then I think we should\n> match list's behavior in the new locations, too.\n\n\"list\" interprets the 'struct pattern_list' in two different ways,\ndepending on the use_cone_patterns member. They are static methods in\nthe builtin code, not used by anything else.\n\nThanks,\n-Stolee\n"},{"id":"436494","messageId":"YUj7GN/qWhw67jyk@nand.local","threadId":"56545","inReplyTo":"427c6d86-f123-035e-b0e6-4a21598ed111@gmail.com","subject":"Re: Memory leak with sparse-checkout","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T21:20:24Z","receivedAt":"2021-09-21T02:20:01Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 04:56:47PM -0400, Derrick Stolee wrote:\n> >> I double-checked this to see how to fix this, and the 'list' subcommand\n> >> already notices that the patterns are not in cone mode and reverts its\n> >> behavior to writing all of the sparse-checkout file to stdout. It also\n> >> writes warnings over stderr before that.\n> >>\n> >> There might not be anything pressing to do here.\n> >\n> > Hmm. I think we'd probably want the same behavior for init and any other\n> > commands which could potentially overwrite the contents of the\n> > sparse-checkout file.\n>\n> Could you elaborate on what you mean by \"the same behavior\"?\n>\n> Do you mean that \"git sparse-checkout add X\" should act as if cone mode\n> is not enabled if the existing patterns are not cone-mode patterns?\n>\n> What exactly do you mean about \"init\" changing behavior here?\n\nNo, I was referring to your suggestion from [1] to add a warning from\n\"git sparse-checkout init --cone\" when there are existing patterns which\nare not in cone-mode.\n\n> > Those may already call list routines internally, in which case I agree\n> > that this is already taken care of. But if not, then I think we should\n> > match list's behavior in the new locations, too.\n>\n> \"list\" interprets the 'struct pattern_list' in two different ways,\n> depending on the use_cone_patterns member. They are static methods in\n> the builtin code, not used by anything else.\n\nAh, bummer. I was hoping that they'd be used internally by init so that\nit would automatically emit a warning in the case where a user's\nexisting patterns are not in cone mode.\n\nApologies for any confusion.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/8a0ddd8e-b585-8f40-c4b1-0a51f11e6b84@gmail.com/\n"},{"id":"436550","messageId":"YUjcMu7Z094eaFRA@nand.local","threadId":"56545","inReplyTo":"b7ee5ff5-dfff-8d3f-36f6-b30daf2d71ec@gmail.com","subject":"Re: Memory leak with sparse-checkout","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-20T19:08:34Z","receivedAt":"2021-09-21T02:59:02Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Sep 20, 2021 at 01:27:25PM -0400, Derrick Stolee wrote:\n> On 9/20/2021 1:25 PM, Derrick Stolee wrote:\n> > On 9/20/2021 12:42 PM, Taylor Blau wrote:\n> >> We would probably want to perform this check both during initialization,\n> >> and when adding patterns in cone mode. It may also be worthwhile to\n> >> check the validity of the cone before running 'list' or 'reapply', too.\n> >\n> > 'list' definitely seems like a good idea, since it is expecting different\n> > output than the literal patterns when cone mode is enabled.\n>\n> I double-checked this to see how to fix this, and the 'list' subcommand\n> already notices that the patterns are not in cone mode and reverts its\n> behavior to writing all of the sparse-checkout file to stdout. It also\n> writes warnings over stderr before that.\n>\n> There might not be anything pressing to do here.\n\nHmm. I think we'd probably want the same behavior for init and any other\ncommands which could potentially overwrite the contents of the\nsparse-checkout file.\n\nThose may already call list routines internally, in which case I agree\nthat this is already taken care of. But if not, then I think we should\nmatch list's behavior in the new locations, too.\n\nThanks,\nTaylor\n"},{"id":"436600","messageId":"734ecf93-e563-20d5-7cf1-74048aa74d56@gmail.com","threadId":"56545","inReplyTo":"YUj7GN/qWhw67jyk@nand.local","subject":"Re: Memory leak with sparse-checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-21T12:55:01Z","receivedAt":"2021-09-21T12:55:06Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/20/2021 5:20 PM, Taylor Blau wrote:\n> On Mon, Sep 20, 2021 at 04:56:47PM -0400, Derrick Stolee wrote:\n>>>> I double-checked this to see how to fix this, and the 'list' subcommand\n>>>> already notices that the patterns are not in cone mode and reverts its\n>>>> behavior to writing all of the sparse-checkout file to stdout. It also\n>>>> writes warnings over stderr before that.\n>>>>\n>>>> There might not be anything pressing to do here.\n>>>\n>>> Hmm. I think we'd probably want the same behavior for init and any other\n>>> commands which could potentially overwrite the contents of the\n>>> sparse-checkout file.\n>>\n>> Could you elaborate on what you mean by \"the same behavior\"?\n>>\n>> Do you mean that \"git sparse-checkout add X\" should act as if cone mode\n>> is not enabled if the existing patterns are not cone-mode patterns?\n>>\n>> What exactly do you mean about \"init\" changing behavior here?\n> \n> No, I was referring to your suggestion from [1] to add a warning from\n> \"git sparse-checkout init --cone\" when there are existing patterns which\n> are not in cone-mode.\n\nThis warning is part of the sparse-checkout pattern parsing logic, so\nit happens whenever the patterns are loaded, including the \"list\"\nsubcommand (among other commands, not just the sparse-checkout builtin).\n\n>>> Those may already call list routines internally, in which case I agree\n>>> that this is already taken care of. But if not, then I think we should\n>>> match list's behavior in the new locations, too.\n>>\n>> \"list\" interprets the 'struct pattern_list' in two different ways,\n>> depending on the use_cone_patterns member. They are static methods in\n>> the builtin code, not used by anything else.\n> \n> Ah, bummer. I was hoping that they'd be used internally by init so that\n> it would automatically emit a warning in the case where a user's\n> existing patterns are not in cone mode.\n> \n> Apologies for any confusion.\n\nThanks for clearing it up!\n\n-Stolee\n"},{"id":"436631","messageId":"YUoJGV0wj0ba7n8X@nand.local","threadId":"56545","inReplyTo":"734ecf93-e563-20d5-7cf1-74048aa74d56@gmail.com","subject":"Re: Memory leak with sparse-checkout","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-21T16:32:25Z","receivedAt":"2021-09-21T16:32:28Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 21, 2021 at 08:55:01AM -0400, Derrick Stolee wrote:\n> On 9/20/2021 5:20 PM, Taylor Blau wrote:\n> > On Mon, Sep 20, 2021 at 04:56:47PM -0400, Derrick Stolee wrote:\n> >>>> I double-checked this to see how to fix this, and the 'list' subcommand\n> >>>> already notices that the patterns are not in cone mode and reverts its\n> >>>> behavior to writing all of the sparse-checkout file to stdout. It also\n> >>>> writes warnings over stderr before that.\n> >>>>\n> >>>> There might not be anything pressing to do here.\n> >>>\n> >>> Hmm. I think we'd probably want the same behavior for init and any other\n> >>> commands which could potentially overwrite the contents of the\n> >>> sparse-checkout file.\n> >>\n> >> Could you elaborate on what you mean by \"the same behavior\"?\n> >>\n> >> Do you mean that \"git sparse-checkout add X\" should act as if cone mode\n> >> is not enabled if the existing patterns are not cone-mode patterns?\n> >>\n> >> What exactly do you mean about \"init\" changing behavior here?\n> >\n> > No, I was referring to your suggestion from [1] to add a warning from\n> > \"git sparse-checkout init --cone\" when there are existing patterns which\n> > are not in cone-mode.\n>\n> This warning is part of the sparse-checkout pattern parsing logic, so\n> it happens whenever the patterns are loaded, including the \"list\"\n> subcommand (among other commands, not just the sparse-checkout builtin).\n\nI might be misunderstanding what you're saying. But what I'm wondering\nis: if we detect when existing patterns aren't in cone-mode, why didn't\nwe catch that case originally when the memory leak was discovered?\n\nI thought that it might have been related to your third patch to change\nhow bad patterns are detected. But I ran the following script after\napplying each of your three patches individually:\n\n rm -fr repo\n git init repo\n cd repo\n\n git sparse-checkout init\n git sparse-checkout add foo\n git sparse-checkout init --cone\n git sparse-checkout add foo\n\nand the only difference is that we started silently dropping the bad\n\"foo\" pattern after re-adding foo in cone-mode starting with the second\npatch.\n\nI guess my question is: it seems like there may be a friendlier way to\ntell the user that we're about to drop their sparse-checkout definition\ninstead of just doing it silently. And it seems like you're saying that\nwe already have something that detects that and is used everywhere. But\nI wonder why it wasn't kicking in in the original report.\n\nThanks,\nTaylor\n"},{"id":"436656","messageId":"3c9af4e9-f3db-99af-d875-fb11bc8a643e@gmail.com","threadId":"56545","inReplyTo":"YUoJGV0wj0ba7n8X@nand.local","subject":"Re: Memory leak with sparse-checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-21T18:56:01Z","receivedAt":"2021-09-21T18:56:06Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/21/2021 12:32 PM, Taylor Blau wrote:\n> On Tue, Sep 21, 2021 at 08:55:01AM -0400, Derrick Stolee wrote:\n>> On 9/20/2021 5:20 PM, Taylor Blau wrote:\n>>> On Mon, Sep 20, 2021 at 04:56:47PM -0400, Derrick Stolee wrote:\n>>>>>> I double-checked this to see how to fix this, and the 'list' subcommand\n>>>>>> already notices that the patterns are not in cone mode and reverts its\n>>>>>> behavior to writing all of the sparse-checkout file to stdout. It also\n>>>>>> writes warnings over stderr before that.\n>>>>>>\n>>>>>> There might not be anything pressing to do here.\n>>>>>\n>>>>> Hmm. I think we'd probably want the same behavior for init and any other\n>>>>> commands which could potentially overwrite the contents of the\n>>>>> sparse-checkout file.\n>>>>\n>>>> Could you elaborate on what you mean by \"the same behavior\"?\n>>>>\n>>>> Do you mean that \"git sparse-checkout add X\" should act as if cone mode\n>>>> is not enabled if the existing patterns are not cone-mode patterns?\n>>>>\n>>>> What exactly do you mean about \"init\" changing behavior here?\n>>>\n>>> No, I was referring to your suggestion from [1] to add a warning from\n>>> \"git sparse-checkout init --cone\" when there are existing patterns which\n>>> are not in cone-mode.\n>>\n>> This warning is part of the sparse-checkout pattern parsing logic, so\n>> it happens whenever the patterns are loaded, including the \"list\"\n>> subcommand (among other commands, not just the sparse-checkout builtin).\n> \n> I might be misunderstanding what you're saying. But what I'm wondering\n> is: if we detect when existing patterns aren't in cone-mode, why didn't\n> we catch that case originally when the memory leak was discovered?\n\nEasy answer: there was a bug. The pattern in the original report evaded\nthe checks that were implemented to identify non-cone-mode patterns.\n\nMy patch 3 [1] fixes that problem, with this hunk:\n\ndiff --git a/dir.c b/dir.c\nindex 03c4d212672..93136442103 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -727,7 +727,7 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern\n \t}\n \n \tif (given->patternlen < 2 ||\n-\t *given->pattern == '*' ||\n+\t *given->pattern != '/' ||\n \t strstr(given->pattern, \"**\")) {\n \t\t/* Not a cone pattern. */\n \t\twarning(_(\"unrecognized pattern: '%s'\"), given->pattern);\n\nand a test case change is required to avoid having the warning message\nappear in the wrong places:\n\n@@ -182,9 +185,9 @@ test_expect_success 'set sparse-checkout using --stdin' '\n test_expect_success 'add to sparse-checkout' '\n \tcat repo/.git/info/sparse-checkout >expect &&\n \tcat >add <<-\\EOF &&\n-\tpattern1\n+\t/pattern1\n \t/folder1/\n-\tpattern2\n+\t/pattern2\n \tEOF\n \tcat add >>expect &&\n \tgit -C repo sparse-checkout add --stdin I thought that it might have been related to your third patch to change\n> how bad patterns are detected. But I ran the following script after\n> applying each of your three patches individually:\n> \n> rm -fr repo\n> git init repo\n> cd repo\n> \n> git sparse-checkout init\n> git sparse-checkout add foo\n> git sparse-checkout init --cone\n> git sparse-checkout add foo\n> \n> and the only difference is that we started silently dropping the bad\n> \"foo\" pattern after re-adding foo in cone-mode starting with the second\n> patch.\n\nIn patch 2, we \"detect\" that the old patterns were not in cone mode\nbecause the core.sparseCheckoutCone config is false when parsing the\npatterns, so use_cone_patterns is 0.\n\n> I guess my question is: it seems like there may be a friendlier way to\n> tell the user that we're about to drop their sparse-checkout definition\n> instead of just doing it silently. And it seems like you're saying that\n> we already have something that detects that and is used everywhere. But\n> I wonder why it wasn't kicking in in the original report.\n\nYou are correct that we should make better documentation around the\nre-initialization of patterns. And this _is_ a change of behavior and\nI will cave to concerns around that. I think this is more a case of\nmatching what users expect from the interface, or at minimum helping\nthem fall into the \"pit of success\".\n\nLet's move the discussion to that thread so we can interleave the\npatches themselves.\n\nThanks,\n-Stolee\n"},{"id":"436664","messageId":"YUpEX7mmZ0WPvbYK@nand.local","threadId":"56545","inReplyTo":"3c9af4e9-f3db-99af-d875-fb11bc8a643e@gmail.com","subject":"Re: Memory leak with sparse-checkout","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-21T20:45:19Z","receivedAt":"2021-09-21T20:45:23Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Tue, Sep 21, 2021 at 02:56:01PM -0400, Derrick Stolee wrote:\n> > I thought that it might have been related to your third patch to change\n> > how bad patterns are detected. But I ran the following script after\n> > applying each of your three patches individually:\n> >\n> > rm -fr repo\n> > git init repo\n> > cd repo\n> >\n> > git sparse-checkout init\n> > git sparse-checkout add foo\n> > git sparse-checkout init --cone\n> > git sparse-checkout add foo\n> >\n> > and the only difference is that we started silently dropping the bad\n> > \"foo\" pattern after re-adding foo in cone-mode starting with the second\n> > patch.\n>\n> In patch 2, we \"detect\" that the old patterns were not in cone mode\n> because the core.sparseCheckoutCone config is false when parsing the\n> patterns, so use_cone_patterns is 0.\n\nI fear that we're talking about different things. With your patches, if\nI munge my .git/info/sparse-checkout file, I can easily get something\nlike:\n\n $ git.compile sparse-checkout list\n warning: unrecognized pattern: 'foo'\n warning: disabling cone pattern matching\n\nto appear. But I'm wondering why the same doesn't happen when running\n`git sparse-checkout init --cone` while the existing sparse-checkout\ndefinition contains non-cone mode entries.\n\nI could be holding it wrong, but I was unsuccessful in getting a warning\nout with the quoted script.\n\n> Let's move the discussion to that thread so we can interleave the\n> patches themselves.\n\nSure, if you want. I just found it easier to reply here since I can more\nreadily quote the things I want to respond to.\n\nThanks,\nTaylor\n"},{"id":"436756","messageId":"98bd3b94-eb8d-b074-b89d-d86dcefaa995@gmail.com","threadId":"56545","inReplyTo":"YUpEX7mmZ0WPvbYK@nand.local","subject":"Re: Memory leak with sparse-checkout","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-09-22T19:16:22Z","receivedAt":"2021-09-22T19:16:26Z","isPatch":false,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 9/21/2021 4:45 PM, Taylor Blau wrote:\n> On Tue, Sep 21, 2021 at 02:56:01PM -0400, Derrick Stolee wrote:\n>>> I thought that it might have been related to your third patch to change\n>>> how bad patterns are detected. But I ran the following script after\n>>> applying each of your three patches individually:\n>>>\n>>> rm -fr repo\n>>> git init repo\n>>> cd repo\n>>>\n>>> git sparse-checkout init\n>>> git sparse-checkout add foo\n>>> git sparse-checkout init --cone\n>>> git sparse-checkout add foo\n>>>\n>>> and the only difference is that we started silently dropping the bad\n>>> \"foo\" pattern after re-adding foo in cone-mode starting with the second\n>>> patch.\n>>\n>> In patch 2, we \"detect\" that the old patterns were not in cone mode\n>> because the core.sparseCheckoutCone config is false when parsing the\n>> patterns, so use_cone_patterns is 0.\n> \n> I fear that we're talking about different things. With your patches, if\n> I munge my .git/info/sparse-checkout file, I can easily get something\n> like:\n> \n> $ git.compile sparse-checkout list\n> warning: unrecognized pattern: 'foo'\n> warning: disabling cone pattern matching\n> \n> to appear. But I'm wondering why the same doesn't happen when running\n> `git sparse-checkout init --cone` while the existing sparse-checkout\n> definition contains non-cone mode entries.\n\nYou don't get that warning because it's not trying to parse the\nprevious patterns using cone mode. Before my series, you would get\nthe warning in a _second_ run of \"git sparse-checkout init --cone\".\n\nWe should add a different warning for overwriting the existing\npatterns.\n\nThanks,\n-Stolee\n"},{"id":"436761","messageId":"YUuF4SKO4lA/xLKZ@nand.local","threadId":"56545","inReplyTo":"98bd3b94-eb8d-b074-b89d-d86dcefaa995@gmail.com","subject":"Re: Memory leak with sparse-checkout","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2021-09-22T19:37:05Z","receivedAt":"2021-09-22T19:37:09Z","isPatch":false,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Wed, Sep 22, 2021 at 03:16:22PM -0400, Derrick Stolee wrote:\n> On 9/21/2021 4:45 PM, Taylor Blau wrote:\n> [...]\n> > I fear that we're talking about different things. With your patches, if\n> > I munge my .git/info/sparse-checkout file, I can easily get something\n> > like:\n> >\n> > $ git.compile sparse-checkout list\n> > warning: unrecognized pattern: 'foo'\n> > warning: disabling cone pattern matching\n> >\n> > to appear. But I'm wondering why the same doesn't happen when running\n> > `git sparse-checkout init --cone` while the existing sparse-checkout\n> > definition contains non-cone mode entries.\n>\n> You don't get that warning because it's not trying to parse the\n> previous patterns using cone mode. Before my series, you would get\n> the warning in a _second_ run of \"git sparse-checkout init --cone\".\n\nOK. I was confused because in [1] you said that we produce this error\nany time we try to load the existing definitions and parse them in\ncone-mode.\n\nI would have assumed that we would try to do that during `init --cone`\nto read the existing sparse-checkout definition.\n\nThanks,\nTaylor\n\n[1]: https://lore.kernel.org/git/734ecf93-e563-20d5-7cf1-74048aa74d56@gmail.com/\n"}]}