[pull] master from git:master - #242
Merged
Merged
Conversation
When collapsing a full index to a sparse index, the recursive convert_to_sparse_rec() walks the cache tree to determine if any of the cache tree entries can be used to represent a sparse directory. As it goes, the method tracks how many cache entries are being represented by the cache tree entry. The cache tree node's 'entry_count' represents how many cache entries are covered by the node. However, this value can be negative, representing that a node is invalid, and is no longer reflecting the number of cache entries fit within. This can happen when the user uses 'git add --intent-to-add' to mark an untracked file with the intent-to-add bit to avoid committing without finishing the add. When such an intent-to-add file exists and the sparse-checkout changes to no longer contain its parent directory, this leads to a segfault. Two tests are added to demonstrate this fault: * One test is added to t3705-add-sparse-checkout.sh to demonstrate how 'git add' behaves with sparse-checkout. * One test is added to t1092-sparse-checkout-compatibility.sh to demonstrate the interaction with the sparse index and to compare it directly to how the commands behave with a full index or no sparse-checkout. The fix involves engaging with the loop that iterates over all cache entries within the parent cache tree node (from 'start' to 'end') and to set the 'span' variable slightly earlier. At this point, the cache entry is for a file that is at least one directory deeper than the current cache tree node. The path is also not in the sparse-checkout because of an earlier path_in_sparse_checkout() check above the loop. So we are trying to collapse this directory by recursively calling convert_to_sparse_rec() over that span of entries, but the negative value prevents us from predicting that number without scanning. Theoretically, we could scan to find the range of entries that match this directory and determine if they truly do have an intent-to-add bit and then collapse as many child trees as possible (the ones with valid cache tree nodes). That would be a non-trivial change for performance-only benefit. Since this combination of the intent-to-add and sparse index features has so far gone undetected by real users, this scenario is unlikely to be worth such a change. We settle for the simplest change that prevents a bug: don't try to collapse a node that is invalid for this reason. The tests that would demonstrate a segfault now pass. Further, they demonstrate that the intent-to-add bit persists in the index file after changing the sparse-checkout scope. The test in t1092 demonstrates how some sparse directories could be collapsed further with a more involved fix, if so desired in the future. Signed-off-by: Derrick Stolee <stolee@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
get_commit_action() reads as a predicate that decides whether a commit is shown or ignored, but for a line-level log without parent rewriting it also calls line_log_process_ranges_arbitrary_commit(), which mutates the tracked line ranges. That hidden side effect makes it unsafe to evaluate ahead of the walk, the way a lookahead would. get_commit_action() was split out of simplify_commit() in beb5af4 (graph API: fix bug in graph_is_interesting(), 2009-08-18) as the show/ignore decision minus the parent rewriting, so the graph renderer could reuse it; line-level log later routed its filtering through it as well, in 3cb9d2b (line-log: more responsive, incremental 'git log -L', 2020-05-11). Besides simplify_commit(), the walk driver, graph_is_interesting() is its only other caller, and it runs only under --graph, which sets rewrite_parents and therefore want_ancestry(); the "-L without ancestry" branch that holds the side effect never fires there, so it is dormant today. The line-level processing folds a commit's tracked ranges onto its parents, which must happen even for a commit that get_commit_action() filters from the output, or the ranges never reach the parents. Move it to simplify_commit() and run it before get_commit_action(), gated by get_commit_action()'s leading checks (already shown, uninteresting, and the like) so a commit ignored by those is not folded, as before; factor those checks out as commit_early_ignore(). get_commit_action() is then side-effect free. commit_early_ignore() runs twice on the -L path, once for that gate and once inside get_commit_action(), but it reads only object flags and pack membership, disjoint from the TREESAME flag the fold sets, so the repeat is harmless. Add a "line-log-peek" subcommand to the revision-walking test helper that evaluates get_commit_action() on a commit the walk has not reached yet, plus a t4211 check that the call leaves the commit's flags unchanged. The flags are compared rather than the commit list because add_line_range() merges ranges by union, which is idempotent, so the side effect never changed which commits a linear -L history shows. Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
…-cached-unmerged-more * jk/diff-relative-cached-unmerged: diff-lib: add idx/tree sanity check to oneway_diff diff: ignore unmerged paths outside prefix with --relative --cached
The comment above oneway_diff() claims that the callback must advance o->pos to skip index entries it has already processed. That stopped being true in da165f4 (unpack-trees.c: prepare for looking ahead in the index, 2010-01-07), which moved that bookkeeping into unpack_trees(). Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Commit 8174627 (diff-lib: ignore paths that are outside $cwd if --relative asked, 2021-08-22) taught run_diff_files() to skip entries outside the requested prefix before processing them. Do the same in oneway_diff(), which handles the diff-index code path. The lower-level diff queue functions already reject such paths, but checking here avoids unnecessary work and keeps them out of every do_oneway_diff() code path. Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The batch mode of cat-file needs to know the object's type in order to print the contents (because it decides whether to stream or not based on object type). The default batch output contains %(objecttype), so we get the type info automatically. But when it doesn't, we have to ask for it explicitly. In the --batch code path, we check while setting up the object_info struct whether we will print the contents, and if so set "typep" to get the value. This comes from 6554dfa (cat-file: handle --batch format with missing type/size, 2013-12-12). But later we added a --batch-command mode, which does not do the same trick. The decision about whether to retrieve the contents is made per-command (a "contents" vs "info" command), so we can't decide when building the object_info originally. As a result, asking for: echo "contents HEAD" | git cat-file --batch-command="%(objectname)" will fail the assertion in print_object_or_die() that the type was actually filled in. We can fix it by tweaking the object_info on the fly as we receive each command. But we should be careful to restore it afterwards; otherwise a sequence of commands like: contents $one info $two info $three will pay the type-lookup price for $two and $three when it does not need to. This wouldn't be incorrect, but just slightly inefficient (and hence there are no tests for that part, because the externally-visible behavior is the same). Reported-by: Alan Stokes <alan@source.dev> Helped-by: Pablo Sabater <pabloosabaterr@gmail.com> Signed-off-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
By default the setting 'merge.defaultToUpstream' for git-merge(1) is set to 'true', which means when `git merge` is invoked with no arguments it merges the upstream branch configured for the current branch. With this configuration set to 'true', setup_with_upstream() is called. That function allocates an array of arguments and hands it back to cmd_merge() via its `argv` parameter. This array is never freed, so cmd_merge() leaks it on every invocation. Track the allocated array in a separate variable and free it at the end. The leak has been present since 93e535a (merge: merge with the default upstream branch without argument, 2011-03-24). Although the leak sanitizer was enabled for tests in fc1ddf4 (t: remove TEST_PASSES_SANITIZE_LEAK annotations, 2024-11-21), it went unnoticed because no test calls `git merge` without arguments, exercising the default-to-upstream path. Add such a test in t7600, which fails under the leak sanitizer without this fix. Signed-off-by: Toon Claes <toon@iotcl.com> Acked-by: Jeff King <peff@peff.net> Signed-off-by: Junio C Hamano <gitster@pobox.com>
`bitmap_position()` only returns a negative value when an object is not present in the bitmap index. In `find_objects()`, we have added a check (11d45a6) to avoid processing a root whose reachability is already represented by the base bitmap, but accidentally uses `pos > 0`. Consequently, it never performs the membership test for an object at position zero. If that object has an individual reachability bitmap, we unnecessarily OR that bitmap into the base again. Otherwise, we add the object to the not-mapped list, only for the subsequent pass to recognize that it is already present. The latter pass correctly treats all non-negative positions as valid, so this does not change the resulting object set, but an off-by-one edge case. Treat position zero as valid by changing the condition to `pos >= 0`. The existing pseudo-merge traversal test exercises this case. Its position-zero commit is presented through multiple roots. Before this change, each occurrence is counted as a bitmap hit; afterwards, only the first occurrence is counted. Assert the resulting hit count to cover the boundary condition. Also cover the non-pseudo-merge case by passing `HEAD` twice. The first occurrence initializes the base from its stored bitmap, and the second must recognize that position zero is already present. Helped-by: Taylor Blau <ttaylorr@openai.com> Signed-off-by: David Lin <davidlin@stripe.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
The code path that deals with relative paths in the 'diff-lib' has been cleaned up. * jk/diff-relative-cached-unmerged-more: diff-lib: skip paths outside prefix in oneway_diff() diff-lib: drop stale comment about advancing o->pos
The get_commit_action() function has been refactored to be a pure predicate by moving the side-effecting line-level log range folding to simplify_commit(). This ensures that evaluating a commit's action before the walk reaches it does not prematurely mutate its tracked line ranges, making it safer for potential lookahead evaluations. * mm/revision-pure-get-commit-action: revision: make get_commit_action() a pure predicate
'git cat-file --batch-command' that asked for 'contents' without 'type' segfaults, which has been corrected. * jk/cat-file-batch-wo-type-fix: cat-file: handle content request for --batch-command without type
A memory leak in 'git merge' when run without arguments (which triggers the default-to-upstream path) has been fixed. A test has been added to cover this case. * tc/merge-default-to-upstream-leakfix: merge: fix leak with merge.defaultToUpstream
A boundary case check in reachability bitmap traversal has been corrected to properly handle the object at position zero, which was previously skipped, leading to redundant bitmap loading. * dl/pack-bitmap-position-zero: pack-bitmap: handle objects at bitmap position zero
A crash in the 'sparse-index' collapse code when encountering an invalidated cache-tree node (due to an intent-to-add path) has been fixed by avoiding collapsing such subtrees. * ds/sparse-index-ita-crash: sparse-index: avoid crash on intent-to-add entry outside the cone
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
5 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.4)
Can you help keep this open source service alive? 💖 Please sponsor : )