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

[Jeff King]

On Fri, Aug 11, 2017 at 10:52:48AM +0200, René Scharfe wrote:

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

Right.

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

I'm not sure either. I had assumed this came from a --prefix argument to
git-apply, but it looks like it only ever comes from setup.c's prefix.
We seem to avoid empty prefixes there, but there are a lot of different
code paths and I didn't check them all.

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

Yeah. All of this is mostly me thinking out loud about whether we can
improve the existing code further. Don't feel like you need to spend
time on it.

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

I don't know that it's been spelled out. But if you do this:

diff --git a/setup.c b/setup.c
index 860507e1fd..48af25cac1 100644
--- a/setup.c
+++ b/setup.c
@@ -765,7 +765,6 @@ static const char *setup_discovered_git_dir(const char 
*gitdir,
if (offset != offset_1st_component(cwd->buf))
offset++;
/* Add a '/' at the end */
-   strbuf_addch(cwd, '/');
return cwd->buf + offset;
 }
 

lots of tests break horribly. So I'm content that we'd probably catch a
regression there, even if it's not spelled out explicitly.

-Peff

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

[René Scharfe]

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

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

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

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

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

[Christian Couder]

On Wed, Aug 9, 2017 at 5:54 PM, 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.

This looks like a good idea.

> @@ -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)
> return 0;
> }

Yeah, or maybe declare "const char *rest;" just after "int i;" and then use:

   if (state->prefix && *state->prefix &&
  (!skip_prefix(pathname, state->prefix, ) || !*rest))
   return 0;