git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Mike Hommey <mh@glandium.org>
To: git@vger.kernel.org
Subject: revision.c alters commit object state ; also no cleanup
Date: Mon, 26 Aug 2019 17:58:44 +0900	[thread overview]
Message-ID: <20190826085844.ue77hen2z6rggpe5@glandium.org> (raw)

Hi,

First, a little context: As you may have noticed, I've recently found a
small bunch of memory leaks in different parts of the code base. The
reason I did is that git-cinnabar[1] uses libgit.a, and I very recently
upgraded its CI to use a slightly more recent version of GCC, which
either enabled leak detection by default in ASAN or detect more leaks
than before. Anyways, my CI was busted because of leaks detected after
the upgrade, which made me look around.

git-cinnabar does make some specific uses of libgit.a that git itself
doesn't, and this is where things kind of fall apart:

First, revision.c doesn't come with a function to clear a struct
rev_info. I did create a function that does enough cleanup for my own
use (which turned out to be not enough), but I'm wondering if git
shouldn't itself have one in revision.c (and seeing that there's now
e.g. repo_clear, which didn't exist when I started git-cinnabar, I think
the answer would be that it should).

Then, revision.c kind of does nasty things to commit objects:
- remove_duplicate_parents alters commit->parents to skip duplicates[2]
- try_to_simplify_commit removes all parents but one[3] or all of them
  [4] in some cases

That's for the case that I did encounter. Maybe there's more.

One problem this causes is that it leaks the commit_list items that used
to contain the removed parents.

Another problem is that if something else uses the commits afterwards,
those commits parents don't match what you'd get before revision
walking. And I don't know how this should be addressed.

Ideas welcome.

Relatedly, the subthread that started at
<xmqqlfvlne3k.fsf@gitster-ct.c.googlers.com> is kind of relevant. There
are two main reasons I'm using libgit.a: one is that I'm doing things
low-level enough that I don't know any other library that would work for
my use case. Another is that I was hoping that in the long term, it
could be merged into git.git. I guess the more time passes, the less
likely the latter is to happen. Which is fine, but it's good to know.

Mike


1. https://github.com/glandium/git-cinnabar/
2. https://github.com/git/git/blob/745f6812895b31c02b29bdfe4ae8e5498f776c26/revision.c#L2742
3. https://github.com/git/git/blob/745f6812895b31c02b29bdfe4ae8e5498f776c26/revision.c#L869
4. https://github.com/git/git/blob/745f6812895b31c02b29bdfe4ae8e5498f776c26/revision.c#L888

             reply	other threads:[~2019-08-26  8:58 UTC|newest]

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-26  8:58 Mike Hommey [this message]
2019-08-26 18:14 ` revision.c alters commit object state ; also no cleanup Junio C Hamano
2019-08-27 23:21   ` Mike Hommey

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20190826085844.ue77hen2z6rggpe5@glandium.org \
    --to=mh@glandium.org \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).