threads / patch / 45228

patch, 2 partscommit: be more precise when searching for headers

Subject: [PATCH 1/2] commit: be more precise when searching for headers

## tl;dr

9 messages between Feb 25, 2017 and Feb 27, 2017. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

René Scharfe· Feb 25, 2017, 19:21 UTC · lore

Search for a space character only within the current line in read_commit_extra_header_lines() instead of searching in the whole buffer (and possibly beyond, if it's not NUL-terminated) and then discarding any results after the end of the current line.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 commit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to commit.c +2 −2
diff --git a/commit.c b/commit.c
index 2cf85158b4..173c6d3818 100644
--- a/commit.c
+++ b/commit.c
@@ -1354,8 +1354,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(
 		strbuf_reset(&buf);
 		it = NULL;
 
-		eof = strchr(line, ' ');
-		if (next <= eof)
+		eof = memchr(line, ' ', next - line);
+		if (!eof)
 			eof = next;
 
 		if (standard_header_field(line, eof - line) ||
-- 
2.12.0
René Scharfe· Feb 25, 2017, 19:27 UTC · re: René Scharfe · lore

[PATCH 2/2] commit: don't check for space twice when looking for header

Both standard_header_field() and excluded_header_field() check if there's a space after the buffer that's handed to them. We already check in the caller if that space is present. Don't bother calling the functions if it's missing, as they are guaranteed to return 0 in that case, and remove the now redundant checks from them.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 commit.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)
Show changes to commit.c +8 −10
diff --git a/commit.c b/commit.c
index 173c6d3818..fab8269731 100644
--- a/commit.c
+++ b/commit.c
@@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
 
 static inline int standard_header_field(const char *field, size_t len)
 {
-	return ((len == 4 && !memcmp(field, "tree ", 5)) ||
-		(len == 6 && !memcmp(field, "parent ", 7)) ||
-		(len == 6 && !memcmp(field, "author ", 7)) ||
-		(len == 9 && !memcmp(field, "committer ", 10)) ||
-		(len == 8 && !memcmp(field, "encoding ", 9)));
+	return ((len == 4 && !memcmp(field, "tree", 4)) ||
+		(len == 6 && !memcmp(field, "parent", 6)) ||
+		(len == 6 && !memcmp(field, "author", 6)) ||
+		(len == 9 && !memcmp(field, "committer", 9)) ||
+		(len == 8 && !memcmp(field, "encoding", 8)));
 }
 
 static int excluded_header_field(const char *field, size_t len, const char **exclude)
@@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, size_t len, const char **exc
 
 	while (*exclude) {
 		size_t xlen = strlen(*exclude);
-		if (len == xlen &&
-		    !memcmp(field, *exclude, xlen) && field[xlen] == ' ')
+		if (len == xlen && !memcmp(field, *exclude, xlen))
 			return 1;
 		exclude++;
 	}
@@ -1357,9 +1356,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(
 		eof = memchr(line, ' ', next - line);
 		if (!eof)
 			eof = next;
-
-		if (standard_header_field(line, eof - line) ||
-		    excluded_header_field(line, eof - line, exclude))
+		else if (standard_header_field(line, eof - line) ||
+			 excluded_header_field(line, eof - line, exclude))
 			continue;
 
 		it = xcalloc(1, sizeof(*it));
-- 
2.12.0
Jeff King· Feb 25, 2017, 20:15 UTC · re: René Scharfe · lore

Re: [PATCH 2/2] commit: don't check for space twice when looking for header

On Sat, Feb 25, 2017 at 08:27:40PM +0100, René Scharfe wrote:
Show 5 quoted lines
> Both standard_header_field() and excluded_header_field() check if
> there's a space after the buffer that's handed to them.  We already
> check in the caller if that space is present.  Don't bother calling
> the functions if it's missing, as they are guaranteed to return 0 in
> that case, and remove the now redundant checks from them.

Makes sense, and I couldn't spot any errors in your logic or in the code.

Show 12 quoted lines
>  static inline int standard_header_field(const char *field, size_t len)
>  {
> -	return ((len == 4 && !memcmp(field, "tree ", 5)) ||
> -		(len == 6 && !memcmp(field, "parent ", 7)) ||
> -		(len == 6 && !memcmp(field, "author ", 7)) ||
> -		(len == 9 && !memcmp(field, "committer ", 10)) ||
> -		(len == 8 && !memcmp(field, "encoding ", 9)));
> +	return ((len == 4 && !memcmp(field, "tree", 4)) ||
> +		(len == 6 && !memcmp(field, "parent", 6)) ||
> +		(len == 6 && !memcmp(field, "author", 6)) ||
> +		(len == 9 && !memcmp(field, "committer", 9)) ||
> +		(len == 8 && !memcmp(field, "encoding", 8)));

Unrelated, but this could probably be spelled with a macro and strlen() to avoid the magic numbers. It would probably be measurably slower for a compiler which doesn't pre-compute strlen() on a string literal, though.

-Peff
René Scharfe· Feb 25, 2017, 21:39 UTC · re: Jeff King · lore

Re: [PATCH 2/2] commit: don't check for space twice when looking for header

Am 25.02.2017 um 21:15 schrieb Jeff King:
Show 10 quoted lines
> On Sat, Feb 25, 2017 at 08:27:40PM +0100, René Scharfe wrote:
>
>> Both standard_header_field() and excluded_header_field() check if
>> there's a space after the buffer that's handed to them.  We already
>> check in the caller if that space is present.  Don't bother calling
>> the functions if it's missing, as they are guaranteed to return 0 in
>> that case, and remove the now redundant checks from them.
>
> Makes sense, and I couldn't spot any errors in your logic or in the
> code.
Thanks for checking!
Show 16 quoted lines
>>  static inline int standard_header_field(const char *field, size_t len)
>>  {
>> -	return ((len == 4 && !memcmp(field, "tree ", 5)) ||
>> -		(len == 6 && !memcmp(field, "parent ", 7)) ||
>> -		(len == 6 && !memcmp(field, "author ", 7)) ||
>> -		(len == 9 && !memcmp(field, "committer ", 10)) ||
>> -		(len == 8 && !memcmp(field, "encoding ", 9)));
>> +	return ((len == 4 && !memcmp(field, "tree", 4)) ||
>> +		(len == 6 && !memcmp(field, "parent", 6)) ||
>> +		(len == 6 && !memcmp(field, "author", 6)) ||
>> +		(len == 9 && !memcmp(field, "committer", 9)) ||
>> +		(len == 8 && !memcmp(field, "encoding", 8)));
>
> Unrelated, but this could probably be spelled with a macro and strlen()
> to avoid the magic numbers. It would probably be measurably slower for a
> compiler which doesn't pre-compute strlen() on a string literal, though.
sizeof(string_constant) - 1 might be a better choice here than strlen().
René
Jeff King· Feb 25, 2017, 21:51 UTC · re: René Scharfe · lore

Re: [PATCH 2/2] commit: don't check for space twice when looking for header

On Sat, Feb 25, 2017 at 10:39:29PM +0100, René Scharfe wrote:
Show 7 quoted lines
> > > +		(len == 8 && !memcmp(field, "encoding", 8)));
> > 
> > Unrelated, but this could probably be spelled with a macro and strlen()
> > to avoid the magic numbers. It would probably be measurably slower for a
> > compiler which doesn't pre-compute strlen() on a string literal, though.
> 
> sizeof(string_constant) - 1 might be a better choice here than strlen().

Yeah. If you use a macro, that works. If it's an inline function you'd need strlen(). That's a tradeoff we've already made in skip_prefix_mem() and strip_suffix(), but it's not like we expect this list to grow much, so it may not be worth fussing with.

-Peff
Jakub Narębski· Feb 27, 2017, 22:27 UTC · re: René Scharfe · lore

Re: [PATCH 2/2] commit: don't check for space twice when looking for header

W dniu 25.02.2017 o 20:27, René Scharfe pisze:
Show 29 quoted lines
> Both standard_header_field() and excluded_header_field() check if
> there's a space after the buffer that's handed to them.  We already
> check in the caller if that space is present.  Don't bother calling
> the functions if it's missing, as they are guaranteed to return 0 in
> that case, and remove the now redundant checks from them.
> 
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  commit.c | 18 ++++++++----------
>  1 file changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/commit.c b/commit.c
> index 173c6d3818..fab8269731 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
>  
>  static inline int standard_header_field(const char *field, size_t len)
>  {
> -	return ((len == 4 && !memcmp(field, "tree ", 5)) ||
> -		(len == 6 && !memcmp(field, "parent ", 7)) ||
> -		(len == 6 && !memcmp(field, "author ", 7)) ||
> -		(len == 9 && !memcmp(field, "committer ", 10)) ||
> -		(len == 8 && !memcmp(field, "encoding ", 9)));
> +	return ((len == 4 && !memcmp(field, "tree", 4)) ||
> +		(len == 6 && !memcmp(field, "parent", 6)) ||
> +		(len == 6 && !memcmp(field, "author", 6)) ||
> +		(len == 9 && !memcmp(field, "committer", 9)) ||
> +		(len == 8 && !memcmp(field, "encoding", 8)));

I agree (for what it is worth from me) with the rest of changes, but I think current code is better self-documenting for this function.

Show 26 quoted lines
>  }
>  
>  static int excluded_header_field(const char *field, size_t len, const char **exclude)
> @@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, size_t len, const char **exc
>  
>  	while (*exclude) {
>  		size_t xlen = strlen(*exclude);
> -		if (len == xlen &&
> -		    !memcmp(field, *exclude, xlen) && field[xlen] == ' ')
> +		if (len == xlen && !memcmp(field, *exclude, xlen))
>  			return 1;
>  		exclude++;
>  	}
> @@ -1357,9 +1356,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(
>  		eof = memchr(line, ' ', next - line);
>  		if (!eof)
>  			eof = next;
> -
> -		if (standard_header_field(line, eof - line) ||
> -		    excluded_header_field(line, eof - line, exclude))
> +		else if (standard_header_field(line, eof - line) ||
> +			 excluded_header_field(line, eof - line, exclude))
>  			continue;
>  
>  		it = xcalloc(1, sizeof(*it));
> 
René Scharfe· Feb 27, 2017, 22:54 UTC · re: Jakub Narębski · lore

Re: [PATCH 2/2] commit: don't check for space twice when looking for header

Am 27.02.2017 um 23:27 schrieb Jakub Narębski:
Show 34 quoted lines
> W dniu 25.02.2017 o 20:27, René Scharfe pisze:
>> Both standard_header_field() and excluded_header_field() check if
>> there's a space after the buffer that's handed to them.  We already
>> check in the caller if that space is present.  Don't bother calling
>> the functions if it's missing, as they are guaranteed to return 0 in
>> that case, and remove the now redundant checks from them.
>>
>> Signed-off-by: Rene Scharfe <l.s.r@web.de>
>> ---
>>  commit.c | 18 ++++++++----------
>>  1 file changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/commit.c b/commit.c
>> index 173c6d3818..fab8269731 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -1308,11 +1308,11 @@ void for_each_mergetag(each_mergetag_fn fn, struct commit *commit, void *data)
>>
>>  static inline int standard_header_field(const char *field, size_t len)
>>  {
>> -	return ((len == 4 && !memcmp(field, "tree ", 5)) ||
>> -		(len == 6 && !memcmp(field, "parent ", 7)) ||
>> -		(len == 6 && !memcmp(field, "author ", 7)) ||
>> -		(len == 9 && !memcmp(field, "committer ", 10)) ||
>> -		(len == 8 && !memcmp(field, "encoding ", 9)));
>> +	return ((len == 4 && !memcmp(field, "tree", 4)) ||
>> +		(len == 6 && !memcmp(field, "parent", 6)) ||
>> +		(len == 6 && !memcmp(field, "author", 6)) ||
>> +		(len == 9 && !memcmp(field, "committer", 9)) ||
>> +		(len == 8 && !memcmp(field, "encoding", 8)));
>
> I agree (for what it is worth from me) with the rest of changes,
> but I think current code is better self-documenting for this
> function.

Having a function that is given a buffer/length pair and accessing the byte after it raises questions, though. :)

Nicer than keeping the space would be to use excluded_header_field() for standard headers as well as a next step, I think -- but that would be a bit slower.

Show 28 quoted lines
>
>>  }
>>
>>  static int excluded_header_field(const char *field, size_t len, const char **exclude)
>> @@ -1322,8 +1322,7 @@ static int excluded_header_field(const char *field, size_t len, const char **exc
>>
>>  	while (*exclude) {
>>  		size_t xlen = strlen(*exclude);
>> -		if (len == xlen &&
>> -		    !memcmp(field, *exclude, xlen) && field[xlen] == ' ')
>> +		if (len == xlen && !memcmp(field, *exclude, xlen))
>>  			return 1;
>>  		exclude++;
>>  	}
>> @@ -1357,9 +1356,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(
>>  		eof = memchr(line, ' ', next - line);
>>  		if (!eof)
>>  			eof = next;
>> -
>> -		if (standard_header_field(line, eof - line) ||
>> -		    excluded_header_field(line, eof - line, exclude))
>> +		else if (standard_header_field(line, eof - line) ||
>> +			 excluded_header_field(line, eof - line, exclude))
>>  			continue;
>>
>>  		it = xcalloc(1, sizeof(*it));
>>
>
Jeff King· Feb 25, 2017, 20:12 UTC · re: René Scharfe · lore

Re: [PATCH 1/2] commit: be more precise when searching for headers

On Sat, Feb 25, 2017 at 08:21:52PM +0100, René Scharfe wrote:
Show 10 quoted lines
> Search for a space character only within the current line in
> read_commit_extra_header_lines() instead of searching in the whole
> buffer (and possibly beyond, if it's not NUL-terminated) and then
> discarding any results after the end of the current line.
> [...]
> -		eof = strchr(line, ' ');
> -		if (next <= eof)
> +		eof = memchr(line, ' ', next - line);
> +		if (!eof)
>  			eof = next;
Nice. More efficient, and I think the intent is more clear.
-Peff
Junio C Hamano· Feb 27, 2017, 19:18 UTC · re: René Scharfe · lore

Re: [PATCH 1/2] commit: be more precise when searching for headers

René Scharfe <l.s.r@web.de> writes:
Show 9 quoted lines
> Search for a space character only within the current line in
> read_commit_extra_header_lines() instead of searching in the whole
> buffer (and possibly beyond, if it's not NUL-terminated) and then
> discarding any results after the end of the current line.
>
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
>  commit.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
Makes sense.
Show 15 quoted lines
> diff --git a/commit.c b/commit.c
> index 2cf85158b4..173c6d3818 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1354,8 +1354,8 @@ static struct commit_extra_header *read_commit_extra_header_lines(
>  		strbuf_reset(&buf);
>  		it = NULL;
>  
> -		eof = strchr(line, ' ');
> -		if (next <= eof)
> +		eof = memchr(line, ' ', next - line);
> +		if (!eof)
>  			eof = next;
>  
>  		if (standard_header_field(line, eof - line) ||

← back to recent threads