On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:
> When :(attr) was added, it supported one of the two main pathspec
> matching functions, the one that works on a list of paths. The other
> one works on a tree, tree_entry_interesting(), which gets :(attr)
> support in this series.
>
> With this, "git grep <pattern> <tree> -- :(attr)" or "git log :(attr)"
> will not abort with BUG() anymore.
>
> But this also reveals an interesting thing: even though we walk on a
> tree, we check attributes from _worktree_ (and optionally fall back to
> the index). This is how attributes are implemented since forever. I
> think this is not a big deal if we communicate clearly with the user.
> But otherwise, this series can be scraped, as reading attributes from
> a specific tree could be a lot of work.
I'm very happy to see this implemented, and I think the behavior
described here is the right way to go. E.g. in git.git we have diff=perl
entries in .gitattributes. It would suck if:
git log ':(attr:diff=perl)'
Would only list commits as far as 20460635a8 (".gitattributes: use the
"perl" differ for Perl", 2018-04-26), since that's when we stop having
that attribute. Ditto for wanting to run "grep" on e.g. perl files in
2.12.0.
I have also run into cases where I want to use a .gitattributes file
from a specific commit. E.g. when writing pre-receive hooks where I've
wanted the .gitattributes of the commit being pushed to configure
something about it. But as you note this isn't supported at all.
But a concern is whether we should be making :(attr:*) behave like this
for now. Are we going to regret it later? I don't think so, I think
wanting to use the current working tree's / index's is the most sane
default, and if we get the ability to read it from revisions as we
e.g. walk the log it would make most sense to just call that
:(treeattr:*) or something like that.
