| Message ID | 20210216163151.76307-2-rafaeloliveira.cs@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | blame: remove unnecessary use of get_commit_info() | expand |
On Tue, Feb 16, 2021 at 05:31:51PM +0100, Rafael Silva wrote: > When `git blame --color-by-age`, the determine_line_heat() is called to > select how to color the output based on the commit author's date. It > uses the get_commit_info() to parse the information into a `commit_info` > structure, however, this is actually unnecessary because the > determine_line_heat() caller also does the same. Interesting. It looks like this micro-optimization could have been safely performed as early as 25d5f52901 (builtin/blame: highlight recently changed lines, 2018-04-23), which is when this feature was originally introduced. I looked at 25d5f52901 to see if there was any reason that we didn't at the time, but couldn't find anything. So this looks correct to me. I'm a little disappointed that some of your more detailed performance numbers from the cover letter didn't make it into the patch description, but it may not be worth belaboring the point further. Reviewed-by: Taylor Blau <me@ttaylorr.com> Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > On Tue, Feb 16, 2021 at 05:31:51PM +0100, Rafael Silva wrote: >> When `git blame --color-by-age`, the determine_line_heat() is called to >> select how to color the output based on the commit author's date. It >> uses the get_commit_info() to parse the information into a `commit_info` >> structure, however, this is actually unnecessary because the >> determine_line_heat() caller also does the same. > > Interesting. It looks like this micro-optimization could have been > safely performed as early as 25d5f52901 (builtin/blame: highlight > recently changed lines, 2018-04-23), which is when this feature was > originally introduced. > > I looked at 25d5f52901 to see if there was any reason that we didn't at > the time, but couldn't find anything. > > So this looks correct to me. I'm a little disappointed that some of > your more detailed performance numbers from the cover letter didn't make > it into the patch description, but it may not be worth belaboring the > point further. > > Reviewed-by: Taylor Blau <me@ttaylorr.com> > Thanks for reviewing this patch. I wasn't sure whether adding the performance number into the patch was a good idea or not. After reading your message and the response from Derrick and Junio, I'll definitely re-roll this patch adding those information. > Thanks, > Taylor
diff --git a/builtin/blame.c b/builtin/blame.c index b66e938022..641523ff9a 100644 --- a/builtin/blame.c +++ b/builtin/blame.c @@ -425,13 +425,11 @@ 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) +static void determine_line_heat(struct commit_info *ci, 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) + while (i < colorfield_nr && ci->author_time > colorfield[i].hop) i++; *dest_color = colorfield[i].col; @@ -453,7 +451,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int cp = blame_nth_line(sb, ent->lno); if (opt & OUTPUT_SHOW_AGE_WITH_COLOR) { - determine_line_heat(ent, &default_color); + determine_line_heat(&ci, &default_color); color = default_color; reset = GIT_COLOR_RESET; }
When `git blame --color-by-age`, the determine_line_heat() is called to select how to color the output based on the commit author's date. It uses the get_commit_info() to parse the information into a `commit_info` structure, however, this is actually unnecessary because the determine_line_heat() caller also does the same. Instead, let's change determine_line_heat() to take a `commit_info` structure and remove the internal call to get_commit_info() thus cleaning up and micro-optimizing the code path. Signed-off-by: Rafael Silva <rafaeloliveira.cs@gmail.com> --- builtin/blame.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-)