threads / patch / 46550

patchapply: remove prefix_length member from apply_state

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

## tl;dr

5 messages between Aug 9, 2017 and Aug 11, 2017. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

René Scharfe· Aug 9, 2017, 15:54 UTC · lore

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.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 apply.c | 12 +++++-------
 apply.h |  1 -
 2 files changed, 5 insertions(+), 8 deletions(-)
Show changes to 2 files +5 −8

apply.c, apply.h

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;
 	state->lock_file = lock_file;
 	state->newfd = -1;
 	state->apply = 1;
@@ -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);
 		else {
 			cp++;
-			if (!strncmp(cp, state->prefix, state->prefix_length))
+			if (starts_with(cp, state->prefix))
 				val = count_slashes(state->prefix) + 1;
 		}
 	}
@@ -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;
 	}
 
diff --git a/apply.h b/apply.h
index b3d6783d55..d9b3957703 100644
--- a/apply.h
+++ b/apply.h
@@ -35,7 +35,6 @@ enum apply_verbosity {
 
 struct apply_state {
 	const char *prefix;
-	int prefix_length;
 
 	/* These are lock_file related */
 	struct lock_file *lock_file;

-- 
2.14.0
Christian Couder· Aug 10, 2017, 15:58 UTC · re: René Scharfe · lore

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

On Wed, Aug 9, 2017 at 5:54 PM, René Scharfe <l.s.r@web.de> 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.
Show 13 quoted lines
> @@ -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;
>         }
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) || !*rest))
               return 0;
Jeff King· Aug 10, 2017, 23:41 UTC · re: René Scharfe · lore

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

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.

Show 9 quoted lines
> 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.
Show 7 quoted lines
> @@ -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.

Show 6 quoted lines
>  		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.
Show 13 quoted lines
> @@ -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
René Scharfe· Aug 11, 2017, 08:52 UTC · re: Jeff King · lore

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

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

Show 23 quoted lines
> 
>> 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.
Show 16 quoted lines
>> @@ -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.

Show 11 quoted lines
> 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.

Show 6 quoted lines
> 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é
Jeff King· Aug 11, 2017, 09:04 UTC · re: René Scharfe · lore

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

On Fri, Aug 11, 2017 at 10:52:48AM +0200, René Scharfe wrote:
Show 15 quoted lines
> > 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.

Show 9 quoted lines
> > 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.

Show 6 quoted lines
> 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:
Show changes to setup.c +0 −2
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

← back to recent threads