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

["Marco Costalba" <mcostalba@gmail.com>]

On Feb 3, 2008 10:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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);



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

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


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