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