line-log: untangle the -L machinery and make get_commit_action() pure#2169
Draft
mmontalbo wants to merge 3 commits into
Draft
line-log: untangle the -L machinery and make get_commit_action() pure#2169mmontalbo wants to merge 3 commits into
mmontalbo wants to merge 3 commits into
Conversation
mmontalbo
changed the title
mmontalbo
force-pushed
the
mm/line-log-tidy-proto
branch
from
9612d75 to
07c79ee
Compare
The "struct range_set" interval algebra (and the "struct range" and
"struct diff_ranges" it builds on) was split awkwardly across two
headers: the structs lived in diffcore.h while the functions were
declared in line-log.h and defined in line-log.c. Both line-log and
"git blame -L" already use it, so it is a shared utility, not a
line-log internal.
Move it into a dedicated range-set.{c,h}: the range_set operations
(init/append/sort/union/difference and so on), the diff_ranges pair, and
range_set_map_across_diff() which maps a range set backward across a
diff. range_set_copy() and range_set_move() were file-static in
line-log.c and become part of the module interface, since line-log.c
still calls them across the new boundary. diffcore.h keeps only a
forward declaration for the "struct range_set *" it stores in a
diff_filepair; diff.c, line-log.c and (through line-log.h) blame pick up
the new header.
As a standalone module it is directly testable, so add a clar suite
(t/unit-tests/u-range-set.c). It covers the set operations (sorting and
merging overlapping, contained and empty ranges; disjoint and
overlapping unions) and, as executable documentation, the
range_set_map_across_diff() cases: an untouched range carried to the
parent and shifted, a range the commit changed blamed on it and replaced
by its parent-side counterpart, and a set that spans both in one diff.
diff_ranges_filter_touched() also gains an early return for an empty
range set so the module is safe to exercise directly; the sole in-tree
caller never passes one, so no real behavior changes.
lookup_line_range() used to re-run range_set_check_invariants() on every
lookup, but that was a debug-era assertion and maintaining the invariant
is the range-set operations' job, so drop it. range_set_check_invariants()
then has no external caller and stays private to the module.
No functional change intended.
Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
With the range-set algebra extracted, line-log.c is the "git log -L" machinery proper, but its structure was still hard to see. Group the remaining code by concern behind short section banners (the per-path line_log_data list; parsing -L arguments; computing a commit's diff; and the history walk), moving collect_diff() and the line_log_data helpers next to the code they belong with. Add a file header describing the two traversal modes (eager parent rewriting via line_log_filter(), versus the lazy per-commit walk driven from get_commit_action()), and a comment on the per-commit entry point describing how it dispatches on a commit's shape. Rename line_log_process_ranges_arbitrary_commit() to the shorter line_log_process_commit(): it processes one commit, and "arbitrary" was more puzzling than descriptive. While here, drop two bits of dead code the motion made obvious: a redundant trailing "return;" at the end of fill_blob_sha1(), and a commented-out debug fprintf in diff_might_be_rename(). No functional change intended. Signed-off-by: Michael Montalbo <mmontalbo@gmail.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 called line_log_process_commit(), which mutates the tracked line ranges. That hidden side effect made it unsafe to call as a plain query. get_commit_action() has only two callers: simplify_commit(), the normal walk driver, and graph_is_interesting() in graph.c. The latter runs only under --graph, which forces rewrite_parents (and therefore want_ancestry()), so the "-L without ancestry" branch that holds the side effect was already unreachable from it. Move the line-level processing to simplify_commit(), which runs it before calling get_commit_action(). Extract the leading "ignore regardless of content" checks into commit_early_ignore() so the same gate applies in both places, and so the tracked ranges are still adjusted for a commit that later, cheaper conditions will ignore. commit_early_ignore() only reads object flags and pack membership, so running it again from get_commit_action() is a harmless repeat. get_commit_action() is now side-effect free and safe to use as a predicate, independent of whether the line-level ranges were processed. Update the line-log.c file header, which pointed the lazy walk at get_commit_action(), to name simplify_commit(). Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>
mmontalbo
force-pushed
the
mm/line-log-tidy-proto
branch
from
07c79ee to
c11f95c
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.
get_commit_action() decides what a revision walk does with each commit it
reaches: show it, ignore it, or stop with an error. It reads as a pure
predicate, and the commit graph (graph_is_interesting()) uses it as one,
calling it only to decide whether a commit is interesting. But for a
line-level log without parent rewriting it also calls
line_log_process_commit(), which mutates the tracked line ranges. This is
currently harmless only because --graph forces parent rewriting, so
want_ancestry() is true and the side-effecting branch is not reached from
the graph. A caller that queried get_commit_action() under a plain
"git log -L" would instead drive the line-level walk.
Removing the side effect safely requires following the line-level walk
through line-log.c, which combined several concerns in one file: a general
interval algebra over line ranges (struct range_set: union, difference,
and mapping a set backward across a diff), the "git log -L" machinery that
uses it, and some dead debug code. The range_set types were also split
across headers, with the structs in diffcore.h and their operations in
line-log.h and line-log.c, even though "git blame -L" is a second user of
the algebra. Patches 1 and 2 separate these concerns; patch 3 removes the
side effect.
Patch 1 extracts range_set, and the struct range and struct diff_ranges it
builds on, into a new range-set.{c,h}. diffcore.h keeps only a forward
declaration for the struct range_set * it stores in a diff_filepair;
diff.c, line-log.c, and (through line-log.h) blame include the new header.
range_set_copy() and range_set_move() were file-static in line-log.c and
become part of the module interface, since line-log.c still calls them.
As a standalone module the algebra can be tested directly, so the patch
adds a clar unit suite, u-range-set.c. It covers the set operations
(sorting and merging overlapping, contained and empty ranges; disjoint and
overlapping unions) and, as executable documentation, the
range_set_map_across_diff() cases: an untouched range carried to the
parent and shifted by the diff, a range the commit changed attributed to
it and replaced by its parent-side counterpart, and a set that spans both
in one diff. Only operations with external callers are exported;
range_set_difference() is used only by the mapping routine and stays
internal, and a per-lookup invariant check is removed. While the module
is being made standalone, diff_ranges_filter_touched() gains an early
return for an empty range set; the sole in-tree caller never passes one,
so this does not change any behavior, but it lets the unit test exercise
the boundary directly.
Patch 2 reorganizes what remains of line-log.c. It groups the code by
concern behind section banners (the per-path line_log_data list, parsing
-L arguments, computing a commit's diff, and the history walk) and adds a
file header describing the two traversal modes: eager parent rewriting via
line_log_filter() under --graph, and the lazy per-commit walk driven from
get_commit_action(). It also renames
line_log_process_ranges_arbitrary_commit() to line_log_process_commit(),
and drops two bits of dead code the motion made obvious: a redundant
trailing "return;" in fill_blob_sha1() and a commented-out debug fprintf
in diff_might_be_rename().
Patch 3 moves the line-level processing from get_commit_action() to the
walk driver, simplify_commit(), which runs it before calling
get_commit_action(). The leading "ignore regardless of content" checks
are extracted into commit_early_ignore() so both places apply the same
gate, and the original ordering is kept: the tracked ranges are still
adjusted for a commit that later, cheaper conditions will ignore, so the
processing runs first. get_commit_action() then has only two callers,
simplify_commit() and graph_is_interesting(), and is free of side effects
for both. This is the change Junio suggested in [1].
There is no change in user-visible output; the two -L traversal modes do
the same work as before, with the lazy mode's line-level step now run by
the walk driver.
Out of scope: neither -L traversal mode changes behavior, and this does
not try to make "git blame -L" share more of the walk; it only relocates
the shared algebra and removes the side effect.
[1] https://lore.kernel.org/git/xmqqtsqxfdl4.fsf@gitster.g/