Relative ls-files

Relative ls-files

[John Tapsell]

Hi,

  Could someone add a feature to  ls-files  to list all the files in
the repository, but relative to the current path?  It would be useful
for many different aliases, such as "git locate".

Thanks,

John

Re: Relative ls-files

[Michael J Gruber]

John Tapsell venit, vidit, dixit 23.03.2011 11:33:
> Hi,
> 
>   Could someone add a feature to  ls-files  to list all the files in
> the repository, but relative to the current path?  It would be useful
> for many different aliases, such as "git locate".

That is in the works, we just have to decide between two suggested
notations:

git ls-files :
git ls-files :/

Michael

Re: Relative ls-files

[John Tapsell]

On 23 March 2011 10:49, Michael J Gruber <git@drmicha.warpmail.net> wrote:
> John Tapsell venit, vidit, dixit 23.03.2011 11:33:
>> Hi,
>>
>>   Could someone add a feature to  ls-files  to list all the files in
>> the repository, but relative to the current path?  It would be useful
>> for many different aliases, such as "git locate".
>
> That is in the works, we just have to decide between two suggested
> notations:
>
> git ls-files :
> git ls-files :/


What's wrong with --relative  or something?

I've never liked magical strange notations.  ":" already has a meaning.

John

Re: Relative ls-files

[demerphq]

On 23 March 2011 12:27, John Tapsell <johnflux@gmail.com> wrote:
> On 23 March 2011 10:49, Michael J Gruber <git@drmicha.warpmail.net> wrote:
>> John Tapsell venit, vidit, dixit 23.03.2011 11:33:
>>> Hi,
>>>
>>>   Could someone add a feature to  ls-files  to list all the files in
>>> the repository, but relative to the current path?  It would be useful
>>> for many different aliases, such as "git locate".
>>
>> That is in the works, we just have to decide between two suggested
>> notations:
>>
>> git ls-files :
>> git ls-files :/
>
>
> What's wrong with --relative  or something?
>
> I've never liked magical strange notations.  ":" already has a meaning.

+1

yves

-- 
perl -Mre=debug -e "/just|another|perl|hacker/"

Re: Relative ls-files

[Michael J Gruber]

demerphq venit, vidit, dixit 23.03.2011 12:28:
> On 23 March 2011 12:27, John Tapsell <johnflux@gmail.com> wrote:
>> On 23 March 2011 10:49, Michael J Gruber <git@drmicha.warpmail.net> wrote:
>>> John Tapsell venit, vidit, dixit 23.03.2011 11:33:
>>>> Hi,
>>>>
>>>>   Could someone add a feature to  ls-files  to list all the files in
>>>> the repository, but relative to the current path?  It would be useful
>>>> for many different aliases, such as "git locate".
>>>
>>> That is in the works, we just have to decide between two suggested
>>> notations:
>>>
>>> git ls-files :
>>> git ls-files :/
>>
>>
>> What's wrong with --relative  or something?

??

ls-files already lists files relative to cwd. What you need is a way to
specify that you want all files, not just those beneath cwd.

>>
>> I've never liked magical strange notations.  ":" already has a meaning.
> 
> +1

I think you've both been missing out on the discussions about
--full-tree and refspec notation for that...

Of course ":" has a meaning. "git show HEAD:" lists all files at the
top-level of the tree at HEAD. That is exactly why ":" has been
suggested to denote the top-level for pathspecs also. Note that this is
much more general than just an option for ls-files.

Michael

Re: Relative ls-files

[Martin Langhoff]

On Wed, Mar 23, 2011 at 7:42 AM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Of course ":" has a meaning. "git show HEAD:" lists all files at the
> top-level of the tree at HEAD.

I got excited for a moment... now tested it and there isn't a recursive flag :-(

I've been pining for something easy and intuitive like git ls-files
<treeish>. You can do git ls-files --with-tree=<treeish> but I find it
very counter-intuitive.

Context: in some projects, I need to ask

 - is this file in branch foo?
 - has this file moved in branch foo?
 - what files with extension .zoo exist in branch foo?

and the answer is git ls-files --with-tree=<treeish> | grep <regex> .
But the --with-tree param is so counterintuitive to me that I read the
manpage, everytime.

Not sure whether the bug is in me or in git, but it sure hits some
cognitive dissonance.


m
-- 
 martin.langhoff@gmail.com
 martin@laptop.org -- Software Architect - OLPC
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

Re: Relative ls-files

[Junio C Hamano]

Martin Langhoff <martin.langhoff@gmail.com> writes:

> and the answer is git ls-files --with-tree=<treeish> | grep <regex> .
> But the --with-tree param is so counterintuitive to me that I read the
> manpage, everytime.

Because --with-tree nor ls-files is the tool that was designed for.

If you want to find out about a branch, why aren't you using "ls-tree -r"?

Re: Relative ls-files

[Nguyen Thai Ngoc Duy]

On Wed, Mar 23, 2011 at 9:09 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Martin Langhoff <martin.langhoff@gmail.com> writes:
>
>> and the answer is git ls-files --with-tree=<treeish> | grep <regex> .
>> But the --with-tree param is so counterintuitive to me that I read the
>> manpage, everytime.
>
> Because --with-tree nor ls-files is the tool that was designed for.
>
> If you want to find out about a branch, why aren't you using "ls-tree -r"?

(off topic?)

"ls-tree -r" does not understand wildcards. Maybe it should not, but I
think read_tree_recursive() should use tree_entry_interesting(). The
function it uses for matching, match_tree_entry, looks like another
variant of t_e_interesting.
-- 
Duy

Re: Relative ls-files

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> (off topic?)
>
> "ls-tree -r" does not understand wildcards. Maybe it should not, but I
> think read_tree_recursive() should use tree_entry_interesting(). The
> function it uses for matching, match_tree_entry, looks like another
> variant of t_e_interesting.

I thought that you already did that within the scope of your earlier
topic.

"ls-tree" is quite a low-level plumbing, and until we absolutely know that
all the old scripts people wrote relying on its original behaviour are
extinct, we shouldn't touch it without a reasonable breakage protection.

It is Ok to give it a command line option to make it in line with other
pathspec users, namely, that causes the command to missing pathspec should
automatically mean the whole tree and to honor globs.  No existing script
would be using that option, so that is a safe thing to do.

Re: Relative ls-files

[Nguyen Thai Ngoc Duy]

On Wed, Mar 23, 2011 at 10:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
>
>> (off topic?)
>>
>> "ls-tree -r" does not understand wildcards. Maybe it should not, but I
>> think read_tree_recursive() should use tree_entry_interesting(). The
>> function it uses for matching, match_tree_entry, looks like another
>> variant of t_e_interesting.
>
> I thought that you already did that within the scope of your earlier
> topic.
>
> "ls-tree" is quite a low-level plumbing, and until we absolutely know that
> all the old scripts people wrote relying on its original behaviour are
> extinct, we shouldn't touch it without a reasonable breakage protection.
>
> It is Ok to give it a command line option to make it in line with other
> pathspec users, namely, that causes the command to missing pathspec should
> automatically mean the whole tree and to honor globs.  No existing script
> would be using that option, so that is a safe thing to do.

This read_tree_recursive() affects 'archive' and 'checkout -- paths'
(ie. no wildcard awareness). I will probably move match_tree_entry()
to ls-tree.c to preserve its behavior, then change
read_tree_recursive() to use tree_entry_interesting().
-- 
Duy

Re: Relative ls-files

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> This read_tree_recursive() affects 'archive' and 'checkout -- paths'
> (ie. no wildcard awareness). I will probably move match_tree_entry()
> to ls-tree.c to preserve its behavior, then change
> read_tree_recursive() to use tree_entry_interesting().

[jc: culled people other than those in the "magic pathspec" discussion
from CC list]

Wouldn't it be more sensible to add a bit ("is this a glob or are we
expected to match it literally") to each element in the pathspec array,
and pass a pathspec whose elements are all marked as "literal" down from
ls-tree?

The internal matcher (and traversal optimizer) would need to become aware
of "not a glob" match (and possibly different kinds of matches like "**/"
support) sooner or later, and when that happens you would need something
like the above on the caller side in ls-tree.

Re: Relative ls-files

[Nguyen Thai Ngoc Duy]

On Thu, Mar 24, 2011 at 5:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Wouldn't it be more sensible to add a bit ("is this a glob or are we
> expected to match it literally") to each element in the pathspec array,
> and pass a pathspec whose elements are all marked as "literal" down from
> ls-tree?
>
> The internal matcher (and traversal optimizer) would need to become aware
> of "not a glob" match (and possibly different kinds of matches like "**/"
> support) sooner or later, and when that happens you would need something
> like the above on the caller side in ls-tree.

I think it's possible even now. fnmatch() is only used if
pathspec_item.has_wildcard is true. So ls-tree.c only needs to clear
this field for all pathspecs and they will be treated literally. Let
me try..
-- 
Duy

[PATCH 1/2] Reimplement read_tree_recursive() using tree_entry_interesting()

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

read_tree_recursive() uses a very similar function, match_tree_entry, to
tree_entry_interesting() to do its path matching. This patch kills
match_tree_entry() in favor of tree_entry_interesting().

match_tree_entry(), like older version of tree_entry_interesting(), does
not support wildcard matching. New read_tree_recursive() retains this
behavior by forcing all pathspecs literal.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 tree.c |  156 +++++++++++++++++++++++++--------------------------------------
 1 files changed, 62 insertions(+), 94 deletions(-)

diff --git a/tree.c b/tree.c
index 5ab90af..2b23381 100644
--- a/tree.c
+++ b/tree.c
@@ -45,62 +45,14 @@ static int read_one_entry_quick(const unsigned char *sha1, const char *base, int
 				  ADD_CACHE_JUST_APPEND);
 }
 
-static int match_tree_entry(const char *base, int baselen, const char *path, unsigned int mode, const char **paths)
-{
-	const char *match;
-	int pathlen;
-
-	if (!paths)
-		return 1;
-	pathlen = strlen(path);
-	while ((match = *paths++) != NULL) {
-		int matchlen = strlen(match);
-
-		if (baselen >= matchlen) {
-			/* If it doesn't match, move along... */
-			if (strncmp(base, match, matchlen))
-				continue;
-			/* pathspecs match only at the directory boundaries */
-			if (!matchlen ||
-			    baselen == matchlen ||
-			    base[matchlen] == '/' ||
-			    match[matchlen - 1] == '/')
-				return 1;
-			continue;
-		}
-
-		/* Does the base match? */
-		if (strncmp(base, match, baselen))
-			continue;
-
-		match += baselen;
-		matchlen -= baselen;
-
-		if (pathlen > matchlen)
-			continue;
-
-		if (matchlen > pathlen) {
-			if (match[pathlen] != '/')
-				continue;
-			if (!S_ISDIR(mode))
-				continue;
-		}
-
-		if (strncmp(path, match, pathlen))
-			continue;
-
-		return 1;
-	}
-	return 0;
-}
-
-int read_tree_recursive(struct tree *tree,
-			const char *base, int baselen,
-			int stage, const char **match,
-			read_tree_fn_t fn, void *context)
+static int read_tree_1(struct tree *tree, struct strbuf *base,
+		       int stage, struct pathspec *pathspecs,
+		       read_tree_fn_t fn, void *context)
 {
 	struct tree_desc desc;
 	struct name_entry entry;
+	unsigned char sha1[20];
+	int len, retval, all_interesting = 0, oldlen = base->len;
 
 	if (parse_tree(tree))
 		return -1;
@@ -108,10 +60,22 @@ int read_tree_recursive(struct tree *tree,
 	init_tree_desc(&desc, tree->buffer, tree->size);
 
 	while (tree_entry(&desc, &entry)) {
-		if (!match_tree_entry(base, baselen, entry.path, entry.mode, match))
+		if (all_interesting)
+			retval = all_interesting > 0;
+		else {
+			retval = tree_entry_interesting(&entry, base, 0, pathspecs);
+			if (retval == 2)
+				all_interesting = 1;
+			else if (retval == -1) {
+				all_interesting = -1;
+				retval = 0;
+			}
+		}
+		if (!retval)
 			continue;
 
-		switch (fn(entry.sha1, base, baselen, entry.path, entry.mode, stage, context)) {
+		switch (fn(entry.sha1, base->buf, base->len,
+			   entry.path, entry.mode, stage, context)) {
 		case 0:
 			continue;
 		case READ_TREE_RECURSIVE:
@@ -119,56 +83,60 @@ int read_tree_recursive(struct tree *tree,
 		default:
 			return -1;
 		}
-		if (S_ISDIR(entry.mode)) {
-			int retval;
-			char *newbase;
-			unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
-
-			newbase = xmalloc(baselen + 1 + pathlen);
-			memcpy(newbase, base, baselen);
-			memcpy(newbase + baselen, entry.path, pathlen);
-			newbase[baselen + pathlen] = '/';
-			retval = read_tree_recursive(lookup_tree(entry.sha1),
-						     newbase,
-						     baselen + pathlen + 1,
-						     stage, match, fn, context);
-			free(newbase);
-			if (retval)
-				return -1;
-			continue;
-		} else if (S_ISGITLINK(entry.mode)) {
-			int retval;
-			struct strbuf path;
-			unsigned int entrylen;
-			struct commit *commit;
 
-			entrylen = tree_entry_len(entry.path, entry.sha1);
-			strbuf_init(&path, baselen + entrylen + 1);
-			strbuf_add(&path, base, baselen);
-			strbuf_add(&path, entry.path, entrylen);
-			strbuf_addch(&path, '/');
+		if (S_ISDIR(entry.mode))
+			hashcpy(sha1, entry.sha1);
+		else if (S_ISGITLINK(entry.mode)) {
+			struct commit *commit;
 
 			commit = lookup_commit(entry.sha1);
 			if (!commit)
-				die("Commit %s in submodule path %s not found",
-				    sha1_to_hex(entry.sha1), path.buf);
+				die("Commit %s in submodule path %s%s not found",
+				    sha1_to_hex(entry.sha1),
+				    base->buf, entry.path);
 
 			if (parse_commit(commit))
-				die("Invalid commit %s in submodule path %s",
-				    sha1_to_hex(entry.sha1), path.buf);
-
-			retval = read_tree_recursive(commit->tree,
-						     path.buf, path.len,
-						     stage, match, fn, context);
-			strbuf_release(&path);
-			if (retval)
-				return -1;
-			continue;
+				die("Invalid commit %s in submodule path %s%s",
+				    sha1_to_hex(entry.sha1),
+				    base->buf, entry.path);
+
+			hashcpy(sha1, commit->tree->object.sha1);
 		}
+		else
+			continue;
+
+		len = tree_entry_len(entry.path, entry.sha1);
+		strbuf_add(base, entry.path, len);
+		strbuf_addch(base, '/');
+		retval = read_tree_1(lookup_tree(sha1),
+				     base, stage, pathspecs,
+				     fn, context);
+		strbuf_setlen(base, oldlen);
+		if (retval)
+			return -1;
 	}
 	return 0;
 }
 
+int read_tree_recursive(struct tree *tree,
+			const char *base, int baselen,
+			int stage, const char **match,
+			read_tree_fn_t fn, void *context)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct pathspec pathspecs;
+	int i, ret;
+
+	init_pathspec(&pathspecs, match);
+	for (i = 0; i < pathspecs.nr; i++)
+		pathspecs.items[i].has_wildcard = 0;
+	strbuf_add(&sb, base, baselen);
+	ret = read_tree_1(tree, &sb, stage, &pathspecs, fn, context);
+	strbuf_release(&sb);
+	free_pathspec(&pathspecs);
+	return ret;
+}
+
 static int cmp_cache_name_compare(const void *a_, const void *b_)
 {
 	const struct cache_entry *ce1, *ce2;
-- 
1.7.4.74.g639db

[PATCH 2/2] Convert read_tree{,_recursive} to support struct pathspec

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

This patch changes behavior of the two functions. Previously it does
prefix matching only. Now it can also do wildcard matching.

All callers are updated. Some gain wildcard matching (archive,
checkout), others reset pathspec_item.has_wildcard to retain old
behavior (ls-files, ls-tree as they are plumbing).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 I should add a few more tests for other touched commands too. Later..

 archive.c                    |   14 ++++++++++----
 builtin/checkout.c           |    5 ++++-
 builtin/ls-files.c           |    9 +++++----
 builtin/ls-tree.c            |   12 +++++++-----
 merge-recursive.c            |    4 +++-
 t/t3102-ls-tree-wildcards.sh |   22 ++++++++++++++++++++++
 tree.c                       |   13 ++++---------
 tree.h                       |    4 ++--
 8 files changed, 57 insertions(+), 26 deletions(-)
 create mode 100755 t/t3102-ls-tree-wildcards.sh

diff --git a/archive.c b/archive.c
index 1944ed4..6eb5837 100644
--- a/archive.c
+++ b/archive.c
@@ -157,6 +157,7 @@ int write_archive_entries(struct archiver_args *args,
 	struct archiver_context context;
 	struct unpack_trees_options opts;
 	struct tree_desc t;
+	struct pathspec pathspecs;
 	int err;
 
 	if (args->baselen > 0 && args->base[args->baselen - 1] == '/') {
@@ -191,8 +192,10 @@ int write_archive_entries(struct archiver_args *args,
 		git_attr_set_direction(GIT_ATTR_INDEX, &the_index);
 	}
 
-	err = read_tree_recursive(args->tree, "", 0, 0, args->pathspec,
+	init_pathspec(&pathspecs, args->pathspec);
+	err = read_tree_recursive(args->tree, "", 0, 0, &pathspecs,
 				  write_archive_entry, &context);
+	free_pathspec(&pathspecs);
 	if (err == READ_TREE_RECURSIVE)
 		err = 0;
 	return err;
@@ -222,10 +225,13 @@ static int reject_entry(const unsigned char *sha1, const char *base,
 static int path_exists(struct tree *tree, const char *path)
 {
 	const char *pathspec[] = { path, NULL };
+	struct pathspec pathspecs;
+	int ret;
 
-	if (read_tree_recursive(tree, "", 0, 0, pathspec, reject_entry, NULL))
-		return 1;
-	return 0;
+	init_pathspec(&pathspecs, pathspec);
+	ret = read_tree_recursive(tree, "", 0, 0, &pathspecs, reject_entry, NULL);
+	free_pathspec(&pathspecs);
+	return ret != 0;
 }
 
 static void parse_pathspec_arg(const char **pathspec,
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2bf02f2..1558910 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -79,7 +79,10 @@ static int update_some(const unsigned char *sha1, const char *base, int baselen,
 
 static int read_tree_some(struct tree *tree, const char **pathspec)
 {
-	read_tree_recursive(tree, "", 0, 0, pathspec, update_some, NULL);
+	struct pathspec pathspecs;
+	init_pathspec(&pathspecs, pathspec);
+	read_tree_recursive(tree, "", 0, 0, &pathspecs, update_some, NULL);
+	free_pathspec(&pathspecs);
 
 	/* update the index with the given tree's info
 	 * for all args, expanding wildcards, and exit
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index fb2d5f4..2e001e1 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -338,7 +338,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 {
 	struct tree *tree;
 	unsigned char sha1[20];
-	const char **match;
+	struct pathspec pathspecs;
 	struct cache_entry *last_stage0 = NULL;
 	int i;
 
@@ -360,10 +360,11 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 		static const char *(matchbuf[2]);
 		matchbuf[0] = prefix;
 		matchbuf[1] = NULL;
-		match = matchbuf;
+		init_pathspec(&pathspecs, matchbuf);
+		pathspecs.items[0].has_wildcard = 0;
 	} else
-		match = NULL;
-	if (read_tree(tree, 1, match))
+		init_pathspec(&pathspecs, NULL);
+	if (read_tree(tree, 1, &pathspecs))
 		die("unable to read tree entries %s", tree_name);
 
 	for (i = 0; i < active_nr; i++) {
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index f73e6bd..9c074de 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -19,7 +19,7 @@ static int line_termination = '\n';
 #define LS_SHOW_SIZE 16
 static int abbrev;
 static int ls_options;
-static const char **pathspec;
+static struct pathspec pathspecs;
 static int chomp_prefix;
 static const char *ls_tree_prefix;
 
@@ -35,7 +35,7 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
 	if (ls_options & LS_RECURSIVE)
 		return 1;
 
-	s = pathspec;
+	s = pathspecs.raw;
 	if (!s)
 		return 0;
 
@@ -120,7 +120,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 {
 	unsigned char sha1[20];
 	struct tree *tree;
-	int full_tree = 0;
+	int i, full_tree = 0;
 	const struct option ls_tree_options[] = {
 		OPT_BIT('d', NULL, &ls_options, "only show trees",
 			LS_TREE_ONLY),
@@ -166,11 +166,13 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	if (get_sha1(argv[0], sha1))
 		die("Not a valid object name %s", argv[0]);
 
-	pathspec = get_pathspec(prefix, argv + 1);
+	init_pathspec(&pathspecs, get_pathspec(prefix, argv + 1));
+	for (i = 0; i < pathspecs.nr; i++)
+		pathspecs.items[i].has_wildcard = 0;
 	tree = parse_tree_indirect(sha1);
 	if (!tree)
 		die("not a tree object");
-	read_tree_recursive(tree, "", 0, 0, pathspec, show_tree, NULL);
+	read_tree_recursive(tree, "", 0, 0, &pathspecs, show_tree, NULL);
 
 	return 0;
 }
diff --git a/merge-recursive.c b/merge-recursive.c
index 8e82a8b..89887de 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -278,7 +278,9 @@ static int save_files_dirs(const unsigned char *sha1,
 static int get_files_dirs(struct merge_options *o, struct tree *tree)
 {
 	int n;
-	if (read_tree_recursive(tree, "", 0, 0, NULL, save_files_dirs, o))
+	struct pathspec match_all;
+	init_pathspec(&match_all, NULL);
+	if (read_tree_recursive(tree, "", 0, 0, &match_all, save_files_dirs, o))
 		return 0;
 	n = o->current_file_set.nr + o->current_directory_set.nr;
 	return n;
diff --git a/t/t3102-ls-tree-wildcards.sh b/t/t3102-ls-tree-wildcards.sh
new file mode 100755
index 0000000..f2b2a52
--- /dev/null
+++ b/t/t3102-ls-tree-wildcards.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+test_description='ls-tree with(out) wildcards'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir a aa "a*" &&
+	touch a/one aa/two "a*/three" &&
+	git add a/one aa/two "a*/three" &&
+	git commit -m test
+'
+
+test_expect_success 'ls-tree a* matches literally' '
+	cat >expected <<EOF &&
+100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391	a*/three
+EOF
+	git ls-tree -r HEAD "a*" >actual &&
+	test_cmp expected actual
+'
+
+test_done
diff --git a/tree.c b/tree.c
index 2b23381..9261b9e 100644
--- a/tree.c
+++ b/tree.c
@@ -120,20 +120,15 @@ static int read_tree_1(struct tree *tree, struct strbuf *base,
 
 int read_tree_recursive(struct tree *tree,
 			const char *base, int baselen,
-			int stage, const char **match,
+			int stage, struct pathspec *pathspecs,
 			read_tree_fn_t fn, void *context)
 {
 	struct strbuf sb = STRBUF_INIT;
-	struct pathspec pathspecs;
-	int i, ret;
+	int ret;
 
-	init_pathspec(&pathspecs, match);
-	for (i = 0; i < pathspecs.nr; i++)
-		pathspecs.items[i].has_wildcard = 0;
 	strbuf_add(&sb, base, baselen);
-	ret = read_tree_1(tree, &sb, stage, &pathspecs, fn, context);
+	ret = read_tree_1(tree, &sb, stage, pathspecs, fn, context);
 	strbuf_release(&sb);
-	free_pathspec(&pathspecs);
 	return ret;
 }
 
@@ -147,7 +142,7 @@ static int cmp_cache_name_compare(const void *a_, const void *b_)
 				  ce2->name, ce2->ce_flags);
 }
 
-int read_tree(struct tree *tree, int stage, const char **match)
+int read_tree(struct tree *tree, int stage, struct pathspec *match)
 {
 	read_tree_fn_t fn = NULL;
 	int i, err;
diff --git a/tree.h b/tree.h
index 2ff01a4..6e5f195 100644
--- a/tree.h
+++ b/tree.h
@@ -25,9 +25,9 @@ typedef int (*read_tree_fn_t)(const unsigned char *, const char *, int, const ch
 
 extern int read_tree_recursive(struct tree *tree,
 			       const char *base, int baselen,
-			       int stage, const char **match,
+			       int stage, struct pathspec *pathspecs,
 			       read_tree_fn_t fn, void *context);
 
-extern int read_tree(struct tree *tree, int stage, const char **paths);
+extern int read_tree(struct tree *tree, int stage, struct pathspec *pathspecs);
 
 #endif /* TREE_H */
-- 
1.7.4.74.g639db

Re: [PATCH 2/2] Convert read_tree{,_recursive} to support struct pathspec

[Nguyen Thai Ngoc Duy]

On Thu, Mar 24, 2011 at 09:41:15PM +0700, Nguyễn Thái Ngọc Duy wrote:
> All callers are updated.

Jeez.. I made last minute changes and it broke something. This should
be squashed in, because tree_entry_interesting() does not like struct
pathspec* == NULL.

--8<--
diff --git a/builtin/log.c b/builtin/log.c
index 796e9e5..cff39cf 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -396,6 +396,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 	struct object_array_entry *objects;
 	struct setup_revision_opt opt;
+	struct pathspec match_all;
 	int i, count, ret = 0;
 
 	git_config(git_log_config, NULL);
@@ -403,6 +404,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
 
+	init_pathspec(&match_all, NULL);
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.always_show_header = 1;
@@ -449,7 +451,7 @@ 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, NULL,
+			read_tree_recursive((struct tree *)o, "", 0, 0, &match_all,
 					show_tree_object, NULL);
 			rev.shown_one = 1;
 			break;
--8<--
-- 
Duy

Re: [PATCH 2/2] Convert read_tree{,_recursive} to support struct pathspec

[Junio C Hamano]

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

> This patch changes behavior of the two functions. Previously it does
> prefix matching only. Now it can also do wildcard matching.

The most important in this change is that it finally starts passing
pathspec, not "char **", down the callchain.

Except that variables should be called pathspec not pathspecs, I think
this is basically in very good shape.

Thanks.

Re: [PATCH 1/2] Reimplement read_tree_recursive() using tree_entry_interesting()

[Junio C Hamano]

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

> read_tree_recursive() uses a very similar function, match_tree_entry, to
> tree_entry_interesting() to do its path matching. This patch kills
> match_tree_entry() in favor of tree_entry_interesting().
> ...
> +int read_tree_recursive(struct tree *tree,
> +			const char *base, int baselen,
> +			int stage, const char **match,
> +			read_tree_fn_t fn, void *context)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	struct pathspec pathspecs;
> +	int i, ret;
> +
> +	init_pathspec(&pathspecs, match);
> +	for (i = 0; i < pathspecs.nr; i++)
> +		pathspecs.items[i].has_wildcard = 0;

I think this is going in the right direction, but I have been hoping that
we would eventually stop passing "const char **" pointers around when the
caller specifies what are the set of paths that are to be affected.

We still retain "const char **raw" field in "struct pathspec" even after
your previous series, but I think that is because there still are too many
callers that already have a "struct pathspec *pathspec" but pass only that
field (i.e. "pathspec->raw") down the callchain, and converting all the
callees was a bit too much to chew on during the previous series (which I
consider is fine and sane).  The eventual goal should be for such callers
to pass the "pathspec" itself, so that the callees can learn wildcarding
and all the "magic" fun stuff later.

As read_tree_recursive() has only 6 external callers, it may be a good
place to start that conversion.  Another offender is fill_directory() in
dir.c that are used in 6 places.

Thanks.

Re: [PATCH 1/2] Reimplement read_tree_recursive() using tree_entry_interesting()

[Junio C Hamano]

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

> +static int read_tree_1(struct tree *tree, struct strbuf *base,
> +		       int stage, struct pathspec *pathspecs,
> +		       read_tree_fn_t fn, void *context)

Micronit: call the variable pathspec, not pathspecs.

An instance of the structure contains a specification to limit the set of
paths to operate on as a set of pathspec elements, and this single set is
collectively called a "struct pathspec"; you are not passing an array that
has one or more pathspecs in it here (i.e. pathspecs[2] in this function
does not make sense).

It looks like the result is easier to follow, perhaps mostly thanks to the
(re)use of strbuf instead of allocating a new path when going deeper.

> @@ -108,10 +60,22 @@ int read_tree_recursive(struct tree *tree,
>  	init_tree_desc(&desc, tree->buffer, tree->size);
>  
>  	while (tree_entry(&desc, &entry)) {
> -		if (!match_tree_entry(base, baselen, entry.path, entry.mode, match))
> +		if (all_interesting)
> +			retval = all_interesting > 0;
> +		else {
> +			retval = tree_entry_interesting(&entry, base, 0, pathspecs);
> +			if (retval == 2)
> +				all_interesting = 1;
> +			else if (retval == -1) {
> +				all_interesting = -1;
> +				retval = 0;
> +			}
> +		}
> +		if (!retval)
>  			continue;

This "if all-interesting then avoid calling into an expensive matcher"
logic looks familiar but is not something that came from the parts deleted
by this patch.  Where did I see it? Is that something we can share in a
common helper function? Is such a sharing worth pursuing, considering that
the above is a reasonably trivial logic in a tight loop?

"That's how the return value of tree-entry-interesting is designed to be
used, and it is unsurprising that all the callers will fall into that
pattern" is a perfectly acceptable answer, I guess.

Thanks.

[PATCH 1/3] Reimplement read_tree_recursive() using tree_entry_interesting()

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

read_tree_recursive() uses a very similar function, match_tree_entry, to
tree_entry_interesting() to do its path matching. This patch kills
match_tree_entry() in favor of tree_entry_interesting().

match_tree_entry(), like older version of tree_entry_interesting(), does
not support wildcard matching. New read_tree_recursive() retains this
behavior by forcing all pathspecs literal.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 2011/3/25 Junio C Hamano <gitster@pobox.com>:
 > Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
 >
 >> +static int read_tree_1(struct tree *tree, struct strbuf *base,
 >> +                    int stage, struct pathspec *pathspecs,
 >> +                    read_tree_fn_t fn, void *context)
 >
 > Micronit: call the variable pathspec, not pathspecs.

 Fixed (also in the second patch)

 > This "if all-interesting then avoid calling into an expensive matcher"
 > logic looks familiar but is not something that came from the parts deleted
 > by this patch.  Where did I see it? Is that something we can share in a
 > common helper function? Is such a sharing worth pursuing, considering that
 > the above is a reasonably trivial logic in a tight loop?
 >
 > "That's how the return value of tree-entry-interesting is designed to be
 > used, and it is unsurprising that all the callers will fall into that
 > pattern" is a perfectly acceptable answer, I guess.

 Actually the surrounding code could be improved so that helper function
 is not needed. I also update other t_e_i() callsites in the third patch.

 tree.c |  152 ++++++++++++++++++++++++----------------------------------------
 1 files changed, 57 insertions(+), 95 deletions(-)

diff --git a/tree.c b/tree.c
index 5ab90af..db3a5c3 100644
--- a/tree.c
+++ b/tree.c
@@ -45,62 +45,14 @@ static int read_one_entry_quick(const unsigned char *sha1, const char *base, int
 				  ADD_CACHE_JUST_APPEND);
 }
 
-static int match_tree_entry(const char *base, int baselen, const char *path, unsigned int mode, const char **paths)
-{
-	const char *match;
-	int pathlen;
-
-	if (!paths)
-		return 1;
-	pathlen = strlen(path);
-	while ((match = *paths++) != NULL) {
-		int matchlen = strlen(match);
-
-		if (baselen >= matchlen) {
-			/* If it doesn't match, move along... */
-			if (strncmp(base, match, matchlen))
-				continue;
-			/* pathspecs match only at the directory boundaries */
-			if (!matchlen ||
-			    baselen == matchlen ||
-			    base[matchlen] == '/' ||
-			    match[matchlen - 1] == '/')
-				return 1;
-			continue;
-		}
-
-		/* Does the base match? */
-		if (strncmp(base, match, baselen))
-			continue;
-
-		match += baselen;
-		matchlen -= baselen;
-
-		if (pathlen > matchlen)
-			continue;
-
-		if (matchlen > pathlen) {
-			if (match[pathlen] != '/')
-				continue;
-			if (!S_ISDIR(mode))
-				continue;
-		}
-
-		if (strncmp(path, match, pathlen))
-			continue;
-
-		return 1;
-	}
-	return 0;
-}
-
-int read_tree_recursive(struct tree *tree,
-			const char *base, int baselen,
-			int stage, const char **match,
-			read_tree_fn_t fn, void *context)
+static int read_tree_1(struct tree *tree, struct strbuf *base,
+		       int stage, struct pathspec *pathspec,
+		       read_tree_fn_t fn, void *context)
 {
 	struct tree_desc desc;
 	struct name_entry entry;
+	unsigned char sha1[20];
+	int len, retval = 0, oldlen = base->len;
 
 	if (parse_tree(tree))
 		return -1;
@@ -108,10 +60,16 @@ int read_tree_recursive(struct tree *tree,
 	init_tree_desc(&desc, tree->buffer, tree->size);
 
 	while (tree_entry(&desc, &entry)) {
-		if (!match_tree_entry(base, baselen, entry.path, entry.mode, match))
-			continue;
+		if (retval != 2) {
+			retval = tree_entry_interesting(&entry, base, 0, pathspec);
+			if (retval < 0)
+				break;
+			if (retval == 0)
+				continue;
+		}
 
-		switch (fn(entry.sha1, base, baselen, entry.path, entry.mode, stage, context)) {
+		switch (fn(entry.sha1, base->buf, base->len,
+			   entry.path, entry.mode, stage, context)) {
 		case 0:
 			continue;
 		case READ_TREE_RECURSIVE:
@@ -119,56 +77,60 @@ int read_tree_recursive(struct tree *tree,
 		default:
 			return -1;
 		}
-		if (S_ISDIR(entry.mode)) {
-			int retval;
-			char *newbase;
-			unsigned int pathlen = tree_entry_len(entry.path, entry.sha1);
-
-			newbase = xmalloc(baselen + 1 + pathlen);
-			memcpy(newbase, base, baselen);
-			memcpy(newbase + baselen, entry.path, pathlen);
-			newbase[baselen + pathlen] = '/';
-			retval = read_tree_recursive(lookup_tree(entry.sha1),
-						     newbase,
-						     baselen + pathlen + 1,
-						     stage, match, fn, context);
-			free(newbase);
-			if (retval)
-				return -1;
-			continue;
-		} else if (S_ISGITLINK(entry.mode)) {
-			int retval;
-			struct strbuf path;
-			unsigned int entrylen;
-			struct commit *commit;
 
-			entrylen = tree_entry_len(entry.path, entry.sha1);
-			strbuf_init(&path, baselen + entrylen + 1);
-			strbuf_add(&path, base, baselen);
-			strbuf_add(&path, entry.path, entrylen);
-			strbuf_addch(&path, '/');
+		if (S_ISDIR(entry.mode))
+			hashcpy(sha1, entry.sha1);
+		else if (S_ISGITLINK(entry.mode)) {
+			struct commit *commit;
 
 			commit = lookup_commit(entry.sha1);
 			if (!commit)
-				die("Commit %s in submodule path %s not found",
-				    sha1_to_hex(entry.sha1), path.buf);
+				die("Commit %s in submodule path %s%s not found",
+				    sha1_to_hex(entry.sha1),
+				    base->buf, entry.path);
 
 			if (parse_commit(commit))
-				die("Invalid commit %s in submodule path %s",
-				    sha1_to_hex(entry.sha1), path.buf);
-
-			retval = read_tree_recursive(commit->tree,
-						     path.buf, path.len,
-						     stage, match, fn, context);
-			strbuf_release(&path);
-			if (retval)
-				return -1;
-			continue;
+				die("Invalid commit %s in submodule path %s%s",
+				    sha1_to_hex(entry.sha1),
+				    base->buf, entry.path);
+
+			hashcpy(sha1, commit->tree->object.sha1);
 		}
+		else
+			continue;
+
+		len = tree_entry_len(entry.path, entry.sha1);
+		strbuf_add(base, entry.path, len);
+		strbuf_addch(base, '/');
+		retval = read_tree_1(lookup_tree(sha1),
+				     base, stage, pathspec,
+				     fn, context);
+		strbuf_setlen(base, oldlen);
+		if (retval)
+			return -1;
 	}
 	return 0;
 }
 
+int read_tree_recursive(struct tree *tree,
+			const char *base, int baselen,
+			int stage, const char **match,
+			read_tree_fn_t fn, void *context)
+{
+	struct strbuf sb = STRBUF_INIT;
+	struct pathspec pathspec;
+	int i, ret;
+
+	init_pathspec(&pathspec, match);
+	for (i = 0; i < pathspec.nr; i++)
+		pathspec.items[i].has_wildcard = 0;
+	strbuf_add(&sb, base, baselen);
+	ret = read_tree_1(tree, &sb, stage, &pathspec, fn, context);
+	strbuf_release(&sb);
+	free_pathspec(&pathspec);
+	return ret;
+}
+
 static int cmp_cache_name_compare(const void *a_, const void *b_)
 {
 	const struct cache_entry *ce1, *ce2;
-- 
1.7.4.74.g639db

[PATCH 2/3] Convert read_tree{,_recursive} to support struct pathspec

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

This patch changes behavior of the two functions. Previously it does
prefix matching only. Now it can also do wildcard matching.

All callers are updated. Some gain wildcard matching (archive,
checkout), others reset pathspec_item.has_wildcard to retain old
behavior (ls-files, ls-tree as they are plumbing).

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 archive.c                    |   18 ++++++++++++------
 builtin/checkout.c           |    5 ++++-
 builtin/log.c                |    4 +++-
 builtin/ls-files.c           |    9 +++++----
 builtin/ls-tree.c            |   12 +++++++-----
 merge-recursive.c            |    4 +++-
 t/t3102-ls-tree-wildcards.sh |   22 ++++++++++++++++++++++
 tree.c                       |   13 ++++---------
 tree.h                       |    4 ++--
 9 files changed, 62 insertions(+), 29 deletions(-)
 create mode 100755 t/t3102-ls-tree-wildcards.sh

diff --git a/archive.c b/archive.c
index 1944ed4..42f2d2f 100644
--- a/archive.c
+++ b/archive.c
@@ -157,6 +157,7 @@ int write_archive_entries(struct archiver_args *args,
 	struct archiver_context context;
 	struct unpack_trees_options opts;
 	struct tree_desc t;
+	struct pathspec pathspec;
 	int err;
 
 	if (args->baselen > 0 && args->base[args->baselen - 1] == '/') {
@@ -191,8 +192,10 @@ int write_archive_entries(struct archiver_args *args,
 		git_attr_set_direction(GIT_ATTR_INDEX, &the_index);
 	}
 
-	err = read_tree_recursive(args->tree, "", 0, 0, args->pathspec,
+	init_pathspec(&pathspec, args->pathspec);
+	err = read_tree_recursive(args->tree, "", 0, 0, &pathspec,
 				  write_archive_entry, &context);
+	free_pathspec(&pathspec);
 	if (err == READ_TREE_RECURSIVE)
 		err = 0;
 	return err;
@@ -221,11 +224,14 @@ static int reject_entry(const unsigned char *sha1, const char *base,
 
 static int path_exists(struct tree *tree, const char *path)
 {
-	const char *pathspec[] = { path, NULL };
-
-	if (read_tree_recursive(tree, "", 0, 0, pathspec, reject_entry, NULL))
-		return 1;
-	return 0;
+	const char *paths[] = { path, NULL };
+	struct pathspec pathspec;
+	int ret;
+
+	init_pathspec(&pathspec, paths);
+	ret = read_tree_recursive(tree, "", 0, 0, &pathspec, reject_entry, NULL);
+	free_pathspec(&pathspec);
+	return ret != 0;
 }
 
 static void parse_pathspec_arg(const char **pathspec,
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2bf02f2..dc3eedf 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -79,7 +79,10 @@ static int update_some(const unsigned char *sha1, const char *base, int baselen,
 
 static int read_tree_some(struct tree *tree, const char **pathspec)
 {
-	read_tree_recursive(tree, "", 0, 0, pathspec, update_some, NULL);
+	struct pathspec ps;
+	init_pathspec(&ps, pathspec);
+	read_tree_recursive(tree, "", 0, 0, &ps, update_some, NULL);
+	free_pathspec(&ps);
 
 	/* 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 796e9e5..cff39cf 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -396,6 +396,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	struct rev_info rev;
 	struct object_array_entry *objects;
 	struct setup_revision_opt opt;
+	struct pathspec match_all;
 	int i, count, ret = 0;
 
 	git_config(git_log_config, NULL);
@@ -403,6 +404,7 @@ int cmd_show(int argc, const char **argv, const char *prefix)
 	if (diff_use_color_default == -1)
 		diff_use_color_default = git_use_color_default;
 
+	init_pathspec(&match_all, NULL);
 	init_revisions(&rev, prefix);
 	rev.diff = 1;
 	rev.always_show_header = 1;
@@ -449,7 +451,7 @@ 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, NULL,
+			read_tree_recursive((struct tree *)o, "", 0, 0, &match_all,
 					show_tree_object, NULL);
 			rev.shown_one = 1;
 			break;
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index fb2d5f4..199af46 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -338,7 +338,7 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 {
 	struct tree *tree;
 	unsigned char sha1[20];
-	const char **match;
+	struct pathspec pathspec;
 	struct cache_entry *last_stage0 = NULL;
 	int i;
 
@@ -360,10 +360,11 @@ void overlay_tree_on_cache(const char *tree_name, const char *prefix)
 		static const char *(matchbuf[2]);
 		matchbuf[0] = prefix;
 		matchbuf[1] = NULL;
-		match = matchbuf;
+		init_pathspec(&pathspec, matchbuf);
+		pathspec.items[0].has_wildcard = 0;
 	} else
-		match = NULL;
-	if (read_tree(tree, 1, match))
+		init_pathspec(&pathspec, NULL);
+	if (read_tree(tree, 1, &pathspec))
 		die("unable to read tree entries %s", tree_name);
 
 	for (i = 0; i < active_nr; i++) {
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index f73e6bd..4290723 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -19,7 +19,7 @@ static int line_termination = '\n';
 #define LS_SHOW_SIZE 16
 static int abbrev;
 static int ls_options;
-static const char **pathspec;
+static struct pathspec pathspec;
 static int chomp_prefix;
 static const char *ls_tree_prefix;
 
@@ -35,7 +35,7 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
 	if (ls_options & LS_RECURSIVE)
 		return 1;
 
-	s = pathspec;
+	s = pathspec.raw;
 	if (!s)
 		return 0;
 
@@ -120,7 +120,7 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 {
 	unsigned char sha1[20];
 	struct tree *tree;
-	int full_tree = 0;
+	int i, full_tree = 0;
 	const struct option ls_tree_options[] = {
 		OPT_BIT('d', NULL, &ls_options, "only show trees",
 			LS_TREE_ONLY),
@@ -166,11 +166,13 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	if (get_sha1(argv[0], sha1))
 		die("Not a valid object name %s", argv[0]);
 
-	pathspec = get_pathspec(prefix, argv + 1);
+	init_pathspec(&pathspec, get_pathspec(prefix, argv + 1));
+	for (i = 0; i < pathspec.nr; i++)
+		pathspec.items[i].has_wildcard = 0;
 	tree = parse_tree_indirect(sha1);
 	if (!tree)
 		die("not a tree object");
-	read_tree_recursive(tree, "", 0, 0, pathspec, show_tree, NULL);
+	read_tree_recursive(tree, "", 0, 0, &pathspec, show_tree, NULL);
 
 	return 0;
 }
diff --git a/merge-recursive.c b/merge-recursive.c
index 8e82a8b..89887de 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -278,7 +278,9 @@ static int save_files_dirs(const unsigned char *sha1,
 static int get_files_dirs(struct merge_options *o, struct tree *tree)
 {
 	int n;
-	if (read_tree_recursive(tree, "", 0, 0, NULL, save_files_dirs, o))
+	struct pathspec match_all;
+	init_pathspec(&match_all, NULL);
+	if (read_tree_recursive(tree, "", 0, 0, &match_all, save_files_dirs, o))
 		return 0;
 	n = o->current_file_set.nr + o->current_directory_set.nr;
 	return n;
diff --git a/t/t3102-ls-tree-wildcards.sh b/t/t3102-ls-tree-wildcards.sh
new file mode 100755
index 0000000..f2b2a52
--- /dev/null
+++ b/t/t3102-ls-tree-wildcards.sh
@@ -0,0 +1,22 @@
+#!/bin/sh
+
+test_description='ls-tree with(out) wildcards'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	mkdir a aa "a*" &&
+	touch a/one aa/two "a*/three" &&
+	git add a/one aa/two "a*/three" &&
+	git commit -m test
+'
+
+test_expect_success 'ls-tree a* matches literally' '
+	cat >expected <<EOF &&
+100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391	a*/three
+EOF
+	git ls-tree -r HEAD "a*" >actual &&
+	test_cmp expected actual
+'
+
+test_done
diff --git a/tree.c b/tree.c
index db3a5c3..698ecf7 100644
--- a/tree.c
+++ b/tree.c
@@ -114,20 +114,15 @@ static int read_tree_1(struct tree *tree, struct strbuf *base,
 
 int read_tree_recursive(struct tree *tree,
 			const char *base, int baselen,
-			int stage, const char **match,
+			int stage, struct pathspec *pathspec,
 			read_tree_fn_t fn, void *context)
 {
 	struct strbuf sb = STRBUF_INIT;
-	struct pathspec pathspec;
-	int i, ret;
+	int ret;
 
-	init_pathspec(&pathspec, match);
-	for (i = 0; i < pathspec.nr; i++)
-		pathspec.items[i].has_wildcard = 0;
 	strbuf_add(&sb, base, baselen);
-	ret = read_tree_1(tree, &sb, stage, &pathspec, fn, context);
+	ret = read_tree_1(tree, &sb, stage, pathspec, fn, context);
 	strbuf_release(&sb);
-	free_pathspec(&pathspec);
 	return ret;
 }
 
@@ -141,7 +136,7 @@ static int cmp_cache_name_compare(const void *a_, const void *b_)
 				  ce2->name, ce2->ce_flags);
 }
 
-int read_tree(struct tree *tree, int stage, const char **match)
+int read_tree(struct tree *tree, int stage, struct pathspec *match)
 {
 	read_tree_fn_t fn = NULL;
 	int i, err;
diff --git a/tree.h b/tree.h
index 2ff01a4..69bcb5e 100644
--- a/tree.h
+++ b/tree.h
@@ -25,9 +25,9 @@ typedef int (*read_tree_fn_t)(const unsigned char *, const char *, int, const ch
 
 extern int read_tree_recursive(struct tree *tree,
 			       const char *base, int baselen,
-			       int stage, const char **match,
+			       int stage, struct pathspec *pathspec,
 			       read_tree_fn_t fn, void *context);
 
-extern int read_tree(struct tree *tree, int stage, const char **paths);
+extern int read_tree(struct tree *tree, int stage, struct pathspec *pathspec);
 
 #endif /* TREE_H */
-- 
1.7.4.74.g639db

[PATCH 3/3] Improve tree_entry_interesting() handling code

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

t_e_i() can return -1 or 2 to early shortcut a search. Current code
may use up to two variables to handle it. One for saving return value
from t_e_i temporarily, one for saving return code 2.

The second variable is not needed. If we make sure the first variable
does not change until the next t_e_i() call, then we can do something
like this:

int ret = 0;

while (...) {
	if (ret != 2) {
		ret = t_e_i();
		if (ret < 0) /* no longer interesting */
			break;
		if (ret == 0) /* skip this round */
			continue;
	}
	/* ret > 0, interesting */
}

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/grep.c |   12 ++++++------
 list-objects.c |   18 +++++++-----------
 tree-diff.c    |   53 ++++++++++++++++++++---------------------------------
 3 files changed, 33 insertions(+), 50 deletions(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index 0bf8c01..9cb1f32 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -520,18 +520,18 @@ static int grep_cache(struct grep_opt *opt, const struct pathspec *pathspec, int
 static int grep_tree(struct grep_opt *opt, const struct pathspec *pathspec,
 		     struct tree_desc *tree, struct strbuf *base, int tn_len)
 {
-	int hit = 0, matched = 0;
+	int hit = 0, match = 0;
 	struct name_entry entry;
 	int old_baselen = base->len;
 
 	while (tree_entry(tree, &entry)) {
 		int te_len = tree_entry_len(entry.path, entry.sha1);
 
-		if (matched != 2) {
-			matched = tree_entry_interesting(&entry, base, tn_len, pathspec);
-			if (matched == -1)
-				break; /* no more matches */
-			if (!matched)
+		if (match != 2) {
+			match = tree_entry_interesting(&entry, base, tn_len, pathspec);
+			if (match < 0)
+				break;
+			if (match == 0)
 				continue;
 		}
 
diff --git a/list-objects.c b/list-objects.c
index 838b6a7..0fb44e7 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -68,7 +68,7 @@ static void process_tree(struct rev_info *revs,
 	struct tree_desc desc;
 	struct name_entry entry;
 	struct name_path me;
-	int all_interesting = (revs->diffopt.pathspec.nr == 0);
+	int match = revs->diffopt.pathspec.nr == 0 ? 2 : 0;
 	int baselen = base->len;
 
 	if (!revs->tree_objects)
@@ -85,7 +85,7 @@ static void process_tree(struct rev_info *revs,
 	me.elem = name;
 	me.elem_len = strlen(name);
 
-	if (!all_interesting) {
+	if (!match) {
 		strbuf_addstr(base, name);
 		if (base->len)
 			strbuf_addch(base, '/');
@@ -94,17 +94,13 @@ static void process_tree(struct rev_info *revs,
 	init_tree_desc(&desc, tree->buffer, tree->size);
 
 	while (tree_entry(&desc, &entry)) {
-		if (!all_interesting) {
-			int showit = tree_entry_interesting(&entry,
-							    base, 0,
-							    &revs->diffopt.pathspec);
-
-			if (showit < 0)
+		if (match != 2) {
+			match = tree_entry_interesting(&entry, base, 0,
+						       &revs->diffopt.pathspec);
+			if (match < 0)
 				break;
-			else if (!showit)
+			if (match == 0)
 				continue;
-			else if (showit == 2)
-				all_interesting = 1;
 		}
 
 		if (S_ISDIR(entry.mode))
diff --git a/tree-diff.c b/tree-diff.c
index 3954281..f291069 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -65,23 +65,17 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2,
 static void show_tree(struct diff_options *opt, const char *prefix,
 		      struct tree_desc *desc, struct strbuf *base)
 {
-	int all_interesting = 0;
-	while (desc->size) {
-		int show;
-
-		if (all_interesting)
-			show = 1;
-		else {
-			show = tree_entry_interesting(&desc->entry, base, 0,
-						      &opt->pathspec);
-			if (show == 2)
-				all_interesting = 1;
+	int match = 0;
+	for (; desc->size; update_tree_entry(desc)) {
+		if (match != 2) {
+			match = tree_entry_interesting(&desc->entry, base, 0,
+						       &opt->pathspec);
+			if (match < 0)
+				break;
+			if (match == 0)
+				continue;
 		}
-		if (show < 0)
-			break;
-		if (show)
-			show_entry(opt, prefix, desc, base);
-		update_tree_entry(desc);
+		show_entry(opt, prefix, desc, base);
 	}
 }
 
@@ -121,20 +115,16 @@ static void show_entry(struct diff_options *opt, const char *prefix,
 }
 
 static void skip_uninteresting(struct tree_desc *t, struct strbuf *base,
-			       struct diff_options *opt, int *all_interesting)
+			       struct diff_options *opt, int *match)
 {
 	while (t->size) {
-		int show = tree_entry_interesting(&t->entry, base, 0, &opt->pathspec);
-		if (show == 2)
-			*all_interesting = 1;
-		if (!show) {
-			update_tree_entry(t);
-			continue;
+		*match = tree_entry_interesting(&t->entry, base, 0, &opt->pathspec);
+		if (*match) {
+			if (*match < 0)
+				t->size = 0;
+			break;
 		}
-		/* Skip it all? */
-		if (show < 0)
-			t->size = 0;
-		return;
+		update_tree_entry(t);
 	}
 }
 
@@ -143,8 +133,7 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
 {
 	struct strbuf base;
 	int baselen = strlen(base_str);
-	int all_t1_interesting = 0;
-	int all_t2_interesting = 0;
+	int t1_match = 0, t2_match = 0;
 
 	/* Enable recursion indefinitely */
 	opt->pathspec.recursive = DIFF_OPT_TST(opt, RECURSIVE);
@@ -158,10 +147,8 @@ int diff_tree(struct tree_desc *t1, struct tree_desc *t2,
 		    DIFF_OPT_TST(opt, HAS_CHANGES))
 			break;
 		if (opt->pathspec.nr) {
-			if (!all_t1_interesting)
-				skip_uninteresting(t1, &base, opt, &all_t1_interesting);
-			if (!all_t2_interesting)
-				skip_uninteresting(t2, &base, opt, &all_t2_interesting);
+			skip_uninteresting(t1, &base, opt, &t1_match);
+			skip_uninteresting(t2, &base, opt, &t2_match);
 		}
 		if (!t1->size) {
 			if (!t2->size)
-- 
1.7.4.74.g639db

Re: Relative ls-files

[Junio C Hamano]

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:

> On Thu, Mar 24, 2011 at 5:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Wouldn't it be more sensible to add a bit ("is this a glob or are we
>> expected to match it literally") to each element in the pathspec array,
>> and pass a pathspec whose elements are all marked as "literal" down from
>> ls-tree?
>>
>> The internal matcher (and traversal optimizer) would need to become aware
>> of "not a glob" match (and possibly different kinds of matches like "**/"
>> support) sooner or later, and when that happens you would need something
>> like the above on the caller side in ls-tree.
>
> I think it's possible even now. fnmatch() is only used if
> pathspec_item.has_wildcard is true. So ls-tree.c only needs to clear
> this field for all pathspecs and they will be treated literally. Let
> me try..

Thanks, that would be good.

Re: Relative ls-files

[Martin Langhoff]

On Wed, Mar 23, 2011 at 10:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Because --with-tree nor ls-files is the tool that was designed for.
>
> If you want to find out about a branch, why aren't you using "ls-tree -r"?

Thanks! Tab-completion wasn't showing it, so I never saw it.

happy happy



m
-- 
 martin.langhoff@gmail.com
 martin@laptop.org -- Software Architect - OLPC
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

Re: Relative ls-files

[Junio C Hamano]

Martin Langhoff <martin.langhoff@gmail.com> writes:

> On Wed, Mar 23, 2011 at 10:09 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Because --with-tree nor ls-files is the tool that was designed for.
>>
>> If you want to find out about a branch, why aren't you using "ls-tree -r"?
>
> Thanks! Tab-completion wasn't showing it, so I never saw it.

Going back to your original (which I suspect may be different from John
Tapsell's original):

 - is this file in branch foo?
 - has this file moved in branch foo?
 - what files with extension .zoo exist in branch foo?

I am not sure what the difference between the first two question, but the
most direct way the scripted Porcelains do to answer these questions is:

	git rev-parse --verify "foo:$the_path"

The third question can be answered by listing paths in branch 'foo' and
looking for .zoo, like so:

	git ls-tree -r --name-only --full-tree foo | grep '\.zoo$'

Re: Relative ls-files

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Martin Langhoff <martin.langhoff@gmail.com> writes:
>
>> and the answer is git ls-files --with-tree=<treeish> | grep <regex> .
>> But the --with-tree param is so counterintuitive to me that I read the
>> manpage, everytime.
>
> Because --with-tree nor ls-files is the tool that was designed for.
>
> If you want to find out about a branch, why aren't you using "ls-tree -r"?

It obviously needs a side note to answer "what is 'ls-files --with-tree'
for, then?", so here is such a note.

The --with-tree option was invented for the sole purpose of helping "git
commit -- $paths" back when "commit" was still a scripted Porcelain.  We
wanted to make sure that the $paths given from the command line made sense
(we wanted to diagnose e.g. "git commit Mkaefile" as a typo).

The obvious plumbing to check if the given pathspec matches what are known
to git (i.e. in the index) is:

	git ls-files --error-unmatch -- $paths

but there is a catch.  If you are about to commit a removal of a path,
i.e.

	$ rm -f generated-file.c
	$ git update-index --remove generated-file.c
        $ git commit -m "Do not keep a generated file in SCM" generated-file.c

the above ls-files would report that the pathspec "generated-file.c" does
not match, as that path is no longer in the index.

To make the "ls-files --error-unmatch" useful for this check, we use:

	git ls-files --error-unmatch --with-tree=HEAD -- $paths

to overlay the contents of the tree on top of what is already in the index
and then determine what paths are "known" to git at that point.

So your use of "git ls-files --with-tree=<unrelated branch>" has been
giving results that you didn't want to see, as the set of paths in your
index that is based on your current branch most likely are different from
what is in that unrelated branch.

I don't think there is any in-tree users of --with-tree option anymore,
other than the tests, so it probably is not even worth thinking about
removing it and replacing it with a --with-head-tree (as using any commit
other than HEAD makes little sense) option.

Re: Relative ls-files

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

>>> What's wrong with --relative  or something?
>
> ??
>
> ls-files already lists files relative to cwd. What you need is a way to
> specify that you want all files, not just those beneath cwd.

Exactly.

>>> I've never liked magical strange notations.  ":" already has a meaning.
>> 
>> +1
>
> I think you've both been missing out on the discussions about
> --full-tree and refspec notation for that...

I like this answer.  Don't pay any much attention +1/-1 from people who
are clueless to the topic, but do pay attention to them as persons by
showing that there are things for them to read and learn if they want to
discuss the topic.  A URL or two to the recent thread may have been nicer.

> Of course ":" has a meaning. "git show HEAD:" lists all files at the
> top-level of the tree at HEAD. That is exactly why ":" has been
> suggested to denote the top-level for pathspecs also. Note that this is
> much more general than just an option for ls-files.

Talking about the "pathspec magic", I think in addition to "this is
relative to root", we at least would need "this is literal, don't apply
globbing" magic.  I am afraid that using ':' as the sole "magic for
from-root" as you advocate may turn out to be too short-sighted and would
hurt us in the future.

It may be more extensible if we used a common "magic introducer" at the
beginning, an character unlikely to collide with any pathspec existing
scripts and people use, and then have additional "what magic I want"
signature immediately following that introducer letter in these magic
filespecs.

Off the to of my head, "/" may be a perfect choice for "here is a magic"
introducer, as we never give absolute paths on the fiesystem from the root
to git [*1*].

Then, even when you are in a subdirectory somewhere, this:

    $ git ls-tree pu -- '/:*.h' "/!$path"

would list paths in the pu branch, limiting its output to any "*.h" header
file, and a path that matches exactly with whatever is in $path variable
without globbing within the current level.  The above uses ':' as "this is
relative to root" (your favorite, whose rationale being that HEAD:Makefile
names Makefile at the top) and '!' (randombly chosen without any rationale
other than being "NOT" and without any association with "glob", which make
it a bad choice of the letter) as "this is NOT a glob".

If we use ':' as the magic introducer, '::*.h" may be more logical if you
follow your "HEAD:Makefile names Makefile at the top" analogy; ':/' breaks
the analogy (the first colon is a general introducer and does not talk
anything about what kind of magic; the second letter should be what magic
it is about, and should use ':' from HEAD:Makefile analogy).

I personally think "//*.h" looks a lot more natural than either ':*.h' or
':/*.h' that have been suggested on the command line, so I am tempted to
suggest using '/' (the first slash in the example) as the magic introducer
and using '/' (the second) as the 'from-root' magic signature.


[Footnote]

*1* I am ignoring "--no-index" variants that are not strictly part of git
here, but I suspect this might also affect "git add /path/from/root" which
I think we should deal with somehow.

Re: Relative ls-files

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 23.03.2011 16:42:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
>>>> What's wrong with --relative  or something?
>>
>> ??
>>
>> ls-files already lists files relative to cwd. What you need is a way to
>> specify that you want all files, not just those beneath cwd.
> 
> Exactly.
> 
>>>> I've never liked magical strange notations.  ":" already has a meaning.
>>>
>>> +1
>>
>> I think you've both been missing out on the discussions about
>> --full-tree and refspec notation for that...
> 
> I like this answer.  Don't pay any much attention +1/-1 from people who
> are clueless to the topic, but do pay attention to them as persons by
> showing that there are things for them to read and learn if they want to
> discuss the topic.  A URL or two to the recent thread may have been nicer.
> 
>> Of course ":" has a meaning. "git show HEAD:" lists all files at the
>> top-level of the tree at HEAD. That is exactly why ":" has been
>> suggested to denote the top-level for pathspecs also. Note that this is
>> much more general than just an option for ls-files.
> 
> Talking about the "pathspec magic", I think in addition to "this is
> relative to root", we at least would need "this is literal, don't apply
> globbing" magic.  I am afraid that using ':' as the sole "magic for
> from-root" as you advocate may turn out to be too short-sighted and would
> hurt us in the future.

I admit I haven't thought much about globbing. Does my current POC patch
affect globbing in the way you want to avoid?

> 
> It may be more extensible if we used a common "magic introducer" at the
> beginning, an character unlikely to collide with any pathspec existing
> scripts and people use, and then have additional "what magic I want"
> signature immediately following that introducer letter in these magic
> filespecs.
> 
> Off the to of my head, "/" may be a perfect choice for "here is a magic"
> introducer, as we never give absolute paths on the fiesystem from the root
> to git [*1*].
> 
> Then, even when you are in a subdirectory somewhere, this:
> 
>     $ git ls-tree pu -- '/:*.h' "/!$path"
> 
> would list paths in the pu branch, limiting its output to any "*.h" header
> file, and a path that matches exactly with whatever is in $path variable
> without globbing within the current level.  The above uses ':' as "this is
> relative to root" (your favorite, whose rationale being that HEAD:Makefile
> names Makefile at the top) and '!' (randombly chosen without any rationale
> other than being "NOT" and without any association with "glob", which make
> it a bad choice of the letter) as "this is NOT a glob".
> 
> If we use ':' as the magic introducer, '::*.h" may be more logical if you
> follow your "HEAD:Makefile names Makefile at the top" analogy; ':/' breaks
> the analogy (the first colon is a general introducer and does not talk
> anything about what kind of magic; the second letter should be what magic
> it is about, and should use ':' from HEAD:Makefile analogy).

Mainly I want "few characters" (up to 2) and "good analogy". "/" and
"//" have obvious good analogies to file system usage.

> 
> I personally think "//*.h" looks a lot more natural than either ':*.h' or

With current git:

(cd Documentation && git show :Makefile)

Which Makefile do you get? That is why I call the ":" proposal natural.
I know, "git show Makefile" does not give the local one, but

git show :./Makefile

does. Given this, I would find ":/Makefile" very natural as well.

> ':/*.h' that have been suggested on the command line, so I am tempted to
> suggest using '/' (the first slash in the example) as the magic introducer
> and using '/' (the second) as the 'from-root' magic signature.

I'm afraid (but not sure) that leading "/" would create problems for Git
on Win! ":/" is fine according to the experts.

Michael

Re: Relative ls-files

[Nguyen Thai Ngoc Duy]

On Wed, Mar 23, 2011 at 10:57 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
>> ':/*.h' that have been suggested on the command line, so I am tempted to
>> suggest using '/' (the first slash in the example) as the magic introducer
>> and using '/' (the second) as the 'from-root' magic signature.
>
> I'm afraid (but not sure) that leading "/" would create problems for Git
> on Win! ":/" is fine according to the experts.

'/' is treated more or less just like '\' on Windows so it would not
be a big deal. The 'git add /foo/bar' issue is more interesting, imo.
-- 
Duy