From 41f43b8243f42b9df2e98be8460646d4c0100ad3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 6 Dec 2024 11:27:19 +0100 Subject: global: mark code units that generate warnings with `-Wsign-compare` Mark code units that generate warnings with `-Wsign-compare`. This allows for a structured approach to get rid of all such warnings over time in a way that can be easily measured. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- combine-diff.c | 1 + 1 file changed, 1 insertion(+) (limited to 'combine-diff.c') diff --git a/combine-diff.c b/combine-diff.c index 33d0ed7097..641bc92dbd 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -1,4 +1,5 @@ #define USE_THE_REPOSITORY_VARIABLE +#define DISABLE_SIGN_COMPARE_WARNINGS #include "git-compat-util.h" #include "object-store-ll.h" -- cgit v1.3 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 --- combine-diff.c | 40 ++++++++++++++++++++++++++-------------- diff-lib.c | 29 ++++++----------------------- diff.h | 5 +++++ 3 files changed, 37 insertions(+), 37 deletions(-) (limited to 'combine-diff.c') diff --git a/combine-diff.c b/combine-diff.c index 641bc92dbd..45548fd438 100644 --- a/combine-diff.c +++ b/combine-diff.c @@ -47,22 +47,13 @@ static struct combine_diff_path *intersect_paths( if (!n) { for (i = 0; i < q->nr; i++) { - int len; - const char *path; if (diff_unmodified_pair(q->queue[i])) continue; - path = q->queue[i]->two->path; - len = strlen(path); - p = xmalloc(combine_diff_path_size(num_parent, len)); - p->path = (char *) &(p->parent[num_parent]); - memcpy(p->path, path, len); - p->path[len] = 0; - p->next = NULL; - memset(p->parent, 0, - sizeof(p->parent[0]) * num_parent); - - oidcpy(&p->oid, &q->queue[i]->two->oid); - p->mode = q->queue[i]->two->mode; + p = combine_diff_path_new(q->queue[i]->two->path, + strlen(q->queue[i]->two->path), + q->queue[i]->two->mode, + &q->queue[i]->two->oid, + num_parent); oidcpy(&p->parent[n].oid, &q->queue[i]->one->oid); p->parent[n].mode = q->queue[i]->one->mode; p->parent[n].status = q->queue[i]->status; @@ -1667,3 +1658,24 @@ void diff_tree_combined_merge(const struct commit *commit, diff_tree_combined(&commit->object.oid, &parents, rev); oid_array_clear(&parents); } + +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) +{ + struct combine_diff_path *p; + + p = xmalloc(combine_diff_path_size(num_parents, path_len)); + p->path = (char *)&(p->parent[num_parents]); + memcpy(p->path, path, path_len); + p->path[path_len] = 0; + p->next = NULL; + p->mode = mode; + oidcpy(&p->oid, oid); + + memset(p->parent, 0, sizeof(p->parent[0]) * num_parents); + + return p; +} diff --git a/diff-lib.c b/diff-lib.c index 85b8f1fa59..471ef99614 100644 --- a/diff-lib.c +++ b/diff-lib.c @@ -153,7 +153,6 @@ void run_diff_files(struct rev_info *revs, unsigned int option) struct diff_filepair *pair; unsigned int wt_mode = 0; int num_compare_stages = 0; - size_t path_len; struct stat st; changed = check_removed(ce, &st); @@ -167,18 +166,8 @@ void run_diff_files(struct rev_info *revs, unsigned int option) wt_mode = 0; } - path_len = ce_namelen(ce); - - dpath = xmalloc(combine_diff_path_size(5, path_len)); - dpath->path = (char *) &(dpath->parent[5]); - - dpath->next = NULL; - memcpy(dpath->path, ce->name, path_len); - dpath->path[path_len] = '\0'; - oidclr(&dpath->oid, the_repository->hash_algo); - dpath->mode = wt_mode; - memset(&(dpath->parent[0]), 0, - sizeof(struct combine_diff_parent)*5); + dpath = combine_diff_path_new(ce->name, ce_namelen(ce), + wt_mode, null_oid(), 5); while (i < entries) { struct cache_entry *nce = istate->cache[i]; @@ -405,16 +394,10 @@ static int show_modified(struct rev_info *revs, if (revs->combine_merges && !cached && (!oideq(oid, &old_entry->oid) || !oideq(&old_entry->oid, &new_entry->oid))) { struct combine_diff_path *p; - int pathlen = ce_namelen(new_entry); - - p = xmalloc(combine_diff_path_size(2, pathlen)); - p->path = (char *) &p->parent[2]; - p->next = NULL; - memcpy(p->path, new_entry->name, pathlen); - p->path[pathlen] = 0; - p->mode = mode; - oidclr(&p->oid, the_repository->hash_algo); - memset(p->parent, 0, 2 * sizeof(struct combine_diff_parent)); + + p = combine_diff_path_new(new_entry->name, + ce_namelen(new_entry), + mode, null_oid(), 2); p->parent[0].status = DIFF_STATUS_MODIFIED; p->parent[0].mode = new_entry->ce_mode; oidcpy(&p->parent[0].oid, &new_entry->oid); 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 'combine-diff.c') 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 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 'combine-diff.c') 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 'combine-diff.c') 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