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

[Thomas Rast <trast@student.ethz.ch>]

Junio C Hamano wrote:
> 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.
[...]
> 	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