[PATCH] Give the hunk comment its own color

[PATCH] Give the hunk comment its own color

[Bert Wesarg]

Insired by the coloring of quilt.

Introduce a separate color for the hunk comment part, i.e. the current function.
Whitespace between hunk header and hunk comment is now printed as plain.

The current default is magenta. But I'm not settled on this. My favorite would
be bold yellow.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>

---
 Documentation/config.txt |    8 +++---
 combine-diff.c           |    5 +++-
 diff.c                   |   62 +++++++++++++++++++++++++++++++++++++++++++--
 diff.h                   |    1 +
 4 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index cb73d75..421cd50 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -598,10 +598,10 @@ color.diff.<slot>::
 	Use customized color for diff colorization.  `<slot>` specifies
 	which part of the patch to use the specified color, and is one
 	of `plain` (context text), `meta` (metainformation), `frag`
-	(hunk header), `old` (removed lines), `new` (added lines),
-	`commit` (commit headers), or `whitespace` (highlighting
-	whitespace errors). The values of these variables may be specified as
-	in color.branch.<slot>.
+	(hunk header), 'func' (function in hunk header), `old` (removed lines),
+	`new` (added lines), `commit` (commit headers), or `whitespace`
+	(highlighting whitespace errors). The values of these variables may be
+	specified as in color.branch.<slot>.
 
 color.grep::
 	When set to `always`, always highlight matches.  When `false` (or
diff --git a/combine-diff.c b/combine-diff.c
index 5b63af1..6162691 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -524,6 +524,7 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
 	int i;
 	unsigned long lno = 0;
 	const char *c_frag = diff_get_color(use_color, DIFF_FRAGINFO);
+	const char *c_func = diff_get_color(use_color, DIFF_FUNCINFO);
 	const char *c_new = diff_get_color(use_color, DIFF_FILE_NEW);
 	const char *c_old = diff_get_color(use_color, DIFF_FILE_OLD);
 	const char *c_plain = diff_get_color(use_color, DIFF_PLAIN);
@@ -588,7 +589,9 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
 				    comment_end = i;
 			}
 			if (comment_end)
-				putchar(' ');
+				printf("%s%s %s%s", c_reset,
+						    c_plain, c_reset,
+						    c_func);
 			for (i = 0; i < comment_end; i++)
 				putchar(hunk_comment[i]);
 		}
diff --git a/diff.c b/diff.c
index 0d7f5ea..8a5ed1b 100644
--- a/diff.c
+++ b/diff.c
@@ -39,6 +39,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_GREEN,	/* NEW */
 	GIT_COLOR_YELLOW,	/* COMMIT */
 	GIT_COLOR_BG_RED,	/* WHITESPACE */
+	GIT_COLOR_MAGENTA,	/* FUNCINFO */
 };
 
 static void diff_filespec_load_driver(struct diff_filespec *one);
@@ -60,6 +61,8 @@ static int parse_diff_color_slot(const char *var, int ofs)
 		return DIFF_COMMIT;
 	if (!strcasecmp(var+ofs, "whitespace"))
 		return DIFF_WHITESPACE;
+	if (!strcasecmp(var+ofs, "func"))
+		return DIFF_FUNCINFO;
 	die("bad config variable '%s'", var);
 }
 
@@ -344,6 +347,61 @@ static void emit_add_line(const char *reset,
 	}
 }
 
+static void emit_hunk_line(struct emit_callback *ecbdata,
+			   const char *line, int len)
+{
+	const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
+	const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
+	const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
+	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
+	const char *part_end = NULL;
+	int part_len = 0;
+
+	/* determine length of @ */
+	while (part_len < len && line[part_len] == '@')
+		part_len++;
+
+	/* find end of frag, (Ie. find second @@) */
+	part_end = memmem(line + part_len, len - part_len,
+			  line, part_len);
+	if (!part_end)
+		return emit_line(ecbdata->file, frag, reset, line, len);
+	/* go to end of @@ */
+	part_end += part_len;
+	/* calculate total length of frag */
+	part_len = part_end - line;
+	/* emit frag */
+	emit_line(ecbdata->file, frag, reset, line, part_len);
+
+	/* consume hunk header */
+	len -= part_len;
+	line += part_len;
+
+	/* return early */
+	if (!len)
+		return;
+
+	/* determine length of sep space */
+	part_len = 0;
+	while (part_len < len && isspace(line[part_len]))
+		part_len++;
+
+	/* no whitespace sep => print reminder as FRAGINFO */
+	if (!part_len)
+		return emit_line(ecbdata->file, frag, reset, line, len);
+
+	/* print whitespace sep as PLAIN */
+	emit_line(ecbdata->file, plain, reset, part_end, part_len);
+
+	/* consume whitespace sep */
+	len -= part_len;
+	line += part_len;
+
+	/* print reminder as FUNCINFO */
+	if (len)
+		emit_line(ecbdata->file, func, reset, line, len);
+}
+
 static struct diff_tempfile *claim_diff_tempfile(void) {
 	int i;
 	for (i = 0; i < ARRAY_SIZE(diff_temp); i++)
@@ -781,9 +839,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 			diff_words_flush(ecbdata);
 		len = sane_truncate_line(ecbdata, line, len);
 		find_lno(line, ecbdata);
-		emit_line(ecbdata->file,
-			  diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO),
-			  reset, line, len);
+		emit_hunk_line(ecbdata, line, len);
 		if (line[len-1] != '\n')
 			putc('\n', ecbdata->file);
 		return;
diff --git a/diff.h b/diff.h
index 2740421..15fcecd 100644
--- a/diff.h
+++ b/diff.h
@@ -130,6 +130,7 @@ enum color_diff {
 	DIFF_FILE_NEW = 5,
 	DIFF_COMMIT = 6,
 	DIFF_WHITESPACE = 7,
+	DIFF_FUNCINFO = 8,
 };
 const char *diff_get_color(int diff_use_color, enum color_diff ix);
 #define diff_get_color_opt(o, ix) \
-- 
tg: (785c58e..) bw/func-color (depends on: master)

Re: [PATCH] Give the hunk comment its own color

[Tay Ray Chuan]

Hi,

On Wed, Nov 18, 2009 at 7:30 PM, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
> Insired by the coloring of quilt.

s/Insired/Inspired/?

-- 
Cheers,
Ray Chuan

Re: [PATCH] Give the hunk comment its own color

[Bert Wesarg]

On Wed, Nov 18, 2009 at 12:44, Tay Ray Chuan <rctay89@gmail.com> wrote:
> Hi,
>
> On Wed, Nov 18, 2009 at 7:30 PM, Bert Wesarg <bert.wesarg@googlemail.com> wrote:
>> Insired by the coloring of quilt.
>
> s/Insired/Inspired/?
Sure.

I also forgot to update the test suit.

New patch follows.

Bert
>
> --
> Cheers,
> Ray Chuan
>

Re: [PATCH] Give the hunk comment its own color

[Jeff King]

On Wed, Nov 18, 2009 at 12:30:36PM +0100, Bert Wesarg wrote:

> Insired by the coloring of quilt.
> 
> Introduce a separate color for the hunk comment part, i.e. the current
> function.  Whitespace between hunk header and hunk comment is now
> printed as plain.
> 
> The current default is magenta. But I'm not settled on this. My
> favorite would be bold yellow.

I don't see any reason not to add this, as it is simply introducing one
extra knob to tweak for people who care. However, after some
experimentation, I found that I don't personally really like it. I ended
up wanting it set to the same color as the hunk header.

I wonder how hard it would be to make it backwards-compatible; that is,
to inherit the color value of the hunk header (be it the original or one
set by the user) unless the func color is set by the user. But maybe
that is over-engineering. It is not like we are breaking scripts, and it
is not that hard for people to see the new behavior and then tweak their
config if they don't like it.

-Peff

PS I almost complained about your default of "magenta" as the same as
the meta color before I remembered that magenta meta is a personal
setting I use. Personally I find the bold meta color to be distractingly
ugly. Blaming it, the default seems to come from Linus, who even in his
commit message (50f575f) seems to indicate that it is somewhat arbitrary
(mostly just dropping the purple from the bold purple).

I'm not sure what is the best way to arrive at a default color for
something like this. Arguing about it really is almost the definition of
bikeshedding.  Maybe next year's git survey should contain a special
section on colors, and majority should rule.  :)

[PATCH v2] Give the hunk comment its own color

[Bert Wesarg]

Inspired by the coloring of quilt.

Introduce a separate color for the hunk comment part, i.e. the current function.
Whitespace between hunk header and hunk comment is now printed as plain.

The current default is magenta. But I'm not settled on this. My favorite would
be bold yellow.

Now with updated test suit.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>

---
 Documentation/config.txt |    8 +++---
 combine-diff.c           |    5 +++-
 diff.c                   |   64 +++++++++++++++++++++++++++++++++++++++++++--
 diff.h                   |    1 +
 t/t4034-diff-words.sh    |    3 +-
 5 files changed, 72 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index cb73d75..421cd50 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -598,10 +598,10 @@ color.diff.<slot>::
 	Use customized color for diff colorization.  `<slot>` specifies
 	which part of the patch to use the specified color, and is one
 	of `plain` (context text), `meta` (metainformation), `frag`
-	(hunk header), `old` (removed lines), `new` (added lines),
-	`commit` (commit headers), or `whitespace` (highlighting
-	whitespace errors). The values of these variables may be specified as
-	in color.branch.<slot>.
+	(hunk header), 'func' (function in hunk header), `old` (removed lines),
+	`new` (added lines), `commit` (commit headers), or `whitespace`
+	(highlighting whitespace errors). The values of these variables may be
+	specified as in color.branch.<slot>.
 
 color.grep::
 	When set to `always`, always highlight matches.  When `false` (or
diff --git a/combine-diff.c b/combine-diff.c
index 5b63af1..6162691 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -524,6 +524,7 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
 	int i;
 	unsigned long lno = 0;
 	const char *c_frag = diff_get_color(use_color, DIFF_FRAGINFO);
+	const char *c_func = diff_get_color(use_color, DIFF_FUNCINFO);
 	const char *c_new = diff_get_color(use_color, DIFF_FILE_NEW);
 	const char *c_old = diff_get_color(use_color, DIFF_FILE_OLD);
 	const char *c_plain = diff_get_color(use_color, DIFF_PLAIN);
@@ -588,7 +589,9 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
 				    comment_end = i;
 			}
 			if (comment_end)
-				putchar(' ');
+				printf("%s%s %s%s", c_reset,
+						    c_plain, c_reset,
+						    c_func);
 			for (i = 0; i < comment_end; i++)
 				putchar(hunk_comment[i]);
 		}
diff --git a/diff.c b/diff.c
index 0d7f5ea..e210525 100644
--- a/diff.c
+++ b/diff.c
@@ -39,6 +39,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_GREEN,	/* NEW */
 	GIT_COLOR_YELLOW,	/* COMMIT */
 	GIT_COLOR_BG_RED,	/* WHITESPACE */
+	GIT_COLOR_MAGENTA,	/* FUNCINFO */
 };
 
 static void diff_filespec_load_driver(struct diff_filespec *one);
@@ -60,6 +61,8 @@ static int parse_diff_color_slot(const char *var, int ofs)
 		return DIFF_COMMIT;
 	if (!strcasecmp(var+ofs, "whitespace"))
 		return DIFF_WHITESPACE;
+	if (!strcasecmp(var+ofs, "func"))
+		return DIFF_FUNCINFO;
 	die("bad config variable '%s'", var);
 }
 
@@ -344,6 +347,63 @@ static void emit_add_line(const char *reset,
 	}
 }
 
+static void emit_hunk_line(struct emit_callback *ecbdata,
+			   const char *line, int len)
+{
+	const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
+	const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
+	const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
+	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
+	const char *orig_line = line;
+	int orig_len = len;
+	const char *frag_start;
+	int frag_len;
+	const char *part_end = NULL;
+	int part_len = 0;
+
+	/* determine length of @ */
+	while (part_len < len && line[part_len] == '@')
+		part_len++;
+
+	/* find end of frag, (Ie. find second @@) */
+	part_end = memmem(line + part_len, len - part_len,
+			  line, part_len);
+	if (!part_end)
+		return emit_line(ecbdata->file, frag, reset, line, len);
+	/* calculate total length of frag */
+	part_len = (part_end + part_len) - line;
+
+	/* remember frag part, we emit only if we find a space separator */
+	frag_start = line;
+	frag_len = part_len;
+
+	/* consume hunk header */
+	len -= part_len;
+	line += part_len;
+
+	/*
+	 * for empty reminder or empty space sequence (exclusive any newlines
+	 * or carriage returns) emit complete original line as FRAGINFO
+	 */
+	if (!len || !(part_len = strspn(line, " \t")))
+		return emit_line(ecbdata->file, frag, reset,
+				 orig_line, orig_len);
+
+	/* now emit the hunk header as FRAGINFO */
+	emit_line(ecbdata->file, frag, reset, frag_start, frag_len);
+
+	/* print whitespace sep as PLAIN */
+	emit_line(ecbdata->file, plain, reset, line, part_len);
+
+	/* consume whitespace sep */
+	len -= part_len;
+	line += part_len;
+
+	/* print reminder as FUNCINFO */
+	if (len)
+		emit_line(ecbdata->file, func, reset, line, len);
+}
+
 static struct diff_tempfile *claim_diff_tempfile(void) {
 	int i;
 	for (i = 0; i < ARRAY_SIZE(diff_temp); i++)
@@ -781,9 +841,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 			diff_words_flush(ecbdata);
 		len = sane_truncate_line(ecbdata, line, len);
 		find_lno(line, ecbdata);
-		emit_line(ecbdata->file,
-			  diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO),
-			  reset, line, len);
+		emit_hunk_line(ecbdata, line, len);
 		if (line[len-1] != '\n')
 			putc('\n', ecbdata->file);
 		return;
diff --git a/diff.h b/diff.h
index 2740421..15fcecd 100644
--- a/diff.h
+++ b/diff.h
@@ -130,6 +130,7 @@ enum color_diff {
 	DIFF_FILE_NEW = 5,
 	DIFF_COMMIT = 6,
 	DIFF_WHITESPACE = 7,
+	DIFF_FUNCINFO = 8,
 };
 const char *diff_get_color(int diff_use_color, enum color_diff ix);
 #define diff_get_color_opt(o, ix) \
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 21db6e9..64a7c38 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -16,6 +16,7 @@ decrypt_color () {
 		-e 's/.\[1m/<WHITE>/g' \
 		-e 's/.\[31m/<RED>/g' \
 		-e 's/.\[32m/<GREEN>/g' \
+		-e 's/.\[35m/<MAGENTA>/g' \
 		-e 's/.\[36m/<BROWN>/g' \
 		-e 's/.\[m/<RESET>/g'
 }
@@ -70,7 +71,7 @@ cat > expect <<\EOF
 <WHITE>+++ b/post<RESET>
 <BROWN>@@ -1 +1 @@<RESET>
 <RED>h(4)<RESET><GREEN>h(4),hh[44]<RESET>
-<BROWN>@@ -3,0 +4,4 @@ a = b + c<RESET>
+<BROWN>@@ -3,0 +4,4 @@<RESET> <RESET><MAGENTA>a = b + c<RESET>
 
 <GREEN>aa = a<RESET>
 
-- 
tg: (785c58e..) bw/func-color (depends on: master)

Re: [PATCH] Give the hunk comment its own color

[Bert Wesarg]

On Wed, Nov 18, 2009 at 15:23, Jeff King <peff@peff.net> wrote:
> PS I almost complained about your default of "magenta" as the same as
> the meta color before I remembered that magenta meta is a personal
> setting I use. Personally I find the bold meta color to be distractingly
> ugly. Blaming it, the default seems to come from Linus, who even in his
> commit message (50f575f) seems to indicate that it is somewhat arbitrary
> (mostly just dropping the purple from the bold purple).
I took magenta, because it is not used as any other default color
value. I think choosing a color other than cyan would bring the
attention to this new feature.

Bert

Re: [PATCH v2] Give the hunk comment its own color

[Jason Sewall]

On Wed, Nov 18, 2009 at 10:11 AM, Bert Wesarg
<bert.wesarg@googlemail.com> wrote:

> Now with updated test suit.

Spelling this as 'suite' would be sweet.

Re: [PATCH v2] Give the hunk comment its own color

[Bert Wesarg]

On Wed, Nov 18, 2009 at 16:17, Jason Sewall <jasonsewall@gmail.com> wrote:
> On Wed, Nov 18, 2009 at 10:11 AM, Bert Wesarg
> <bert.wesarg@googlemail.com> wrote:
>
>> Now with updated test suit.
>
> Spelling this as 'suite' would be sweet.
Thanks. I re-submit only if you find a bug too.

Bert
>

Re: [PATCH] Give the hunk comment its own color

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> PS I almost complained about your default of "magenta" as the same as
> the meta color before I remembered that magenta meta is a personal
> setting I use.

On black-on-white terminals, cyan tends to be less visible, and I think
that is the whole point of painting the hunk header @@ .. @@ in that
color--- make it less distracting). 

But the function name on the line is not something that should be made
less visible---if that part of the line were a meaningless cruft, we
wouldn't have configurable funcname patterns after all.

I would suggest "normal" as the neutral default.  After all, the purpose
of the funcname in the hunk header is to give context to people who read
patches.

> I'm not sure what is the best way to arrive at a default color for
> something like this. Arguing about it really is almost the definition of
> bikeshedding.  Maybe next year's git survey should contain a special
> section on colors, and majority should rule.  :)

Sorry, but this is no democracy ;-)

Re: [PATCH] Give the hunk comment its own color

[Jeff King]

On Wed, Nov 18, 2009 at 01:56:34PM -0800, Junio C Hamano wrote:

> On black-on-white terminals, cyan tends to be less visible, and I think
> that is the whole point of painting the hunk header @@ .. @@ in that
> color--- make it less distracting).

Hmm. I find cyan-on-white gratingly ugly instead of "less distracting",
but then again, I find black-on-white terminals to be eye-searing in
general.

> I would suggest "normal" as the neutral default.  After all, the purpose
> of the funcname in the hunk header is to give context to people who read
> patches.

I think that is sensible.

-Peff

Re: [PATCH] Give the hunk comment its own color

[Bert Wesarg]

Junio,

may I kindly remind you of this patch. If it is only the nen-existing
consensus of the default color, than please use the die.

Kind regards,
Bert Wesarg

Re: [PATCH] Give the hunk comment its own color

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> may I kindly remind you of this patch.

Yes you may ;-)  A more effective would have been a resend but it is
always appreciated.

> ... If it is only the nen-existing
> consensus of the default color, than please use the die.

If you are having me go find the mail and apply I would probably use
"plain" as I suggested.

Re: [PATCH] Give the hunk comment its own color

[Bert Wesarg]

On Fri, Nov 27, 2009 at 03:38, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>> may I kindly remind you of this patch.
>
> Yes you may ;-)  A more effective would have been a resend but it is
> always appreciated.
>
>> ... If it is only the nen-existing
>> consensus of the default color, than please use the die.
>
> If you are having me go find the mail and apply I would probably use
> "plain" as I suggested.
So I resend with plain as default. And therefore saved one resend ;-)

Bert
>

Re: [PATCH] Give the hunk comment its own color

[Jeff King]

On Thu, Nov 26, 2009 at 06:38:55PM -0800, Junio C Hamano wrote:

> > ... If it is only the nen-existing
> > consensus of the default color, than please use the die.
> 
> If you are having me go find the mail and apply I would probably use
> "plain" as I suggested.

As the other person in the discussion, I'll just chime in that I also
think "plain" is the best of the suggested defaults.

-Peff

[PATCH v3] Give the hunk comment its own color

[Bert Wesarg]

Inspired by the coloring of quilt.

Introduce a separate color for the hunk comment part, i.e. the current function.
Whitespace between hunk header and hunk comment is now printed as plain.

The current default is plain.

Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>

---
 Documentation/config.txt |    8 +++---
 combine-diff.c           |    5 +++-
 diff.c                   |   64 +++++++++++++++++++++++++++++++++++++++++++--
 diff.h                   |    1 +
 t/t4034-diff-words.sh    |    4 ++-
 5 files changed, 73 insertions(+), 9 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a8e0876..a1e36d7 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -635,10 +635,10 @@ color.diff.<slot>::
 	Use customized color for diff colorization.  `<slot>` specifies
 	which part of the patch to use the specified color, and is one
 	of `plain` (context text), `meta` (metainformation), `frag`
-	(hunk header), `old` (removed lines), `new` (added lines),
-	`commit` (commit headers), or `whitespace` (highlighting
-	whitespace errors). The values of these variables may be specified as
-	in color.branch.<slot>.
+	(hunk header), 'func' (function in hunk header), `old` (removed lines),
+	`new` (added lines), `commit` (commit headers), or `whitespace`
+	(highlighting whitespace errors). The values of these variables may be
+	specified as in color.branch.<slot>.
 
 color.grep::
 	When set to `always`, always highlight matches.  When `false` (or
diff --git a/combine-diff.c b/combine-diff.c
index 5b63af1..6162691 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -524,6 +524,7 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
 	int i;
 	unsigned long lno = 0;
 	const char *c_frag = diff_get_color(use_color, DIFF_FRAGINFO);
+	const char *c_func = diff_get_color(use_color, DIFF_FUNCINFO);
 	const char *c_new = diff_get_color(use_color, DIFF_FILE_NEW);
 	const char *c_old = diff_get_color(use_color, DIFF_FILE_OLD);
 	const char *c_plain = diff_get_color(use_color, DIFF_PLAIN);
@@ -588,7 +589,9 @@ static void dump_sline(struct sline *sline, unsigned long cnt, int num_parent,
 				    comment_end = i;
 			}
 			if (comment_end)
-				putchar(' ');
+				printf("%s%s %s%s", c_reset,
+						    c_plain, c_reset,
+						    c_func);
 			for (i = 0; i < comment_end; i++)
 				putchar(hunk_comment[i]);
 		}
diff --git a/diff.c b/diff.c
index 0d7f5ea..fd999fb 100644
--- a/diff.c
+++ b/diff.c
@@ -39,6 +39,7 @@ static char diff_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_GREEN,	/* NEW */
 	GIT_COLOR_YELLOW,	/* COMMIT */
 	GIT_COLOR_BG_RED,	/* WHITESPACE */
+	GIT_COLOR_NORMAL,	/* FUNCINFO */
 };
 
 static void diff_filespec_load_driver(struct diff_filespec *one);
@@ -60,6 +61,8 @@ static int parse_diff_color_slot(const char *var, int ofs)
 		return DIFF_COMMIT;
 	if (!strcasecmp(var+ofs, "whitespace"))
 		return DIFF_WHITESPACE;
+	if (!strcasecmp(var+ofs, "func"))
+		return DIFF_FUNCINFO;
 	die("bad config variable '%s'", var);
 }
 
@@ -344,6 +347,63 @@ static void emit_add_line(const char *reset,
 	}
 }
 
+static void emit_hunk_line(struct emit_callback *ecbdata,
+			   const char *line, int len)
+{
+	const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
+	const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
+	const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
+	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
+	const char *orig_line = line;
+	int orig_len = len;
+	const char *frag_start;
+	int frag_len;
+	const char *part_end = NULL;
+	int part_len = 0;
+
+	/* determine length of @ */
+	while (part_len < len && line[part_len] == '@')
+		part_len++;
+
+	/* find end of frag, (Ie. find second @@) */
+	part_end = memmem(line + part_len, len - part_len,
+			  line, part_len);
+	if (!part_end)
+		return emit_line(ecbdata->file, frag, reset, line, len);
+	/* calculate total length of frag */
+	part_len = (part_end + part_len) - line;
+
+	/* remember frag part, we emit only if we find a space separator */
+	frag_start = line;
+	frag_len = part_len;
+
+	/* consume hunk header */
+	len -= part_len;
+	line += part_len;
+
+	/*
+	 * for empty reminder or empty space sequence (exclusive any newlines
+	 * or carriage returns) emit complete original line as FRAGINFO
+	 */
+	if (!len || !(part_len = strspn(line, " \t")))
+		return emit_line(ecbdata->file, frag, reset,
+				 orig_line, orig_len);
+
+	/* now emit the hunk header as FRAGINFO */
+	emit_line(ecbdata->file, frag, reset, frag_start, frag_len);
+
+	/* print whitespace sep as PLAIN */
+	emit_line(ecbdata->file, plain, reset, line, part_len);
+
+	/* consume whitespace sep */
+	len -= part_len;
+	line += part_len;
+
+	/* print reminder as FUNCINFO */
+	if (len)
+		emit_line(ecbdata->file, func, reset, line, len);
+}
+
 static struct diff_tempfile *claim_diff_tempfile(void) {
 	int i;
 	for (i = 0; i < ARRAY_SIZE(diff_temp); i++)
@@ -781,9 +841,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
 			diff_words_flush(ecbdata);
 		len = sane_truncate_line(ecbdata, line, len);
 		find_lno(line, ecbdata);
-		emit_line(ecbdata->file,
-			  diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO),
-			  reset, line, len);
+		emit_hunk_line(ecbdata, line, len);
 		if (line[len-1] != '\n')
 			putc('\n', ecbdata->file);
 		return;
diff --git a/diff.h b/diff.h
index 2740421..15fcecd 100644
--- a/diff.h
+++ b/diff.h
@@ -130,6 +130,7 @@ enum color_diff {
 	DIFF_FILE_NEW = 5,
 	DIFF_COMMIT = 6,
 	DIFF_WHITESPACE = 7,
+	DIFF_FUNCINFO = 8,
 };
 const char *diff_get_color(int diff_use_color, enum color_diff ix);
 #define diff_get_color_opt(o, ix) \
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 21db6e9..186eff8 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -8,6 +8,7 @@ test_expect_success setup '
 
 	git config diff.color.old red
 	git config diff.color.new green
+	git config diff.color.func magenta
 
 '
 
@@ -16,6 +17,7 @@ decrypt_color () {
 		-e 's/.\[1m/<WHITE>/g' \
 		-e 's/.\[31m/<RED>/g' \
 		-e 's/.\[32m/<GREEN>/g' \
+		-e 's/.\[35m/<MAGENTA>/g' \
 		-e 's/.\[36m/<BROWN>/g' \
 		-e 's/.\[m/<RESET>/g'
 }
@@ -70,7 +72,7 @@ cat > expect <<\EOF
 <WHITE>+++ b/post<RESET>
 <BROWN>@@ -1 +1 @@<RESET>
 <RED>h(4)<RESET><GREEN>h(4),hh[44]<RESET>
-<BROWN>@@ -3,0 +4,4 @@ a = b + c<RESET>
+<BROWN>@@ -3,0 +4,4 @@<RESET> <RESET><MAGENTA>a = b + c<RESET>
 
 <GREEN>aa = a<RESET>
 
-- 
tg: (ad7ace7..) bw/func-color (depends on: master)

Re: [PATCH] Give the hunk comment its own color

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Nov 26, 2009 at 06:38:55PM -0800, Junio C Hamano wrote:
>
>> > ... If it is only the nen-existing
>> > consensus of the default color, than please use the die.
>> 
>> If you are having me go find the mail and apply I would probably use
>> "plain" as I suggested.
>
> As the other person in the discussion, I'll just chime in that I also
> think "plain" is the best of the suggested defaults.

Ok, I tweaked the patch locally and applied.

Thanks.

Re: [PATCH v3] Give the hunk comment its own color

[Junio C Hamano]

I was slightly surprised that this seems to have differences other than
the flipping of the default color since the last one, especially after you
sounded like you would be sending with only that change.

Re: [PATCH v3] Give the hunk comment its own color

[Bert Wesarg]

On Fri, Nov 27, 2009 at 09:38, Junio C Hamano <gitster@pobox.com> wrote:
> I was slightly surprised that this seems to have differences other than
> the flipping of the default color since the last one, especially after you
> sounded like you would be sending with only that change.
v3 does only that and adopt the change to the t/t4034-diff-words.sh
test. There I set an explicit value of func to magenta for testing.

Maybe you missed v2 (Message-Id:
<1258557087-31540-1-git-send-email-bert.wesarg@googlemail.com>)? Which
fixed the test and also a small bug.

Bert
>

Re: [PATCH v3] Give the hunk comment its own color

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> Maybe you missed v2 (Message-Id:
> <1258557087-31540-1-git-send-email-bert.wesarg@googlemail.com>)? Which
> fixed the test and also a small bug.

Yeah, that was what happened.  Thanks for clarifying.

Re: [PATCH v3] Give the hunk comment its own color

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

>  diff.c                   |   64 +++++++++++++++++++++++++++++++++++++++++++--
> ...
> @@ -344,6 +347,63 @@ static void emit_add_line(const char *reset,
>  	}
>  }
>  
> +static void emit_hunk_line(struct emit_callback *ecbdata,
> +			   const char *line, int len)
> +{
> +	const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
> +	const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
> +	const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
> +	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
> +	const char *orig_line = line;
> +	int orig_len = len;
> +	const char *frag_start;
> +	int frag_len;
> +	const char *part_end = NULL;
> +	int part_len = 0;
> +
> +	/* determine length of @ */
> +	while (part_len < len && line[part_len] == '@')
> +		part_len++;
> +
> +	/* find end of frag, (Ie. find second @@) */
> +	part_end = memmem(line + part_len, len - part_len,
> +			  line, part_len);

This is not incorrect per-se, but probably is overkill; this codepath only
deals with two-way diff and we know we are looking at "@@ -..., +... @@"
at this point.

	part_end = memmem(line + 2, len - 2, "@@", 2);

would be sufficient.

> +	if (!part_end)
> +		return emit_line(ecbdata->file, frag, reset, line, len);
> +	/* calculate total length of frag */
> +	part_len = (part_end + part_len) - line;
> +
> +	/* remember frag part, we emit only if we find a space separator */
> +	frag_start = line;
> +	frag_len = part_len;
> +
> +	/* consume hunk header */
> +	len -= part_len;
> +	line += part_len;
> +
> +	/*
> +	 * for empty reminder or empty space sequence (exclusive any newlines
> +	 * or carriage returns) emit complete original line as FRAGINFO
> +	 */
> +	if (!len || !(part_len = strspn(line, " \t")))

Slightly worrisome is what guarantees this strspn() won't step outside
len.

I would probably write the function like this instead.

-- >8 --

static void emit_hunk_header(struct emit_callback *ecbdata,
			     const char *line, int len)
{
	const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
	const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
	const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
	const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
	static const char atat[2] = { '@', '@' };
	const char *cp, *ep;

	/*
	 * As a hunk header must begin with "@@ -<old>, +<new> @@",
	 * it always is at least 10 bytes long.
	 */
	if (len < 10 ||
	    memcmp(line, atat, 2) ||
	    !(ep = memmem(line + 2, len - 2, atat, 2))) {
		emit_line(ecbdata->file, plain, reset, line, len);
		return;
	}
	ep += 2; /* skip over the second @@ */

	/* The hunk header in fraginfo color */
	emit_line(ecbdata->file, frag, reset, line, ep - line);

	/* blank before the func header */
	for (cp = ep; ep - line < len; ep++)
		if (*ep != ' ' && *ep != 't')
			break;
	if (ep != cp)
		emit_line(ecbdata->file, plain, reset, cp, ep - cp);

	if (ep < line + len)
		emit_line(ecbdata->file, func, reset, ep, line + len - ep);
}

Re: [PATCH v3] Give the hunk comment its own color

[Bert Wesarg]

On Sat, Nov 28, 2009 at 06:52, Junio C Hamano <gitster@pobox.com> wrote:
> Bert Wesarg <bert.wesarg@googlemail.com> writes:
>
>>  diff.c                   |   64 +++++++++++++++++++++++++++++++++++++++++++--
>> ...
>> @@ -344,6 +347,63 @@ static void emit_add_line(const char *reset,
>>       }
>>  }
>>
>> +static void emit_hunk_line(struct emit_callback *ecbdata,
>> +                        const char *line, int len)
>> +{
>> +     const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
>> +     const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
>> +     const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
>> +     const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
>> +     const char *orig_line = line;
>> +     int orig_len = len;
>> +     const char *frag_start;
>> +     int frag_len;
>> +     const char *part_end = NULL;
>> +     int part_len = 0;
>> +
>> +     /* determine length of @ */
>> +     while (part_len < len && line[part_len] == '@')
>> +             part_len++;
>> +
>> +     /* find end of frag, (Ie. find second @@) */
>> +     part_end = memmem(line + part_len, len - part_len,
>> +                       line, part_len);
>
> This is not incorrect per-se, but probably is overkill; this codepath only
> deals with two-way diff and we know we are looking at "@@ -..., +... @@"
> at this point.
>
>        part_end = memmem(line + 2, len - 2, "@@", 2);
>
> would be sufficient.
Thats right, I made it generic by purpose.

>
>> +     if (!part_end)
>> +             return emit_line(ecbdata->file, frag, reset, line, len);
>> +     /* calculate total length of frag */
>> +     part_len = (part_end + part_len) - line;
>> +
>> +     /* remember frag part, we emit only if we find a space separator */
>> +     frag_start = line;
>> +     frag_len = part_len;
>> +
>> +     /* consume hunk header */
>> +     len -= part_len;
>> +     line += part_len;
>> +
>> +     /*
>> +      * for empty reminder or empty space sequence (exclusive any newlines
>> +      * or carriage returns) emit complete original line as FRAGINFO
>> +      */
>> +     if (!len || !(part_len = strspn(line, " \t")))
>
> Slightly worrisome is what guarantees this strspn() won't step outside
> len.
Thats a valid concern and should be addressed.

>
> I would probably write the function like this instead.
>
> -- >8 --
>
> static void emit_hunk_header(struct emit_callback *ecbdata,
>                             const char *line, int len)
> {
>        const char *plain = diff_get_color(ecbdata->color_diff, DIFF_PLAIN);
>        const char *frag = diff_get_color(ecbdata->color_diff, DIFF_FRAGINFO);
>        const char *func = diff_get_color(ecbdata->color_diff, DIFF_FUNCINFO);
>        const char *reset = diff_get_color(ecbdata->color_diff, DIFF_RESET);
>        static const char atat[2] = { '@', '@' };
>        const char *cp, *ep;
>
>        /*
>         * As a hunk header must begin with "@@ -<old>, +<new> @@",
>         * it always is at least 10 bytes long.
>         */
>        if (len < 10 ||
>            memcmp(line, atat, 2) ||
>            !(ep = memmem(line + 2, len - 2, atat, 2))) {
>                emit_line(ecbdata->file, plain, reset, line, len);
>                return;
>        }
>        ep += 2; /* skip over the second @@ */
>
>        /* The hunk header in fraginfo color */
>        emit_line(ecbdata->file, frag, reset, line, ep - line);
>
>        /* blank before the func header */
>        for (cp = ep; ep - line < len; ep++)
>                if (*ep != ' ' && *ep != 't')
>                        break;
>        if (ep != cp)
>                emit_line(ecbdata->file, plain, reset, cp, ep - cp);
>
>        if (ep < line + len)
>                emit_line(ecbdata->file, func, reset, ep, line + len - ep);
> }
Please check that its really an *ep != '\t'. Its wrong in this mail, I
see only an *ep != 't'. Otherwise:

Acked-by: Bert.Wesarg@googlemail.com
>
>

Re: [PATCH v3] Give the hunk comment its own color

[Bert Wesarg]

>>        /* blank before the func header */
>>        for (cp = ep; ep - line < len; ep++)
>>                if (*ep != ' ' && *ep != 't')
> Please check that its really an *ep != '\t'. Its wrong in this mail, I
> see only an *ep != 't'.
Obviously, you have not checked it. Please squash this in:

diff --git i/diff.c w/diff.c
index eaa1983..e126304 100644
--- i/diff.c
+++ w/diff.c
@@ -376,7 +376,7 @@ static void emit_hunk_header(struct emit_callback *ecbdata,

        /* blank before the func header */
        for (cp = ep; ep - line < len; ep++)
-               if (*ep != ' ' && *ep != 't')
+               if (*ep != ' ' && *ep != '\t')
                        break;
        if (ep != cp)
                emit_line(ecbdata->file, plain, reset, cp, ep - cp);

Bert

Re: [PATCH v3] Give the hunk comment its own color

[Junio C Hamano]

Sorry, but I think the fix is already in 'next', no?

Re: [PATCH v3] Give the hunk comment its own color

[Bert Wesarg]

On Mon, Nov 30, 2009 at 08:15, Junio C Hamano <gitster@pobox.com> wrote:
> Sorry, but I think the fix is already in 'next', no?
Yes it is, should have fetched first. sorry.

Re: [PATCH v3] Give the hunk comment its own color

[Junio C Hamano]

Bert Wesarg <bert.wesarg@googlemail.com> writes:

> On Mon, Nov 30, 2009 at 08:15, Junio C Hamano <gitster@pobox.com> wrote:
>> Sorry, but I think the fix is already in 'next', no?
> Yes it is, should have fetched first. sorry.

Don't be sorry; thanks for catching it.

The current 'next' branch has the original commit with a botched rewrite
by mine, and also a fixed commit, so unfortunately shortlog would list the
patch twice.  I'll merge the updated (i.e. rewound and then rebuilt) tip
of the topic branch when the topic graduates to the master (hopefully
before 1.6.6-rc1), so we won't see the botched one in the end result.

Re: [PATCH v3] Give the hunk comment its own color

[Sverre Rabbelier]

Heya,

On Mon, Nov 30, 2009 at 08:47, Junio C Hamano <gitster@pobox.com> wrote:
> I'll merge the updated (i.e. rewound and then rebuilt) tip
> of the topic branch when the topic graduates to the master (hopefully
> before 1.6.6-rc1), so we won't see the botched one in the end result.

I'm curious how you do this. Do you keep a list of replacements, that
is "when merging branch foo from next to master, instead merge bar",
or is it something the original author should remind you of when it's
time to merge to master?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v3] Give the hunk comment its own color

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> On Mon, Nov 30, 2009 at 08:47, Junio C Hamano <gitster@pobox.com> wrote:
>
>> I'll merge the updated (i.e. rewound and then rebuilt) tip
>> of the topic branch when the topic graduates to the master (hopefully
>> before 1.6.6-rc1), so we won't see the botched one in the end result.
>
> I'm curious how you do this. Do you keep a list of replacements, that
> is "when merging branch foo from next to master, instead merge bar",
> or is it something the original author should remind you of when it's
> time to merge to master?

If you run "git log --oneline --first-parent master..pu", you will notice
that there is no "Merge ... into next" at all.

I maintain a private 'jch' branch that merges everything that has been
merged so far to 'next' and the branch always builds on top of 'master'
whenever 'pu' is pushed out.  The tree object recorded by the tip of 'jch'
is designed to always match that of 'next'.  And 'pu' is built on top of
'jch', instead of 'next', these days.

The Reintegrate script fron 'todo' (recall that I have a checkout of the
branch in "Meta/" directory) is used this way:

    ... update 'jch' by merging topics that had new commits
    $ git checkout jch && git merge xx/topic && ...
    ... update the list of topics
    $ Meta/Reintegrate master..jch >/var/tmp/redo-jch.sh

    ... update 'master' with commits and merges
    $ git checkout master
    $ git am trivially-correct-patch.mbox
    $ git merge yy/topic

    ... update 'next'
    $ git checkout next
    $ git merge master
    ... will merge new topics and topics that had new commits
    $ sh /var/tmp/redo-jch.sh

    ... rebuild 'jch' on top of the updated master
    $ git checkout jch
    $ git reset --hard master
    $ sh /var/tmp/redo-jch.sh

    ... then this shouldn't produce any output
    $ git diff next

This time, what I did _after_ Bert noticed my typo was:

    $ git checkout bw/diff-color-hunk-header
    $ edit diff.c ;# fix my typo
    $ git commit --amend -a
    $ git diff HEAD@{1} >P.diff
    $ git checkout next
    $ git apply --index P.diff
    $ git commit -m 'typofix'

After this, while rebuilding 'jch' branch the next time, Reintegrate
script will notice that the bw/diff-color-hunk-header topic has been
rebased.  I can simply edit that note out in the /var/tmp/redo-jch.sh
script and rebuild 'jch' branch---the result will exactly match 'next'.

The end result is that the commit merged in 'next' is not the tip of
bw/diff-color-hunk-header anymore, but merging the _current_ tip of that
branch together with all the other topics on top of 'master' would produce
the desired result without "oops---that was a stupid typo" fixups.

Re: [PATCH v3] Give the hunk comment its own color

[Sverre Rabbelier]

Heya,

On Mon, Nov 30, 2009 at 10:00, Junio C Hamano <gitster@pobox.com> wrote:
> I maintain a private 'jch' branch that merges everything that has been
> merged so far to 'next' and the branch always builds on top of 'master'
> whenever 'pu' is pushed out.  The tree object recorded by the tip of 'jch'
> is designed to always match that of 'next'.  And 'pu' is built on top of
> 'jch', instead of 'next', these days.

Ah, so _that''s_ how you pull of not rebasing and still maintaining a
clean history in master: keeping a private shadow branch with the
cleaned up history that is tree-identical to the no-rebase next
branch. Interesting.


> The end result is that the commit merged in 'next' is not the tip of
> bw/diff-color-hunk-header anymore, but merging the _current_ tip of that
> branch together with all the other topics on top of 'master' would produce
> the desired result without "oops---that was a stupid typo" fixups.

Thanks for explaining, git really does allow for a lot of interesting
workflows :).

-- 
Cheers,

Sverre Rabbelier