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

From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Nov 19, 2018, 11:16 UTC
Message-ID: <878t1p1fnr.fsf@evledraar.gmail.com>
In-Reply-To: <87d0r217vr.fsf@evledraar.gmail.com>

On Sun, Nov 18 2018, Ævar Arnfjörð Bjarmason wrote:

Show 39 quoted lines

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

As an aside, how do you do the inverse of matching for an attribute by value? I.e.:

    $ git ls-files | wc -l; git ls-files ':(attr:diff=perl)' | wc -l
    3522
    65

I'd like something gives me all files that don't match diff=perl, i.e. 3522-65 = 3457 files, or what I'd get if I constructed such a match manually with excludes:

    $ git ls-files $(grep diff=perl .gitattributes | cut -d ' ' -f1 | sed 's!^!:(exclude)!') | wc -l
    3457

From my reading of parse_pathspec_attr_match() and match_attrs() this isn't possible and I'd need to support ':(attr:diff!=perl)' via a new MATCH_NOT_VALUE mode. But I wanted to make sure I wasn't missing some subtlety, i.e. that this was implemented already via some other feature.

I thought I could do:

    git ls-files ':(exclude):(attr:diff=perl)'

But we don't support chaining like that, and this would only exclude a file that's actually called ":(attr:diff=perl)". I.e. created via something like "touch ':(attr:diff=perl)'".