On Sun, Feb 14, 2010 at 6:39 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Mark Lodato <lodatom@xxxxxxxxx> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html