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

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

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

The main patch is the last one. The others are just to open a path to
pass "struct index_state *" down to tree_entry_interesting(). This may
become standard procedure because we don't want to stick the_index (or
the_repository) here and there.

Nguyễn Thái Ngọc Duy (5):
  tree.c: make read_tree*() take 'struct repository *'
  tree-walk.c: make tree_entry_interesting() take an index
  pathspec.h: clean up "extern" in function declarations
  dir.c: move, rename and export match_attrs()
  tree-walk: support :(attr) matching

 Documentation/glossary-content.txt |  2 +
 archive.c                          |  6 +-
 builtin/checkout.c                 |  3 +-
 builtin/grep.c                     |  3 +-
 builtin/log.c                      |  5 +-
 builtin/ls-files.c                 |  2 +-
 builtin/ls-tree.c                  |  3 +-
 builtin/merge-tree.c               |  2 +-
 dir.c                              | 41 +-------------
 list-objects.c                     |  3 +-
 merge-recursive.c                  |  3 +-
 pathspec.c                         | 38 +++++++++++++
 pathspec.h                         | 27 +++++----
 revision.c                         |  1 +
 t/t6135-pathspec-with-attrs.sh     | 58 ++++++++++++++++++-
 tree-diff.c                        |  3 +-
 tree-walk.c                        | 89 ++++++++++++++++++++++--------
 tree-walk.h                        | 10 ++--
 tree.c                             | 21 ++++---
 tree.h                             | 18 +++---
 unpack-trees.c                     |  6 +-
 21 files changed, 235 insertions(+), 109 deletions(-)

-- 
2.19.1.1327.g328c130451.dirty

[PATCH 1/5] tree.c: make read_tree*() take 'struct repository *'

[Nguyễn Thái Ngọc Duy]

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.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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(-)

diff --git a/archive.c b/archive.c
index fd556c28e4..bfa9cc20c9 100644
--- a/archive.c
+++ b/archive.c
@@ -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;
diff --git a/builtin/checkout.c b/builtin/checkout.c
index acdafc6e4c..c9dda8e82e 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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
diff --git a/builtin/log.c b/builtin/log.c
index 061d4fd864..d493fa915e 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -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:
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 7f9919a362..8db0cccbf3 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -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++) {
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index fe3b952cb3..6855f7fea5 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -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);
 }
diff --git a/merge-recursive.c b/merge-recursive.c
index acc2f64a4e..b9467f5ecf 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -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,
diff --git a/tree.c b/tree.c
index 215d3fdc7c..ecd8c8ac29 100644
--- a/tree.c
+++ b/tree.c
@@ -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;
 
diff --git a/tree.h b/tree.h
index d4807dc805..9383745073 100644
--- a/tree.h
+++ b/tree.h
@@ -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 1
 typedef 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 */
-- 
2.19.1.1327.g328c130451.dirty

[PATCH 2/5] tree-walk.c: make tree_entry_interesting() take an index

[Nguyễn Thái Ngọc Duy]

In 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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 56e4a11052..f6e086c287 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -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);
 
diff --git a/builtin/merge-tree.c b/builtin/merge-tree.c
index 70f6fc9167..4984b7e12e 100644
--- a/builtin/merge-tree.c
+++ b/builtin/merge-tree.c
@@ -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)
diff --git a/list-objects.c b/list-objects.c
index c41cc80db5..63c395d9c2 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -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;
diff --git a/revision.c b/revision.c
index bdd3e7c9f1..7ced1ee02b 100644
--- a/revision.c
+++ b/revision.c
@@ -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;
diff --git a/tree-diff.c b/tree-diff.c
index 0e54324610..34ee3b13b8 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -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;
diff --git a/tree-walk.c b/tree-walk.c
index 79bafbd1a2..517bcdecf9 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -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 &&
diff --git a/tree-walk.h b/tree-walk.h
index 196831007e..eefd26bb62 100644
--- a/tree-walk.h
+++ b/tree-walk.h
@@ -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
diff --git a/tree.c b/tree.c
index ecd8c8ac29..0b5c84d0d7 100644
--- a/tree.c
+++ b/tree.c
@@ -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)
diff --git a/unpack-trees.c b/unpack-trees.c
index 7570df481b..7f9548e741 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -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;
-- 
2.19.1.1327.g328c130451.dirty

[PATCH 3/5] pathspec.h: clean up "extern" in function declarations

[Nguyễn Thái Ngọc Duy]

"extern" on functions is not required and the trend has been removing
it from header files.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 pathspec.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/pathspec.h b/pathspec.h
index a6525a6551..5fd781d695 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -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 */
-- 
2.19.1.1327.g328c130451.dirty

[PATCH 4/5] dir.c: move, rename and export match_attrs()

[Nguyễn Thái Ngọc Duy]

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 dir.c      | 41 ++---------------------------------------
 pathspec.c | 38 ++++++++++++++++++++++++++++++++++++++
 pathspec.h |  3 +++
 3 files changed, 43 insertions(+), 39 deletions(-)

diff --git a/dir.c b/dir.c
index ab6477d777..2128448219 100644
--- a/dir.c
+++ b/dir.c
@@ -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 */
diff --git a/pathspec.c b/pathspec.c
index 6f005996fd..e85298f68c 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -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;
+}
diff --git a/pathspec.h b/pathspec.h
index 5fd781d695..1c18a2c90c 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -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 */
-- 
2.19.1.1327.g328c130451.dirty

[PATCH 5/5] tree-walk: support :(attr) matching

[Nguyễn Thái Ngọc Duy]

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

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Documentation/glossary-content.txt |  2 +
 t/t6135-pathspec-with-attrs.sh     | 58 +++++++++++++++++++++++++-
 tree-walk.c                        | 67 +++++++++++++++++++++++-------
 3 files changed, 112 insertions(+), 15 deletions(-)

diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
index 0d2aa48c63..023ca95e7c 100644
--- a/Documentation/glossary-content.txt
+++ b/Documentation/glossary-content.txt
@@ -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
diff --git a/t/t6135-pathspec-with-attrs.sh b/t/t6135-pathspec-with-attrs.sh
index e436a73962..457cc167c7 100755
--- a/t/t6135-pathspec-with-attrs.sh
+++ b/t/t6135-pathspec-with-attrs.sh
@@ -31,7 +31,7 @@ test_expect_success 'setup a tree' '
 	mkdir sub &&
 	while read path
 	do
-		: >$path &&
+		echo content >$path &&
 		git add $path || return 1
 	done <expect &&
 	git commit -m "initial commit" &&
@@ -48,6 +48,10 @@ test_expect_success 'pathspec with labels and non existent .gitattributes' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'pathspec with labels and non existent .gitattributes (2)' '
+	test_must_fail git grep content HEAD -- ":(attr:label)"
+'
+
 test_expect_success 'setup .gitattributes' '
 	cat <<-\EOF >.gitattributes &&
 	fileA labelA
@@ -74,6 +78,15 @@ test_expect_success 'check specific set attr' '
 	test_cmp expect actual
 '
 
+test_expect_success 'check specific set attr (2)' '
+	cat <<-\EOF >expect &&
+	HEAD:fileSetLabel
+	HEAD:sub/fileSetLabel
+	EOF
+	git grep -l content HEAD ":(attr:label)" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'check specific unset attr' '
 	cat <<-\EOF >expect &&
 	fileUnsetLabel
@@ -83,6 +96,15 @@ test_expect_success 'check specific unset attr' '
 	test_cmp expect actual
 '
 
+test_expect_success 'check specific unset attr (2)' '
+	cat <<-\EOF >expect &&
+	HEAD:fileUnsetLabel
+	HEAD:sub/fileUnsetLabel
+	EOF
+	git grep -l content HEAD ":(attr:-label)" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'check specific value attr' '
 	cat <<-\EOF >expect &&
 	fileValue
@@ -94,6 +116,16 @@ test_expect_success 'check specific value attr' '
 	test_must_be_empty actual
 '
 
+test_expect_success 'check specific value attr (2)' '
+	cat <<-\EOF >expect &&
+	HEAD:fileValue
+	HEAD:sub/fileValue
+	EOF
+	git grep -l content HEAD ":(attr:label=foo)" >actual &&
+	test_cmp expect actual &&
+	test_must_fail git grep -l content HEAD ":(attr:label=bar)"
+'
+
 test_expect_success 'check unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
@@ -118,6 +150,30 @@ test_expect_success 'check unspecified attr' '
 	test_cmp expect actual
 '
 
+test_expect_success 'check unspecified attr (2)' '
+	cat <<-\EOF >expect &&
+	HEAD:.gitattributes
+	HEAD:fileA
+	HEAD:fileAB
+	HEAD:fileAC
+	HEAD:fileB
+	HEAD:fileBC
+	HEAD:fileC
+	HEAD:fileNoLabel
+	HEAD:fileWrongLabel
+	HEAD:sub/fileA
+	HEAD:sub/fileAB
+	HEAD:sub/fileAC
+	HEAD:sub/fileB
+	HEAD:sub/fileBC
+	HEAD:sub/fileC
+	HEAD:sub/fileNoLabel
+	HEAD:sub/fileWrongLabel
+	EOF
+	git grep -l ^ HEAD ":(attr:!label)" >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'check multiple unspecified attr' '
 	cat <<-\EOF >expect &&
 	.gitattributes
diff --git a/tree-walk.c b/tree-walk.c
index 517bcdecf9..08210a4109 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -949,7 +949,8 @@ static enum interesting do_match(struct index_state *istate,
 		       PATHSPEC_LITERAL |
 		       PATHSPEC_GLOB |
 		       PATHSPEC_ICASE |
-		       PATHSPEC_EXCLUDE);
+		       PATHSPEC_EXCLUDE |
+		       PATHSPEC_ATTR);
 
 	if (!ps->nr) {
 		if (!ps->recursive ||
@@ -981,14 +982,20 @@ static enum interesting do_match(struct index_state *istate,
 
 			if (!ps->recursive ||
 			    !(ps->magic & PATHSPEC_MAXDEPTH) ||
-			    ps->max_depth == -1)
-				return all_entries_interesting;
-
-			return within_depth(base_str + matchlen + 1,
-					    baselen - matchlen - 1,
-					    !!S_ISDIR(entry->mode),
-					    ps->max_depth) ?
-				entry_interesting : entry_not_interesting;
+			    ps->max_depth == -1) {
+				if (!item->attr_match_nr)
+					return all_entries_interesting;
+				else
+					goto interesting;
+			}
+
+			if (within_depth(base_str + matchlen + 1,
+					 baselen - matchlen - 1,
+					 !!S_ISDIR(entry->mode),
+					 ps->max_depth))
+				goto interesting;
+			else
+				return entry_not_interesting;
 		}
 
 		/* Either there must be no base, or the base must match. */
@@ -996,12 +1003,12 @@ static enum interesting do_match(struct index_state *istate,
 			if (match_entry(item, entry, pathlen,
 					match + baselen, matchlen - baselen,
 					&never_interesting))
-				return entry_interesting;
+				goto interesting;
 
 			if (item->nowildcard_len < item->len) {
 				if (!git_fnmatch(item, match + baselen, entry->path,
 						 item->nowildcard_len - baselen))
-					return entry_interesting;
+					goto interesting;
 
 				/*
 				 * Match all directories. We'll try to
@@ -1022,7 +1029,7 @@ static enum interesting do_match(struct index_state *istate,
 				    !ps_strncmp(item, match + baselen,
 						entry->path,
 						item->nowildcard_len - baselen))
-					return entry_interesting;
+					goto interesting;
 			}
 
 			continue;
@@ -1057,7 +1064,7 @@ static enum interesting do_match(struct index_state *istate,
 		if (!git_fnmatch(item, match, base->buf + base_offset,
 				 item->nowildcard_len)) {
 			strbuf_setlen(base, base_offset + baselen);
-			return entry_interesting;
+			goto interesting;
 		}
 
 		/*
@@ -1071,7 +1078,7 @@ static enum interesting do_match(struct index_state *istate,
 		    !ps_strncmp(item, match, base->buf + base_offset,
 				item->nowildcard_len)) {
 			strbuf_setlen(base, base_offset + baselen);
-			return entry_interesting;
+			goto interesting;
 		}
 
 		strbuf_setlen(base, base_offset + baselen);
@@ -1085,6 +1092,38 @@ static enum interesting do_match(struct index_state *istate,
 		 */
 		if (ps->recursive && S_ISDIR(entry->mode))
 			return entry_interesting;
+		continue;
+interesting:
+		if (item->attr_match_nr) {
+			int ret;
+
+			/*
+			 * Must not return all_entries_not_interesting
+			 * prematurely. We do not know if all entries do not
+			 * match some attributes with current attr API.
+			 */
+			never_interesting = entry_not_interesting;
+
+			/*
+			 * Consider all directories interesting (because some
+			 * of those files inside may match some attributes
+			 * even though the parent dir does not)
+			 *
+			 * FIXME: attributes _can_ match directories and we
+			 * can probably return all_entries_interesting or
+			 * all_entries_not_interesting here if matched.
+			 */
+			if (S_ISDIR(entry->mode))
+				return entry_interesting;
+
+			strbuf_add(base, entry->path, pathlen);
+			ret = match_pathspec_attrs(istate, base->buf + base_offset,
+						   base->len - base_offset, item);
+			strbuf_setlen(base, base_offset + baselen);
+			if (!ret)
+				continue;
+		}
+		return entry_interesting;
 	}
 	return never_interesting; /* No matches */
 }
-- 
2.19.1.1327.g328c130451.dirty

Re: [PATCH 5/5] tree-walk: support :(attr) matching

[Æ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/87d0r217vr.fsf@evledraar.gmail.com/ I'm
happy to see this implemented. I have not read this patch in much
detail...

> [...]
>  Documentation/glossary-content.txt |  2 +
> [...]
> diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
> index 0d2aa48c63..023ca95e7c 100644
> --- a/Documentation/glossary-content.txt
> +++ b/Documentation/glossary-content.txt
> @@ -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

[Duy Nguyen]

On Sun, Nov 18, 2018 at 8:58 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:
>
> As noted in
> https://public-inbox.org/git/87d0r217vr.fsf@evledraar.gmail.com/ I'm
> happy to see this implemented. I have not read this patch in much
> detail...
>
> > [...]
> >  Documentation/glossary-content.txt |  2 +
> > [...]
> > diff --git a/Documentation/glossary-content.txt b/Documentation/glossary-content.txt
> > index 0d2aa48c63..023ca95e7c 100644
> > --- a/Documentation/glossary-content.txt
> > +++ b/Documentation/glossary-content.txt
> > @@ -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).

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

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

[Ævar Arnfjörð Bjarmason]

On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:

> When :(attr) was added, it supported one of the two main pathspec
> matching functions, the one that works on a list of paths. The other
> one works on a tree, tree_entry_interesting(), which gets :(attr)
> support in this series.
>
> With this, "git grep <pattern> <tree> -- :(attr)" or "git log :(attr)"
> will not abort with BUG() anymore.
>
> But this also reveals an interesting thing: even though we walk on a
> tree, we check attributes from _worktree_ (and optionally fall back to
> the index). This is how attributes are implemented since forever. I
> think this is not a big deal if we communicate clearly with the user.
> But otherwise, this series can be scraped, as reading attributes from
> a specific tree could be a lot of work.

I'm very happy to see this implemented, and I think the behavior
described here is the right way to go. E.g. in git.git we have diff=perl
entries in .gitattributes. It would suck if:

    git log ':(attr:diff=perl)'

Would only list commits as far as 20460635a8 (".gitattributes: use the
"perl" differ for Perl", 2018-04-26), since that's when we stop having
that attribute. Ditto for wanting to run "grep" on e.g. perl files in
2.12.0.

I have also run into cases where I want to use a .gitattributes file
from a specific commit. E.g. when writing pre-receive hooks where I've
wanted the .gitattributes of the commit being pushed to configure
something about it. But as you note this isn't supported at all.

But a concern is whether we should be making :(attr:*) behave like this
for now. Are we going to regret it later? I don't think so, I think
wanting to use the current working tree's / index's is the most sane
default, and if we get the ability to read it from revisions as we
e.g. walk the log it would make most sense to just call that
:(treeattr:*) or something like that.

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

[Ævar Arnfjörð Bjarmason]

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

> On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:
>
>> When :(attr) was added, it supported one of the two main pathspec
>> matching functions, the one that works on a list of paths. The other
>> one works on a tree, tree_entry_interesting(), which gets :(attr)
>> support in this series.
>>
>> With this, "git grep <pattern> <tree> -- :(attr)" or "git log :(attr)"
>> will not abort with BUG() anymore.
>>
>> But this also reveals an interesting thing: even though we walk on a
>> tree, we check attributes from _worktree_ (and optionally fall back to
>> the index). This is how attributes are implemented since forever. I
>> think this is not a big deal if we communicate clearly with the user.
>> But otherwise, this series can be scraped, as reading attributes from
>> a specific tree could be a lot of work.
>
> I'm very happy to see this implemented, and I think the behavior
> described here is the right way to go. E.g. in git.git we have diff=perl
> entries in .gitattributes. It would suck if:
>
>     git log ':(attr:diff=perl)'
>
> Would only list commits as far as 20460635a8 (".gitattributes: use the
> "perl" differ for Perl", 2018-04-26), since that's when we stop having
> that attribute. Ditto for wanting to run "grep" on e.g. perl files in
> 2.12.0.
>
> I have also run into cases where I want to use a .gitattributes file
> from a specific commit. E.g. when writing pre-receive hooks where I've
> wanted the .gitattributes of the commit being pushed to configure
> something about it. But as you note this isn't supported at all.
>
> But a concern is whether we should be making :(attr:*) behave like this
> for now. Are we going to regret it later? I don't think so, I think
> wanting to use the current working tree's / index's is the most sane
> default, and if we get the ability to read it from revisions as we
> e.g. walk the log it would make most sense to just call that
> :(treeattr:*) or something like that.

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

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

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

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

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

I thought I could do:

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

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

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

[Duy Nguyen]

On Mon, Nov 19, 2018 at 12:16 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> As an aside, how do you do the inverse of matching for an attribute by
> value? I.e.:
>
>     $ git ls-files | wc -l; git ls-files ':(attr:diff=perl)' | wc -l
>     3522
>     65
>
> I'd like something gives me all files that don't match diff=perl,
> i.e. 3522-65 = 3457 files, or what I'd get if I constructed such a match
> manually with excludes:
>
>     $ git ls-files $(grep diff=perl .gitattributes | cut -d ' ' -f1 | sed 's!^!:(exclude)!') | wc -l
>     3457
>
> From my reading of parse_pathspec_attr_match() and match_attrs() this
> isn't possible and I'd need to support ':(attr:diff!=perl)' via a new
> MATCH_NOT_VALUE mode. But I wanted to make sure I wasn't missing some
> subtlety, i.e. that this was implemented already via some other feature.
>
> I thought I could do:
>
>     git ls-files ':(exclude):(attr:diff=perl)'
>
> But we don't support chaining like that, and this would only exclude a
> file that's actually called ":(attr:diff=perl)". I.e. created via
> something like "touch ':(attr:diff=perl)'".

I think we allow :(exclude,attr:diff=perl) which should "exclude all
paths that have diff=perl attribute". It's actually tested in t6135
for ls-files (but I didn't add the same test for 'git grep' because I
was so confident it would work; I'll work on that).
-- 
Duy

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

[Jeff King]

On Sun, Nov 18, 2018 at 08:51:52PM +0100, Ævar Arnfjörð Bjarmason wrote:

> > But this also reveals an interesting thing: even though we walk on a
> > tree, we check attributes from _worktree_ (and optionally fall back to
> > the index). This is how attributes are implemented since forever. I
> > think this is not a big deal if we communicate clearly with the user.
> > But otherwise, this series can be scraped, as reading attributes from
> > a specific tree could be a lot of work.
> 
> I'm very happy to see this implemented, and I think the behavior
> described here is the right way to go. E.g. in git.git we have diff=perl
> entries in .gitattributes. It would suck if:
> 
>     git log ':(attr:diff=perl)'
> 
> Would only list commits as far as 20460635a8 (".gitattributes: use the
> "perl" differ for Perl", 2018-04-26), since that's when we stop having
> that attribute. Ditto for wanting to run "grep" on e.g. perl files in
> 2.12.0.
> 
> I have also run into cases where I want to use a .gitattributes file
> from a specific commit. E.g. when writing pre-receive hooks where I've
> wanted the .gitattributes of the commit being pushed to configure
> something about it. But as you note this isn't supported at all.
> 
> But a concern is whether we should be making :(attr:*) behave like this
> for now. Are we going to regret it later? I don't think so, I think
> wanting to use the current working tree's / index's is the most sane
> default, and if we get the ability to read it from revisions as we
> e.g. walk the log it would make most sense to just call that
> :(treeattr:*) or something like that.

I think that ship already sailed with the fact that "git log -p" will
show diffs using the worktree attrs. I agree that it would sometimes be
nice to specify attributes from a particular tree, but at this point the
default probably needs to remain as it is.

-Peff

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> But this also reveals an interesting thing: even though we walk on a
> tree, we check attributes from _worktree_ (and optionally fall back to
> the index). This is how attributes are implemented since forever. I
> think this is not a big deal if we communicate clearly with the user.

This is pretty much deliberately designed to be so; the set of the
attributes in HEAD may be stale but by taking the contents from the
working tree the user can work it around.

> The main patch is the last one. The others are just to open a path to
> pass "struct index_state *" down to tree_entry_interesting(). This may
> become standard procedure because we don't want to stick the_index (or
> the_repository) here and there.

Yup.

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

[Ævar Arnfjörð Bjarmason]

On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:

> When :(attr) was added, it supported one of the two main pathspec
> matching functions, the one that works on a list of paths. The other
> one works on a tree, tree_entry_interesting(), which gets :(attr)
> support in this series.
>
> With this, "git grep <pattern> <tree> -- :(attr)" or "git log :(attr)"
> will not abort with BUG() anymore.
>
> But this also reveals an interesting thing: even though we walk on a
> tree, we check attributes from _worktree_ (and optionally fall back to
> the index). This is how attributes are implemented since forever. I
> think this is not a big deal if we communicate clearly with the user.
> But otherwise, this series can be scraped, as reading attributes from
> a specific tree could be a lot of work.
>
> The main patch is the last one. The others are just to open a path to
> pass "struct index_state *" down to tree_entry_interesting(). This may
> become standard procedure because we don't want to stick the_index (or
> the_repository) here and there.

Another side-note (this thread is turning into my personal blog at this
point...) I found an old related thread:
https://public-inbox.org/git/20170509225219.GB106700@google.com/

So this series fixes 1/2 of the issues noted there, but git-ls-tree will
still die with the same error.

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

[Duy Nguyen]

On Mon, Nov 19, 2018 at 12:42 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
>
>
> On Sun, Nov 18 2018, Nguyễn Thái Ngọc Duy wrote:
>
> > When :(attr) was added, it supported one of the two main pathspec
> > matching functions, the one that works on a list of paths. The other
> > one works on a tree, tree_entry_interesting(), which gets :(attr)
> > support in this series.
> >
> > With this, "git grep <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.
>
> Another side-note (this thread is turning into my personal blog at this
> point...) I found an old related thread:
> https://public-inbox.org/git/20170509225219.GB106700@google.com/
>
> So this series fixes 1/2 of the issues noted there, but git-ls-tree will
> still die with the same error.

If you mean BUG(), not it does not. There are just a couple more
places besides tree_entry_interesting() and match_pathspec() that need
to be aware of all the magic things. ls-tree is one of those but it's
well guarded and will display this instead

$ git ls-tree HEAD ':(attr:abc=def)'
fatal: :(attr:abc=def): pathspec magic not supported by this command: 'attr'

If you mean making ls-tree support :(attr) and other stuff, it's not
going to happen in near future. It may be nice to switch to
tree_entry_interesting() in this command, but if it was easy to do I
would have done it years ago :P
-- 
Duy