On Tue, Aug 02, 2022 at 05:33:13PM +0200, Ævar Arnfjörð Bjarmason wrote:
> Let's fix this memory leak. Now when we encounter an OBJ_COMMIT we
> save away the "rev.pending" before clearing it. We then add a single
> commit to it, which our indirect invocation of prepare_revision_walk()
> will remove. After that we restore the "rev.pending".
>
> Our "rev.pending" will then get free'd by the release_revisions()
> added in f6bfea0ad01 (revisions API users: use release_revisions() in
> builtin/log.c, 2022-04-13)
OK, I agree this is doing the correct thing. I actually found the
earlier "let's dissociate rev.pending from rev entirely" approach easier
to reason about, though.
> diff --git a/builtin/log.c b/builtin/log.c
> index 88a5e98875a..b4b1d974617 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -743,11 +743,17 @@ int cmd_show(int argc, const char **argv, const char *prefix)
> rev.shown_one = 1;
> break;
> case OBJ_COMMIT:
> + {
> + struct object_array old;
> +
> + memcpy(&old, &rev.pending, sizeof(old));
> rev.pending.nr = rev.pending.alloc = 0;
> rev.pending.objects = NULL;
> add_object_array(o, name, &rev.pending);
> ret = cmd_log_walk_no_free(&rev);
> + memcpy(&rev.pending, &old, sizeof(rev.pending));
> break;
> + }
Here we overwrite the one-item rev.pending without freeing it, but just
immediately after instead of before. It's a little subtle, but your
comment in the commit message:
[...] and only free the new "rev.pending" in the "OBJ_COMMIT" case arm
as prepare_revision_walk() would draw it down.
covers that. IMHO that could be spelled out a bit more (particularly
that this only works for OBJ_COMMIT, but that's OK because that's the
only type we're adding), but I can live with it.
-Peff
