line-log: fix -L with pickaxe options#2061
Open
mmontalbo wants to merge 2 commits intogitgitgadget:masterfrom
Open
line-log: fix -L with pickaxe options#2061mmontalbo wants to merge 2 commits intogitgitgadget:masterfrom
mmontalbo wants to merge 2 commits intogitgitgadget:masterfrom
Conversation
queue_diffs() calls diffcore_std() to detect renames so that line-level history can follow files across renames. When pickaxe options are present on the command line (-G and -S to filter by text pattern, --find-object to filter by object identity), diffcore_std() also runs diffcore_pickaxe(), which may discard diff pairs that are relevant for rename detection. Losing those pairs breaks rename following. Before a2bb801 (line-log: avoid unnecessary full tree diffs, 2019-08-21), diffcore_std() was only invoked when a rename was already suspected, so the pickaxe interference was unlikely in practice. That commit made the diffcore_std() call unconditional, and with filter_diffs_for_paths() now framing that call, a queue pruned by pickaxe violates filter_diffs_for_paths()'s expectation that diff pairs correspond to tracked paths, triggering an assertion failure. Fix this by calling diffcore_rename() directly instead of diffcore_std(). The line-log machinery only needs rename detection from this call site; the other stages run by diffcore_std() (pickaxe, order, break/rewrite) are unnecessary here. Note that this only fixes the crash. The -G, -S, and --find-object options still have no effect on -L output because line-log uses its own commit-filtering logic that bypasses the normal pickaxe pipeline. Add tests that verify the crash is fixed and mark the silent-ignore behavior as known breakage for all three options. Reported-by: Matthew Hughes <matthewhughes934@gmail.com> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
The previous commit fixed a crash when -G, -S, or --find-object was used together with -L and rename detection. However, these options still have no effect on -L output: line-log uses its own commit-filtering logic in line_log_filter() and never consults the pickaxe machinery. Rather than silently ignoring these options, reject the combination with a clear error message. This replaces the known-breakage tests from the previous commit with tests that verify the rejection for all three options. A future series could teach line-log to honor these options and remove this restriction. Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
mmontalbo
force-pushed
the
fix-line-log-G-crash
branch
from
c18d89c to
ae5269a
Compare
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
1 participant
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.
This series fixes a crash in
git log -Lwhen combined with pickaxe options (-G,-S, or--find-object) on a history involving renames, reported in [1].The crash bisects to
a2bb801f6a(line-log: avoid unnecessary full tree diffs, 2019-08-21), which made thediffcore_std()call inqueue_diffs()unconditional. Before that commit, the same combination silently truncated history at rename boundaries rather than crashing. The root cause is thatdiffcore_std()runsdiffcore_pickaxe(), which may discard diff pairs needed for rename detection.Patch 1 fixes the crash by calling
diffcore_rename()directly instead ofdiffcore_std(), and adds tests including known-breakage markers showing that the pickaxe options are silently ignored by-L.Patch 2 explicitly rejects the unsupported combination with
die(), replacing the known-breakage tests with rejection tests.[1] https://lore.kernel.org/git/aac-QdjY1ohAqgw_@desktop/
CC: Matthew Hughes matthewhughes934@gmail.com
CC: SZEDER Gábor szeder.dev@gmail.com