From: Jeff King <peff@peff•net>
To: Git List <git@vger•kernel.org>
Cc: Junio C Hamano <gitster@pobox•com>, Wink Saville <wink@saville•com>
Subject: [PATCH 04/14] combine-diff: use pointer for parent paths
Date: Thu, 9 Jan 2025 03:42:29 -0500 [thread overview]
Message-ID: <20250109084229.GD2748836@coredump.intra.peff.net> (raw)
In-Reply-To: <20250109082723.GA2748497@coredump.intra.peff.net>
Commit d76ce4f734 (log,diff-tree: add --combined-all-paths option,
2019-02-07) added a "path" field to each combine_diff_parent struct.
It's defined as a strbuf, but this is overkill. We never manipulate the
buffer beyond inserting a single string into it.
And in fact there's a small bug: we zero the parent structs, including
the path strbufs. For the 0th parent, we strbuf_init() the strbuf before
adding to it. But for subsequent parents, we never do the init. This is
technically violating the strbuf API, though the code there is resilient
enough to handle this zero'd state.
This patch switches us to just store an allocated string pointer.
Zeroing it is enough to properly initialize it there (modulo the usual
assumption we make that a NULL pointer is all-zeroes).
And as a bonus, we can just check for a non-NULL value to see if it is
present, rather than repeating the combined_all_paths logic at each
site.
Signed-off-by: Jeff King <peff@peff•net>
---
combine-diff.c | 30 +++++++++++-------------------
diff.h | 2 +-
2 files changed, 12 insertions(+), 20 deletions(-)
diff --git a/combine-diff.c b/combine-diff.c
index 45548fd438..ae3cbfc699 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -60,9 +60,7 @@ static struct combine_diff_path *intersect_paths(
if (combined_all_paths &&
filename_changed(p->parent[n].status)) {
- strbuf_init(&p->parent[n].path, 0);
- strbuf_addstr(&p->parent[n].path,
- q->queue[i]->one->path);
+ p->parent[n].path = xstrdup(q->queue[i]->one->path);
}
*tail = p;
tail = &p->next;
@@ -83,9 +81,7 @@ static struct combine_diff_path *intersect_paths(
/* p->path not in q->queue[]; drop it */
*tail = p->next;
for (j = 0; j < num_parent; j++)
- if (combined_all_paths &&
- filename_changed(p->parent[j].status))
- strbuf_release(&p->parent[j].path);
+ free(p->parent[j].path);
free(p);
continue;
}
@@ -101,8 +97,7 @@ static struct combine_diff_path *intersect_paths(
p->parent[n].status = q->queue[i]->status;
if (combined_all_paths &&
filename_changed(p->parent[n].status))
- strbuf_addstr(&p->parent[n].path,
- q->queue[i]->one->path);
+ p->parent[n].path = xstrdup(q->queue[i]->one->path);
tail = &p->next;
i++;
@@ -987,8 +982,9 @@ static void show_combined_header(struct combine_diff_path *elem,
if (rev->combined_all_paths) {
for (i = 0; i < num_parent; i++) {
- char *path = filename_changed(elem->parent[i].status)
- ? elem->parent[i].path.buf : elem->path;
+ const char *path = elem->parent[i].path ?
+ elem->parent[i].path :
+ elem->path;
if (elem->parent[i].status == DIFF_STATUS_ADDED)
dump_quoted_path("--- ", "", "/dev/null",
line_prefix, c_meta, c_reset);
@@ -1269,12 +1265,10 @@ static void show_raw_diff(struct combine_diff_path *p, int num_parent, struct re
for (i = 0; i < num_parent; i++)
if (rev->combined_all_paths) {
- if (filename_changed(p->parent[i].status))
- write_name_quoted(p->parent[i].path.buf, stdout,
- inter_name_termination);
- else
- write_name_quoted(p->path, stdout,
- inter_name_termination);
+ const char *path = p->parent[i].path ?
+ p->parent[i].path :
+ p->path;
+ write_name_quoted(path, stdout, inter_name_termination);
}
write_name_quoted(p->path, stdout, line_termination);
}
@@ -1636,9 +1630,7 @@ void diff_tree_combined(const struct object_id *oid,
struct combine_diff_path *tmp = paths;
paths = paths->next;
for (i = 0; i < num_parent; i++)
- if (rev->combined_all_paths &&
- filename_changed(tmp->parent[i].status))
- strbuf_release(&tmp->parent[i].path);
+ free(tmp->parent[i].path);
free(tmp);
}
diff --git a/diff.h b/diff.h
index 5cddd5a870..f5f6ea00fb 100644
--- a/diff.h
+++ b/diff.h
@@ -480,7 +480,7 @@ struct combine_diff_path {
char status;
unsigned int mode;
struct object_id oid;
- struct strbuf path;
+ char *path;
} parent[FLEX_ARRAY];
};
#define combine_diff_path_size(n, l) \
--
2.48.0.rc2.413.gc1c80375a3
next prev parent reply other threads:[~2025-01-09 8:42 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-03 19:28 [BUGREPORT] git diff-tree --cc SEGFAUTs Wink Saville
2025-01-03 20:46 ` Jeff King
2025-01-03 23:34 ` Wink Saville
2025-01-04 0:31 ` Jeff King
2025-01-04 2:55 ` Junio C Hamano
2025-01-04 3:32 ` Jeff King
2025-01-04 18:09 ` Wink Saville
2025-01-05 22:13 ` Wink Saville
2025-01-09 8:27 ` [PATCH 0/14] combine-diff cleanups Jeff King
2025-01-09 8:28 ` [PATCH 01/14] run_diff_files(): delay allocation of combine_diff_path Jeff King
2025-01-09 17:57 ` Junio C Hamano
2025-01-09 8:32 ` [PATCH 02/14] combine-diff: add combine_diff_path_new() Jeff King
2025-01-09 18:05 ` Junio C Hamano
2025-01-13 15:40 ` Patrick Steinhardt
2025-01-14 9:29 ` Jeff King
2025-01-09 8:33 ` [PATCH 03/14] tree-diff: clear parent array in path_appendnew() Jeff King
2025-01-09 18:28 ` Junio C Hamano
2025-01-10 10:54 ` Jeff King
2025-01-09 8:42 ` Jeff King [this message]
2025-01-09 18:49 ` [PATCH 04/14] combine-diff: use pointer for parent paths Junio C Hamano
2025-01-09 8:42 ` [PATCH 05/14] diff: add a comment about combine_diff_path.parent.path Jeff King
2025-01-13 15:40 ` Patrick Steinhardt
2025-01-09 8:44 ` [PATCH 06/14] run_diff_files(): de-mystify the size of combine_diff_path struct Jeff King
2025-01-10 16:40 ` Junio C Hamano
2025-01-09 8:46 ` [PATCH 07/14] tree-diff: drop path_appendnew() alloc optimization Jeff King
2025-01-13 15:40 ` Patrick Steinhardt
2025-01-14 10:30 ` Jeff King
2025-01-09 8:49 ` [PATCH 08/14] tree-diff: pass whole path string to path_appendnew() Jeff King
2025-01-13 15:40 ` Patrick Steinhardt
2025-01-14 9:26 ` Jeff King
2025-01-09 8:49 ` [PATCH 09/14] tree-diff: inline path_appendnew() Jeff King
2025-01-11 0:41 ` Junio C Hamano
2025-01-09 8:50 ` [PATCH 10/14] combine-diff: drop public declaration of combine_diff_path_size() Jeff King
2025-01-09 8:51 ` [PATCH 11/14] tree-diff: drop list-tail argument to diff_tree_paths() Jeff King
2025-01-18 0:33 ` Junio C Hamano
2025-01-09 8:53 ` [PATCH 12/14] tree-diff: use the name "tail" to refer to list tail Jeff King
2025-01-09 8:54 ` [PATCH 13/14] tree-diff: simplify emit_path() list management Jeff King
2025-01-09 8:57 ` [PATCH 14/14] tree-diff: make list tail-passing more explicit Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20250109084229.GD2748836@coredump.intra.peff.net \
--to=peff@peff$(echo .)net \
--cc=git@vger$(echo .)kernel.org \
--cc=gitster@pobox$(echo .)com \
--cc=wink@saville$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox