@matheusbsbs / matheus-bagliano / commits / da0fb7e36e

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 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 beb5af43a6 (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 3cb9d2b6 (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>

Michael Montalbo committed Jul 15, 2026 at 19:29 UTC da0fb7e36e9864f7f41d3a2dd291ffd34fcec97f
3 files changed +127 -26
revision.c
+44 -26
@@ -4174,37 +4174,39 @@ static timestamp_t comparison_date(const struct rev_info *revs,
4174 commit->date;
4175 }
4176
4177 -enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)
4177 +/*
4178 + * Whether the commit is ignored by the cheap checks that read only its
4179 + * traversal flags and pack membership (e.g. already shown, or marked
4180 + * uninteresting), before any check that examines the commit's date,
4181 + * parents, message, or diff.
4182 + */
4183 +static int commit_early_ignore(struct rev_info *revs, struct commit *commit)
4184 {
4185 if (commit->object.flags & SHOWN)
4180 - return commit_ignore;
4186 + return 1;
4187 if (revs->maximal_only && (commit->object.flags & CHILD_VISITED))
4182 - return commit_ignore;
4188 + return 1;
4189 if (revs->unpacked && has_object_pack(revs->repo, &commit->object.oid))
4184 - return commit_ignore;
4185 - if (revs->no_kept_objects) {
4186 - if (has_object_kept_pack(revs->repo, &commit->object.oid,
4187 - revs->keep_pack_cache_flags))
4188 - return commit_ignore;
4189 - }
4190 + return 1;
4191 + if (revs->no_kept_objects &&
4192 + has_object_kept_pack(revs->repo, &commit->object.oid,
4193 + revs->keep_pack_cache_flags))
4194 + return 1;
4195 if (commit->object.flags & UNINTERESTING)
4196 + return 1;
4197 + return 0;
4198 +}
4199 +
4200 +/*
4201 + * Decide whether this commit is shown or ignored. Keep it a pure
4202 + * predicate: callers such as the commit graph depend on it having no
4203 + * side effects, so per-commit mutations (such as -L range tracking)
4204 + * belong in the caller, simplify_commit(), not here.
4205 + */
4206 +enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)
4207 +{
4208 + if (commit_early_ignore(revs, commit))
4209 return commit_ignore;
4192 - if (revs->line_level_traverse && !want_ancestry(revs)) {
4193 - /*
4194 - * In case of line-level log with parent rewriting
4195 - * prepare_revision_walk() already took care of all line-level
4196 - * log filtering, and there is nothing left to do here.
4197 - *
4198 - * If parent rewriting was not requested, then this is the
4199 - * place to perform the line-level log filtering. Notably,
4200 - * this check, though expensive, must come before the other,
4201 - * cheaper filtering conditions, because the tracked line
4202 - * ranges must be adjusted even when the commit will end up
4203 - * being ignored based on other conditions.
4204 - */
4205 - if (!line_log_process_ranges_arbitrary_commit(revs, commit))
4206 - return commit_ignore;
4207 - }
4210 if (revs->min_age != -1 &&
4211 comparison_date(revs, commit) > revs->min_age)
4212 return commit_ignore;
@@ -4313,7 +4315,23 @@ struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit
4315
4316 enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
4317 {
4316 - enum commit_action action = get_commit_action(revs, commit);
4318 + enum commit_action action;
4319 +
4320 + /*
4321 + * For a line-level log without parent rewriting, fold each commit's
4322 + * ranges as the walk reaches it (parent rewriting does this eagerly in
4323 + * prepare_revision_walk()). Fold before get_commit_action() so the
4324 + * ranges carry across a commit that a later, cheaper check ignores;
4325 + * the commit_early_ignore() guard skips a commit get_commit_action()
4326 + * would ignore outright.
4327 + */
4328 + if (revs->line_level_traverse && !want_ancestry(revs) &&
4329 + !commit_early_ignore(revs, commit)) {
4330 + if (!line_log_process_ranges_arbitrary_commit(revs, commit))
4331 + return commit_ignore;
4332 + }
4333 +
4334 + action = get_commit_action(revs, commit);
4335
4336 if (action == commit_show &&
4337 revs->prune && revs->dense && want_ancestry(revs)) {
t/helper/test-revision-walking.c
+63
@@ -13,9 +13,12 @@
13 #include "test-tool.h"
14 #include "commit.h"
15 #include "diff.h"
16 +#include "line-log.h"
17 +#include "object-name.h"
18 #include "repository.h"
19 #include "revision.h"
20 #include "setup.h"
21 +#include "string-list.h"
22
23 static void print_commit(struct commit *commit)
24 {
@@ -51,6 +54,60 @@ static int run_revision_walk(void)
54 return got_revision;
55 }
56
57 +/*
58 + * Check that get_commit_action() is a pure predicate by evaluating it on a
59 + * commit the walk has not reached yet. No git command makes that out-of-order
60 + * call, so this probe does it deliberately, and reports whether the call
61 + * mutated the peeked commit: a pure get_commit_action() leaves it untouched.
62 + * We compare the commit's flags rather than the emitted commit list because
63 + * range merges are idempotent, so a side effect would not change which commits
64 + * are shown. Only meaningful for a plain "-L" walk with no parent rewriting.
65 + */
66 +static int line_log_peek(const char **argv)
67 +{
68 + struct repository *repo = the_repository;
69 + struct rev_info rev;
70 + struct string_list range_args = STRING_LIST_INIT_DUP;
71 + struct object_id oid;
72 + struct commit *peek;
73 + const char *rev_argv[3];
74 + unsigned before, after;
75 +
76 + if (repo_get_oid(repo, argv[0], &oid))
77 + die("bad peek commit: %s", argv[0]);
78 + peek = lookup_commit_reference(repo, &oid);
79 + if (!peek || repo_parse_commit(repo, peek))
80 + die("cannot parse peek commit: %s", argv[0]);
81 +
82 + repo_init_revisions(repo, &rev, NULL);
83 + rev.diffopt.flags.recursive = 1;
84 + rev.line_level_traverse = 1;
85 + string_list_append(&range_args, argv[1]);
86 +
87 + rev_argv[0] = "line-log-peek";
88 + rev_argv[1] = argv[2];
89 + rev_argv[2] = NULL;
90 + setup_revisions(2, rev_argv, &rev, NULL);
91 +
92 + line_log_init(&rev, NULL, &range_args);
93 +
94 + if (rev.rewrite_parents || rev.children.name)
95 + die("line-log-peek requires a non-ancestry (-L, no --graph) walk");
96 +
97 + if (prepare_revision_walk(&rev))
98 + die("prepare_revision_walk failed");
99 +
100 + before = peek->object.flags;
101 + get_commit_action(&rev, peek);
102 + after = peek->object.flags;
103 +
104 + printf("mutated %d\n", before != after);
105 +
106 + release_revisions(&rev);
107 + string_list_clear(&range_args, 0);
108 + return 0;
109 +}
110 +
111 int cmd__revision_walking(int argc, const char **argv)
112 {
113 if (argc < 2)
@@ -69,6 +126,12 @@ int cmd__revision_walking(int argc, const char **argv)
126 return 0;
127 }
128
129 + if (!strcmp(argv[1], "line-log-peek")) {
130 + if (argc != 5)
131 + die("usage: test-tool revision-walking line-log-peek <peek-commit> <start,end:file> <rev>");
132 + return line_log_peek(argv + 2);
133 + }
134 +
135 fprintf(stderr, "check usage\n");
136 return 1;
137 }
t/t4211-line-log.sh
+20
@@ -781,4 +781,24 @@ test_expect_success '--summary shows new file on root commit' '
781 test_grep "create mode 100644 file.c" actual
782 '
783
784 +test_expect_success 'get_commit_action() does not mutate a not-yet-walked commit' '
785 + git init peek &&
786 + (
787 + cd peek &&
788 + test_write_lines 1 2 3 4 5 >f.c &&
789 + git add f.c && test_tick && git commit -m base &&
790 + test_write_lines 1 two 3 4 5 >f.c &&
791 + test_tick && git commit -am change &&
792 +
793 + # Peek HEAD^, which the walk has not reached (the out-of-order
794 + # call a lookahead makes), and confirm get_commit_action() leaves
795 + # it untouched. A side effect is invisible in the commit list
796 + # (range merges are idempotent), so the helper reports whether the
797 + # call mutated the peeked commit at all.
798 + echo "mutated 0" >expect &&
799 + test-tool revision-walking line-log-peek HEAD^ 1,3:f.c HEAD >actual &&
800 + test_cmp expect actual
801 + )
802 +'
803 +
804 test_done