{"thread":{"id":"62697","subject":"[PATCH 0/9] commit-reach: -Wsign-compare follow-ups","startedAt":"2024-12-27T10:47:18Z","lastAt":"2025-01-03T06:43:16Z","messageCount":34,"participants":["Patrick Steinhardt","shejialuo","Jeff King","Junio C Hamano","René Scharfe","Justin Tobler"],"isPatch":true,"patchVersion":1,"patchTotal":9},"messages":[{"id":"509592","messageId":"20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im","threadId":"62697","inReplyTo":null,"subject":"[PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:46:20Z","receivedAt":"2024-12-27T10:47:18Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nthis patch series is a follow-up for [1], fixing two smallish issues\nintroduced in that patch. Naturally I couldn't stop there and decided to\nalso make \"commit-reach.c\" and two other files -Wsign-compare-clean.\n\nThanks!\n\nPatrick\n\n[1]: <20241220084949.GA132704@coredump.intra.peff.net>\n\n---\nPatrick Steinhardt (9):\n      prio-queue: fix type of `insertion_ctr`\n      commit-reach: fix index used to loop through unsigned integer\n      commit-reach: fix type of `min_commit_date`\n      commit-reach: use `size_t` to track indices in `remove_redundant()`\n      commit-reach: use `size_t` to track indices in `get_reachable_subset()`\n      builtin/log: use `size_t` to track indices\n      builtin/log: fix remaining -Wsign-compare warnings\n      shallow: fix -Wsign-compare warnings\n      commit-reach: use `size_t` to track indices when computing merge bases\n\n bisect.c              | 11 ++++----\n builtin/log.c         | 50 +++++++++++++++++-----------------\n builtin/merge-base.c  |  4 +--\n commit-reach.c        | 75 +++++++++++++++++++++++++++------------------------\n commit-reach.h        | 10 +++----\n commit.c              |  4 +--\n commit.h              |  2 +-\n prio-queue.h          |  4 +--\n ref-filter.c          |  2 +-\n remote.c              |  4 +--\n shallow.c             | 38 +++++++++++++-------------\n shallow.h             |  6 ++---\n t/helper/test-reach.c |  6 ++---\n 13 files changed, 111 insertions(+), 105 deletions(-)\n\n\n---\nbase-commit: 76cf4f61c87855ebf0784b88aaf737d6b09f504b\nchange-id: 20241227-b4-pks-commit-reach-sign-compare-235a03fffc78\n\n"},{"id":"509593","messageId":"20241227-b4-pks-commit-reach-sign-compare-v1-1-07c59c2aa632@pks.im","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im","subject":"[PATCH 1/9] prio-queue: fix type of `insertion_ctr`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:46:21Z","receivedAt":"2024-12-27T10:47:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 62e745ced2 (prio-queue: use size_t rather than int for size,\n2024-12-20), we have converted `struct prio_queue` to use `size_t` to\ntrack the number of entries in the queue as well as the allocated size\nof the underlying array. There is one more counter though, namely the\ninsertion counter, that is still using an `unsigned` instead of a\n`size_t`. This is unlikely to ever be a problem, but it makes one wonder\nwhy some indices use `size_t` while others use `unsigned`. Furthermore,\nthe mentioned commit stated the intent to also adapt these variables,\nbut seemingly forgot to do so.\n\nFix the issue by converting those counters to use `size_t`, as well.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n prio-queue.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/prio-queue.h b/prio-queue.h\nindex 36f370625f0802cb84082fea904ad6e8a456520a..38d032636d4cf9c544811cff6c3e6a080d6c7b82 100644\n--- a/prio-queue.h\n+++ b/prio-queue.h\n@@ -22,13 +22,13 @@\n typedef int (*prio_queue_compare_fn)(const void *one, const void *two, void *cb_data);\n \n struct prio_queue_entry {\n-\tunsigned ctr;\n+\tsize_t ctr;\n \tvoid *data;\n };\n \n struct prio_queue {\n \tprio_queue_compare_fn compare;\n-\tunsigned insertion_ctr;\n+\tsize_t insertion_ctr;\n \tvoid *cb_data;\n \tsize_t alloc, nr;\n \tstruct prio_queue_entry *array;\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509594","messageId":"20241227-b4-pks-commit-reach-sign-compare-v1-2-07c59c2aa632@pks.im","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im","subject":"[PATCH 2/9] commit-reach: fix index used to loop through unsigned integer","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:46:22Z","receivedAt":"2024-12-27T10:47:19Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 62e745ced2 (prio-queue: use size_t rather than int for size,\n2024-12-20), we refactored `struct prio_queue` to track the number of\ncontained entries via a `size_t`. While the refactoring adapted one of\nthe users of that variable, it forgot to also adapt \"commit-reach.c\"\naccordingly. This was missed because that file has -Wsign-conversion\ndisabled.\n\nFix the issue by using a `size_t` to iterate through entries.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n commit-reach.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex e3edd1199529792fafbaa03999c5b7b202f7cf1b..e65872617003d0e43776909c30343f091d6b42f2 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -42,8 +42,7 @@ static int compare_commits_by_gen(const void *_a, const void *_b)\n \n static int queue_has_nonstale(struct prio_queue *queue)\n {\n-\tint i;\n-\tfor (i = 0; i < queue->nr; i++) {\n+\tfor (size_t i = 0; i < queue->nr; i++) {\n \t\tstruct commit *commit = queue->array[i].data;\n \t\tif (!(commit->object.flags & STALE))\n \t\t\treturn 1;\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509595","messageId":"20241227-b4-pks-commit-reach-sign-compare-v1-3-07c59c2aa632@pks.im","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im","subject":"[PATCH 3/9] commit-reach: fix type of `min_commit_date`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:46:23Z","receivedAt":"2024-12-27T10:47:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The `can_all_from_reach_with_flag()` function accepts a parameter that\nallows callers to cut off traversal at a specified commit date. This\nparameter is of type `time_t`, which is a signed type, while we end up\ncomparing it to a commit's `date` field, which is of the unsigned type\n`timestamp_t`.\n\nFix the parameter to be of type `timestamp_t`. There is only a single\ncaller in \"upload-pack.c\" that sets this parameter, and that caller\nknows to pass in a `timestamp_t` already.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n commit-reach.c | 4 ++--\n commit-reach.h | 2 +-\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex e65872617003d0e43776909c30343f091d6b42f2..9f8b2457bcc12bebf725a5276d1aec467bb7af05 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -780,7 +780,7 @@ int commit_contains(struct ref_filter *filter, struct commit *commit,\n int can_all_from_reach_with_flag(struct object_array *from,\n \t\t\t\t unsigned int with_flag,\n \t\t\t\t unsigned int assign_flag,\n-\t\t\t\t time_t min_commit_date,\n+\t\t\t\t timestamp_t min_commit_date,\n \t\t\t\t timestamp_t min_generation)\n {\n \tstruct commit **list = NULL;\n@@ -883,9 +883,9 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,\n \t\t       int cutoff_by_min_date)\n {\n \tstruct object_array from_objs = OBJECT_ARRAY_INIT;\n-\ttime_t min_commit_date = cutoff_by_min_date ? from->item->date : 0;\n \tstruct commit_list *from_iter = from, *to_iter = to;\n \tint result;\n+\ttimestamp_t min_commit_date = cutoff_by_min_date ? from->item->date : 0;\n \ttimestamp_t min_generation = GENERATION_NUMBER_INFINITY;\n \n \twhile (from_iter) {\ndiff --git a/commit-reach.h b/commit-reach.h\nindex 9a745b7e1766850a77fbe52c3eeae290b68038d0..d5f3347376b6310727c74b81cb7660485b96c0bc 100644\n--- a/commit-reach.h\n+++ b/commit-reach.h\n@@ -81,7 +81,7 @@ int commit_contains(struct ref_filter *filter, struct commit *commit,\n int can_all_from_reach_with_flag(struct object_array *from,\n \t\t\t\t unsigned int with_flag,\n \t\t\t\t unsigned int assign_flag,\n-\t\t\t\t time_t min_commit_date,\n+\t\t\t\t timestamp_t min_commit_date,\n \t\t\t\t timestamp_t min_generation);\n int can_all_from_reach(struct commit_list *from, struct commit_list *to,\n \t\t       int commit_date_cutoff);\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509596","messageId":"20241227-b4-pks-commit-reach-sign-compare-v1-4-07c59c2aa632@pks.im","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im","subject":"[PATCH 4/9] commit-reach: use `size_t` to track indices in `remove_redundant()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:46:24Z","receivedAt":"2024-12-27T10:47:20Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `remove_redundant()` gets as input an array of commits as\nwell as the size of that array and then drops redundant commits from\nthat array. It then returns either `-1` in case an error occurred, or\nthe new number of items in the array.\n\nThe function receives and returns these sizes with a signed integer,\nwhich causes several warnings with -Wsign-compare. Fix this issue by\nconsistently using `size_t` to track array indices and splitting up\nthe returned value into a returned error code and a separate out pointer\nfor the new computed size.\n\nNote that `get_merge_bases_many()` and related functions still track\narray sizes as a signed integer. This will be fixed in a subsequent\ncommit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n commit-reach.c | 53 ++++++++++++++++++++++++++++++-----------------------\n 1 file changed, 30 insertions(+), 23 deletions(-)\n\ndiff --git a/commit-reach.c b/commit-reach.c\nindex 9f8b2457bcc12bebf725a5276d1aec467bb7af05..d7f6f1be75e95cc834d60be719e930a77ad0518f 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -212,12 +212,13 @@ int get_octopus_merge_bases(struct commit_list *in, struct commit_list **result)\n }\n \n static int remove_redundant_no_gen(struct repository *r,\n-\t\t\t\t   struct commit **array, int cnt)\n+\t\t\t\t   struct commit **array,\n+\t\t\t\t   size_t cnt, size_t *dedup_cnt)\n {\n \tstruct commit **work;\n \tunsigned char *redundant;\n-\tint *filled_index;\n-\tint i, j, filled;\n+\tsize_t *filled_index;\n+\tsize_t i, j, filled;\n \n \tCALLOC_ARRAY(work, cnt);\n \tredundant = xcalloc(cnt, 1);\n@@ -267,20 +268,22 @@ static int remove_redundant_no_gen(struct repository *r,\n \tfor (i = filled = 0; i < cnt; i++)\n \t\tif (!redundant[i])\n \t\t\tarray[filled++] = work[i];\n+\t*dedup_cnt = filled;\n \tfree(work);\n \tfree(redundant);\n \tfree(filled_index);\n-\treturn filled;\n+\treturn 0;\n }\n \n static int remove_redundant_with_gen(struct repository *r,\n-\t\t\t\t     struct commit **array, int cnt)\n+\t\t\t\t     struct commit **array, size_t cnt,\n+\t\t\t\t     size_t *dedup_cnt)\n {\n-\tint i, count_non_stale = 0, count_still_independent = cnt;\n+\tsize_t i, count_non_stale = 0, count_still_independent = cnt;\n \ttimestamp_t min_generation = GENERATION_NUMBER_INFINITY;\n \tstruct commit **walk_start, **sorted;\n \tsize_t walk_start_nr = 0, walk_start_alloc = cnt;\n-\tint min_gen_pos = 0;\n+\tsize_t min_gen_pos = 0;\n \n \t/*\n \t * Sort the input by generation number, ascending. This allows\n@@ -326,12 +329,12 @@ static int remove_redundant_with_gen(struct repository *r,\n \t * terminate early. Otherwise, we will do the same amount of work\n \t * as before.\n \t */\n-\tfor (i = walk_start_nr - 1; i >= 0 && count_still_independent > 1; i--) {\n+\tfor (i = walk_start_nr; i && count_still_independent > 1; i--) {\n \t\t/* push the STALE bits up to min generation */\n \t\tstruct commit_list *stack = NULL;\n \n-\t\tcommit_list_insert(walk_start[i], &stack);\n-\t\twalk_start[i]->object.flags |= STALE;\n+\t\tcommit_list_insert(walk_start[i - 1], &stack);\n+\t\twalk_start[i - 1]->object.flags |= STALE;\n \n \t\twhile (stack) {\n \t\t\tstruct commit_list *parents;\n@@ -388,10 +391,12 @@ static int remove_redundant_with_gen(struct repository *r,\n \tclear_commit_marks_many(walk_start_nr, walk_start, STALE);\n \tfree(walk_start);\n \n-\treturn count_non_stale;\n+\t*dedup_cnt = count_non_stale;\n+\treturn 0;\n }\n \n-static int remove_redundant(struct repository *r, struct commit **array, int cnt)\n+static int remove_redundant(struct repository *r, struct commit **array,\n+\t\t\t    size_t cnt, size_t *dedup_cnt)\n {\n \t/*\n \t * Some commit in the array may be an ancestor of\n@@ -401,19 +406,17 @@ static int remove_redundant(struct repository *r, struct commit **array, int cnt\n \t * that number.\n \t */\n \tif (generation_numbers_enabled(r)) {\n-\t\tint i;\n-\n \t\t/*\n \t\t * If we have a single commit with finite generation\n \t\t * number, then the _with_gen algorithm is preferred.\n \t\t */\n-\t\tfor (i = 0; i < cnt; i++) {\n+\t\tfor (size_t i = 0; i < cnt; i++) {\n \t\t\tif (commit_graph_generation(array[i]) < GENERATION_NUMBER_INFINITY)\n-\t\t\t\treturn remove_redundant_with_gen(r, array, cnt);\n+\t\t\t\treturn remove_redundant_with_gen(r, array, cnt, dedup_cnt);\n \t\t}\n \t}\n \n-\treturn remove_redundant_no_gen(r, array, cnt);\n+\treturn remove_redundant_no_gen(r, array, cnt, dedup_cnt);\n }\n \n static int get_merge_bases_many_0(struct repository *r,\n@@ -425,7 +428,8 @@ static int get_merge_bases_many_0(struct repository *r,\n {\n \tstruct commit_list *list;\n \tstruct commit **rslt;\n-\tint cnt, i;\n+\tsize_t cnt, i;\n+\tint ret;\n \n \tif (merge_bases_many(r, one, n, twos, result) < 0)\n \t\treturn -1;\n@@ -452,8 +456,8 @@ static int get_merge_bases_many_0(struct repository *r,\n \tclear_commit_marks(one, all_flags);\n \tclear_commit_marks_many(n, twos, all_flags);\n \n-\tcnt = remove_redundant(r, rslt, cnt);\n-\tif (cnt < 0) {\n+\tret = remove_redundant(r, rslt, cnt, &cnt);\n+\tif (ret < 0) {\n \t\tfree(rslt);\n \t\treturn -1;\n \t}\n@@ -582,7 +586,8 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n \tstruct commit_list *p;\n \tstruct commit_list *result = NULL, **tail = &result;\n \tstruct commit **array;\n-\tint num_head, i;\n+\tsize_t num_head, i;\n+\tint ret;\n \n \tif (!heads)\n \t\treturn NULL;\n@@ -603,11 +608,13 @@ struct commit_list *reduce_heads(struct commit_list *heads)\n \t\t\tp->item->object.flags &= ~STALE;\n \t\t}\n \t}\n-\tnum_head = remove_redundant(the_repository, array, num_head);\n-\tif (num_head < 0) {\n+\n+\tret = remove_redundant(the_repository, array, num_head, &num_head);\n+\tif (ret < 0) {\n \t\tfree(array);\n \t\treturn NULL;\n \t}\n+\n \tfor (i = 0; i < num_head; i++)\n \t\ttail = &commit_list_insert(array[i], tail)->next;\n \tfree(array);\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509597","messageId":"20241227-b4-pks-commit-reach-sign-compare-v1-5-07c59c2aa632@pks.im","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im","subject":"[PATCH 5/9] commit-reach: use `size_t` to track indices in `get_reachable_subset()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:46:25Z","receivedAt":"2024-12-27T10:47:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Similar as with the preceding commit, adapt `get_reachable_subset()` so\nthat it tracks array indices via `size_t` instead of using signed\nintegers to fix a couple of -Wsign-compare warnings. Adapt callers\naccordingly.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n bisect.c       | 9 +++++----\n commit-reach.c | 8 ++++----\n commit-reach.h | 4 ++--\n commit.c       | 4 ++--\n commit.h       | 2 +-\n ref-filter.c   | 2 +-\n remote.c       | 4 ++--\n 7 files changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 1a9069c9ad8a19d6e0ee45415407c70f36885514..7a1afc46e5fc0250212f5b6eaf952cf8e36b56fe 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -780,10 +780,10 @@ static struct commit *get_commit_reference(struct repository *r,\n }\n \n static struct commit **get_bad_and_good_commits(struct repository *r,\n-\t\t\t\t\t\tint *rev_nr)\n+\t\t\t\t\t\tsize_t *rev_nr)\n {\n \tstruct commit **rev;\n-\tint i, n = 0;\n+\tsize_t i, n = 0;\n \n \tALLOC_ARRAY(rev, 1 + good_revs.nr);\n \trev[n++] = get_commit_reference(r, current_bad_oid);\n@@ -887,7 +887,7 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int\n \treturn res;\n }\n \n-static int check_ancestors(struct repository *r, int rev_nr,\n+static int check_ancestors(struct repository *r, size_t rev_nr,\n \t\t\t   struct commit **rev, const char *prefix)\n {\n \tstruct strvec rev_argv = STRVEC_INIT;\n@@ -922,7 +922,8 @@ static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,\n {\n \tchar *filename;\n \tstruct stat st;\n-\tint fd, rev_nr;\n+\tint fd;\n+\tsize_t rev_nr;\n \tenum bisect_error res = BISECT_OK;\n \tstruct commit **rev;\n \ndiff --git a/commit-reach.c b/commit-reach.c\nindex d7f6f1be75e95cc834d60be719e930a77ad0518f..bab40f557580476d59d3a0b0ef56f40263e6615e 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -791,8 +791,8 @@ int can_all_from_reach_with_flag(struct object_array *from,\n \t\t\t\t timestamp_t min_generation)\n {\n \tstruct commit **list = NULL;\n-\tint i;\n-\tint nr_commits;\n+\tsize_t i;\n+\tsize_t nr_commits;\n \tint result = 1;\n \n \tALLOC_ARRAY(list, from->nr);\n@@ -944,8 +944,8 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,\n \treturn result;\n }\n \n-struct commit_list *get_reachable_subset(struct commit **from, int nr_from,\n-\t\t\t\t\t struct commit **to, int nr_to,\n+struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from,\n+\t\t\t\t\t struct commit **to, size_t nr_to,\n \t\t\t\t\t unsigned int reachable_flag)\n {\n \tstruct commit **item;\ndiff --git a/commit-reach.h b/commit-reach.h\nindex d5f3347376b6310727c74b81cb7660485b96c0bc..fa5408054ac01372c041d18595a405cfdaec6af3 100644\n--- a/commit-reach.h\n+++ b/commit-reach.h\n@@ -95,8 +95,8 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,\n  * This method uses the PARENT1 and PARENT2 flags during its operation,\n  * so be sure these flags are not set before calling the method.\n  */\n-struct commit_list *get_reachable_subset(struct commit **from, int nr_from,\n-\t\t\t\t\t struct commit **to, int nr_to,\n+struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from,\n+\t\t\t\t\t struct commit **to, size_t nr_to,\n \t\t\t\t\t unsigned int reachable_flag);\n \n struct ahead_behind_count {\ndiff --git a/commit.c b/commit.c\nindex a127fe60c5e83c3968fc0538f0e801e4afd52acd..540660359d4190f7dbc1aaa7668b033a6d16afe9 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -778,11 +778,11 @@ static void clear_commit_marks_1(struct commit_list **plist,\n \t}\n }\n \n-void clear_commit_marks_many(int nr, struct commit **commit, unsigned int mark)\n+void clear_commit_marks_many(size_t nr, struct commit **commit, unsigned int mark)\n {\n \tstruct commit_list *list = NULL;\n \n-\twhile (nr--) {\n+\tfor (size_t i = 0; i < nr; i++) {\n \t\tclear_commit_marks_1(&list, *commit, mark);\n \t\tcommit++;\n \t}\ndiff --git a/commit.h b/commit.h\nindex 943e3d74b2ae026d77e3bac59a4e993314e79ce4..70c870dae4d4b972a1f608983fc17024ad04d2e9 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -210,7 +210,7 @@ struct commit *pop_most_recent_commit(struct commit_list **list,\n struct commit *pop_commit(struct commit_list **stack);\n \n void clear_commit_marks(struct commit *commit, unsigned int mark);\n-void clear_commit_marks_many(int nr, struct commit **commit, unsigned int mark);\n+void clear_commit_marks_many(size_t nr, struct commit **commit, unsigned int mark);\n \n \n enum rev_sort_order {\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 23054694c2c96014ef411a225c6c941eb8d2d56c..bf5534605e234d7cdd01d52f5c83f5314745e13d 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -3041,7 +3041,7 @@ static void reach_filter(struct ref_array *array,\n \t\t\t struct commit_list **check_reachable,\n \t\t\t int include_reached)\n {\n-\tint i, old_nr;\n+\tsize_t i, old_nr;\n \tstruct commit **to_clear;\n \n \tif (!*check_reachable)\ndiff --git a/remote.c b/remote.c\nindex 18e5ccf391844516e2fcc54fc0d6835283ff6a48..0f6fba85625b523122e50e28a0f64b6e143cd9fb 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1535,7 +1535,7 @@ static struct ref **tail_ref(struct ref **head)\n \n struct tips {\n \tstruct commit **tip;\n-\tint nr, alloc;\n+\tsize_t nr, alloc;\n };\n \n static void add_to_tips(struct tips *tips, const struct object_id *oid)\n@@ -1602,7 +1602,7 @@ static void add_missing_tags(struct ref *src, struct ref **dst, struct ref ***ds\n \t\tconst int reachable_flag = 1;\n \t\tstruct commit_list *found_commits;\n \t\tstruct commit **src_commits;\n-\t\tint nr_src_commits = 0, alloc_src_commits = 16;\n+\t\tsize_t nr_src_commits = 0, alloc_src_commits = 16;\n \t\tALLOC_ARRAY(src_commits, alloc_src_commits);\n \n \t\tfor_each_string_list_item(item, &src_tag) {\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509598","messageId":"20241227-b4-pks-commit-reach-sign-compare-v1-6-07c59c2aa632@pks.im","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im","subject":"[PATCH 6/9] builtin/log: use `size_t` to track indices","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:46:26Z","receivedAt":"2024-12-27T10:47:21Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Similar as with the preceding commit, adapt \"builtin/log.c\" so that it\ntracks array indices via `size_t` instead of using signed integers. This\nfixes a couple of -Wsign-compare warnings and prepares the code for\na similar refactoring of `repo_get_merge_bases_many()` in a subsequent\ncommit.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/log.c | 23 +++++++++++++----------\n 1 file changed, 13 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 75e1b34123b348f57334d5592879398064de246e..805b2355d964915732edf5928d54fb6d06e394d4 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1746,11 +1746,12 @@ struct base_tree_info {\n \n static struct commit *get_base_commit(const struct format_config *cfg,\n \t\t\t\t      struct commit **list,\n-\t\t\t\t      int total)\n+\t\t\t\t      size_t total)\n {\n \tstruct commit *base = NULL;\n \tstruct commit **rev;\n-\tint i = 0, rev_nr = 0, auto_select, die_on_failure, ret;\n+\tint auto_select, die_on_failure, ret;\n+\tsize_t i = 0, rev_nr = 0;\n \n \tswitch (cfg->auto_base) {\n \tcase AUTO_BASE_NEVER:\n@@ -1885,13 +1886,12 @@ define_commit_slab(commit_base, int);\n static void prepare_bases(struct base_tree_info *bases,\n \t\t\t  struct commit *base,\n \t\t\t  struct commit **list,\n-\t\t\t  int total)\n+\t\t\t  size_t total)\n {\n \tstruct commit *commit;\n \tstruct rev_info revs;\n \tstruct diff_options diffopt;\n \tstruct commit_base commit_base;\n-\tint i;\n \n \tif (!base)\n \t\treturn;\n@@ -1906,7 +1906,7 @@ static void prepare_bases(struct base_tree_info *bases,\n \trepo_init_revisions(the_repository, &revs, NULL);\n \trevs.max_parents = 1;\n \trevs.topo_order = 1;\n-\tfor (i = 0; i < total; i++) {\n+\tfor (size_t i = 0; i < total; i++) {\n \t\tlist[i]->object.flags &= ~UNINTERESTING;\n \t\tadd_pending_object(&revs, &list[i]->object, \"rev_list\");\n \t\t*commit_base_at(&commit_base, list[i]) = 1;\n@@ -2007,7 +2007,7 @@ int cmd_format_patch(int argc,\n \tstruct rev_info rev;\n \tchar *to_free = NULL;\n \tstruct setup_revision_opt s_r_opt;\n-\tint nr = 0, total, i;\n+\tsize_t nr = 0, total, i;\n \tint use_stdout = 0;\n \tint start_number = -1;\n \tint just_numbers = 0;\n@@ -2500,11 +2500,14 @@ int cmd_format_patch(int argc,\n \n \tif (show_progress)\n \t\tprogress = start_delayed_progress(_(\"Generating patches\"), total);\n-\twhile (0 <= --nr) {\n+\tfor (i = 0; i < nr; i++) {\n+\t\tsize_t idx = nr - i - 1;\n \t\tint shown;\n-\t\tdisplay_progress(progress, total - nr);\n-\t\tcommit = list[nr];\n-\t\trev.nr = total - nr + (start_number - 1);\n+\n+\t\tdisplay_progress(progress, total - idx);\n+\t\tcommit = list[idx];\n+\t\trev.nr = total - idx + (start_number - 1);\n+\n \t\t/* Make the second and subsequent mails replies to the first */\n \t\tif (cfg.thread) {\n \t\t\t/* Have we already had a message ID? */\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509599","messageId":"20241227-b4-pks-commit-reach-sign-compare-v1-7-07c59c2aa632@pks.im","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im","subject":"[PATCH 7/9] builtin/log: fix remaining -Wsign-compare warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:46:27Z","receivedAt":"2024-12-27T10:47:22Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Fix remaining -Wsign-compare warnings in \"builtin/log.c\" and mark the\nfile as -Wsign-compare-clean. While most of the fixes are obvious, one\nfix requires us to use `cast_size_t_to_int()`, which will cause us to\ndie in case the `size_t` cannot be represented as `int`. This should be\nfine though, as the data would typically be set either via a config key\nor via the command line, neither of which should ever exceed a couple of\nkilobytes of data.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n builtin/log.c | 27 +++++++++++++--------------\n 1 file changed, 13 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 805b2355d964915732edf5928d54fb6d06e394d4..a4f41aafcae069541ee987dc94d245edfe9787a8 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -6,7 +6,6 @@\n  */\n \n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"builtin.h\"\n #include \"abspath.h\"\n@@ -209,7 +208,6 @@ static void cmd_log_init_defaults(struct rev_info *rev,\n \n static void set_default_decoration_filter(struct decoration_filter *decoration_filter)\n {\n-\tint i;\n \tchar *value = NULL;\n \tstruct string_list *include = decoration_filter->include_ref_pattern;\n \tconst struct string_list *config_exclude;\n@@ -243,7 +241,7 @@ static void set_default_decoration_filter(struct decoration_filter *decoration_f\n \t * No command-line or config options were given, so\n \t * populate with sensible defaults.\n \t */\n-\tfor (i = 0; i < ARRAY_SIZE(ref_namespace); i++) {\n+\tfor (size_t i = 0; i < ARRAY_SIZE(ref_namespace); i++) {\n \t\tif (!ref_namespace[i].decoration)\n \t\t\tcontinue;\n \n@@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev)\n \tunsigned long size;\n \tenum object_type type;\n \tchar *buf = repo_read_object_file(the_repository, oid, &type, &size);\n-\tint offset = 0;\n+\tunsigned long offset = 0;\n \n \tif (!buf)\n \t\treturn error(_(\"could not read object %s\"), oid_to_hex(oid));\n \n \tassert(type == OBJ_TAG);\n \twhile (offset < size && buf[offset] != '\\n') {\n-\t\tint new_offset = offset + 1;\n+\t\tunsigned long new_offset = offset + 1;\n \t\tconst char *ident;\n \t\twhile (new_offset < size && buf[new_offset++] != '\\n')\n \t\t\t; /* do nothing */\n@@ -1316,24 +1314,25 @@ static void print_signature(const char *signature, FILE *file)\n \n static char *find_branch_name(struct rev_info *rev)\n {\n-\tint i, positive = -1;\n \tstruct object_id branch_oid;\n \tconst struct object_id *tip_oid;\n \tconst char *ref, *v;\n \tchar *full_ref, *branch = NULL;\n+\tint interesting_found = 0;\n+\tsize_t idx;\n \n-\tfor (i = 0; i < rev->cmdline.nr; i++) {\n+\tfor (size_t i = 0; i < rev->cmdline.nr; i++) {\n \t\tif (rev->cmdline.rev[i].flags & UNINTERESTING)\n \t\t\tcontinue;\n-\t\tif (positive < 0)\n-\t\t\tpositive = i;\n-\t\telse\n+\t\tif (interesting_found)\n \t\t\treturn NULL;\n+\t\tinteresting_found = 1;\n+\t\tidx = i;\n \t}\n-\tif (positive < 0)\n+\tif (!interesting_found)\n \t\treturn NULL;\n-\tref = rev->cmdline.rev[positive].name;\n-\ttip_oid = &rev->cmdline.rev[positive].item->oid;\n+\tref = rev->cmdline.rev[idx].name;\n+\ttip_oid = &rev->cmdline.rev[idx].item->oid;\n \tif (repo_dwim_ref(the_repository, ref, strlen(ref), &branch_oid,\n \t\t\t  &full_ref, 0) &&\n \t    skip_prefix(full_ref, \"refs/heads/\", &v) &&\n@@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc,\n \t\tfmt_patch_suffix = cfg.fmt_patch_suffix;\n \n \t/* Make sure \"0000-$sub.patch\" gives non-negative length for $sub */\n-\tif (cfg.log.fmt_patch_name_max <= strlen(\"0000-\") + strlen(fmt_patch_suffix))\n+\tif (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen(\"0000-\") + strlen(fmt_patch_suffix)))\n \t\tcfg.log.fmt_patch_name_max = strlen(\"0000-\") + strlen(fmt_patch_suffix);\n \n \tif (cover_from_description_arg)\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509600","messageId":"20241227-b4-pks-commit-reach-sign-compare-v1-8-07c59c2aa632@pks.im","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im","subject":"[PATCH 8/9] shallow: fix -Wsign-compare warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:46:28Z","receivedAt":"2024-12-27T10:47:23Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Fix a couple of -Wsign-compare issues in \"shallow.c\" and mark the file\nas -Wsign-compare-clean. This change prepares the code for a refactoring\nof `repo_in_merge_bases_many()`, which will be adapted to accept the\nnumber of commits as `size_t` instead of `int`.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n shallow.c | 38 ++++++++++++++++++--------------------\n shallow.h |  6 +++---\n 2 files changed, 21 insertions(+), 23 deletions(-)\n\ndiff --git a/shallow.c b/shallow.c\nindex 82a8da3d730b2f4f6bda462c8ac347f0b10da993..b8fcfbef0f9cdbfa895d4d6e1214f67942f88d25 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -1,5 +1,4 @@\n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"git-compat-util.h\"\n #include \"hex.h\"\n@@ -134,7 +133,8 @@ static void free_depth_in_slab(int **ptr)\n struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n \t\tint shallow_flag, int not_shallow_flag)\n {\n-\tint i = 0, cur_depth = 0;\n+\tsize_t i = 0;\n+\tint cur_depth = 0;\n \tstruct commit_list *result = NULL;\n \tstruct object_array stack = OBJECT_ARRAY_INIT;\n \tstruct commit *commit = NULL;\n@@ -335,16 +335,16 @@ static int write_shallow_commits_1(struct strbuf *out, int use_pack_protocol,\n \t\t\t\t   const struct oid_array *extra,\n \t\t\t\t   unsigned flags)\n {\n-\tstruct write_shallow_data data;\n-\tint i;\n-\tdata.out = out;\n-\tdata.use_pack_protocol = use_pack_protocol;\n-\tdata.count = 0;\n-\tdata.flags = flags;\n+\tstruct write_shallow_data data = {\n+\t\t.out = out,\n+\t\t.use_pack_protocol = use_pack_protocol,\n+\t\t.flags = flags,\n+\t};\n+\n \tfor_each_commit_graft(write_one_shallow, &data);\n \tif (!extra)\n \t\treturn data.count;\n-\tfor (i = 0; i < extra->nr; i++) {\n+\tfor (size_t i = 0; i < extra->nr; i++) {\n \t\tstrbuf_addstr(out, oid_to_hex(extra->oid + i));\n \t\tstrbuf_addch(out, '\\n');\n \t\tdata.count++;\n@@ -466,7 +466,6 @@ struct trace_key trace_shallow = TRACE_KEY_INIT(SHALLOW);\n  */\n void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)\n {\n-\tint i;\n \ttrace_printf_key(&trace_shallow, \"shallow: prepare_shallow_info\\n\");\n \tmemset(info, 0, sizeof(*info));\n \tinfo->shallow = sa;\n@@ -474,7 +473,7 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)\n \t\treturn;\n \tALLOC_ARRAY(info->ours, sa->nr);\n \tALLOC_ARRAY(info->theirs, sa->nr);\n-\tfor (i = 0; i < sa->nr; i++) {\n+\tfor (size_t i = 0; i < sa->nr; i++) {\n \t\tif (repo_has_object_file(the_repository, sa->oid + i)) {\n \t\t\tstruct commit_graft *graft;\n \t\t\tgraft = lookup_commit_graft(the_repository,\n@@ -507,7 +506,7 @@ void clear_shallow_info(struct shallow_info *info)\n void remove_nonexistent_theirs_shallow(struct shallow_info *info)\n {\n \tstruct object_id *oid = info->shallow->oid;\n-\tint i, dst;\n+\tsize_t i, dst;\n \ttrace_printf_key(&trace_shallow, \"shallow: remove_nonexistent_theirs_shallow\\n\");\n \tfor (i = dst = 0; i < info->nr_theirs; i++) {\n \t\tif (i != dst)\n@@ -560,7 +559,7 @@ static void paint_down(struct paint_info *info, const struct object_id *oid,\n {\n \tunsigned int i, nr;\n \tstruct commit_list *head = NULL;\n-\tint bitmap_nr = DIV_ROUND_UP(info->nr_bits, 32);\n+\tsize_t bitmap_nr = DIV_ROUND_UP(info->nr_bits, 32);\n \tsize_t bitmap_size = st_mult(sizeof(uint32_t), bitmap_nr);\n \tstruct commit *c = lookup_commit_reference_gently(the_repository, oid,\n \t\t\t\t\t\t\t  1);\n@@ -660,7 +659,7 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,\n \tstruct object_id *oid = info->shallow->oid;\n \tstruct oid_array *ref = info->ref;\n \tunsigned int i, nr;\n-\tint *shallow, nr_shallow = 0;\n+\tsize_t *shallow, nr_shallow = 0;\n \tstruct paint_info pi;\n \n \ttrace_printf_key(&trace_shallow, \"shallow: assign_shallow_commits_to_refs\\n\");\n@@ -735,7 +734,7 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,\n \n struct commit_array {\n \tstruct commit **commits;\n-\tint nr, alloc;\n+\tsize_t nr, alloc;\n };\n \n static int add_ref(const char *refname UNUSED,\n@@ -753,12 +752,11 @@ static int add_ref(const char *refname UNUSED,\n \treturn 0;\n }\n \n-static void update_refstatus(int *ref_status, int nr, uint32_t *bitmap)\n+static void update_refstatus(int *ref_status, size_t nr, uint32_t *bitmap)\n {\n-\tunsigned int i;\n \tif (!ref_status)\n \t\treturn;\n-\tfor (i = 0; i < nr; i++)\n+\tfor (size_t i = 0; i < nr; i++)\n \t\tif (bitmap[i / 32] & (1U << (i % 32)))\n \t\t\tref_status[i]++;\n }\n@@ -773,8 +771,8 @@ static void post_assign_shallow(struct shallow_info *info,\n \tstruct object_id *oid = info->shallow->oid;\n \tstruct commit *c;\n \tuint32_t **bitmap;\n-\tint dst, i, j;\n-\tint bitmap_nr = DIV_ROUND_UP(info->ref->nr, 32);\n+\tsize_t dst, i, j;\n+\tsize_t bitmap_nr = DIV_ROUND_UP(info->ref->nr, 32);\n \tstruct commit_array ca;\n \n \ttrace_printf_key(&trace_shallow, \"shallow: post_assign_shallow\\n\");\ndiff --git a/shallow.h b/shallow.h\nindex e9ca7e4bc80451a74dc10fb6d4e1a0b190dc4dcc..9bfeade93ead7421d7f2991862dde88ba546fc64 100644\n--- a/shallow.h\n+++ b/shallow.h\n@@ -59,8 +59,8 @@ void prune_shallow(unsigned options);\n  */\n struct shallow_info {\n \tstruct oid_array *shallow;\n-\tint *ours, nr_ours;\n-\tint *theirs, nr_theirs;\n+\tsize_t *ours, nr_ours;\n+\tsize_t *theirs, nr_theirs;\n \tstruct oid_array *ref;\n \n \t/* for receive-pack */\n@@ -69,7 +69,7 @@ struct shallow_info {\n \tint *reachable;\n \tint *shallow_ref;\n \tstruct commit **commits;\n-\tint nr_commits;\n+\tsize_t nr_commits;\n };\n \n void prepare_shallow_info(struct shallow_info *, struct oid_array *);\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509601","messageId":"20241227-b4-pks-commit-reach-sign-compare-v1-9-07c59c2aa632@pks.im","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im","subject":"[PATCH 9/9] commit-reach: use `size_t` to track indices when computing merge bases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T10:46:29Z","receivedAt":"2024-12-27T10:47:24Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The functions `repo_get_merge_bases_many()` and friends accepts an array\nof commits as well as a parameter that indicates how large that array\nis. This parameter is using a signed integer, which leads to a couple of\nwarnings with -Wsign-compare.\n\nRefactor the code to use `size_t` to track indices instead and adapt\ncallers accordingly. While most callers are trivial, there are two\ncallers that require a bit more scrutiny:\n\n  - builtin/merge-base.c:show_merge_base() subtracts `1` from the\n    `rev_nr` before calling `repo_get_merge_bases_many_dirty()`, so if\n    the variable was `0` it would wrap. This code is fine though because\n    its only caller will execute that code only when `argc >= 2`, and it\n    follows that `rev_nr >= 2`, as well.\n\n  - bisect.ccheck_merge_bases() similarly subtracts `1` from `rev_nr`.\n    Again, there is only a single caller that populates `rev_nr` with\n    `good_revs.nr`. And because a bisection always requires at least one\n    good revision it follws that `rev_nr >= 1`.\n\nMark the file as -Wsign-compare-clean.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n bisect.c              | 2 +-\n builtin/merge-base.c  | 4 ++--\n commit-reach.c        | 7 +++----\n commit-reach.h        | 4 ++--\n t/helper/test-reach.c | 6 +++---\n 5 files changed, 11 insertions(+), 12 deletions(-)\n\ndiff --git a/bisect.c b/bisect.c\nindex 7a1afc46e5fc0250212f5b6eaf952cf8e36b56fe..7a3c77c6d84da0cb6e135b6e0b1ca3596903af5c 100644\n--- a/bisect.c\n+++ b/bisect.c\n@@ -855,7 +855,7 @@ static void handle_skipped_merge_base(const struct object_id *mb)\n  * for early success, this will be converted back to 0 in\n  * check_good_are_ancestors_of_bad().\n  */\n-static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)\n+static enum bisect_error check_merge_bases(size_t rev_nr, struct commit **rev, int no_checkout)\n {\n \tenum bisect_error res = BISECT_OK;\n \tstruct commit_list *result = NULL;\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex a20c93b11aaa0b9f380b843469da2a3b5db10d00..123c81515e1f5f12045e4073f78b6e8a2b04bb4b 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -8,7 +8,7 @@\n #include \"parse-options.h\"\n #include \"commit-reach.h\"\n \n-static int show_merge_base(struct commit **rev, int rev_nr, int show_all)\n+static int show_merge_base(struct commit **rev, size_t rev_nr, int show_all)\n {\n \tstruct commit_list *result = NULL, *r;\n \n@@ -149,7 +149,7 @@ int cmd_merge_base(int argc,\n \t\t   struct repository *repo UNUSED)\n {\n \tstruct commit **rev;\n-\tint rev_nr = 0;\n+\tsize_t rev_nr = 0;\n \tint show_all = 0;\n \tint cmdmode = 0;\n \tint ret;\ndiff --git a/commit-reach.c b/commit-reach.c\nindex bab40f557580476d59d3a0b0ef56f40263e6615e..a339e41aa4ed1e375ee6f2f42a163ff1c654c3e4 100644\n--- a/commit-reach.c\n+++ b/commit-reach.c\n@@ -1,5 +1,4 @@\n #define USE_THE_REPOSITORY_VARIABLE\n-#define DISABLE_SIGN_COMPARE_WARNINGS\n \n #include \"git-compat-util.h\"\n #include \"commit.h\"\n@@ -421,7 +420,7 @@ static int remove_redundant(struct repository *r, struct commit **array,\n \n static int get_merge_bases_many_0(struct repository *r,\n \t\t\t\t  struct commit *one,\n-\t\t\t\t  int n,\n+\t\t\t\t  size_t n,\n \t\t\t\t  struct commit **twos,\n \t\t\t\t  int cleanup,\n \t\t\t\t  struct commit_list **result)\n@@ -469,7 +468,7 @@ static int get_merge_bases_many_0(struct repository *r,\n \n int repo_get_merge_bases_many(struct repository *r,\n \t\t\t      struct commit *one,\n-\t\t\t      int n,\n+\t\t\t      size_t n,\n \t\t\t      struct commit **twos,\n \t\t\t      struct commit_list **result)\n {\n@@ -478,7 +477,7 @@ int repo_get_merge_bases_many(struct repository *r,\n \n int repo_get_merge_bases_many_dirty(struct repository *r,\n \t\t\t\t    struct commit *one,\n-\t\t\t\t    int n,\n+\t\t\t\t    size_t n,\n \t\t\t\t    struct commit **twos,\n \t\t\t\t    struct commit_list **result)\n {\ndiff --git a/commit-reach.h b/commit-reach.h\nindex fa5408054ac01372c041d18595a405cfdaec6af3..6012402dfcfe453fd710d0b4c9a9e09f8953f63a 100644\n--- a/commit-reach.h\n+++ b/commit-reach.h\n@@ -14,12 +14,12 @@ int repo_get_merge_bases(struct repository *r,\n \t\t\t struct commit *rev2,\n \t\t\t struct commit_list **result);\n int repo_get_merge_bases_many(struct repository *r,\n-\t\t\t      struct commit *one, int n,\n+\t\t\t      struct commit *one, size_t n,\n \t\t\t      struct commit **twos,\n \t\t\t      struct commit_list **result);\n /* To be used only when object flags after this call no longer matter */\n int repo_get_merge_bases_many_dirty(struct repository *r,\n-\t\t\t\t    struct commit *one, int n,\n+\t\t\t\t    struct commit *one, size_t n,\n \t\t\t\t    struct commit **twos,\n \t\t\t\t    struct commit_list **result);\n \ndiff --git a/t/helper/test-reach.c b/t/helper/test-reach.c\nindex 01cf77ae65b9a7db2f31a9a1558b8bb84b2e81d3..028ec0030678284eba844e121c6eff88abdd3139 100644\n--- a/t/helper/test-reach.c\n+++ b/t/helper/test-reach.c\n@@ -35,7 +35,7 @@ int cmd__reach(int ac, const char **av)\n \tstruct commit_list *X, *Y;\n \tstruct object_array X_obj = OBJECT_ARRAY_INIT;\n \tstruct commit **X_array, **Y_array;\n-\tint X_nr, X_alloc, Y_nr, Y_alloc;\n+\tsize_t X_nr, X_alloc, Y_nr, Y_alloc;\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct repository *r = the_repository;\n \n@@ -157,7 +157,7 @@ int cmd__reach(int ac, const char **av)\n \t\tclear_contains_cache(&cache);\n \t} else if (!strcmp(av[1], \"get_reachable_subset\")) {\n \t\tconst int reachable_flag = 1;\n-\t\tint i, count = 0;\n+\t\tint count = 0;\n \t\tstruct commit_list *current;\n \t\tstruct commit_list *list = get_reachable_subset(X_array, X_nr,\n \t\t\t\t\t\t\t\tY_array, Y_nr,\n@@ -169,7 +169,7 @@ int cmd__reach(int ac, const char **av)\n \t\t\t\t    oid_to_hex(&list->item->object.oid));\n \t\t\tcount++;\n \t\t}\n-\t\tfor (i = 0; i < Y_nr; i++) {\n+\t\tfor (size_t i = 0; i < Y_nr; i++) {\n \t\t\tif (Y_array[i]->object.flags & reachable_flag)\n \t\t\t\tcount--;\n \t\t}\n\n-- \n2.48.0.rc0.184.g0fc57dec57.dirty\n\n"},{"id":"509608","messageId":"Z26p9GJbmyUd6bG-@ArchLinux","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-7-07c59c2aa632@pks.im","subject":"Re: [PATCH 7/9] builtin/log: fix remaining -Wsign-compare warnings","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-12-27T13:21:56Z","receivedAt":"2024-12-27T13:21:06Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Fri, Dec 27, 2024 at 11:46:27AM +0100, Patrick Steinhardt wrote:\n\n[snip]\n\n> @@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev)\n>  \tunsigned long size;\n>  \tenum object_type type;\n>  \tchar *buf = repo_read_object_file(the_repository, oid, &type, &size);\n> -\tint offset = 0;\n> +\tunsigned long offset = 0;\n\nWhy here we use `unsigned long`, is this a special situation where we\ncannot use `size_t`?\n\n>  \n>  \tif (!buf)\n>  \t\treturn error(_(\"could not read object %s\"), oid_to_hex(oid));\n>  \n>  \tassert(type == OBJ_TAG);\n>  \twhile (offset < size && buf[offset] != '\\n') {\n> -\t\tint new_offset = offset + 1;\n> +\t\tunsigned long new_offset = offset + 1;\n>  \t\tconst char *ident;\n>  \t\twhile (new_offset < size && buf[new_offset++] != '\\n')\n>  \t\t\t; /* do nothing */\n\n> @@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc,\n>  \t\tfmt_patch_suffix = cfg.fmt_patch_suffix;\n>  \n>  \t/* Make sure \"0000-$sub.patch\" gives non-negative length for $sub */\n> -\tif (cfg.log.fmt_patch_name_max <= strlen(\"0000-\") + strlen(fmt_patch_suffix))\n> +\tif (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen(\"0000-\") + strlen(fmt_patch_suffix)))\n\nA design question, why we don't change the type of\n`cfg.log.fmt_patch_name_max` to be `size_t`?\n\n>  \t\tcfg.log.fmt_patch_name_max = strlen(\"0000-\") + strlen(fmt_patch_suffix);\n>  \n>  \tif (cover_from_description_arg)\n> \n> -- \n> 2.48.0.rc0.184.g0fc57dec57.dirty\n> \n\nThanks,\nJialuo\n"},{"id":"509611","messageId":"Z26yPlLMlxyecZVk@pks.im","threadId":"62697","inReplyTo":"Z26p9GJbmyUd6bG-@ArchLinux","subject":"Re: [PATCH 7/9] builtin/log: fix remaining -Wsign-compare warnings","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-27T13:57:18Z","receivedAt":"2024-12-27T13:57:42Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 27, 2024 at 09:21:56PM +0800, shejialuo wrote:\n> On Fri, Dec 27, 2024 at 11:46:27AM +0100, Patrick Steinhardt wrote:\n> > @@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev)\n> >  \tunsigned long size;\n> >  \tenum object_type type;\n> >  \tchar *buf = repo_read_object_file(the_repository, oid, &type, &size);\n> > -\tint offset = 0;\n> > +\tunsigned long offset = 0;\n> \n> Why here we use `unsigned long`, is this a special situation where we\n> cannot use `size_t`?\n\nMostly because other variables already use `unsigned long` here,\nincluding `repo_read_object_file()`. So given that our object layer\ndoesn't support `size_t` it wouldn't make sense to use it for the\noffset, either.\n\n> >  \n> >  \tif (!buf)\n> >  \t\treturn error(_(\"could not read object %s\"), oid_to_hex(oid));\n> >  \n> >  \tassert(type == OBJ_TAG);\n> >  \twhile (offset < size && buf[offset] != '\\n') {\n> > -\t\tint new_offset = offset + 1;\n> > +\t\tunsigned long new_offset = offset + 1;\n> >  \t\tconst char *ident;\n> >  \t\twhile (new_offset < size && buf[new_offset++] != '\\n')\n> >  \t\t\t; /* do nothing */\n> \n> > @@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc,\n> >  \t\tfmt_patch_suffix = cfg.fmt_patch_suffix;\n> >  \n> >  \t/* Make sure \"0000-$sub.patch\" gives non-negative length for $sub */\n> > -\tif (cfg.log.fmt_patch_name_max <= strlen(\"0000-\") + strlen(fmt_patch_suffix))\n> > +\tif (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen(\"0000-\") + strlen(fmt_patch_suffix)))\n> \n> A design question, why we don't change the type of\n> `cfg.log.fmt_patch_name_max` to be `size_t`?\n\nThe whole infra around this uses `int`s, too, so changing this variable\nhere alone wouldn't suffice. We'd also have to adapt option handling,\nconfig handling, `struct rev_info::patch_name_max` and other bits and\npieces. So in the end the size of required changes would likely balloon\nand thus I decided to not go down this rabbit hole.\n\nPatrick\n"},{"id":"509628","messageId":"Z26zu7X7VCSX3yn9@ArchLinux","threadId":"62697","inReplyTo":"Z26yPlLMlxyecZVk@pks.im","subject":"Re: [PATCH 7/9] builtin/log: fix remaining -Wsign-compare warnings","fromName":"shejialuo","fromEmail":"shejialuo@gmail.com","sentAt":"2024-12-27T14:03:39Z","receivedAt":"2024-12-27T14:02:48Z","isPatch":true,"sender":{"key":"shejialuo@gmail.com","avatar":"https://avatars.githubusercontent.com/u/56911263?v=4"},"body":"On Fri, Dec 27, 2024 at 02:57:18PM +0100, Patrick Steinhardt wrote:\n> On Fri, Dec 27, 2024 at 09:21:56PM +0800, shejialuo wrote:\n> > On Fri, Dec 27, 2024 at 11:46:27AM +0100, Patrick Steinhardt wrote:\n> > > @@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev)\n> > >  \tunsigned long size;\n> > >  \tenum object_type type;\n> > >  \tchar *buf = repo_read_object_file(the_repository, oid, &type, &size);\n> > > -\tint offset = 0;\n> > > +\tunsigned long offset = 0;\n> > \n> > Why here we use `unsigned long`, is this a special situation where we\n> > cannot use `size_t`?\n> \n> Mostly because other variables already use `unsigned long` here,\n> including `repo_read_object_file()`. So given that our object layer\n> doesn't support `size_t` it wouldn't make sense to use it for the\n> offset, either.\n> \n\nMake sense. Thanks for the explanation.\n\n> > >  \n> > >  \tif (!buf)\n> > >  \t\treturn error(_(\"could not read object %s\"), oid_to_hex(oid));\n> > >  \n> > >  \tassert(type == OBJ_TAG);\n> > >  \twhile (offset < size && buf[offset] != '\\n') {\n> > > -\t\tint new_offset = offset + 1;\n> > > +\t\tunsigned long new_offset = offset + 1;\n> > >  \t\tconst char *ident;\n> > >  \t\twhile (new_offset < size && buf[new_offset++] != '\\n')\n> > >  \t\t\t; /* do nothing */\n> > \n> > > @@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc,\n> > >  \t\tfmt_patch_suffix = cfg.fmt_patch_suffix;\n> > >  \n> > >  \t/* Make sure \"0000-$sub.patch\" gives non-negative length for $sub */\n> > > -\tif (cfg.log.fmt_patch_name_max <= strlen(\"0000-\") + strlen(fmt_patch_suffix))\n> > > +\tif (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen(\"0000-\") + strlen(fmt_patch_suffix)))\n> > \n> > A design question, why we don't change the type of\n> > `cfg.log.fmt_patch_name_max` to be `size_t`?\n> \n> The whole infra around this uses `int`s, too, so changing this variable\n> here alone wouldn't suffice. We'd also have to adapt option handling,\n> config handling, `struct rev_info::patch_name_max` and other bits and\n> pieces. So in the end the size of required changes would likely balloon\n> and thus I decided to not go down this rabbit hole.\n> \n\nYes, I agree. If we do this, there is a lot of efforts we need to deal\nwith.\n\n> Patrick\n"},{"id":"509637","messageId":"20241227143608.GC88306@coredump.intra.peff.net","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-1-07c59c2aa632@pks.im","subject":"Re: [PATCH 1/9] prio-queue: fix type of `insertion_ctr`","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-27T14:36:08Z","receivedAt":"2024-12-27T14:36:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 27, 2024 at 11:46:21AM +0100, Patrick Steinhardt wrote:\n\n> In 62e745ced2 (prio-queue: use size_t rather than int for size,\n> 2024-12-20), we have converted `struct prio_queue` to use `size_t` to\n> track the number of entries in the queue as well as the allocated size\n> of the underlying array. There is one more counter though, namely the\n> insertion counter, that is still using an `unsigned` instead of a\n> `size_t`. This is unlikely to ever be a problem, but it makes one wonder\n> why some indices use `size_t` while others use `unsigned`. Furthermore,\n> the mentioned commit stated the intent to also adapt these variables,\n> but seemingly forgot to do so.\n> \n> Fix the issue by converting those counters to use `size_t`, as well.\n\nYep, this looks good, and was what I meant to send in 62e745ced2.\n\n-Peff\n"},{"id":"509638","messageId":"20241227144610.GD88306@coredump.intra.peff.net","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-2-07c59c2aa632@pks.im","subject":"Re: [PATCH 2/9] commit-reach: fix index used to loop through unsigned integer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-27T14:46:10Z","receivedAt":"2024-12-27T14:46:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 27, 2024 at 11:46:22AM +0100, Patrick Steinhardt wrote:\n\n> In 62e745ced2 (prio-queue: use size_t rather than int for size,\n> 2024-12-20), we refactored `struct prio_queue` to track the number of\n> contained entries via a `size_t`. While the refactoring adapted one of\n> the users of that variable, it forgot to also adapt \"commit-reach.c\"\n> accordingly. This was missed because that file has -Wsign-conversion\n> disabled.\n\nYes, that's exactly what happened (I used a merge of my series and your\n-Wsign-compare one and relied on the compiler).\n\nI grepped through for other instances and couldn't find any problems.\nMany places just do:\n\n  while (queue->nr)\n     ...remove top...\n\nwhich is fine with either type (which might explain why the problem\nwasn't more common).\n\nI thought there was one more instance in commit-graph.c, which looks at\n\"info->commits->nr\", and I was grepping for \"commits->nr\" since\nrev_info->commits is a prio_queue. But it is actually a totally\ndifferent type, compute_generation_info, and \"commits\" there is a\npacked_commit_list.\n\nThat data type _also_ has the same int/size_t problem. ;) But it is\ntotally separate, so it can happen later in its own patch (and there are\na ton of other -Wsign-compare warnings in that file).\n\nThanks for cleaning up after me.\n\n-Peff\n"},{"id":"509647","messageId":"xmqqbjwwucvy.fsf@gitster.g","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-0-07c59c2aa632@pks.im","subject":"Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-27T19:47:13Z","receivedAt":"2024-12-27T19:47:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Speaking of -Wsign-compare (cf. [*1*]), we seem to be hitting this\nerror on linux32 CI job [*2*]:\n\nError: shallow.c:537:32: comparison of integer expressions of different signedness:\n'unsigned int' and 'int' [-Werror=sign-compare]\n\n    537 |  if (!info->pool_count || size > info->end - info->free) {\n\nI didn't dig deeper than this.  After taking three tries to get\n'seen' build with linux-meson job (needed merge-fix for ds/backfill\ntopic, which needed (1) a new built-in hence a new entry in\nmeson.build, (2) a new test hence a new entry in t/meson.build, and\n(3) a new doc hence a new entry in Documentation/meson.build), I am\na bit exhausted right now.\n\nAlso breakage of linux-meson job we have at 'master' seems gone,\nwhich probably has to do with your recent update with gitweb thing.\n\nThanks.\n\n\n[References]\n\n*1* \n\nhttps://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/\n\n*2*\n\nhttps://github.com/git/git/actions/runs/12519901432/job/34924707204#step:6:181\n"},{"id":"509648","messageId":"xmqq7c7kubx8.fsf@gitster.g","threadId":"62697","inReplyTo":"xmqqbjwwucvy.fsf@gitster.g","subject":"Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-27T20:08:03Z","receivedAt":"2024-12-27T20:08:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Error: shallow.c:537:32: comparison of integer expressions of different signedness:\n> 'unsigned int' and 'int' [-Werror=sign-compare]\n>\n>     537 |  if (!info->pool_count || size > info->end - info->free) {\n>\n> I didn't dig deeper than this.\n\nWhat we want to express seems to be:\n\n    If the region between \"end\" and \"free\" is smaller than the\n    required \"size\" then we are in trouble.\n\nwhich can be said more naturally with\n\n    If the region of size \"size\" starting at \"free\" pointer overruns the\n    \"end\" pointer, we are in trouble.\n\nSo perhaps something like this would help?  We are no longer making\na comparison between two integers with this rewrite.\n\n shallow.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git c/shallow.c w/shallow.c\nindex b8fcfbef0f..b54244ffa9 100644\n--- c/shallow.c\n+++ w/shallow.c\n@@ -534,7 +534,7 @@ static uint32_t *paint_alloc(struct paint_info *info)\n \tunsigned nr = DIV_ROUND_UP(info->nr_bits, 32);\n \tunsigned size = nr * sizeof(uint32_t);\n \tvoid *p;\n-\tif (!info->pool_count || size > info->end - info->free) {\n+\tif (!info->pool_count || info->end < info->free + size) {\n \t\tif (size > POOL_SIZE)\n \t\t\tBUG(\"pool size too small for %d in paint_alloc()\",\n \t\t\t    size);\n"},{"id":"509654","messageId":"20241227213729.GA796141@coredump.intra.peff.net","threadId":"62697","inReplyTo":"xmqq7c7kubx8.fsf@gitster.g","subject":"Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-27T21:37:29Z","receivedAt":"2024-12-27T21:37:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Dec 27, 2024 at 12:08:03PM -0800, Junio C Hamano wrote:\n\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > Error: shallow.c:537:32: comparison of integer expressions of different signedness:\n> > 'unsigned int' and 'int' [-Werror=sign-compare]\n> >\n> >     537 |  if (!info->pool_count || size > info->end - info->free) {\n> >\n> > I didn't dig deeper than this.\n> \n> What we want to express seems to be:\n> \n>     If the region between \"end\" and \"free\" is smaller than the\n>     required \"size\" then we are in trouble.\n> \n> which can be said more naturally with\n> \n>     If the region of size \"size\" starting at \"free\" pointer overruns the\n>     \"end\" pointer, we are in trouble.\n> \n> So perhaps something like this would help?  We are no longer making\n> a comparison between two integers with this rewrite.\n> \n>  shallow.c | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n> \n> diff --git c/shallow.c w/shallow.c\n> index b8fcfbef0f..b54244ffa9 100644\n> --- c/shallow.c\n> +++ w/shallow.c\n> @@ -534,7 +534,7 @@ static uint32_t *paint_alloc(struct paint_info *info)\n>  \tunsigned nr = DIV_ROUND_UP(info->nr_bits, 32);\n>  \tunsigned size = nr * sizeof(uint32_t);\n>  \tvoid *p;\n> -\tif (!info->pool_count || size > info->end - info->free) {\n> +\tif (!info->pool_count || info->end < info->free + size) {\n>  \t\tif (size > POOL_SIZE)\n>  \t\t\tBUG(\"pool size too small for %d in paint_alloc()\",\n>  \t\t\t    size);\n\nI doubt it is a practical problem in this instance, but as a general\nrule, I think bounds checks like this should avoid using addition\nbecause it can lead to overflow. In this code, imagine that the free\npool was allocated near the end of the address space, but somebody asked\nfor a very large \"size\", causing the addition to wrap around.\n\nI think even without that overflow this might be undefined behavior,\nbecause \"info->free + size\" is forming a pointer that may be outside the\nallocated array.\n\nSo as a general rule, bounds comparisons like this should be formulated\nas \"how much free space do we have\" compared to \"how much are we asking\nfor\". And there \"info->end - info->free\" is the right way to ask the\nfirst half of that question.\n\nNow where it gets weird with -Wsign-compare is that the pointer\ndifference is giving us a signed value. Which makes sense in the general\ncase (you could ask for \"info->free - info->end\", for example). But we\nknow that it will always be non-negative because we know that info->free\nis <= info->end, coming from earlier in the same allocation.\n\nI doubt there is a way to tell the compiler that (or that a compiler\ncould even switch to an unsigned ptrdiff type if it knew that). But I\nwonder if there is a generalized helper we can devise that would avoid\nsimply casting here. I guess that could be a checked cast like:\n\n  static inline size_t ptrdiff_to_size(ptrdiff_t v)\n  {\n\tif (v < 0)\n\t\tBUG(\"surprising negative value: %\"PRIdMAX, v);\n\treturn (size_t)v;\n  }\n\nor even:\n\n  static inline bool has_space(const void *vs, const void *ve, size_t want)\n  {\n\tconst char *s = vs, e = ve;\n\treturn want <= ptrdiff_to_size(ve - vs);\n  }\n\nI don't love hiding basic things like this behind macros or inlines. But\nallocation and bounds comparisons do have gotchas (especially against an\nadversary that can try to create pathological situations). Maybe it's worth\nhaving an easy way to do them safely without having to think about each\none. I dunno.\n\n-Peff\n"},{"id":"509656","messageId":"xmqqbjww65i1.fsf@gitster.g","threadId":"62697","inReplyTo":"xmqq7c7kubx8.fsf@gitster.g","subject":"Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-28T00:00:38Z","receivedAt":"2024-12-28T00:00:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> So perhaps something like this would help?  We are no longer making\n> a comparison between two integers with this rewrite.\n\n... and this gave us a first \"pass\" of the tip of 'seen' for all\njobs since for quite some time, like a few weeks.\n\n"},{"id":"509669","messageId":"Z2-2dbYVuuLxpNmK@pks.im","threadId":"62697","inReplyTo":"xmqqbjww65i1.fsf@gitster.g","subject":"Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-28T08:27:41Z","receivedAt":"2024-12-28T08:28:05Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 27, 2024 at 04:00:38PM -0800, Junio C Hamano wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n> \n> > So perhaps something like this would help?  We are no longer making\n> > a comparison between two integers with this rewrite.\n> \n> ... and this gave us a first \"pass\" of the tip of 'seen' for all\n> jobs since for quite some time, like a few weeks.\n\nThanks for your fix. I'll have a look at whether I can include a 32 bit\njob into GitLab CI for improved test coverage here so that it does not\nfall on you to fix up things like this going forward.\n\nOverall, CI has been in pretty rough shape recently. Besides the\nmentioned failures there's also been the issue with flaky tests:\n\n  - t5616-partial-clone regularly fails on macOS. [1] This seems like a\n    race condition or to me:\n\n    error: unable to open .git/objects/pack/pack-029d08823bd8a8eab510ad6ac75c823cfd3ed31e.pack: No such file or directory\n    fatal: unable to rename temporary file to '.git/objects/pack/pack-029d08823bd8a8eab510ad6ac75c823cfd3ed31e.pack'\n    fatal: could not finish pack-objects to repack local links\n    fatal: index-pack failed\n    error: last command exited with $?=128\n    not ok 18 - fetch --refetch triggers repacking\n\n  - The leak-checking jobs fail quite regularly in t0003 with something\n    that feels like either a race caused by a leak or an issue with the\n    sanitizer itself [2]:\n\n    ==git==17055==ERROR: LeakSanitizer: detected memory leaks\n    Direct leak of 32 byte(s) in 1 object(s) allocated from:\n    #0 0x7aa0d03c7713 in realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98\n    #1 0x7aa0d0221f69 in pthread_getattr_np (/lib/x86_64-linux-gnu/libc.so.6+0x9df69) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)\n    #2 0x7aa0d03d9544 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150\n    #3 0x7aa0d03d96fa in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:614\n    #4 0x7aa0d03cb2b9 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:53\n    #5 0x7aa0d03c756a in ThreadStartFunc<false> ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:431\n    #6 0x7aa0d0220a93  (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)\n    #7 0x7aa0d02adc3b  (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)\n    DEDUP_TOKEN: ___interceptor_realloc--pthread_getattr_np--__sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*)--__sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*)--__lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType)--ThreadStartFunc<false>----\n    SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).\n\n  - Windows has been quite flaky since adding it to GitLab CI. No idea\n    whether it's the same for GitHub Actions.\n\nThe thing is, the less reliable it becomes the more likely it is that\npeople are simply going to ignore its results.\n\nI'll try to have a look at some of the problems on Monday.\n\nPatrick\n\n[1]: https://gitlab.com/git-scm/git/-/jobs/8671168627\n[2]: https://gitlab.com/git-scm/git/-/jobs/8671168849\n"},{"id":"509670","messageId":"Z2-5nnBzPKP3iPxl@pks.im","threadId":"62697","inReplyTo":"20241227213729.GA796141@coredump.intra.peff.net","subject":"Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-12-28T08:41:02Z","receivedAt":"2024-12-28T08:41:25Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Fri, Dec 27, 2024 at 04:37:29PM -0500, Jeff King wrote:\n> On Fri, Dec 27, 2024 at 12:08:03PM -0800, Junio C Hamano wrote:\n> I doubt there is a way to tell the compiler that (or that a compiler\n> could even switch to an unsigned ptrdiff type if it knew that). But I\n> wonder if there is a generalized helper we can devise that would avoid\n> simply casting here. I guess that could be a checked cast like:\n> \n>   static inline size_t ptrdiff_to_size(ptrdiff_t v)\n>   {\n> \tif (v < 0)\n> \t\tBUG(\"surprising negative value: %\"PRIdMAX, v);\n> \treturn (size_t)v;\n>   }\n> \n> or even:\n> \n>   static inline bool has_space(const void *vs, const void *ve, size_t want)\n>   {\n> \tconst char *s = vs, e = ve;\n> \treturn want <= ptrdiff_to_size(ve - vs);\n>   }\n> \n> I don't love hiding basic things like this behind macros or inlines. But\n> allocation and bounds comparisons do have gotchas (especially against an\n> adversary that can try to create pathological situations). Maybe it's worth\n> having an easy way to do them safely without having to think about each\n> one. I dunno.\n\nI think having a wrapper like `cast_ptrdiff_to_size_t()` would be a\nsensible solution for now, also because it fits in nicely with\n`cast_size_t_to_int()`. I'll introduce such a wrapper once I've got a\ngood excuse to do so.\n\nPatrick\n"},{"id":"509681","messageId":"xmqq1pxr6cmn.fsf@gitster.g","threadId":"62697","inReplyTo":"Z2-2dbYVuuLxpNmK@pks.im","subject":"Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-28T15:38:56Z","receivedAt":"2024-12-28T15:38:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt <ps@pks.im> writes:\n\n> Thanks for your fix. I'll have a look at whether I can include a 32 bit\n> job into GitLab CI for improved test coverage here so that it does not\n> fall on you to fix up things like this going forward.\n\nI noticed it since it failed GitHub actions thing which already has 32-bit\njob.\n\n>   - t5616-partial-clone regularly fails on macOS. [1] This seems like a\n>     race condition or to me:\n\nI've seen it before as well at GitHub actions side.  Running\n\"t5616-*.sh --stress\" locally on Debian (x86-64) did not help\nisolate it very well.\n\n>   - The leak-checking jobs fail quite regularly in t0003 with something\n>     that feels like either a race caused by a leak or an issue with the\n>     sanitizer itself [2]:\n\nThis one I am not aware of.\n\n>   - Windows has been quite flaky since adding it to GitLab CI. No idea\n>     whether it's the same for GitHub Actions.\n\nSimilar on GitHub CI front.  Not that I am playing favors between\nGitHub and GitLab, but for historical reasons I've pushed to the\nformer myself but not to the latter, so I do not notice breakages on\nthe latter.\n\n\n> The thing is, the less reliable it becomes the more likely it is that\n> people are simply going to ignore its results.\n\nIndeed.  Also, for macOS and Windows, I have no access to an\nenvironment to let me debug, so it is really up to the platform\nstakeholders to see what they can do to help.\n\nThanks.\n\n"},{"id":"509694","messageId":"20241228190541.GA815586@coredump.intra.peff.net","threadId":"62697","inReplyTo":"Z2-2dbYVuuLxpNmK@pks.im","subject":"racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-28T19:05:41Z","receivedAt":"2024-12-28T19:05:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 28, 2024 at 09:27:41AM +0100, Patrick Steinhardt wrote:\n\n>   - The leak-checking jobs fail quite regularly in t0003 with something\n>     that feels like either a race caused by a leak or an issue with the\n>     sanitizer itself [2]:\n> \n>     ==git==17055==ERROR: LeakSanitizer: detected memory leaks\n>     Direct leak of 32 byte(s) in 1 object(s) allocated from:\n>     #0 0x7aa0d03c7713 in realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98\n>     #1 0x7aa0d0221f69 in pthread_getattr_np (/lib/x86_64-linux-gnu/libc.so.6+0x9df69) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)\n>     #2 0x7aa0d03d9544 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150\n>     #3 0x7aa0d03d96fa in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:614\n>     #4 0x7aa0d03cb2b9 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:53\n>     #5 0x7aa0d03c756a in ThreadStartFunc<false> ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:431\n>     #6 0x7aa0d0220a93  (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)\n>     #7 0x7aa0d02adc3b  (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)\n>     DEDUP_TOKEN: ___interceptor_realloc--pthread_getattr_np--__sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*)--__sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*)--__lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType)--ThreadStartFunc<false>----\n>     SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).\n\nI did a bit of digging on this last week, but didn't come up with a very\nsatisfactory solution. You can reproduce easily by building with\nSANITIZE=leak and running t0003 with --stress.\n\nIt's the same issue we tried to address in 993d38a066 (index-pack: spawn\nthreads atomically, 2024-01-05): one thread calls exit() and takes down\nthe process while other threads are still being spawned (so the leak\nchecker runs while those other threads are in a weird state where lsan\nhas allocated some memory but not yet set up the thread stack as a place\nto look for reachable memory).\n\nThere we dealt with it by making sure no thread started work (and thus\nhit the exit call) until all of them were sanitized. I tried doing\nsomething similar here, like:\n\n  diff --git a/builtin/grep.c b/builtin/grep.c\n  index 98b85c7fca..866645e6f8 100644\n  --- a/builtin/grep.c\n  +++ b/builtin/grep.c\n  @@ -233,18 +233,20 @@ static void start_threads(struct grep_opt *opt)\n                  strbuf_init(&todo[i].out, 0);\n          }\n\n  +       grep_lock();\n          CALLOC_ARRAY(threads, num_threads);\n          for (i = 0; i < num_threads; i++) {\n                  int err;\n                  struct grep_opt *o = grep_opt_dup(opt);\n                  o->output = strbuf_out;\n                  compile_grep_patterns(o);\n                  err = pthread_create(&threads[i], NULL, run, o);\n\n                  if (err)\n                          die(_(\"grep: failed to create thread: %s\"),\n                              strerror(err));\n          }\n  +       grep_unlock();\n   }\n\n   static int wait_all(void)\n\nbut it doesn't work. In fact, this does nothing at all because each\nthread will start by looking for work to do via get_work(), and we do\nnot call add_work() to give them anything to do until all threads are\nspawned.\n\nSo I suspect the race is actually trickier, and that the \"weird state\"\nis not something that happens just while pthread_create() is being\ncalled, but is actually running _in the thread itself_. So even though\npthread_create() has returned for each thread, they are still setting\nthemselves up before running.\n\nIt mostly worked in 993d38a066 because index-pack has to do more work to\nget to the exit() call. So delaying the start of the threads was enough\nto usually win the race. But here, the individual grep threads get to\nthe exit() call very quickly, and it's not enough.\n\nWhich would mean that 993d38a066 is not actually a full fix, either. And\nindeed, I can get t5309 to fail even with that patch (which is weird,\nbecause that was how I tested it originally; I wonder if anything on the\nLSan side changed?).\n\nSo a full fix would actually require synchronization where we spawn each\nthread, then wait for all of them to hit the barrier to declare\nthemselves ready, and then let them all start running. There is a\npthread_barrier type that would help with this, but we've never used it\nbefore (so we'd probably need to at least provide a Windows compat\nlayer).\n\nOne quick workaround is this:\n\ndiff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh\nindex 3c98b622f2..7ecaf8f4e3 100755\n--- a/t/t0003-attributes.sh\n+++ b/t/t0003-attributes.sh\n@@ -443,7 +443,7 @@ test_expect_success 'diff without repository with attr source' '\n \t\tcat >expect <<-EOF &&\n \t\tfatal: cannot use --attr-source or GIT_ATTR_SOURCE without repo\n \t\tEOF\n-\t\ttest_must_fail env GIT_ATTR_SOURCE=HEAD git grep --no-index foo file 2>err &&\n+\t\ttest_must_fail env GIT_ATTR_SOURCE=HEAD git grep --threads=1 --no-index foo file 2>err &&\n \t\ttest_cmp expect err\n \t)\n '\n\nOr you could even imagine automatically forcing online_cpus() to \"1\" for\nLSan builds, which would fix it everywhere. But then we'd miss any leaks\nthat are specific to the threaded code.\n\nOr of course we could try to engage with LSan folks about whether this\ncan be fixed there. I don't think we've reported it anywhere there.\n\n-Peff\n"},{"id":"509696","messageId":"20241228192307.GC815586@coredump.intra.peff.net","threadId":"62697","inReplyTo":"20241228190541.GA815586@coredump.intra.peff.net","subject":"Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-28T19:23:07Z","receivedAt":"2024-12-28T19:23:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 28, 2024 at 02:05:41PM -0500, Jeff King wrote:\n\n> So I suspect the race is actually trickier, and that the \"weird state\"\n> is not something that happens just while pthread_create() is being\n> called, but is actually running _in the thread itself_. So even though\n> pthread_create() has returned for each thread, they are still setting\n> themselves up before running.\n> [...]\n> So a full fix would actually require synchronization where we spawn each\n> thread, then wait for all of them to hit the barrier to declare\n> themselves ready, and then let them all start running. There is a\n> pthread_barrier type that would help with this, but we've never used it\n> before (so we'd probably need to at least provide a Windows compat\n> layer).\n\nSo here's a fix that seems to work, but doesn't address the portability\nissues. One tweak is that the exit() call is actually not in a\nsub-thread, but in the main thread itself. I don't think that really\nchanges the analysis too much, but it does mean we need to include the\nmain thread in the barrier wait.\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex d00ee76f24..933b4503b8 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -101,6 +101,9 @@ static pthread_cond_t cond_write;\n /* Signalled when we are finished with everything. */\n static pthread_cond_t cond_result;\n \n+/* Synchronize the start of all threads */\n+static pthread_barrier_t start_barrier;\n+\n static int skip_first_line;\n \n static void add_work(struct grep_opt *opt, struct grep_source *gs)\n@@ -198,6 +201,8 @@ static void *run(void *arg)\n \tint hit = 0;\n \tstruct grep_opt *opt = arg;\n \n+\tpthread_barrier_wait(&start_barrier);\n+\n \twhile (1) {\n \t\tstruct work_item *w = get_work();\n \t\tif (!w)\n@@ -229,6 +234,7 @@ static void start_threads(struct grep_opt *opt)\n \tpthread_cond_init(&cond_add, NULL);\n \tpthread_cond_init(&cond_write, NULL);\n \tpthread_cond_init(&cond_result, NULL);\n+\tpthread_barrier_init(&start_barrier, NULL, num_threads + 1);\n \tgrep_use_locks = 1;\n \tenable_obj_read_lock();\n \n@@ -248,6 +254,7 @@ static void start_threads(struct grep_opt *opt)\n \t\t\tdie(_(\"grep: failed to create thread: %s\"),\n \t\t\t    strerror(err));\n \t}\n+\tpthread_barrier_wait(&start_barrier);\n }\n \n static int wait_all(void)\n@@ -284,6 +291,7 @@ static int wait_all(void)\n \tpthread_cond_destroy(&cond_add);\n \tpthread_cond_destroy(&cond_write);\n \tpthread_cond_destroy(&cond_result);\n+\tpthread_barrier_destroy(&start_barrier);\n \tgrep_use_locks = 0;\n \tdisable_obj_read_lock();\n \n\nIt would probably be pretty easy to do something similar for index-pack,\nthough really this is something I guess we'd need to do manually\nanywhere we spawn worker threads.\n\n-Peff\n"},{"id":"509697","messageId":"20241228193151.GA1515174@coredump.intra.peff.net","threadId":"62697","inReplyTo":"20241228192307.GC815586@coredump.intra.peff.net","subject":"Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-28T19:31:51Z","receivedAt":"2024-12-28T19:31:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Dec 28, 2024 at 02:23:07PM -0500, Jeff King wrote:\n\n> It would probably be pretty easy to do something similar for index-pack,\n> though really this is something I guess we'd need to do manually\n> anywhere we spawn worker threads.\n\nHere's what that would look like. Confirmed that this fixes the issue\nrunning t5309 with --stress. Dropping the extra locks in\nresolve_deltas() is just backing out that earlier insufficient attempt\nto synchronize things.\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex d773809c4c..de1f5d8628 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -185,6 +185,8 @@ static pthread_mutex_t deepest_delta_mutex;\n \n static pthread_key_t key;\n \n+static pthread_barrier_t start_barrier;\n+\n static inline void lock_mutex(pthread_mutex_t *mutex)\n {\n \tif (threads_active)\n@@ -209,6 +211,7 @@ static void init_thread(void)\n \tif (show_stat)\n \t\tpthread_mutex_init(&deepest_delta_mutex, NULL);\n \tpthread_key_create(&key, NULL);\n+\tpthread_barrier_init(&start_barrier, NULL, nr_threads);\n \tCALLOC_ARRAY(thread_data, nr_threads);\n \tfor (i = 0; i < nr_threads; i++) {\n \t\tthread_data[i].pack_fd = xopen(curr_pack, O_RDONLY);\n@@ -231,6 +234,7 @@ static void cleanup_thread(void)\n \tfor (i = 0; i < nr_threads; i++)\n \t\tclose(thread_data[i].pack_fd);\n \tpthread_key_delete(key);\n+\tpthread_barrier_destroy(&start_barrier);\n \tfree(thread_data);\n }\n \n@@ -1100,6 +1104,8 @@ static int compare_ref_delta_entry(const void *a, const void *b)\n \n static void *threaded_second_pass(void *data)\n {\n+\tif (threads_active)\n+\t\tpthread_barrier_wait(&start_barrier);\n \tif (data)\n \t\tset_thread_data(data);\n \tfor (;;) {\n@@ -1336,15 +1342,13 @@ static void resolve_deltas(struct pack_idx_option *opts)\n \tbase_cache_limit = opts->delta_base_cache_limit * nr_threads;\n \tif (nr_threads > 1 || getenv(\"GIT_FORCE_THREADS\")) {\n \t\tinit_thread();\n-\t\twork_lock();\n \t\tfor (i = 0; i < nr_threads; i++) {\n \t\t\tint ret = pthread_create(&thread_data[i].thread, NULL,\n \t\t\t\t\t\t threaded_second_pass, thread_data + i);\n \t\t\tif (ret)\n \t\t\t\tdie(_(\"unable to create thread: %s\"),\n \t\t\t\t    strerror(ret));\n \t\t}\n-\t\twork_unlock();\n \t\tfor (i = 0; i < nr_threads; i++)\n \t\t\tpthread_join(thread_data[i].thread, NULL);\n \t\tcleanup_thread();\n"},{"id":"509709","messageId":"965bd864-3ac0-454f-b7bb-f8e0214e9969@web.de","threadId":"62697","inReplyTo":"20241228192307.GC815586@coredump.intra.peff.net","subject":"Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2024-12-29T12:02:13Z","receivedAt":"2024-12-29T12:02:27Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 28.12.2024 um 20:23 schrieb Jeff King:\n> On Sat, Dec 28, 2024 at 02:05:41PM -0500, Jeff King wrote:\n>\n>> So I suspect the race is actually trickier, and that the \"weird state\"\n>> is not something that happens just while pthread_create() is being\n>> called, but is actually running _in the thread itself_. So even though\n>> pthread_create() has returned for each thread, they are still setting\n>> themselves up before running.\n>> [...]\n>> So a full fix would actually require synchronization where we spawn each\n>> thread, then wait for all of them to hit the barrier to declare\n>> themselves ready, and then let them all start running. There is a\n>> pthread_barrier type that would help with this, but we've never used it\n>> before (so we'd probably need to at least provide a Windows compat\n>> layer).\n>\n> So here's a fix that seems to work, but doesn't address the portability\n> issues.\n\nWindows has Synchronization Barriers.  Adding the following lines to\ncompat/win32/pthread.h at least lets your example compile and run:\n\n#include <synchapi.h>\n#define pthread_barrier_t SYNCHRONIZATION_BARRIER\n#define PTHREAD_BARRIER_SERIAL_THREAD TRUE\n#define pthread_barrier_init(b, a, c) return_0(InitializeSynchronizationBarrier((b), (c), -1))\n#define pthread_barrier_destroy(b) return_0(DeleteSynchronizationBarrier(b))\n#define pthread_barrier_wait(b) EnterSynchronizationBarrier((b), 0)\n\nCatching a non-NULL second argument to pthread_barrier_init() would be a\ngood idea in a production version.  Error handling would be a good idea\nin general, but callers would then actually have to check those errors.\n\nSynchronization Barriers were added with Windows 8 and Windows Server\n2012, Git for Windows requires higher versions, so this native\nmechanism should be usable.  Relevant links:\n\nhttps://learn.microsoft.com/en-us/windows/win32/sync/synchronization-barriers\nhttps://github.com/git-for-windows/git/wiki/FAQ\n\n\nHowever, macOS doesn't have pthread barriers.  Here's an implementation\nthat had to be fixed to satisfy Coverity, so it might be good now?\n\nhttps://github.com/libusb/hidapi/blob/master/mac/hid.c\n\nPerhaps that implementation could be used for Windows as well?  All\nfunctions it uses are provided by compat/win32/pthread.h; not sure if\nthey are sufficiently fleshed out, though.\n\nRené\n"},{"id":"509711","messageId":"20241229165715.GA3158370@coredump.intra.peff.net","threadId":"62697","inReplyTo":"965bd864-3ac0-454f-b7bb-f8e0214e9969@web.de","subject":"Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-29T16:57:15Z","receivedAt":"2024-12-29T16:57:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 29, 2024 at 01:02:13PM +0100, René Scharfe wrote:\n\n> Synchronization Barriers were added with Windows 8 and Windows Server\n> 2012, Git for Windows requires higher versions, so this native\n> mechanism should be usable.  Relevant links:\n> [...]\n> However, macOS doesn't have pthread barriers.  Here's an implementation\n> that had to be fixed to satisfy Coverity, so it might be good now?\n\nYep, that matches what I'd found so far.\n\nOne of the reasons I hadn't sent anything is that I was waffling between\ntwo approaches:\n\n  - implement barriers everywhere and just use them. More work, but we'd\n    have the tool if we wanted to use it later, and all builds behave\n    the same.\n\n  - make a \"maybe_barrier\" interface that might be a noop, and let most\n    platforms compile without them. They are not needed for correct\n    operation in most cases, but only to work around a sanitizer problem.\n    And it is not even a problem that comes up frequently; it is a race\n    that we occasionally see in CI. So enabling it only for our\n    linux-leaks CI job would be enough to dull the pain.\n\n    And there is no risk of any portability or run-time issues, because\n    the code is a noop for most builds.\n\n> https://github.com/libusb/hidapi/blob/master/mac/hid.c\n> \n> Perhaps that implementation could be used for Windows as well?  All\n> functions it uses are provided by compat/win32/pthread.h; not sure if\n> they are sufficiently fleshed out, though.\n\nYeah, I found a similar one. I think it's an undergrad OS assignment to\nimplement barriers using semaphores, but probably building on a mutex\nand cond is less horrible. ;)\n\n-Peff\n"},{"id":"509720","messageId":"20241230043242.GA112402@coredump.intra.peff.net","threadId":"62697","inReplyTo":"20241229165715.GA3158370@coredump.intra.peff.net","subject":"Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-12-30T04:32:42Z","receivedAt":"2024-12-30T04:32:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Dec 29, 2024 at 11:57:15AM -0500, Jeff King wrote:\n\n> One of the reasons I hadn't sent anything is that I was waffling between\n> two approaches:\n> \n>   - implement barriers everywhere and just use them. More work, but we'd\n>     have the tool if we wanted to use it later, and all builds behave\n>     the same.\n> \n>   - make a \"maybe_barrier\" interface that might be a noop, and let most\n>     platforms compile without them. They are not needed for correct\n>     operation in most cases, but only to work around a sanitizer problem.\n>     And it is not even a problem that comes up frequently; it is a race\n>     that we occasionally see in CI. So enabling it only for our\n>     linux-leaks CI job would be enough to dull the pain.\n> \n>     And there is no risk of any portability or run-time issues, because\n>     the code is a noop for most builds.\n\nI sent the \"maybe\" interface in this series if you want to take a look:\n\n  https://lore.kernel.org/git/20241230042325.GA112439@coredump.intra.peff.net/\n\nI'm not married to that approach, but it seemed like the easiest place\nto start. And I especially wanted to see how much hand-waving was\nrequired to justify it in the third patch. It's a fair bit. ;)\n\n-Peff\n"},{"id":"509731","messageId":"xmqqy0zx1dx5.fsf@gitster.g","threadId":"62697","inReplyTo":"20241229165715.GA3158370@coredump.intra.peff.net","subject":"Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-12-30T13:46:46Z","receivedAt":"2024-12-30T13:46:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> One of the reasons I hadn't sent anything is that I was waffling between\n> two approaches:\n>\n>   - implement barriers everywhere and just use them. More work, but we'd\n>     have the tool if we wanted to use it later, and all builds behave\n>     the same.\n>\n>   - make a \"maybe_barrier\" interface that might be a noop, and let most\n>     platforms compile without them. They are not needed for correct\n>     operation in most cases, but only to work around a sanitizer problem.\n>     And it is not even a problem that comes up frequently; it is a race\n>     that we occasionally see in CI. So enabling it only for our\n>     linux-leaks CI job would be enough to dull the pain.\n>\n>     And there is no risk of any portability or run-time issues, because\n>     the code is a noop for most builds.\n\nI love when people think before committing to an approach, and after\nseeing these two cohices, I tend to have slight preference for the\nlatter over the former.  The work will not be wasted even if it\nlater turns out that we need a full-blown barrier implementation for\nother platforms.\n\nThanks.\n\n"},{"id":"509816","messageId":"miadr6xzejrkauyoqbj2su6drnz53xd443ylvxertawa7ovuke@42bigdksj5iw","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-4-07c59c2aa632@pks.im","subject":"Re: [PATCH 4/9] commit-reach: use `size_t` to track indices in `remove_redundant()`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-01-03T01:46:57Z","receivedAt":"2025-01-03T01:49:29Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/12/27 11:46AM, Patrick Steinhardt wrote:\n> The function `remove_redundant()` gets as input an array of commits as\n> well as the size of that array and then drops redundant commits from\n> that array. It then returns either `-1` in case an error occurred, or\n> the new number of items in the array.\n> \n> The function receives and returns these sizes with a signed integer,\n> which causes several warnings with -Wsign-compare. Fix this issue by\n> consistently using `size_t` to track array indices and splitting up\n> the returned value into a returned error code and a separate out pointer\n> for the new computed size.\n> \n> Note that `get_merge_bases_many()` and related functions still track\n> array sizes as a signed integer. This will be fixed in a subsequent\n> commit.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  commit-reach.c | 53 ++++++++++++++++++++++++++++++-----------------------\n>  1 file changed, 30 insertions(+), 23 deletions(-)\n> \n> diff --git a/commit-reach.c b/commit-reach.c\n> index 9f8b2457bcc12bebf725a5276d1aec467bb7af05..d7f6f1be75e95cc834d60be719e930a77ad0518f 100644\n> --- a/commit-reach.c\n> +++ b/commit-reach.c\n> @@ -212,12 +212,13 @@ int get_octopus_merge_bases(struct commit_list *in, struct commit_list **result)\n>  }\n>  \n>  static int remove_redundant_no_gen(struct repository *r,\n> -\t\t\t\t   struct commit **array, int cnt)\n> +\t\t\t\t   struct commit **array,\n> +\t\t\t\t   size_t cnt, size_t *dedup_cnt)\n>  {\n>  \tstruct commit **work;\n>  \tunsigned char *redundant;\n> -\tint *filled_index;\n> -\tint i, j, filled;\n> +\tsize_t *filled_index;\n> +\tsize_t i, j, filled;\n>  \n>  \tCALLOC_ARRAY(work, cnt);\n>  \tredundant = xcalloc(cnt, 1);\n> @@ -267,20 +268,22 @@ static int remove_redundant_no_gen(struct repository *r,\n>  \tfor (i = filled = 0; i < cnt; i++)\n>  \t\tif (!redundant[i])\n>  \t\t\tarray[filled++] = work[i];\n> +\t*dedup_cnt = filled;\n>  \tfree(work);\n>  \tfree(redundant);\n>  \tfree(filled_index);\n> -\treturn filled;\n> +\treturn 0;\n\nPreviously the return value indicated either a potential error if its\nvalue was negative or the new count otherwise. Since we now want to make\nthe count a `size_t` to avoid -Wsign-compare warnings, we split the two\nconcerns and have a separate pointer used to record the count. This\napproach is used for each of the `remove_redundant*()` functions. Seems\nsensible to me.\n\n-Justin\n"},{"id":"509817","messageId":"3b7ep2hex2vae56p2ba2kjeextjrjcsu5jrufr7hqewrnv3z45@ftrrcuvihxxs","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-6-07c59c2aa632@pks.im","subject":"Re: [PATCH 6/9] builtin/log: use `size_t` to track indices","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-01-03T01:58:24Z","receivedAt":"2025-01-03T02:00:56Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/12/27 11:46AM, Patrick Steinhardt wrote:\n> Similar as with the preceding commit, adapt \"builtin/log.c\" so that it\n> tracks array indices via `size_t` instead of using signed integers. This\n> fixes a couple of -Wsign-compare warnings and prepares the code for\n> a similar refactoring of `repo_get_merge_bases_many()` in a subsequent\n> commit.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> ---\n>  builtin/log.c | 23 +++++++++++++----------\n>  1 file changed, 13 insertions(+), 10 deletions(-)\n> \n[snip]\n>  \tif (show_progress)\n>  \t\tprogress = start_delayed_progress(_(\"Generating patches\"), total);\n> -\twhile (0 <= --nr) {\n> +\tfor (i = 0; i < nr; i++) {\n> +\t\tsize_t idx = nr - i - 1;\n>  \t\tint shown;\n> -\t\tdisplay_progress(progress, total - nr);\n> -\t\tcommit = list[nr];\n> -\t\trev.nr = total - nr + (start_number - 1);\n> +\n> +\t\tdisplay_progress(progress, total - idx);\n> +\t\tcommit = list[idx];\n> +\t\trev.nr = total - idx + (start_number - 1);\n\nAlong with updating array indices variables to use `size_t`, the loop\nstructure here is also changed. Instead of iterating backwards from\n`nr`, the loop iterator increases and each iteration computes the index\nstarting from the end. This is functionally the same behavior and it\nlooks like it was done to improve readability.\n\n> +\n>  \t\t/* Make the second and subsequent mails replies to the first */\n>  \t\tif (cfg.thread) {\n>  \t\t\t/* Have we already had a message ID? */\n> \n> -- \n> 2.48.0.rc0.184.g0fc57dec57.dirty\n> \n> \n"},{"id":"509818","messageId":"s7b4o6elbbxrzp55mbgtj5tmfdtmdi4kczxad5ogy7quotrfhj@yq7ppxdjp4kd","threadId":"62697","inReplyTo":"20241227-b4-pks-commit-reach-sign-compare-v1-9-07c59c2aa632@pks.im","subject":"Re: [PATCH 9/9] commit-reach: use `size_t` to track indices when computing merge bases","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2025-01-03T02:08:15Z","receivedAt":"2025-01-03T02:10:44Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 24/12/27 11:46AM, Patrick Steinhardt wrote:\n> The functions `repo_get_merge_bases_many()` and friends accepts an array\n> of commits as well as a parameter that indicates how large that array\n> is. This parameter is using a signed integer, which leads to a couple of\n> warnings with -Wsign-compare.\n> \n> Refactor the code to use `size_t` to track indices instead and adapt\n> callers accordingly. While most callers are trivial, there are two\n> callers that require a bit more scrutiny:\n> \n>   - builtin/merge-base.c:show_merge_base() subtracts `1` from the\n>     `rev_nr` before calling `repo_get_merge_bases_many_dirty()`, so if\n>     the variable was `0` it would wrap. This code is fine though because\n>     its only caller will execute that code only when `argc >= 2`, and it\n>     follows that `rev_nr >= 2`, as well.\n> \n>   - bisect.ccheck_merge_bases() similarly subtracts `1` from `rev_nr`.\n\ns/ccheck/check/\n\nSmall typo, but probably not worth rerolling.\n\n>     Again, there is only a single caller that populates `rev_nr` with\n>     `good_revs.nr`. And because a bisection always requires at least one\n>     good revision it follws that `rev_nr >= 1`.\n> \n> Mark the file as -Wsign-compare-clean.\n> \n> Signed-off-by: Patrick Steinhardt <ps@pks.im>\n\nThanks Patrick! I reviewed the series and overall it looks good to me :)\n\n-Justin\n"},{"id":"509821","messageId":"Z3eG_ZPPMOa4KO3J@pks.im","threadId":"62697","inReplyTo":"s7b4o6elbbxrzp55mbgtj5tmfdtmdi4kczxad5ogy7quotrfhj@yq7ppxdjp4kd","subject":"Re: [PATCH 9/9] commit-reach: use `size_t` to track indices when computing merge bases","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T06:43:09Z","receivedAt":"2025-01-03T06:43:13Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 02, 2025 at 08:08:15PM -0600, Justin Tobler wrote:\n> On 24/12/27 11:46AM, Patrick Steinhardt wrote:\n> > The functions `repo_get_merge_bases_many()` and friends accepts an array\n> > of commits as well as a parameter that indicates how large that array\n> > is. This parameter is using a signed integer, which leads to a couple of\n> > warnings with -Wsign-compare.\n> > \n> > Refactor the code to use `size_t` to track indices instead and adapt\n> > callers accordingly. While most callers are trivial, there are two\n> > callers that require a bit more scrutiny:\n> > \n> >   - builtin/merge-base.c:show_merge_base() subtracts `1` from the\n> >     `rev_nr` before calling `repo_get_merge_bases_many_dirty()`, so if\n> >     the variable was `0` it would wrap. This code is fine though because\n> >     its only caller will execute that code only when `argc >= 2`, and it\n> >     follows that `rev_nr >= 2`, as well.\n> > \n> >   - bisect.ccheck_merge_bases() similarly subtracts `1` from `rev_nr`.\n> \n> s/ccheck/check/\n> \n> Small typo, but probably not worth rerolling.\n> \n> >     Again, there is only a single caller that populates `rev_nr` with\n> >     `good_revs.nr`. And because a bisection always requires at least one\n> >     good revision it follws that `rev_nr >= 1`.\n> > \n> > Mark the file as -Wsign-compare-clean.\n> > \n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> \n> Thanks Patrick! I reviewed the series and overall it looks good to me :)\n\nThanks for your review!\n\nPatrick\n"},{"id":"509822","messageId":"Z3eHAWnkrwyGqtki@pks.im","threadId":"62697","inReplyTo":"3b7ep2hex2vae56p2ba2kjeextjrjcsu5jrufr7hqewrnv3z45@ftrrcuvihxxs","subject":"Re: [PATCH 6/9] builtin/log: use `size_t` to track indices","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-01-03T06:43:13Z","receivedAt":"2025-01-03T06:43:16Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Thu, Jan 02, 2025 at 07:58:24PM -0600, Justin Tobler wrote:\n> On 24/12/27 11:46AM, Patrick Steinhardt wrote:\n> > Similar as with the preceding commit, adapt \"builtin/log.c\" so that it\n> > tracks array indices via `size_t` instead of using signed integers. This\n> > fixes a couple of -Wsign-compare warnings and prepares the code for\n> > a similar refactoring of `repo_get_merge_bases_many()` in a subsequent\n> > commit.\n> > \n> > Signed-off-by: Patrick Steinhardt <ps@pks.im>\n> > ---\n> >  builtin/log.c | 23 +++++++++++++----------\n> >  1 file changed, 13 insertions(+), 10 deletions(-)\n> > \n> [snip]\n> >  \tif (show_progress)\n> >  \t\tprogress = start_delayed_progress(_(\"Generating patches\"), total);\n> > -\twhile (0 <= --nr) {\n> > +\tfor (i = 0; i < nr; i++) {\n> > +\t\tsize_t idx = nr - i - 1;\n> >  \t\tint shown;\n> > -\t\tdisplay_progress(progress, total - nr);\n> > -\t\tcommit = list[nr];\n> > -\t\trev.nr = total - nr + (start_number - 1);\n> > +\n> > +\t\tdisplay_progress(progress, total - idx);\n> > +\t\tcommit = list[idx];\n> > +\t\trev.nr = total - idx + (start_number - 1);\n> \n> Along with updating array indices variables to use `size_t`, the loop\n> structure here is also changed. Instead of iterating backwards from\n> `nr`, the loop iterator increases and each iteration computes the index\n> starting from the end. This is functionally the same behavior and it\n> looks like it was done to improve readability.\n\nIt wasn't really done to improve readability, but to retain correctness.\nBefore this change we relied on `nr` being signed, and thus it could\nalso have a negative value. Now that it's a `size_t` it would overflow\neventually and that would make the loop condition a tautology. So I had\nto refactor the condition so that it doesn't rely on such an overflow\nanymore.\n\nPatrick\n"}]}