[PATCH] revision: make get_commit_action() a pure predicate

[PATCH] revision: make get_commit_action() a pure predicate

[Michael Montalbo via GitGitGadget]

From: 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 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>
---
    revision: make get_commit_action() a pure predicate

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-2169%2Fmmontalbo%2Fmm%2Fline-log-tidy-proto-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2169/mmontalbo/mm/line-log-tidy-proto-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/2169

 revision.c                       | 70 ++++++++++++++++++++------------
 t/helper/test-revision-walking.c | 63 ++++++++++++++++++++++++++++
 t/t4211-line-log.sh              | 20 +++++++++
 3 files changed, 127 insertions(+), 26 deletions(-)

diff --git a/revision.c b/revision.c
index 0c95edef59..5d650affc0 100644
--- a/revision.c
+++ b/revision.c
@@ -4175,37 +4175,39 @@ static timestamp_t comparison_date(const struct rev_info *revs,
 		commit->date;
 }
 
-enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)
+/*
+ * Whether the commit is ignored by the cheap checks that read only its
+ * traversal flags and pack membership (e.g. already shown, or marked
+ * uninteresting), before any check that examines the commit's date,
+ * parents, message, or diff.
+ */
+static int commit_early_ignore(struct rev_info *revs, struct commit *commit)
 {
 	if (commit->object.flags & SHOWN)
-		return commit_ignore;
+		return 1;
 	if (revs->maximal_only && (commit->object.flags & CHILD_VISITED))
-		return commit_ignore;
+		return 1;
 	if (revs->unpacked && has_object_pack(revs->repo, &commit->object.oid))
-		return commit_ignore;
-	if (revs->no_kept_objects) {
-		if (has_object_kept_pack(revs->repo, &commit->object.oid,
-					 revs->keep_pack_cache_flags))
-			return commit_ignore;
-	}
+		return 1;
+	if (revs->no_kept_objects &&
+	    has_object_kept_pack(revs->repo, &commit->object.oid,
+				 revs->keep_pack_cache_flags))
+		return 1;
 	if (commit->object.flags & UNINTERESTING)
+		return 1;
+	return 0;
+}
+
+/*
+ * Decide whether this commit is shown or ignored.  Keep it a pure
+ * predicate: callers such as the commit graph depend on it having no
+ * side effects, so per-commit mutations (such as -L range tracking)
+ * belong in the caller, simplify_commit(), not here.
+ */
+enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)
+{
+	if (commit_early_ignore(revs, commit))
 		return commit_ignore;
-	if (revs->line_level_traverse && !want_ancestry(revs)) {
-		/*
-		 * In case of line-level log with parent rewriting
-		 * prepare_revision_walk() already took care of all line-level
-		 * log filtering, and there is nothing left to do here.
-		 *
-		 * If parent rewriting was not requested, then this is the
-		 * place to perform the line-level log filtering.  Notably,
-		 * this check, though expensive, must come before the other,
-		 * cheaper filtering conditions, because the tracked line
-		 * ranges must be adjusted even when the commit will end up
-		 * being ignored based on other conditions.
-		 */
-		if (!line_log_process_ranges_arbitrary_commit(revs, commit))
-			return commit_ignore;
-	}
 	if (revs->min_age != -1 &&
 	    comparison_date(revs, commit) > revs->min_age)
 			return commit_ignore;
@@ -4314,7 +4316,23 @@ struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit
 
 enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
 {
-	enum commit_action action = get_commit_action(revs, commit);
+	enum commit_action action;
+
+	/*
+	 * For a line-level log without parent rewriting, fold each commit's
+	 * ranges as the walk reaches it (parent rewriting does this eagerly in
+	 * prepare_revision_walk()).  Fold before get_commit_action() so the
+	 * ranges carry across a commit that a later, cheaper check ignores;
+	 * the commit_early_ignore() guard skips a commit get_commit_action()
+	 * would ignore outright.
+	 */
+	if (revs->line_level_traverse && !want_ancestry(revs) &&
+	    !commit_early_ignore(revs, commit)) {
+		if (!line_log_process_ranges_arbitrary_commit(revs, commit))
+			return commit_ignore;
+	}
+
+	action = get_commit_action(revs, commit);
 
 	if (action == commit_show &&
 	    revs->prune && revs->dense && want_ancestry(revs)) {
diff --git a/t/helper/test-revision-walking.c b/t/helper/test-revision-walking.c
index 70051eeaf8..24d7f29417 100644
--- a/t/helper/test-revision-walking.c
+++ b/t/helper/test-revision-walking.c
@@ -13,9 +13,12 @@
 #include "test-tool.h"
 #include "commit.h"
 #include "diff.h"
+#include "line-log.h"
+#include "object-name.h"
 #include "repository.h"
 #include "revision.h"
 #include "setup.h"
+#include "string-list.h"
 
 static void print_commit(struct commit *commit)
 {
@@ -51,6 +54,60 @@ static int run_revision_walk(void)
 	return got_revision;
 }
 
+/*
+ * Check that get_commit_action() is a pure predicate by evaluating it on a
+ * commit the walk has not reached yet.  No git command makes that out-of-order
+ * call, so this probe does it deliberately, and reports whether the call
+ * mutated the peeked commit: a pure get_commit_action() leaves it untouched.
+ * We compare the commit's flags rather than the emitted commit list because
+ * range merges are idempotent, so a side effect would not change which commits
+ * are shown.  Only meaningful for a plain "-L" walk with no parent rewriting.
+ */
+static int line_log_peek(const char **argv)
+{
+	struct repository *repo = the_repository;
+	struct rev_info rev;
+	struct string_list range_args = STRING_LIST_INIT_DUP;
+	struct object_id oid;
+	struct commit *peek;
+	const char *rev_argv[3];
+	unsigned before, after;
+
+	if (repo_get_oid(repo, argv[0], &oid))
+		die("bad peek commit: %s", argv[0]);
+	peek = lookup_commit_reference(repo, &oid);
+	if (!peek || repo_parse_commit(repo, peek))
+		die("cannot parse peek commit: %s", argv[0]);
+
+	repo_init_revisions(repo, &rev, NULL);
+	rev.diffopt.flags.recursive = 1;
+	rev.line_level_traverse = 1;
+	string_list_append(&range_args, argv[1]);
+
+	rev_argv[0] = "line-log-peek";
+	rev_argv[1] = argv[2];
+	rev_argv[2] = NULL;
+	setup_revisions(2, rev_argv, &rev, NULL);
+
+	line_log_init(&rev, NULL, &range_args);
+
+	if (rev.rewrite_parents || rev.children.name)
+		die("line-log-peek requires a non-ancestry (-L, no --graph) walk");
+
+	if (prepare_revision_walk(&rev))
+		die("prepare_revision_walk failed");
+
+	before = peek->object.flags;
+	get_commit_action(&rev, peek);
+	after = peek->object.flags;
+
+	printf("mutated %d\n", before != after);
+
+	release_revisions(&rev);
+	string_list_clear(&range_args, 0);
+	return 0;
+}
+
 int cmd__revision_walking(int argc, const char **argv)
 {
 	if (argc < 2)
@@ -69,6 +126,12 @@ int cmd__revision_walking(int argc, const char **argv)
 		return 0;
 	}
 
+	if (!strcmp(argv[1], "line-log-peek")) {
+		if (argc != 5)
+			die("usage: test-tool revision-walking line-log-peek <peek-commit> <start,end:file> <rev>");
+		return line_log_peek(argv + 2);
+	}
+
 	fprintf(stderr, "check usage\n");
 	return 1;
 }
diff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh
index ca4eb7bbc7..f4a7d8ab61 100755
--- a/t/t4211-line-log.sh
+++ b/t/t4211-line-log.sh
@@ -781,4 +781,24 @@ test_expect_success '--summary shows new file on root commit' '
 	test_grep "create mode 100644 file.c" actual
 '
 
+test_expect_success 'get_commit_action() does not mutate a not-yet-walked commit' '
+	git init peek &&
+	(
+		cd peek &&
+		test_write_lines 1 2 3 4 5 >f.c &&
+		git add f.c && test_tick && git commit -m base &&
+		test_write_lines 1 two 3 4 5 >f.c &&
+		test_tick && git commit -am change &&
+
+		# Peek HEAD^, which the walk has not reached (the out-of-order
+		# call a lookahead makes), and confirm get_commit_action() leaves
+		# it untouched.  A side effect is invisible in the commit list
+		# (range merges are idempotent), so the helper reports whether the
+		# call mutated the peeked commit at all.
+		echo "mutated 0" >expect &&
+		test-tool revision-walking line-log-peek HEAD^ 1,3:f.c HEAD >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done

base-commit: f60db8d575adb79761d363e026fb49bddf330c73
-- 
gitgitgadget

Re: [PATCH] revision: make get_commit_action() a pure predicate

[Junio C Hamano]

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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>
> ---
>     revision: make get_commit_action() a pure predicate

Sorry, but I completely lost track and I do not recall suggesting a
change that amounts to 100+ lines of new lines.  Are we doing any
code clean-up?  Bugfix?  A new feature?

Re: [PATCH] revision: make get_commit_action() a pure predicate

[Michael Montalbo]

On Fri, Jul 24, 2026 at 2:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Sorry, but I completely lost track and I do not recall suggesting a
> change that amounts to 100+ lines of new lines.  Are we doing any
> code clean-up?  Bugfix?  A new feature?

A latent bug fix, but I understand why this was confusing.

This was the discussion I should have linked to:

https://lore.kernel.org/git/xmqqtsqxfdl4.fsf@gitster.g/.

I had the link in my GGG PR description but accidentally deleted it
without re-adding when I remembered GGG PRs shouldn't use a
description for one commit series.

The linked discussion refers to a new graph feature that invokes
get_commit_action() under the assumption the function will not
modify any commit state. The graph feature in question uses a
configuration that just happens to avoid the branch of
get_commit_action() that modifies a commit's line range state,
so a bug isn't ultimately surfaced in the linked topic feature, but
it remains a potential issue for future callers.

Unfortunately, I couldn't figure out a way to make a test that
validates if the change is effective without creating a bespoke
test-tool that calls the function with the "right" options set.

Re: [PATCH] revision: make get_commit_action() a pure predicate

[Junio C Hamano]

Michael Montalbo <mmontalbo@gmail.com> writes:

> On Fri, Jul 24, 2026 at 2:38 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Sorry, but I completely lost track and I do not recall suggesting a
>> change that amounts to 100+ lines of new lines.  Are we doing any
>> code clean-up?  Bugfix?  A new feature?
>
> A latent bug fix, but I understand why this was confusing.
>
> This was the discussion I should have linked to:
>
> https://lore.kernel.org/git/xmqqtsqxfdl4.fsf@gitster.g/.
>
> I had the link in my GGG PR description but accidentally deleted it
> without re-adding when I remembered GGG PRs shouldn't use a
> description for one commit series.

Ah, I recall that discussion.

> Unfortunately, I couldn't figure out a way to make a test that
> validates if the change is effective without creating a bespoke
> test-tool that calls the function with the "right" options set.

Understandable, as it does not fix an active bug so much as clean up
the API to make it harder to introduce bugs in code that calls it.

Thanks.

Re: [PATCH] revision: make get_commit_action() a pure predicate

[Junio C Hamano]

"Michael Montalbo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> 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.

This one confused me a bit, so I'll think aloud below to see if you
can spot where I am misunderstanding your code.

> +/*
> + * Whether the commit is ignored by the cheap checks that read only its
> + * traversal flags and pack membership (e.g. already shown, or marked
> + * uninteresting), before any check that examines the commit's date,
> + * parents, message, or diff.
> + */
> +static int commit_early_ignore(struct rev_info *revs, struct commit *commit)
>  {
>  	if (commit->object.flags & SHOWN)
> -		return commit_ignore;
> +		return 1;
>  	if (revs->maximal_only && (commit->object.flags & CHILD_VISITED))
> -		return commit_ignore;
> +		return 1;
>  	if (revs->unpacked && has_object_pack(revs->repo, &commit->object.oid))
> -		return commit_ignore;
> -	if (revs->no_kept_objects) {
> -		if (has_object_kept_pack(revs->repo, &commit->object.oid,
> -					 revs->keep_pack_cache_flags))
> -			return commit_ignore;
> -	}
> +		return 1;
> +	if (revs->no_kept_objects &&
> +	    has_object_kept_pack(revs->repo, &commit->object.oid,
> +				 revs->keep_pack_cache_flags))
> +		return 1;
>  	if (commit->object.flags & UNINTERESTING)
> +		return 1;
> +	return 0;
> +}

This mirrors what the original get_commit_action() did to return
early with 'commit_ignore'.  Collapsing the nested 'if' for the
kept-objects case is a nice touch that makes the result easier to
follow.

> +/*
> + * Decide whether this commit is shown or ignored.  Keep it a pure
> + * predicate: callers such as the commit graph depend on it having no
> + * side effects, so per-commit mutations (such as -L range tracking)
> + * belong in the caller, simplify_commit(), not here.
> + */
> +enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)
> +{
> +	if (commit_early_ignore(revs, commit))
>  		return commit_ignore;
> -	if (revs->line_level_traverse && !want_ancestry(revs)) {
> -		/*
> -		 * In case of line-level log with parent rewriting
> -		 * prepare_revision_walk() already took care of all line-level
> -		 * log filtering, and there is nothing left to do here.
> -		 *
> -		 * If parent rewriting was not requested, then this is the
> -		 * place to perform the line-level log filtering.  Notably,
> -		 * this check, though expensive, must come before the other,
> -		 * cheaper filtering conditions, because the tracked line
> -		 * ranges must be adjusted even when the commit will end up
> -		 * being ignored based on other conditions.
> -		 */
> -		if (!line_log_process_ranges_arbitrary_commit(revs, commit))
> -			return commit_ignore;
> -	}
>  	if (revs->min_age != -1 &&
>  	    comparison_date(revs, commit) > revs->min_age)
>  			return commit_ignore;
> @@ -4314,7 +4316,23 @@ struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit
>  
>  enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)
>  {
> -	enum commit_action action = get_commit_action(revs, commit);
> +	enum commit_action action;
> +
> +	/*
> +	 * For a line-level log without parent rewriting, fold each commit's
> +	 * ranges as the walk reaches it (parent rewriting does this eagerly in
> +	 * prepare_revision_walk()).  Fold before get_commit_action() so the
> +	 * ranges carry across a commit that a later, cheaper check ignores;
> +	 * the commit_early_ignore() guard skips a commit get_commit_action()
> +	 * would ignore outright.
> +	 */
> +	if (revs->line_level_traverse && !want_ancestry(revs) &&
> +	    !commit_early_ignore(revs, commit)) {
> +		if (!line_log_process_ranges_arbitrary_commit(revs, commit))
> +			return commit_ignore;
> +	}
> +
> +	action = get_commit_action(revs, commit);

The primary change in the patch is to lift the "line-level" code out
of get_commit_action() and move it to one of its callers (namely
simplify_commit()).  The other caller is known not to trigger the
affected parts of the function, which was discussed previously at
https://lore.kernel.org/git/xmqqtsqxfdl4.fsf@gitster.g/ and started
this leftover bit.

We used to call get_commit_action() to decide the fate of the
commit.  If get_commit_action() returned anything other than
'commit_show', simplify_commit() simply returned that action without
doing anything further.

The original get_commit_action(), when on the code path that calls
line_log_process_ranges_arbitrary_commit() to check if we want to
ignore this commit, did what the commit_early_ignore() helper does
in this version before reaching that point.  So this updated caller
in simplify_commit() recreates the exact same logic.

We do end up executing the commit_early_ignore() logic twice if
line_log_process_ranges_arbitrary_commit() does not tell us to ignore
this commit.  With only two callers of get_commit_action(), we could
easily reuse the result of commit_early_ignore() if we wanted to, but
it is probably not worth it.

So the patch looks good.  Will queue.  Thanks.