Re: [PATCH] git-svn: fix ls-tree usage with dash-prefixed paths

Eric Wong [off-list ref] writes:

Junio C Hamano [off-list ref] wrote:

quoted

I think that is an independent bug.  Not just "--" but it appears "--d"
seems to hit it (and this is an ancient bug---even v1.0.0 seems to have
it).

quoted

I suspect that ls-tree needs a fix, not about "--" but about the pathspec
filtering.  It appears that the part that decides if a subtree is worth
traversing into uses the correct "is a pathspec pattern match leading path
components?" semantics (i.e. "--dashed" matches but "--" doesn't), but
after traversing into subtrees, the part that emits the output uses a
broken semantics "does the path have any pathspec patter as its prefix?"
It shouldn't check for "prefix", but for "leading path components", in
other words, the match must happen at directory boundaries.

And I do not think *this* bug is too late to fix.  We should fix it.

From the ls-tree documentation, I was under the impression that "--"
matching "--dashed" was intended:

  When paths are given, show them (note that this isn't really raw
  pathnames, but rather a list of patterns to match).

It doesn't make sense to me match like this, either; but I do think it
was intended and it will break things if people depend on the
existing behavior.

Ok, but then the decision to descend into --dashed should be consistent
with that policy, no?  Right now, it appears that giving "--" alone says
"Anything under --dashed can never match that pattern, so I wouldn't
bother recursing into it".

Junio C Hamano [off-list ref] wrote:

Eric Wong [off-list ref] writes:

quoted

Junio C Hamano [off-list ref] wrote:

quoted

I think that is an independent bug.  Not just "--" but it appears "--d"
seems to hit it (and this is an ancient bug---even v1.0.0 seems to have
it).

quoted

I suspect that ls-tree needs a fix, not about "--" but about the pathspec
filtering.  It appears that the part that decides if a subtree is worth
traversing into uses the correct "is a pathspec pattern match leading path
components?" semantics (i.e. "--dashed" matches but "--" doesn't), but
after traversing into subtrees, the part that emits the output uses a
broken semantics "does the path have any pathspec patter as its prefix?"
It shouldn't check for "prefix", but for "leading path components", in
other words, the match must happen at directory boundaries.

And I do not think *this* bug is too late to fix.  We should fix it.

From the ls-tree documentation, I was under the impression that "--"
matching "--dashed" was intended:

  When paths are given, show them (note that this isn't really raw
  pathnames, but rather a list of patterns to match).

It doesn't make sense to me match like this, either; but I do think it
was intended and it will break things if people depend on the
existing behavior.

Ok, but then the decision to descend into --dashed should be consistent
with that policy, no?  Right now, it appears that giving "--" alone says
"Anything under --dashed can never match that pattern, so I wouldn't
bother recursing into it".

Right.  Except in the case when there are multiple files inside --dashed/
as Björn's email illustrated.  So there seems to be a bug in the way
the number of files inside --dashed/ affects what "--" does when used
with "--dashed/1" (if --dashed/2 also exists).  Very confusing :x

-- 
Eric Wong

On 2009.03.30 15:58:34 -0700, Eric Wong wrote:

Junio C Hamano [off-list ref] wrote:

quoted

Eric Wong [off-list ref] writes:

quoted

From the ls-tree documentation, I was under the impression that "--"
matching "--dashed" was intended:

  When paths are given, show them (note that this isn't really raw
  pathnames, but rather a list of patterns to match).

It doesn't make sense to me match like this, either; but I do think it
was intended and it will break things if people depend on the
existing behavior.

I guess that paragraph was meant to explain why "git ls-tree HEAD
Documentation" and "git ls-tree HEAD Documentation/" give different
results.  The first one shows the entry for the tree object, while the
second one shows the contents of the tree object. In contrast to "ls"
which would descend into the directory in both cases.

quoted

Ok, but then the decision to descend into --dashed should be consistent
with that policy, no?  Right now, it appears that giving "--" alone says
"Anything under --dashed can never match that pattern, so I wouldn't
bother recursing into it".

Right.  Except in the case when there are multiple files inside --dashed/
as Björn's email illustrated.  So there seems to be a bug in the way
the number of files inside --dashed/ affects what "--" does when used
with "--dashed/1" (if --dashed/2 also exists).  Very confusing :x

It's not the number of files that matters. With just one file, you just
don't notice the buggy behaviour, because showing all files is the same
as showing the specified file.

And interestingly, the problem doesn't seem to be in
show_tree/show_recursive, but in match_tree_entry.

With "git ls-tree HEAD gitweb/git-favicon.png g" we descend into gitweb/
and at some point we get:

match = "g"
base = "gitweb/"

And we have:
if (baselen >= matchlen) {
	if (strncmp(base, match, matchlen))
		continue;
	/* The base is a subdirectory of a path which was specified */
	return 1;
}

So we return 1 there. The code doesn't do what the comment says, so I
guess we can be pretty sure that the behaviour is not intended.

Björn

On 2009.03.31 09:11:00 +0200, Björn Steinbrink wrote:

And interestingly, the problem doesn't seem to be in
show_tree/show_recursive, but in match_tree_entry.

With "git ls-tree HEAD gitweb/git-favicon.png g" we descend into gitweb/
and at some point we get:

match = "g"
base = "gitweb/"

And we have:
if (baselen >= matchlen) {
	if (strncmp(base, match, matchlen))
		continue;
	/* The base is a subdirectory of a path which was specified */
	return 1;
}

So we return 1 there. The code doesn't do what the comment says, so I
guess we can be pretty sure that the behaviour is not intended.

Yup, it's in match_tree_entry, you get the same thing with git show.
With git.git, you can try with:

git show 4fa535a -- Documentation/git-merge.txt D

I'll try to get a patch done, if noone beats me to it.

Björn

On 2009.03.31 09:31:47 +0200, Björn Steinbrink wrote:

On 2009.03.31 09:11:00 +0200, Björn Steinbrink wrote:

quoted

And interestingly, the problem doesn't seem to be in
show_tree/show_recursive, but in match_tree_entry.

With "git ls-tree HEAD gitweb/git-favicon.png g" we descend into gitweb/
and at some point we get:

match = "g"
base = "gitweb/"

And we have:
if (baselen >= matchlen) {
	if (strncmp(base, match, matchlen))
		continue;
	/* The base is a subdirectory of a path which was specified */
	return 1;
}

So we return 1 there. The code doesn't do what the comment says, so I
guess we can be pretty sure that the behaviour is not intended.

Yup, it's in match_tree_entry, you get the same thing with git show.
With git.git, you can try with:

git show 4fa535a -- Documentation/git-merge.txt D

I'll try to get a patch done, if noone beats me to it.

Ah, crap, "git show" actually uses a different function,
tree_entry_interesting, which happens to have the same problem, but
needs a slightly different fix.

Björn

Previously the code did a simple prefix match, which means that it
treated for example "foo/" as a subdirectory of "f".

Signed-off-by: Björn Steinbrink <redacted>
---
I'm not exactly happy with the commit message, but that's the best I
could come up with. Probably shows how little I know about that code :-/
The test suite still passes and I'll try to provide a new testcase
tonight or tommorow.

 tree-diff.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/tree-diff.c b/tree-diff.c
index 9f67af6..b05d0f4 100644
--- a/tree-diff.c
+++ b/tree-diff.c

@@ -118,10 +118,16 @@ static int tree_entry_interesting(struct tree_desc *desc, const char *base, int
 				continue;
 
 			/*
-			 * The base is a subdirectory of a path which
-			 * was specified, so all of them are interesting.
+			 * If the base is a subdirectory of a path which
+			 * was specified, all of them are interesting.
 			 */
-			return 2;
+			if (!matchlen ||
+			    base[matchlen] == '/' ||
+			    match[matchlen - 1] == '/')
+				return 2;
+
+			/* Just a random prefix match */
+			continue;
 		}
 
 		/* Does the base match? */

-- 
1.6.2.1.426.gf94cd