Re: [PATCH] allow pathspec to end with a slash

Re: [PATCH] allow pathspec to end with a slash

[Linus Torvalds]

Junio, 
 pathspec is still subtly broken.

Just removing the slash at the end is not the right thing to do, since 
that means that

	git-diff-files drivers/char/

will trigger on a _file_ "drivers/char", even though the spec makes it 
unambiguous that the user only wants to see stuff under that directory.

Again, git-diff-tree gets this right (not to say that it always did - I
had to fix "interesting()" several times, exactly because it's not that 
simple).

		Linus

Re: [PATCH] allow pathspec to end with a slash

[Junio C Hamano]

Do you want the same for git-ls-tree as well?  That is, when
you have drivers/char as a blob in the tree, should

	git-ls-tree $tree drivers/char/

show nothing?

[PATCH 1/2] diff: consolidate test helper script pieces.

[Junio C Hamano]

There were duplicate script pieces to help comparing diff
output, which this patch consolidates into the t/diff-lib.sh
library.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

*** The next one is to fix the pathspec, whose test relies on
*** this cleanup.

 t/diff-lib.sh                 |   35 +++++++++++++++++++++++++++++++++++
 t/t4003-diff-rename-1.sh      |    9 +--------
 t/t4005-diff-rename-2.sh      |   23 +----------------------
 t/t4007-rename-3.sh           |   15 +--------------
 t/t4008-diff-break-rewrite.sh |   15 +--------------
 t/t4009-diff-rename-4.sh      |   29 ++++-------------------------
 6 files changed, 43 insertions(+), 83 deletions(-)

diff --git a/t/diff-lib.sh b/t/diff-lib.sh
new file mode 100644
--- /dev/null
+++ b/t/diff-lib.sh
@@ -0,0 +1,35 @@
+:
+
+_x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
+_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
+sanitize_diff_raw='/^:/s/ '"$_x40"' '"$_x40"' \([A-Z]\)[0-9]*	/ X X \1#	/'
+compare_diff_raw () {
+    # When heuristics are improved, the score numbers would change.
+    # Ignore them while comparing.
+    # Also we do not check SHA1 hash generation in this test, which
+    # is a job for t0000-basic.sh
+
+    sed -e "$sanitize_diff_raw" <"$1" >.tmp-1
+    sed -e "$sanitize_diff_raw" <"$2" >.tmp-2
+    diff -u .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
+}
+
+sanitize_diff_raw_z='/^:/s/ '"$_x40"' '"$_x40"' \([A-Z]\)[0-9]*$/ X X \1#/'
+compare_diff_raw_z () {
+    # When heuristics are improved, the score numbers would change.
+    # Ignore them while comparing.
+    # Also we do not check SHA1 hash generation in this test, which
+    # is a job for t0000-basic.sh
+
+    tr '\0' '\012' <"$1" | sed -e "$sanitize_diff_raw_z" >.tmp-1
+    tr '\0' '\012' <"$2" | sed -e "$sanitize_diff_raw_z" >.tmp-2
+    diff -u .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
+}
+
+compare_diff_patch () {
+    # When heuristics are improved, the score numbers would change.
+    # Ignore them while comparing.
+    sed -e '/^[dis]*imilarity index [0-9]*%$/d' <"$1" >.tmp-1
+    sed -e '/^[dis]*imilarity index [0-9]*%$/d' <"$2" >.tmp-2
+    diff -u .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
+}
diff --git a/t/t4003-diff-rename-1.sh b/t/t4003-diff-rename-1.sh
--- a/t/t4003-diff-rename-1.sh
+++ b/t/t4003-diff-rename-1.sh
@@ -7,14 +7,7 @@ test_description='More rename detection
 
 '
 . ./test-lib.sh
-
-compare_diff_patch () {
-    # When heuristics are improved, the score numbers would change.
-    # Ignore them while comparing.
-    sed -e '/^similarity index [0-9]*%$/d' <"$1" >.tmp-1
-    sed -e '/^similarity index [0-9]*%$/d' <"$2" >.tmp-2
-    diff -u .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
-}
+. ../diff-lib.sh ;# test-lib chdir's into trash
 
 test_expect_success \
     'prepare reference tree' \
diff --git a/t/t4005-diff-rename-2.sh b/t/t4005-diff-rename-2.sh
--- a/t/t4005-diff-rename-2.sh
+++ b/t/t4005-diff-rename-2.sh
@@ -7,28 +7,7 @@ test_description='Same rename detection 
 
 '
 . ./test-lib.sh
-
-_x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
-_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
-sanitize_diff_raw='s/ '"$_x40"' '"$_x40"' \([A-Z]\)[0-9]*	/ X X \1#	/'
-compare_diff_raw () {
-    # When heuristics are improved, the score numbers would change.
-    # Ignore them while comparing.
-    # Also we do not check SHA1 hash generation in this test, which
-    # is a job for t0000-basic.sh
-
-    sed -e "$sanitize_diff_raw" <"$1" >.tmp-1
-    sed -e "$sanitize_diff_raw" <"$2" >.tmp-2
-    diff -u .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
-}
-
-compare_diff_patch () {
-    # When heuristics are improved, the score numbers would change.
-    # Ignore them while comparing.
-    sed -e '/^similarity index [0-9]*%$/d' <"$1" >.tmp-1
-    sed -e '/^similarity index [0-9]*%$/d' <"$2" >.tmp-2
-    diff -u .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
-}
+. ../diff-lib.sh ;# test-lib chdir's into trash
 
 test_expect_success \
     'prepare reference tree' \
diff --git a/t/t4007-rename-3.sh b/t/t4007-rename-3.sh
--- a/t/t4007-rename-3.sh
+++ b/t/t4007-rename-3.sh
@@ -7,20 +7,7 @@ test_description='Rename interaction wit
 
 '
 . ./test-lib.sh
-
-_x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
-_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
-sanitize_diff_raw='s/ '"$_x40"' '"$_x40"' \([A-Z]\)[0-9]*	/ X X \1#	/'
-compare_diff_raw () {
-    # When heuristics are improved, the score numbers would change.
-    # Ignore them while comparing.
-    # Also we do not check SHA1 hash generation in this test, which
-    # is a job for t0000-basic.sh
-
-    sed -e "$sanitize_diff_raw" <"$1" >.tmp-1
-    sed -e "$sanitize_diff_raw" <"$2" >.tmp-2
-    diff -u .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
-}
+. ../diff-lib.sh ;# test-lib chdir's into trash
 
 test_expect_success \
     'prepare reference tree' \
diff --git a/t/t4008-diff-break-rewrite.sh b/t/t4008-diff-break-rewrite.sh
--- a/t/t4008-diff-break-rewrite.sh
+++ b/t/t4008-diff-break-rewrite.sh
@@ -22,20 +22,7 @@ four changes in total.
 Further, with -B and -M together, these should turn into two renames.
 '
 . ./test-lib.sh
-
-_x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
-_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
-sanitize_diff_raw='s/ '"$_x40"' '"$_x40"' \([CDNR]\)[0-9]*	/ X X \1#	/'
-compare_diff_raw () {
-    # When heuristics are improved, the score numbers would change.
-    # Ignore them while comparing.
-    # Also we do not check SHA1 hash generation in this test, which
-    # is a job for t0000-basic.sh
-
-    sed -e "$sanitize_diff_raw" <"$1" >.tmp-1
-    sed -e "$sanitize_diff_raw" <"$2" >.tmp-2
-    diff -u .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
-}
+. ../diff-lib.sh ;# test-lib chdir's into trash
 
 test_expect_success \
     setup \
diff --git a/t/t4009-diff-rename-4.sh b/t/t4009-diff-rename-4.sh
--- a/t/t4009-diff-rename-4.sh
+++ b/t/t4009-diff-rename-4.sh
@@ -7,28 +7,7 @@ test_description='Same rename detection 
 
 '
 . ./test-lib.sh
-
-_x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
-_x40="$_x40$_x40$_x40$_x40$_x40$_x40$_x40$_x40"
-sanitize_diff_raw='/^:/s/ '"$_x40"' '"$_x40"' \([A-Z]\)[0-9]*$/ X X \1#/'
-compare_diff_raw () {
-    # When heuristics are improved, the score numbers would change.
-    # Ignore them while comparing.
-    # Also we do not check SHA1 hash generation in this test, which
-    # is a job for t0000-basic.sh
-
-    tr '\0' '\012' <"$1" | sed -e "$sanitize_diff_raw" >.tmp-1
-    tr '\0' '\012' <"$2" | sed -e "$sanitize_diff_raw" >.tmp-2
-    diff -u .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
-}
-
-compare_diff_patch () {
-    # When heuristics are improved, the score numbers would change.
-    # Ignore them while comparing.
-    sed -e '/^similarity index [0-9]*%$/d' <"$1" >.tmp-1
-    sed -e '/^similarity index [0-9]*%$/d' <"$2" >.tmp-2
-    diff -u .tmp-1 .tmp-2 && rm -f .tmp-1 .tmp-2
-}
+. ../diff-lib.sh ;# test-lib chdir's into trash
 
 test_expect_success \
     'prepare reference tree' \
@@ -63,7 +42,7 @@ EOF
 
 test_expect_success \
     'validate output from rename/copy detection (#1)' \
-    'compare_diff_raw current expected'
+    'compare_diff_raw_z current expected'
 
 # make sure diff-helper can grok it.
 mv current diff-raw
@@ -120,7 +99,7 @@ EOF
 
 test_expect_success \
     'validate output from rename/copy detection (#2)' \
-    'compare_diff_raw current expected'
+    'compare_diff_raw_z current expected'
 
 # make sure diff-helper can grok it.
 mv current diff-raw
@@ -173,7 +152,7 @@ EOF
 
 test_expect_success \
     'validate output from rename/copy detection (#3)' \
-    'compare_diff_raw current expected'
+    'compare_diff_raw_z current expected'
 
 # make sure diff-helper can grok it.
 mv current diff-raw
------------

[PATCH 2/2] diff: Fix trailing slash handling.

[Junio C Hamano]

When limiting the world to "drivers/char/", we should not
consider "drivers/char" which is not a directory.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 t/t4010-diff-pathspec.sh |   65 ++++++++++++++++++++++++++++++++++++++++++++++
 diffcore-pathspec.c      |   60 +++++++++++++++++++++++++++++++++++++------
 2 files changed, 117 insertions(+), 8 deletions(-)

diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
new file mode 100644
--- /dev/null
+++ b/t/t4010-diff-pathspec.sh
@@ -0,0 +1,65 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+test_description='Pathspec restrictions
+
+Prepare:
+        file0
+        path1/file1
+'
+. ./test-lib.sh
+. ../diff-lib.sh ;# test-lib chdir's into trash
+
+test_expect_success \
+    setup \
+    'echo frotz >file0 &&
+     mkdir path1 &&
+     echo rezrov >path1/file1 &&
+     git-update-cache --add file0 path1/file1 &&
+     tree=`git-write-tree` &&
+     echo "$tree" &&
+     echo nitfol >file0 &&
+     echo yomin >path1/file1 &&
+     git-update-cache file0 path1/file1' 
+
+cat >expected <<\EOF
+EOF
+test_expect_success \
+    'limit to path should show nothing' \
+    'git-diff-cache --cached $tree path >current &&
+     compare_diff_raw current expected'
+
+cat >expected <<\EOF
+:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M	path1/file1
+EOF
+test_expect_success \
+    'limit to path1 should show path1/file1' \
+    'git-diff-cache --cached $tree path1 >current &&
+     compare_diff_raw current expected'
+
+cat >expected <<\EOF
+:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M	path1/file1
+EOF
+test_expect_success \
+    'limit to path1/ should show path1/file1' \
+    'git-diff-cache --cached $tree path1/ >current &&
+     compare_diff_raw current expected'
+
+cat >expected <<\EOF
+:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M	file0
+EOF
+test_expect_success \
+    'limit to file0 should show file0' \
+    'git-diff-cache --cached $tree file0 >current &&
+     compare_diff_raw current expected'
+
+cat >expected <<\EOF
+EOF
+test_expect_success \
+    'limit to file0/ should emit nothing.' \
+    'git-diff-cache --cached $tree file0/ >current &&
+     compare_diff_raw current expected'
+
+test_done
diff --git a/diffcore-pathspec.c b/diffcore-pathspec.c
--- a/diffcore-pathspec.c
+++ b/diffcore-pathspec.c
@@ -8,9 +8,13 @@
 struct path_spec {
 	const char *spec;
 	int len;
+	int reject_non_tree;
 };
 
-static int matches_pathspec(const char *name, struct path_spec *s, int cnt)
+static int matches_pathspec(const char *name,
+			    int obviously_non_tree,
+			    struct path_spec *s,
+			    int cnt)
 {
 	int i;
 	int namelen;
@@ -21,10 +25,25 @@ static int matches_pathspec(const char *
 	namelen = strlen(name);
 	for (i = 0; i < cnt; i++) {
 		int len = s[i].len;
-		if (! strncmp(s[i].spec, name, len) &&
-		    len <= namelen &&
-		    (name[len] == 0 || name[len] == '/'))
-			return 1;
+		if (!strncmp(s[i].spec, name, len)) {
+			/* Leading part matches. */
+			if (len == namelen) {
+				/* Exact match: spec "drivers/char/"
+				 * should not match against path
+				 * "drivers/char".
+				 */
+				if (s[i].reject_non_tree && obviously_non_tree)
+					continue;
+				return 1;
+			}
+			else if ((len < namelen) && name[len] == '/') {
+				/* Spec is leading path */
+				return 1;
+			}
+			/* otherwise, it is a false match of spec "abc"
+			 * against path "abcdefg", so we continue.
+			 */
+		}
 	}
 	return 0;
 }
@@ -47,14 +66,39 @@ void diffcore_pathspec(const char **path
 		int l;
 		spec[i].spec = pathspec[i];
 		l = strlen(pathspec[i]);
-		while (l > 0 && pathspec[i][l-1] == '/')
-			l--;
+		if (l > 0 && pathspec[i][l-1] == '/') {
+			spec[i].reject_non_tree = 1;
+			while (l > 0 && pathspec[i][l-1] == '/')
+				l--;
+		}
 		spec[i].len = l;
 	}
 
 	for (i = 0; i < q->nr; i++) {
 		struct diff_filepair *p = q->queue[i];
-		if (matches_pathspec(p->two->path, spec, speccnt))
+		int obviously_non_tree;
+#ifndef DIFF_TREE_CALLS_PATHSPEC
+		/*
+		 * NOTE: This relies on the current behaviour of
+		 * diff-tree which does not use diffcore-pathspec
+		 * and we would not ever see "tree" objects
+		 * in our input.
+		 */
+		obviously_non_tree = 1;
+#else
+		if (DIFF_FILE_VALID(p->two))
+			obviously_non_tree = !S_ISDIR(p->two->mode);
+		else if (DIFF_FILE_VALID(p->one))
+			/* path is being deleted.  is it a tree? */
+			obviously_non_tree = !S_ISDIR(p->one->mode);
+		else
+			/* path is unmerged, which comes from cache
+			 * so it cannot be a tree
+			 */
+			obviously_non_tree = 1;
+#endif
+		if (matches_pathspec(p->two->path, obviously_non_tree,
+				     spec, speccnt))
 			diff_q(&outq, p);
 		else
 			diff_free_filepair(p);
------------

[PATCH] ls-tree: remove trailing slashes properly.

[Junio C Hamano]

A typo prevented trailing slashes from being removed properly.

This fixes the problem that "drivers/char" which is a tree was
not shown when "drivers/char/" is given.

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 t/t3100-ls-tree-restrict.sh |   32 +++++++++++++++++++++++++++-----
 ls-tree.c                   |    2 +-
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/t/t3100-ls-tree-restrict.sh b/t/t3100-ls-tree-restrict.sh
--- a/t/t3100-ls-tree-restrict.sh
+++ b/t/t3100-ls-tree-restrict.sh
@@ -14,7 +14,7 @@ This test runs git-ls-tree with the foll
     path2/baz/b - a file in a directory in a directory
 
 The new path restriction code should do the right thing for path2 and
-path2/baz
+path2/baz.  Also path0/ should snow nothing.
 '
 . ./test-lib.sh
 
@@ -63,7 +63,7 @@ EOF
      test_output'
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path' \
     'git-ls-tree $tree path >current &&
      cat >expected <<\EOF &&
 EOF
@@ -71,7 +71,7 @@ EOF
 
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path1 path0' \
     'git-ls-tree $tree path1 path0 >current &&
      cat >expected <<\EOF &&
 120000 blob X	path1
@@ -80,7 +80,7 @@ EOF
      test_output'
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path2' \
     'git-ls-tree $tree path2 >current &&
      cat >expected <<\EOF &&
 040000 tree X	path2
@@ -91,7 +91,7 @@ EOF
      test_output'
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path2/baz' \
     'git-ls-tree $tree path2/baz >current &&
      cat >expected <<\EOF &&
 040000 tree X	path2/baz
@@ -99,4 +99,26 @@ test_expect_success \
 EOF
      test_output'
 
+test_expect_success \
+    'ls-tree filtered with path2' \
+    'git-ls-tree $tree path2 >current &&
+     cat >expected <<\EOF &&
+040000 tree X	path2
+040000 tree X	path2/baz
+120000 blob X	path2/bazbo
+100644 blob X	path2/foo
+EOF
+     test_output'
+
+test_expect_success \
+    'ls-tree filtered with path2/' \
+    'git-ls-tree $tree path2/ >current &&
+     cat >expected <<\EOF &&
+040000 tree X	path2
+040000 tree X	path2/baz
+120000 blob X	path2/bazbo
+100644 blob X	path2/foo
+EOF
+     test_output'
+
 test_done
diff --git a/ls-tree.c b/ls-tree.c
--- a/ls-tree.c
+++ b/ls-tree.c
@@ -201,7 +201,7 @@ static int list(char **path)
 	int err = 0;
 	for (i = 0; path[i]; i++) {
 		int len = strlen(path[i]);
-		while (0 <= len && path[i][len] == '/')
+		while (0 < len && path[i][len-1] == '/')
 			len--;
 		err = err | list_one(path[i], path[i] + len);
 	}
------------

Re: [PATCH] ls-tree: remove trailing slashes properly.

[Linus Torvalds]

On Tue, 31 May 2005, Junio C Hamano wrote:
>
> A typo prevented trailing slashes from being removed properly.

This is still wrong.

> This fixes the problem that "drivers/char" which is a tree was
> not shown when "drivers/char/" is given.

I sent an earlier email pointing out that removing trailing slashes is 
_incorrect_, because it means that "drivers/char/" will match the _file_ 
"drivers/char", which is wrong.

IOW, the trailing slash should not be removed, it's the _test_ that is 
wrong.

I just checked in a fix for this in diffcore-patchspec.c, I'd hope that 
ls-tree could get it right too. Removing trailing slashes is a bandaid 
that hides one bug by making it appear as a different bug.

		Linus

Re: [PATCH] ls-tree: remove trailing slashes properly.

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> I just checked in a fix for this in diffcore-patchspec.c,
LT> I'd hope that ls-tree could get it right too. Removing
LT> trailing slashes is a bandaid that hides one bug by making
LT> it appear as a different bug.

Yes, I am aware of that problem but have not touched it; I sent
an earlier email about this to you.  While I was waiting for
your response, I found that drivers/char/ does not even match
drivers/char tree, which is what the patch you are responding to
is about.  Will look into it later.

[PATCH] ls-tree: handle trailing slashes in the pathspec properly.

[Junio C Hamano]

>>>>> "LT" == Linus Torvalds <torvalds@osdl.org> writes:

LT> I just checked in a fix for this in diffcore-patchspec.c, I'd hope that 
LT> ls-tree could get it right too. Removing trailing slashes is a bandaid 
LT> that hides one bug by making it appear as a different bug.

I take it to mean that you took my other patch for diffcore-pathspec.

Here is a fixed ls-tree, with a couple of new tests in an
existing test script, to catch this bug.

------------
This fixes one problem while avoiding the same mistake earlier
diffcore-pathspec had:

 - "drivers/char" which is a tree was not shown given
   "drivers/char/".

 - "drivers/char" which is not a tree is not shown given
   "drivers/char/".

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

 ls-tree.c                   |   57 ++++++++++++++++++++++++++-----------------
 t/t3100-ls-tree-restrict.sh |   39 ++++++++++++++++++++++++++----
 2 files changed, 68 insertions(+), 28 deletions(-)

diff --git a/ls-tree.c b/ls-tree.c
--- a/ls-tree.c
+++ b/ls-tree.c
@@ -54,13 +54,22 @@ static int prepare_children(struct tree_
 	return 0;
 }
 
-static struct tree_entry_list *find_entry_0(struct tree_entry_list *elem,
-					    const char *path,
-					    const char *path_end)
+static struct tree_entry_list *find_entry(const char *path,
+					  const char *path_end,
+					  int require_tree)
 {
-	const char *ep;
 	int len;
+	struct tree_entry_list *elem = &root_entry;
+	const char *ep;
 
+	if (path == path_end)
+		/* Special.  This is the root level */
+		return elem;
+
+	/* Find tree element, descending from root, that
+	 * corresponds to the named path, lazily expanding
+	 * the tree if possible.
+	 */
 	while (path < path_end) {
 		if (prepare_children(elem))
 			return NULL;
@@ -81,27 +90,25 @@ static struct tree_entry_list *find_entr
 				break;
 			elem = elem->next;
 		}
-		if (path_end <= ep || !elem)
+		if (!elem)
+			return NULL;
+		if (path_end <= ep) {
+			/* elem matches the specified path.  However,
+			 * if the user said "drivers/char/" and
+			 * elem is "drivers/char", _and_ it is not
+			 * a tree, then we should reject, just like
+			 * "/bin/ls -a ls-tree.c/" says "Not a directory".
+			 */
+			if (require_tree && !elem->directory)
+				return NULL;
 			return elem;
+		}
 		while (*ep == '/' && ep < path_end)
 			ep++;
 		path = ep;
 	}
 	return NULL;
-}
 
-static struct tree_entry_list *find_entry(const char *path,
-					  const char *path_end)
-{
-	/* Find tree element, descending from root, that
-	 * corresponds to the named path, lazily expanding
-	 * the tree if possible.
-	 */
-	if (path == path_end) {
-		/* Special.  This is the root level */
-		return &root_entry;
-	}
-	return find_entry_0(&root_entry, path, path_end);
 }
 
 static void show_entry_name(struct tree_entry_list *e)
@@ -180,10 +187,10 @@ static int show_entry(struct tree_entry_
 	return err;
 }
 
-static int list_one(const char *path, const char *path_end)
+static int list_one(const char *path, const char *path_end, int require_tree)
 {
 	int err = 0;
-	struct tree_entry_list *e = find_entry(path, path_end);
+	struct tree_entry_list *e = find_entry(path, path_end, require_tree);
 	if (!e) {
 		/* traditionally ls-tree does not complain about
 		 * missing path.  We may change this later to match
@@ -201,9 +208,13 @@ static int list(char **path)
 	int err = 0;
 	for (i = 0; path[i]; i++) {
 		int len = strlen(path[i]);
-		while (0 <= len && path[i][len] == '/')
-			len--;
-		err = err | list_one(path[i], path[i] + len);
+		int require_tree = 0;
+		if (0 < len && path[i][len-1] == '/') {
+			require_tree = 1;
+			while (0 < len && path[i][len-1] == '/')
+				len--;
+		}
+		err = err | list_one(path[i], path[i] + len, require_tree);
 	}
 	return err;
 }
diff --git a/t/t3100-ls-tree-restrict.sh b/t/t3100-ls-tree-restrict.sh
--- a/t/t3100-ls-tree-restrict.sh
+++ b/t/t3100-ls-tree-restrict.sh
@@ -14,7 +14,7 @@ This test runs git-ls-tree with the foll
     path2/baz/b - a file in a directory in a directory
 
 The new path restriction code should do the right thing for path2 and
-path2/baz
+path2/baz.  Also path0/ should snow nothing.
 '
 . ./test-lib.sh
 
@@ -63,7 +63,7 @@ EOF
      test_output'
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path' \
     'git-ls-tree $tree path >current &&
      cat >expected <<\EOF &&
 EOF
@@ -71,7 +71,7 @@ EOF
 
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path1 path0' \
     'git-ls-tree $tree path1 path0 >current &&
      cat >expected <<\EOF &&
 120000 blob X	path1
@@ -80,7 +80,14 @@ EOF
      test_output'
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path0/' \
+    'git-ls-tree $tree path0/ >current &&
+     cat >expected <<\EOF &&
+EOF
+     test_output'
+
+test_expect_success \
+    'ls-tree filtered with path2' \
     'git-ls-tree $tree path2 >current &&
      cat >expected <<\EOF &&
 040000 tree X	path2
@@ -91,7 +98,7 @@ EOF
      test_output'
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path2/baz' \
     'git-ls-tree $tree path2/baz >current &&
      cat >expected <<\EOF &&
 040000 tree X	path2/baz
@@ -99,4 +106,26 @@ test_expect_success \
 EOF
      test_output'
 
+test_expect_success \
+    'ls-tree filtered with path2' \
+    'git-ls-tree $tree path2 >current &&
+     cat >expected <<\EOF &&
+040000 tree X	path2
+040000 tree X	path2/baz
+120000 blob X	path2/bazbo
+100644 blob X	path2/foo
+EOF
+     test_output'
+
+test_expect_success \
+    'ls-tree filtered with path2/' \
+    'git-ls-tree $tree path2/ >current &&
+     cat >expected <<\EOF &&
+040000 tree X	path2
+040000 tree X	path2/baz
+120000 blob X	path2/bazbo
+100644 blob X	path2/foo
+EOF
+     test_output'
+
 test_done
------------

Re: [PATCH] ls-tree: remove trailing slashes properly.

[Junio C Hamano]

Should we do the same for git-ls-files?

Re: [PATCH] ls-tree: handle trailing slashes in the pathspec properly.

[Linus Torvalds]

On Tue, 31 May 2005, Junio C Hamano wrote:
> 
> I take it to mean that you took my other patch for diffcore-pathspec.

No, I did my own. Rather than add more code to handle '/' as a special 
case, I just removed it all, and fixed the compare logic.

> Here is a fixed ls-tree, with a couple of new tests in an
> existing test script, to catch this bug.

You seem to think that '/' at the end is a special case, and it really 
shouldn't be. It should just fall out as a natural special case of a 
zero-sized name (which is, btw, the same natural special case that the 
path of "" should have in &root_dir).

For some reason your ls-tree.c logic seems to think that zero-sized names
means "root entry", when the _natural_ thing to do is to pass in the "base
directory", and then a zero-sized name is that base.

IOW, it would make much more sense to have

	list_one(struct tree_entry_list *tree, const char *name)
	{
		const char *slash = strchr(name, '/');
		const char *next;
		int len;

		for (;;) {
			if (!slash) {
				len = strlen(name);
				next = NULL;
			} else {
				len = slash - name;
				next = slash+1;
			}
			newtree = tree;
			if (len)
				newtree = lookup(tree, name, len);
			if (!next)
				break;
			tree = newtree;
			name = next;
		}

		/* Ok, "newtree" is the last component */
		show_entry(newtree);
	}

and then call it with

	list_one(&root_entry, full_path)

and notice how the cases of an empty path "" and "xxx//yy" and "xx/"  
just fall out from the exact same logic - a zero-sized name is the same as
the directory it is in. No special cases for slashes or empty names.

		Linus

[PATCH] ls-tree: handle trailing slashes in the pathspec properly.

[Junio C Hamano]

This fixes the problem with ls-tree which failed to show
"drivers/char" directory when the user asked for "drivers/char/"
from the command line.  At the same time, if "drivers/char" were
a non directory, "drivers/char/" would not show it.  This is
consistent with the way diffcore-pathspec has been recently
fixed.

This adds back the diffcore-pathspec test,dropped when my
earlier diffcore-pathspec fix was rejected. 

Signed-off-by: Junio C Hamano <junkio@cox.net>
---

*** OK, this is the third try (2 1/2nd to be exact).  I love how
*** you always turn out to be right ;-).  BTW could you limit
*** git-apply --stat output to 79 cols not 80?

 ls-tree.c                   |   94 ++++++++++++++++++++++----------------------
 t/t3100-ls-tree-restrict.sh |   39 ++++++++++++++++--
 t/t4010-diff-pathspec.sh    |   65 ++++++++++++++++++++++++++++++
3 files changed, 146 insertions(+), 52 deletions(-)

diff --git a/ls-tree.c b/ls-tree.c
--- a/ls-tree.c
+++ b/ls-tree.c
@@ -54,54 +54,58 @@ static int prepare_children(struct tree_
 	return 0;
 }
 
-static struct tree_entry_list *find_entry_0(struct tree_entry_list *elem,
-					    const char *path,
-					    const char *path_end)
+static struct tree_entry_list *find_entry(const char *path)
 {
-	const char *ep;
+	const char *next, *slash;
 	int len;
+	struct tree_entry_list *elem = &root_entry;
 
-	while (path < path_end) {
-		if (prepare_children(elem))
-			return NULL;
+	/* Find tree element, descending from root, that
+	 * corresponds to the named path, lazily expanding
+	 * the tree if possible.
+	 */
 
-		/* In elem->tree->entries, find the one that has name
-		 * that matches what is between path and ep.
+	while (path) {
+		/* The fact we still have path means that the caller
+		 * wants us to make sure that elem at this point is a
+		 * directory, and possibly descend into it.  Even what
+		 * is left is just trailing slashes, we loop back to
+		 * here, and this call to prepare_children() will
+		 * catch elem not being a tree.  Nice.
 		 */
-		elem = elem->item.tree->entries;
+		if (prepare_children(elem))
+			return NULL;
 
-		ep = strchr(path, '/');
-		if (!ep || path_end <= ep)
-			ep = path_end;
-		len = ep - path;
-
-		while (elem) {
-			if ((strlen(elem->name) == len) &&
-			    !strncmp(elem->name, path, len))
-				break;
-			elem = elem->next;
+		slash = strchr(path, '/');
+		if (!slash) {
+			len = strlen(path);
+			next = 0;
+		}
+		else {
+			next = slash + 1;
+			len = slash - path;
 		}
-		if (path_end <= ep || !elem)
-			return elem;
-		while (*ep == '/' && ep < path_end)
-			ep++;
-		path = ep;
+		if (len) {
+			/* (len == 0) if the original path was "drivers/char/"
+			 * and we have run already two rounds, having elem
+			 * pointing at the drivers/char directory.
+			 */
+			elem = elem->item.tree->entries;
+			while (elem) {
+				if ((strlen(elem->name) == len) &&
+				    !strncmp(elem->name, path, len)) {
+					/* found */
+					break;
+				}
+				elem = elem->next;
+			}
+			if (!elem)
+				return NULL;
+		}
+		path = next;
 	}
-	return NULL;
-}
 
-static struct tree_entry_list *find_entry(const char *path,
-					  const char *path_end)
-{
-	/* Find tree element, descending from root, that
-	 * corresponds to the named path, lazily expanding
-	 * the tree if possible.
-	 */
-	if (path == path_end) {
-		/* Special.  This is the root level */
-		return &root_entry;
-	}
-	return find_entry_0(&root_entry, path, path_end);
+	return elem;
 }
 
 static void show_entry_name(struct tree_entry_list *e)
@@ -180,10 +184,10 @@ static int show_entry(struct tree_entry_
 	return err;
 }
 
-static int list_one(const char *path, const char *path_end)
+static int list_one(const char *path)
 {
 	int err = 0;
-	struct tree_entry_list *e = find_entry(path, path_end);
+	struct tree_entry_list *e = find_entry(path);
 	if (!e) {
 		/* traditionally ls-tree does not complain about
 		 * missing path.  We may change this later to match
@@ -199,12 +203,8 @@ static int list(char **path)
 {
 	int i;
 	int err = 0;
-	for (i = 0; path[i]; i++) {
-		int len = strlen(path[i]);
-		while (0 <= len && path[i][len] == '/')
-			len--;
-		err = err | list_one(path[i], path[i] + len);
-	}
+	for (i = 0; path[i]; i++)
+		err = err | list_one(path[i]);
 	return err;
 }
 
diff --git a/t/t3100-ls-tree-restrict.sh b/t/t3100-ls-tree-restrict.sh
--- a/t/t3100-ls-tree-restrict.sh
+++ b/t/t3100-ls-tree-restrict.sh
@@ -14,7 +14,7 @@ This test runs git-ls-tree with the foll
     path2/baz/b - a file in a directory in a directory
 
 The new path restriction code should do the right thing for path2 and
-path2/baz
+path2/baz.  Also path0/ should snow nothing.
 '
 . ./test-lib.sh
 
@@ -63,7 +63,7 @@ EOF
      test_output'
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path' \
     'git-ls-tree $tree path >current &&
      cat >expected <<\EOF &&
 EOF
@@ -71,7 +71,7 @@ EOF
 
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path1 path0' \
     'git-ls-tree $tree path1 path0 >current &&
      cat >expected <<\EOF &&
 120000 blob X	path1
@@ -80,7 +80,14 @@ EOF
      test_output'
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path0/' \
+    'git-ls-tree $tree path0/ >current &&
+     cat >expected <<\EOF &&
+EOF
+     test_output'
+
+test_expect_success \
+    'ls-tree filtered with path2' \
     'git-ls-tree $tree path2 >current &&
      cat >expected <<\EOF &&
 040000 tree X	path2
@@ -91,7 +98,7 @@ EOF
      test_output'
 
 test_expect_success \
-    'ls-tree filtered' \
+    'ls-tree filtered with path2/baz' \
     'git-ls-tree $tree path2/baz >current &&
      cat >expected <<\EOF &&
 040000 tree X	path2/baz
@@ -99,4 +106,26 @@ test_expect_success \
 EOF
      test_output'
 
+test_expect_success \
+    'ls-tree filtered with path2' \
+    'git-ls-tree $tree path2 >current &&
+     cat >expected <<\EOF &&
+040000 tree X	path2
+040000 tree X	path2/baz
+120000 blob X	path2/bazbo
+100644 blob X	path2/foo
+EOF
+     test_output'
+
+test_expect_success \
+    'ls-tree filtered with path2/' \
+    'git-ls-tree $tree path2/ >current &&
+     cat >expected <<\EOF &&
+040000 tree X	path2
+040000 tree X	path2/baz
+120000 blob X	path2/bazbo
+100644 blob X	path2/foo
+EOF
+     test_output'
+
 test_done
diff --git a/t/t4010-diff-pathspec.sh b/t/t4010-diff-pathspec.sh
new file mode 100644
--- /dev/null
+++ b/t/t4010-diff-pathspec.sh
@@ -0,0 +1,65 @@
+#!/bin/sh
+#
+# Copyright (c) 2005 Junio C Hamano
+#
+
+test_description='Pathspec restrictions
+
+Prepare:
+        file0
+        path1/file1
+'
+. ./test-lib.sh
+. ../diff-lib.sh ;# test-lib chdir's into trash
+
+test_expect_success \
+    setup \
+    'echo frotz >file0 &&
+     mkdir path1 &&
+     echo rezrov >path1/file1 &&
+     git-update-cache --add file0 path1/file1 &&
+     tree=`git-write-tree` &&
+     echo "$tree" &&
+     echo nitfol >file0 &&
+     echo yomin >path1/file1 &&
+     git-update-cache file0 path1/file1' 
+
+cat >expected <<\EOF
+EOF
+test_expect_success \
+    'limit to path should show nothing' \
+    'git-diff-cache --cached $tree path >current &&
+     compare_diff_raw current expected'
+
+cat >expected <<\EOF
+:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M	path1/file1
+EOF
+test_expect_success \
+    'limit to path1 should show path1/file1' \
+    'git-diff-cache --cached $tree path1 >current &&
+     compare_diff_raw current expected'
+
+cat >expected <<\EOF
+:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M	path1/file1
+EOF
+test_expect_success \
+    'limit to path1/ should show path1/file1' \
+    'git-diff-cache --cached $tree path1/ >current &&
+     compare_diff_raw current expected'
+
+cat >expected <<\EOF
+:100644 100644 766498d93a4b06057a8e49d23f4068f1170ff38f 0a41e115ab61be0328a19b29f18cdcb49338d516 M	file0
+EOF
+test_expect_success \
+    'limit to file0 should show file0' \
+    'git-diff-cache --cached $tree file0 >current &&
+     compare_diff_raw current expected'
+
+cat >expected <<\EOF
+EOF
+test_expect_success \
+    'limit to file0/ should emit nothing.' \
+    'git-diff-cache --cached $tree file0/ >current &&
+     compare_diff_raw current expected'
+
+test_done
------------

Re: [PATCH] ls-tree: handle trailing slashes in the pathspec properly.

[Linus Torvalds]

On Tue, 31 May 2005, Junio C Hamano wrote:
> 
> BTW could you limit git-apply --stat output to 79 cols not 80?

Hmm, I thought I already did, but it turns out it could round up both 
add/del, causing my 79 to be 80.

I think I fixed it.

		Linus