Re: [RFC 0/1] mailinfo: de-quote quoted-pair in header fields

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 14, 2016, 00:04 UTC
Message-ID: <xmqqr38ns5wi.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20160913234612.22806-2-me@ikke.info>

Kevin Daudt <me@ikke.info> writes:

Show 6 quoted lines

> +static int unescape_quoted_pair(struct mailinfo *mi, struct strbuf *line)
> +{
> +	struct strbuf outbuf = STRBUF_INIT;
> +	const char *in = line->buf;
> +	int c, skip=0;
> +	char escape_context=0;

Have SP around '=', i.e.

	int c, skip = 0;
	char escape_context = 0;

Show 13 quoted lines

> +	while ((c = *in++) != 0) {
> +		if (!skip) {
> +			switch (c) {
> +				case '"':
> +...
> +					break;
> +			}
> +		} else {
> +			skip = 0;
> +		}
> +
> +		strbuf_addch(&outbuf, c);
> +	}

It often is easier to read if smaller of the two are in the if part and the larger in else part. Also your switch/case is indented one level too deep. I.e.

        while (...) {
                if (skip) {
                        skip = 0;
                } else {
                        switch (c) {
                        case '"':
                                do this;
                                ...
                        }
                }
                strbuf_addch(...);
        }

I found the variable name "skip" a bit hard to reason about. What it does is to signal the next round of the processing that we have seen a single-byte quote and it should keep the byte it will get, no matter what its value is. It is "skipping" the conditional processing, but I'd imagine most people would consider it is "keeping the byte".

Show 8 quoted lines

> @@ -461,6 +506,7 @@ static int check_header(struct mailinfo *mi,
>  			 */
>  			strbuf_add(&sb, line->buf + len + 2, line->len - len - 2);
>  			decode_header(mi, &sb);
> +			unescape_quoted_pair(mi, &sb);
>  			handle_header(&hdr_data[i], &sb);
>  			ret = 1;
>  			goto check_header_out;

I wonder why this call is only in here, not on other headers that all call decode_header(). For that matter, I wonder if the call (or the logic of the helper function itself) should go at the end of decode_header(). After all, this is different kind of decoding; the current one knows how to do b/q encoding but forgot about the more traditional quoting done with backslash, and you are teaching the code that the current decoding it does is insufficient and how to handle the one that the original implementors forgot about.

Thanks.