From: Wincent Colaiuta <win@wincent.com>
To: Jean-Luc Herren <jlh@gmx.ch>
Cc: git@vger.kernel.org
Subject: Re: [PATCH] Color support added to git-add--interactive.
Date: Sat, 13 Oct 2007 19:14:19 +0200 [thread overview]
Message-ID: <6E1CBEF5-C2EA-447A-9FED-1423A17C2D19@wincent.com> (raw)
In-Reply-To: <4710F47D.2070306@gmx.ch>
El 13/10/2007, a las 18:38, Jean-Luc Herren escribió:
> Here are my two cents.
>
> Wincent Colaiuta wrote:
>> +sub print_ansi_color($$;$) {
>> + my $color = shift;
>> + my $string = shift;
>> + my $trailer = shift;
>
> None of the other subs in this file have a prototype, so for
> consistency I'd suggest to not add it on this function either.
> However maybe a patch that adds it to all subs would be welcome.
> (I wouldn't see the necessity though.)
Yes, I saw that the other functions didn't use prototypes and I agree
that consistency would be a good thing. I liked the idea of it in
this case because it makes explicit the fact that the function takes
two params plus a third, optional one. So definitely not necessary,
but a preference of mine. In any case, a change to all the other
functions is a question for a separate patch.
> And the common way of getting the arguments is reading @_ (see all
> other subs in the file). So maybe instead write:
>
> [...]
> sub print_ansi_color {
> my ($color, $string, $trailer) = @_;
> [...]
Yes, I actually did write it that way first but then my doubts about
Perl made me write it the longer way; but if they are equivalent then
I prefer the shorter way.
>> + if ($use_color) {
>> + printf '%s%s%s', Term::ANSIColor::color($color), $string,
>> + Term::ANSIColor::color('clear');
>> + } else {
>
> Why use printf when you could directly use print here? It's only
> used for concatenating.
True. I had may brain in the C-world.
>> + if ($trailer) {
>> + print $trailer;
>> + }
>
> This will fail to print $trailer when $trailer happens to be a
> string that evaluates to false in bool context, like '0'. Write
> this as:
>
> if (defined $trailer) {
> print $trailer;
> }
Again, I actually wrote it that way the first time, and then changed
it, this time because I thought they were the same. Like I said, not
a perl hacker.
> IMHO, parsing the output of 'git diff-files --color' is a very bad
> idea and it makes all regexes uglier and more difficult to read.
> You're much better off recolorizing it yourself, which makes it a
> more localized change.
You're probably right, although it is also duplicating the work
that's already done elsewhere. In general I favor making the simplest
change that would work, and tweaking a few of the regexes did look
simpler than re-implementing the colorization logic.
But the approach you suggest might be more robust, perhaps, seeing as
there's not much to the diff output. As far as I can tell there are
really only five or six different things to look for, and they'd be
fairly easy to catch:
- lines beginning with "@@ " (hunk headers)
- lines beginning with "+" (insertions)
- lines beginning with "-" (deletions)
- lines beginning with " " (context lines, no color)
- lines beginning with "\" (things like "\ No newline at end of
file", again, no color)
- everything else; ie. the diff header stuff (eg "diff --git a/foo b/
foo")
The only special cases seem to be the "+++" and "---" lines in the
header, which look like insertions and deletions when they're not.
Trickier would be the highlighting of dubious whitespace, and that's
when it starts to sound like re-inventing the wheel and duplicating
the logic for the detection that's defined elsewhere (possibly in
diff-lib.c? haven't found the exact spot yet).
> Especially, I don't think that you have
> any guarantee that escape sequences won't ever contain the
> characters '+', '-' or ' ' (space)
Yes, that was one of the things I didn't like about the sloppy
regexes. I couldn't really make them any stricter though because I
wasn't confident about the range of possible characters that might be
included in the escape sequences.
> Finally -- and this might be just my eyes -- blue is a very nice
> color, but it looks a bit too dark on black background. Maybe
> choose a default color that looks reasonable on black *and* white
> background.
Yeah, well I didn't choose the colours and I didn't really want to
get into it. Before being considered for inclusion a patch like this
would need to tap in to the existing config settings for color.diff
and color.diff.<slot> anyway...
Wincent
next prev parent reply other threads:[~2007-10-13 17:15 UTC|newest]
Thread overview: 86+ messages / expand[flat|nested] mbox.gz Atom feed top
2007-10-13 4:13 [PATCH] Color support added to git-add--interactive Dan Zwell
2007-10-13 8:12 ` Jeff King
2007-10-13 12:49 ` Frank Lichtenheld
2007-10-13 12:25 ` Johannes Schindelin
2007-10-13 14:45 ` Wincent Colaiuta
2007-10-13 16:38 ` Jean-Luc Herren
2007-10-13 17:14 ` Wincent Colaiuta [this message]
2007-10-13 18:31 ` Andreas Ericsson
2007-10-13 16:38 ` Johannes Schindelin
2007-10-13 17:27 ` Jeff King
2007-10-13 17:51 ` Jeff King
2007-10-13 20:03 ` Dan Zwell
2007-10-13 20:36 ` Wincent Colaiuta
2007-10-13 21:50 ` Dan Z
2007-10-13 22:23 ` Jean-Luc Herren
2007-10-15 3:43 ` Jeff King
2007-10-17 0:47 ` revised: " Dan Zwell
2007-10-17 1:51 ` Shawn O. Pearce
2007-10-17 7:57 ` Dan Zwell
2007-10-17 8:11 ` Shawn O. Pearce
2007-10-22 21:32 ` [PATCH 1/2] Added basic color support to git add --interactive Dan Zwell
2007-10-23 2:11 ` [PATCH] resend of git-add--interactive color patch against spearce/pu Dan Zwell
2007-10-23 2:19 ` [PATCH] Let git-add--interactive read "git colors" from git-config Dan Zwell
2007-10-23 4:29 ` Jeff King
2007-10-23 4:40 ` Shawn O. Pearce
2007-10-23 4:03 ` [PATCH 1/2] Added basic color support to git add --interactive Jeff King
2007-10-23 6:28 ` Wincent Colaiuta
2007-10-23 6:41 ` Jeff King
2007-10-23 7:44 ` Wincent Colaiuta
2007-10-22 21:40 ` [PATCH 2/2] Let git-add--interactive read colors from git-config Dan Zwell
2007-10-23 4:27 ` Jeff King
2007-10-23 8:52 ` Dan Zwell
2007-11-03 3:41 ` [PATCH 1/2] Added basic color support to git add --interactive Dan Zwell
2007-11-04 4:57 ` Jeff King
2007-11-04 5:36 ` Junio C Hamano
2007-11-04 5:43 ` Jeff King
2007-11-11 0:01 ` [PATCH 0/3] Adding colors to git-add--interactive Dan Zwell
2007-11-11 7:54 ` Jeff King
2007-11-11 8:23 ` Junio C Hamano
2007-11-11 8:39 ` Dan Zwell
2007-11-22 10:54 ` [PATCH 0/5] Colors for git-add--interactive Dan Zwell
2007-11-22 11:57 ` Jeff King
2007-11-22 19:20 ` Junio C Hamano
2007-11-22 10:54 ` [PATCH 1/5] Added basic color support to git add --interactive Dan Zwell
2007-11-22 10:55 ` [PATCH 2/5] Don't return 'undef' in case called in a vector context Dan Zwell
2007-11-22 12:06 ` Jeff King
2007-11-22 21:17 ` Junio C Hamano
2007-11-23 4:15 ` Dan Zwell
2007-11-22 10:55 ` [PATCH 3/5] Added config_default($key, $default) to Git.pm Dan Zwell
2007-11-22 12:14 ` Jeff King
2007-11-22 10:56 ` [PATCH 4/5] Let git-add--interactive read colors from configuration Dan Zwell
2007-11-22 12:18 ` Jeff King
2007-11-22 21:28 ` Junio C Hamano
2007-11-22 22:30 ` Jeff King
2007-11-23 5:32 ` Dan Zwell
2007-11-23 9:09 ` Jeff King
2007-11-23 9:17 ` Junio C Hamano
2007-11-22 10:56 ` [PATCH 5/5] Added diff hunk coloring to git-add--interactive Dan Zwell
2007-11-22 12:25 ` Jeff King
2007-11-22 21:37 ` Junio C Hamano
2007-11-23 10:21 ` Jeff King
2007-11-22 22:35 ` Junio C Hamano
2007-11-11 0:01 ` [PATCH 1/3] Added basic color support to git add --interactive Dan Zwell
2007-11-11 0:02 ` [PATCH 2/3] Let git-add--interactive read colors from .gitconfig Dan Zwell
2007-11-11 0:03 ` [PATCH 3/3] Added diff hunk coloring to git-add--interactive Dan Zwell
2007-11-11 10:00 ` Junio C Hamano
2007-11-11 2:21 ` [PATCH 0/3] Adding colors " Dan Zwell
2007-11-11 2:23 ` Subject: [PATCH 1/3] Added basic color support to git add --interactive Dan Zwell
2007-11-11 19:56 ` Junio C Hamano
2007-11-11 2:23 ` Subject: [PATCH 2/3] Let git-add--interactive read colors from .gitconfig Dan Zwell
2007-11-11 9:53 ` Junio C Hamano
2007-11-11 10:34 ` Junio C Hamano
2007-11-13 1:39 ` Dan Zwell
2007-11-13 2:32 ` Junio C Hamano
2007-11-13 2:55 ` Dan Zwell
2007-11-13 7:26 ` Jeff King
2007-11-13 7:29 ` Junio C Hamano
2007-11-13 8:25 ` Dan Zwell
2007-11-13 9:46 ` Jakub Narebski
2007-11-03 3:41 ` [PATCH 2/2] " Dan Zwell
2007-11-03 5:06 ` *[PATCH " Junio C Hamano
2007-11-03 7:26 ` Dan Zwell
2007-11-03 18:11 ` Junio C Hamano
2007-10-15 4:12 ` [PATCH] Color support added to git-add--interactive Jeff King
2007-10-13 20:21 ` Tom Tobin
2007-10-13 20:26 ` Tom Tobin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=6E1CBEF5-C2EA-447A-9FED-1423A17C2D19@wincent.com \
--to=win@wincent.com \
--cc=git@vger.kernel.org \
--cc=jlh@gmx.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).