Re: [PATCH RESEND] Avoid a useless prefix lookup in strbuf_expand()

From: Marco Costalba <mcostalba@gmail.com>
Date: Feb 3, 2008, 22:22 UTC
Message-ID: <e5bfff550802031422q7ac72ee4ra53bb4152e34ea29@mail.gmail.com>
In-Reply-To: <7vsl09u2oz.fsf@gitster.siamese.dyndns.org>

On Feb 3, 2008 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> Marco Costalba <mcostalba@gmail.com> writes:
>
> >
> > -static void format_person_part(struct strbuf *sb, char part,
> > +/* returns placeholder length or 0 if placeholder is not known */
>
> That "return placeholder length" is a bit confusing, and I suspect
> the reason may be because the interface is misdesigned.
>
> This function gets only a single character "part" and adds the
> matching information to sb if found, otherwise it doesn't, so
> the only possible return values are 0 or 2.
>
> Wouldn't it be much cleaner if this returned a bool that says "I
> found and substituted that 'part' you asked me to handle"?
>

It happens that _currently_ placeholders that start with 'a' or 'c' have length of 2 chars, so return value can be only 0 or 2, but this is by accident, the return value is really the length of parsed placeholder. Indeed if you see the return value of the caller format_commit_item() is also the length of the parsed placeholder and so should be our format_person_part() whom return value is forwarded by caller:

from format_commit_item()

	case 'a':	/* author ... */
		return format_person_part(sb, placeholder[1],
		                   msg + c->author.off, c->author.len);

Show 16 quoted lines

> >       /*
> >        * If it does not even have a '<' and '>', that is
> >        * quite a bogus commit author and we discard it;
> > @@ -301,65 +303,72 @@ static void format_person_part(struct strbuf *sb, char part,
> >        * which means start (beginning of email address) must
> >        * be strictly below len.
> >        */
> > -     start = end + 1;
> > -     if (start >= len - 1)
> > -             return;
> > -     while (end > 0 && isspace(msg[end - 1]))
> > -             end--;
>
> The comment you can see in the context seems to refer to the
> logic implemented by the part you are rewriting.  Don't you need
> to update it?

Why? if I had written

end_of_data = (end > len - 1);

instead of

end_of_data = (end >= len - 1);

I agree with you comment would have been obsoleted but is not the case.

Show 8 quoted lines

>
> > +     end_of_data = (end >= len - 1);
> > +
>
> The variable name "end_of_data" is unclear.  What does this
> boolean mean?  The line is without address and timestamp?
> The item you are parsing is not properly terminated?
>

It means "we have nothing more to parse here" and we _could_ return now but we keep on because we need to know if the part placeholder is valid or is unkown, so we really need to go through following switch statement before to return with a correct return value.

Show 17 quoted lines

> >       if (part == 'n') {      /* name */
> > -             strbuf_add(sb, msg, end);
> > -             return;
> > +             if (!end_of_data) {
> > +                     while (end > 0 && isspace(msg[end - 1]))
> > +                             end--;
> > +                     strbuf_add(sb, msg, end);
> > +             }
> > +             return 2;
> >       }
> > +     start = ++end; /* save email start position */
>
> What happens if end_of_data was already true in this case, I
> have to wonder...  Language lawyers may point out that the
> result of ++end would be undefined, which I do not personally
> care about in this case, but this feels dirty if not wrong.
>

Could be dirty, but is not wrong because when end_of_data flag as is set as is the case, any use of 'end' variable is skipped, only the following swicth statement is evaluated with the only purpose to compute a valid return value.

Thanks for your review Marco