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

[Junio C Hamano <gitster@pobox.com>]

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;