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

Junio C Hamano [off-list ref] 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.

static int common_prefix(const char **pathspec)
{
	const char *path, *slash, *next;
	int prefix;

	if (!pathspec)
		return 0;

	path = *pathspec;
	slash = strrchr(path, '/');
	if (!slash)
		return 0;
	prefix = slash - path + 1;

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

Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

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