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

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

[Tanay Abhra]

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

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

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

[Junio C Hamano]

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.

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

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

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

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

[Max Horn]

[-- Attachment #1: Type: text/plain, Size: 1718 bytes --]


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?


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;



[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 235 bytes --]

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

[Junio C Hamano]

Max Horn <max@quendi.de> writes:

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

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