On Sun, Nov 1, 2015 at 9:06 PM, Jim Meyering <[email protected]> wrote: > On Tue, Oct 20, 2015 at 9:23 AM, Giuseppe Scrivano <[email protected]> wrote: >> Hi Jim, >> >> Jim Meyering <[email protected]> writes: >> >>> Thank you for that patch and for your patience. >> >> thank you very much for the review! >> >>> I've skimmed through and so far have only a question and a request: > ... >>> Why does set_add_color_context call fflush, yet the others do not? >>> Please use fputs rather than fprintf for those literal strings. >>> The former is often far more efficient. >> >> Yes, it shouldn't as well. I have changed it. > > Thanks for the quick work. > I'll try to be quicker, this time. > >>> Finally, should there be some way to specify different colors, >>> e.g., for those who use different-background-colored terminals, >>> or for the color blind? >> >> I have took more code from coreutils ls and diff honors DIFF_COLORS >> now. I added it in a separate patch to facilitate the review. Probably >> all the shared code should end up in a gnulib module, but it probably >> needs a better API before it can happen. >> >> Changes in the first patch: >> >> 1) dropped fflush from set_add_color_context >> 2) use fputs instead of fprintf (the second patch replaces it) >> 3) change the code color of the header to the default color to match >> the "git diff" output. > > Those revisions to your first patch look fine, and I will look over that > one once more in the morning, then expect to push.
Your first patch adds a useful new feature, but includes neither a NEWS addition nor any test. Would you please add those?
