Re: [PATCH] Add an optional argument for --color options

From: Mark Lodato <lodatom@gmail.com>
Date: Feb 14, 2010, 14:46 UTC
Message-ID: <ca433831002140646v57ce6b1dm4d372d07b0220a16@mail.gmail.com>
In-Reply-To: <7vmxzcoxla.fsf@alter.siamese.dyndns.org>

On Sun, Feb 14, 2010 at 6:39 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 16 quoted lines

> Mark Lodato <lodatom@gmail.com> writes:
>>
>> +`OPT_COLOR_FLAG(short, long, &int_var, description)`::
>> +     Introduce an option that takes an optional argument that can
>> +     have one of three values: "always", "never", or "auto".  If the
>> +     argument is not given, it defaults to "always".  The +--no-+ form
>> +     works like +--long=never+; it cannot take an argument.  If
>> +     "always", set +int_var+ to 1; if "never", set +int_var+ to 0; if
>> +     "auto", set +int_var+ to 1 if stdout is a tty or a pager,
>> +     0 otherwise.
>> +
>
> Everybody else in the vicinity seems to write these like `--something` and
> this new paragraph uses '+--something+'.  Why be original only to be
> different?  Is the mark-up known to be understood by various versions of
> AsciiDoc people use?

In my version of AsciiDoc (8.4.4), backticks were not rendering correctly. Instead they were showing up starting with an opening quote and ending with a grave accent. The same problem was occurring in the other paragraphs, too. I believe the problem occurred whenever there was a single quote on the same line as a backtick: AsciiDoc was reading the first backtick to the first single quote as a quoted string, rather than the first backtick to the second backtick as a literal string. So, I used +'s here to fix it in this paragraph, and then I was going to post a patch to fix the others. But, after I emailed this patch, I realized that the 'html' branch does *not* have this problem. So, I'll change this to backticks in the next version of the patch.

(In case you're wondering: no, there are no single quotes in the above paragraph, but there were in earlier versions. I didn't realize it was a single quote issue until now.)

Show 16 quoted lines

>> diff --git a/color.c b/color.c
>> index 62977f4..790ac91 100644
>> --- a/color.c
>> +++ b/color.c
>> @@ -138,6 +138,9 @@ int git_config_colorbool(const char *var, const char *value, int stdout_is_tty)
>>                       goto auto_color;
>>       }
>>
>> +     /* If var is not given, return an error */
>> +     if (!var)
>> +             return -1;
>
> This is not a good comment for more than one reasons:
>
> [...]
>

You're right, it's a crappy comment. Should I try to reword it, or just leave it out?

Show 20 quoted lines

>> diff --git a/diff.c b/diff.c
>> index 381cc8d..110e63b 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -2826,6 +2826,15 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
>>               DIFF_OPT_SET(options, FOLLOW_RENAMES);
>>       else if (!strcmp(arg, "--color"))
>>               DIFF_OPT_SET(options, COLOR_DIFF);
>> +     else if (!prefixcmp(arg, "--color=")) {
>> +             int value = git_config_colorbool(NULL, arg+8, -1);
>> +             if (value == 0)
>> +                     DIFF_OPT_CLR(options, COLOR_DIFF);
>> +             else if (value > 0)
>> +                     DIFF_OPT_SET(options, COLOR_DIFF);
>> +             else
>> +                     return 0;
>
> Earlier you said "git diff --blorb" says "error: invalid option: --blorb"
> and that is unhelpful, but I do not understand why that justifies to
> silently ignore "git diff --color=bogo".

"git diff --color=bogo" is not silently ignored. A useless message is printed instead.

$ ./git-diff --color=asdf error: invalid option: --color=asdf

I did this because that's what the surrounding code did. Instead, I would have preferred

    return error("option `color' expects \"always\", \"auto\", or \"never\"");

which seems to work.  Is this the right way to go?

> [...]
> missing SP after colon.

Oops.  Fixed.

Show 11 quoted lines

>> +     value = git_config_colorbool(NULL, arg, -1);
>> +     if (value < 0)
>> +             return opterror(opt, "expects \"always\", \"auto\", "
>> +                             "or \"never\"", 0);
>
> Instead of breaking a string into two lines, please write it like this:
>
>                return opterror(opt,
>                        "expects \"always\", \"auto\", or \"never\"", 0);
>
> so that we can grep.

How do you prefer to handle really long lines? Just let them extend past 80 columns? This is what appears to be the convention, but I just want to double check.

I have some other color-related patches that I plan to email today. They are all essentially independent, meaning you can accept some but not others, but I will roll them all into a single PATCHv2 thread so that they stay together. That is, unless you'd prefer them to be separate.

Thanks for the feedback, Mark