threads / patch / 776
Subject: Re: [PATCH] allow pathspec to end with a slash
Linus TorvaldsJunio, 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
Junio C HamanoDo 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?
Junio C Hamano[PATCH 1/2] diff: consolidate test helper script pieces.
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(-)
t/diff-lib.sh, t/t4003-diff-rename-1.sh, t/t4005-diff-rename-2.sh, t/t4007-rename-3.sh, t/t4008-diff-break-rewrite.sh, t/t4009-diff-rename-4.sh
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 ------------
Junio C Hamano[PATCH 2/2] diff: Fix trailing slash handling.
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(-)
t/t4010-diff-pathspec.sh, diffcore-pathspec.c
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); ------------
Junio C Hamano[PATCH] ls-tree: remove trailing slashes properly.
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(-)
t/t3100-ls-tree-restrict.sh, ls-tree.c
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); } ------------
Linus TorvaldsRe: [PATCH] ls-tree: remove trailing slashes properly.
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
Junio C HamanoRe: [PATCH] ls-tree: remove trailing slashes properly.
>>>>> "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.
Junio C Hamano[PATCH] ls-tree: handle trailing slashes in the pathspec properly.
>>>>> "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(-)
ls-tree.c, t/t3100-ls-tree-restrict.sh
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 ------------
Linus TorvaldsRe: [PATCH] ls-tree: handle trailing slashes in the pathspec properly.
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
Junio C Hamano[PATCH] ls-tree: handle trailing slashes in the pathspec properly.
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(-)
ls-tree.c, t/t3100-ls-tree-restrict.sh, t/t4010-diff-pathspec.sh
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 ------------
Linus TorvaldsRe: [PATCH] ls-tree: handle trailing slashes in the pathspec properly.
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
Junio C HamanoRe: [PATCH] ls-tree: remove trailing slashes properly.
Should we do the same for git-ls-files?