fix: return libgit error rather than asserting in Repository::submodules

[WillLillis]

The Problem

When calling Repository::submodules concurrently with another git process on the same repository, the assertion on git_submodule_lookup's return code can be reached (most often with a value of -3, sometimes -1), causing a crash. I've found this can be mitigated against by acquiring the repository's .git/index.lock file before calling Repository::submodules, but the assert can still be triggered, albeit more rarely. I've created a standalone reproducer repo for the issue here: https://github.com/WillLillis/git2-submodule-race.

The Solution

Return the error from the callback instead of asserting.

Considerations

• The doc comment for git_submodule_foreach states that a nonzero return from the callback will halt iteration.

• I checked the source of git_submodule_lookup and the out pointer (raw, in the callback) is never written to in the error case, so there's no need to clean it up in the callback on error (i.e. with raw::git_submodule_free).

• I didn't think it would be appropriate to add as a test, since this isn't possible to test deterministically.

• If there is some other reason not to return an error here, would it make sense to add a # Panics section to the function's doc comment indicating that it can assert?

Thanks for maintaining such a great project!

[weihanglo]

Looks reasonable. Thank you for the fix and the comprehensive reproducible example!

View changes since this review