On Sat, Nov 19, 2011 at 7:26 AM, Majic <[email protected]> wrote: [...] > So for the last time (hopefully), here are the patches in the order > they should be applied: > > 1) 0001-Deduplicate-the-warn-in-in-color_init_unchecked.patch
I think displaying the warning is best left to `color_init_unchecked`. `color_init_unchecked` needs `colstr` to be parsed, so it should be the one to decide what to do in the event of a failure -- display a warning, set `req.has_error` to true, etc. > if(len == 0 || !color_parse(colstr, len, &red, &green, &blue)) Since `color_parse` checks for the appropriate value of `len` (which is 7), I do not see a reason to pre-check it, except maybe to avoid an unnecessary call to `color_parse`, which your version doesn't (the original version did). I don't see how re-ordering the rest of the statements (`req.has_error = false` and `req.colstr = colstr`) aligns with the subject of the patch. It should be a separate commit, if needed at all. > 2) 0001-Refine-RGB_-macros-use-RGB_16TO8-in-luaA_push_color.patch I honestly feel that it should be split into two like before. You should consider using git rebase to iteratively improve your patches, after a peer review perhaps. And git format-patch to make it easier for upstream to track (and blame) your contribution. -- Anurag Priyam -- To unsubscribe, send mail to [email protected].
