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

[Jeff King <peff@peff.net>]

On Sat, Jul 18, 2026 at 04:22:31PM -0500, Taylor Blau wrote:

> I think that we could feasibly get rid of "d" in the output in this
> particular case within last-modified. As you note, the command is marked
> EXPERIMENTAL for a reason, after all ;-).
> 
> If we wanted to do that, it should be straightforward to do. I think the
> following (untested) patch would be sufficient:
> 
> --- 8< ---
> diff --git a/builtin/last-modified.c b/builtin/last-modified.c
> index adc7cd8c74..0f0c1d1d17 100644
> --- a/builtin/last-modified.c
> +++ b/builtin/last-modified.c
> @@ -103,7 +103,7 @@ struct last_modified_callback_data {
>  };
> 
>  static void add_path_from_diff(struct diff_queue_struct *q,
> -			       struct diff_options *opt UNUSED, void *data)
> +			       struct diff_options *opt, void *data)
>  {
>  	struct last_modified *lm = data;
> 
> @@ -112,6 +112,11 @@ static void add_path_from_diff(struct diff_queue_struct *q,
>  		struct last_modified_entry *ent;
>  		const char *path = p->two->path;
> 
> +		if (!match_pathspec(opt->repo->index, &opt->pathspec, path,
> +				    strlen(path), 0, NULL,
> +				    S_ISDIR(p->two->mode)))
> +			continue;
> +

Yeah, that was exactly what I was thinking, but I wasn't sure if
match_pathspec() was the right tool. I mean, obviously it sounds like it
should be from the name, but I don't think it is actually what is used
in tree-diffs! There we have tree-walk.c:do_match() which does some
magic. And match_pathspec() is used more for dir.c callers.

I guess the two are supposed to be equivalent, or else we'd have weird
discrepancies between commands. So maybe just a weird existing oddity
that we don't need to worry about here.

There is one other interesting corner case here. If I do this in
git.git, for example:

  git last-modified -t Documentation/technical/

it shows an entry for Documentation/, which we both find weird. And the
patch above would remove that. But it also shows an entry for
Documentation/technical/, which _is_ within the pathspec and would still
be shown after the patch above. That's OK for the optimization we're
talking about (it would be part of the filter key), but I do find it
still a little funny. The invocation above, at least as we used to use
it as blame-tree at GitHub, is really about asking for the entries
inside that directory, not the directory itself.

Perhaps not worth worrying too much about, though. The caller can easily
ignore the extra entry.

> If, on the other hand, we wanted to retain "d" in the output (which I am
> inclined to suggest is a bad idea), we could keep a list of paths which
> are not covered by the given pathspec.

Yeah, your analysis here makes sense, but I agree that it is not worth
retaining "d". Besides reducing our ability to optimize, it is IMHO just
plain confusing to have in the output.

-Peff