This should be ready for review. PTAL!

This looks good to me. @carlosmn - are there any subtleties in fixing thin packs?

There often are subtleties. I'd like us to do the same kind of check that git does in this case in order to detect when no progress can be made. I'd have to get back to the details, but I'm not sure that we can assert we're stuck just because a single round did no yield any progress.

IIUC git does this by making sure each unresolved delta is fully resolved before resolving the next one. at its core, there's a check that verifies that progress was made in the last two rounds (similar in spirit to the stuck flag I'm proposing). It's probably doable to restructure libgit2's code to make it behave closer to that, but it might take a bit more work.

I did try to find a good scenario in which to test the double free() that didn't involve a malformed packfile to avoid having the fix depend on the stuck logic (otherwise the test never finishes), but I was unfortunately unable to find anything :(

Is this a security hole?

@DemiMarie yes, if you feed untrusted/arbitrary packfile data into the git_indexer_append function.

@carlosmn sorry for the delay (almost a year!?). i think we can get away without doing significant refactoring by either:

• checking that the inserted object will effectively resolve the delta, or
• by tagging each delta as having been resolved, and only process each delta once in fix_thin_pack, which avoids having a coarse two-try attempt.

i already have the latter coded with tests passing, but want to explore the former since it seems more robust and might catch more errors.

@carlosmn all right, found a much more elegant solution! PTAAL.

@carlosmn @ethomson @pks-t gentle ping? This is the only thing missing before the OSS-Fuzz support can be committed.

Sorry for the delay - this looks good to me. 😁

Would you mind squashing this? I'll

I do not mind! Squashed and rebased.

Thanks! 👍 ✨