Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Junio C Hamano]

René Scharfe  writes:

>
> This adds a minor memory leak; fix below.
>
> -- >8 --
> Subject: [PATCH] blame: release string_list after use in parse_color_fields()
>
> Signed-off-by: Rene Scharfe 
> ---

Thanks.  Will apply.

>  builtin/blame.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4202584f97..3295718841 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -411,6 +411,7 @@ static void parse_color_fields(const char *s)
>   die (_("must end with a color"));
>  
>   colorfield[colorfield_nr].hop = TIME_MAX;
> + string_list_clear(, 0);
>  }
>  
>  static void setup_default_color_by_age(void)

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Stefan Beller]

On Sat, Jun 9, 2018 at 4:26 AM René Scharfe  wrote:
>
> Am 17.04.2018 um 23:30 schrieb Stefan Beller:
> > +static void parse_color_fields(const char *s)
> > +{
> > + struct string_list l = STRING_LIST_INIT_DUP;
> > + struct string_list_item *item;
> > + enum { EXPECT_DATE, EXPECT_COLOR } next = EXPECT_COLOR;
> > +
> > + colorfield_nr = 0;
> > +
> > + /* Ideally this would be stripped and split at the same time? */
>
> Why?  Both approxidate() and color_parse() handle spaces.

I think that comment is stale; I remember experimenting with
different internal structs to write a parser until I came up with this
implementation. It is a left over.

> This adds a minor memory leak; fix below.

Thanks!

> -- >8 --
> Subject: [PATCH] blame: release string_list after use in parse_color_fields()
>
> Signed-off-by: Rene Scharfe 
> ---
>  builtin/blame.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/builtin/blame.c b/builtin/blame.c
> index 4202584f97..3295718841 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -411,6 +411,7 @@ static void parse_color_fields(const char *s)
> die (_("must end with a color"));
>
> colorfield[colorfield_nr].hop = TIME_MAX;
> +   string_list_clear(, 0);
>  }
>
>  static void setup_default_color_by_age(void)
> --
> 2.17.1

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[René Scharfe]

Am 17.04.2018 um 23:30 schrieb Stefan Beller:
> +static void parse_color_fields(const char *s)
> +{
> + struct string_list l = STRING_LIST_INIT_DUP;
> + struct string_list_item *item;
> + enum { EXPECT_DATE, EXPECT_COLOR } next = EXPECT_COLOR;
> +
> + colorfield_nr = 0;
> +
> + /* Ideally this would be stripped and split at the same time? */

Why?  Both approxidate() and color_parse() handle spaces.

> + string_list_split(, s, ',', -1);
> + ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
> +
> + for_each_string_list_item(item, ) {
> + switch (next) {
> + case EXPECT_DATE:
> + colorfield[colorfield_nr].hop = 
> approxidate(item->string);
> + next = EXPECT_COLOR;
> + colorfield_nr++;
> + ALLOC_GROW(colorfield, colorfield_nr + 1, 
> colorfield_alloc);
> + break;
> + case EXPECT_COLOR:
> + if (color_parse(item->string, 
> colorfield[colorfield_nr].col))
> + die(_("expecting a color: %s"), item->string);
> + next = EXPECT_DATE;
> + break;
> + }
> + }
> +
> + if (next == EXPECT_COLOR)
> + die (_("must end with a color"));
> +
> + colorfield[colorfield_nr].hop = TIME_MAX;
> +}

This adds a minor memory leak; fix below.

-- >8 --
Subject: [PATCH] blame: release string_list after use in parse_color_fields()

Signed-off-by: Rene Scharfe 
---
 builtin/blame.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/blame.c b/builtin/blame.c
index 4202584f97..3295718841 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -411,6 +411,7 @@ static void parse_color_fields(const char *s)
die (_("must end with a color"));
 
colorfield[colorfield_nr].hop = TIME_MAX;
+   string_list_clear(, 0);
 }
 
 static void setup_default_color_by_age(void)
-- 
2.17.1

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Junio C Hamano]

Eric Sunshine  writes:

> On Tue, Apr 17, 2018 at 5:30 PM, Stefan Beller  wrote:
>> Choose a different color for dates and imitate a 'temperature cool down'
>> depending upon age.
>>
>> Originally I had planned to have the temperature cooldown dependent on
>> the age of the project or file for example, as that might scale better,
>> but that can be added on top of this commit, e.g. instead of giving a
>> date, you could imagine giving a percentage that would be the linearly
>> interpolated between now and the beginning of the file.
>>
>> Signed-off-by: Stefan Beller 
>>
>> # Conflicts:
>> #   builtin/blame.c
>
> Meh.

Sloppy.

Will edit it out.

Thanks.

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Junio C Hamano]

Stefan Beller  writes:

> On Mon, Apr 16, 2018 at 8:29 PM, Junio C Hamano  wrote:
>> It seems that this
>>
>> $ git -c color.blame.repeatedlines=cyan blame --heated-lines builtin/blame.c
>>
>> refuses to run.
>>
>> Would it work if the configuration is in .git/config instead, or
>> would it forever disable --heated-lines once somebody choses to use
>> --color-lines feature by default by configuring it in?
>
> That is the unfortunate part of this series, I have not figured out how to
> treat these two options at the same time.


Perhaps I wasn't clear enough, but I did not want to use both at the
same time.  "git blame --color-lines --heated-lines" that errors out
saying these cannot be used at the same time is an acceptable
limitation.

My sole complaint was that just like command line is used to
override (weaker) configs, the wish to use "repeatedlines" painting
by default expressed in the configuration form should be overriden
when there is an explicit command line --heated-lines option that is
incompatible with it.

In this particular case, you might be able to come up with a scheme
where both can be made effective at the same time, but the principle
still stands, and that is the more important lesson I'd like to see
you learn.

Thanks.

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Eric Sunshine]

On Tue, Apr 17, 2018 at 5:30 PM, Stefan Beller  wrote:
> Choose a different color for dates and imitate a 'temperature cool down'
> depending upon age.
>
> Originally I had planned to have the temperature cooldown dependent on
> the age of the project or file for example, as that might scale better,
> but that can be added on top of this commit, e.g. instead of giving a
> date, you could imagine giving a percentage that would be the linearly
> interpolated between now and the beginning of the file.
>
> Signed-off-by: Stefan Beller 
>
> # Conflicts:
> #   builtin/blame.c

Meh.

[PATCH 2/2] builtin/blame: highlight recently changed lines

[Stefan Beller]

Choose a different color for dates and imitate a 'temperature cool down'
depending upon age.

Originally I had planned to have the temperature cooldown dependent on
the age of the project or file for example, as that might scale better,
but that can be added on top of this commit, e.g. instead of giving a
date, you could imagine giving a percentage that would be the linearly
interpolated between now and the beginning of the file.

Signed-off-by: Stefan Beller 

# Conflicts:
#   builtin/blame.c
---
 Documentation/config.txt | 17 +
 builtin/blame.c  | 81 ++--
 t/t8012-blame-colors.sh  | 25 +
 3 files changed, 119 insertions(+), 4 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 8128eee4f9..eae88be662 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1223,6 +1223,23 @@ color.blame.repeatedMeta::
is repeated meta information per line (such as commit id,
author name, date and timezone). Defaults to cyan.
 
+color.blame.highlightRecent::
+   This can be used to color the metadata of a blame line depending
+   on age of the line.
++
+This setting should be set to a comma-separated list of color and date 
settings,
+starting and ending with a color, the dates should be set from oldest to 
newest.
+The metadata will be colored given the colors if the the line was introduced
+before the given timestamp, overwriting older timestamped colors.
++
+Instead of an absolute timestamp relative timestamps work as well, e.g.
+2.weeks.ago is valid to address anything older than 2 weeks.
++
+It defaults to "blue,12 month ago,white,1 month ago,red", which colors
+everything older than one year blue, recent changes between one month and
+one year old are kept white, and lines introduced within the last month are
+colored red.
+
 color.ui::
This variable determines the default value for variables such
as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index 5190ff5df3..53999df511 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -24,6 +24,7 @@
 #include "dir.h"
 #include "progress.h"
 #include "blame.h"
+#include "string-list.h"
 
 static char blame_usage[] = N_("git blame [] [] [] 
[--] ");
 
@@ -323,6 +324,7 @@ static const char *format_time(timestamp_t time, const char 
*tz_str,
 #define OUTPUT_SHOW_EMAIL  0400
 #define OUTPUT_LINE_PORCELAIN  01000
 #define OUTPUT_COLOR_LINE  02000
+#define OUTPUT_SHOW_AGE_WITH_COLOR 04000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -370,6 +372,63 @@ static void emit_porcelain(struct blame_scoreboard *sb, 
struct blame_entry *ent,
putchar('\n');
 }
 
+static struct color_field {
+   timestamp_t hop;
+   char col[COLOR_MAXLEN];
+} *colorfield;
+static int colorfield_nr, colorfield_alloc;
+
+static void parse_color_fields(const char *s)
+{
+   struct string_list l = STRING_LIST_INIT_DUP;
+   struct string_list_item *item;
+   enum { EXPECT_DATE, EXPECT_COLOR } next = EXPECT_COLOR;
+
+   colorfield_nr = 0;
+
+   /* Ideally this would be stripped and split at the same time? */
+   string_list_split(, s, ',', -1);
+   ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
+
+   for_each_string_list_item(item, ) {
+   switch (next) {
+   case EXPECT_DATE:
+   colorfield[colorfield_nr].hop = 
approxidate(item->string);
+   next = EXPECT_COLOR;
+   colorfield_nr++;
+   ALLOC_GROW(colorfield, colorfield_nr + 1, 
colorfield_alloc);
+   break;
+   case EXPECT_COLOR:
+   if (color_parse(item->string, 
colorfield[colorfield_nr].col))
+   die(_("expecting a color: %s"), item->string);
+   next = EXPECT_DATE;
+   break;
+   }
+   }
+
+   if (next == EXPECT_COLOR)
+   die (_("must end with a color"));
+
+   colorfield[colorfield_nr].hop = TIME_MAX;
+}
+
+static void setup_default_color_by_age(void)
+{
+   parse_color_fields("blue,12 month ago,white,1 month ago,red");
+}
+
+static void determine_line_heat(struct blame_entry *ent, const char 
**dest_color)
+{
+   int i = 0;
+   struct commit_info ci;
+   get_commit_info(ent->suspect->commit, , 1);
+
+   while (i < colorfield_nr && ci.author_time > colorfield[i].hop)
+   i++;
+
+   *dest_color = colorfield[i].col;
+}
+
 static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, 
int opt)
 {
int cnt;
@@ -378,12 +437,19 @@ static void emit_other(struct blame_scoreboard *sb, 
struct blame_entry *ent, int
struct commit_info ci;
char hex[GIT_MAX_HEXSZ + 1];
int 

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Stefan Beller]

On Tue, Apr 17, 2018 at 12:31 PM, Stefan Beller  wrote:
> On Mon, Apr 16, 2018 at 8:29 PM, Junio C Hamano  wrote:
>> It seems that this
>>
>> $ git -c color.blame.repeatedlines=cyan blame --heated-lines builtin/blame.c
>>
>> refuses to run.
>>
>> Would it work if the configuration is in .git/config instead, or
>> would it forever disable --heated-lines once somebody choses to use
>> --color-lines feature by default by configuring it in?
>
> That is the unfortunate part of this series, I have not figured out how to
> treat these two options at the same time.
>
> One could take the approach to check the config first and see if there
> are conflicts and then overlay it with the command line options
> (and resolve conflicts there, but CLI taking precedence over config).
>
> Or we'd need to introduce another config
> blame.coloring={none, repeatedlines, highlightrecent}
> which breaks the tie.

Nevermind, we can overlay these color schemes just fine, which I'll do in
a resend.

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Stefan Beller]

On Mon, Apr 16, 2018 at 8:29 PM, Junio C Hamano  wrote:
> It seems that this
>
> $ git -c color.blame.repeatedlines=cyan blame --heated-lines builtin/blame.c
>
> refuses to run.
>
> Would it work if the configuration is in .git/config instead, or
> would it forever disable --heated-lines once somebody choses to use
> --color-lines feature by default by configuring it in?

That is the unfortunate part of this series, I have not figured out how to
treat these two options at the same time.

One could take the approach to check the config first and see if there
are conflicts and then overlay it with the command line options
(and resolve conflicts there, but CLI taking precedence over config).

Or we'd need to introduce another config
blame.coloring={none, repeatedlines, highlightrecent}
which breaks the tie.

Thanks,
Stefan

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Junio C Hamano]

It seems that this

$ git -c color.blame.repeatedlines=cyan blame --heated-lines builtin/blame.c

refuses to run.

Would it work if the configuration is in .git/config instead, or
would it forever disable --heated-lines once somebody choses to use
--color-lines feature by default by configuring it in?

Re: [PATCH 2/2] builtin/blame: highlight recently changed lines

[Junio C Hamano]

Stefan Beller  writes:

> Choose a different color for dates and imitate a 'temperature cool down'
> depending upon age.
>
> Originally I had planned to have the temperature cooldown dependent on
> the age of the project or file for example, as that might scale better,
> but that can be added on top of this commit, e.g. instead of giving a
> date, you could imagine giving a percentage that would be the linearly
> interpolated between now and the beginning of the file.
> ...
> @@ -323,6 +324,7 @@ static const char *format_time(timestamp_t time, const 
> char *tz_str,
>  #define OUTPUT_SHOW_EMAIL0400
>  #define OUTPUT_LINE_PORCELAIN01000
>  #define OUTPUT_COLOR_LINE02000
> +#define OUTPUT_HEATED_LINES  04000

How about calling it OUTPUT_SHOW_AGE_WITH_COLOR or something like
that instead?  Anything with "AGE" in it, if that is what you are
trying to indicate, would be more appropriate than "HEATED", which
does not convey much meaning to readers unless you explain what
determines the temperature in your mind.

[PATCH 2/2] builtin/blame: highlight recently changed lines

[Stefan Beller]

Choose a different color for dates and imitate a 'temperature cool down'
depending upon age.

Originally I had planned to have the temperature cooldown dependent on
the age of the project or file for example, as that might scale better,
but that can be added on top of this commit, e.g. instead of giving a
date, you could imagine giving a percentage that would be the linearly
interpolated between now and the beginning of the file.

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt | 18 ++
 builtin/blame.c  | 78 +++-
 t/t8012-blame-colors.sh  | 25 +
 3 files changed, 120 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index a223232263..acc456e1fd 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1223,6 +1223,24 @@ color.blame.repeatedMeta::
is repeated meta information per line (such as commit id,
author name, date and timezone). Defaults to dark gray.
 
+color.blame.highlightRecent::
+   This can be used to color the author and date of a blame line.
+   This overrides `color.blame.repeatedMeta` setting, which colors
+   repetitions.
++
+This setting should be set to a comma-separated list of color and date 
settings,
+starting and ending with a color, the dates should be set from oldest to 
newest.
+The metadata will be colored given the colors if the the line was introduced
+before the given timestamp, overwriting older timestamped colors.
++
+Instead of an absolute timestamp relative timestamps work as well, e.g.
+2.weeks.ago is valid to address anything older than 2 weeks.
++
+It defaults to "blue,12 month ago,white,1 month ago,red", which colors
+everything older than one year blue, recent changes between one month and
+one year old are kept white, and lines introduced within the last month are
+colored red.
+
 color.ui::
This variable determines the default value for variables such
as `color.diff` and `color.grep` that control the use of color
diff --git a/builtin/blame.c b/builtin/blame.c
index b49fee70a9..9b1021f10d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -24,6 +24,7 @@
 #include "dir.h"
 #include "progress.h"
 #include "blame.h"
+#include "string-list.h"
 
 static char blame_usage[] = N_("git blame [] [] [] 
[--] ");
 
@@ -323,6 +324,7 @@ static const char *format_time(timestamp_t time, const char 
*tz_str,
 #define OUTPUT_SHOW_EMAIL  0400
 #define OUTPUT_LINE_PORCELAIN  01000
 #define OUTPUT_COLOR_LINE  02000
+#define OUTPUT_HEATED_LINES04000
 
 static void emit_porcelain_details(struct blame_origin *suspect, int repeat)
 {
@@ -370,6 +372,63 @@ static void emit_porcelain(struct blame_scoreboard *sb, 
struct blame_entry *ent,
putchar('\n');
 }
 
+static struct color_field {
+   timestamp_t hop;
+   char col[COLOR_MAXLEN];
+} *colorfield;
+static int colorfield_nr, colorfield_alloc;
+
+static void parse_color_fields(const char *s)
+{
+   struct string_list l = STRING_LIST_INIT_DUP;
+   struct string_list_item *item;
+   enum { EXPECT_DATE, EXPECT_COLOR } next = EXPECT_COLOR;
+
+   colorfield_nr = 0;
+
+   /* Ideally this would be stripped and split at the same time? */
+   string_list_split(, s, ',', -1);
+   ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc);
+
+   for_each_string_list_item(item, ) {
+   switch (next) {
+   case EXPECT_DATE:
+   colorfield[colorfield_nr].hop = 
approxidate(item->string);
+   next = EXPECT_COLOR;
+   colorfield_nr++;
+   ALLOC_GROW(colorfield, colorfield_nr + 1, 
colorfield_alloc);
+   break;
+   case EXPECT_COLOR:
+   if (color_parse(item->string, 
colorfield[colorfield_nr].col))
+   die(_("expecting a color: %s"), item->string);
+   next = EXPECT_DATE;
+   break;
+   }
+   }
+
+   if (next == EXPECT_COLOR)
+   die (_("must end with a color"));
+
+   colorfield[colorfield_nr].hop = TIME_MAX;
+}
+
+static void setup_default_colors_heated_lines(void)
+{
+   parse_color_fields("blue,12 month ago,white,1 month ago,red");
+}
+
+static void determine_line_heat(struct blame_entry *ent, const char 
**dest_color)
+{
+   int i = 0;
+   struct commit_info ci;
+   get_commit_info(ent->suspect->commit, , 1);
+
+   while (i < colorfield_nr && ci.author_time > colorfield[i].hop)
+   i++;
+
+   *dest_color = colorfield[i].col;
+}
+
 static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, 
int opt)
 {
int cnt;
@@ -384,6 +443,12 @@ static void emit_other(struct blame_scoreboard *sb, struct 
blame_entry *ent, int
oid_to_hex_r(hex, >commit->object.oid);