Re: [PATCH] fast-import: fix over-allocation of marks storage

From: Jeff King <peff@peff.net>
Date: Oct 15, 2020, 18:58 UTC
Message-ID: <20201015185853.GA1108210@coredump.intra.peff.net>
In-Reply-To: <xmqqo8l3fibz.fsf@gitster.c.googlers.com>

On Thu, Oct 15, 2020 at 11:35:28AM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> "Dipl. Ing. Sergey Brester" <serg.brester@sebres.de> writes:
> 
> > May be this is a sign to introduce real issue tracker finally? :)
> > No offence, but I was always wondering how a team is able to hold all
> > the issue related stuff in form
> > of a mailing list, without to experience such an "embarrassments".
> > Especially on such large projects and communities.
> 
> I do not know if an issue-tracker would have helped, though.  The
> issue was discovered and discussed there the day before:
> 
>   https://lore.kernel.org/git/xmqqimg5o5fq.fsf@gitster.c.googlers.com/

Doh, and I was so proud of myself for diagnosing and fixing it. ;)

I hadn't read either of the threads you linked before today (I found them in my "catch up on list reading" queue, though likely I would have declared bankruptcy before reading them anyway).

At least that explains my surprise that the issue was not reported earlier. It was. :)

IMHO an issue tracker wouldn't really change things here. The original can be found in the first page of results of:

  https://lore.kernel.org/git/?q=fast-import+leak

(though if you add "-cooking -announce" there is even less noise). I don't know that searching an issue tracker would do much better.

Show 9 quoted lines

> By the way, now I know why it looked familiar---the fix largely was
> my code.  And the diff between Brian's from June and Peff's in this
> thread is indeed quite small (shown below), which actually worries
> me.  Was there something in the old attempt that was incomplete that
> made us wait for the final finishing touches?  If so, is the current
> round missing the same thing?  Or perhaps the test was what was
> missing in the old attempt, in which case it's perfect (in the
> attached diff, I excluded t/ directroy as the old fix didn't have
> tests).

Looking over the thread, I don't see any problems pointed out (though as your diff below shows, the original patch missed the re-ordering required for the submodule mapping call).

So I'd prefer my patch because of that fix and because of the tests.

-Peff