threads / patch / 36055

v4commit.c: use skip_prefix() instead of starts_with()

Subject: [PATCH v4] commit.c: use skip_prefix() instead of starts_with()

## tl;dr

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

replies: 4people: 2as markdown or json

Tanay Abhra· Mar 4, 2014, 21:06 UTC · lore

In record_author_date() & parse_gpg_output(), the callers of starts_with() not just want to know if the string starts with the prefix, but also can benefit from knowing the string that follows the prefix.

By using skip_prefix(), we can do both at the same time.
Helped-by: Max Horn <max@quendi.de>
Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
Patch V4 Identation improved, removed useless comment. [1]
		Thanks to Junio C Hamano and Max Horn.
[1] http://article.gmane.org/gmane.comp.version-control.git/243388
Patch V3 Variable naming improved, removed assignments inside conditionals.
        Thanks to Junio C Hamano and Max Horn.
Patch V2 Corrected email formatting ,reapplied the implementation according to suggestions.
        Thanks to Michael Haggerty.
This is in respect to GSoC microproject #10.

In record_author_date(), extra and useless calls to strlen due to using starts_with() were removed by using skip_prefix(). Extra variable "ident_line" was used as "buf" is used in for loop update check.

Other usages of starts_with() in the same file can be found with,
$ grep -n starts_with commit.c

1116: else if (starts_with(line, gpg_sig_header) && 1196: if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {

The starts_with() in line 1116 was left as it is, as strlen values were pre computed as global variables, and replacing may hamper the clarity. The starts_with() in line 1196 was replaced as it abstracts way the skip_prefix part by directly using the function. Also skip_prefix() is inline when compared to starts_with().

 commit.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)
Show changes to commit.c +6 −8
diff --git a/commit.c b/commit.c
index 6bf4fe0..01526f7 100644
--- a/commit.c
+++ b/commit.c
@@ -548,7 +548,7 @@ define_commit_slab(author_date_slab, unsigned long);
 static void record_author_date(struct author_date_slab *author_date,
 			       struct commit *commit)
 {
-	const char *buf, *line_end;
+	const char *buf, *line_end, *ident_line;
 	char *buffer = NULL;
 	struct ident_split ident;
 	char *date_end;
@@ -566,14 +566,14 @@ static void record_author_date(struct author_date_slab *author_date,
 	     buf;
 	     buf = line_end + 1) {
 		line_end = strchrnul(buf, '\n');
-		if (!starts_with(buf, "author ")) {
+		ident_line = skip_prefix(buf, "author ");
+		if (!ident_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 "))) ||
+				    		 ident_line, line_end - ident_line) ||
 		    !ident.date_begin || !ident.date_end)
 			goto fail_exit; /* malformed "author" line */
 		break;
@@ -1193,10 +1193,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
Junio C Hamano· Mar 4, 2014, 21:23 UTC · re: Tanay Abhra · lore

Re: [PATCH v4] commit.c: use skip_prefix() instead of starts_with()

Tanay Abhra <tanayabh@gmail.com> writes:
Show 12 quoted lines
> In record_author_date() & parse_gpg_output(), the callers of
> starts_with() not just want to know if the string starts with the
> prefix, but also can benefit from knowing the string that follows
> the prefix.
>
> By using skip_prefix(), we can do both at the same time.
>
> Helped-by: Max Horn <max@quendi.de>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
> Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Do not add the last when sending; I never signed-off this particular version of this patch.

Show 28 quoted lines
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..01526f7 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -548,7 +548,7 @@ define_commit_slab(author_date_slab, unsigned long);
>  static void record_author_date(struct author_date_slab *author_date,
>  			       struct commit *commit)
>  {
> -	const char *buf, *line_end;
> +	const char *buf, *line_end, *ident_line;
>  	char *buffer = NULL;
>  	struct ident_split ident;
>  	char *date_end;
> @@ -566,14 +566,14 @@ static void record_author_date(struct author_date_slab *author_date,
>  	     buf;
>  	     buf = line_end + 1) {
>  		line_end = strchrnul(buf, '\n');
> -		if (!starts_with(buf, "author ")) {
> +		ident_line = skip_prefix(buf, "author ");
> +		if (!ident_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 "))) ||
> +				    		 ident_line, line_end - ident_line) ||
Funny indentation with some SP followed by HT followed by SP.
Show 16 quoted lines
>  		    !ident.date_begin || !ident.date_end)
>  			goto fail_exit; /* malformed "author" line */
>  		break;
> @@ -1193,10 +1193,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;
Junio C Hamano· Mar 4, 2014, 22:11 UTC · re: Junio C Hamano · lore

Re: [PATCH v4] commit.c: use skip_prefix() instead of starts_with()

Junio C Hamano <gitster@pobox.com> writes:
>> +		found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
>> +		if(!found) {

Missing SP between the control keyword and parenthesized expression the keyword uses.

I've fixed this (and the broken indentation) locally and queued the result to 'pu', so no need to resend to correct this one.

Thanks.
Tanay Abhra· Mar 4, 2014, 22:32 UTC · re: Junio C Hamano · lore

Re: [PATCH v4] commit.c: use skip_prefix() instead of starts_with()

Junio C Hamano <gitster <at> pobox.com> writes:
Show 15 quoted lines
> 
> Junio C Hamano <gitster <at> pobox.com> writes:
> 
> >> +		found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
> >> +		if(!found) {
> 
> Missing SP between the control keyword and parenthesized expression
> the keyword uses.
> 
> I've fixed this (and the broken indentation) locally and queued the
> result to 'pu', so no need to resend to correct this one.
> 
> Thanks.
> 
> 

Sorry about the indentation, it was due to not setting the tab to eight spaces. I found your mail late, so I had already sent a revised patch [1]. Please ignore that mail.

Also , what should be my next step ,should I present a rough draft of a proposal , or tackle other bugs on the mailing list?

Thanks for the suggestions and advice,

Regards, Tanay Abhra.

[1] http://article.gmane.org/gmane.comp.version-control.git/243395
Junio C Hamano· Mar 5, 2014, 22:23 UTC · lore

Re: [PATCH v4] commit.c: use skip_prefix() instead of starts_with()

tanay abhra <tanayabh@gmail.com> writes:
Show 22 quoted lines
> On Wed, Mar 5, 2014 at 3:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>> >> +            found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1);
>> >> +            if(!found) {
>>
>> Missing SP between the control keyword and parenthesized expression
>> the keyword uses.
>>
>> I've fixed this (and the broken indentation) locally and queued the
>> result to 'pu', so no need to resend to correct this one.
>>
>> Thanks.
>>
>>
> Sorry about the indentation, it was due to not setting the tab to eight
> spaces. I found your mail late, so I had already
> sent a revised patch [1]. Please ignore that mail.
>
> Also , what should be my next step ,should I present a rough draft of a
> proposal , or tackle other bugs on the mailing list?

As far as I, as the maintainer of the project, am concerned [*1*], we are done and I expect/require nothing more from you on this change.

The MicroProject page says:
    ... If you've already done a microproject and are itching to do
    more, then get involved in other ways, like finding and fixing
    other problems in the code, or improving the documentation or
    code comments, or helping to review other people's patches on
    the mailing list, or answering questions on the mailing list or
    in IRC, or writing new tests, etc., etc. In short, start doing
    things that other Git developers do!

FYI, [GSoC 2014 timeline] (Google for it) tells us that would-be students are supposed to be discussing project ideas with their mentoring organizations now, in preparation for the actual student application that begins on Mar 10 and ends on Mar 21.

[Footnote]

*1* I should mention that I am not involved in GSoC administration and student selection for the Git project.

← back to recent threads