Re: [PATCH] apply: remove prefix_length member from apply_state

["René Scharfe" <l.s.r@web.de>]

Am 11.08.2017 um 01:41 schrieb Jeff King:
> 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.

Yes; I meant: Check against NULL to see if we even have a string and check
its first byte against NUL to see if that string is empty instead of
checking that its length is greater than zero.

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

Yes, diff -U5 or -W would have shown that easily. 

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

Right, the patch is only meant to stop storing the string length
without changing any user-visible behavior.

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

So you mean that removing the prefix length check would just cause
empty paths to be rejected if we have an empty prefix, which shouldn't
bother anyone because empty paths can't be used anyway, right?

Actually I'm not even sure it's possible to have an empty, non-NULL
prefix.

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

As the literal translation it is intended to be it should have been a
no-brainer. :)

Applying a patch to foobar when we're in foo/ is not intended ("Paths
outside are not touched").

I don't know if prefixes are guaranteed to end with a slash; the code
in setup.c seems to ensure that, but has it been spelled out
explicitly somewhere?  apply.c::use_patch() certainly relies on that.

René