{"thread":{"id":"51739","subject":"revision.c alters commit object state ; also no cleanup","startedAt":"2019-08-26T08:58:49Z","lastAt":"2019-08-27T23:21:16Z","messageCount":3,"participants":["Mike Hommey","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"381168","messageId":"20190826085844.ue77hen2z6rggpe5@glandium.org","threadId":"51739","inReplyTo":null,"subject":"revision.c alters commit object state ; also no cleanup","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-08-26T08:58:44Z","receivedAt":"2019-08-26T08:58:49Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"Hi,\n\nFirst, a little context: As you may have noticed, I've recently found a\nsmall bunch of memory leaks in different parts of the code base. The\nreason I did is that git-cinnabar[1] uses libgit.a, and I very recently\nupgraded its CI to use a slightly more recent version of GCC, which\neither enabled leak detection by default in ASAN or detect more leaks\nthan before. Anyways, my CI was busted because of leaks detected after\nthe upgrade, which made me look around.\n\ngit-cinnabar does make some specific uses of libgit.a that git itself\ndoesn't, and this is where things kind of fall apart:\n\nFirst, revision.c doesn't come with a function to clear a struct\nrev_info. I did create a function that does enough cleanup for my own\nuse (which turned out to be not enough), but I'm wondering if git\nshouldn't itself have one in revision.c (and seeing that there's now\ne.g. repo_clear, which didn't exist when I started git-cinnabar, I think\nthe answer would be that it should).\n\nThen, revision.c kind of does nasty things to commit objects:\n- remove_duplicate_parents alters commit->parents to skip duplicates[2]\n- try_to_simplify_commit removes all parents but one[3] or all of them\n [4] in some cases\n\nThat's for the case that I did encounter. Maybe there's more.\n\nOne problem this causes is that it leaks the commit_list items that used\nto contain the removed parents.\n\nAnother problem is that if something else uses the commits afterwards,\nthose commits parents don't match what you'd get before revision\nwalking. And I don't know how this should be addressed.\n\nIdeas welcome.\n\nRelatedly, the subthread that started at\n is kind of relevant. There\nare two main reasons I'm using libgit.a: one is that I'm doing things\nlow-level enough that I don't know any other library that would work for\nmy use case. Another is that I was hoping that in the long term, it\ncould be merged into git.git. I guess the more time passes, the less\nlikely the latter is to happen. Which is fine, but it's good to know.\n\nMike\n\n\n1. https://github.com/glandium/git-cinnabar/\n2. https://github.com/git/git/blob/745f6812895b31c02b29bdfe4ae8e5498f776c26/revision.c#L2742\n3. https://github.com/git/git/blob/745f6812895b31c02b29bdfe4ae8e5498f776c26/revision.c#L869\n4. https://github.com/git/git/blob/745f6812895b31c02b29bdfe4ae8e5498f776c26/revision.c#L888\n"},{"id":"381224","messageId":"xmqq7e6zhk0a.fsf@gitster-ct.c.googlers.com","threadId":"51739","inReplyTo":"20190826085844.ue77hen2z6rggpe5@glandium.org","subject":"Re: revision.c alters commit object state ; also no cleanup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-26T18:14:13Z","receivedAt":"2019-08-26T18:14:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mike Hommey writes:\n\n> First, revision.c doesn't come with a function to clear a struct\n> rev_info....\n> Then, revision.c kind of does nasty things to commit objects...\n\nYeah, these two stem from the \"run once and let exit() clean things\nup\" design the oldest part of the system shares. Regarding the\nlatter, i.e. parent rewriting, I recall that there is a codepath\nthat saves the original true parents away in a second field when we\nknow we would want to show it (and avoid the overhead of copying\nwhen we do not have to), so you should be able to extend it to keep\nthe original parent list.\n\n"},{"id":"381407","messageId":"20190827232110.f3xdnr3i4e2aqenn@glandium.org","threadId":"51739","inReplyTo":"xmqq7e6zhk0a.fsf@gitster-ct.c.googlers.com","subject":"Re: revision.c alters commit object state ; also no cleanup","fromName":"Mike Hommey","fromEmail":"mh@glandium.org","sentAt":"2019-08-27T23:21:10Z","receivedAt":"2019-08-27T23:21:16Z","isPatch":false,"sender":{"key":"mh@glandium.org","avatar":"https://avatars.githubusercontent.com/u/1038527?v=4"},"body":"On Mon, Aug 26, 2019 at 11:14:13AM -0700, Junio C Hamano wrote:\n> Mike Hommey writes:\n> \n> > First, revision.c doesn't come with a function to clear a struct\n> > rev_info....\n> > Then, revision.c kind of does nasty things to commit objects...\n> \n> Yeah, these two stem from the \"run once and let exit() clean things\n> up\" design the oldest part of the system shares. Regarding the\n> latter, i.e. parent rewriting, I recall that there is a codepath\n> that saves the original true parents away in a second field when we\n> know we would want to show it (and avoid the overhead of copying\n> when we do not have to), so you should be able to extend it to keep\n> the original parent list.\n\nI guess you're refering to save_parents/get_saved_parents? Would it make\nsense for things to be reversed, as in, make revision.c keep the\nsimplified parents separately, and traverse through that?\n\nMike\n"}]}