Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 18, 2018, 19:51 UTC
Message-ID: <87d0r217vr.fsf@evledraar.gmail.com>
In-Reply-To: <20181118164800.32759-1-pclouds@gmail.com>

On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:

Show 14 quoted lines

> 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.