Detect duplicated submodules for the same path#4641
Conversation
pks-t
force-pushed
the
pks/submodule-names-memleak
branch
from
252310e to
4dad00c
Compare
|
Do we want some tests here to make sure we reject the right things? |
|
Probably, but I don't have enough time right now. I was very surprised to see that my changes didn't break anything at all. The "submodules" test repository has a gitmodules file with two modules pointing to the same path, so I'd have expected us erroring out to have at least some kind of visible change in behaviour. |
|
Then we should be expecting this fail, and since it's not that seems like a failing test on its own. |
|
I think the reason we're not failing is because we only use the repository with this duplicate path in places where we're testing stash or status, and I see it return failure, but it looks like we're ignoring it as part of trying to make status resilient to transient failures. |
pks-t
force-pushed
the
pks/submodule-names-memleak
branch
from
4dad00c to
5abbabf
Compare
pks-t
force-pushed
the
pks/submodule-names-memleak
branch
from
5abbabf to
e691fbe
Compare
When loading submodule names, we build a map of submodule paths and their respective names. While looping over the configuration keys, we do not check though whether a submodule path was seen already. This leads to a memory leak in case we have multiple submodules with the same path, as we just overwrite the old value in the map in that case. Fix the error by verifying that the path to be added is not yet part of the string map. Git does not allow to have multiple submodules for a path anyway, so we now do the same and detect this duplication, reporting it to the user.
The function `load_submodule_names` was always being called with a newly allocated string map, which was then getting filled by the function. Move the string map allocation into `load_submodule_names`, instead, and pass the whole map back to the caller in case no error occurs. This change helps to avoid misuse by handing in pre-populated maps.
Previous to dfda2f6 (submodule: remove the per-repo cache, 2015-04-27), we tried to cache our submodules per repository to avoid having to reload it too frequently. As it created some headaches with regards to multithreading, we removed that cache. Previous to that removal, we had to compute what submodule status to refresh. The mask computation was not removed, though, resulting in confusing and actually dead code. While it seems like the mask is currently in use in a conditional, it is not, as we unconditionally assign to the mask previous to that condition. Remove all mask computations to clean up stale code.
pks-t
force-pushed
the
pks/submodule-names-memleak
branch
from
e691fbe to
9c698a2
Compare
|
Rebased to fix conflicts with the submodule name validation fix. This now got a test which detects the issue and works on Windows. This plugs all currently known memory leaks. |
|
@carlosmn: you got any more feedback on this PR? I'd like to get this merged soon, as I think it should be part of v0.27.2 |
2 participants
We currently have a memory leak in AppVeyor wrt handling submodules. The issue is that we're accepting duplicated submodules for the same path, silently updating an already existing key in the submodule names hash map and leaking its previous value. Git does not allow for duplicated submodule paths, so we shouldn't either.