Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
On Mon, Nov 19, 2018 at 12:42 PM Ævar Arnfjörð Bjarmason wrote: > > > 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 -- :(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. > > > > The main patch is the last one. The others are just to open a path to > > pass "struct index_state *" down to tree_entry_interesting(). This may > > become standard procedure because we don't want to stick the_index (or > > the_repository) here and there. > > Another side-note (this thread is turning into my personal blog at this > point...) I found an old related thread: > https://public-inbox.org/git/20170509225219.gb106...@google.com/ > > So this series fixes 1/2 of the issues noted there, but git-ls-tree will > still die with the same error. If you mean BUG(), not it does not. There are just a couple more places besides tree_entry_interesting() and match_pathspec() that need to be aware of all the magic things. ls-tree is one of those but it's well guarded and will display this instead $ git ls-tree HEAD ':(attr:abc=def)' fatal: :(attr:abc=def): pathspec magic not supported by this command: 'attr' If you mean making ls-tree support :(attr) and other stuff, it's not going to happen in near future. It may be nice to switch to tree_entry_interesting() in this command, but if it was easy to do I would have done it years ago :P -- Duy
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
On Mon, Nov 19, 2018 at 12:16 PM Ævar Arnfjörð Bjarmason wrote: > 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)'". I think we allow :(exclude,attr:diff=perl) which should "exclude all paths that have diff=perl attribute". It's actually tested in t6135 for ls-files (but I didn't add the same test for 'git grep' because I was so confident it would work; I'll work on that). -- Duy
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
On Sun, Nov 18, 2018 at 08:51:52PM +0100, Ævar Arnfjörð Bjarmason wrote:
> > 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.
I think that ship already sailed with the fact that "git log -p" will
show diffs using the worktree attrs. I agree that it would sometimes be
nice to specify attributes from a particular tree, but at this point the
default probably needs to remain as it is.
-Peff
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
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 -- :(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. > > The main patch is the last one. The others are just to open a path to > pass "struct index_state *" down to tree_entry_interesting(). This may > become standard procedure because we don't want to stick the_index (or > the_repository) here and there. Another side-note (this thread is turning into my personal blog at this point...) I found an old related thread: https://public-inbox.org/git/20170509225219.gb106...@google.com/ So this series fixes 1/2 of the issues noted there, but git-ls-tree will still die with the same error.
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
On Sun, Nov 18 2018, Ævar Arnfjörð Bjarmason wrote:
> 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 -- :(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)'".
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
Nguyễn Thái Ngọc Duy writes: > 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. This is pretty much deliberately designed to be so; the set of the attributes in HEAD may be stale but by taking the contents from the working tree the user can work it around. > The main patch is the last one. The others are just to open a path to > pass "struct index_state *" down to tree_entry_interesting(). This may > become standard procedure because we don't want to stick the_index (or > the_repository) here and there. Yup.
Re: [PATCH 0/5] Make :(attr) pathspec work with "git log"
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 -- :(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.
[PATCH 0/5] Make :(attr) pathspec work with "git log"
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 -- :(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. The main patch is the last one. The others are just to open a path to pass "struct index_state *" down to tree_entry_interesting(). This may become standard procedure because we don't want to stick the_index (or the_repository) here and there. Nguyễn Thái Ngọc Duy (5): tree.c: make read_tree*() take 'struct repository *' tree-walk.c: make tree_entry_interesting() take an index pathspec.h: clean up "extern" in function declarations dir.c: move, rename and export match_attrs() tree-walk: support :(attr) matching Documentation/glossary-content.txt | 2 + archive.c | 6 +- builtin/checkout.c | 3 +- builtin/grep.c | 3 +- builtin/log.c | 5 +- builtin/ls-files.c | 2 +- builtin/ls-tree.c | 3 +- builtin/merge-tree.c | 2 +- dir.c | 41 +- list-objects.c | 3 +- merge-recursive.c | 3 +- pathspec.c | 38 + pathspec.h | 27 + revision.c | 1 + t/t6135-pathspec-with-attrs.sh | 58 ++- tree-diff.c| 3 +- tree-walk.c| 89 ++ tree-walk.h| 10 ++-- tree.c | 21 --- tree.h | 18 +++--- unpack-trees.c | 6 +- 21 files changed, 235 insertions(+), 109 deletions(-) -- 2.19.1.1327.g328c130451.dirty
