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

From: Björn Steinbrink <b.steinbrink@gmx.de>
Date: Mar 31, 2009, 07:11 UTC
Message-ID: <20090331071100.GA3307@atjola.homenet>
In-Reply-To: <20090330225834.GA24254@dcvr.yhbt.net>

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

Show 11 quoted lines

> Junio C Hamano <gitster@pobox.com> wrote:
> > Eric Wong <normalperson@yhbt.net> writes:
> > > 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.

Show 9 quoted lines

> > 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