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

Subsystems: the rest

DORMANTno replies

2 messages, 2 authors, 2016-06-15 · open the first message on its own page

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

From: Junio C Hamano <hidden>
Date: 2016-06-15 22:41:59

quoted
quoted
quoted
quoted
"LT" == Linus Torvalds [off-list ref] 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 <redacted>
---

 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: handle trailing slashes in the pathspec properly.

From: Linus Torvalds <torvalds@osdl.org>
Date: 2016-06-15 22:41:59


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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help