RE: [PATCH 1/2] mailinfo: Add support for keep_cr

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



From: Jonathan Tan [mailto:jonathantanmy@xxxxxxxxxx]
> On 01/12/2017 01:20 AM, Matthew Wilcox wrote:
> A test exercising the new functionality would be nice.

Roger.

> Also, maybe a more descriptive title like "mailinfo: also respect
> keep_cr after base64 decode" (50 characters) is better.

No problem.

> > @@ -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.

I wondered why the existing code didn't do that, but I wanted to make a minimal change rather than clean up an older mistake.  I'm happy to do it that way.

> > 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).

Probably best to stick to the existing file ... someone can always do a cleanup patch later, and having this match the others will make that easier, not harder.

Thanks for the review.





[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]