threads / patch / 36051

patchcommit.c: Replace starts_with() with skip_prefix()

Subject: [PATCH] [PATCH] commit.c: Replace starts_with() with skip_prefix()

## tl;dr

7 messages between Mar 4, 2014 and Mar 5, 2014. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Karthik Nayak· Mar 4, 2014, 15:54 UTC · lore

In record_author_date() : Replace "buf + strlen(author )" by skip_prefix(), which is saved in a new "const char" variable "indent_line".

In parse_signed_commit() : Replace "line + gpg_sig_header_len" by skip_prefix(), which is saved in a new "const char" variable "indent_line".

In parse_gpg_output() : Replace "buf + strlen(sigcheck_gpg_status[i].check + 1)" by skip_prefix(), which is saved in already available variable "found".

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 commit.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)
Show changes to commit.c +12 −11
diff --git a/commit.c b/commit.c
index 6bf4fe0..71a03e3 100644
--- a/commit.c
+++ b/commit.c
@@ -553,6 +553,7 @@ static void record_author_date(struct author_date_slab *author_date,
 	struct ident_split ident;
 	char *date_end;
 	unsigned long date;
+	const char *indent_line;
 
 	if (!commit->buffer) {
 		unsigned long size;
@@ -562,18 +563,19 @@ static void record_author_date(struct author_date_slab *author_date,
 			return;
 	}
 
+	indent_line = skip_prefix(buf, "author ");
+
 	for (buf = commit->buffer ? commit->buffer : buffer;
 	     buf;
 	     buf = line_end + 1) {
 		line_end = strchrnul(buf, '\n');
-		if (!starts_with(buf, "author ")) {
+		if (!indent_line) {
 			if (!line_end[0] || line_end[1] == '\n')
 				return; /* end of header */
 			continue;
 		}
-		if (split_ident_line(&ident,
-				     buf + strlen("author "),
-				     line_end - (buf + strlen("author "))) ||
+		if (split_ident_line(&ident, indent_line,
+							 line_end - indent_line) ||
 		    !ident.date_begin || !ident.date_end)
 			goto fail_exit; /* malformed "author" line */
 		break;
@@ -1098,6 +1100,7 @@ int parse_signed_commit(const unsigned char *sha1,
 	char *buffer = read_sha1_file(sha1, &type, &size);
 	int in_signature, saw_signature = -1;
 	char *line, *tail;
+	const char *indent_line;
 
 	if (!buffer || type != OBJ_COMMIT)
 		goto cleanup;
@@ -1111,11 +1114,11 @@ int parse_signed_commit(const unsigned char *sha1,
 		char *next = memchr(line, '\n', tail - line);
 
 		next = next ? next + 1 : tail;
+		indent_line = skip_prefix(line, gpg_sig_header);
 		if (in_signature && line[0] == ' ')
 			sig = line + 1;
-		else if (starts_with(line, gpg_sig_header) &&
-			 line[gpg_sig_header_len] == ' ')
-			sig = line + gpg_sig_header_len + 1;
+		else if (indent_line && indent_line[1] == ' ')
+			sig = indent_line + 2;
 		if (sig) {
 			strbuf_add(signature, sig, next - sig);
 			saw_signature = 1;
@@ -1193,10 +1196,8 @@ static void parse_gpg_output(struct signature_check *sigc)
 	for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
 		const char *found, *next;
 
-		if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
-			/* At the very beginning of the buffer */
-			found = buf + strlen(sigcheck_gpg_status[i].check + 1);
-		} else {
+		found = skip_prefix(buf, sigcheck_gpg_status[i].check +1);
+		if(!found) {
 			found = strstr(buf, sigcheck_gpg_status[i].check);
 			if (!found)
 				continue;

-- 
1.9.0.138.g2de3478

Hey Eric,
As per your suggestion in the previous mail :
http://article.gmane.org/gmane.comp.version-control.git/243316
I have made necessary changes:
1. Better Naming of variables as per suggestion
2. Proper replacement of skip_prefix() over starts_with() .
Tanay Abhra· Mar 4, 2014, 17:31 UTC · re: Karthik Nayak · lore

Re: [PATCH] [PATCH] commit.c: Replace starts_with() with skip_prefix()

Karthik Nayak <karthik.188 <at> gmail.com> writes:
Show 23 quoted lines
> 
> In record_author_date() :
> Replace "buf + strlen(author )" by skip_prefix(), which is
> saved in a new "const char" variable "indent_line".
> 
> In parse_signed_commit() :
> Replace "line + gpg_sig_header_len" by skip_prefix(), which
> is saved in a new "const char" variable "indent_line".
> 
> In parse_gpg_output() :
> Replace "buf + strlen(sigcheck_gpg_status[i].check + 1)" by
> skip_prefix(), which is saved in already available
> variable "found".
> 
> Signed-off-by: Karthik Nayak <karthik.188 <at> gmail.com>
> ---
>  commit.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..71a03e3 100644
> --- a/commit.c
> +++ b/commit.c
>  <at>  <at>  -1098,6 +1100,7  <at>  <at>  int parse_signed_commit(const
unsigned char *sha1,
Show 8 quoted lines
>  	char *buffer = read_sha1_file(sha1, &type, &size);
>  	int in_signature, saw_signature = -1;
>  	char *line, *tail;
> +	const char *indent_line;
> 
>  	if (!buffer || type != OBJ_COMMIT)
>  		goto cleanup;
>  <at>  <at>  -1111,11 +1114,11  <at>  <at>  int parse_signed_commit(const
unsigned char *sha1,
Show 14 quoted lines
>  		char *next = memchr(line, '\n', tail - line);
> 
>  		next = next ? next + 1 : tail;
> +		indent_line = skip_prefix(line, gpg_sig_header);
>  		if (in_signature && line[0] == ' ')
>  			sig = line + 1;
> -		else if (starts_with(line, gpg_sig_header) &&
> -			 line[gpg_sig_header_len] == ' ')
> -			sig = line + gpg_sig_header_len + 1;
> +		else if (indent_line && indent_line[1] == ' ')
> +			sig = indent_line + 2;
>  		if (sig) {
>  			strbuf_add(signature, sig, next - sig);
>  			saw_signature = 1;
Hi,

I was attempting the same microproject so I thought I would share some points that were told to me earlier .The mail subject should have a increasing order of submission numbers for each revision(for example here it should be [PATCH v2]).

Also write the superfluous stuff which you have written in the bottom after ---(the three dashes after the signed off statement) .

In the parse_signed_commit() function, gpg_sig_header_len and gpg_sig_header variables are precomputed outside of the function, and also Junio said to prefer starts_with(), whenever skip_prefix() advantages are not so important in the context.So the replace may not be so advantageous ,I may be wrong in this case.

Cheers, Tanay Abhra.

karthik nayak· Mar 4, 2014, 17:58 UTC · re: Tanay Abhra · lore

Re: [PATCH] [PATCH] commit.c: Replace starts_with() with skip_prefix()

Hey Tanay,
1. Yes just getting used to git send-email now, should follow that from now
2. I thought it shouldn't be a part of the commit, so i put it after
the last ---
3. I did have a thought on your lines also , but concluded to it being
more advantageous, you might be right though

Nice to hear from you. Thanks -Karthik Nayak

On Tue, Mar 4, 2014 at 11:01 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
Show 80 quoted lines
>
> Karthik Nayak <karthik.188 <at> gmail.com> writes:
>
>>
>> In record_author_date() :
>> Replace "buf + strlen(author )" by skip_prefix(), which is
>> saved in a new "const char" variable "indent_line".
>>
>> In parse_signed_commit() :
>> Replace "line + gpg_sig_header_len" by skip_prefix(), which
>> is saved in a new "const char" variable "indent_line".
>>
>> In parse_gpg_output() :
>> Replace "buf + strlen(sigcheck_gpg_status[i].check + 1)" by
>> skip_prefix(), which is saved in already available
>> variable "found".
>>
>> Signed-off-by: Karthik Nayak <karthik.188 <at> gmail.com>
>> ---
>>  commit.c | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/commit.c b/commit.c
>> index 6bf4fe0..71a03e3 100644
>> --- a/commit.c
>> +++ b/commit.c
>
>>  <at>  <at>  -1098,6 +1100,7  <at>  <at>  int parse_signed_commit(const
> unsigned char *sha1,
>>       char *buffer = read_sha1_file(sha1, &type, &size);
>>       int in_signature, saw_signature = -1;
>>       char *line, *tail;
>> +     const char *indent_line;
>>
>>       if (!buffer || type != OBJ_COMMIT)
>>               goto cleanup;
>>  <at>  <at>  -1111,11 +1114,11  <at>  <at>  int parse_signed_commit(const
> unsigned char *sha1,
>>               char *next = memchr(line, '\n', tail - line);
>>
>>               next = next ? next + 1 : tail;
>> +             indent_line = skip_prefix(line, gpg_sig_header);
>>               if (in_signature && line[0] == ' ')
>>                       sig = line + 1;
>> -             else if (starts_with(line, gpg_sig_header) &&
>> -                      line[gpg_sig_header_len] == ' ')
>> -                     sig = line + gpg_sig_header_len + 1;
>> +             else if (indent_line && indent_line[1] == ' ')
>> +                     sig = indent_line + 2;
>>               if (sig) {
>>                       strbuf_add(signature, sig, next - sig);
>>                       saw_signature = 1;
>
>
> Hi,
>
> I was attempting the same microproject so I thought I would share some
> points that were told to me earlier .The mail subject should have a
> increasing order of submission numbers for each revision(for example here it
> should be [PATCH v2]).
>
> Also write the superfluous stuff which you have written in the bottom
> after ---(the three dashes after the signed off statement) .
>
> In the parse_signed_commit() function, gpg_sig_header_len and gpg_sig_header
> variables are precomputed outside of the function, and also Junio said to
> prefer starts_with(), whenever skip_prefix() advantages are not so important
> in the context.So the replace may not be so advantageous ,I may be wrong in
> this case.
>
>
>
> Cheers,
> Tanay Abhra.
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sunshine· Mar 4, 2014, 22:30 UTC · re: karthik nayak · lore

Re: [PATCH] [PATCH] commit.c: Replace starts_with() with skip_prefix()

On Tue, Mar 4, 2014 at 12:58 PM, karthik nayak <karthik.188@gmail.com> wrote:
Show 7 quoted lines
> Hey Tanay,
>
> 1. Yes just getting used to git send-email now, should follow that from now
> 2. I thought it shouldn't be a part of the commit, so i put it after
> the last ---
> 3. I did have a thought on your lines also , but concluded to it being
> more advantageous, you might be right though
Minor etiquette note: On this list, respond inline rather than top-posting.

To understand why, look at how much scrolling up and down a person has to do to relate your points 1, 2, and 3 to review statements made by Tanay at the very bottom of the email.

Show 89 quoted lines
> Nice to hear from you.
> Thanks
> -Karthik Nayak
>
> On Tue, Mar 4, 2014 at 11:01 PM, Tanay Abhra <tanayabh@gmail.com> wrote:
>>
>> Karthik Nayak <karthik.188 <at> gmail.com> writes:
>>
>>>
>>> In record_author_date() :
>>> Replace "buf + strlen(author )" by skip_prefix(), which is
>>> saved in a new "const char" variable "indent_line".
>>>
>>> In parse_signed_commit() :
>>> Replace "line + gpg_sig_header_len" by skip_prefix(), which
>>> is saved in a new "const char" variable "indent_line".
>>>
>>> In parse_gpg_output() :
>>> Replace "buf + strlen(sigcheck_gpg_status[i].check + 1)" by
>>> skip_prefix(), which is saved in already available
>>> variable "found".
>>>
>>> Signed-off-by: Karthik Nayak <karthik.188 <at> gmail.com>
>>> ---
>>>  commit.c | 23 ++++++++++++-----------
>>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>>
>>> diff --git a/commit.c b/commit.c
>>> index 6bf4fe0..71a03e3 100644
>>> --- a/commit.c
>>> +++ b/commit.c
>>
>>>  <at>  <at>  -1098,6 +1100,7  <at>  <at>  int parse_signed_commit(const
>> unsigned char *sha1,
>>>       char *buffer = read_sha1_file(sha1, &type, &size);
>>>       int in_signature, saw_signature = -1;
>>>       char *line, *tail;
>>> +     const char *indent_line;
>>>
>>>       if (!buffer || type != OBJ_COMMIT)
>>>               goto cleanup;
>>>  <at>  <at>  -1111,11 +1114,11  <at>  <at>  int parse_signed_commit(const
>> unsigned char *sha1,
>>>               char *next = memchr(line, '\n', tail - line);
>>>
>>>               next = next ? next + 1 : tail;
>>> +             indent_line = skip_prefix(line, gpg_sig_header);
>>>               if (in_signature && line[0] == ' ')
>>>                       sig = line + 1;
>>> -             else if (starts_with(line, gpg_sig_header) &&
>>> -                      line[gpg_sig_header_len] == ' ')
>>> -                     sig = line + gpg_sig_header_len + 1;
>>> +             else if (indent_line && indent_line[1] == ' ')
>>> +                     sig = indent_line + 2;
>>>               if (sig) {
>>>                       strbuf_add(signature, sig, next - sig);
>>>                       saw_signature = 1;
>>
>>
>> Hi,
>>
>> I was attempting the same microproject so I thought I would share some
>> points that were told to me earlier .The mail subject should have a
>> increasing order of submission numbers for each revision(for example here it
>> should be [PATCH v2]).
>>
>> Also write the superfluous stuff which you have written in the bottom
>> after ---(the three dashes after the signed off statement) .
>>
>> In the parse_signed_commit() function, gpg_sig_header_len and gpg_sig_header
>> variables are precomputed outside of the function, and also Junio said to
>> prefer starts_with(), whenever skip_prefix() advantages are not so important
>> in the context.So the replace may not be so advantageous ,I may be wrong in
>> this case.
>>
>>
>>
>> Cheers,
>> Tanay Abhra.
>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe git" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Sunshine· Mar 4, 2014, 22:27 UTC · re: Karthik Nayak · lore

Re: [PATCH] [PATCH] commit.c: Replace starts_with() with skip_prefix()

Thanks for the resend. Etiquette on this list is to cc: people who commented on previous versions of the submission. As Tanay already mentioned, use [PATCH vN] in the subject where N is the version number of this attempt. The -v option of "git format-email" can help.

More below.
On Tue, Mar 4, 2014 at 10:54 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
Show 12 quoted lines
> In record_author_date() :
> Replace "buf + strlen(author )" by skip_prefix(), which is
> saved in a new "const char" variable "indent_line".
>
> In parse_signed_commit() :
> Replace "line + gpg_sig_header_len" by skip_prefix(), which
> is saved in a new "const char" variable "indent_line".
>
> In parse_gpg_output() :
> Replace "buf + strlen(sigcheck_gpg_status[i].check + 1)" by
> skip_prefix(), which is saved in already available
> variable "found".

It's not necessary to explain in prose what the patch itself already states more concisely and precisely. All of this text should be dropped from the commit message. Instead, explain the purpose of the patch, the problem it solves, etc. Please read the "(2) Describe your changes well" section of Documentation/SubmittingPatches to get an idea of what sort of information to include in the commit message.

A better commit message should say something about how the affected functions want to know not only if the string has a prefix, but also the text following the prefix, and that skip_prefix() can provide both pieces of information.

Show 14 quoted lines
> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  commit.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..71a03e3 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -553,6 +553,7 @@ static void record_author_date(struct author_date_slab *author_date,
>         struct ident_split ident;
>         char *date_end;
>         unsigned long date;
> +       const char *indent_line;

Strange variable name. The code in question splits apart a person's identification string (name, email, etc.). It has nothing to do with indentation.

Show 23 quoted lines
>         if (!commit->buffer) {
>                 unsigned long size;
> @@ -562,18 +563,19 @@ static void record_author_date(struct author_date_slab *author_date,
>                         return;
>         }
>
> +       indent_line = skip_prefix(buf, "author ");
> +
>         for (buf = commit->buffer ? commit->buffer : buffer;
>              buf;
>              buf = line_end + 1) {
>                 line_end = strchrnul(buf, '\n');
> -               if (!starts_with(buf, "author ")) {
> +               if (!indent_line) {
>                         if (!line_end[0] || line_end[1] == '\n')
>                                 return; /* end of header */
>                         continue;
>                 }
> -               if (split_ident_line(&ident,
> -                                    buf + strlen("author "),
> -                                    line_end - (buf + strlen("author "))) ||
> +               if (split_ident_line(&ident, indent_line,
> +                                                        line_end - indent_line) ||

Indent the second line (using tabs plus possible spaces) so that it lines up with the &ident in the line above. Be sure to set your editor so tabs have width 8.

Show 16 quoted lines
>                     !ident.date_begin || !ident.date_end)
>                         goto fail_exit; /* malformed "author" line */
>                 break;
> @@ -1098,6 +1100,7 @@ int parse_signed_commit(const unsigned char *sha1,
>         char *buffer = read_sha1_file(sha1, &type, &size);
>         int in_signature, saw_signature = -1;
>         char *line, *tail;
> +       const char *indent_line;
>
>         if (!buffer || type != OBJ_COMMIT)
>                 goto cleanup;
> @@ -1111,11 +1114,11 @@ int parse_signed_commit(const unsigned char *sha1,
>                 char *next = memchr(line, '\n', tail - line);
>
>                 next = next ? next + 1 : tail;
> +               indent_line = skip_prefix(line, gpg_sig_header);

Even stranger variable name for a GPG signature (which has nothing at all to do with indentation).

Show 7 quoted lines
>                 if (in_signature && line[0] == ' ')
>                         sig = line + 1;
> -               else if (starts_with(line, gpg_sig_header) &&
> -                        line[gpg_sig_header_len] == ' ')
> -                       sig = line + gpg_sig_header_len + 1;
> +               else if (indent_line && indent_line[1] == ' ')
> +                       sig = indent_line + 2;
Why is this adding 2 rather than 1?
Show 12 quoted lines
>                 if (sig) {
>                         strbuf_add(signature, sig, next - sig);
>                         saw_signature = 1;
> @@ -1193,10 +1196,8 @@ static void parse_gpg_output(struct signature_check *sigc)
>         for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
>                 const char *found, *next;
>
> -               if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
> -                       /* At the very beginning of the buffer */
> -                       found = buf + strlen(sigcheck_gpg_status[i].check + 1);
> -               } else {
> +               found = skip_prefix(buf, sigcheck_gpg_status[i].check +1);
Add a space after the plus sign.
> +               if(!found) {
>                         found = strstr(buf, sigcheck_gpg_status[i].check);

'found' is being computed again here, even though you already computed it just above via skip_prefix(). This seems pretty wasteful.

Show 11 quoted lines
>                         if (!found)
>                                 continue;
> --
> 1.9.0.138.g2de3478
>
> Hey Eric,
> As per your suggestion in the previous mail :
> http://article.gmane.org/gmane.comp.version-control.git/243316
> I have made necessary changes:
> 1. Better Naming of variables as per suggestion
> 2. Proper replacement of skip_prefix() over starts_with() .

Thanks for explaining the changes since the previous version and providing a link. That's good etiquette.

As Tanay already mentioned in his review, place this commentary just below the "---" line following your sign off. (You placed it below the "--" line.)

Eric Sunshine· Mar 4, 2014, 22:43 UTC · re: Eric Sunshine · lore

Re: [PATCH] [PATCH] commit.c: Replace starts_with() with skip_prefix()

On Tue, Mar 4, 2014 at 5:27 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 22 quoted lines
> On Tue, Mar 4, 2014 at 10:54 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> diff --git a/commit.c b/commit.c
>> index 6bf4fe0..71a03e3 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -1193,10 +1196,8 @@ static void parse_gpg_output(struct signature_check *sigc)
>>         for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
>>                 const char *found, *next;
>>
>> -               if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
>> -                       /* At the very beginning of the buffer */
>> -                       found = buf + strlen(sigcheck_gpg_status[i].check + 1);
>> -               } else {
>> +               found = skip_prefix(buf, sigcheck_gpg_status[i].check +1);
>
> Add a space after the plus sign.
>
>> +               if(!found) {
>>                         found = strstr(buf, sigcheck_gpg_status[i].check);
>
> 'found' is being computed again here, even though you already computed
> it just above via skip_prefix(). This seems pretty wasteful.

Ignore this objection. I must have misread the code as I was composing the email.

Eric Sunshine· Mar 5, 2014, 07:48 UTC · re: Eric Sunshine · lore

Re: [PATCH] [PATCH] commit.c: Replace starts_with() with skip_prefix()

On Tue, Mar 4, 2014 at 5:27 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 20 quoted lines
> On Tue, Mar 4, 2014 at 10:54 AM, Karthik Nayak <karthik.188@gmail.com> wrote:
>> diff --git a/commit.c b/commit.c
>> index 6bf4fe0..71a03e3 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -1111,11 +1114,11 @@ int parse_signed_commit(const unsigned char *sha1,
>>                 char *next = memchr(line, '\n', tail - line);
>>
>>                 next = next ? next + 1 : tail;
>> +               indent_line = skip_prefix(line, gpg_sig_header);
>
> Even stranger variable name for a GPG signature (which has nothing at
> all to do with indentation).
>
>>                 if (in_signature && line[0] == ' ')
>>                         sig = line + 1;
>> -               else if (starts_with(line, gpg_sig_header) &&
>> -                        line[gpg_sig_header_len] == ' ')
>> -                       sig = line + gpg_sig_header_len + 1;
>> +               else if (indent_line && indent_line[1] == ' ')

Also, shouldn't this be checking *indent_line (or indent_line[0]) rather than indent_line[1]?

Show 7 quoted lines
>> +                       sig = indent_line + 2;
>
> Why is this adding 2 rather than 1?
>
>>                 if (sig) {
>>                         strbuf_add(signature, sig, next - sig);
>>                         saw_signature = 1;

← back to recent threads