From 706779344155823518745a19515601905877c41f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 Jan 2025 03:32:36 -0500 Subject: combine-diff: add combine_diff_path_new() The combine_diff_path struct has variable size, since it embeds both the memory allocation for the path field as well as a variable-sized parent array. This makes allocating one a bit tricky. We have a helper to compute the required size, but it's up to individual sites to actually initialize all of the fields. Let's provide a constructor function to make that a little nicer. Besides being shorter, it also hides away tricky bits like the computation of the "path" pointer (which is right after the "parent" flex array). As a bonus, using the same constructor everywhere means that we'll consistently initialize all parts of the struct. A few code paths left the parent array unitialized. This didn't cause any bugs, but we'll be able to simplify some code in the next few patches knowing that the parent fields have all been zero'd. This also gets rid of some questionable uses of "int" to store buffer lengths. Though we do use them to allocate, I don't think there are any integer overflow vulnerabilities here (the allocation helper promotes them to size_t and checks arithmetic for overflow, and the actual memcpy of the bytes is done using the possibly-truncated "int" value). Sadly we can't use the FLEX_* macros to simplify the allocation here, because there are two variable-sized parts to the struct (and those macros only handle one). Nor can we get stop publicly declaring combine_diff_path_size(). This patch does not touch the code in path_appendnew() at all, which is not ready to be moved to our new constructor for a few reasons: - path_appendnew() has a memory-reuse optimization where it tries to reuse combine_diff_path structs rather than freeing and reallocating. - path_appendnew() does not create the struct from a single path string, but rather allocates and copies into the buffer from multiple sources. These can be addressed by some refactoring, but let's leave it as-is for now. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.h | 5 +++++ 1 file changed, 5 insertions(+) (limited to 'diff.h') diff --git a/diff.h b/diff.h index 6e6007c17b..5cddd5a870 100644 --- a/diff.h +++ b/diff.h @@ -486,6 +486,11 @@ struct combine_diff_path { #define combine_diff_path_size(n, l) \ st_add4(sizeof(struct combine_diff_path), (l), 1, \ st_mult(sizeof(struct combine_diff_parent), (n))) +struct combine_diff_path *combine_diff_path_new(const char *path, + size_t path_len, + unsigned int mode, + const struct object_id *oid, + size_t num_parents); void show_combined_diff(struct combine_diff_path *elem, int num_parent, struct rev_info *); -- cgit v1.3 From 3a0599788fd38822dcd2f32de538afdd36a478aa Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 Jan 2025 03:42:29 -0500 Subject: combine-diff: use pointer for parent paths 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 Signed-off-by: Junio C Hamano --- combine-diff.c | 30 +++++++++++------------------- diff.h | 2 +- 2 files changed, 12 insertions(+), 20 deletions(-) (limited to 'diff.h') 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) \ -- cgit v1.3 From 30f7414ca17bc675105d3d731a827778d7367b11 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 Jan 2025 03:42:48 -0500 Subject: diff: add a comment about combine_diff_path.parent.path We only fill in the per-parent "path" field when it differs from what's in combine_diff_path.path (and even then only when the option is appropriate). Let's document that. Suggested-by: Wink Saville Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- diff.h | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'diff.h') diff --git a/diff.h b/diff.h index f5f6ea00fb..60e7db4ad6 100644 --- a/diff.h +++ b/diff.h @@ -480,6 +480,12 @@ struct combine_diff_path { char status; unsigned int mode; struct object_id oid; + /* + * This per-parent path is filled only when doing a combined + * diff with revs.combined_all_paths set, and only if the path + * differs from the post-image (e.g., a rename or copy). + * Otherwise it is left NULL. + */ char *path; } parent[FLEX_ARRAY]; }; -- cgit v1.3 From 69f6dea44cf272dc80be6dffd0ac8db5c50585b4 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 Jan 2025 03:50:19 -0500 Subject: combine-diff: drop public declaration of combine_diff_path_size() We want callers to use combine_diff_path_new() to allocate structs, rather than using combine_diff_path_size() and xmalloc(). That gives us more consistency over the initialization of the fields. Now that the final external user of combine_diff_path_size() is gone, we can stop declaring it publicly. And since our constructor is the only caller, we can just inline it there. Breaking the size computation into two parts also lets us reuse the intermediate multiplication result of the parent length, since we need to know it to perform our memset(). The result is a little easier to read. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- combine-diff.c | 5 +++-- diff.h | 3 --- 2 files changed, 3 insertions(+), 5 deletions(-) (limited to 'diff.h') diff --git a/combine-diff.c b/combine-diff.c index ae3cbfc699..f21e1f58ba 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1658,8 +1658,9 @@ struct combine_diff_path *combine_diff_path_new(const char *path, size_t num_parents) { struct combine_diff_path *p; + size_t parent_len = st_mult(sizeof(p->parent[0]), num_parents); - p = xmalloc(combine_diff_path_size(num_parents, path_len)); + p = xmalloc(st_add4(sizeof(*p), path_len, 1, parent_len)); p->path = (char *)&(p->parent[num_parents]); memcpy(p->path, path, path_len); p->path[path_len] = 0; @@ -1667,7 +1668,7 @@ struct combine_diff_path *combine_diff_path_new(const char *path, p->mode = mode; oidcpy(&p->oid, oid); - memset(p->parent, 0, sizeof(p->parent[0]) * num_parents); + memset(p->parent, 0, parent_len); return p; } diff --git a/diff.h b/diff.h index 60e7db4ad6..32ad17fd38 100644 --- a/diff.h +++ b/diff.h @@ -489,9 +489,6 @@ struct combine_diff_path { char *path; } parent[FLEX_ARRAY]; }; -#define combine_diff_path_size(n, l) \ - st_add4(sizeof(struct combine_diff_path), (l), 1, \ - st_mult(sizeof(struct combine_diff_parent), (n))) struct combine_diff_path *combine_diff_path_new(const char *path, size_t path_len, unsigned int mode, -- cgit v1.3 From a5c4e31af9b8b8fb362472ce3a1ec404df0da032 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Thu, 9 Jan 2025 03:51:56 -0500 Subject: tree-diff: drop list-tail argument to diff_tree_paths() The internals of the path diffing code, including ll_diff_tree_paths(), all take an extra combine_diff_path parameter which they use as the tail of a list of results, appending any new entries to it. The public-facing diff_tree_paths() takes the same argument, but it just makes the callers more awkward. They always start with a clean list, and have to set up a fake head struct to pass in. Let's keep the public API clean by always returning a new list. That keeps the fake struct as an implementation detail of tree-diff.c. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- combine-diff.c | 9 +++------ diff.h | 2 +- tree-diff.c | 14 ++++++++------ 3 files changed, 12 insertions(+), 13 deletions(-) (limited to 'diff.h') diff --git a/combine-diff.c b/combine-diff.c index f21e1f58ba..9527f3160d 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1428,22 +1428,19 @@ static struct combine_diff_path *find_paths_multitree( { int i, nparent = parents->nr; const struct object_id **parents_oid; - struct combine_diff_path paths_head; + struct combine_diff_path *paths; struct strbuf base; ALLOC_ARRAY(parents_oid, nparent); for (i = 0; i < nparent; i++) parents_oid[i] = &parents->oid[i]; - /* fake list head, so worker can assume it is non-NULL */ - paths_head.next = NULL; - strbuf_init(&base, PATH_MAX); - diff_tree_paths(&paths_head, oid, parents_oid, nparent, &base, opt); + paths = diff_tree_paths(oid, parents_oid, nparent, &base, opt); strbuf_release(&base); free(parents_oid); - return paths_head.next; + return paths; } static int match_objfind(struct combine_diff_path *path, diff --git a/diff.h b/diff.h index 32ad17fd38..7831ed1a2b 100644 --- a/diff.h +++ b/diff.h @@ -462,7 +462,7 @@ const char *diff_line_prefix(struct diff_options *); extern const char mime_boundary_leader[]; struct combine_diff_path *diff_tree_paths( - struct combine_diff_path *p, const struct object_id *oid, + const struct object_id *oid, const struct object_id **parents_oid, int nparent, struct strbuf *base, struct diff_options *opt); void diff_tree_oid(const struct object_id *old_oid, diff --git a/tree-diff.c b/tree-diff.c index 18e5a16716..e99e40da18 100644 --- a/tree-diff.c +++ b/tree-diff.c @@ -510,11 +510,14 @@ static struct combine_diff_path *ll_diff_tree_paths( } struct combine_diff_path *diff_tree_paths( - struct combine_diff_path *p, const struct object_id *oid, + const struct object_id *oid, const struct object_id **parents_oid, int nparent, struct strbuf *base, struct diff_options *opt) { - p = ll_diff_tree_paths(p, oid, parents_oid, nparent, base, opt, 0); + struct combine_diff_path head, *p; + /* fake list head, so worker can assume it is non-NULL */ + head.next = NULL; + p = ll_diff_tree_paths(&head, oid, parents_oid, nparent, base, opt, 0); return p; } @@ -631,14 +634,13 @@ static void ll_diff_tree_oid(const struct object_id *old_oid, const struct object_id *new_oid, struct strbuf *base, struct diff_options *opt) { - struct combine_diff_path phead, *p; + struct combine_diff_path *paths, *p; pathchange_fn_t pathchange_old = opt->pathchange; - phead.next = NULL; opt->pathchange = emit_diff_first_parent_only; - diff_tree_paths(&phead, new_oid, &old_oid, 1, base, opt); + paths = diff_tree_paths(new_oid, &old_oid, 1, base, opt); - for (p = phead.next; p;) { + for (p = paths; p;) { struct combine_diff_path *pprev = p; p = p->next; free(pprev); -- cgit v1.3