index: release the snapshot instead of freeing the index

[tiennou]

Previously we would assert in index_free because the reader incrementation would not be balanced. Release the snapshot normally, so the variable gets decremented before the index is freed.

Fixes #4802.

[ethomson]

Fixes half of it, anyway - we need to not malloc(0) as well. 😀

[ethomson]

/rebuild

[libgit2-azure-pipelines]

Okay, @ethomson, I started to rebuild this pull request.

[ethomson]

(That wasn't meant as snark, BTW, I just meant that we need to not close the issue after merging this.)

[tiennou]

I was under the impression that NULL was a valid return value from malloc(0), but it happens that the other value allowed by the standard is a non-NULL, non-dereferencable pointer (thanks, but no thanks 😁).

Would it be acceptable to just MIN_ALLOCSIZE a 0-sized vector on dup then ? Or should I just skip the allocation altogether ?

[tiennou]

Fixed the malloc(0) issue when duping empty/unallocated vectors. I'm still not sure the vector code is completely safe in that regard though, as resize_vector calls realloc, which has the same caveat.

[pks-t]

if (src->length) {
    size_t bytes;
    GITERR_CHECKALLOC_MULTIPLY(&bytes, src->length, sizeof(void *));
    ...
}

No need to do unnecessary work in case where src->length is already 0. I think like this intentions also become a bit clearer.

[tiennou]

Fixed. I was attempting to remove the different codepaths when duping vs a normal init (ie. make dup go through init/resize/memcpy instead of doing its own malloc), but I don't think it's worthwhile.

[pks-t]

Jeez, this _alloc_size member is really confusing. One tends to think it is the actual size (as you also seem to have interpreted it in your first go), but it actually is the allocated length. We should probably refactor this in another PR

[pks-t]

One might tend to think that resizing to 0 might mean that we should deallocate the vector. But resize_vector is only called from git_vector_init, git_vector_insert, git_vector_insert_sorted, git_vector_insert_null, git_vector_size_hint and git_vector_resize_to. Only for the last two it might make sense to do the deallocation dance, though.

• git_vector_resize_to is only used in git_vector_set, so the deallocation path could never be hit here.
• git_vector_size_hint will only do any work if the new size hint is bigger than alloc_size. So it also cannot ever hit the deallocation path.

So the dealloc would in fact be useless.

I think the only code path that can actually invoke resize_vector(v, 0) is git_vector_size_hint. And I can imagine a caller trivially invoking it e.g. if he wants to allocate a vector for a set of n items, where n might also be 0.

Anyway, long story short: makes sense to me.

[pks-t]

Thanks @tiennou!