On Sun, Aug 25, 2019 at 1:06 AM Jeff King <peff@xxxxxxxx> wrote: > > On Sun, Aug 25, 2019 at 02:57:48AM -0400, Jeff King wrote: > > > And I think this is actually a real bug in the current code! We keep a > > pointer to the encoding string, which survives because of the history. > > But that history is bounded, and we could have an indefinite number of > > changed files in the middle. If I modify t9300 like this: > > Here are two patches. The first fixes the existing bug with "encoding", > and the second uses the approach I suggested to fix the leak you > noticed. > > The second one does carry a greater risk of regression than your patch, > but I think it's worth it for the fact that it makes any other bugs > (like the "encoding" one) more obvious. I agree, both patches look good to me, and I particularly appreciate some extra help to avoid making the same mistake again. :-) Just for good measure, I also went and tested these patches by running the git filter-repo testsuite and by re-running the filter-repo timing cases at https://public-inbox.org/git/CABPp-BGOz8nks0+Tdw5GyGqxeYR-3FF6FT5JcgVqZDYVRQ6qog@xxxxxxxxxxxxxx/. While filter-repo only uses a subset of fast-export functionality, it tests it with a variety of weird and unusual tiny test repos. And the timing cases run on three real-world repositories (git, rails, and linux). Everything looks good on all of these testcases.
