{"thread":{"id":"49848","subject":"[PATCH 0/5] Make :(attr) pathspec work with \"git log\"","startedAt":"2018-11-18T16:48:11Z","lastAt":"2018-11-19T15:33:54Z","messageCount":15,"participants":["Nguyễn Thái Ngọc Duy","Ævar Arnfjörð Bjarmason","Junio C Hamano","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"363583","messageId":"20181118164800.32759-1-pclouds@gmail.com","threadId":"49848","inReplyTo":null,"subject":"[PATCH 0/5] Make :(attr) pathspec work with \"git log\"","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-18T16:47:55Z","receivedAt":"2018-11-18T16:48:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When :(attr) was added, it supported one of the two main pathspec\nmatching functions, the one that works on a list of paths. The other\none works on a tree, tree_entry_interesting(), which gets :(attr)\nsupport in this series.\n\nWith this, \"git grep <pattern> <tree> -- :(attr)\" or \"git log :(attr)\"\nwill not abort with BUG() anymore.\n\nBut this also reveals an interesting thing: even though we walk on a\ntree, we check attributes from _worktree_ (and optionally fall back to\nthe index). This is how attributes are implemented since forever. I\nthink this is not a big deal if we communicate clearly with the user.\nBut otherwise, this series can be scraped, as reading attributes from\na specific tree could be a lot of work.\n\nThe main patch is the last one. The others are just to open a path to\npass \"struct index_state *\" down to tree_entry_interesting(). This may\nbecome standard procedure because we don't want to stick the_index (or\nthe_repository) here and there.\n\nNguyễn Thái Ngọc Duy (5):\n  tree.c: make read_tree*() take 'struct repository *'\n  tree-walk.c: make tree_entry_interesting() take an index\n  pathspec.h: clean up \"extern\" in function declarations\n  dir.c: move, rename and export match_attrs()\n  tree-walk: support :(attr) matching\n\n Documentation/glossary-content.txt |  2 +\n archive.c                          |  6 +-\n builtin/checkout.c                 |  3 +-\n builtin/grep.c                     |  3 +-\n builtin/log.c                      |  5 +-\n builtin/ls-files.c                 |  2 +-\n builtin/ls-tree.c                  |  3 +-\n builtin/merge-tree.c               |  2 +-\n dir.c                              | 41 +-------------\n list-objects.c                     |  3 +-\n merge-recursive.c                  |  3 +-\n pathspec.c                         | 38 +++++++++++++\n pathspec.h                         | 27 +++++----\n revision.c                         |  1 +\n t/t6135-pathspec-with-attrs.sh     | 58 ++++++++++++++++++-\n tree-diff.c                        |  3 +-\n tree-walk.c                        | 89 ++++++++++++++++++++++--------\n tree-walk.h                        | 10 ++--\n tree.c                             | 21 ++++---\n tree.h                             | 18 +++---\n unpack-trees.c                     |  6 +-\n 21 files changed, 235 insertions(+), 109 deletions(-)\n\n-- \n2.19.1.1327.g328c130451.dirty\n\n"},{"id":"363584","messageId":"20181118164800.32759-2-pclouds@gmail.com","threadId":"49848","inReplyTo":"20181118164800.32759-1-pclouds@gmail.com","subject":"[PATCH 1/5] tree.c: make read_tree*() take 'struct repository *'","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-18T16:47:56Z","receivedAt":"2018-11-18T16:48:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"These functions call tree_entry_interesting() which will soon require\na 'struct index_state *' to be passed in. Instead of just changing the\nfunction signature to take an index, update to take a repo instead\nbecause these functions do need object database access.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n archive.c          |  6 ++++--\n builtin/checkout.c |  3 ++-\n builtin/log.c      |  5 +++--\n builtin/ls-files.c |  2 +-\n builtin/ls-tree.c  |  3 ++-\n merge-recursive.c  |  3 ++-\n tree.c             | 18 ++++++++++--------\n tree.h             | 18 +++++++++++-------\n 8 files changed, 35 insertions(+), 23 deletions(-)\n\ndiff --git a/archive.c b/archive.c\nindex fd556c28e4..bfa9cc20c9 100644\n--- a/archive.c\n+++ b/archive.c\n@@ -285,7 +285,8 @@ int write_archive_entries(struct archiver_args *args,\n \t\tgit_attr_set_direction(GIT_ATTR_INDEX);\n \t}\n \n-\terr = read_tree_recursive(args->tree, \"\", 0, 0, &args->pathspec,\n+\terr = read_tree_recursive(args->repo, args->tree, \"\",\n+\t\t\t\t  0, 0, &args->pathspec,\n \t\t\t\t  queue_or_write_archive_entry,\n \t\t\t\t  &context);\n \tif (err == READ_TREE_RECURSIVE)\n@@ -346,7 +347,8 @@ static int path_exists(struct archiver_args *args, const char *path)\n \tctx.args = args;\n \tparse_pathspec(&ctx.pathspec, 0, 0, \"\", paths);\n \tctx.pathspec.recursive = 1;\n-\tret = read_tree_recursive(args->tree, \"\", 0, 0, &ctx.pathspec,\n+\tret = read_tree_recursive(args->repo, args->tree, \"\",\n+\t\t\t\t  0, 0, &ctx.pathspec,\n \t\t\t\t  reject_entry, &ctx);\n \tclear_pathspec(&ctx.pathspec);\n \treturn ret != 0;\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex acdafc6e4c..c9dda8e82e 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -115,7 +115,8 @@ static int update_some(const struct object_id *oid, struct strbuf *base,\n \n static int read_tree_some(struct tree *tree, const struct pathspec *pathspec)\n {\n-\tread_tree_recursive(tree, \"\", 0, 0, pathspec, update_some, NULL);\n+\tread_tree_recursive(the_repository, tree, \"\", 0, 0,\n+\t\t\t    pathspec, update_some, NULL);\n \n \t/* update the index with the given tree's info\n \t * for all args, expanding wildcards, and exit\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 061d4fd864..d493fa915e 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -639,8 +639,9 @@ int cmd_show(int argc, const char **argv, const char *prefix)\n \t\t\t\t\tdiff_get_color_opt(&rev.diffopt, DIFF_COMMIT),\n \t\t\t\t\tname,\n \t\t\t\t\tdiff_get_color_opt(&rev.diffopt, DIFF_RESET));\n-\t\t\tread_tree_recursive((struct tree *)o, \"\", 0, 0, &match_all,\n-\t\t\t\t\tshow_tree_object, rev.diffopt.file);\n+\t\t\tread_tree_recursive(the_repository, (struct tree *)o, \"\",\n+\t\t\t\t\t    0, 0, &match_all, show_tree_object,\n+\t\t\t\t\t    rev.diffopt.file);\n \t\t\trev.shown_one = 1;\n \t\t\tbreak;\n \t\tcase OBJ_COMMIT:\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 7f9919a362..8db0cccbf3 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -441,7 +441,7 @@ void overlay_tree_on_index(struct index_state *istate,\n \t\t\t       PATHSPEC_PREFER_CWD, prefix, matchbuf);\n \t} else\n \t\tmemset(&pathspec, 0, sizeof(pathspec));\n-\tif (read_tree(tree, 1, &pathspec, istate))\n+\tif (read_tree(the_repository, tree, 1, &pathspec, istate))\n \t\tdie(\"unable to read tree entries %s\", tree_name);\n \n \tfor (i = 0; i < istate->cache_nr; i++) {\ndiff --git a/builtin/ls-tree.c b/builtin/ls-tree.c\nindex fe3b952cb3..6855f7fea5 100644\n--- a/builtin/ls-tree.c\n+++ b/builtin/ls-tree.c\n@@ -185,5 +185,6 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)\n \ttree = parse_tree_indirect(&oid);\n \tif (!tree)\n \t\tdie(\"not a tree object\");\n-\treturn !!read_tree_recursive(tree, \"\", 0, 0, &pathspec, show_tree, NULL);\n+\treturn !!read_tree_recursive(the_repository, tree, \"\", 0, 0,\n+\t\t\t\t     &pathspec, show_tree, NULL);\n }\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex acc2f64a4e..b9467f5ecf 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -463,7 +463,8 @@ static void get_files_dirs(struct merge_options *o, struct tree *tree)\n {\n \tstruct pathspec match_all;\n \tmemset(&match_all, 0, sizeof(match_all));\n-\tread_tree_recursive(tree, \"\", 0, 0, &match_all, save_files_dirs, o);\n+\tread_tree_recursive(the_repository, tree, \"\", 0, 0,\n+\t\t\t    &match_all, save_files_dirs, o);\n }\n \n static int get_tree_entry_if_blob(const struct object_id *tree,\ndiff --git a/tree.c b/tree.c\nindex 215d3fdc7c..ecd8c8ac29 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -60,7 +60,8 @@ static int read_one_entry_quick(const struct object_id *oid, struct strbuf *base\n \t\t\t\t  ADD_CACHE_JUST_APPEND);\n }\n \n-static int read_tree_1(struct tree *tree, struct strbuf *base,\n+static int read_tree_1(struct repository *r,\n+\t\t       struct tree *tree, struct strbuf *base,\n \t\t       int stage, const struct pathspec *pathspec,\n \t\t       read_tree_fn_t fn, void *context)\n {\n@@ -99,7 +100,7 @@ static int read_tree_1(struct tree *tree, struct strbuf *base,\n \t\telse if (S_ISGITLINK(entry.mode)) {\n \t\t\tstruct commit *commit;\n \n-\t\t\tcommit = lookup_commit(the_repository, entry.oid);\n+\t\t\tcommit = lookup_commit(r, entry.oid);\n \t\t\tif (!commit)\n \t\t\t\tdie(\"Commit %s in submodule path %s%s not found\",\n \t\t\t\t    oid_to_hex(entry.oid),\n@@ -118,7 +119,7 @@ static int read_tree_1(struct tree *tree, struct strbuf *base,\n \t\tlen = tree_entry_len(&entry);\n \t\tstrbuf_add(base, entry.path, len);\n \t\tstrbuf_addch(base, '/');\n-\t\tretval = read_tree_1(lookup_tree(the_repository, &oid),\n+\t\tretval = read_tree_1(r, lookup_tree(r, &oid),\n \t\t\t\t     base, stage, pathspec,\n \t\t\t\t     fn, context);\n \t\tstrbuf_setlen(base, oldlen);\n@@ -128,7 +129,8 @@ static int read_tree_1(struct tree *tree, struct strbuf *base,\n \treturn 0;\n }\n \n-int read_tree_recursive(struct tree *tree,\n+int read_tree_recursive(struct repository *r,\n+\t\t\tstruct tree *tree,\n \t\t\tconst char *base, int baselen,\n \t\t\tint stage, const struct pathspec *pathspec,\n \t\t\tread_tree_fn_t fn, void *context)\n@@ -137,7 +139,7 @@ int read_tree_recursive(struct tree *tree,\n \tint ret;\n \n \tstrbuf_add(&sb, base, baselen);\n-\tret = read_tree_1(tree, &sb, stage, pathspec, fn, context);\n+\tret = read_tree_1(r, tree, &sb, stage, pathspec, fn, context);\n \tstrbuf_release(&sb);\n \treturn ret;\n }\n@@ -152,8 +154,8 @@ static int cmp_cache_name_compare(const void *a_, const void *b_)\n \t\t\t\t  ce2->name, ce2->ce_namelen, ce_stage(ce2));\n }\n \n-int read_tree(struct tree *tree, int stage, struct pathspec *match,\n-\t      struct index_state *istate)\n+int read_tree(struct repository *r, struct tree *tree, int stage,\n+\t      struct pathspec *match, struct index_state *istate)\n {\n \tread_tree_fn_t fn = NULL;\n \tint i, err;\n@@ -181,7 +183,7 @@ int read_tree(struct tree *tree, int stage, struct pathspec *match,\n \n \tif (!fn)\n \t\tfn = read_one_entry_quick;\n-\terr = read_tree_recursive(tree, \"\", 0, stage, match, fn, istate);\n+\terr = read_tree_recursive(r, tree, \"\", 0, stage, match, fn, istate);\n \tif (fn == read_one_entry || err)\n \t\treturn err;\n \ndiff --git a/tree.h b/tree.h\nindex d4807dc805..9383745073 100644\n--- a/tree.h\n+++ b/tree.h\n@@ -3,7 +3,7 @@\n \n #include \"object.h\"\n \n-extern const char *tree_type;\n+struct repository;\n struct strbuf;\n \n struct tree {\n@@ -12,6 +12,8 @@ struct tree {\n \tunsigned long size;\n };\n \n+extern const char *tree_type;\n+\n struct tree *lookup_tree(struct repository *r, const struct object_id *oid);\n \n int parse_tree_buffer(struct tree *item, void *buffer, unsigned long size);\n@@ -29,12 +31,14 @@ struct tree *parse_tree_indirect(const struct object_id *oid);\n #define READ_TREE_RECURSIVE 1\n typedef int (*read_tree_fn_t)(const struct object_id *, struct strbuf *, const char *, unsigned int, int, void *);\n \n-extern int read_tree_recursive(struct tree *tree,\n-\t\t\t       const char *base, int baselen,\n-\t\t\t       int stage, const struct pathspec *pathspec,\n-\t\t\t       read_tree_fn_t fn, void *context);\n+int read_tree_recursive(struct repository *r,\n+\t\t\tstruct tree *tree,\n+\t\t\tconst char *base, int baselen,\n+\t\t\tint stage, const struct pathspec *pathspec,\n+\t\t\tread_tree_fn_t fn, void *context);\n \n-extern int read_tree(struct tree *tree, int stage, struct pathspec *pathspec,\n-\t\t     struct index_state *istate);\n+int read_tree(struct repository *r, struct tree *tree,\n+\t      int stage, struct pathspec *pathspec,\n+\t      struct index_state *istate);\n \n #endif /* TREE_H */\n-- \n2.19.1.1327.g328c130451.dirty\n\n"},{"id":"363585","messageId":"20181118164800.32759-3-pclouds@gmail.com","threadId":"49848","inReplyTo":"20181118164800.32759-1-pclouds@gmail.com","subject":"[PATCH 2/5] tree-walk.c: make tree_entry_interesting() take an index","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-18T16:47:57Z","receivedAt":"2018-11-18T16:48:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In order to support :(attr) when matching pathspec on a tree,\ntree_entry_interesting() needs to take an index (because\ngit_check_attr() needs it). This is the preparation step for it. This\nalso makes it clearer what index we fall back to when looking up\nattributes during an unpack-trees operation: the source index.\n\nThis also fixes revs->pruning.repo initialization that should have\nbeen done in 2abf350385 (revision.c: remove implicit dependency on\nthe_index - 2018-09-21). Without it, skip_uninteresting() will\ndereference a NULL pointer through this call chain\n\n  get_revision(revs)\n  get_revision_internal\n  get_revision_1\n  try_to_simplify_commit\n  rev_compare_tree\n  diff_tree_oid(..., &revs->pruning)\n  ll_diff_tree_oid\n  diff_tree_paths\n  ll_diff_tree\n  skip_uninteresting\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/grep.c       |  3 ++-\n builtin/merge-tree.c |  2 +-\n list-objects.c       |  3 ++-\n revision.c           |  1 +\n tree-diff.c          |  3 ++-\n tree-walk.c          | 22 ++++++++++++++--------\n tree-walk.h          | 10 ++++++----\n tree.c               |  3 ++-\n unpack-trees.c       |  6 +++---\n 9 files changed, 33 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 56e4a11052..f6e086c287 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -568,7 +568,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,\n \n \t\tif (match != all_entries_interesting) {\n \t\t\tstrbuf_addstr(&name, base->buf + tn_len);\n-\t\t\tmatch = tree_entry_interesting(&entry, &name,\n+\t\t\tmatch = tree_entry_interesting(repo->index,\n+\t\t\t\t\t\t       &entry, &name,\n \t\t\t\t\t\t       0, pathspec);\n \t\t\tstrbuf_setlen(&name, name_base_len);\n \ndiff --git a/builtin/merge-tree.c b/builtin/merge-tree.c\nindex 70f6fc9167..4984b7e12e 100644\n--- a/builtin/merge-tree.c\n+++ b/builtin/merge-tree.c\n@@ -346,7 +346,7 @@ static void merge_trees(struct tree_desc t[3], const char *base)\n \n \tsetup_traverse_info(&info, base);\n \tinfo.fn = threeway_callback;\n-\ttraverse_trees(3, t, &info);\n+\ttraverse_trees(&the_index, 3, t, &info);\n }\n \n static void *get_tree_descriptor(struct tree_desc *desc, const char *rev)\ndiff --git a/list-objects.c b/list-objects.c\nindex c41cc80db5..63c395d9c2 100644\n--- a/list-objects.c\n+++ b/list-objects.c\n@@ -113,7 +113,8 @@ static void process_tree_contents(struct traversal_context *ctx,\n \n \twhile (tree_entry(&desc, &entry)) {\n \t\tif (match != all_entries_interesting) {\n-\t\t\tmatch = tree_entry_interesting(&entry, base, 0,\n+\t\t\tmatch = tree_entry_interesting(ctx->revs->repo->index,\n+\t\t\t\t\t\t       &entry, base, 0,\n \t\t\t\t\t\t       &ctx->revs->diffopt.pathspec);\n \t\t\tif (match == all_entries_not_interesting)\n \t\t\t\tbreak;\ndiff --git a/revision.c b/revision.c\nindex bdd3e7c9f1..7ced1ee02b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1459,6 +1459,7 @@ void repo_init_revisions(struct repository *r,\n \trevs->abbrev = DEFAULT_ABBREV;\n \trevs->ignore_merges = 1;\n \trevs->simplify_history = 1;\n+\trevs->pruning.repo = r;\n \trevs->pruning.flags.recursive = 1;\n \trevs->pruning.flags.quick = 1;\n \trevs->pruning.add_remove = file_add_remove;\ndiff --git a/tree-diff.c b/tree-diff.c\nindex 0e54324610..34ee3b13b8 100644\n--- a/tree-diff.c\n+++ b/tree-diff.c\n@@ -299,7 +299,8 @@ static void skip_uninteresting(struct tree_desc *t, struct strbuf *base,\n \tenum interesting match;\n \n \twhile (t->size) {\n-\t\tmatch = tree_entry_interesting(&t->entry, base, 0, &opt->pathspec);\n+\t\tmatch = tree_entry_interesting(opt->repo->index, &t->entry,\n+\t\t\t\t\t       base, 0, &opt->pathspec);\n \t\tif (match) {\n \t\t\tif (match == all_entries_not_interesting)\n \t\t\t\tt->size = 0;\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 79bafbd1a2..517bcdecf9 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -365,7 +365,8 @@ static void free_extended_entry(struct tree_desc_x *t)\n \t}\n }\n \n-static inline int prune_traversal(struct name_entry *e,\n+static inline int prune_traversal(struct index_state *istate,\n+\t\t\t\t  struct name_entry *e,\n \t\t\t\t  struct traverse_info *info,\n \t\t\t\t  struct strbuf *base,\n \t\t\t\t  int still_interesting)\n@@ -374,10 +375,13 @@ static inline int prune_traversal(struct name_entry *e,\n \t\treturn 2;\n \tif (still_interesting < 0)\n \t\treturn still_interesting;\n-\treturn tree_entry_interesting(e, base, 0, info->pathspec);\n+\treturn tree_entry_interesting(istate, e, base,\n+\t\t\t\t      0, info->pathspec);\n }\n \n-int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n+int traverse_trees(struct index_state *istate,\n+\t\t   int n, struct tree_desc *t,\n+\t\t   struct traverse_info *info)\n {\n \tint error = 0;\n \tstruct name_entry *entry = xmalloc(n*sizeof(*entry));\n@@ -461,7 +465,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info)\n \t\t}\n \t\tif (!mask)\n \t\t\tbreak;\n-\t\tinteresting = prune_traversal(e, info, &base, interesting);\n+\t\tinteresting = prune_traversal(istate, e, info, &base, interesting);\n \t\tif (interesting < 0)\n \t\t\tbreak;\n \t\tif (interesting) {\n@@ -928,7 +932,8 @@ static int match_wildcard_base(const struct pathspec_item *item,\n  * Pre-condition: either baselen == base_offset (i.e. empty path)\n  * or base[baselen-1] == '/' (i.e. with trailing slash).\n  */\n-static enum interesting do_match(const struct name_entry *entry,\n+static enum interesting do_match(struct index_state *istate,\n+\t\t\t\t const struct name_entry *entry,\n \t\t\t\t struct strbuf *base, int base_offset,\n \t\t\t\t const struct pathspec *ps,\n \t\t\t\t int exclude)\n@@ -1090,12 +1095,13 @@ static enum interesting do_match(const struct name_entry *entry,\n  * Pre-condition: either baselen == base_offset (i.e. empty path)\n  * or base[baselen-1] == '/' (i.e. with trailing slash).\n  */\n-enum interesting tree_entry_interesting(const struct name_entry *entry,\n+enum interesting tree_entry_interesting(struct index_state *istate,\n+\t\t\t\t\tconst struct name_entry *entry,\n \t\t\t\t\tstruct strbuf *base, int base_offset,\n \t\t\t\t\tconst struct pathspec *ps)\n {\n \tenum interesting positive, negative;\n-\tpositive = do_match(entry, base, base_offset, ps, 0);\n+\tpositive = do_match(istate, entry, base, base_offset, ps, 0);\n \n \t/*\n \t * case | entry | positive | negative | result\n@@ -1132,7 +1138,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry,\n \t    positive <= entry_not_interesting) /* #1, #2, #11, #12 */\n \t\treturn positive;\n \n-\tnegative = do_match(entry, base, base_offset, ps, 1);\n+\tnegative = do_match(istate, entry, base, base_offset, ps, 1);\n \n \t/* #8, #18 */\n \tif (positive == all_entries_interesting &&\ndiff --git a/tree-walk.h b/tree-walk.h\nindex 196831007e..eefd26bb62 100644\n--- a/tree-walk.h\n+++ b/tree-walk.h\n@@ -1,6 +1,7 @@\n #ifndef TREE_WALK_H\n #define TREE_WALK_H\n \n+struct index_state;\n struct strbuf;\n \n struct name_entry {\n@@ -48,7 +49,7 @@ void *fill_tree_descriptor(struct tree_desc *desc, const struct object_id *oid);\n \n struct traverse_info;\n typedef int (*traverse_callback_t)(int n, unsigned long mask, unsigned long dirmask, struct name_entry *entry, struct traverse_info *);\n-int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info);\n+int traverse_trees(struct index_state *istate, int n, struct tree_desc *t, struct traverse_info *info);\n \n enum follow_symlinks_result {\n \tFOUND = 0, /* This includes out-of-tree links */\n@@ -98,8 +99,9 @@ enum interesting {\n \tall_entries_interesting = 2 /* yes, and all subsequent entries will be */\n };\n \n-extern enum interesting tree_entry_interesting(const struct name_entry *,\n-\t\t\t\t\t       struct strbuf *, int,\n-\t\t\t\t\t       const struct pathspec *ps);\n+enum interesting tree_entry_interesting(struct index_state *istate,\n+\t\t\t\t\tconst struct name_entry *,\n+\t\t\t\t\tstruct strbuf *, int,\n+\t\t\t\t\tconst struct pathspec *ps);\n \n #endif\ndiff --git a/tree.c b/tree.c\nindex ecd8c8ac29..0b5c84d0d7 100644\n--- a/tree.c\n+++ b/tree.c\n@@ -78,7 +78,8 @@ static int read_tree_1(struct repository *r,\n \n \twhile (tree_entry(&desc, &entry)) {\n \t\tif (retval != all_entries_interesting) {\n-\t\t\tretval = tree_entry_interesting(&entry, base, 0, pathspec);\n+\t\t\tretval = tree_entry_interesting(r->index, &entry,\n+\t\t\t\t\t\t\tbase, 0, pathspec);\n \t\t\tif (retval == all_entries_not_interesting)\n \t\t\t\tbreak;\n \t\t\tif (retval == entry_not_interesting)\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 7570df481b..7f9548e741 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -794,6 +794,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \t\t\t\t    struct name_entry *names,\n \t\t\t\t    struct traverse_info *info)\n {\n+\tstruct unpack_trees_options *o = info->data;\n \tint i, ret, bottom;\n \tint nr_buf = 0;\n \tstruct tree_desc t[MAX_UNPACK_TREES];\n@@ -804,7 +805,6 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \n \tnr_entries = all_trees_same_as_cache_tree(n, dirmask, names, info);\n \tif (nr_entries > 0) {\n-\t\tstruct unpack_trees_options *o = info->data;\n \t\tint pos = index_pos_by_traverse_info(names, info);\n \n \t\tif (!o->merge || df_conflicts)\n@@ -863,7 +863,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,\n \t}\n \n \tbottom = switch_cache_bottom(&newinfo);\n-\tret = traverse_trees(n, t, &newinfo);\n+\tret = traverse_trees(o->src_index, n, t, &newinfo);\n \trestore_cache_bottom(&newinfo, bottom);\n \n \tfor (i = 0; i < nr_buf; i++)\n@@ -1550,7 +1550,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \t\t}\n \n \t\ttrace_performance_enter();\n-\t\tret = traverse_trees(len, t, &info);\n+\t\tret = traverse_trees(o->src_index, len, t, &info);\n \t\ttrace_performance_leave(\"traverse_trees\");\n \t\tif (ret < 0)\n \t\t\tgoto return_failed;\n-- \n2.19.1.1327.g328c130451.dirty\n\n"},{"id":"363586","messageId":"20181118164800.32759-4-pclouds@gmail.com","threadId":"49848","inReplyTo":"20181118164800.32759-1-pclouds@gmail.com","subject":"[PATCH 3/5] pathspec.h: clean up \"extern\" in function declarations","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-18T16:47:58Z","receivedAt":"2018-11-18T16:48:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\"extern\" on functions is not required and the trend has been removing\nit from header files.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n pathspec.h | 24 ++++++++++++------------\n 1 file changed, 12 insertions(+), 12 deletions(-)\n\ndiff --git a/pathspec.h b/pathspec.h\nindex a6525a6551..5fd781d695 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -80,13 +80,13 @@ struct pathspec {\n  * Any arguments used are copied. It is safe for the caller to modify\n  * or free 'prefix' and 'args' after calling this function.\n  */\n-extern void parse_pathspec(struct pathspec *pathspec,\n-\t\t\t   unsigned magic_mask,\n-\t\t\t   unsigned flags,\n-\t\t\t   const char *prefix,\n-\t\t\t   const char **args);\n-extern void copy_pathspec(struct pathspec *dst, const struct pathspec *src);\n-extern void clear_pathspec(struct pathspec *);\n+void parse_pathspec(struct pathspec *pathspec,\n+\t\t    unsigned magic_mask,\n+\t\t    unsigned flags,\n+\t\t    const char *prefix,\n+\t\t    const char **args);\n+void copy_pathspec(struct pathspec *dst, const struct pathspec *src);\n+void clear_pathspec(struct pathspec *);\n \n static inline int ps_strncmp(const struct pathspec_item *item,\n \t\t\t     const char *s1, const char *s2, size_t n)\n@@ -106,10 +106,10 @@ static inline int ps_strcmp(const struct pathspec_item *item,\n \t\treturn strcmp(s1, s2);\n }\n \n-extern void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n-\t\t\t\t\t       const struct index_state *istate,\n-\t\t\t\t\t       char *seen);\n-extern char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,\n-\t\t\t\t\t\t   const struct index_state *istate);\n+void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n+\t\t\t\t\tconst struct index_state *istate,\n+\t\t\t\t\tchar *seen);\n+char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,\n+\t\t\t\t\t    const struct index_state *istate);\n \n #endif /* PATHSPEC_H */\n-- \n2.19.1.1327.g328c130451.dirty\n\n"},{"id":"363587","messageId":"20181118164800.32759-5-pclouds@gmail.com","threadId":"49848","inReplyTo":"20181118164800.32759-1-pclouds@gmail.com","subject":"[PATCH 4/5] dir.c: move, rename and export match_attrs()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-18T16:47:59Z","receivedAt":"2018-11-18T16:48:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The function will be reused for matching attributes in pathspec when\nwalking trees (currently it's used for matching pathspec when walking\na list). pathspec.c would be a more neutral place for this.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n dir.c      | 41 ++---------------------------------------\n pathspec.c | 38 ++++++++++++++++++++++++++++++++++++++\n pathspec.h |  3 +++\n 3 files changed, 43 insertions(+), 39 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex ab6477d777..2128448219 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -276,44 +276,6 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,\n #define DO_MATCH_DIRECTORY (1<<1)\n #define DO_MATCH_SUBMODULE (1<<2)\n \n-static int match_attrs(const struct index_state *istate,\n-\t\t       const char *name, int namelen,\n-\t\t       const struct pathspec_item *item)\n-{\n-\tint i;\n-\tchar *to_free = NULL;\n-\n-\tif (name[namelen])\n-\t\tname = to_free = xmemdupz(name, namelen);\n-\n-\tgit_check_attr(istate, name, item->attr_check);\n-\n-\tfree(to_free);\n-\n-\tfor (i = 0; i < item->attr_match_nr; i++) {\n-\t\tconst char *value;\n-\t\tint matched;\n-\t\tenum attr_match_mode match_mode;\n-\n-\t\tvalue = item->attr_check->items[i].value;\n-\t\tmatch_mode = item->attr_match[i].match_mode;\n-\n-\t\tif (ATTR_TRUE(value))\n-\t\t\tmatched = (match_mode == MATCH_SET);\n-\t\telse if (ATTR_FALSE(value))\n-\t\t\tmatched = (match_mode == MATCH_UNSET);\n-\t\telse if (ATTR_UNSET(value))\n-\t\t\tmatched = (match_mode == MATCH_UNSPECIFIED);\n-\t\telse\n-\t\t\tmatched = (match_mode == MATCH_VALUE &&\n-\t\t\t\t   !strcmp(item->attr_match[i].value, value));\n-\t\tif (!matched)\n-\t\t\treturn 0;\n-\t}\n-\n-\treturn 1;\n-}\n-\n /*\n  * Does 'match' match the given name?\n  * A match is found if\n@@ -367,7 +329,8 @@ static int match_pathspec_item(const struct index_state *istate,\n \t    strncmp(item->match, name - prefix, item->prefix))\n \t\treturn 0;\n \n-\tif (item->attr_match_nr && !match_attrs(istate, name, namelen, item))\n+\tif (item->attr_match_nr &&\n+\t    !match_pathspec_attrs(istate, name, namelen, item))\n \t\treturn 0;\n \n \t/* If the match was just the prefix, we matched */\ndiff --git a/pathspec.c b/pathspec.c\nindex 6f005996fd..e85298f68c 100644\n--- a/pathspec.c\n+++ b/pathspec.c\n@@ -659,3 +659,41 @@ void clear_pathspec(struct pathspec *pathspec)\n \tFREE_AND_NULL(pathspec->items);\n \tpathspec->nr = 0;\n }\n+\n+int match_pathspec_attrs(const struct index_state *istate,\n+\t\t\t const char *name, int namelen,\n+\t\t\t const struct pathspec_item *item)\n+{\n+\tint i;\n+\tchar *to_free = NULL;\n+\n+\tif (name[namelen])\n+\t\tname = to_free = xmemdupz(name, namelen);\n+\n+\tgit_check_attr(istate, name, item->attr_check);\n+\n+\tfree(to_free);\n+\n+\tfor (i = 0; i < item->attr_match_nr; i++) {\n+\t\tconst char *value;\n+\t\tint matched;\n+\t\tenum attr_match_mode match_mode;\n+\n+\t\tvalue = item->attr_check->items[i].value;\n+\t\tmatch_mode = item->attr_match[i].match_mode;\n+\n+\t\tif (ATTR_TRUE(value))\n+\t\t\tmatched = (match_mode == MATCH_SET);\n+\t\telse if (ATTR_FALSE(value))\n+\t\t\tmatched = (match_mode == MATCH_UNSET);\n+\t\telse if (ATTR_UNSET(value))\n+\t\t\tmatched = (match_mode == MATCH_UNSPECIFIED);\n+\t\telse\n+\t\t\tmatched = (match_mode == MATCH_VALUE &&\n+\t\t\t\t   !strcmp(item->attr_match[i].value, value));\n+\t\tif (!matched)\n+\t\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\ndiff --git a/pathspec.h b/pathspec.h\nindex 5fd781d695..1c18a2c90c 100644\n--- a/pathspec.h\n+++ b/pathspec.h\n@@ -111,5 +111,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec,\n \t\t\t\t\tchar *seen);\n char *find_pathspecs_matching_against_index(const struct pathspec *pathspec,\n \t\t\t\t\t    const struct index_state *istate);\n+int match_pathspec_attrs(const struct index_state *istate,\n+\t\t\t const char *name, int namelen,\n+\t\t\t const struct pathspec_item *item);\n \n #endif /* PATHSPEC_H */\n-- \n2.19.1.1327.g328c130451.dirty\n\n"},{"id":"363588","messageId":"20181118164800.32759-6-pclouds@gmail.com","threadId":"49848","inReplyTo":"20181118164800.32759-1-pclouds@gmail.com","subject":"[PATCH 5/5] tree-walk: support :(attr) matching","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-18T16:48:00Z","receivedAt":"2018-11-18T16:48:17Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This lets us use :(attr) with \"git grep <tree-ish>\" or \"git log\".\n\n:(attr) requires another round of checking before we can declare that\na path is matched. This is done after path matching since we have lots\nof optimization to take a shortcut when things don't match.\n\nNote that if :(attr) is present, we can't return\nall_entries_interesting / all_entries_not_interesting anymore because\nwe can't be certain about that. Not until match_pathspec_attrs() can\ntell us \"yes all these paths satisfy :(attr)\".\n\nSecond note. Even though we walk a specific tree, we use attributes\nfrom _worktree_ (or falling back to the index), not from .gitattributes\nfiles on that tree. This by itself is not necessarily wrong, but the\nuser just have to be aware of this.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Documentation/glossary-content.txt |  2 +\n t/t6135-pathspec-with-attrs.sh     | 58 +++++++++++++++++++++++++-\n tree-walk.c                        | 67 +++++++++++++++++++++++-------\n 3 files changed, 112 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\nindex 0d2aa48c63..023ca95e7c 100644\n--- a/Documentation/glossary-content.txt\n+++ b/Documentation/glossary-content.txt\n@@ -404,6 +404,8 @@ these forms:\n - \"`!ATTR`\" requires that the attribute `ATTR` be\n   unspecified.\n +\n+Note that when matching against a tree object, attributes are still\n+obtained from working tree, not from the given tree object.\n \n exclude;;\n \tAfter a path matches any non-exclude pathspec, it will be run\ndiff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh\nindex e436a73962..457cc167c7 100755\n--- a/t/t6135-pathspec-with-attrs.sh\n+++ b/t/t6135-pathspec-with-attrs.sh\n@@ -31,7 +31,7 @@ test_expect_success 'setup a tree' '\n \tmkdir sub &&\n \twhile read path\n \tdo\n-\t\t: >$path &&\n+\t\techo content >$path &&\n \t\tgit add $path || return 1\n \tdone <expect &&\n \tgit commit -m \"initial commit\" &&\n@@ -48,6 +48,10 @@ test_expect_success 'pathspec with labels and non existent .gitattributes' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'pathspec with labels and non existent .gitattributes (2)' '\n+\ttest_must_fail git grep content HEAD -- \":(attr:label)\"\n+'\n+\n test_expect_success 'setup .gitattributes' '\n \tcat <<-\\EOF >.gitattributes &&\n \tfileA labelA\n@@ -74,6 +78,15 @@ test_expect_success 'check specific set attr' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check specific set attr (2)' '\n+\tcat <<-\\EOF >expect &&\n+\tHEAD:fileSetLabel\n+\tHEAD:sub/fileSetLabel\n+\tEOF\n+\tgit grep -l content HEAD \":(attr:label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'check specific unset attr' '\n \tcat <<-\\EOF >expect &&\n \tfileUnsetLabel\n@@ -83,6 +96,15 @@ test_expect_success 'check specific unset attr' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check specific unset attr (2)' '\n+\tcat <<-\\EOF >expect &&\n+\tHEAD:fileUnsetLabel\n+\tHEAD:sub/fileUnsetLabel\n+\tEOF\n+\tgit grep -l content HEAD \":(attr:-label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'check specific value attr' '\n \tcat <<-\\EOF >expect &&\n \tfileValue\n@@ -94,6 +116,16 @@ test_expect_success 'check specific value attr' '\n \ttest_must_be_empty actual\n '\n \n+test_expect_success 'check specific value attr (2)' '\n+\tcat <<-\\EOF >expect &&\n+\tHEAD:fileValue\n+\tHEAD:sub/fileValue\n+\tEOF\n+\tgit grep -l content HEAD \":(attr:label=foo)\" >actual &&\n+\ttest_cmp expect actual &&\n+\ttest_must_fail git grep -l content HEAD \":(attr:label=bar)\"\n+'\n+\n test_expect_success 'check unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\n@@ -118,6 +150,30 @@ test_expect_success 'check unspecified attr' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'check unspecified attr (2)' '\n+\tcat <<-\\EOF >expect &&\n+\tHEAD:.gitattributes\n+\tHEAD:fileA\n+\tHEAD:fileAB\n+\tHEAD:fileAC\n+\tHEAD:fileB\n+\tHEAD:fileBC\n+\tHEAD:fileC\n+\tHEAD:fileNoLabel\n+\tHEAD:fileWrongLabel\n+\tHEAD:sub/fileA\n+\tHEAD:sub/fileAB\n+\tHEAD:sub/fileAC\n+\tHEAD:sub/fileB\n+\tHEAD:sub/fileBC\n+\tHEAD:sub/fileC\n+\tHEAD:sub/fileNoLabel\n+\tHEAD:sub/fileWrongLabel\n+\tEOF\n+\tgit grep -l ^ HEAD \":(attr:!label)\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'check multiple unspecified attr' '\n \tcat <<-\\EOF >expect &&\n \t.gitattributes\ndiff --git a/tree-walk.c b/tree-walk.c\nindex 517bcdecf9..08210a4109 100644\n--- a/tree-walk.c\n+++ b/tree-walk.c\n@@ -949,7 +949,8 @@ static enum interesting do_match(struct index_state *istate,\n \t\t       PATHSPEC_LITERAL |\n \t\t       PATHSPEC_GLOB |\n \t\t       PATHSPEC_ICASE |\n-\t\t       PATHSPEC_EXCLUDE);\n+\t\t       PATHSPEC_EXCLUDE |\n+\t\t       PATHSPEC_ATTR);\n \n \tif (!ps->nr) {\n \t\tif (!ps->recursive ||\n@@ -981,14 +982,20 @@ static enum interesting do_match(struct index_state *istate,\n \n \t\t\tif (!ps->recursive ||\n \t\t\t    !(ps->magic & PATHSPEC_MAXDEPTH) ||\n-\t\t\t    ps->max_depth == -1)\n-\t\t\t\treturn all_entries_interesting;\n-\n-\t\t\treturn within_depth(base_str + matchlen + 1,\n-\t\t\t\t\t    baselen - matchlen - 1,\n-\t\t\t\t\t    !!S_ISDIR(entry->mode),\n-\t\t\t\t\t    ps->max_depth) ?\n-\t\t\t\tentry_interesting : entry_not_interesting;\n+\t\t\t    ps->max_depth == -1) {\n+\t\t\t\tif (!item->attr_match_nr)\n+\t\t\t\t\treturn all_entries_interesting;\n+\t\t\t\telse\n+\t\t\t\t\tgoto interesting;\n+\t\t\t}\n+\n+\t\t\tif (within_depth(base_str + matchlen + 1,\n+\t\t\t\t\t baselen - matchlen - 1,\n+\t\t\t\t\t !!S_ISDIR(entry->mode),\n+\t\t\t\t\t ps->max_depth))\n+\t\t\t\tgoto interesting;\n+\t\t\telse\n+\t\t\t\treturn entry_not_interesting;\n \t\t}\n \n \t\t/* Either there must be no base, or the base must match. */\n@@ -996,12 +1003,12 @@ static enum interesting do_match(struct index_state *istate,\n \t\t\tif (match_entry(item, entry, pathlen,\n \t\t\t\t\tmatch + baselen, matchlen - baselen,\n \t\t\t\t\t&never_interesting))\n-\t\t\t\treturn entry_interesting;\n+\t\t\t\tgoto interesting;\n \n \t\t\tif (item->nowildcard_len < item->len) {\n \t\t\t\tif (!git_fnmatch(item, match + baselen, entry->path,\n \t\t\t\t\t\t item->nowildcard_len - baselen))\n-\t\t\t\t\treturn entry_interesting;\n+\t\t\t\t\tgoto interesting;\n \n \t\t\t\t/*\n \t\t\t\t * Match all directories. We'll try to\n@@ -1022,7 +1029,7 @@ static enum interesting do_match(struct index_state *istate,\n \t\t\t\t    !ps_strncmp(item, match + baselen,\n \t\t\t\t\t\tentry->path,\n \t\t\t\t\t\titem->nowildcard_len - baselen))\n-\t\t\t\t\treturn entry_interesting;\n+\t\t\t\t\tgoto interesting;\n \t\t\t}\n \n \t\t\tcontinue;\n@@ -1057,7 +1064,7 @@ static enum interesting do_match(struct index_state *istate,\n \t\tif (!git_fnmatch(item, match, base->buf + base_offset,\n \t\t\t\t item->nowildcard_len)) {\n \t\t\tstrbuf_setlen(base, base_offset + baselen);\n-\t\t\treturn entry_interesting;\n+\t\t\tgoto interesting;\n \t\t}\n \n \t\t/*\n@@ -1071,7 +1078,7 @@ static enum interesting do_match(struct index_state *istate,\n \t\t    !ps_strncmp(item, match, base->buf + base_offset,\n \t\t\t\titem->nowildcard_len)) {\n \t\t\tstrbuf_setlen(base, base_offset + baselen);\n-\t\t\treturn entry_interesting;\n+\t\t\tgoto interesting;\n \t\t}\n \n \t\tstrbuf_setlen(base, base_offset + baselen);\n@@ -1085,6 +1092,38 @@ static enum interesting do_match(struct index_state *istate,\n \t\t */\n \t\tif (ps->recursive && S_ISDIR(entry->mode))\n \t\t\treturn entry_interesting;\n+\t\tcontinue;\n+interesting:\n+\t\tif (item->attr_match_nr) {\n+\t\t\tint ret;\n+\n+\t\t\t/*\n+\t\t\t * Must not return all_entries_not_interesting\n+\t\t\t * prematurely. We do not know if all entries do not\n+\t\t\t * match some attributes with current attr API.\n+\t\t\t */\n+\t\t\tnever_interesting = entry_not_interesting;\n+\n+\t\t\t/*\n+\t\t\t * Consider all directories interesting (because some\n+\t\t\t * of those files inside may match some attributes\n+\t\t\t * even though the parent dir does not)\n+\t\t\t *\n+\t\t\t * FIXME: attributes _can_ match directories and we\n+\t\t\t * can probably return all_entries_interesting or\n+\t\t\t * all_entries_not_interesting here if matched.\n+\t\t\t */\n+\t\t\tif (S_ISDIR(entry->mode))\n+\t\t\t\treturn entry_interesting;\n+\n+\t\t\tstrbuf_add(base, entry->path, pathlen);\n+\t\t\tret = match_pathspec_attrs(istate, base->buf + base_offset,\n+\t\t\t\t\t\t   base->len - base_offset, item);\n+\t\t\tstrbuf_setlen(base, base_offset + baselen);\n+\t\t\tif (!ret)\n+\t\t\t\tcontinue;\n+\t\t}\n+\t\treturn entry_interesting;\n \t}\n \treturn never_interesting; /* No matches */\n }\n-- \n2.19.1.1327.g328c130451.dirty\n\n"},{"id":"363594","messageId":"87d0r217vr.fsf@evledraar.gmail.com","threadId":"49848","inReplyTo":"20181118164800.32759-1-pclouds@gmail.com","subject":"Re: [PATCH 0/5] Make :(attr) pathspec work with \"git log\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-18T19:51:52Z","receivedAt":"2018-11-18T19:51:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:\n\n> When :(attr) was added, it supported one of the two main pathspec\n> matching functions, the one that works on a list of paths. The other\n> one works on a tree, tree_entry_interesting(), which gets :(attr)\n> support in this series.\n>\n> With this, \"git grep <pattern> <tree> -- :(attr)\" or \"git log :(attr)\"\n> will not abort with BUG() anymore.\n>\n> But this also reveals an interesting thing: even though we walk on a\n> tree, we check attributes from _worktree_ (and optionally fall back to\n> the index). This is how attributes are implemented since forever. I\n> think this is not a big deal if we communicate clearly with the user.\n> But otherwise, this series can be scraped, as reading attributes from\n> a specific tree could be a lot of work.\n\nI'm very happy to see this implemented, and I think the behavior\ndescribed here is the right way to go. E.g. in git.git we have diff=perl\nentries in .gitattributes. It would suck if:\n\n    git log ':(attr:diff=perl)'\n\nWould only list commits as far as 20460635a8 (\".gitattributes: use the\n\"perl\" differ for Perl\", 2018-04-26), since that's when we stop having\nthat attribute. Ditto for wanting to run \"grep\" on e.g. perl files in\n2.12.0.\n\nI have also run into cases where I want to use a .gitattributes file\nfrom a specific commit. E.g. when writing pre-receive hooks where I've\nwanted the .gitattributes of the commit being pushed to configure\nsomething about it. But as you note this isn't supported at all.\n\nBut a concern is whether we should be making :(attr:*) behave like this\nfor now. Are we going to regret it later? I don't think so, I think\nwanting to use the current working tree's / index's is the most sane\ndefault, and if we get the ability to read it from revisions as we\ne.g. walk the log it would make most sense to just call that\n:(treeattr:*) or something like that.\n"},{"id":"363595","messageId":"87bm6m17kt.fsf@evledraar.gmail.com","threadId":"49848","inReplyTo":"20181118164800.32759-6-pclouds@gmail.com","subject":"Re: [PATCH 5/5] tree-walk: support :(attr) matching","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-18T19:58:26Z","receivedAt":"2018-11-18T19:58:31Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:\n\nAs noted in\nhttps://public-inbox.org/git/87d0r217vr.fsf@evledraar.gmail.com/ I'm\nhappy to see this implemented. I have not read this patch in much\ndetail...\n\n> [...]\n>  Documentation/glossary-content.txt |  2 +\n> [...]\n> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\n> index 0d2aa48c63..023ca95e7c 100644\n> --- a/Documentation/glossary-content.txt\n> +++ b/Documentation/glossary-content.txt\n> @@ -404,6 +404,8 @@ these forms:\n>  - \"`!ATTR`\" requires that the attribute `ATTR` be\n>    unspecified.\n>  +\n> +Note that when matching against a tree object, attributes are still\n> +obtained from working tree, not from the given tree object.\n>\n>  exclude;;\n>  \tAfter a path matches any non-exclude pathspec, it will be run\n\nJust a poke again about what I brought up in the thread you replied to\nin\nhttps://public-inbox.org/git/CACsJy8CLHQ0mKhKXvTDAqy9TLwEFBSvHEu5UbPxHX4is2mK+Cg@mail.gmail.com/\n\nI.e. the documentation of these various wildmatch() / attributes\npatterns we support is all over the place. Some in gitignore(5), some\nnot documented at all, and some in gitglossary(7) (which really should\nnot be serving as primary documentation for anything).\n"},{"id":"363606","messageId":"xmqqo9aleszv.fsf@gitster-ct.c.googlers.com","threadId":"49848","inReplyTo":"20181118164800.32759-1-pclouds@gmail.com","subject":"Re: [PATCH 0/5] Make :(attr) pathspec work with \"git log\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-11-19T01:49:40Z","receivedAt":"2018-11-19T01:49:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> But this also reveals an interesting thing: even though we walk on a\n> tree, we check attributes from _worktree_ (and optionally fall back to\n> the index). This is how attributes are implemented since forever. I\n> think this is not a big deal if we communicate clearly with the user.\n\nThis is pretty much deliberately designed to be so; the set of the\nattributes in HEAD may be stale but by taking the contents from the\nworking tree the user can work it around.\n\n> The main patch is the last one. The others are just to open a path to\n> pass \"struct index_state *\" down to tree_entry_interesting(). This may\n> become standard procedure because we don't want to stick the_index (or\n> the_repository) here and there.\n\nYup.\n\n"},{"id":"363625","messageId":"878t1p1fnr.fsf@evledraar.gmail.com","threadId":"49848","inReplyTo":"87d0r217vr.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/5] Make :(attr) pathspec work with \"git log\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-19T11:16:08Z","receivedAt":"2018-11-19T11:16:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 18 2018, Ævar Arnfjörð Bjarmason wrote:\n\n> On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:\n>\n>> When :(attr) was added, it supported one of the two main pathspec\n>> matching functions, the one that works on a list of paths. The other\n>> one works on a tree, tree_entry_interesting(), which gets :(attr)\n>> support in this series.\n>>\n>> With this, \"git grep <pattern> <tree> -- :(attr)\" or \"git log :(attr)\"\n>> will not abort with BUG() anymore.\n>>\n>> But this also reveals an interesting thing: even though we walk on a\n>> tree, we check attributes from _worktree_ (and optionally fall back to\n>> the index). This is how attributes are implemented since forever. I\n>> think this is not a big deal if we communicate clearly with the user.\n>> But otherwise, this series can be scraped, as reading attributes from\n>> a specific tree could be a lot of work.\n>\n> I'm very happy to see this implemented, and I think the behavior\n> described here is the right way to go. E.g. in git.git we have diff=perl\n> entries in .gitattributes. It would suck if:\n>\n>     git log ':(attr:diff=perl)'\n>\n> Would only list commits as far as 20460635a8 (\".gitattributes: use the\n> \"perl\" differ for Perl\", 2018-04-26), since that's when we stop having\n> that attribute. Ditto for wanting to run \"grep\" on e.g. perl files in\n> 2.12.0.\n>\n> I have also run into cases where I want to use a .gitattributes file\n> from a specific commit. E.g. when writing pre-receive hooks where I've\n> wanted the .gitattributes of the commit being pushed to configure\n> something about it. But as you note this isn't supported at all.\n>\n> But a concern is whether we should be making :(attr:*) behave like this\n> for now. Are we going to regret it later? I don't think so, I think\n> wanting to use the current working tree's / index's is the most sane\n> default, and if we get the ability to read it from revisions as we\n> e.g. walk the log it would make most sense to just call that\n> :(treeattr:*) or something like that.\n\nAs an aside, how do you do the inverse of matching for an attribute by\nvalue? I.e.:\n\n    $ git ls-files | wc -l; git ls-files ':(attr:diff=perl)' | wc -l\n    3522\n    65\n\nI'd like something gives me all files that don't match diff=perl,\ni.e. 3522-65 = 3457 files, or what I'd get if I constructed such a match\nmanually with excludes:\n\n    $ git ls-files $(grep diff=perl .gitattributes | cut -d ' ' -f1 | sed 's!^!:(exclude)!') | wc -l\n    3457\n\nFrom my reading of parse_pathspec_attr_match() and match_attrs() this\nisn't possible and I'd need to support ':(attr:diff!=perl)' via a new\nMATCH_NOT_VALUE mode. But I wanted to make sure I wasn't missing some\nsubtlety, i.e. that this was implemented already via some other feature.\n\nI thought I could do:\n\n    git ls-files ':(exclude):(attr:diff=perl)'\n\nBut we don't support chaining like that, and this would only exclude a\nfile that's actually called \":(attr:diff=perl)\". I.e. created via\nsomething like \"touch ':(attr:diff=perl)'\".\n"},{"id":"363627","messageId":"877eh91egn.fsf@evledraar.gmail.com","threadId":"49848","inReplyTo":"20181118164800.32759-1-pclouds@gmail.com","subject":"Re: [PATCH 0/5] Make :(attr) pathspec work with \"git log\"","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2018-11-19T11:42:00Z","receivedAt":"2018-11-19T11:42:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:\n\n> When :(attr) was added, it supported one of the two main pathspec\n> matching functions, the one that works on a list of paths. The other\n> one works on a tree, tree_entry_interesting(), which gets :(attr)\n> support in this series.\n>\n> With this, \"git grep <pattern> <tree> -- :(attr)\" or \"git log :(attr)\"\n> will not abort with BUG() anymore.\n>\n> But this also reveals an interesting thing: even though we walk on a\n> tree, we check attributes from _worktree_ (and optionally fall back to\n> the index). This is how attributes are implemented since forever. I\n> think this is not a big deal if we communicate clearly with the user.\n> But otherwise, this series can be scraped, as reading attributes from\n> a specific tree could be a lot of work.\n>\n> The main patch is the last one. The others are just to open a path to\n> pass \"struct index_state *\" down to tree_entry_interesting(). This may\n> become standard procedure because we don't want to stick the_index (or\n> the_repository) here and there.\n\nAnother side-note (this thread is turning into my personal blog at this\npoint...) I found an old related thread:\nhttps://public-inbox.org/git/20170509225219.GB106700@google.com/\n\nSo this series fixes 1/2 of the issues noted there, but git-ls-tree will\nstill die with the same error.\n"},{"id":"363629","messageId":"20181119120927.GA31660@sigill.intra.peff.net","threadId":"49848","inReplyTo":"87d0r217vr.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/5] Make :(attr) pathspec work with \"git log\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-11-19T12:09:27Z","receivedAt":"2018-11-19T12:09:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Nov 18, 2018 at 08:51:52PM +0100, Ævar Arnfjörð Bjarmason wrote:\n\n> > But this also reveals an interesting thing: even though we walk on a\n> > tree, we check attributes from _worktree_ (and optionally fall back to\n> > the index). This is how attributes are implemented since forever. I\n> > think this is not a big deal if we communicate clearly with the user.\n> > But otherwise, this series can be scraped, as reading attributes from\n> > a specific tree could be a lot of work.\n> \n> I'm very happy to see this implemented, and I think the behavior\n> described here is the right way to go. E.g. in git.git we have diff=perl\n> entries in .gitattributes. It would suck if:\n> \n>     git log ':(attr:diff=perl)'\n> \n> Would only list commits as far as 20460635a8 (\".gitattributes: use the\n> \"perl\" differ for Perl\", 2018-04-26), since that's when we stop having\n> that attribute. Ditto for wanting to run \"grep\" on e.g. perl files in\n> 2.12.0.\n> \n> I have also run into cases where I want to use a .gitattributes file\n> from a specific commit. E.g. when writing pre-receive hooks where I've\n> wanted the .gitattributes of the commit being pushed to configure\n> something about it. But as you note this isn't supported at all.\n> \n> But a concern is whether we should be making :(attr:*) behave like this\n> for now. Are we going to regret it later? I don't think so, I think\n> wanting to use the current working tree's / index's is the most sane\n> default, and if we get the ability to read it from revisions as we\n> e.g. walk the log it would make most sense to just call that\n> :(treeattr:*) or something like that.\n\nI think that ship already sailed with the fact that \"git log -p\" will\nshow diffs using the worktree attrs. I agree that it would sometimes be\nnice to specify attributes from a particular tree, but at this point the\ndefault probably needs to remain as it is.\n\n-Peff\n"},{"id":"363640","messageId":"CACsJy8Aq8kjtwv8ZtPOUHOp=59KL9dRiTQkFfbr5oPh9Swytwg@mail.gmail.com","threadId":"49848","inReplyTo":"878t1p1fnr.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/5] Make :(attr) pathspec work with \"git log\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-19T15:25:22Z","receivedAt":"2018-11-19T15:25:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Nov 19, 2018 at 12:16 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> As an aside, how do you do the inverse of matching for an attribute by\n> value? I.e.:\n>\n>     $ git ls-files | wc -l; git ls-files ':(attr:diff=perl)' | wc -l\n>     3522\n>     65\n>\n> I'd like something gives me all files that don't match diff=perl,\n> i.e. 3522-65 = 3457 files, or what I'd get if I constructed such a match\n> manually with excludes:\n>\n>     $ git ls-files $(grep diff=perl .gitattributes | cut -d ' ' -f1 | sed 's!^!:(exclude)!') | wc -l\n>     3457\n>\n> From my reading of parse_pathspec_attr_match() and match_attrs() this\n> isn't possible and I'd need to support ':(attr:diff!=perl)' via a new\n> MATCH_NOT_VALUE mode. But I wanted to make sure I wasn't missing some\n> subtlety, i.e. that this was implemented already via some other feature.\n>\n> I thought I could do:\n>\n>     git ls-files ':(exclude):(attr:diff=perl)'\n>\n> But we don't support chaining like that, and this would only exclude a\n> file that's actually called \":(attr:diff=perl)\". I.e. created via\n> something like \"touch ':(attr:diff=perl)'\".\n\nI think we allow :(exclude,attr:diff=perl) which should \"exclude all\npaths that have diff=perl attribute\". It's actually tested in t6135\nfor ls-files (but I didn't add the same test for 'git grep' because I\nwas so confident it would work; I'll work on that).\n-- \nDuy\n"},{"id":"363641","messageId":"CACsJy8B=-7Z6qP6UwoKnQSugxDi8o1t2aKXHWzf_ST0Qo5kezA@mail.gmail.com","threadId":"49848","inReplyTo":"877eh91egn.fsf@evledraar.gmail.com","subject":"Re: [PATCH 0/5] Make :(attr) pathspec work with \"git log\"","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-19T15:31:17Z","receivedAt":"2018-11-19T15:31:45Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Nov 19, 2018 at 12:42 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:\n>\n> > When :(attr) was added, it supported one of the two main pathspec\n> > matching functions, the one that works on a list of paths. The other\n> > one works on a tree, tree_entry_interesting(), which gets :(attr)\n> > support in this series.\n> >\n> > With this, \"git grep <pattern> <tree> -- :(attr)\" or \"git log :(attr)\"\n> > will not abort with BUG() anymore.\n> >\n> > But this also reveals an interesting thing: even though we walk on a\n> > tree, we check attributes from _worktree_ (and optionally fall back to\n> > the index). This is how attributes are implemented since forever. I\n> > think this is not a big deal if we communicate clearly with the user.\n> > But otherwise, this series can be scraped, as reading attributes from\n> > a specific tree could be a lot of work.\n> >\n> > The main patch is the last one. The others are just to open a path to\n> > pass \"struct index_state *\" down to tree_entry_interesting(). This may\n> > become standard procedure because we don't want to stick the_index (or\n> > the_repository) here and there.\n>\n> Another side-note (this thread is turning into my personal blog at this\n> point...) I found an old related thread:\n> https://public-inbox.org/git/20170509225219.GB106700@google.com/\n>\n> So this series fixes 1/2 of the issues noted there, but git-ls-tree will\n> still die with the same error.\n\nIf you mean BUG(), not it does not. There are just a couple more\nplaces besides tree_entry_interesting() and match_pathspec() that need\nto be aware of all the magic things. ls-tree is one of those but it's\nwell guarded and will display this instead\n\n$ git ls-tree HEAD ':(attr:abc=def)'\nfatal: :(attr:abc=def): pathspec magic not supported by this command: 'attr'\n\nIf you mean making ls-tree support :(attr) and other stuff, it's not\ngoing to happen in near future. It may be nice to switch to\ntree_entry_interesting() in this command, but if it was easy to do I\nwould have done it years ago :P\n-- \nDuy\n"},{"id":"363642","messageId":"CACsJy8BNs=0qyaHnce7-ey3vd8bMh=ObBpCdF=Z+6NT2C_fuAw@mail.gmail.com","threadId":"49848","inReplyTo":"87bm6m17kt.fsf@evledraar.gmail.com","subject":"Re: [PATCH 5/5] tree-walk: support :(attr) matching","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2018-11-19T15:33:25Z","receivedAt":"2018-11-19T15:33:54Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Nov 18, 2018 at 8:58 PM Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n>\n>\n> On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:\n>\n> As noted in\n> https://public-inbox.org/git/87d0r217vr.fsf@evledraar.gmail.com/ I'm\n> happy to see this implemented. I have not read this patch in much\n> detail...\n>\n> > [...]\n> >  Documentation/glossary-content.txt |  2 +\n> > [...]\n> > diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt\n> > index 0d2aa48c63..023ca95e7c 100644\n> > --- a/Documentation/glossary-content.txt\n> > +++ b/Documentation/glossary-content.txt\n> > @@ -404,6 +404,8 @@ these forms:\n> >  - \"`!ATTR`\" requires that the attribute `ATTR` be\n> >    unspecified.\n> >  +\n> > +Note that when matching against a tree object, attributes are still\n> > +obtained from working tree, not from the given tree object.\n> >\n> >  exclude;;\n> >       After a path matches any non-exclude pathspec, it will be run\n>\n> Just a poke again about what I brought up in the thread you replied to\n> in\n> https://public-inbox.org/git/CACsJy8CLHQ0mKhKXvTDAqy9TLwEFBSvHEu5UbPxHX4is2mK+Cg@mail.gmail.com/\n>\n> I.e. the documentation of these various wildmatch() / attributes\n> patterns we support is all over the place. Some in gitignore(5), some\n> not documented at all, and some in gitglossary(7) (which really should\n> not be serving as primary documentation for anything).\n\nPoking me is not going help, I'm afraid. Except bugs, which have\nhighest priority to me, I do other stuff first either because I need\nit or it's fun to do, and this wildmatch documentation is neither (and\nI have a long backlog).\n-- \nDuy\n"}]}