{"thread":{"id":"66008","subject":"[PATCH] revision: make get_commit_action() a pure predicate","startedAt":"2026-07-15T19:29:56Z","lastAt":"2026-07-27T13:45:44Z","messageCount":5,"participants":["Michael Montalbo via GitGitGadget","Junio C Hamano","Michael Montalbo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"548320","messageId":"pull.2169.git.1784143793613.gitgitgadget@gmail.com","threadId":"66008","inReplyTo":null,"subject":"[PATCH] revision: make get_commit_action() a pure predicate","fromName":"Michael Montalbo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2026-07-15T19:29:52Z","receivedAt":"2026-07-15T19:29:56Z","isPatch":true,"body":"From: Michael Montalbo <mmontalbo@gmail.com>\n\nget_commit_action() reads as a predicate that decides whether a commit\nis shown or ignored, but for a line-level log without parent rewriting\nit also calls line_log_process_ranges_arbitrary_commit(), which\nmutates the tracked line ranges.  That hidden side effect makes it unsafe\nto evaluate ahead of the walk, the way a lookahead would.\n\nget_commit_action() was split out of simplify_commit() in beb5af43a6\n(graph API: fix bug in graph_is_interesting(), 2009-08-18) as the\nshow/ignore decision minus the parent rewriting, so the graph renderer\ncould reuse it; line-level log later routed its filtering through it as\nwell, in 3cb9d2b6 (line-log: more responsive, incremental 'git log -L',\n2020-05-11).  Besides simplify_commit(), the walk driver,\ngraph_is_interesting() is its only other caller, and it runs only under\n--graph, which sets rewrite_parents and therefore want_ancestry(); the\n\"-L without ancestry\" branch that holds the side effect never fires\nthere, so it is dormant today.\n\nThe line-level processing folds a commit's tracked ranges onto its\nparents, which must happen even for a commit that get_commit_action()\nfilters from the output, or the ranges never reach the parents.  Move it\nto simplify_commit() and run it before get_commit_action(), gated by\nget_commit_action()'s leading checks (already shown, uninteresting, and\nthe like) so a commit ignored by those is not folded, as before; factor\nthose checks out as commit_early_ignore().  get_commit_action() is then\nside-effect free.\n\ncommit_early_ignore() runs twice on the -L path, once for that gate and\nonce inside get_commit_action(), but it reads only object flags and pack\nmembership, disjoint from the TREESAME flag the fold sets, so the repeat\nis harmless.\n\nAdd a \"line-log-peek\" subcommand to the revision-walking test helper\nthat evaluates get_commit_action() on a commit the walk has not reached\nyet, plus a t4211 check that the call leaves the commit's flags\nunchanged.  The flags are compared rather than the commit list because\nadd_line_range() merges ranges by union, which is idempotent, so the\nside effect never changed which commits a linear -L history shows.\n\nSuggested-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Michael Montalbo <mmontalbo@gmail.com>\n---\n    revision: make get_commit_action() a pure predicate\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-2169%2Fmmontalbo%2Fmm%2Fline-log-tidy-proto-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-2169/mmontalbo/mm/line-log-tidy-proto-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/2169\n\n revision.c                       | 70 ++++++++++++++++++++------------\n t/helper/test-revision-walking.c | 63 ++++++++++++++++++++++++++++\n t/t4211-line-log.sh              | 20 +++++++++\n 3 files changed, 127 insertions(+), 26 deletions(-)\n\ndiff --git a/revision.c b/revision.c\nindex 0c95edef59..5d650affc0 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -4175,37 +4175,39 @@ static timestamp_t comparison_date(const struct rev_info *revs,\n \t\tcommit->date;\n }\n \n-enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)\n+/*\n+ * Whether the commit is ignored by the cheap checks that read only its\n+ * traversal flags and pack membership (e.g. already shown, or marked\n+ * uninteresting), before any check that examines the commit's date,\n+ * parents, message, or diff.\n+ */\n+static int commit_early_ignore(struct rev_info *revs, struct commit *commit)\n {\n \tif (commit->object.flags & SHOWN)\n-\t\treturn commit_ignore;\n+\t\treturn 1;\n \tif (revs->maximal_only && (commit->object.flags & CHILD_VISITED))\n-\t\treturn commit_ignore;\n+\t\treturn 1;\n \tif (revs->unpacked && has_object_pack(revs->repo, &commit->object.oid))\n-\t\treturn commit_ignore;\n-\tif (revs->no_kept_objects) {\n-\t\tif (has_object_kept_pack(revs->repo, &commit->object.oid,\n-\t\t\t\t\t revs->keep_pack_cache_flags))\n-\t\t\treturn commit_ignore;\n-\t}\n+\t\treturn 1;\n+\tif (revs->no_kept_objects &&\n+\t    has_object_kept_pack(revs->repo, &commit->object.oid,\n+\t\t\t\t revs->keep_pack_cache_flags))\n+\t\treturn 1;\n \tif (commit->object.flags & UNINTERESTING)\n+\t\treturn 1;\n+\treturn 0;\n+}\n+\n+/*\n+ * Decide whether this commit is shown or ignored.  Keep it a pure\n+ * predicate: callers such as the commit graph depend on it having no\n+ * side effects, so per-commit mutations (such as -L range tracking)\n+ * belong in the caller, simplify_commit(), not here.\n+ */\n+enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)\n+{\n+\tif (commit_early_ignore(revs, commit))\n \t\treturn commit_ignore;\n-\tif (revs->line_level_traverse && !want_ancestry(revs)) {\n-\t\t/*\n-\t\t * In case of line-level log with parent rewriting\n-\t\t * prepare_revision_walk() already took care of all line-level\n-\t\t * log filtering, and there is nothing left to do here.\n-\t\t *\n-\t\t * If parent rewriting was not requested, then this is the\n-\t\t * place to perform the line-level log filtering.  Notably,\n-\t\t * this check, though expensive, must come before the other,\n-\t\t * cheaper filtering conditions, because the tracked line\n-\t\t * ranges must be adjusted even when the commit will end up\n-\t\t * being ignored based on other conditions.\n-\t\t */\n-\t\tif (!line_log_process_ranges_arbitrary_commit(revs, commit))\n-\t\t\treturn commit_ignore;\n-\t}\n \tif (revs->min_age != -1 &&\n \t    comparison_date(revs, commit) > revs->min_age)\n \t\t\treturn commit_ignore;\n@@ -4314,7 +4316,23 @@ struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit\n \n enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n {\n-\tenum commit_action action = get_commit_action(revs, commit);\n+\tenum commit_action action;\n+\n+\t/*\n+\t * For a line-level log without parent rewriting, fold each commit's\n+\t * ranges as the walk reaches it (parent rewriting does this eagerly in\n+\t * prepare_revision_walk()).  Fold before get_commit_action() so the\n+\t * ranges carry across a commit that a later, cheaper check ignores;\n+\t * the commit_early_ignore() guard skips a commit get_commit_action()\n+\t * would ignore outright.\n+\t */\n+\tif (revs->line_level_traverse && !want_ancestry(revs) &&\n+\t    !commit_early_ignore(revs, commit)) {\n+\t\tif (!line_log_process_ranges_arbitrary_commit(revs, commit))\n+\t\t\treturn commit_ignore;\n+\t}\n+\n+\taction = get_commit_action(revs, commit);\n \n \tif (action == commit_show &&\n \t    revs->prune && revs->dense && want_ancestry(revs)) {\ndiff --git a/t/helper/test-revision-walking.c b/t/helper/test-revision-walking.c\nindex 70051eeaf8..24d7f29417 100644\n--- a/t/helper/test-revision-walking.c\n+++ b/t/helper/test-revision-walking.c\n@@ -13,9 +13,12 @@\n #include \"test-tool.h\"\n #include \"commit.h\"\n #include \"diff.h\"\n+#include \"line-log.h\"\n+#include \"object-name.h\"\n #include \"repository.h\"\n #include \"revision.h\"\n #include \"setup.h\"\n+#include \"string-list.h\"\n \n static void print_commit(struct commit *commit)\n {\n@@ -51,6 +54,60 @@ static int run_revision_walk(void)\n \treturn got_revision;\n }\n \n+/*\n+ * Check that get_commit_action() is a pure predicate by evaluating it on a\n+ * commit the walk has not reached yet.  No git command makes that out-of-order\n+ * call, so this probe does it deliberately, and reports whether the call\n+ * mutated the peeked commit: a pure get_commit_action() leaves it untouched.\n+ * We compare the commit's flags rather than the emitted commit list because\n+ * range merges are idempotent, so a side effect would not change which commits\n+ * are shown.  Only meaningful for a plain \"-L\" walk with no parent rewriting.\n+ */\n+static int line_log_peek(const char **argv)\n+{\n+\tstruct repository *repo = the_repository;\n+\tstruct rev_info rev;\n+\tstruct string_list range_args = STRING_LIST_INIT_DUP;\n+\tstruct object_id oid;\n+\tstruct commit *peek;\n+\tconst char *rev_argv[3];\n+\tunsigned before, after;\n+\n+\tif (repo_get_oid(repo, argv[0], &oid))\n+\t\tdie(\"bad peek commit: %s\", argv[0]);\n+\tpeek = lookup_commit_reference(repo, &oid);\n+\tif (!peek || repo_parse_commit(repo, peek))\n+\t\tdie(\"cannot parse peek commit: %s\", argv[0]);\n+\n+\trepo_init_revisions(repo, &rev, NULL);\n+\trev.diffopt.flags.recursive = 1;\n+\trev.line_level_traverse = 1;\n+\tstring_list_append(&range_args, argv[1]);\n+\n+\trev_argv[0] = \"line-log-peek\";\n+\trev_argv[1] = argv[2];\n+\trev_argv[2] = NULL;\n+\tsetup_revisions(2, rev_argv, &rev, NULL);\n+\n+\tline_log_init(&rev, NULL, &range_args);\n+\n+\tif (rev.rewrite_parents || rev.children.name)\n+\t\tdie(\"line-log-peek requires a non-ancestry (-L, no --graph) walk\");\n+\n+\tif (prepare_revision_walk(&rev))\n+\t\tdie(\"prepare_revision_walk failed\");\n+\n+\tbefore = peek->object.flags;\n+\tget_commit_action(&rev, peek);\n+\tafter = peek->object.flags;\n+\n+\tprintf(\"mutated %d\\n\", before != after);\n+\n+\trelease_revisions(&rev);\n+\tstring_list_clear(&range_args, 0);\n+\treturn 0;\n+}\n+\n int cmd__revision_walking(int argc, const char **argv)\n {\n \tif (argc < 2)\n@@ -69,6 +126,12 @@ int cmd__revision_walking(int argc, const char **argv)\n \t\treturn 0;\n \t}\n \n+\tif (!strcmp(argv[1], \"line-log-peek\")) {\n+\t\tif (argc != 5)\n+\t\t\tdie(\"usage: test-tool revision-walking line-log-peek <peek-commit> <start,end:file> <rev>\");\n+\t\treturn line_log_peek(argv + 2);\n+\t}\n+\n \tfprintf(stderr, \"check usage\\n\");\n \treturn 1;\n }\ndiff --git a/t/t4211-line-log.sh b/t/t4211-line-log.sh\nindex ca4eb7bbc7..f4a7d8ab61 100755\n--- a/t/t4211-line-log.sh\n+++ b/t/t4211-line-log.sh\n@@ -781,4 +781,24 @@ test_expect_success '--summary shows new file on root commit' '\n \ttest_grep \"create mode 100644 file.c\" actual\n '\n \n+test_expect_success 'get_commit_action() does not mutate a not-yet-walked commit' '\n+\tgit init peek &&\n+\t(\n+\t\tcd peek &&\n+\t\ttest_write_lines 1 2 3 4 5 >f.c &&\n+\t\tgit add f.c && test_tick && git commit -m base &&\n+\t\ttest_write_lines 1 two 3 4 5 >f.c &&\n+\t\ttest_tick && git commit -am change &&\n+\n+\t\t# Peek HEAD^, which the walk has not reached (the out-of-order\n+\t\t# call a lookahead makes), and confirm get_commit_action() leaves\n+\t\t# it untouched.  A side effect is invisible in the commit list\n+\t\t# (range merges are idempotent), so the helper reports whether the\n+\t\t# call mutated the peeked commit at all.\n+\t\techo \"mutated 0\" >expect &&\n+\t\ttest-tool revision-walking line-log-peek HEAD^ 1,3:f.c HEAD >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n\nbase-commit: f60db8d575adb79761d363e026fb49bddf330c73\n-- \ngitgitgadget\n"},{"id":"548928","messageId":"xmqqjyqk3w7d.fsf@gitster.g","threadId":"66008","inReplyTo":"pull.2169.git.1784143793613.gitgitgadget@gmail.com","subject":"Re: [PATCH] revision: make get_commit_action() a pure predicate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-24T21:37:58Z","receivedAt":"2026-07-24T21:38:02Z","isPatch":true,"body":"\"Michael Montalbo via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> Add a \"line-log-peek\" subcommand to the revision-walking test helper\n> that evaluates get_commit_action() on a commit the walk has not reached\n> yet, plus a t4211 check that the call leaves the commit's flags\n> unchanged.  The flags are compared rather than the commit list because\n> add_line_range() merges ranges by union, which is idempotent, so the\n> side effect never changed which commits a linear -L history shows.\n>\n> Suggested-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Michael Montalbo <mmontalbo@gmail.com>\n> ---\n>     revision: make get_commit_action() a pure predicate\n\nSorry, but I completely lost track and I do not recall suggesting a\nchange that amounts to 100+ lines of new lines.  Are we doing any\ncode clean-up?  Bugfix?  A new feature?\n"},{"id":"548976","messageId":"CAC2QwmKP16cyw0get3hEWP8GjcFkUHB3uXxcQi9hBCCM-B+ECw@mail.gmail.com","threadId":"66008","inReplyTo":"xmqqjyqk3w7d.fsf@gitster.g","subject":"Re: [PATCH] revision: make get_commit_action() a pure predicate","fromName":"Michael Montalbo","fromEmail":"mmontalbo@gmail.com","sentAt":"2026-07-25T19:25:05Z","receivedAt":"2026-07-25T19:25:18Z","isPatch":true,"body":"On Fri, Jul 24, 2026 at 2:38 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Sorry, but I completely lost track and I do not recall suggesting a\n> change that amounts to 100+ lines of new lines.  Are we doing any\n> code clean-up?  Bugfix?  A new feature?\n\nA latent bug fix, but I understand why this was confusing.\n\nThis was the discussion I should have linked to:\n\nhttps://lore.kernel.org/git/xmqqtsqxfdl4.fsf@gitster.g/.\n\nI had the link in my GGG PR description but accidentally deleted it\nwithout re-adding when I remembered GGG PRs shouldn't use a\ndescription for one commit series.\n\nThe linked discussion refers to a new graph feature that invokes\nget_commit_action() under the assumption the function will not\nmodify any commit state. The graph feature in question uses a\nconfiguration that just happens to avoid the branch of\nget_commit_action() that modifies a commit's line range state,\nso a bug isn't ultimately surfaced in the linked topic feature, but\nit remains a potential issue for future callers.\n\nUnfortunately, I couldn't figure out a way to make a test that\nvalidates if the change is effective without creating a bespoke\ntest-tool that calls the function with the \"right\" options set.\n"},{"id":"549085","messageId":"xmqqcxw8pnsa.fsf@gitster.g","threadId":"66008","inReplyTo":"CAC2QwmKP16cyw0get3hEWP8GjcFkUHB3uXxcQi9hBCCM-B+ECw@mail.gmail.com","subject":"Re: [PATCH] revision: make get_commit_action() a pure predicate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-27T13:25:41Z","receivedAt":"2026-07-27T13:25:44Z","isPatch":true,"body":"Michael Montalbo <mmontalbo@gmail.com> writes:\n\n> On Fri, Jul 24, 2026 at 2:38 PM Junio C Hamano <gitster@pobox.com> wrote:\n>>\n>> Sorry, but I completely lost track and I do not recall suggesting a\n>> change that amounts to 100+ lines of new lines.  Are we doing any\n>> code clean-up?  Bugfix?  A new feature?\n>\n> A latent bug fix, but I understand why this was confusing.\n>\n> This was the discussion I should have linked to:\n>\n> https://lore.kernel.org/git/xmqqtsqxfdl4.fsf@gitster.g/.\n>\n> I had the link in my GGG PR description but accidentally deleted it\n> without re-adding when I remembered GGG PRs shouldn't use a\n> description for one commit series.\n\nAh, I recall that discussion.\n\n> Unfortunately, I couldn't figure out a way to make a test that\n> validates if the change is effective without creating a bespoke\n> test-tool that calls the function with the \"right\" options set.\n\nUnderstandable, as it does not fix an active bug so much as clean up\nthe API to make it harder to introduce bugs in code that calls it.\n\nThanks.\n"},{"id":"549086","messageId":"xmqq8q6wpmuy.fsf@gitster.g","threadId":"66008","inReplyTo":"pull.2169.git.1784143793613.gitgitgadget@gmail.com","subject":"Re: [PATCH] revision: make get_commit_action() a pure predicate","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-07-27T13:45:41Z","receivedAt":"2026-07-27T13:45:44Z","isPatch":true,"body":"\"Michael Montalbo via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> commit_early_ignore() runs twice on the -L path, once for that gate and\n> once inside get_commit_action(), but it reads only object flags and pack\n> membership, disjoint from the TREESAME flag the fold sets, so the repeat\n> is harmless.\n\nThis one confused me a bit, so I'll think aloud below to see if you\ncan spot where I am misunderstanding your code.\n\n> +/*\n> + * Whether the commit is ignored by the cheap checks that read only its\n> + * traversal flags and pack membership (e.g. already shown, or marked\n> + * uninteresting), before any check that examines the commit's date,\n> + * parents, message, or diff.\n> + */\n> +static int commit_early_ignore(struct rev_info *revs, struct commit *commit)\n>  {\n>  \tif (commit->object.flags & SHOWN)\n> -\t\treturn commit_ignore;\n> +\t\treturn 1;\n>  \tif (revs->maximal_only && (commit->object.flags & CHILD_VISITED))\n> -\t\treturn commit_ignore;\n> +\t\treturn 1;\n>  \tif (revs->unpacked && has_object_pack(revs->repo, &commit->object.oid))\n> -\t\treturn commit_ignore;\n> -\tif (revs->no_kept_objects) {\n> -\t\tif (has_object_kept_pack(revs->repo, &commit->object.oid,\n> -\t\t\t\t\t revs->keep_pack_cache_flags))\n> -\t\t\treturn commit_ignore;\n> -\t}\n> +\t\treturn 1;\n> +\tif (revs->no_kept_objects &&\n> +\t    has_object_kept_pack(revs->repo, &commit->object.oid,\n> +\t\t\t\t revs->keep_pack_cache_flags))\n> +\t\treturn 1;\n>  \tif (commit->object.flags & UNINTERESTING)\n> +\t\treturn 1;\n> +\treturn 0;\n> +}\n\nThis mirrors what the original get_commit_action() did to return\nearly with 'commit_ignore'.  Collapsing the nested 'if' for the\nkept-objects case is a nice touch that makes the result easier to\nfollow.\n\n> +/*\n> + * Decide whether this commit is shown or ignored.  Keep it a pure\n> + * predicate: callers such as the commit graph depend on it having no\n> + * side effects, so per-commit mutations (such as -L range tracking)\n> + * belong in the caller, simplify_commit(), not here.\n> + */\n> +enum commit_action get_commit_action(struct rev_info *revs, struct commit *commit)\n> +{\n> +\tif (commit_early_ignore(revs, commit))\n>  \t\treturn commit_ignore;\n> -\tif (revs->line_level_traverse && !want_ancestry(revs)) {\n> -\t\t/*\n> -\t\t * In case of line-level log with parent rewriting\n> -\t\t * prepare_revision_walk() already took care of all line-level\n> -\t\t * log filtering, and there is nothing left to do here.\n> -\t\t *\n> -\t\t * If parent rewriting was not requested, then this is the\n> -\t\t * place to perform the line-level log filtering.  Notably,\n> -\t\t * this check, though expensive, must come before the other,\n> -\t\t * cheaper filtering conditions, because the tracked line\n> -\t\t * ranges must be adjusted even when the commit will end up\n> -\t\t * being ignored based on other conditions.\n> -\t\t */\n> -\t\tif (!line_log_process_ranges_arbitrary_commit(revs, commit))\n> -\t\t\treturn commit_ignore;\n> -\t}\n>  \tif (revs->min_age != -1 &&\n>  \t    comparison_date(revs, commit) > revs->min_age)\n>  \t\t\treturn commit_ignore;\n> @@ -4314,7 +4316,23 @@ struct commit_list *get_saved_parents(struct rev_info *revs, const struct commit\n>  \n>  enum commit_action simplify_commit(struct rev_info *revs, struct commit *commit)\n>  {\n> -\tenum commit_action action = get_commit_action(revs, commit);\n> +\tenum commit_action action;\n> +\n> +\t/*\n> +\t * For a line-level log without parent rewriting, fold each commit's\n> +\t * ranges as the walk reaches it (parent rewriting does this eagerly in\n> +\t * prepare_revision_walk()).  Fold before get_commit_action() so the\n> +\t * ranges carry across a commit that a later, cheaper check ignores;\n> +\t * the commit_early_ignore() guard skips a commit get_commit_action()\n> +\t * would ignore outright.\n> +\t */\n> +\tif (revs->line_level_traverse && !want_ancestry(revs) &&\n> +\t    !commit_early_ignore(revs, commit)) {\n> +\t\tif (!line_log_process_ranges_arbitrary_commit(revs, commit))\n> +\t\t\treturn commit_ignore;\n> +\t}\n> +\n> +\taction = get_commit_action(revs, commit);\n\nThe primary change in the patch is to lift the \"line-level\" code out\nof get_commit_action() and move it to one of its callers (namely\nsimplify_commit()).  The other caller is known not to trigger the\naffected parts of the function, which was discussed previously at\nhttps://lore.kernel.org/git/xmqqtsqxfdl4.fsf@gitster.g/ and started\nthis leftover bit.\n\nWe used to call get_commit_action() to decide the fate of the\ncommit.  If get_commit_action() returned anything other than\n'commit_show', simplify_commit() simply returned that action without\ndoing anything further.\n\nThe original get_commit_action(), when on the code path that calls\nline_log_process_ranges_arbitrary_commit() to check if we want to\nignore this commit, did what the commit_early_ignore() helper does\nin this version before reaching that point.  So this updated caller\nin simplify_commit() recreates the exact same logic.\n\nWe do end up executing the commit_early_ignore() logic twice if\nline_log_process_ranges_arbitrary_commit() does not tell us to ignore\nthis commit.  With only two callers of get_commit_action(), we could\neasily reuse the result of commit_early_ignore() if we wanted to, but\nit is probably not worth it.\n\nSo the patch looks good.  Will queue.  Thanks.\n"}]}