Make :(attr) pathspec work with "git log"
cover 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 <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.
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(-)
Signature · 1 line
2.19.1.1327.g328c130451.dirty
PATCH 1/5 tree.c: make read_tree*() take 'struct repository *'
These functions call tree_entry_interesting() which will soon require a 'struct index_state *' to be passed in. Instead of just changing the function signature to take an index, update to take a repo instead because these functions do need object database access.
--- archive.c | 6 ++++-- builtin/checkout.c | 3 ++- builtin/log.c | 5 +++-- builtin/ls-files.c | 2 +- builtin/ls-tree.c | 3 ++- merge-recursive.c | 3 ++- tree.c | 18 ++++++++++-------- tree.h | 18 +++++++++++------- 8 files changed, 35 insertions(+), 23 deletions(-)
archive.c +4 -2
@@ -285,7 +285,8 @@ int write_archive_entries(struct archiver_args *args,git_attr_set_direction (GIT_ATTR_INDEX ); } - err =read_tree_recursive (args->tree,"" ,0 ,0 , &args->pathspec, + err =read_tree_recursive (args->repo, args->tree,"" , +0 ,0 , &args->pathspec, queue_or_write_archive_entry, &context);if (err ==READ_TREE_RECURSIVE )
@@ -346,7 +347,8 @@ static int path_exists(struct archiver_args *args, const char *path) ctx.args = args;parse_pathspec (&ctx.pathspec,0 ,0 ,"" , paths); ctx.pathspec.recursive =1 ; - ret =read_tree_recursive (args->tree,"" ,0 ,0 , &ctx.pathspec, + ret =read_tree_recursive (args->repo, args->tree,"" , +0 ,0 , &ctx.pathspec, reject_entry, &ctx);clear_pathspec (&ctx.pathspec);return ret !=0 ;
builtin/checkout.c +2 -1
@@ -115,7 +115,8 @@ static int update_some(const struct object_id *oid, struct strbuf *base,static int read_tree_some (struct tree *tree,const struct pathspec *pathspec) { -read_tree_recursive (tree,"" ,0 ,0 , pathspec, update_some,NULL ); +read_tree_recursive (the_repository, tree,"" ,0 ,0 , + pathspec, update_some,NULL );/* update the index with the given tree's info * for all args, expanding wildcards, and exit
builtin/log.c +3 -2
@@ -639,8 +639,9 @@ int cmd_show(int argc, const char **argv, const char *prefix)diff_get_color_opt (&rev.diffopt,DIFF_COMMIT ), name, diff_get_color_opt(&rev.diffopt, DIFF_RESET)); -read_tree_recursive ((struct tree *)o,"" ,0 ,0 , &match_all, - show_tree_object, rev.diffopt.file); +read_tree_recursive (the_repository, (struct tree *)o,"" , +0 ,0 , &match_all, show_tree_object, + rev.diffopt.file); rev.shown_one =1 ;break ;case OBJ_COMMIT :
builtin/ls-files.c +1 -1
@@ -441,7 +441,7 @@ void overlay_tree_on_index(struct index_state *istate, PATHSPEC_PREFER_CWD, prefix, matchbuf); }else memset (&pathspec,0 ,sizeof (pathspec)); -if (read_tree (tree,1 , &pathspec, istate)) +if (read_tree (the_repository, tree,1 , &pathspec, istate))die ("unable to read tree entries %s" , tree_name);for (i =0 ; i < istate->cache_nr; i++) {
builtin/ls-tree.c +2 -1
@@ -185,5 +185,6 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix) tree =parse_tree_indirect (&oid);if (!tree)die ("not a tree object" ); -return !!read_tree_recursive (tree,"" ,0 ,0 , &pathspec, show_tree,NULL ); +return !!read_tree_recursive (the_repository, tree,"" ,0 ,0 , + &pathspec, show_tree,NULL ); }
merge-recursive.c +2 -1
@@ -463,7 +463,8 @@ static void get_files_dirs(struct merge_options *o, struct tree *tree) {struct pathspec match_all;memset (&match_all,0 ,sizeof (match_all)); -read_tree_recursive (tree,"" ,0 ,0 , &match_all, save_files_dirs, o); +read_tree_recursive (the_repository, tree,"" ,0 ,0 , + &match_all, save_files_dirs, o); } static int get_tree_entry_if_blob(const struct object_id *tree,
tree.c +10 -8
@@ -60,7 +60,8 @@ static int read_one_entry_quick(const struct object_id *oid, struct strbuf *base ADD_CACHE_JUST_APPEND); } -static int read_tree_1 (struct tree *tree,struct strbuf *base, +static int read_tree_1 (struct repository *r, +struct tree *tree,struct strbuf *base,int stage,const struct pathspec *pathspec,read_tree_fn_t fn,void *context) {
@@ -99,7 +100,7 @@ static int read_tree_1(struct tree *tree, struct strbuf *base,else if (S_ISGITLINK (entry.mode)) {struct commit *commit; - commit =lookup_commit (the_repository, entry.oid); + commit =lookup_commit (r, entry.oid);if (!commit) die("Commit %s in submodule path %s%s not found", oid_to_hex(entry.oid),
@@ -118,7 +119,7 @@ static int read_tree_1(struct tree *tree, struct strbuf *base, len =tree_entry_len (&entry);strbuf_add (base, entry.path, len);strbuf_addch (base,'/' ); - retval =read_tree_1 (lookup_tree (the_repository, &oid), + retval =read_tree_1 (r,lookup_tree (r, &oid), base, stage, pathspec, fn, context);strbuf_setlen (base, oldlen);
@@ -128,7 +129,8 @@ static int read_tree_1(struct tree *tree, struct strbuf *base,return 0 ; } -int read_tree_recursive (struct tree *tree, +int read_tree_recursive (struct repository *r, +struct tree *tree,const char *base,int baselen,int stage,const struct pathspec *pathspec,read_tree_fn_t fn,void *context)
@@ -137,7 +139,7 @@ int read_tree_recursive(struct tree *tree,int ret;strbuf_add (&sb, base, baselen); - ret =read_tree_1 (tree, &sb, stage, pathspec, fn, context); + ret =read_tree_1 (r, tree, &sb, stage, pathspec, fn, context);strbuf_release (&sb);return ret; }
@@ -152,8 +154,8 @@ static int cmp_cache_name_compare(const void *a_, const void *b_) ce2->name, ce2->ce_namelen, ce_stage(ce2)); } -int read_tree (struct tree *tree,int stage,struct pathspec *match, -struct index_state *istate) +int read_tree (struct repository *r,struct tree *tree,int stage, +struct pathspec *match,struct index_state *istate) {read_tree_fn_t fn =NULL ;int i, err;
@@ -181,7 +183,7 @@ int read_tree(struct tree *tree, int stage, struct pathspec *match,if (!fn) fn = read_one_entry_quick; - err =read_tree_recursive (tree,"" ,0 , stage, match, fn, istate); + err =read_tree_recursive (r, tree,"" ,0 , stage, match, fn, istate);if (fn == read_one_entry || err)return err;
tree.h +11 -7
@@ -3,7 +3,7 @@#include "object.h" -extern const char *tree_type; +struct repository ;struct strbuf ;struct tree {
@@ -12,6 +12,8 @@ struct tree {unsigned long size; }; +extern const char *tree_type; +struct tree *lookup_tree (struct repository *r,const struct object_id *oid);int parse_tree_buffer (struct tree *item,void *buffer,unsigned long size);
@@ -29,12 +31,14 @@ struct tree *parse_tree_indirect(const struct object_id *oid);#define READ_TREE_RECURSIVE 1typedef int (*read_tree_fn_t )(const struct object_id *,struct strbuf *,const char *,unsigned int ,int ,void *); -extern int read_tree_recursive (struct tree *tree, -const char *base,int baselen, -int stage,const struct pathspec *pathspec, -read_tree_fn_t fn,void *context); +int read_tree_recursive (struct repository *r, +struct tree *tree, +const char *base,int baselen, +int stage,const struct pathspec *pathspec, +read_tree_fn_t fn,void *context); -extern int read_tree (struct tree *tree,int stage,struct pathspec *pathspec, -struct index_state *istate); +int read_tree (struct repository *r,struct tree *tree, +int stage,struct pathspec *pathspec, +struct index_state *istate);#endif /* TREE_H */
Signature · 1 line
2.19.1.1327.g328c130451.dirty
PATCH 2/5 tree-walk.c: make tree_entry_interesting() take an index
In reply to Nguyễn Thái Ngọc DuyIn order to support :(attr) when matching pathspec on a tree, tree_entry_interesting() needs to take an index (because git_check_attr() needs it). This is the preparation step for it. This also makes it clearer what index we fall back to when looking up attributes during an unpack-trees operation: the source index.
This also fixes revs->pruning.repo initialization that should have been done in 2abf350385 (revision.c: remove implicit dependency on the_index - 2018-09-21). Without it, skip_uninteresting() will dereference a NULL pointer through this call chain
get_revision(revs)
get_revision_internal
get_revision_1
try_to_simplify_commit
rev_compare_tree
diff_tree_oid(..., &revs->pruning)
ll_diff_tree_oid
diff_tree_paths
ll_diff_tree
skip_uninteresting
--- builtin/grep.c | 3 ++- builtin/merge-tree.c | 2 +- list-objects.c | 3 ++- revision.c | 1 + tree-diff.c | 3 ++- tree-walk.c | 22 ++++++++++++++-------- tree-walk.h | 10 ++++++---- tree.c | 3 ++- unpack-trees.c | 6 +++--- 9 files changed, 33 insertions(+), 20 deletions(-)
builtin/grep.c +2 -1
@@ -568,7 +568,8 @@ static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,if (match != all_entries_interesting) {strbuf_addstr (&name, base->buf + tn_len); - match =tree_entry_interesting (&entry, &name, + match =tree_entry_interesting (repo->index, + &entry, &name,0 , pathspec);strbuf_setlen (&name, name_base_len);
builtin/merge-tree.c +1 -1
@@ -346,7 +346,7 @@ static void merge_trees(struct tree_desc t[3], const char *base)setup_traverse_info (&info, base); info.fn = threeway_callback; -traverse_trees (3 , t, &info); +traverse_trees (&the_index,3 , t, &info); }static void *get_tree_descriptor (struct tree_desc *desc,const char *rev)
list-objects.c +2 -1
@@ -113,7 +113,8 @@ static void process_tree_contents(struct traversal_context *ctx,while (tree_entry (&desc, &entry)) {if (match != all_entries_interesting) { - match =tree_entry_interesting (&entry, base,0 , + match =tree_entry_interesting (ctx->revs->repo->index, + &entry, base,0 , &ctx->revs->diffopt.pathspec);if (match == all_entries_not_interesting)break ;
revision.c +1 -0
@@ -1459,6 +1459,7 @@ void repo_init_revisions(struct repository *r, revs->abbrev =DEFAULT_ABBREV ; revs->ignore_merges =1 ; revs->simplify_history =1 ; + revs->pruning.repo = r; revs->pruning.flags.recursive =1 ; revs->pruning.flags.quick =1 ; revs->pruning.add_remove = file_add_remove;
tree-diff.c +2 -1
@@ -299,7 +299,8 @@ static void skip_uninteresting(struct tree_desc *t, struct strbuf *base,enum interesting match;while (t->size) { - match =tree_entry_interesting (&t->entry, base,0 , &opt->pathspec); + match =tree_entry_interesting (opt->repo->index, &t->entry, + base,0 , &opt->pathspec);if (match) {if (match == all_entries_not_interesting) t->size =0 ;
tree-walk.c +14 -8
@@ -365,7 +365,8 @@ static void free_extended_entry(struct tree_desc_x *t) } } -static inline int prune_traversal (struct name_entry *e, +static inline int prune_traversal (struct index_state *istate, +struct name_entry *e,struct traverse_info *info,struct strbuf *base,int still_interesting)
@@ -374,10 +375,13 @@ static inline int prune_traversal(struct name_entry *e,return 2 ;if (still_interesting <0 )return still_interesting; -return tree_entry_interesting (e, base,0 , info->pathspec); +return tree_entry_interesting (istate, e, base, +0 , info->pathspec); } -int traverse_trees (int n,struct tree_desc *t,struct traverse_info *info) +int traverse_trees (struct index_state *istate, +int n,struct tree_desc *t, +struct traverse_info *info) {int error =0 ;struct name_entry *entry =xmalloc (n*sizeof (*entry));
@@ -461,7 +465,7 @@ int traverse_trees(int n, struct tree_desc *t, struct traverse_info *info) }if (!mask)break ; - interesting =prune_traversal (e, info, &base, interesting); + interesting =prune_traversal (istate, e, info, &base, interesting);if (interesting <0 )break ;if (interesting) {
@@ -928,7 +932,8 @@ static int match_wildcard_base(const struct pathspec_item *item,* Pre-condition: either baselen == base_offset (i.e. empty path) * or base[baselen-1] == '/' (i.e. with trailing slash). */ -static enum interesting do_match (const struct name_entry *entry, +static enum interesting do_match (struct index_state *istate, +const struct name_entry *entry,struct strbuf *base,int base_offset,const struct pathspec *ps,int exclude)
@@ -1090,12 +1095,13 @@ static enum interesting do_match(const struct name_entry *entry,* Pre-condition: either baselen == base_offset (i.e. empty path) * or base[baselen-1] == '/' (i.e. with trailing slash). */ -enum interesting tree_entry_interesting (const struct name_entry *entry, +enum interesting tree_entry_interesting (struct index_state *istate, +const struct name_entry *entry,struct strbuf *base,int base_offset,const struct pathspec *ps) {enum interesting positive, negative; - positive =do_match (entry, base, base_offset, ps,0 ); + positive =do_match (istate, entry, base, base_offset, ps,0 );/* * case | entry | positive | negative | result
@@ -1132,7 +1138,7 @@ enum interesting tree_entry_interesting(const struct name_entry *entry, positive <= entry_not_interesting) /* #1, #2, #11, #12 */return positive; - negative =do_match (entry, base, base_offset, ps,1 ); + negative =do_match (istate, entry, base, base_offset, ps,1 );/* #8, #18 */ if (positive == all_entries_interesting &&
tree-walk.h +6 -4
@@ -1,6 +1,7 @@#ifndef TREE_WALK_H #define TREE_WALK_H +struct index_state ;struct strbuf ; struct name_entry {
@@ -48,7 +49,7 @@ void *fill_tree_descriptor(struct tree_desc *desc, const struct object_id *oid);struct traverse_info ;typedef int (*traverse_callback_t )(int n,unsigned long mask,unsigned long dirmask,struct name_entry *entry,struct traverse_info *); -int traverse_trees (int n,struct tree_desc *t,struct traverse_info *info); +int traverse_trees (struct index_state *istate,int n,struct tree_desc *t,struct traverse_info *info);enum follow_symlinks_result {FOUND =0 ,/* This includes out-of-tree links */
@@ -98,8 +99,9 @@ enum interesting { all_entries_interesting =2 /* yes, and all subsequent entries will be */ }; -extern enum interesting tree_entry_interesting (const struct name_entry *, -struct strbuf *,int , -const struct pathspec *ps); +enum interesting tree_entry_interesting (struct index_state *istate, +const struct name_entry *, +struct strbuf *,int , +const struct pathspec *ps);#endif
tree.c +2 -1
@@ -78,7 +78,8 @@ static int read_tree_1(struct repository *r,while (tree_entry (&desc, &entry)) {if (retval != all_entries_interesting) { - retval =tree_entry_interesting (&entry, base,0 , pathspec); + retval =tree_entry_interesting (r->index, &entry, + base,0 , pathspec);if (retval == all_entries_not_interesting)break ;if (retval == entry_not_interesting)
unpack-trees.c +3 -3
@@ -794,6 +794,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask,struct name_entry *names, struct traverse_info *info) { + struct unpack_trees_options *o = info->data;int i, ret, bottom;int nr_buf =0 ;struct tree_desc t[MAX_UNPACK_TREES ];
@@ -804,7 +805,6 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, nr_entries =all_trees_same_as_cache_tree (n, dirmask, names, info);if (nr_entries >0 ) { -struct unpack_trees_options *o = info->data;int pos =index_pos_by_traverse_info (names, info);if (!o->merge || df_conflicts)
@@ -863,7 +863,7 @@ static int traverse_trees_recursive(int n, unsigned long dirmask, } bottom =switch_cache_bottom (&newinfo); - ret =traverse_trees (n, t, &newinfo); + ret =traverse_trees (o->src_index, n, t, &newinfo);restore_cache_bottom (&newinfo, bottom);for (i =0 ; i < nr_buf; i++)
@@ -1550,7 +1550,7 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options }trace_performance_enter (); - ret =traverse_trees (len, t, &info); + ret =traverse_trees (o->src_index, len, t, &info);trace_performance_leave ("traverse_trees" );if (ret <0 ) goto return_failed;
Signature · 1 line
2.19.1.1327.g328c130451.dirty
PATCH 3/5 pathspec.h: clean up "extern" in function declarations
In reply to Nguyễn Thái Ngọc Duy"extern" on functions is not required and the trend has been removing it from header files.
--- pathspec.h | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-)
pathspec.h +12 -12
@@ -80,13 +80,13 @@ struct pathspec {* Any arguments used are copied. It is safe for the caller to modify * or free 'prefix' and 'args' after calling this function. */ -extern void parse_pathspec (struct pathspec *pathspec, -unsigned magic_mask, -unsigned flags, -const char *prefix, -const char **args); -extern void copy_pathspec (struct pathspec *dst,const struct pathspec *src); -extern void clear_pathspec (struct pathspec *); +void parse_pathspec (struct pathspec *pathspec, +unsigned magic_mask, +unsigned flags, +const char *prefix, +const char **args); +void copy_pathspec (struct pathspec *dst,const struct pathspec *src); +void clear_pathspec (struct pathspec *);static inline int ps_strncmp (const struct pathspec_item *item,const char *s1,const char *s2,size_t n)
@@ -106,10 +106,10 @@ static inline int ps_strcmp(const struct pathspec_item *item, return strcmp(s1, s2); } -extern void add_pathspec_matches_against_index (const struct pathspec *pathspec, -const struct index_state *istate, -char *seen); -extern char *find_pathspecs_matching_against_index (const struct pathspec *pathspec, -const struct index_state *istate); +void add_pathspec_matches_against_index (const struct pathspec *pathspec, +const struct index_state *istate, +char *seen); +char *find_pathspecs_matching_against_index (const struct pathspec *pathspec, +const struct index_state *istate);#endif /* PATHSPEC_H */
Signature · 1 line
2.19.1.1327.g328c130451.dirty
PATCH 4/5 dir.c: move, rename and export match_attrs()
In reply to Nguyễn Thái Ngọc DuyThe function will be reused for matching attributes in pathspec when walking trees (currently it's used for matching pathspec when walking a list). pathspec.c would be a more neutral place for this.
--- dir.c | 41 ++--------------------------------------- pathspec.c | 38 ++++++++++++++++++++++++++++++++++++++ pathspec.h | 3 +++ 3 files changed, 43 insertions(+), 39 deletions(-)
dir.c +2 -39
@@ -276,44 +276,6 @@ static int do_read_blob(const struct object_id *oid, struct oid_stat *oid_stat,#define DO_MATCH_DIRECTORY (1<<1)#define DO_MATCH_SUBMODULE (1<<2) -static int match_attrs (const struct index_state *istate, -const char *name,int namelen, -const struct pathspec_item *item) -{ -int i; -char *to_free =NULL ; - -if (name[namelen]) - name = to_free =xmemdupz (name, namelen); - -git_check_attr (istate, name, item->attr_check); - -free (to_free); - -for (i =0 ; i < item->attr_match_nr; i++) { -const char *value; -int matched; -enum attr_match_mode match_mode; - - value = item->attr_check->items[i].value; - match_mode = item->attr_match[i].match_mode; - -if (ATTR_TRUE (value)) - matched = (match_mode ==MATCH_SET ); -else if (ATTR_FALSE (value)) - matched = (match_mode ==MATCH_UNSET ); -else if (ATTR_UNSET (value)) - matched = (match_mode ==MATCH_UNSPECIFIED ); -else - matched = (match_mode ==MATCH_VALUE && - !strcmp (item->attr_match[i].value, value)); -if (!matched) -return 0 ; - } - -return 1 ; -} -/* * Does 'match' match the given name? * A match is found if
@@ -367,7 +329,8 @@ static int match_pathspec_item(const struct index_state *istate, strncmp(item->match, name - prefix, item->prefix))return 0 ; -if (item->attr_match_nr && !match_attrs (istate, name, namelen, item)) +if (item->attr_match_nr && + !match_pathspec_attrs (istate, name, namelen, item))return 0 ;/* If the match was just the prefix, we matched */
pathspec.c +38 -0
@@ -659,3 +659,41 @@ void clear_pathspec(struct pathspec *pathspec)FREE_AND_NULL (pathspec->items); pathspec->nr =0 ; } + +int match_pathspec_attrs (const struct index_state *istate, +const char *name,int namelen, +const struct pathspec_item *item) +{ +int i; +char *to_free =NULL ; + +if (name[namelen]) + name = to_free =xmemdupz (name, namelen); + +git_check_attr (istate, name, item->attr_check); + +free (to_free); + +for (i =0 ; i < item->attr_match_nr; i++) { +const char *value; +int matched; +enum attr_match_mode match_mode; + + value = item->attr_check->items[i].value; + match_mode = item->attr_match[i].match_mode; + +if (ATTR_TRUE (value)) + matched = (match_mode ==MATCH_SET ); +else if (ATTR_FALSE (value)) + matched = (match_mode ==MATCH_UNSET ); +else if (ATTR_UNSET (value)) + matched = (match_mode ==MATCH_UNSPECIFIED ); +else + matched = (match_mode ==MATCH_VALUE && + !strcmp (item->attr_match[i].value, value)); +if (!matched) +return 0 ; + } + +return 1 ; +}
pathspec.h +3 -0
@@ -111,5 +111,8 @@ void add_pathspec_matches_against_index(const struct pathspec *pathspec, char *seen);char *find_pathspecs_matching_against_index (const struct pathspec *pathspec,const struct index_state *istate); +int match_pathspec_attrs (const struct index_state *istate, +const char *name,int namelen, +const struct pathspec_item *item);#endif /* PATHSPEC_H */
Signature · 1 line
2.19.1.1327.g328c130451.dirty
PATCH 5/5 tree-walk: support :(attr) matching
In reply to Nguyễn Thái Ngọc DuyThis lets us use :(attr) with "git grep <tree-ish>" or "git log".
:(attr) requires another round of checking before we can declare that a path is matched. This is done after path matching since we have lots of optimization to take a shortcut when things don't match.
Note that if :(attr) is present, we can't return all_entries_interesting / all_entries_not_interesting anymore because we can't be certain about that. Not until match_pathspec_attrs() can tell us "yes all these paths satisfy :(attr)".
Second note. Even though we walk a specific tree, we use attributes from _worktree_ (or falling back to the index), not from .gitattributes files on that tree. This by itself is not necessarily wrong, but the user just have to be aware of this.
--- Documentation/glossary-content.txt | 2 + t/t6135-pathspec-with-attrs.sh | 58 +++++++++++++++++++++++++- tree-walk.c | 67 +++++++++++++++++++++++------- 3 files changed, 112 insertions(+), 15 deletions(-)
Documentation/glossary-content.txt +2 -0
@@ -404,6 +404,8 @@ these forms: - "`!ATTR`" requires that the attribute `ATTR` be unspecified. + +Note that when matching against a tree object, attributes are still +obtained from working tree, not from the given tree object. exclude;; After a path matches any non-exclude pathspec, it will be run
t/t6135-pathspec-with-attrs.sh +57 -1
94 lines not on this page · raw message
tree-walk.c +53 -14
113 lines not on this page · raw message
Signature · 1 line
2.19.1.1327.g328c130451.dirty
Re: [PATCH 5/5] tree-walk: support :(attr) matching
On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:
As noted in
https://public-inbox.org/git/[email protected]/ I'm happy to see this implemented. I have not read this patch in much detail...
Quoted 8 lines
[...]
Documentation/glossary-content.txt | 2 +[...]
Documentation/glossary-content.txt +0
-0
Documentation/glossary-content.txt these forms: · line 411
Quoted 8 lines
Documentation/glossary-content.txt +2
-0@@ -404,6 +404,8 @@ these forms: - "`!ATTR`" requires that the attribute `ATTR` be unspecified. + +Note that when matching against a tree object, attributes are still +obtained from working tree, not from the given tree object. exclude;; After a path matches any non-exclude pathspec, it will be run
Just a poke again about what I brought up in the thread you replied to in
https://public-inbox.org/git/CACsJy8CLHQ0mKhKXvTDAqy9TLwEFBSvHEu5UbPxHX4is2mK+Cg@mail.gmail.com/
I.e. the documentation of these various wildmatch() / attributes patterns we support is all over the place. Some in gitignore(5), some not documented at all, and some in gitglossary(7) (which really should not be serving as primary documentation for anything).
Re: [PATCH 5/5] tree-walk: support :(attr) matching
Quoted 30 lines · Ævar Arnfjörð Bjarmason
On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:
As noted in
https://public-inbox.org/git/[email protected]/ I'm happy to see this implemented. I have not read this patch in much detail...Quoted 8 lines
[...]
Documentation/glossary-content.txt | 2 +[...]
Documentation/glossary-content.txt +0
-0Documentation/glossary-content.txt these forms: · line 411
Quoted 8 lines
Documentation/glossary-content.txt +2
-0@@ -404,6 +404,8 @@ these forms: - "`!ATTR`" requires that the attribute `ATTR` be unspecified. + +Note that when matching against a tree object, attributes are still +obtained from working tree, not from the given tree object. exclude;; After a path matches any non-exclude pathspec, it will be runJust a poke again about what I brought up in the thread you replied to in
https://public-inbox.org/git/CACsJy8CLHQ0mKhKXvTDAqy9TLwEFBSvHEu5UbPxHX4is2mK+Cg@mail.gmail.com/
Quoted 4 lines
I.e. the documentation of these various
wildmatch()/ attributes patterns we support is all over the place. Some in gitignore(5), some not documented at all, and some in gitglossary(7) (which really should not be serving as primary documentation for anything).
Poking me is not going help, I'm afraid. Except bugs, which have highest priority to me, I do other stuff first either because I need it or it's fun to do, and this wildmatch documentation is neither (and I have a long backlog).
Signature · 1 line
Duy
Quoted 8 lines · Nguyễn Thái Ngọc Duy
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.
Quoted 6 lines
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.
Quoted 33 lines · Ævar Arnfjörð Bjarmason
Quoted 8 lines · Nguyễn Thái Ngọc Duy
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.Quoted 6 lines
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.
Quoted 6 lines
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
65I'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
3457From 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)'".
Quoted 19 lines · Ævar Arnfjörð Bjarmason
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 65I'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 3457From my reading of
parse_pathspec_attr_match()andmatch_attrs()this isn't possible and I'd need to support ':(attr:diff!=perl)' via a newMATCH_NOT_VALUEmode. But I wanted to make sure I wasn't missing some subtlety, i.e. that this was implemented already via some other feature.
Quoted 7 lines
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).
Signature · 1 line
Duy
Quoted 23 lines · Ævar Arnfjörð Bjarmason
Quoted 6 lines
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.
Quoted 6 lines
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
Quoted 4 lines · Nguyễn Thái Ngọc Duy
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.
Quoted 4 lines
The main patch is the last one. The others are just to open a path to pass "struct
index_state*" down totree_entry_interesting(). This may become standard procedure because we don't want to stickthe_index(orthe_repository) here and there.
Yup.
Quoted 15 lines · Nguyễn Thái Ngọc Duy
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.
Quoted 4 lines
The main patch is the last one. The others are just to open a path to pass "struct
index_state*" down totree_entry_interesting(). This may become standard procedure because we don't want to stickthe_index(orthe_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/[email protected]/
So this series fixes 1/2 of the issues noted there, but git-ls-tree will still die with the same error.
Quoted 24 lines · Ævar Arnfjörð Bjarmason
Quoted 15 lines · Nguyễn Thái Ngọc Duy
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.
Quoted 4 lines
The main patch is the last one. The others are just to open a path to pass "struct
index_state*" down totree_entry_interesting(). This may become standard procedure because we don't want to stickthe_index(orthe_repository) here and there.
Quoted 6 lines
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/[email protected]/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
Signature · 1 line
Duy