From: Mark Lodato <lodatom@gmail.com>
Date: Sun, 14 Feb 2010 14:46:35 GMT
Subject: Re: [PATCH] Add an optional argument for --color options
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:
> 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.)

>> 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?

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

>> +     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