Re: [PATCH] common_prefix: be more careful about pathspec bounds

From: Thomas Rast <trast@student.ethz.ch>
Date: Jun 15, 2010, 18:04 UTC
Message-ID: <201006152004.01507.trast@student.ethz.ch>
In-Reply-To: <7v4oh48elm.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 9 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Isn't it more intuitive to structure the loop by saying 'Ok, if "path" up
> > to the currently proposed "prefix" is too long to match, let's shorten it
> > by one path component and try again'?
> 
> Another way of saying this, which probably needs less number of scans,
> would be to shorten prefix to what is known to match --- use of memcmp()
> discards this information.

[...]

Show 13 quoted lines

> 	while ((next = *++pathspec) != NULL) {
> 		int len, last_matching_slash = -1;
> 		for (len = 0; len < prefix && next[len] == path[len]; len++)
> 			if (next[len] == '/')
> 				last_matching_slash = len;
> 		if (len == prefix)
> 			continue;
> 		if (last_matching_slash < 0)
> 			return 0;
> 		prefix = last_matching_slash + 1;
> 	}
> 	return prefix;
> }

I really didn't like the two-interleaved-loops version in your last mail, but this one is way more readable than even the original.

(Why did you wrap the for loop? It's only 76 chars.)

Maybe you could add a comment like

> 	slash = strrchr(path, '/');
> 	if (!slash)
> 		return 0;

	/* The first 'prefix' characters of 'path' always include the
	   trailing slash of the prefix directory */

> 	prefix = slash - path + 1;

to clean up any doubts about the correctness of the matching.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch