From: Jonathan Tan Date: Thu, 12 Jan 2017 18:04:36 GMT Subject: Re: [PATCH 1/2] mailinfo: Add support for keep_cr Message-ID: <8f2be1e9-9199-44af-9d57-41763cb4d666@google.com> In-Reply-To: <1484212824-14108-1-git-send-email-mawilcox@linuxonhyperv.com> On 01/12/2017 01:20 AM, Matthew Wilcox wrote: > From: Matthew Wilcox > > If you have a base-64 encoded patch with CRLF endings (as produced > by forwarding a patch from Outlook to a Linux machine, for example), > the keep_cr setting is not honoured because keep_cr is only passed > to mailsplit, which does not look through the encoding. The keep_cr > logic needs to be applied after the base64 decode. I copied that > logic to handle_filter(), and rather than add a new keep_cr parameter > to handle_filter, I opted to add keep_cr to struct mailinfo; it seemed > appropriate given use_scissors was already there. > > Then I needed to initialise keep_cr in the struct mailinfo passed from > git-am, and rather than thread a 'keep_cr' parameter all the way through > to parse_mail(), I decided to add keep_cr to struct am_state, which let > it be removed as a parameter from five other functions. > > Signed-off-by: Matthew Wilcox A test exercising the new functionality would be nice. Also, maybe a more descriptive title like "mailinfo: also respect keep_cr after base64 decode" (50 characters) is better. > @@ -143,6 +144,7 @@ static void am_state_init(struct am_state *state, const char *dir) > > memset(state, 0, sizeof(*state)); > > + state->keep_cr = -1; Maybe query the git config here (instead of later) so that we never have to worry about state->keep_cr being neither 0 nor 1? This function already queries the git config anyway. > diff --git a/mailinfo.h b/mailinfo.h > index 04a25351d..9fddcf684 100644 > --- a/mailinfo.h > +++ b/mailinfo.h > @@ -12,6 +12,7 @@ struct mailinfo { > struct strbuf email; > int keep_subject; > int keep_non_patch_brackets_in_subject; > + int keep_cr; > int add_message_id; > int use_scissors; > int use_inbody_headers; I personally would write "unsigned keep_cr : 1" to further emphasize that this can only be 0 or 1, but I don't know if it's better to keep with the style existing in the file (that is, using int).