[pull] master from git:master#46
Merged
pull[bot] merged 8 commits intoturkdevops:masterfrom May 8, 2025
Merged
Conversation
In the series merged at bf0a430 (Merge branch 'en/strmap', 2020-11-21), strmap was built on top of hashmap and hashmap was extended in a few ways to support strmap and be more generally useful. One of the extensions was that hashmap_partial_clear() was introduced to allow reuse of the hashmap without freeing the table. Peff believed that it also made sense to introduce a hashmap_clear() which freed everything while allowing reuse. I added hashmap_clear(), but in doing so, overlooked the fact that for a hashmap to be reusable, it needs a defined cmpfn and data (the HASHMAP_INIT macro requires these fields as parameters, for example). So, if we want the hashmap to be reusable, we shouldn't zero out those fields. We probably also shouldn't zero out do_count_items. (We could zero out grow_at and shrink_at, but whether we zero those or not is irrelevant as they'll be automatically updated whenever a new entry is inserted.) Since clearing is associated with freeing map->table, and the only thing required for consistency after freeing map->table is zeroing tablesize and private_size, let's only zero those fields out. Signed-off-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Before accessing an array element at a given index, it should be verified that the index is within the desired bounds, not afterwards, otherwise it may not make sense to even access the array element in the first place. This is the point of CodeQL's `cpp/offset-use-before-range-check` rule. This CodeQL rule unfortunately is also triggered by the `fill_es_indent_data()` code, even though the condition `off < len - 1` does not even need to guarantee that the offset is in bounds (`s` points to a NUL-terminated string, for which `s[off] == '\r'` would fail before running out of bounds). Let's work around this rare false positive to help us use an otherwise mostly useful tool is a worthy thing to do. Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> Signed-off-by: Junio C Hamano <gitster@pobox.com>
We have a known issue in git-mv(1) where moving both a child and any of its parents causes an assert to trigger because the child cannot be found anymore in the index. We have added a test for this in commit 0fcd473 (t7001: add failure test which triggers assertion, 2024-10-22) without addressing the issue, which is why the test itself is marked as `test_expect_failure`. The behaviour of that test relies on a call to assert(3p) though, which may or may not be compiled into the resulting binary depending on whether or not we pass `-DNDEBUG`. When these asserts are compiled into Git this may cause our CI to hang on Windows though, because asserts may cause a modal window to be shown. While we could work around the issue by converting this into a call to `BUG()`, let's rather address the root cause of the issue by bailing out in case we see that both a child and any of its parents are being moved in the same command. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The use of asserts is discouraged in our codebase because they lead to different behaviour depending on how Git is built. When being unsure enough whether a condition always holds so that one adds the assert, then the assert should probably trigger regardless of how Git is being built. Drop the call to assert(3p) in git-mv(1) and instead use `BUG()`. Signed-off-by: Patrick Steinhardt <ps@pks.im> Signed-off-by: Junio C Hamano <gitster@pobox.com>
hashmap API clean-up to ensure hashmap_clear() leaves a cleared map in a reusable state. * en/hashmap-clear-fix: hashmap: ensure hashmaps are reusable after hashmap_clear()
"git mv a a/b dst" would ask to move the directory 'a' itself, as well as its contents, in a single destination directory, which is a contradicting request that is impossible to satisfy. This case is now detected and the command errors out. * ps/mv-contradiction-fix: builtin/mv: convert assert(3p) into `BUG()` builtin/mv: bail out when trying to move child and its parent
Work around false positive given by CodeQL. * js/diff-codeql-false-positive-workaround: diff: check range before dereferencing an array element
Signed-off-by: Junio C Hamano <gitster@pobox.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.1)
Can you help keep this open source service alive? 💖 Please sponsor : )