threads / patch / 36039

v2commit.c: Use skip_prefix() instead of starts_with()

Subject: [PATCH V2] commit.c: Use skip_prefix() instead of starts_with()

## tl;dr

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

replies: 3people: 3as markdown or json

Tanay Abhra· Mar 3, 2014, 15:59 UTC · lore

In record_author_date() & parse_gpg_output() ,using skip_prefix() instead of starts_with() is more elegant and abstracts away the details.

Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Tanay Abhra <tanayabh@gmail.com>
---
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 "skip" 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 generated as global variables. 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 | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)
Show changes to commit.c +7 −6
diff --git a/commit.c b/commit.c
index 6bf4fe0..668c703 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, *skip;
 	char *buffer = NULL;
 	struct ident_split ident;
 	char *date_end;
@@ -566,14 +566,15 @@ 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 ")) {
+		if (!(skip = skip_prefix(buf, "author "))) {
 			if (!line_end[0] || line_end[1] == '\n')
 				return; /* end of header */
 			continue;
 		}
+		buf = skip;
 		if (split_ident_line(&ident,
-				     buf + strlen("author "),
-				     line_end - (buf + strlen("author "))) ||
+				     buf,
+				     line_end - buf) ||
 		    !ident.date_begin || !ident.date_end)
 			goto fail_exit; /* malformed "author" line */
 		break;
@@ -1193,9 +1194,9 @@ 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)) {
+		if (found = skip_prefix(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 = strstr(buf, sigcheck_gpg_status[i].check);
 			if (!found)

-- 
1.9.0
Junio C Hamano· Mar 3, 2014, 19:43 UTC · re: Tanay Abhra · lore

Re: [PATCH V2] commit.c: Use skip_prefix() instead of starts_with()

Tanay Abhra <tanayabh@gmail.com> writes:
> In record_author_date() & parse_gpg_output() ,using skip_prefix() instead of
> starts_with() is more elegant and abstracts away the details.

Avoid subjective judgement like "more elegant" when justifying your change; you are not your own judge.

The caller of starts_with() actually can use the string that follows the expected prefix and that is the reason why using skip_prefix() in these places is a good idea. There is no need to be subjective to justify that change.

I do not think there is any more abstracting away of the details in this change. The updated uses a different and more suitable abstraction than the original.

Show 19 quoted lines
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..668c703 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, *skip;
>  	char *buffer = NULL;
>  	struct ident_split ident;
>  	char *date_end;
> @@ -566,14 +566,15 @@ 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 ")) {
> +		if (!(skip = skip_prefix(buf, "author "))) {
We tend to avoid assignments in conditionals.
Show 13 quoted lines
>  			if (!line_end[0] || line_end[1] == '\n')
>  				return; /* end of header */
>  			continue;
>  		}
> +		buf = skip;
>  		if (split_ident_line(&ident,
> -				     buf + strlen("author "),
> -				     line_end - (buf + strlen("author "))) ||
> +				     buf,
> +				     line_end - buf) ||
>  		    !ident.date_begin || !ident.date_end)
>  			goto fail_exit; /* malformed "author" line */
>  		break;

If you give a sensible name to what 'buf + strlen("author ")' is, then the result becomes a lot more readable compared to the original, and I think that is what this change is about.

And "skip" is not a good name for that. 'but + strlen("author ")' is what split_ident_line() expects its input to be split; let's tentatively call it "ident_line" and see what the call looks like:

	split_ident_line(&ident, ident_line, line_end - ident_line))

And that is what we want to see here. It is a bit more clear than the original that we are splitting the ident information on the line, ident_line (you could call it ident_begin) points at the beginning and line_end points at the end of that ident information.

Use of skip_prefix(), which I am sure you took the name of the new variable "skip" from, is merely an implementation detail of finding where the ident begins. A good rule of thumb to remember is to name things after what they are, not how you obtain them, how they are used or what they are used for/as.

Show 12 quoted lines
> @@ -1193,9 +1194,9 @@ 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)) {
> +		if (found = skip_prefix(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 = strstr(buf, sigcheck_gpg_status[i].check);
>  			if (!found)

This hunk looks good. It can be a separate patch but they are both minor changes so it is OK to have it in a single patch.

Max Horn· Mar 3, 2014, 22:59 UTC · re: Junio C Hamano · lore

Re: [PATCH V2] commit.c: Use skip_prefix() instead of starts_with()

On 03.03.2014, at 20:43, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> Tanay Abhra <tanayabh@gmail.com> writes:
> 
>> @@ -1193,9 +1194,9 @@ 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)) {
>> +		if (found = skip_prefix(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 = strstr(buf, sigcheck_gpg_status[i].check);
>> 			if (!found)
> 
> This hunk looks good.  It can be a separate patch but they are both
> minor changes so it is OK to have it in a single patch.
Hm, but that hunk also introduces an assignment in a conditional, and introduces an empty block. Maybe like this?
Show changes to commit.c +2 −4
diff --git a/commit.c b/commit.c
index 6bf4fe0..0ee0725 100644
--- a/commit.c
+++ b/commit.c
@@ -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 3, 2014, 23:45 UTC · re: Max Horn · lore

Re: [PATCH V2] commit.c: Use skip_prefix() instead of starts_with()

Max Horn <max@quendi.de> writes:
Show 22 quoted lines
> On 03.03.2014, at 20:43, Junio C Hamano <gitster@pobox.com> wrote:
>
>> Tanay Abhra <tanayabh@gmail.com> writes:
>> 
>>> @@ -1193,9 +1194,9 @@ 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)) {
>>> +		if (found = skip_prefix(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 = strstr(buf, sigcheck_gpg_status[i].check);
>>> 			if (!found)
>> 
>> This hunk looks good.  It can be a separate patch but they are both
>> minor changes so it is OK to have it in a single patch.
>
> Hm, but that hunk also introduces an assignment in a conditional,
> and introduces an empty block. Maybe like this?
Much better.

If we anticipate that we may add _more_ ways to find the thing later, then I would say this code structure is better:

	/* Is it at the beginning of the buffer? */
	found = skip_prefix(...);
        if (!found) {
		/* Oh, maybe it is on the second or later line? */
        	found = ... find it on a later line...
	}
	if (!found)
        	continue;

but I do not think that is the case for this particular one. We either try to find it at the very beginning (i.e. no leading newline), or we have some other lines before it (i.e. require leading newline), and there will be no future extension, so what you suggested below looks sensible.

Show 17 quoted lines
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..0ee0725 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -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;

← back to recent threads