threads / patch / 63864

patchblame: remove parameter detailed in get_commit_info()

Subject: [PATCH] blame: remove parameter detailed in get_commit_info()

## tl;dr

5 messages between Jul 28, 2025 and Jul 29, 2025. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Han Young· Jul 28, 2025, 03:55 UTC · lore

The get_commit_info() function accepts a parameter that can be used to stop the commit parsing early. However, none of the callers use this feature, and testing proved that the performance gain of stopping parsing early is negligible.

Signed-off-by: Han Young <hanyang.tony@bytedance.com>
---
 builtin/blame.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)
Show changes to builtin/blame.c +4 −11
diff --git a/builtin/blame.c b/builtin/blame.c
index 91586e685..dc934abef 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -197,9 +197,7 @@ static void commit_info_destroy(struct commit_info *ci)
 	strbuf_release(&ci->summary);
 }
 
-static void get_commit_info(struct commit *commit,
-			    struct commit_info *ret,
-			    int detailed)
+static void get_commit_info(struct commit *commit, struct commit_info *ret)
 {
 	int len;
 	const char *subject, *encoding;
@@ -211,11 +209,6 @@ static void get_commit_info(struct commit *commit,
 		    &ret->author, &ret->author_mail,
 		    &ret->author_time, &ret->author_tz);
 
-	if (!detailed) {
-		repo_unuse_commit_buffer(the_repository, commit, message);
-		return;
-	}
-
 	get_ac_line(message, "\ncommitter ",
 		    &ret->committer, &ret->committer_mail,
 		    &ret->committer_time, &ret->committer_tz);
@@ -263,7 +256,7 @@ static int emit_one_suspect_detail(struct blame_origin *suspect, int repeat)
 		return 0;
 
 	suspect->commit->object.flags |= METAINFO_SHOWN;
-	get_commit_info(suspect->commit, &ci, 1);
+	get_commit_info(suspect->commit, &ci);
 	printf("author %s\n", ci.author.buf);
 	printf("author-mail %s\n", ci.author_mail.buf);
 	printf("author-time %"PRItime"\n", ci.author_time);
@@ -471,7 +464,7 @@ static void emit_other(struct blame_scoreboard *sb, struct blame_entry *ent, int
 	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
 	const char *default_color = NULL, *color = NULL, *reset = NULL;
 
-	get_commit_info(suspect->commit, &ci, 1);
+	get_commit_info(suspect->commit, &ci);
 	oid_to_hex_r(hex, &suspect->commit->object.oid);
 
 	cp = blame_nth_line(sb, ent->lno);
@@ -665,7 +658,7 @@ static void find_alignment(struct blame_scoreboard *sb, int *option)
 		if (!(suspect->commit->object.flags & METAINFO_SHOWN)) {
 			struct commit_info ci = COMMIT_INFO_INIT;
 			suspect->commit->object.flags |= METAINFO_SHOWN;
-			get_commit_info(suspect->commit, &ci, 1);
+			get_commit_info(suspect->commit, &ci);
 			if (*option & OUTPUT_SHOW_EMAIL)
 				num = utf8_strwidth(ci.author_mail.buf);
 			else

-- 
2.50.0
Patrick Steinhardt· Jul 28, 2025, 06:02 UTC · re: Han Young · lore

Re: [PATCH] blame: remove parameter detailed in get_commit_info()

On Mon, Jul 28, 2025 at 11:55:48AM +0800, Han Young wrote:
> The get_commit_info() function accepts a parameter that can be used to
> stop the commit parsing early.
> However, none of the callers use this feature, and testing proved that
> the performance gain of stopping parsing early is negligible.

Funny enough it doesn't seem like the `detailed` field was ever used. `get_commit_info()` was introduced all the way back in cee7f245dca (git-pickaxe: blame rewritten., 2006-10-19), and even back then all callers passed `1` as the `detailed` parameter.

So this patch looks obviously correct to me, thanks!
Patrick
Junio C Hamano· Jul 28, 2025, 15:40 UTC · re: Patrick Steinhardt · lore

Re: [PATCH] blame: remove parameter detailed in get_commit_info()

Patrick Steinhardt <ps@pks.im> writes:
Show 5 quoted lines
> On Mon, Jul 28, 2025 at 11:55:48AM +0800, Han Young wrote:
>> The get_commit_info() function accepts a parameter that can be used to
>> stop the commit parsing early.
>> However, none of the callers use this feature, and testing proved that
>> the performance gain of stopping parsing early is negligible.
Is it negligible but measurable, or negligible and unmeasurable?
Show 6 quoted lines
> Funny enough it doesn't seem like the `detailed` field was ever used.
> `get_commit_info()` was introduced all the way back in cee7f245dca
> (git-pickaxe: blame rewritten., 2006-10-19), and even back then all
> callers passed `1` as the `detailed` parameter.
>
> So this patch looks obviously correct to me, thanks!
I am all for simplifying.  It is great to see us lose more lines.
Thanks.
Han Young· Jul 29, 2025, 02:50 UTC · re: Junio C Hamano · lore

Re: [External] Re: [PATCH] blame: remove parameter detailed in get_commit_info()

On Mon, Jul 28, 2025 at 11:40 PM Junio C Hamano <gitster@pobox.com> wrote:
> Is it negligible but measurable, or negligible and unmeasurable?

On a 5000-line file with a fairly long history, running "git blame --porcelain FILE" for 100 times, the speedup is less than 1 second. Considering the total run time is 180 seconds, I think it could be system noise. So negligible and unmeasurable.

Thanks.
Junio C Hamano· Jul 29, 2025, 05:02 UTC · re: Han Young · lore

Re: [External] Re: [PATCH] blame: remove parameter detailed in get_commit_info()

Han Young <hanyang.tony@bytedance.com> writes:
Show 6 quoted lines
> On Mon, Jul 28, 2025 at 11:40 PM Junio C Hamano <gitster@pobox.com> wrote:
>> Is it negligible but measurable, or negligible and unmeasurable?
> On a 5000-line file with a fairly long history, running
> "git blame --porcelain FILE" for 100 times, the speedup is less
> than 1 second. Considering the total run time is 180 seconds,
> I think it could be system noise. So negligible and unmeasurable.
OK.  Sounds good.

← back to recent threads