Re: ! [PATCH/RFC] git-mailinfo: use strbuf's instead of fixed buffers

["Lukas Sandström" <lukass@etek.chalmers.se>]

Junio C Hamano wrote:
> 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).

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