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 --- commit-reach.c | 1 + 1 file changed, 1 insertion(+) (limited to 'commit-reach.c') diff --git a/commit-reach.c b/commit-reach.c index c3518aa360..e3edd11995 100644 --- a/commit-reach.c +++ b/commit-reach.c @@ -1,4 +1,5 @@ #define USE_THE_REPOSITORY_VARIABLE +#define DISABLE_SIGN_COMPARE_WARNINGS #include "git-compat-util.h" #include "commit.h" -- cgit v1.3 From 95c09e4d07492fa9e4ad951a268b4ea6bae69038 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 27 Dec 2024 11:46:22 +0100 Subject: commit-reach: fix index used to loop through unsigned integer In 62e745ced2 (prio-queue: use size_t rather than int for size, 2024-12-20), we refactored `struct prio_queue` to track the number of contained entries via a `size_t`. While the refactoring adapted one of the users of that variable, it forgot to also adapt "commit-reach.c" accordingly. This was missed because that file has -Wsign-conversion disabled. Fix the issue by using a `size_t` to iterate through entries. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- commit-reach.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'commit-reach.c') diff --git a/commit-reach.c b/commit-reach.c index e3edd11995..e658726170 100644 --- a/commit-reach.c +++ b/commit-reach.c @@ -42,8 +42,7 @@ static int compare_commits_by_gen(const void *_a, const void *_b) static int queue_has_nonstale(struct prio_queue *queue) { - int i; - for (i = 0; i < queue->nr; i++) { + for (size_t i = 0; i < queue->nr; i++) { struct commit *commit = queue->array[i].data; if (!(commit->object.flags & STALE)) return 1; -- cgit v1.3 From 04aeeeaab1f02213703c4e1997b2c2f1ca0f8f96 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 27 Dec 2024 11:46:23 +0100 Subject: commit-reach: fix type of `min_commit_date` The `can_all_from_reach_with_flag()` function accepts a parameter that allows callers to cut off traversal at a specified commit date. This parameter is of type `time_t`, which is a signed type, while we end up comparing it to a commit's `date` field, which is of the unsigned type `timestamp_t`. Fix the parameter to be of type `timestamp_t`. There is only a single caller in "upload-pack.c" that sets this parameter, and that caller knows to pass in a `timestamp_t` already. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- commit-reach.c | 4 ++-- commit-reach.h | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) (limited to 'commit-reach.c') diff --git a/commit-reach.c b/commit-reach.c index e658726170..9f8b2457bc 100644 --- a/commit-reach.c +++ b/commit-reach.c @@ -780,7 +780,7 @@ int commit_contains(struct ref_filter *filter, struct commit *commit, int can_all_from_reach_with_flag(struct object_array *from, unsigned int with_flag, unsigned int assign_flag, - time_t min_commit_date, + timestamp_t min_commit_date, timestamp_t min_generation) { struct commit **list = NULL; @@ -883,9 +883,9 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to, int cutoff_by_min_date) { struct object_array from_objs = OBJECT_ARRAY_INIT; - time_t min_commit_date = cutoff_by_min_date ? from->item->date : 0; struct commit_list *from_iter = from, *to_iter = to; int result; + timestamp_t min_commit_date = cutoff_by_min_date ? from->item->date : 0; timestamp_t min_generation = GENERATION_NUMBER_INFINITY; while (from_iter) { diff --git a/commit-reach.h b/commit-reach.h index 9a745b7e17..d5f3347376 100644 --- a/commit-reach.h +++ b/commit-reach.h @@ -81,7 +81,7 @@ int commit_contains(struct ref_filter *filter, struct commit *commit, int can_all_from_reach_with_flag(struct object_array *from, unsigned int with_flag, unsigned int assign_flag, - time_t min_commit_date, + timestamp_t min_commit_date, timestamp_t min_generation); int can_all_from_reach(struct commit_list *from, struct commit_list *to, int commit_date_cutoff); -- cgit v1.3 From 45843d8f4eb2bbfc73cc361ba9d612d088dc8a4f Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 27 Dec 2024 11:46:24 +0100 Subject: commit-reach: use `size_t` to track indices in `remove_redundant()` The function `remove_redundant()` gets as input an array of commits as well as the size of that array and then drops redundant commits from that array. It then returns either `-1` in case an error occurred, or the new number of items in the array. The function receives and returns these sizes with a signed integer, which causes several warnings with -Wsign-compare. Fix this issue by consistently using `size_t` to track array indices and splitting up the returned value into a returned error code and a separate out pointer for the new computed size. Note that `get_merge_bases_many()` and related functions still track array sizes as a signed integer. This will be fixed in a subsequent commit. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- commit-reach.c | 53 ++++++++++++++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 23 deletions(-) (limited to 'commit-reach.c') diff --git a/commit-reach.c b/commit-reach.c index 9f8b2457bc..d7f6f1be75 100644 --- a/commit-reach.c +++ b/commit-reach.c @@ -212,12 +212,13 @@ int get_octopus_merge_bases(struct commit_list *in, struct commit_list **result) } static int remove_redundant_no_gen(struct repository *r, - struct commit **array, int cnt) + struct commit **array, + size_t cnt, size_t *dedup_cnt) { struct commit **work; unsigned char *redundant; - int *filled_index; - int i, j, filled; + size_t *filled_index; + size_t i, j, filled; CALLOC_ARRAY(work, cnt); redundant = xcalloc(cnt, 1); @@ -267,20 +268,22 @@ static int remove_redundant_no_gen(struct repository *r, for (i = filled = 0; i < cnt; i++) if (!redundant[i]) array[filled++] = work[i]; + *dedup_cnt = filled; free(work); free(redundant); free(filled_index); - return filled; + return 0; } static int remove_redundant_with_gen(struct repository *r, - struct commit **array, int cnt) + struct commit **array, size_t cnt, + size_t *dedup_cnt) { - int i, count_non_stale = 0, count_still_independent = cnt; + size_t i, count_non_stale = 0, count_still_independent = cnt; timestamp_t min_generation = GENERATION_NUMBER_INFINITY; struct commit **walk_start, **sorted; size_t walk_start_nr = 0, walk_start_alloc = cnt; - int min_gen_pos = 0; + size_t min_gen_pos = 0; /* * Sort the input by generation number, ascending. This allows @@ -326,12 +329,12 @@ static int remove_redundant_with_gen(struct repository *r, * terminate early. Otherwise, we will do the same amount of work * as before. */ - for (i = walk_start_nr - 1; i >= 0 && count_still_independent > 1; i--) { + for (i = walk_start_nr; i && count_still_independent > 1; i--) { /* push the STALE bits up to min generation */ struct commit_list *stack = NULL; - commit_list_insert(walk_start[i], &stack); - walk_start[i]->object.flags |= STALE; + commit_list_insert(walk_start[i - 1], &stack); + walk_start[i - 1]->object.flags |= STALE; while (stack) { struct commit_list *parents; @@ -388,10 +391,12 @@ static int remove_redundant_with_gen(struct repository *r, clear_commit_marks_many(walk_start_nr, walk_start, STALE); free(walk_start); - return count_non_stale; + *dedup_cnt = count_non_stale; + return 0; } -static int remove_redundant(struct repository *r, struct commit **array, int cnt) +static int remove_redundant(struct repository *r, struct commit **array, + size_t cnt, size_t *dedup_cnt) { /* * Some commit in the array may be an ancestor of @@ -401,19 +406,17 @@ static int remove_redundant(struct repository *r, struct commit **array, int cnt * that number. */ if (generation_numbers_enabled(r)) { - int i; - /* * If we have a single commit with finite generation * number, then the _with_gen algorithm is preferred. */ - for (i = 0; i < cnt; i++) { + for (size_t i = 0; i < cnt; i++) { if (commit_graph_generation(array[i]) < GENERATION_NUMBER_INFINITY) - return remove_redundant_with_gen(r, array, cnt); + return remove_redundant_with_gen(r, array, cnt, dedup_cnt); } } - return remove_redundant_no_gen(r, array, cnt); + return remove_redundant_no_gen(r, array, cnt, dedup_cnt); } static int get_merge_bases_many_0(struct repository *r, @@ -425,7 +428,8 @@ static int get_merge_bases_many_0(struct repository *r, { struct commit_list *list; struct commit **rslt; - int cnt, i; + size_t cnt, i; + int ret; if (merge_bases_many(r, one, n, twos, result) < 0) return -1; @@ -452,8 +456,8 @@ static int get_merge_bases_many_0(struct repository *r, clear_commit_marks(one, all_flags); clear_commit_marks_many(n, twos, all_flags); - cnt = remove_redundant(r, rslt, cnt); - if (cnt < 0) { + ret = remove_redundant(r, rslt, cnt, &cnt); + if (ret < 0) { free(rslt); return -1; } @@ -582,7 +586,8 @@ struct commit_list *reduce_heads(struct commit_list *heads) struct commit_list *p; struct commit_list *result = NULL, **tail = &result; struct commit **array; - int num_head, i; + size_t num_head, i; + int ret; if (!heads) return NULL; @@ -603,11 +608,13 @@ struct commit_list *reduce_heads(struct commit_list *heads) p->item->object.flags &= ~STALE; } } - num_head = remove_redundant(the_repository, array, num_head); - if (num_head < 0) { + + ret = remove_redundant(the_repository, array, num_head, &num_head); + if (ret < 0) { free(array); return NULL; } + for (i = 0; i < num_head; i++) tail = &commit_list_insert(array[i], tail)->next; free(array); -- cgit v1.3 From 85ee0680e2d5d667919e06394ca7622f09652310 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 27 Dec 2024 11:46:25 +0100 Subject: commit-reach: use `size_t` to track indices in `get_reachable_subset()` Similar as with the preceding commit, adapt `get_reachable_subset()` so that it tracks array indices via `size_t` instead of using signed integers to fix a couple of -Wsign-compare warnings. Adapt callers accordingly. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bisect.c | 9 +++++---- commit-reach.c | 8 ++++---- commit-reach.h | 4 ++-- commit.c | 4 ++-- commit.h | 2 +- ref-filter.c | 2 +- remote.c | 4 ++-- 7 files changed, 17 insertions(+), 16 deletions(-) (limited to 'commit-reach.c') diff --git a/bisect.c b/bisect.c index 1a9069c9ad..7a1afc46e5 100644 --- a/bisect.c +++ b/bisect.c @@ -780,10 +780,10 @@ static struct commit *get_commit_reference(struct repository *r, } static struct commit **get_bad_and_good_commits(struct repository *r, - int *rev_nr) + size_t *rev_nr) { struct commit **rev; - int i, n = 0; + size_t i, n = 0; ALLOC_ARRAY(rev, 1 + good_revs.nr); rev[n++] = get_commit_reference(r, current_bad_oid); @@ -887,7 +887,7 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int return res; } -static int check_ancestors(struct repository *r, int rev_nr, +static int check_ancestors(struct repository *r, size_t rev_nr, struct commit **rev, const char *prefix) { struct strvec rev_argv = STRVEC_INIT; @@ -922,7 +922,8 @@ static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r, { char *filename; struct stat st; - int fd, rev_nr; + int fd; + size_t rev_nr; enum bisect_error res = BISECT_OK; struct commit **rev; diff --git a/commit-reach.c b/commit-reach.c index d7f6f1be75..bab40f5575 100644 --- a/commit-reach.c +++ b/commit-reach.c @@ -791,8 +791,8 @@ int can_all_from_reach_with_flag(struct object_array *from, timestamp_t min_generation) { struct commit **list = NULL; - int i; - int nr_commits; + size_t i; + size_t nr_commits; int result = 1; ALLOC_ARRAY(list, from->nr); @@ -944,8 +944,8 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to, return result; } -struct commit_list *get_reachable_subset(struct commit **from, int nr_from, - struct commit **to, int nr_to, +struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from, + struct commit **to, size_t nr_to, unsigned int reachable_flag) { struct commit **item; diff --git a/commit-reach.h b/commit-reach.h index d5f3347376..fa5408054a 100644 --- a/commit-reach.h +++ b/commit-reach.h @@ -95,8 +95,8 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to, * This method uses the PARENT1 and PARENT2 flags during its operation, * so be sure these flags are not set before calling the method. */ -struct commit_list *get_reachable_subset(struct commit **from, int nr_from, - struct commit **to, int nr_to, +struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from, + struct commit **to, size_t nr_to, unsigned int reachable_flag); struct ahead_behind_count { diff --git a/commit.c b/commit.c index a127fe60c5..540660359d 100644 --- a/commit.c +++ b/commit.c @@ -778,11 +778,11 @@ static void clear_commit_marks_1(struct commit_list **plist, } } -void clear_commit_marks_many(int nr, struct commit **commit, unsigned int mark) +void clear_commit_marks_many(size_t nr, struct commit **commit, unsigned int mark) { struct commit_list *list = NULL; - while (nr--) { + for (size_t i = 0; i < nr; i++) { clear_commit_marks_1(&list, *commit, mark); commit++; } diff --git a/commit.h b/commit.h index 943e3d74b2..70c870dae4 100644 --- a/commit.h +++ b/commit.h @@ -210,7 +210,7 @@ struct commit *pop_most_recent_commit(struct commit_list **list, struct commit *pop_commit(struct commit_list **stack); void clear_commit_marks(struct commit *commit, unsigned int mark); -void clear_commit_marks_many(int nr, struct commit **commit, unsigned int mark); +void clear_commit_marks_many(size_t nr, struct commit **commit, unsigned int mark); enum rev_sort_order { diff --git a/ref-filter.c b/ref-filter.c index 23054694c2..bf5534605e 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -3041,7 +3041,7 @@ static void reach_filter(struct ref_array *array, struct commit_list **check_reachable, int include_reached) { - int i, old_nr; + size_t i, old_nr; struct commit **to_clear; if (!*check_reachable) diff --git a/remote.c b/remote.c index 18e5ccf391..0f6fba8562 100644 --- a/remote.c +++ b/remote.c @@ -1535,7 +1535,7 @@ static struct ref **tail_ref(struct ref **head) struct tips { struct commit **tip; - int nr, alloc; + size_t nr, alloc; }; static void add_to_tips(struct tips *tips, const struct object_id *oid) @@ -1602,7 +1602,7 @@ static void add_missing_tags(struct ref *src, struct ref **dst, struct ref ***ds const int reachable_flag = 1; struct commit_list *found_commits; struct commit **src_commits; - int nr_src_commits = 0, alloc_src_commits = 16; + size_t nr_src_commits = 0, alloc_src_commits = 16; ALLOC_ARRAY(src_commits, alloc_src_commits); for_each_string_list_item(item, &src_tag) { -- cgit v1.3 From 5e7fe8a7b89a07d8c3ab298ac69bc33f6ba88b47 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 27 Dec 2024 11:46:29 +0100 Subject: commit-reach: use `size_t` to track indices when computing merge bases The functions `repo_get_merge_bases_many()` and friends accepts an array of commits as well as a parameter that indicates how large that array is. This parameter is using a signed integer, which leads to a couple of warnings with -Wsign-compare. Refactor the code to use `size_t` to track indices instead and adapt callers accordingly. While most callers are trivial, there are two callers that require a bit more scrutiny: - builtin/merge-base.c:show_merge_base() subtracts `1` from the `rev_nr` before calling `repo_get_merge_bases_many_dirty()`, so if the variable was `0` it would wrap. This code is fine though because its only caller will execute that code only when `argc >= 2`, and it follows that `rev_nr >= 2`, as well. - bisect.ccheck_merge_bases() similarly subtracts `1` from `rev_nr`. Again, there is only a single caller that populates `rev_nr` with `good_revs.nr`. And because a bisection always requires at least one good revision it follws that `rev_nr >= 1`. Mark the file as -Wsign-compare-clean. Signed-off-by: Patrick Steinhardt Signed-off-by: Junio C Hamano --- bisect.c | 2 +- builtin/merge-base.c | 4 ++-- commit-reach.c | 7 +++---- commit-reach.h | 4 ++-- t/helper/test-reach.c | 6 +++--- 5 files changed, 11 insertions(+), 12 deletions(-) (limited to 'commit-reach.c') diff --git a/bisect.c b/bisect.c index 7a1afc46e5..7a3c77c6d8 100644 --- a/bisect.c +++ b/bisect.c @@ -855,7 +855,7 @@ static void handle_skipped_merge_base(const struct object_id *mb) * for early success, this will be converted back to 0 in * check_good_are_ancestors_of_bad(). */ -static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int no_checkout) +static enum bisect_error check_merge_bases(size_t rev_nr, struct commit **rev, int no_checkout) { enum bisect_error res = BISECT_OK; struct commit_list *result = NULL; diff --git a/builtin/merge-base.c b/builtin/merge-base.c index a20c93b11a..123c81515e 100644 --- a/builtin/merge-base.c +++ b/builtin/merge-base.c @@ -8,7 +8,7 @@ #include "parse-options.h" #include "commit-reach.h" -static int show_merge_base(struct commit **rev, int rev_nr, int show_all) +static int show_merge_base(struct commit **rev, size_t rev_nr, int show_all) { struct commit_list *result = NULL, *r; @@ -149,7 +149,7 @@ int cmd_merge_base(int argc, struct repository *repo UNUSED) { struct commit **rev; - int rev_nr = 0; + size_t rev_nr = 0; int show_all = 0; int cmdmode = 0; int ret; diff --git a/commit-reach.c b/commit-reach.c index bab40f5575..a339e41aa4 100644 --- a/commit-reach.c +++ b/commit-reach.c @@ -1,5 +1,4 @@ #define USE_THE_REPOSITORY_VARIABLE -#define DISABLE_SIGN_COMPARE_WARNINGS #include "git-compat-util.h" #include "commit.h" @@ -421,7 +420,7 @@ static int remove_redundant(struct repository *r, struct commit **array, static int get_merge_bases_many_0(struct repository *r, struct commit *one, - int n, + size_t n, struct commit **twos, int cleanup, struct commit_list **result) @@ -469,7 +468,7 @@ static int get_merge_bases_many_0(struct repository *r, int repo_get_merge_bases_many(struct repository *r, struct commit *one, - int n, + size_t n, struct commit **twos, struct commit_list **result) { @@ -478,7 +477,7 @@ int repo_get_merge_bases_many(struct repository *r, int repo_get_merge_bases_many_dirty(struct repository *r, struct commit *one, - int n, + size_t n, struct commit **twos, struct commit_list **result) { diff --git a/commit-reach.h b/commit-reach.h index fa5408054a..6012402dfc 100644 --- a/commit-reach.h +++ b/commit-reach.h @@ -14,12 +14,12 @@ int repo_get_merge_bases(struct repository *r, struct commit *rev2, struct commit_list **result); int repo_get_merge_bases_many(struct repository *r, - struct commit *one, int n, + struct commit *one, size_t n, struct commit **twos, struct commit_list **result); /* To be used only when object flags after this call no longer matter */ int repo_get_merge_bases_many_dirty(struct repository *r, - struct commit *one, int n, + struct commit *one, size_t n, struct commit **twos, struct commit_list **result); diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c index 01cf77ae65..028ec00306 100644 --- a/t/helper/test-reach.c +++ b/t/helper/test-reach.c @@ -35,7 +35,7 @@ int cmd__reach(int ac, const char **av) struct commit_list *X, *Y; struct object_array X_obj = OBJECT_ARRAY_INIT; struct commit **X_array, **Y_array; - int X_nr, X_alloc, Y_nr, Y_alloc; + size_t X_nr, X_alloc, Y_nr, Y_alloc; struct strbuf buf = STRBUF_INIT; struct repository *r = the_repository; @@ -157,7 +157,7 @@ int cmd__reach(int ac, const char **av) clear_contains_cache(&cache); } else if (!strcmp(av[1], "get_reachable_subset")) { const int reachable_flag = 1; - int i, count = 0; + int count = 0; struct commit_list *current; struct commit_list *list = get_reachable_subset(X_array, X_nr, Y_array, Y_nr, @@ -169,7 +169,7 @@ int cmd__reach(int ac, const char **av) oid_to_hex(&list->item->object.oid)); count++; } - for (i = 0; i < Y_nr; i++) { + for (size_t i = 0; i < Y_nr; i++) { if (Y_array[i]->object.flags & reachable_flag) count--; } -- cgit v1.3