Re: [PATCH 3/4] last-modified: check pathspec against Bloom filter first

[Jeff King <peff@peff.net>]

On Fri, Jul 17, 2026 at 06:05:39PM -0500, Taylor Blau wrote:

> > diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> > index 5478182f2e..e8ee610404 100644
> > --- a/builtin/last-modified.c
> > +++ b/builtin/last-modified.c
> > @@ -272,6 +272,9 @@ static bool maybe_changed_path(struct last_modified *lm,
> >  	if (!filter)
> >  		return true;
> >
> > +	if (revs_maybe_changed_in_bloom(&lm->rev, filter) == 0)
> 
> Nit: please prefer 'if (!foo())' over 'if (foo() == 0)'.

Yeah, though there is some subtlety here because of the tristate return
I described elsewhere in the thread. I think if we switch to a boolean
return then a straight "!" becomes even more desirable.

> I don't think this is safe with '--show-trees'. The original pathspec
> does not cover every entry in 'lm->paths', since the function
> 'populate_paths_from_revs()' also adds ancestor tree entries.

Hmm, interesting. I am surprised to learn that "-t" includes "d" when
the pathspec asked for "d/a". I thought it was mostly about showing
"d/a" when we recurse to find "d/a/b". But I guess it does not make a
distinction between the two (probably because it is just telling the
diff code to show trees, and it does not further apply the pathspec to
the output).

Does this mean there is also a bug in "git log"? I guess not, because it
is purely pruning based on the pathspec, and only shows "d/" for those
commits.

>         git -c core.commitGraph=false last-modified -t HEAD -- d/a \
>             >expect &&
>         git -c core.commitGraph=true last-modified -t HEAD -- d/a \
>             >actual &&

A minor side note: the documentation claims "-t" has no effect without
"-r", but it clearly is not true (it tells us to show "d", even when we
are not recursing).

> I think that the conditional is otherwise correct, if guarded when we
> know that 'lm->show_trees' is false, like so:
> 
>     if (!lm->show_trees &&
>         !revs_maybe_changed_in_bloom(&lm->rev, filter))
>             return false;

Hmph. That makes this optimization all but useless, because the intended
use case of last-modified is almost always going to use "-t" to be able
to mark the interior trees. And most callers are not going to care about
seeing "d" here; their purpose was to find out about the things _inside_
"d".

Would we consider removing "d" from the output for this case? Presumably
by double-checking the pathspecs again in add_path_from_diff(). That
gives less surprising output (to me, anyway) and would enable this
optimization. And the command is still marked as experimental, and I
think this is exactly the kind of corner case that is meant to cover.

-Peff