From: Junio C Hamano <gitster@pobox•com>
To: "Lukas Sandström" <lukass@etek•chalmers.se>
Cc: Git Mailing List <git@vger•kernel.org>
Subject: Re:! [PATCH/RFC] git-mailinfo: use strbuf's instead of fixed buffers
Date: Fri, 11 Jul 2008 23:10:18 -0700 [thread overview]
Message-ID: <7vy747fx9x.fsf_-_@gitster.siamese.dyndns.org> (raw)
In-Reply-To: <48769E91.60205@etek.chalmers.se> (Lukas Sandström's message of "Fri, 11 Jul 2008 01:43:13 +0200")
Lukas Sandström <lukass@etek•chalmers.se> 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).
> -static int bogus_from(char *line)
> +static int bogus_from(const struct strbuf *line)
> {
> /* John Doe <johndoe> */
> - char *bra, *ket, *dst, *cp;
>
> + char *bra, *ket;
> /* This is fallback, so do not bother if we already have an
> * e-mail address.
> */
> - if (*email)
> + if (email.len)
> return 0;
>
> - bra = strchr(line, '<');
> + bra = strchr(line->buf, '<');
> if (!bra)
> return 0;
> ket = strchr(bra, '>');
> if (!ket)
> return 0;
>
> - for (dst = email, cp = bra+1; cp < ket; )
> - *dst++ = *cp++;
> - *dst = 0;
> - for (cp = line; isspace(*cp); cp++)
> - ;
> - for (bra--; isspace(*bra); bra--)
> - *bra = 0;
> - cp = sanity_check(cp, email);
> - strcpy(name, cp);
> + strbuf_reset(&email);
> + strbuf_add(&email, bra + 1, ket - bra - 1);
> +
> + strbuf_reset(&name);
> + strbuf_add(&name, line->buf, bra - line->buf);
> + strbuf_trim(&name);
> + sanity_check(&name, &name, &email);
> 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.
> -static int handle_from(char *in_line)
> +static int handle_from(struct strbuf *from)
> ...
> + el = strcspn(at, " \n\t\r\v\f>");
> + strbuf_reset(&email);
> + strbuf_add(&email, at, el);
> + strbuf_remove(from, at - from->buf, el + 1);
> /* 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.
> + 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?
> + 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?
> -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?
> @@ -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?
I'll review from handle_content_type() til the rest of the file
separately, as my concentration is wearing out..
next prev parent reply other threads:[~2008-07-12 6:11 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2008-07-10 21:41 [PATCH] git-mailinfo: Fix getting the subject from the body Lukas Sandström
[not found] ` <7vod55o0tx.fsf@gitster.siamese.dyndns.org>
2008-07-10 22:37 ` Lukas Sandström
2008-07-10 23:25 ` Junio C Hamano
2008-07-10 23:41 ` [PATCH] Add some useful functions for strbuf manipulation Lukas Sandström
2008-07-10 23:43 ` [PATCH/RFC] git-mailinfo: use strbuf's instead of fixed buffers Lukas Sandström
2008-07-12 6:10 ` Junio C Hamano [this message]
2008-07-13 18:17 ` ! " Lukas Sandström
2008-07-13 18:28 ` [PATCH] Make some strbuf_*() struct strbuf arguments const Lukas Sandström
2008-07-13 18:29 ` [PATCH] Add some useful functions for strbuf manipulation Lukas Sandström
2008-07-13 18:30 ` [PATCH] git-mailinfo: use strbuf's instead of fixed buffers Lukas Sandström
2008-07-13 21:37 ` Junio C Hamano
2008-07-12 9:36 ` [PATCH] git-mailinfo: Fix getting the subject from the body Junio C Hamano
2008-07-12 21:45 ` Lukas Sandström
2008-07-15 3:13 ` Don Zickus
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=7vy747fx9x.fsf_-_@gitster.siamese.dyndns.org \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=lukass@etek$(echo .)chalmers.se \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox