On Wed, Aug 09, 2017 at 05:54:46PM +0200, René Scharfe wrote:
> Use a NULL-and-NUL check to see if we have a prefix and consistently use
> C string functions on it instead of storing its length in a member of
> struct apply_state. This avoids strlen() calls and simplifies the code.
I had to read the code to figure out exactly what you meant by
NULL-and-NUL (and even then it took me a minute).
I thought at first the latter half just means "use starts_with to walk
the string incrementally rather than bothering to find the length ahead
of time". Which makes perfect sense to me.
But actually, I think you mean the final block which makes sure we have
a non-empty string.
> diff --git a/apply.c b/apply.c
> index 970bed6d41..168dfe3d16 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -80,7 +80,6 @@ int init_apply_state(struct apply_state *state,
> {
> memset(state, 0, sizeof(*state));
> state->prefix = prefix;
> - state->prefix_length = state->prefix ? strlen(state->prefix) : 0;
So we suspect that state->prefix might be NULL here.
> @@ -786,11 +785,11 @@ static int guess_p_value(struct apply_state *state, const char *nameline)
> * Does it begin with "a/$our-prefix" and such? Then this is
> * very likely to apply to our directory.
> */
> - if (!strncmp(name, state->prefix, state->prefix_length))
> + if (starts_with(name, state->prefix))
> val = count_slashes(state->prefix);
At first this looked wrong to me. Don't we need to check for NULL? But
the check is simply just outside the context, so we are good.
> else {
> cp++;
> - if (!strncmp(cp, state->prefix, state->prefix_length))
> + if (starts_with(cp, state->prefix))
> val = count_slashes(state->prefix) + 1;
> }
And likewise for this one, which is part of the same block.
> @@ -2088,10 +2087,9 @@ static int use_patch(struct apply_state *state, struct patch *p)
> int i;
>
> /* Paths outside are not touched regardless of "--include" */
> - if (0 < state->prefix_length) {
> - int pathlen = strlen(pathname);
> - if (pathlen <= state->prefix_length ||
> - memcmp(state->prefix, pathname, state->prefix_length))
> + if (state->prefix && *state->prefix) {
> + const char *rest;
> + if (!skip_prefix(pathname, state->prefix, &rest) || !*rest)
> return 0;
> }
The check for *state->prefix here makes sure the behavior remains
identical. I wondered at first whether it's actually necessary. Wouldn't
an empty prefix always match?
But I think we're looking for the pathname to be a proper superset of
the prefix (hence the "!*rest" check). So I guess an empty path would
not be a proper superset of an empty prefix, even though with the
current code it doesn't hit that block at all.
I'm still not sure it's possible to have an empty pathname, so that
corner case may be moot and we could simplify the condition a little.
But certainly as you've written it, it could not be a regression.
The use of skip_prefix also took me a minute. I wonder if it's worth a
comment with the words "proper superset" or some other indicator (it
also surprised me that we're ok with matching "foobar" in the prefix
"foo", and not demanding "foo/bar". But I didn't look at the context to
know whether that's right or not. This may be a case where the prefix is
supposed to have "/" on it already).
-Peff
