Re: [PATCH 2/2] builtin/blame: highlight recently changed lines
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(&l, 0); > } > > static void setup_default_color_by_age(void)
Re: [PATCH 2/2] builtin/blame: highlight recently changed lines
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(&l, 0); > } > > static void setup_default_color_by_age(void) > -- > 2.17.1
Re: [PATCH 2/2] builtin/blame: highlight recently changed lines
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(&l, s, ',', -1); > + ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc); > + > + for_each_string_list_item(item, &l) { > + 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(&l, 0); } static void setup_default_color_by_age(void) -- 2.17.1
Re: [PATCH 2/2] builtin/blame: highlight recently changed lines
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
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
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
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(&l, s, ',', -1); + ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc); + + for_each_string_list_item(item, &l) { + 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, &ci, 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 show_raw_time = !!(op
Re: [PATCH 2/2] builtin/blame: highlight recently changed lines
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
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
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
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
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(&l, s, ',', -1); + ALLOC_GROW(colorfield, colorfield_nr + 1, colorfield_alloc); + + for_each_string_list_item(item, &l) { + 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, &ci, 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, &suspect->commit->object.oid); cp = b