Skip to content

Commit 3ee39a8

Browse files
committed
revision: make get_commit_action() a pure predicate
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>
1 parent 14d8c63 commit 3ee39a8

2 files changed

Lines changed: 47 additions & 27 deletions

File tree

line-log.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@
3737
* and rewrites the parent links.
3838
*
3939
* - Lazy, for a plain "git log -L": each commit is processed on demand
40-
* during the walk, from get_commit_action() in revision.c.
40+
* during the walk, from simplify_commit() in revision.c.
4141
*
4242
* Both share line_log_process_commit(), which processes
4343
* one commit and reports whether it changed a tracked range.

revision.c

Lines changed: 46 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -4159,37 +4159,40 @@ static timestamp_t comparison_date(const struct rev_info *revs,
41594159
commit->date;
41604160
}
41614161

4162-
enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)
4162+
/*
4163+
* Which commits are ignored regardless of content, and before any
4164+
* line-level log processing. Extracted so the line-log walk driver
4165+
* (simplify_commit) can apply the same gate before it calls
4166+
* line_log_process_commit().
4167+
*/
4168+
static int commit_early_ignore(struct rev_info *revs, struct commit *commit)
41634169
{
41644170
if (commit->object.flags & SHOWN)
4165-
return commit_ignore;
4171+
return 1;
41664172
if (revs->maximal_only && (commit->object.flags & CHILD_VISITED))
4167-
return commit_ignore;
4173+
return 1;
41684174
if (revs->unpacked && has_object_pack(revs->repo, &commit->object.oid))
4169-
return commit_ignore;
4170-
if (revs->no_kept_objects) {
4171-
if (has_object_kept_pack(revs->repo, &commit->object.oid,
4172-
revs->keep_pack_cache_flags))
4173-
return commit_ignore;
4174-
}
4175+
return 1;
4176+
if (revs->no_kept_objects &&
4177+
has_object_kept_pack(revs->repo, &commit->object.oid,
4178+
revs->keep_pack_cache_flags))
4179+
return 1;
41754180
if (commit->object.flags & UNINTERESTING)
4181+
return 1;
4182+
return 0;
4183+
}
4184+
4185+
/*
4186+
* A pure predicate: should this commit be shown, ignored, or does it
4187+
* error? Line-level log filtering for the no-parent-rewriting case
4188+
* mutates the tracked ranges, so it is done by simplify_commit() before
4189+
* it calls us; keeping this function side-effect free lets callers such
4190+
* as the commit graph use it as a plain query.
4191+
*/
4192+
enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)
4193+
{
4194+
if (commit_early_ignore(revs, commit))
41764195
return commit_ignore;
4177-
if (revs->line_level_traverse && !want_ancestry(revs)) {
4178-
/*
4179-
* In case of line-level log with parent rewriting
4180-
* prepare_revision_walk() already took care of all line-level
4181-
* log filtering, and there is nothing left to do here.
4182-
*
4183-
* If parent rewriting was not requested, then this is the
4184-
* place to perform the line-level log filtering. Notably,
4185-
* this check, though expensive, must come before the other,
4186-
* cheaper filtering conditions, because the tracked line
4187-
* ranges must be adjusted even when the commit will end up
4188-
* being ignored based on other conditions.
4189-
*/
4190-
if (!line_log_process_commit(revs, commit))
4191-
return commit_ignore;
4192-
}
41934196
if (revs->min_age != -1 &&
41944197
comparison_date(revs, commit) > revs->min_age)
41954198
return commit_ignore;
@@ -4298,7 +4301,24 @@ struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit
42984301

42994302
enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
43004303
{
4301-
enum commit_action action = get_commit_action(revs, commit);
4304+
enum commit_action action;
4305+
4306+
/*
4307+
* Line-level log without parent rewriting is filtered here, as the
4308+
* walk reaches each commit; with parent rewriting,
4309+
* prepare_revision_walk() already did it via line_log_filter(). Run
4310+
* it before get_commit_action() (a pure predicate), under the same
4311+
* commit_early_ignore() gate: a SHOWN or UNINTERESTING commit is not
4312+
* processed, but the tracked ranges must still be carried across a
4313+
* commit that later, cheaper conditions will ignore, so this must
4314+
* come first.
4315+
*/
4316+
if (revs->line_level_traverse && !want_ancestry(revs) &&
4317+
!commit_early_ignore(revs, commit) &&
4318+
!line_log_process_commit(revs, commit))
4319+
return commit_ignore;
4320+
4321+
action = get_commit_action(revs, commit);
43024322

43034323
if (action == commit_show &&
43044324
revs->prune && revs->dense && want_ancestry(revs)) {

0 commit comments

Comments
 (0)