[2/1] mailinfo: don't insert header prefix for handle_content_type()
diff mbox series

Message ID 35ef5149-2da9-a147-fb5b-8f175ff617e7@web.de
State New
Headers show
Series
  • [v2] strbuf: add and use strbuf_insertstr()
Related show

Commit Message

René Scharfe Feb. 10, 2020, 7:15 a.m. UTC
handle_content_type() only cares about the value after "Content-Type: ";
there is no need to insert that string for it.

Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: René Scharfe <l.s.r@web.de>
---
 mailinfo.c | 1 -
 1 file changed, 1 deletion(-)

--
2.25.0

Comments

Junio C Hamano Feb. 10, 2020, 5:27 p.m. UTC | #1
René Scharfe <l.s.r@web.de> writes:

> handle_content_type() only cares about the value after "Content-Type: ";
> there is no need to insert that string for it.
>
> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  mailinfo.c | 1 -
>  1 file changed, 1 deletion(-)

OK.  There is only one caller that just saw that the line it is
looking at begins with "Content-Type:", and the only thing the
helper function does is to look for a handful of attr=value pairs
(without even looking at the primary thing the header wants to
convey---the type itself, like text/plain etc.).  The helper may
want to learn a bit more careful than the current implementation
in doing what it does, but it does not need to see the header name
to do so.  It's not like we will be accepting more than one kind of
content-type like header with this helper and make it capable of
tailoring its behaviour based on which exact header type it is.

Will queue.  Thanks.

> diff --git a/mailinfo.c b/mailinfo.c
> index 543962d40c..402ef04dd1 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -570,7 +570,6 @@ static int check_header(struct mailinfo *mi,
>  		len = strlen("Content-Type: ");
>  		strbuf_add(&sb, line->buf + len, line->len - len);
>  		decode_header(mi, &sb);
> -		strbuf_insertstr(&sb, 0, "Content-Type: ");
>  		handle_content_type(mi, &sb);
>  		ret = 1;
>  		goto check_header_out;
> --
> 2.25.0
Taylor Blau Feb. 10, 2020, 7:55 p.m. UTC | #2
Hi René,

On Mon, Feb 10, 2020 at 08:15:19AM +0100, René Scharfe wrote:
> handle_content_type() only cares about the value after "Content-Type: ";
> there is no need to insert that string for it.
>
> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  mailinfo.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/mailinfo.c b/mailinfo.c
> index 543962d40c..402ef04dd1 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -570,7 +570,6 @@ static int check_header(struct mailinfo *mi,
>  		len = strlen("Content-Type: ");
>  		strbuf_add(&sb, line->buf + len, line->len - len);
>  		decode_header(mi, &sb);
> -		strbuf_insertstr(&sb, 0, "Content-Type: ");

This looks quite good to me. When I first looked, I was wondering if we
should have used 'skip_prefix' and passed that result into
'strbuf_insertstr', but I think that's a bigger change than we need for
now.

This looks all right to me.

>  		handle_content_type(mi, &sb);
>  		ret = 1;
>  		goto check_header_out;
> --
> 2.25.0

Here is my:

  Reviewed-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor

Patch
diff mbox series

diff --git a/mailinfo.c b/mailinfo.c
index 543962d40c..402ef04dd1 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -570,7 +570,6 @@  static int check_header(struct mailinfo *mi,
 		len = strlen("Content-Type: ");
 		strbuf_add(&sb, line->buf + len, line->len - len);
 		decode_header(mi, &sb);
-		strbuf_insertstr(&sb, 0, "Content-Type: ");
 		handle_content_type(mi, &sb);
 		ret = 1;
 		goto check_header_out;