Jim Meyering <[email protected]> writes: > 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?
sure, I will do. I forgot to update "option_help_msgid" in my previous patch which adds --color-scheme to define the colors used by diff. Anyway, I would like an opinion about the new name before I send te full series again. I avoided --colors as it is very similar to --color, but I am not sure how --color-scheme sounds for a native speaker. Regards, Giuseppe
