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

"Michael Montalbo via GitGitGadget" <[email protected]> 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/[email protected]/ 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.