On Tue, Sep 13, 2016 at 05:04:45PM -0700, Junio C Hamano wrote: > Kevin Daudt <me@xxxxxxxxx> writes: > > > 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. > Thanks, I've switched the order and fixed indentation. > > 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". Yes, I agree and was trying to find a better name. I have renamed it to "take_next_literally", which indicates better what it means. > > > @@ -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. Makes sense, it should be applied to all headers (I missed the other decode_header calls). I will send a new version later.