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

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano

Signed-off-by: Junio C Hamano <[hidden email]>
---
 commit.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Johannes Schindelin
Hi Junio,

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

Ciao,
Dscho
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Johannes Schindelin <[hidden email]> writes:

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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html