Junio C Hamano wrote:
> Lukas Sandström <lukass@xxxxxxxxxxxxxxxx> writes:
>
>> -static char *sanity_check(char *name, char *email)
>> +static void sanity_check(struct strbuf *out, struct strbuf *name, struct strbuf *email)
>> {
>> - int len = strlen(name);
>> - if (len < 3 || len > 60)
>> - return email;
>> - if (strchr(name, '@') || strchr(name, '<') || strchr(name, '>'))
>> - return email;
>> - return name;
>> + struct strbuf o = STRBUF_INIT;
>> + if (name->len < 3 || name->len > 60)
>> + strbuf_addbuf(&o, email);
>> + if (strchr(name->buf, '@') || strchr(name->buf, '<') ||
>> + strchr(name->buf, '>'))
>> + strbuf_addbuf(&o, email);
>> + strbuf_addbuf(&o, name);
>> + strbuf_reset(out);
>> + strbuf_addbuf(out, &o);
>> + strbuf_release(&o);
>
> This does not look like a correct conversion. When name is too short or
> too long, we do not even look at name and return email straight. Perhaps
> this would be more faithful conversion:
>
> struct strbuf *src = name;
> if (name->len < 3 ||
> 60 < name->len ||
> strchr(name->buf, '@') ||
> strchr(name->buf, '<') ||
> strchr(name->buf, '>'))
> src = email;
> else if (name == out)
> return;
> strbuf_reset(out);
> strbuf_addbuf(out, src);
>
> It is not your fault, but sanity_check() is a grave misnomer for this
> function. This does "get_sane_name" (i.e. we have name and email but if
> name does not look right, use email instead).
Right. I changed the name and used your implementation.
>> -static int bogus_from(char *line)
>> +static int bogus_from(const struct strbuf *line)
>> {
[ ... ]
>> return 1;
>> }
>
> Conversion looks correct but its return value does not make much sense
> (again, not your fault). bogus_from() is given a bogus looking from line
> (it is not about checking if it is bogus), and returns 0 if we already
> have e-mail address, if the from line does not have bra-ket for grabbing
> e-mail address for, but returns 1 if we managed to get name and email
> pairs. The inconsistency does not matter only because its sole caller
> handle_from() returns its return value, and its caller discards it. We
> may be better off declaring this function and handle_from() as void.
>
Done.
>> /* The remainder is name. It could be "John Doe <john.doe@xz>"
>> * or "john.doe@xz (John Doe)", but we have whited out the
>> * email part, so trim from both ends, possibly removing
>> * the () pair at the end.
>> */
>
> Now, it should read "but we have removed the email part", I think.
Done.
>> + strbuf_trim(from);
>> + if (*from->buf == '(')
>> + strbuf_remove(&name, 0, 1);
>> + if (*(from->buf + from->len - 1) == ')')
>
> Can from be empty at this point before this check?
>
Yes. Added a test for that.
>> + strbuf_setlen(from, from->len - 1);
>> +
>> + sanity_check(&name, from, &email);
>> return 1;
>> }
>
> We used to copy the data from the argument (in_line) before munging it in
> this function, but now we are modifying it in place (from). Does this
> upset our caller, or the original code was just doing an extra unnecessary
> copy?
No, it's ok with the current code, since when handle_from() is called, it's
the last time we look at the header[i] field. I changed it to copy its argument
anyway, for future-proofing.
>
>> -static int handle_header(char *line, char *data, int ofs)
>> +static void handle_header(struct strbuf **out, const struct strbuf *line)
>> {
>> - if (!line || !data)
>> - return 1;
>> -
>> - strcpy(data, line+ofs);
>> + if (!*out) {
>> + *out = xmalloc(sizeof(struct strbuf));
>> + strbuf_init(*out, line->len);
>> + } else
>> + strbuf_reset(*out);
>>
>> - return 0;
>> + strbuf_addbuf(*out, (struct strbuf *)line); /* const warning */
>> }
>
> I think its second parameter can safely become "const struct strbuf *";
> perhaps we should fix the definition of strbuf_addbuf() in your first
> patch?
>
Done.
>> @@ -173,180 +153,176 @@ static int slurp_attr(const char *line, const char *name, char *attr)
>> else
>> ends = "; \t";
>> sz = strcspn(ap, ends);
>> - memcpy(attr, ap, sz);
>> - attr[sz] = 0;
>> + strbuf_add(attr, ap, sz);
>> return 1;
>> }
>>
>> struct content_type {
>> - char *boundary;
>> - int boundary_len;
>> + struct strbuf *boundary;
>> };
>>
>> static struct content_type content[MAX_BOUNDARIES];
>
> Wouldn't it make more sense to get rid of "struct content_type" altogether
> and use "struct strbuf *content[MAX_BOUNDARIES]" directly?
Sure.
I'll send a patch updated with your comments shortly.
/Lukas
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
