threads / patch / 42330

patchcommit.c: use strchrnul() to scan for one line

Subject: [PATCH] commit.c: use strchrnul() to scan for one line

## tl;dr

3 messages between May 15, 2016 and May 16, 2016. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Junio C Hamano· May 15, 2016, 22:23 UTC · lore

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 commit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

Show changes to commit.c +1 −2

diff --git a/commit.c b/commit.c
index 3f4f371..1f9ee8a 100644
--- a/commit.c
+++ b/commit.c
@@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
 		p++;
 	if (*p) {
 		p += 2;
-		for (eol = p; *eol && *eol != '\n'; eol++)
-			; /* do nothing */
+		eol = strchrnul(p, '\n');
 	} else
 		eol = p;

-- 
2.8.2-748-gfb85f76

Johannes Schindelin· May 16, 2016, 06:12 UTC · re: Junio C Hamano · lore

Re: [PATCH] commit.c: use strchrnul() to scan for one line

Hi Junio,

On Sun, 15 May 2016, Junio C Hamano wrote:

Show 13 quoted lines

> diff --git a/commit.c b/commit.c
> index 3f4f371..1f9ee8a 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
>  		p++;
>  	if (*p) {
>  		p += 2;
> -		for (eol = p; *eol && *eol != '\n'; eol++)
> -			; /* do nothing */
> +		eol = strchrnul(p, '\n');
>  	} else
>  		eol = p;

ACK. This was my fault, when I introduced the code in 9509af68 (Make git-revert & git-cherry-pick a builtin, 2007-03-01). To be fair, strchrnul() was introduced only later, in 659c69c (Add strchrnul(), 2007-11-09).

Ciao, Dscho

Junio C Hamano· May 16, 2016, 15:57 UTC · re: Johannes Schindelin · lore

Re: [PATCH] commit.c: use strchrnul() to scan for one line

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 20 quoted lines

> On Sun, 15 May 2016, Junio C Hamano wrote:
>
>> diff --git a/commit.c b/commit.c
>> index 3f4f371..1f9ee8a 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -415,8 +415,7 @@ int find_commit_subject(const char *commit_buffer, const char **subject)
>>  		p++;
>>  	if (*p) {
>>  		p += 2;
>> -		for (eol = p; *eol && *eol != '\n'; eol++)
>> -			; /* do nothing */
>> +		eol = strchrnul(p, '\n');
>>  	} else
>>  		eol = p;
>
> ACK. This was my fault, when I introduced the code in 9509af68 (Make
> git-revert & git-cherry-pick a builtin, 2007-03-01). To be fair,
> strchrnul() was introduced only later, in 659c69c (Add strchrnul(),
> 2007-11-09).

Oh, there is no fault.

I was reading through attr.c to see how bad a work to revamp attribute lookup would look like, found a hand-rolled strchrnul() that predates the widespread use of the function, and looked for similar patterns while I was updating it. As there were many false positives (i.e. "Yes, this loop is looking for the end of line, but it does something else to the characters on the line while doing so, so it cannot become strchrnul()") that I needed eyeballing in order to reject and avoid incorrect conversion, it is very much appreciated that you double-checked this one that I spotted.

Thanks.