[PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Patrick Steinhardt]

Hi,

this patch series is a follow-up for [1], fixing two smallish issues
introduced in that patch. Naturally I couldn't stop there and decided to
also make "commit-reach.c" and two other files -Wsign-compare-clean.

Thanks!

Patrick

[1]: <20241220084949.GA132704@coredump.intra.peff.net>

---
Patrick Steinhardt (9):
      prio-queue: fix type of `insertion_ctr`
      commit-reach: fix index used to loop through unsigned integer
      commit-reach: fix type of `min_commit_date`
      commit-reach: use `size_t` to track indices in `remove_redundant()`
      commit-reach: use `size_t` to track indices in `get_reachable_subset()`
      builtin/log: use `size_t` to track indices
      builtin/log: fix remaining -Wsign-compare warnings
      shallow: fix -Wsign-compare warnings
      commit-reach: use `size_t` to track indices when computing merge bases

 bisect.c              | 11 ++++----
 builtin/log.c         | 50 +++++++++++++++++-----------------
 builtin/merge-base.c  |  4 +--
 commit-reach.c        | 75 +++++++++++++++++++++++++++------------------------
 commit-reach.h        | 10 +++----
 commit.c              |  4 +--
 commit.h              |  2 +-
 prio-queue.h          |  4 +--
 ref-filter.c          |  2 +-
 remote.c              |  4 +--
 shallow.c             | 38 +++++++++++++-------------
 shallow.h             |  6 ++---
 t/helper/test-reach.c |  6 ++---
 13 files changed, 111 insertions(+), 105 deletions(-)


---
base-commit: 76cf4f61c87855ebf0784b88aaf737d6b09f504b
change-id: 20241227-b4-pks-commit-reach-sign-compare-235a03fffc78

[PATCH 1/9] prio-queue: fix type of `insertion_ctr`

[Patrick Steinhardt]

In 62e745ced2 (prio-queue: use size_t rather than int for size,
2024-12-20), we have converted `struct prio_queue` to use `size_t` to
track the number of entries in the queue as well as the allocated size
of the underlying array. There is one more counter though, namely the
insertion counter, that is still using an `unsigned` instead of a
`size_t`. This is unlikely to ever be a problem, but it makes one wonder
why some indices use `size_t` while others use `unsigned`. Furthermore,
the mentioned commit stated the intent to also adapt these variables,
but seemingly forgot to do so.

Fix the issue by converting those counters to use `size_t`, as well.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 prio-queue.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/prio-queue.h b/prio-queue.h
index 36f370625f0802cb84082fea904ad6e8a456520a..38d032636d4cf9c544811cff6c3e6a080d6c7b82 100644
--- a/prio-queue.h
+++ b/prio-queue.h
@@ -22,13 +22,13 @@
 typedef int (*prio_queue_compare_fn)(const void *one, const void *two, void *cb_data);
 
 struct prio_queue_entry {
-	unsigned ctr;
+	size_t ctr;
 	void *data;
 };
 
 struct prio_queue {
 	prio_queue_compare_fn compare;
-	unsigned insertion_ctr;
+	size_t insertion_ctr;
 	void *cb_data;
 	size_t alloc, nr;
 	struct prio_queue_entry *array;

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

Re: [PATCH 1/9] prio-queue: fix type of `insertion_ctr`

[Jeff King]

On Fri, Dec 27, 2024 at 11:46:21AM +0100, Patrick Steinhardt wrote:

> In 62e745ced2 (prio-queue: use size_t rather than int for size,
> 2024-12-20), we have converted `struct prio_queue` to use `size_t` to
> track the number of entries in the queue as well as the allocated size
> of the underlying array. There is one more counter though, namely the
> insertion counter, that is still using an `unsigned` instead of a
> `size_t`. This is unlikely to ever be a problem, but it makes one wonder
> why some indices use `size_t` while others use `unsigned`. Furthermore,
> the mentioned commit stated the intent to also adapt these variables,
> but seemingly forgot to do so.
> 
> Fix the issue by converting those counters to use `size_t`, as well.

Yep, this looks good, and was what I meant to send in 62e745ced2.

-Peff

[PATCH 2/9] commit-reach: fix index used to loop through unsigned integer

[Patrick Steinhardt]

In 62e745ced2 (prio-queue: use size_t rather than int for size,
2024-12-20), we refactored `struct prio_queue` to track the number of
contained entries via a `size_t`. While the refactoring adapted one of
the users of that variable, it forgot to also adapt "commit-reach.c"
accordingly. This was missed because that file has -Wsign-conversion
disabled.

Fix the issue by using a `size_t` to iterate through entries.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 commit-reach.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/commit-reach.c b/commit-reach.c
index e3edd1199529792fafbaa03999c5b7b202f7cf1b..e65872617003d0e43776909c30343f091d6b42f2 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -42,8 +42,7 @@ static int compare_commits_by_gen(const void *_a, const void *_b)
 
 static int queue_has_nonstale(struct prio_queue *queue)
 {
-	int i;
-	for (i = 0; i < queue->nr; i++) {
+	for (size_t i = 0; i < queue->nr; i++) {
 		struct commit *commit = queue->array[i].data;
 		if (!(commit->object.flags & STALE))
 			return 1;

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

Re: [PATCH 2/9] commit-reach: fix index used to loop through unsigned integer

[Jeff King]

On Fri, Dec 27, 2024 at 11:46:22AM +0100, Patrick Steinhardt wrote:

> In 62e745ced2 (prio-queue: use size_t rather than int for size,
> 2024-12-20), we refactored `struct prio_queue` to track the number of
> contained entries via a `size_t`. While the refactoring adapted one of
> the users of that variable, it forgot to also adapt "commit-reach.c"
> accordingly. This was missed because that file has -Wsign-conversion
> disabled.

Yes, that's exactly what happened (I used a merge of my series and your
-Wsign-compare one and relied on the compiler).

I grepped through for other instances and couldn't find any problems.
Many places just do:

  while (queue->nr)
     ...remove top...

which is fine with either type (which might explain why the problem
wasn't more common).

I thought there was one more instance in commit-graph.c, which looks at
"info->commits->nr", and I was grepping for "commits->nr" since
rev_info->commits is a prio_queue. But it is actually a totally
different type, compute_generation_info, and "commits" there is a
packed_commit_list.

That data type _also_ has the same int/size_t problem. ;) But it is
totally separate, so it can happen later in its own patch (and there are
a ton of other -Wsign-compare warnings in that file).

Thanks for cleaning up after me.

-Peff

[PATCH 3/9] commit-reach: fix type of `min_commit_date`

[Patrick Steinhardt]

The `can_all_from_reach_with_flag()` function accepts a parameter that
allows callers to cut off traversal at a specified commit date. This
parameter is of type `time_t`, which is a signed type, while we end up
comparing it to a commit's `date` field, which is of the unsigned type
`timestamp_t`.

Fix the parameter to be of type `timestamp_t`. There is only a single
caller in "upload-pack.c" that sets this parameter, and that caller
knows to pass in a `timestamp_t` already.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 commit-reach.c | 4 ++--
 commit-reach.h | 2 +-
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/commit-reach.c b/commit-reach.c
index e65872617003d0e43776909c30343f091d6b42f2..9f8b2457bcc12bebf725a5276d1aec467bb7af05 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -780,7 +780,7 @@ int commit_contains(struct ref_filter *filter, struct commit *commit,
 int can_all_from_reach_with_flag(struct object_array *from,
 				 unsigned int with_flag,
 				 unsigned int assign_flag,
-				 time_t min_commit_date,
+				 timestamp_t min_commit_date,
 				 timestamp_t min_generation)
 {
 	struct commit **list = NULL;
@@ -883,9 +883,9 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,
 		       int cutoff_by_min_date)
 {
 	struct object_array from_objs = OBJECT_ARRAY_INIT;
-	time_t min_commit_date = cutoff_by_min_date ? from->item->date : 0;
 	struct commit_list *from_iter = from, *to_iter = to;
 	int result;
+	timestamp_t min_commit_date = cutoff_by_min_date ? from->item->date : 0;
 	timestamp_t min_generation = GENERATION_NUMBER_INFINITY;
 
 	while (from_iter) {
diff --git a/commit-reach.h b/commit-reach.h
index 9a745b7e1766850a77fbe52c3eeae290b68038d0..d5f3347376b6310727c74b81cb7660485b96c0bc 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -81,7 +81,7 @@ int commit_contains(struct ref_filter *filter, struct commit *commit,
 int can_all_from_reach_with_flag(struct object_array *from,
 				 unsigned int with_flag,
 				 unsigned int assign_flag,
-				 time_t min_commit_date,
+				 timestamp_t min_commit_date,
 				 timestamp_t min_generation);
 int can_all_from_reach(struct commit_list *from, struct commit_list *to,
 		       int commit_date_cutoff);

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

[PATCH 4/9] commit-reach: use `size_t` to track indices in `remove_redundant()`

[Patrick Steinhardt]

The function `remove_redundant()` gets as input an array of commits as
well as the size of that array and then drops redundant commits from
that array. It then returns either `-1` in case an error occurred, or
the new number of items in the array.

The function receives and returns these sizes with a signed integer,
which causes several warnings with -Wsign-compare. Fix this issue by
consistently using `size_t` to track array indices and splitting up
the returned value into a returned error code and a separate out pointer
for the new computed size.

Note that `get_merge_bases_many()` and related functions still track
array sizes as a signed integer. This will be fixed in a subsequent
commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 commit-reach.c | 53 ++++++++++++++++++++++++++++++-----------------------
 1 file changed, 30 insertions(+), 23 deletions(-)

diff --git a/commit-reach.c b/commit-reach.c
index 9f8b2457bcc12bebf725a5276d1aec467bb7af05..d7f6f1be75e95cc834d60be719e930a77ad0518f 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -212,12 +212,13 @@ int get_octopus_merge_bases(struct commit_list *in, struct commit_list **result)
 }
 
 static int remove_redundant_no_gen(struct repository *r,
-				   struct commit **array, int cnt)
+				   struct commit **array,
+				   size_t cnt, size_t *dedup_cnt)
 {
 	struct commit **work;
 	unsigned char *redundant;
-	int *filled_index;
-	int i, j, filled;
+	size_t *filled_index;
+	size_t i, j, filled;
 
 	CALLOC_ARRAY(work, cnt);
 	redundant = xcalloc(cnt, 1);
@@ -267,20 +268,22 @@ static int remove_redundant_no_gen(struct repository *r,
 	for (i = filled = 0; i < cnt; i++)
 		if (!redundant[i])
 			array[filled++] = work[i];
+	*dedup_cnt = filled;
 	free(work);
 	free(redundant);
 	free(filled_index);
-	return filled;
+	return 0;
 }
 
 static int remove_redundant_with_gen(struct repository *r,
-				     struct commit **array, int cnt)
+				     struct commit **array, size_t cnt,
+				     size_t *dedup_cnt)
 {
-	int i, count_non_stale = 0, count_still_independent = cnt;
+	size_t i, count_non_stale = 0, count_still_independent = cnt;
 	timestamp_t min_generation = GENERATION_NUMBER_INFINITY;
 	struct commit **walk_start, **sorted;
 	size_t walk_start_nr = 0, walk_start_alloc = cnt;
-	int min_gen_pos = 0;
+	size_t min_gen_pos = 0;
 
 	/*
 	 * Sort the input by generation number, ascending. This allows
@@ -326,12 +329,12 @@ static int remove_redundant_with_gen(struct repository *r,
 	 * terminate early. Otherwise, we will do the same amount of work
 	 * as before.
 	 */
-	for (i = walk_start_nr - 1; i >= 0 && count_still_independent > 1; i--) {
+	for (i = walk_start_nr; i && count_still_independent > 1; i--) {
 		/* push the STALE bits up to min generation */
 		struct commit_list *stack = NULL;
 
-		commit_list_insert(walk_start[i], &stack);
-		walk_start[i]->object.flags |= STALE;
+		commit_list_insert(walk_start[i - 1], &stack);
+		walk_start[i - 1]->object.flags |= STALE;
 
 		while (stack) {
 			struct commit_list *parents;
@@ -388,10 +391,12 @@ static int remove_redundant_with_gen(struct repository *r,
 	clear_commit_marks_many(walk_start_nr, walk_start, STALE);
 	free(walk_start);
 
-	return count_non_stale;
+	*dedup_cnt = count_non_stale;
+	return 0;
 }
 
-static int remove_redundant(struct repository *r, struct commit **array, int cnt)
+static int remove_redundant(struct repository *r, struct commit **array,
+			    size_t cnt, size_t *dedup_cnt)
 {
 	/*
 	 * Some commit in the array may be an ancestor of
@@ -401,19 +406,17 @@ static int remove_redundant(struct repository *r, struct commit **array, int cnt
 	 * that number.
 	 */
 	if (generation_numbers_enabled(r)) {
-		int i;
-
 		/*
 		 * If we have a single commit with finite generation
 		 * number, then the _with_gen algorithm is preferred.
 		 */
-		for (i = 0; i < cnt; i++) {
+		for (size_t i = 0; i < cnt; i++) {
 			if (commit_graph_generation(array[i]) < GENERATION_NUMBER_INFINITY)
-				return remove_redundant_with_gen(r, array, cnt);
+				return remove_redundant_with_gen(r, array, cnt, dedup_cnt);
 		}
 	}
 
-	return remove_redundant_no_gen(r, array, cnt);
+	return remove_redundant_no_gen(r, array, cnt, dedup_cnt);
 }
 
 static int get_merge_bases_many_0(struct repository *r,
@@ -425,7 +428,8 @@ static int get_merge_bases_many_0(struct repository *r,
 {
 	struct commit_list *list;
 	struct commit **rslt;
-	int cnt, i;
+	size_t cnt, i;
+	int ret;
 
 	if (merge_bases_many(r, one, n, twos, result) < 0)
 		return -1;
@@ -452,8 +456,8 @@ static int get_merge_bases_many_0(struct repository *r,
 	clear_commit_marks(one, all_flags);
 	clear_commit_marks_many(n, twos, all_flags);
 
-	cnt = remove_redundant(r, rslt, cnt);
-	if (cnt < 0) {
+	ret = remove_redundant(r, rslt, cnt, &cnt);
+	if (ret < 0) {
 		free(rslt);
 		return -1;
 	}
@@ -582,7 +586,8 @@ struct commit_list *reduce_heads(struct commit_list *heads)
 	struct commit_list *p;
 	struct commit_list *result = NULL, **tail = &result;
 	struct commit **array;
-	int num_head, i;
+	size_t num_head, i;
+	int ret;
 
 	if (!heads)
 		return NULL;
@@ -603,11 +608,13 @@ struct commit_list *reduce_heads(struct commit_list *heads)
 			p->item->object.flags &= ~STALE;
 		}
 	}
-	num_head = remove_redundant(the_repository, array, num_head);
-	if (num_head < 0) {
+
+	ret = remove_redundant(the_repository, array, num_head, &num_head);
+	if (ret < 0) {
 		free(array);
 		return NULL;
 	}
+
 	for (i = 0; i < num_head; i++)
 		tail = &commit_list_insert(array[i], tail)->next;
 	free(array);

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

Re: [PATCH 4/9] commit-reach: use `size_t` to track indices in `remove_redundant()`

[Justin Tobler]

On 24/12/27 11:46AM, Patrick Steinhardt wrote:
> The function `remove_redundant()` gets as input an array of commits as
> well as the size of that array and then drops redundant commits from
> that array. It then returns either `-1` in case an error occurred, or
> the new number of items in the array.
> 
> The function receives and returns these sizes with a signed integer,
> which causes several warnings with -Wsign-compare. Fix this issue by
> consistently using `size_t` to track array indices and splitting up
> the returned value into a returned error code and a separate out pointer
> for the new computed size.
> 
> Note that `get_merge_bases_many()` and related functions still track
> array sizes as a signed integer. This will be fixed in a subsequent
> commit.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  commit-reach.c | 53 ++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 30 insertions(+), 23 deletions(-)
> 
> diff --git a/commit-reach.c b/commit-reach.c
> index 9f8b2457bcc12bebf725a5276d1aec467bb7af05..d7f6f1be75e95cc834d60be719e930a77ad0518f 100644
> --- a/commit-reach.c
> +++ b/commit-reach.c
> @@ -212,12 +212,13 @@ int get_octopus_merge_bases(struct commit_list *in, struct commit_list **result)
>  }
>  
>  static int remove_redundant_no_gen(struct repository *r,
> -				   struct commit **array, int cnt)
> +				   struct commit **array,
> +				   size_t cnt, size_t *dedup_cnt)
>  {
>  	struct commit **work;
>  	unsigned char *redundant;
> -	int *filled_index;
> -	int i, j, filled;
> +	size_t *filled_index;
> +	size_t i, j, filled;
>  
>  	CALLOC_ARRAY(work, cnt);
>  	redundant = xcalloc(cnt, 1);
> @@ -267,20 +268,22 @@ static int remove_redundant_no_gen(struct repository *r,
>  	for (i = filled = 0; i < cnt; i++)
>  		if (!redundant[i])
>  			array[filled++] = work[i];
> +	*dedup_cnt = filled;
>  	free(work);
>  	free(redundant);
>  	free(filled_index);
> -	return filled;
> +	return 0;

Previously the return value indicated either a potential error if its
value was negative or the new count otherwise. Since we now want to make
the count a `size_t` to avoid -Wsign-compare warnings, we split the two
concerns and have a separate pointer used to record the count. This
approach is used for each of the `remove_redundant*()` functions. Seems
sensible to me.

-Justin

[PATCH 5/9] commit-reach: use `size_t` to track indices in `get_reachable_subset()`

[Patrick Steinhardt]

Similar as with the preceding commit, adapt `get_reachable_subset()` so
that it tracks array indices via `size_t` instead of using signed
integers to fix a couple of -Wsign-compare warnings. Adapt callers
accordingly.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c       | 9 +++++----
 commit-reach.c | 8 ++++----
 commit-reach.h | 4 ++--
 commit.c       | 4 ++--
 commit.h       | 2 +-
 ref-filter.c   | 2 +-
 remote.c       | 4 ++--
 7 files changed, 17 insertions(+), 16 deletions(-)

diff --git a/bisect.c b/bisect.c
index 1a9069c9ad8a19d6e0ee45415407c70f36885514..7a1afc46e5fc0250212f5b6eaf952cf8e36b56fe 100644
--- a/bisect.c
+++ b/bisect.c
@@ -780,10 +780,10 @@ static struct commit *get_commit_reference(struct repository *r,
 }
 
 static struct commit **get_bad_and_good_commits(struct repository *r,
-						int *rev_nr)
+						size_t *rev_nr)
 {
 	struct commit **rev;
-	int i, n = 0;
+	size_t i, n = 0;
 
 	ALLOC_ARRAY(rev, 1 + good_revs.nr);
 	rev[n++] = get_commit_reference(r, current_bad_oid);
@@ -887,7 +887,7 @@ static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int
 	return res;
 }
 
-static int check_ancestors(struct repository *r, int rev_nr,
+static int check_ancestors(struct repository *r, size_t rev_nr,
 			   struct commit **rev, const char *prefix)
 {
 	struct strvec rev_argv = STRVEC_INIT;
@@ -922,7 +922,8 @@ static enum bisect_error check_good_are_ancestors_of_bad(struct repository *r,
 {
 	char *filename;
 	struct stat st;
-	int fd, rev_nr;
+	int fd;
+	size_t rev_nr;
 	enum bisect_error res = BISECT_OK;
 	struct commit **rev;
 
diff --git a/commit-reach.c b/commit-reach.c
index d7f6f1be75e95cc834d60be719e930a77ad0518f..bab40f557580476d59d3a0b0ef56f40263e6615e 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -791,8 +791,8 @@ int can_all_from_reach_with_flag(struct object_array *from,
 				 timestamp_t min_generation)
 {
 	struct commit **list = NULL;
-	int i;
-	int nr_commits;
+	size_t i;
+	size_t nr_commits;
 	int result = 1;
 
 	ALLOC_ARRAY(list, from->nr);
@@ -944,8 +944,8 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,
 	return result;
 }
 
-struct commit_list *get_reachable_subset(struct commit **from, int nr_from,
-					 struct commit **to, int nr_to,
+struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from,
+					 struct commit **to, size_t nr_to,
 					 unsigned int reachable_flag)
 {
 	struct commit **item;
diff --git a/commit-reach.h b/commit-reach.h
index d5f3347376b6310727c74b81cb7660485b96c0bc..fa5408054ac01372c041d18595a405cfdaec6af3 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -95,8 +95,8 @@ int can_all_from_reach(struct commit_list *from, struct commit_list *to,
  * This method uses the PARENT1 and PARENT2 flags during its operation,
  * so be sure these flags are not set before calling the method.
  */
-struct commit_list *get_reachable_subset(struct commit **from, int nr_from,
-					 struct commit **to, int nr_to,
+struct commit_list *get_reachable_subset(struct commit **from, size_t nr_from,
+					 struct commit **to, size_t nr_to,
 					 unsigned int reachable_flag);
 
 struct ahead_behind_count {
diff --git a/commit.c b/commit.c
index a127fe60c5e83c3968fc0538f0e801e4afd52acd..540660359d4190f7dbc1aaa7668b033a6d16afe9 100644
--- a/commit.c
+++ b/commit.c
@@ -778,11 +778,11 @@ static void clear_commit_marks_1(struct commit_list **plist,
 	}
 }
 
-void clear_commit_marks_many(int nr, struct commit **commit, unsigned int mark)
+void clear_commit_marks_many(size_t nr, struct commit **commit, unsigned int mark)
 {
 	struct commit_list *list = NULL;
 
-	while (nr--) {
+	for (size_t i = 0; i < nr; i++) {
 		clear_commit_marks_1(&list, *commit, mark);
 		commit++;
 	}
diff --git a/commit.h b/commit.h
index 943e3d74b2ae026d77e3bac59a4e993314e79ce4..70c870dae4d4b972a1f608983fc17024ad04d2e9 100644
--- a/commit.h
+++ b/commit.h
@@ -210,7 +210,7 @@ struct commit *pop_most_recent_commit(struct commit_list **list,
 struct commit *pop_commit(struct commit_list **stack);
 
 void clear_commit_marks(struct commit *commit, unsigned int mark);
-void clear_commit_marks_many(int nr, struct commit **commit, unsigned int mark);
+void clear_commit_marks_many(size_t nr, struct commit **commit, unsigned int mark);
 
 
 enum rev_sort_order {
diff --git a/ref-filter.c b/ref-filter.c
index 23054694c2c96014ef411a225c6c941eb8d2d56c..bf5534605e234d7cdd01d52f5c83f5314745e13d 100644
--- a/ref-filter.c
+++ b/ref-filter.c
@@ -3041,7 +3041,7 @@ static void reach_filter(struct ref_array *array,
 			 struct commit_list **check_reachable,
 			 int include_reached)
 {
-	int i, old_nr;
+	size_t i, old_nr;
 	struct commit **to_clear;
 
 	if (!*check_reachable)
diff --git a/remote.c b/remote.c
index 18e5ccf391844516e2fcc54fc0d6835283ff6a48..0f6fba85625b523122e50e28a0f64b6e143cd9fb 100644
--- a/remote.c
+++ b/remote.c
@@ -1535,7 +1535,7 @@ static struct ref **tail_ref(struct ref **head)
 
 struct tips {
 	struct commit **tip;
-	int nr, alloc;
+	size_t nr, alloc;
 };
 
 static void add_to_tips(struct tips *tips, const struct object_id *oid)
@@ -1602,7 +1602,7 @@ static void add_missing_tags(struct ref *src, struct ref **dst, struct ref ***ds
 		const int reachable_flag = 1;
 		struct commit_list *found_commits;
 		struct commit **src_commits;
-		int nr_src_commits = 0, alloc_src_commits = 16;
+		size_t nr_src_commits = 0, alloc_src_commits = 16;
 		ALLOC_ARRAY(src_commits, alloc_src_commits);
 
 		for_each_string_list_item(item, &src_tag) {

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

[PATCH 6/9] builtin/log: use `size_t` to track indices

[Patrick Steinhardt]

Similar as with the preceding commit, adapt "builtin/log.c" so that it
tracks array indices via `size_t` instead of using signed integers. This
fixes a couple of -Wsign-compare warnings and prepares the code for
a similar refactoring of `repo_get_merge_bases_many()` in a subsequent
commit.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/log.c | 23 +++++++++++++----------
 1 file changed, 13 insertions(+), 10 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 75e1b34123b348f57334d5592879398064de246e..805b2355d964915732edf5928d54fb6d06e394d4 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1746,11 +1746,12 @@ struct base_tree_info {
 
 static struct commit *get_base_commit(const struct format_config *cfg,
 				      struct commit **list,
-				      int total)
+				      size_t total)
 {
 	struct commit *base = NULL;
 	struct commit **rev;
-	int i = 0, rev_nr = 0, auto_select, die_on_failure, ret;
+	int auto_select, die_on_failure, ret;
+	size_t i = 0, rev_nr = 0;
 
 	switch (cfg->auto_base) {
 	case AUTO_BASE_NEVER:
@@ -1885,13 +1886,12 @@ define_commit_slab(commit_base, int);
 static void prepare_bases(struct base_tree_info *bases,
 			  struct commit *base,
 			  struct commit **list,
-			  int total)
+			  size_t total)
 {
 	struct commit *commit;
 	struct rev_info revs;
 	struct diff_options diffopt;
 	struct commit_base commit_base;
-	int i;
 
 	if (!base)
 		return;
@@ -1906,7 +1906,7 @@ static void prepare_bases(struct base_tree_info *bases,
 	repo_init_revisions(the_repository, &revs, NULL);
 	revs.max_parents = 1;
 	revs.topo_order = 1;
-	for (i = 0; i < total; i++) {
+	for (size_t i = 0; i < total; i++) {
 		list[i]->object.flags &= ~UNINTERESTING;
 		add_pending_object(&revs, &list[i]->object, "rev_list");
 		*commit_base_at(&commit_base, list[i]) = 1;
@@ -2007,7 +2007,7 @@ int cmd_format_patch(int argc,
 	struct rev_info rev;
 	char *to_free = NULL;
 	struct setup_revision_opt s_r_opt;
-	int nr = 0, total, i;
+	size_t nr = 0, total, i;
 	int use_stdout = 0;
 	int start_number = -1;
 	int just_numbers = 0;
@@ -2500,11 +2500,14 @@ int cmd_format_patch(int argc,
 
 	if (show_progress)
 		progress = start_delayed_progress(_("Generating patches"), total);
-	while (0 <= --nr) {
+	for (i = 0; i < nr; i++) {
+		size_t idx = nr - i - 1;
 		int shown;
-		display_progress(progress, total - nr);
-		commit = list[nr];
-		rev.nr = total - nr + (start_number - 1);
+
+		display_progress(progress, total - idx);
+		commit = list[idx];
+		rev.nr = total - idx + (start_number - 1);
+
 		/* Make the second and subsequent mails replies to the first */
 		if (cfg.thread) {
 			/* Have we already had a message ID? */

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

Re: [PATCH 6/9] builtin/log: use `size_t` to track indices

[Justin Tobler]

On 24/12/27 11:46AM, Patrick Steinhardt wrote:
> Similar as with the preceding commit, adapt "builtin/log.c" so that it
> tracks array indices via `size_t` instead of using signed integers. This
> fixes a couple of -Wsign-compare warnings and prepares the code for
> a similar refactoring of `repo_get_merge_bases_many()` in a subsequent
> commit.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/log.c | 23 +++++++++++++----------
>  1 file changed, 13 insertions(+), 10 deletions(-)
> 
[snip]
>  	if (show_progress)
>  		progress = start_delayed_progress(_("Generating patches"), total);
> -	while (0 <= --nr) {
> +	for (i = 0; i < nr; i++) {
> +		size_t idx = nr - i - 1;
>  		int shown;
> -		display_progress(progress, total - nr);
> -		commit = list[nr];
> -		rev.nr = total - nr + (start_number - 1);
> +
> +		display_progress(progress, total - idx);
> +		commit = list[idx];
> +		rev.nr = total - idx + (start_number - 1);

Along with updating array indices variables to use `size_t`, the loop
structure here is also changed. Instead of iterating backwards from
`nr`, the loop iterator increases and each iteration computes the index
starting from the end. This is functionally the same behavior and it
looks like it was done to improve readability.

> +
>  		/* Make the second and subsequent mails replies to the first */
>  		if (cfg.thread) {
>  			/* Have we already had a message ID? */
> 
> -- 
> 2.48.0.rc0.184.g0fc57dec57.dirty
> 
> 

Re: [PATCH 6/9] builtin/log: use `size_t` to track indices

[Patrick Steinhardt]

On Thu, Jan 02, 2025 at 07:58:24PM -0600, Justin Tobler wrote:
> On 24/12/27 11:46AM, Patrick Steinhardt wrote:
> > Similar as with the preceding commit, adapt "builtin/log.c" so that it
> > tracks array indices via `size_t` instead of using signed integers. This
> > fixes a couple of -Wsign-compare warnings and prepares the code for
> > a similar refactoring of `repo_get_merge_bases_many()` in a subsequent
> > commit.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> >  builtin/log.c | 23 +++++++++++++----------
> >  1 file changed, 13 insertions(+), 10 deletions(-)
> > 
> [snip]
> >  	if (show_progress)
> >  		progress = start_delayed_progress(_("Generating patches"), total);
> > -	while (0 <= --nr) {
> > +	for (i = 0; i < nr; i++) {
> > +		size_t idx = nr - i - 1;
> >  		int shown;
> > -		display_progress(progress, total - nr);
> > -		commit = list[nr];
> > -		rev.nr = total - nr + (start_number - 1);
> > +
> > +		display_progress(progress, total - idx);
> > +		commit = list[idx];
> > +		rev.nr = total - idx + (start_number - 1);
> 
> Along with updating array indices variables to use `size_t`, the loop
> structure here is also changed. Instead of iterating backwards from
> `nr`, the loop iterator increases and each iteration computes the index
> starting from the end. This is functionally the same behavior and it
> looks like it was done to improve readability.

It wasn't really done to improve readability, but to retain correctness.
Before this change we relied on `nr` being signed, and thus it could
also have a negative value. Now that it's a `size_t` it would overflow
eventually and that would make the loop condition a tautology. So I had
to refactor the condition so that it doesn't rely on such an overflow
anymore.

Patrick

[PATCH 7/9] builtin/log: fix remaining -Wsign-compare warnings

[Patrick Steinhardt]

Fix remaining -Wsign-compare warnings in "builtin/log.c" and mark the
file as -Wsign-compare-clean. While most of the fixes are obvious, one
fix requires us to use `cast_size_t_to_int()`, which will cause us to
die in case the `size_t` cannot be represented as `int`. This should be
fine though, as the data would typically be set either via a config key
or via the command line, neither of which should ever exceed a couple of
kilobytes of data.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 builtin/log.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 805b2355d964915732edf5928d54fb6d06e394d4..a4f41aafcae069541ee987dc94d245edfe9787a8 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -6,7 +6,6 @@
  */
 
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "builtin.h"
 #include "abspath.h"
@@ -209,7 +208,6 @@ static void cmd_log_init_defaults(struct rev_info *rev,
 
 static void set_default_decoration_filter(struct decoration_filter *decoration_filter)
 {
-	int i;
 	char *value = NULL;
 	struct string_list *include = decoration_filter->include_ref_pattern;
 	const struct string_list *config_exclude;
@@ -243,7 +241,7 @@ static void set_default_decoration_filter(struct decoration_filter *decoration_f
 	 * No command-line or config options were given, so
 	 * populate with sensible defaults.
 	 */
-	for (i = 0; i < ARRAY_SIZE(ref_namespace); i++) {
+	for (size_t i = 0; i < ARRAY_SIZE(ref_namespace); i++) {
 		if (!ref_namespace[i].decoration)
 			continue;
 
@@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev)
 	unsigned long size;
 	enum object_type type;
 	char *buf = repo_read_object_file(the_repository, oid, &type, &size);
-	int offset = 0;
+	unsigned long offset = 0;
 
 	if (!buf)
 		return error(_("could not read object %s"), oid_to_hex(oid));
 
 	assert(type == OBJ_TAG);
 	while (offset < size && buf[offset] != '\n') {
-		int new_offset = offset + 1;
+		unsigned long new_offset = offset + 1;
 		const char *ident;
 		while (new_offset < size && buf[new_offset++] != '\n')
 			; /* do nothing */
@@ -1316,24 +1314,25 @@ static void print_signature(const char *signature, FILE *file)
 
 static char *find_branch_name(struct rev_info *rev)
 {
-	int i, positive = -1;
 	struct object_id branch_oid;
 	const struct object_id *tip_oid;
 	const char *ref, *v;
 	char *full_ref, *branch = NULL;
+	int interesting_found = 0;
+	size_t idx;
 
-	for (i = 0; i < rev->cmdline.nr; i++) {
+	for (size_t i = 0; i < rev->cmdline.nr; i++) {
 		if (rev->cmdline.rev[i].flags & UNINTERESTING)
 			continue;
-		if (positive < 0)
-			positive = i;
-		else
+		if (interesting_found)
 			return NULL;
+		interesting_found = 1;
+		idx = i;
 	}
-	if (positive < 0)
+	if (!interesting_found)
 		return NULL;
-	ref = rev->cmdline.rev[positive].name;
-	tip_oid = &rev->cmdline.rev[positive].item->oid;
+	ref = rev->cmdline.rev[idx].name;
+	tip_oid = &rev->cmdline.rev[idx].item->oid;
 	if (repo_dwim_ref(the_repository, ref, strlen(ref), &branch_oid,
 			  &full_ref, 0) &&
 	    skip_prefix(full_ref, "refs/heads/", &v) &&
@@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc,
 		fmt_patch_suffix = cfg.fmt_patch_suffix;
 
 	/* Make sure "0000-$sub.patch" gives non-negative length for $sub */
-	if (cfg.log.fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix))
+	if (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen("0000-") + strlen(fmt_patch_suffix)))
 		cfg.log.fmt_patch_name_max = strlen("0000-") + strlen(fmt_patch_suffix);
 
 	if (cover_from_description_arg)

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

Re: [PATCH 7/9] builtin/log: fix remaining -Wsign-compare warnings

[shejialuo]

On Fri, Dec 27, 2024 at 11:46:27AM +0100, Patrick Steinhardt wrote:

[snip]

> @@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev)
>  	unsigned long size;
>  	enum object_type type;
>  	char *buf = repo_read_object_file(the_repository, oid, &type, &size);
> -	int offset = 0;
> +	unsigned long offset = 0;

Why here we use `unsigned long`, is this a special situation where we
cannot use `size_t`?

>  
>  	if (!buf)
>  		return error(_("could not read object %s"), oid_to_hex(oid));
>  
>  	assert(type == OBJ_TAG);
>  	while (offset < size && buf[offset] != '\n') {
> -		int new_offset = offset + 1;
> +		unsigned long new_offset = offset + 1;
>  		const char *ident;
>  		while (new_offset < size && buf[new_offset++] != '\n')
>  			; /* do nothing */

> @@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc,
>  		fmt_patch_suffix = cfg.fmt_patch_suffix;
>  
>  	/* Make sure "0000-$sub.patch" gives non-negative length for $sub */
> -	if (cfg.log.fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix))
> +	if (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen("0000-") + strlen(fmt_patch_suffix)))

A design question, why we don't change the type of
`cfg.log.fmt_patch_name_max` to be `size_t`?

>  		cfg.log.fmt_patch_name_max = strlen("0000-") + strlen(fmt_patch_suffix);
>  
>  	if (cover_from_description_arg)
> 
> -- 
> 2.48.0.rc0.184.g0fc57dec57.dirty
> 

Thanks,
Jialuo

Re: [PATCH 7/9] builtin/log: fix remaining -Wsign-compare warnings

[Patrick Steinhardt]

On Fri, Dec 27, 2024 at 09:21:56PM +0800, shejialuo wrote:
> On Fri, Dec 27, 2024 at 11:46:27AM +0100, Patrick Steinhardt wrote:
> > @@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev)
> >  	unsigned long size;
> >  	enum object_type type;
> >  	char *buf = repo_read_object_file(the_repository, oid, &type, &size);
> > -	int offset = 0;
> > +	unsigned long offset = 0;
> 
> Why here we use `unsigned long`, is this a special situation where we
> cannot use `size_t`?

Mostly because other variables already use `unsigned long` here,
including `repo_read_object_file()`. So given that our object layer
doesn't support `size_t` it wouldn't make sense to use it for the
offset, either.

> >  
> >  	if (!buf)
> >  		return error(_("could not read object %s"), oid_to_hex(oid));
> >  
> >  	assert(type == OBJ_TAG);
> >  	while (offset < size && buf[offset] != '\n') {
> > -		int new_offset = offset + 1;
> > +		unsigned long new_offset = offset + 1;
> >  		const char *ident;
> >  		while (new_offset < size && buf[new_offset++] != '\n')
> >  			; /* do nothing */
> 
> > @@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc,
> >  		fmt_patch_suffix = cfg.fmt_patch_suffix;
> >  
> >  	/* Make sure "0000-$sub.patch" gives non-negative length for $sub */
> > -	if (cfg.log.fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix))
> > +	if (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen("0000-") + strlen(fmt_patch_suffix)))
> 
> A design question, why we don't change the type of
> `cfg.log.fmt_patch_name_max` to be `size_t`?

The whole infra around this uses `int`s, too, so changing this variable
here alone wouldn't suffice. We'd also have to adapt option handling,
config handling, `struct rev_info::patch_name_max` and other bits and
pieces. So in the end the size of required changes would likely balloon
and thus I decided to not go down this rabbit hole.

Patrick

Re: [PATCH 7/9] builtin/log: fix remaining -Wsign-compare warnings

[shejialuo]

On Fri, Dec 27, 2024 at 02:57:18PM +0100, Patrick Steinhardt wrote:
> On Fri, Dec 27, 2024 at 09:21:56PM +0800, shejialuo wrote:
> > On Fri, Dec 27, 2024 at 11:46:27AM +0100, Patrick Steinhardt wrote:
> > > @@ -717,14 +715,14 @@ static int show_tag_object(const struct object_id *oid, struct rev_info *rev)
> > >  	unsigned long size;
> > >  	enum object_type type;
> > >  	char *buf = repo_read_object_file(the_repository, oid, &type, &size);
> > > -	int offset = 0;
> > > +	unsigned long offset = 0;
> > 
> > Why here we use `unsigned long`, is this a special situation where we
> > cannot use `size_t`?
> 
> Mostly because other variables already use `unsigned long` here,
> including `repo_read_object_file()`. So given that our object layer
> doesn't support `size_t` it wouldn't make sense to use it for the
> offset, either.
> 

Make sense. Thanks for the explanation.

> > >  
> > >  	if (!buf)
> > >  		return error(_("could not read object %s"), oid_to_hex(oid));
> > >  
> > >  	assert(type == OBJ_TAG);
> > >  	while (offset < size && buf[offset] != '\n') {
> > > -		int new_offset = offset + 1;
> > > +		unsigned long new_offset = offset + 1;
> > >  		const char *ident;
> > >  		while (new_offset < size && buf[new_offset++] != '\n')
> > >  			; /* do nothing */
> > 
> > > @@ -2183,7 +2182,7 @@ int cmd_format_patch(int argc,
> > >  		fmt_patch_suffix = cfg.fmt_patch_suffix;
> > >  
> > >  	/* Make sure "0000-$sub.patch" gives non-negative length for $sub */
> > > -	if (cfg.log.fmt_patch_name_max <= strlen("0000-") + strlen(fmt_patch_suffix))
> > > +	if (cfg.log.fmt_patch_name_max <= cast_size_t_to_int(strlen("0000-") + strlen(fmt_patch_suffix)))
> > 
> > A design question, why we don't change the type of
> > `cfg.log.fmt_patch_name_max` to be `size_t`?
> 
> The whole infra around this uses `int`s, too, so changing this variable
> here alone wouldn't suffice. We'd also have to adapt option handling,
> config handling, `struct rev_info::patch_name_max` and other bits and
> pieces. So in the end the size of required changes would likely balloon
> and thus I decided to not go down this rabbit hole.
> 

Yes, I agree. If we do this, there is a lot of efforts we need to deal
with.

> Patrick

[PATCH 8/9] shallow: fix -Wsign-compare warnings

[Patrick Steinhardt]

Fix a couple of -Wsign-compare issues in "shallow.c" and mark the file
as -Wsign-compare-clean. This change prepares the code for a refactoring
of `repo_in_merge_bases_many()`, which will be adapted to accept the
number of commits as `size_t` instead of `int`.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 shallow.c | 38 ++++++++++++++++++--------------------
 shallow.h |  6 +++---
 2 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/shallow.c b/shallow.c
index 82a8da3d730b2f4f6bda462c8ac347f0b10da993..b8fcfbef0f9cdbfa895d4d6e1214f67942f88d25 100644
--- a/shallow.c
+++ b/shallow.c
@@ -1,5 +1,4 @@
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "git-compat-util.h"
 #include "hex.h"
@@ -134,7 +133,8 @@ static void free_depth_in_slab(int **ptr)
 struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 		int shallow_flag, int not_shallow_flag)
 {
-	int i = 0, cur_depth = 0;
+	size_t i = 0;
+	int cur_depth = 0;
 	struct commit_list *result = NULL;
 	struct object_array stack = OBJECT_ARRAY_INIT;
 	struct commit *commit = NULL;
@@ -335,16 +335,16 @@ static int write_shallow_commits_1(struct strbuf *out, int use_pack_protocol,
 				   const struct oid_array *extra,
 				   unsigned flags)
 {
-	struct write_shallow_data data;
-	int i;
-	data.out = out;
-	data.use_pack_protocol = use_pack_protocol;
-	data.count = 0;
-	data.flags = flags;
+	struct write_shallow_data data = {
+		.out = out,
+		.use_pack_protocol = use_pack_protocol,
+		.flags = flags,
+	};
+
 	for_each_commit_graft(write_one_shallow, &data);
 	if (!extra)
 		return data.count;
-	for (i = 0; i < extra->nr; i++) {
+	for (size_t i = 0; i < extra->nr; i++) {
 		strbuf_addstr(out, oid_to_hex(extra->oid + i));
 		strbuf_addch(out, '\n');
 		data.count++;
@@ -466,7 +466,6 @@ struct trace_key trace_shallow = TRACE_KEY_INIT(SHALLOW);
  */
 void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)
 {
-	int i;
 	trace_printf_key(&trace_shallow, "shallow: prepare_shallow_info\n");
 	memset(info, 0, sizeof(*info));
 	info->shallow = sa;
@@ -474,7 +473,7 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)
 		return;
 	ALLOC_ARRAY(info->ours, sa->nr);
 	ALLOC_ARRAY(info->theirs, sa->nr);
-	for (i = 0; i < sa->nr; i++) {
+	for (size_t i = 0; i < sa->nr; i++) {
 		if (repo_has_object_file(the_repository, sa->oid + i)) {
 			struct commit_graft *graft;
 			graft = lookup_commit_graft(the_repository,
@@ -507,7 +506,7 @@ void clear_shallow_info(struct shallow_info *info)
 void remove_nonexistent_theirs_shallow(struct shallow_info *info)
 {
 	struct object_id *oid = info->shallow->oid;
-	int i, dst;
+	size_t i, dst;
 	trace_printf_key(&trace_shallow, "shallow: remove_nonexistent_theirs_shallow\n");
 	for (i = dst = 0; i < info->nr_theirs; i++) {
 		if (i != dst)
@@ -560,7 +559,7 @@ static void paint_down(struct paint_info *info, const struct object_id *oid,
 {
 	unsigned int i, nr;
 	struct commit_list *head = NULL;
-	int bitmap_nr = DIV_ROUND_UP(info->nr_bits, 32);
+	size_t bitmap_nr = DIV_ROUND_UP(info->nr_bits, 32);
 	size_t bitmap_size = st_mult(sizeof(uint32_t), bitmap_nr);
 	struct commit *c = lookup_commit_reference_gently(the_repository, oid,
 							  1);
@@ -660,7 +659,7 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,
 	struct object_id *oid = info->shallow->oid;
 	struct oid_array *ref = info->ref;
 	unsigned int i, nr;
-	int *shallow, nr_shallow = 0;
+	size_t *shallow, nr_shallow = 0;
 	struct paint_info pi;
 
 	trace_printf_key(&trace_shallow, "shallow: assign_shallow_commits_to_refs\n");
@@ -735,7 +734,7 @@ void assign_shallow_commits_to_refs(struct shallow_info *info,
 
 struct commit_array {
 	struct commit **commits;
-	int nr, alloc;
+	size_t nr, alloc;
 };
 
 static int add_ref(const char *refname UNUSED,
@@ -753,12 +752,11 @@ static int add_ref(const char *refname UNUSED,
 	return 0;
 }
 
-static void update_refstatus(int *ref_status, int nr, uint32_t *bitmap)
+static void update_refstatus(int *ref_status, size_t nr, uint32_t *bitmap)
 {
-	unsigned int i;
 	if (!ref_status)
 		return;
-	for (i = 0; i < nr; i++)
+	for (size_t i = 0; i < nr; i++)
 		if (bitmap[i / 32] & (1U << (i % 32)))
 			ref_status[i]++;
 }
@@ -773,8 +771,8 @@ static void post_assign_shallow(struct shallow_info *info,
 	struct object_id *oid = info->shallow->oid;
 	struct commit *c;
 	uint32_t **bitmap;
-	int dst, i, j;
-	int bitmap_nr = DIV_ROUND_UP(info->ref->nr, 32);
+	size_t dst, i, j;
+	size_t bitmap_nr = DIV_ROUND_UP(info->ref->nr, 32);
 	struct commit_array ca;
 
 	trace_printf_key(&trace_shallow, "shallow: post_assign_shallow\n");
diff --git a/shallow.h b/shallow.h
index e9ca7e4bc80451a74dc10fb6d4e1a0b190dc4dcc..9bfeade93ead7421d7f2991862dde88ba546fc64 100644
--- a/shallow.h
+++ b/shallow.h
@@ -59,8 +59,8 @@ void prune_shallow(unsigned options);
  */
 struct shallow_info {
 	struct oid_array *shallow;
-	int *ours, nr_ours;
-	int *theirs, nr_theirs;
+	size_t *ours, nr_ours;
+	size_t *theirs, nr_theirs;
 	struct oid_array *ref;
 
 	/* for receive-pack */
@@ -69,7 +69,7 @@ struct shallow_info {
 	int *reachable;
 	int *shallow_ref;
 	struct commit **commits;
-	int nr_commits;
+	size_t nr_commits;
 };
 
 void prepare_shallow_info(struct shallow_info *, struct oid_array *);

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

[PATCH 9/9] commit-reach: use `size_t` to track indices when computing merge bases

[Patrick Steinhardt]

The functions `repo_get_merge_bases_many()` and friends accepts an array
of commits as well as a parameter that indicates how large that array
is. This parameter is using a signed integer, which leads to a couple of
warnings with -Wsign-compare.

Refactor the code to use `size_t` to track indices instead and adapt
callers accordingly. While most callers are trivial, there are two
callers that require a bit more scrutiny:

  - builtin/merge-base.c:show_merge_base() subtracts `1` from the
    `rev_nr` before calling `repo_get_merge_bases_many_dirty()`, so if
    the variable was `0` it would wrap. This code is fine though because
    its only caller will execute that code only when `argc >= 2`, and it
    follows that `rev_nr >= 2`, as well.

  - bisect.ccheck_merge_bases() similarly subtracts `1` from `rev_nr`.
    Again, there is only a single caller that populates `rev_nr` with
    `good_revs.nr`. And because a bisection always requires at least one
    good revision it follws that `rev_nr >= 1`.

Mark the file as -Wsign-compare-clean.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 bisect.c              | 2 +-
 builtin/merge-base.c  | 4 ++--
 commit-reach.c        | 7 +++----
 commit-reach.h        | 4 ++--
 t/helper/test-reach.c | 6 +++---
 5 files changed, 11 insertions(+), 12 deletions(-)

diff --git a/bisect.c b/bisect.c
index 7a1afc46e5fc0250212f5b6eaf952cf8e36b56fe..7a3c77c6d84da0cb6e135b6e0b1ca3596903af5c 100644
--- a/bisect.c
+++ b/bisect.c
@@ -855,7 +855,7 @@ static void handle_skipped_merge_base(const struct object_id *mb)
  * for early success, this will be converted back to 0 in
  * check_good_are_ancestors_of_bad().
  */
-static enum bisect_error check_merge_bases(int rev_nr, struct commit **rev, int no_checkout)
+static enum bisect_error check_merge_bases(size_t rev_nr, struct commit **rev, int no_checkout)
 {
 	enum bisect_error res = BISECT_OK;
 	struct commit_list *result = NULL;
diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index a20c93b11aaa0b9f380b843469da2a3b5db10d00..123c81515e1f5f12045e4073f78b6e8a2b04bb4b 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -8,7 +8,7 @@
 #include "parse-options.h"
 #include "commit-reach.h"
 
-static int show_merge_base(struct commit **rev, int rev_nr, int show_all)
+static int show_merge_base(struct commit **rev, size_t rev_nr, int show_all)
 {
 	struct commit_list *result = NULL, *r;
 
@@ -149,7 +149,7 @@ int cmd_merge_base(int argc,
 		   struct repository *repo UNUSED)
 {
 	struct commit **rev;
-	int rev_nr = 0;
+	size_t rev_nr = 0;
 	int show_all = 0;
 	int cmdmode = 0;
 	int ret;
diff --git a/commit-reach.c b/commit-reach.c
index bab40f557580476d59d3a0b0ef56f40263e6615e..a339e41aa4ed1e375ee6f2f42a163ff1c654c3e4 100644
--- a/commit-reach.c
+++ b/commit-reach.c
@@ -1,5 +1,4 @@
 #define USE_THE_REPOSITORY_VARIABLE
-#define DISABLE_SIGN_COMPARE_WARNINGS
 
 #include "git-compat-util.h"
 #include "commit.h"
@@ -421,7 +420,7 @@ static int remove_redundant(struct repository *r, struct commit **array,
 
 static int get_merge_bases_many_0(struct repository *r,
 				  struct commit *one,
-				  int n,
+				  size_t n,
 				  struct commit **twos,
 				  int cleanup,
 				  struct commit_list **result)
@@ -469,7 +468,7 @@ static int get_merge_bases_many_0(struct repository *r,
 
 int repo_get_merge_bases_many(struct repository *r,
 			      struct commit *one,
-			      int n,
+			      size_t n,
 			      struct commit **twos,
 			      struct commit_list **result)
 {
@@ -478,7 +477,7 @@ int repo_get_merge_bases_many(struct repository *r,
 
 int repo_get_merge_bases_many_dirty(struct repository *r,
 				    struct commit *one,
-				    int n,
+				    size_t n,
 				    struct commit **twos,
 				    struct commit_list **result)
 {
diff --git a/commit-reach.h b/commit-reach.h
index fa5408054ac01372c041d18595a405cfdaec6af3..6012402dfcfe453fd710d0b4c9a9e09f8953f63a 100644
--- a/commit-reach.h
+++ b/commit-reach.h
@@ -14,12 +14,12 @@ int repo_get_merge_bases(struct repository *r,
 			 struct commit *rev2,
 			 struct commit_list **result);
 int repo_get_merge_bases_many(struct repository *r,
-			      struct commit *one, int n,
+			      struct commit *one, size_t n,
 			      struct commit **twos,
 			      struct commit_list **result);
 /* To be used only when object flags after this call no longer matter */
 int repo_get_merge_bases_many_dirty(struct repository *r,
-				    struct commit *one, int n,
+				    struct commit *one, size_t n,
 				    struct commit **twos,
 				    struct commit_list **result);
 
diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c
index 01cf77ae65b9a7db2f31a9a1558b8bb84b2e81d3..028ec0030678284eba844e121c6eff88abdd3139 100644
--- a/t/helper/test-reach.c
+++ b/t/helper/test-reach.c
@@ -35,7 +35,7 @@ int cmd__reach(int ac, const char **av)
 	struct commit_list *X, *Y;
 	struct object_array X_obj = OBJECT_ARRAY_INIT;
 	struct commit **X_array, **Y_array;
-	int X_nr, X_alloc, Y_nr, Y_alloc;
+	size_t X_nr, X_alloc, Y_nr, Y_alloc;
 	struct strbuf buf = STRBUF_INIT;
 	struct repository *r = the_repository;
 
@@ -157,7 +157,7 @@ int cmd__reach(int ac, const char **av)
 		clear_contains_cache(&cache);
 	} else if (!strcmp(av[1], "get_reachable_subset")) {
 		const int reachable_flag = 1;
-		int i, count = 0;
+		int count = 0;
 		struct commit_list *current;
 		struct commit_list *list = get_reachable_subset(X_array, X_nr,
 								Y_array, Y_nr,
@@ -169,7 +169,7 @@ int cmd__reach(int ac, const char **av)
 				    oid_to_hex(&list->item->object.oid));
 			count++;
 		}
-		for (i = 0; i < Y_nr; i++) {
+		for (size_t i = 0; i < Y_nr; i++) {
 			if (Y_array[i]->object.flags & reachable_flag)
 				count--;
 		}

-- 
2.48.0.rc0.184.g0fc57dec57.dirty

Re: [PATCH 9/9] commit-reach: use `size_t` to track indices when computing merge bases

[Justin Tobler]

On 24/12/27 11:46AM, Patrick Steinhardt wrote:
> The functions `repo_get_merge_bases_many()` and friends accepts an array
> of commits as well as a parameter that indicates how large that array
> is. This parameter is using a signed integer, which leads to a couple of
> warnings with -Wsign-compare.
> 
> Refactor the code to use `size_t` to track indices instead and adapt
> callers accordingly. While most callers are trivial, there are two
> callers that require a bit more scrutiny:
> 
>   - builtin/merge-base.c:show_merge_base() subtracts `1` from the
>     `rev_nr` before calling `repo_get_merge_bases_many_dirty()`, so if
>     the variable was `0` it would wrap. This code is fine though because
>     its only caller will execute that code only when `argc >= 2`, and it
>     follows that `rev_nr >= 2`, as well.
> 
>   - bisect.ccheck_merge_bases() similarly subtracts `1` from `rev_nr`.

s/ccheck/check/

Small typo, but probably not worth rerolling.

>     Again, there is only a single caller that populates `rev_nr` with
>     `good_revs.nr`. And because a bisection always requires at least one
>     good revision it follws that `rev_nr >= 1`.
> 
> Mark the file as -Wsign-compare-clean.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Thanks Patrick! I reviewed the series and overall it looks good to me :)

-Justin

Re: [PATCH 9/9] commit-reach: use `size_t` to track indices when computing merge bases

[Patrick Steinhardt]

On Thu, Jan 02, 2025 at 08:08:15PM -0600, Justin Tobler wrote:
> On 24/12/27 11:46AM, Patrick Steinhardt wrote:
> > The functions `repo_get_merge_bases_many()` and friends accepts an array
> > of commits as well as a parameter that indicates how large that array
> > is. This parameter is using a signed integer, which leads to a couple of
> > warnings with -Wsign-compare.
> > 
> > Refactor the code to use `size_t` to track indices instead and adapt
> > callers accordingly. While most callers are trivial, there are two
> > callers that require a bit more scrutiny:
> > 
> >   - builtin/merge-base.c:show_merge_base() subtracts `1` from the
> >     `rev_nr` before calling `repo_get_merge_bases_many_dirty()`, so if
> >     the variable was `0` it would wrap. This code is fine though because
> >     its only caller will execute that code only when `argc >= 2`, and it
> >     follows that `rev_nr >= 2`, as well.
> > 
> >   - bisect.ccheck_merge_bases() similarly subtracts `1` from `rev_nr`.
> 
> s/ccheck/check/
> 
> Small typo, but probably not worth rerolling.
> 
> >     Again, there is only a single caller that populates `rev_nr` with
> >     `good_revs.nr`. And because a bisection always requires at least one
> >     good revision it follws that `rev_nr >= 1`.
> > 
> > Mark the file as -Wsign-compare-clean.
> > 
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> 
> Thanks Patrick! I reviewed the series and overall it looks good to me :)

Thanks for your review!

Patrick

Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Junio C Hamano]

Speaking of -Wsign-compare (cf. [*1*]), we seem to be hitting this
error on linux32 CI job [*2*]:

Error: shallow.c:537:32: comparison of integer expressions of different signedness:
'unsigned int' and 'int' [-Werror=sign-compare]

    537 |  if (!info->pool_count || size > info->end - info->free) {

I didn't dig deeper than this.  After taking three tries to get
'seen' build with linux-meson job (needed merge-fix for ds/backfill
topic, which needed (1) a new built-in hence a new entry in
meson.build, (2) a new test hence a new entry in t/meson.build, and
(3) a new doc hence a new entry in Documentation/meson.build), I am
a bit exhausted right now.

Also breakage of linux-meson job we have at 'master' seems gone,
which probably has to do with your recent update with gitweb thing.

Thanks.


[References]

*1* 

https://staticthinking.wordpress.com/2023/07/25/wsign-compare-is-garbage/

*2*

https://github.com/git/git/actions/runs/12519901432/job/34924707204#step:6:181

Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Error: shallow.c:537:32: comparison of integer expressions of different signedness:
> 'unsigned int' and 'int' [-Werror=sign-compare]
>
>     537 |  if (!info->pool_count || size > info->end - info->free) {
>
> I didn't dig deeper than this.

What we want to express seems to be:

    If the region between "end" and "free" is smaller than the
    required "size" then we are in trouble.

which can be said more naturally with

    If the region of size "size" starting at "free" pointer overruns the
    "end" pointer, we are in trouble.

So perhaps something like this would help?  We are no longer making
a comparison between two integers with this rewrite.

 shallow.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git c/shallow.c w/shallow.c
index b8fcfbef0f..b54244ffa9 100644
--- c/shallow.c
+++ w/shallow.c
@@ -534,7 +534,7 @@ static uint32_t *paint_alloc(struct paint_info *info)
 	unsigned nr = DIV_ROUND_UP(info->nr_bits, 32);
 	unsigned size = nr * sizeof(uint32_t);
 	void *p;
-	if (!info->pool_count || size > info->end - info->free) {
+	if (!info->pool_count || info->end < info->free + size) {
 		if (size > POOL_SIZE)
 			BUG("pool size too small for %d in paint_alloc()",
 			    size);

Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Jeff King]

On Fri, Dec 27, 2024 at 12:08:03PM -0800, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Error: shallow.c:537:32: comparison of integer expressions of different signedness:
> > 'unsigned int' and 'int' [-Werror=sign-compare]
> >
> >     537 |  if (!info->pool_count || size > info->end - info->free) {
> >
> > I didn't dig deeper than this.
> 
> What we want to express seems to be:
> 
>     If the region between "end" and "free" is smaller than the
>     required "size" then we are in trouble.
> 
> which can be said more naturally with
> 
>     If the region of size "size" starting at "free" pointer overruns the
>     "end" pointer, we are in trouble.
> 
> So perhaps something like this would help?  We are no longer making
> a comparison between two integers with this rewrite.
> 
>  shallow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git c/shallow.c w/shallow.c
> index b8fcfbef0f..b54244ffa9 100644
> --- c/shallow.c
> +++ w/shallow.c
> @@ -534,7 +534,7 @@ static uint32_t *paint_alloc(struct paint_info *info)
>  	unsigned nr = DIV_ROUND_UP(info->nr_bits, 32);
>  	unsigned size = nr * sizeof(uint32_t);
>  	void *p;
> -	if (!info->pool_count || size > info->end - info->free) {
> +	if (!info->pool_count || info->end < info->free + size) {
>  		if (size > POOL_SIZE)
>  			BUG("pool size too small for %d in paint_alloc()",
>  			    size);

I doubt it is a practical problem in this instance, but as a general
rule, I think bounds checks like this should avoid using addition
because it can lead to overflow. In this code, imagine that the free
pool was allocated near the end of the address space, but somebody asked
for a very large "size", causing the addition to wrap around.

I think even without that overflow this might be undefined behavior,
because "info->free + size" is forming a pointer that may be outside the
allocated array.

So as a general rule, bounds comparisons like this should be formulated
as "how much free space do we have" compared to "how much are we asking
for". And there "info->end - info->free" is the right way to ask the
first half of that question.

Now where it gets weird with -Wsign-compare is that the pointer
difference is giving us a signed value. Which makes sense in the general
case (you could ask for "info->free - info->end", for example). But we
know that it will always be non-negative because we know that info->free
is <= info->end, coming from earlier in the same allocation.

I doubt there is a way to tell the compiler that (or that a compiler
could even switch to an unsigned ptrdiff type if it knew that). But I
wonder if there is a generalized helper we can devise that would avoid
simply casting here. I guess that could be a checked cast like:

  static inline size_t ptrdiff_to_size(ptrdiff_t v)
  {
	if (v < 0)
		BUG("surprising negative value: %"PRIdMAX, v);
	return (size_t)v;
  }

or even:

  static inline bool has_space(const void *vs, const void *ve, size_t want)
  {
	const char *s = vs, e = ve;
	return want <= ptrdiff_to_size(ve - vs);
  }

I don't love hiding basic things like this behind macros or inlines. But
allocation and bounds comparisons do have gotchas (especially against an
adversary that can try to create pathological situations). Maybe it's worth
having an easy way to do them safely without having to think about each
one. I dunno.

-Peff

Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Patrick Steinhardt]

On Fri, Dec 27, 2024 at 04:37:29PM -0500, Jeff King wrote:
> On Fri, Dec 27, 2024 at 12:08:03PM -0800, Junio C Hamano wrote:
> I doubt there is a way to tell the compiler that (or that a compiler
> could even switch to an unsigned ptrdiff type if it knew that). But I
> wonder if there is a generalized helper we can devise that would avoid
> simply casting here. I guess that could be a checked cast like:
> 
>   static inline size_t ptrdiff_to_size(ptrdiff_t v)
>   {
> 	if (v < 0)
> 		BUG("surprising negative value: %"PRIdMAX, v);
> 	return (size_t)v;
>   }
> 
> or even:
> 
>   static inline bool has_space(const void *vs, const void *ve, size_t want)
>   {
> 	const char *s = vs, e = ve;
> 	return want <= ptrdiff_to_size(ve - vs);
>   }
> 
> I don't love hiding basic things like this behind macros or inlines. But
> allocation and bounds comparisons do have gotchas (especially against an
> adversary that can try to create pathological situations). Maybe it's worth
> having an easy way to do them safely without having to think about each
> one. I dunno.

I think having a wrapper like `cast_ptrdiff_to_size_t()` would be a
sensible solution for now, also because it fits in nicely with
`cast_size_t_to_int()`. I'll introduce such a wrapper once I've got a
good excuse to do so.

Patrick

Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> So perhaps something like this would help?  We are no longer making
> a comparison between two integers with this rewrite.

... and this gave us a first "pass" of the tip of 'seen' for all
jobs since for quite some time, like a few weeks.

Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Patrick Steinhardt]

On Fri, Dec 27, 2024 at 04:00:38PM -0800, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > So perhaps something like this would help?  We are no longer making
> > a comparison between two integers with this rewrite.
> 
> ... and this gave us a first "pass" of the tip of 'seen' for all
> jobs since for quite some time, like a few weeks.

Thanks for your fix. I'll have a look at whether I can include a 32 bit
job into GitLab CI for improved test coverage here so that it does not
fall on you to fix up things like this going forward.

Overall, CI has been in pretty rough shape recently. Besides the
mentioned failures there's also been the issue with flaky tests:

  - t5616-partial-clone regularly fails on macOS. [1] This seems like a
    race condition or to me:

    error: unable to open .git/objects/pack/pack-029d08823bd8a8eab510ad6ac75c823cfd3ed31e.pack: No such file or directory
    fatal: unable to rename temporary file to '.git/objects/pack/pack-029d08823bd8a8eab510ad6ac75c823cfd3ed31e.pack'
    fatal: could not finish pack-objects to repack local links
    fatal: index-pack failed
    error: last command exited with $?=128
    not ok 18 - fetch --refetch triggers repacking

  - The leak-checking jobs fail quite regularly in t0003 with something
    that feels like either a race caused by a leak or an issue with the
    sanitizer itself [2]:

    ==git==17055==ERROR: LeakSanitizer: detected memory leaks
    Direct leak of 32 byte(s) in 1 object(s) allocated from:
    #0 0x7aa0d03c7713 in realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
    #1 0x7aa0d0221f69 in pthread_getattr_np (/lib/x86_64-linux-gnu/libc.so.6+0x9df69) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
    #2 0x7aa0d03d9544 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150
    #3 0x7aa0d03d96fa in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:614
    #4 0x7aa0d03cb2b9 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:53
    #5 0x7aa0d03c756a in ThreadStartFunc<false> ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:431
    #6 0x7aa0d0220a93  (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
    #7 0x7aa0d02adc3b  (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
    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>----
    SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).

  - Windows has been quite flaky since adding it to GitLab CI. No idea
    whether it's the same for GitHub Actions.

The thing is, the less reliable it becomes the more likely it is that
people are simply going to ignore its results.

I'll try to have a look at some of the problems on Monday.

Patrick

[1]: https://gitlab.com/git-scm/git/-/jobs/8671168627
[2]: https://gitlab.com/git-scm/git/-/jobs/8671168849

Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Thanks for your fix. I'll have a look at whether I can include a 32 bit
> job into GitLab CI for improved test coverage here so that it does not
> fall on you to fix up things like this going forward.

I noticed it since it failed GitHub actions thing which already has 32-bit
job.

>   - t5616-partial-clone regularly fails on macOS. [1] This seems like a
>     race condition or to me:

I've seen it before as well at GitHub actions side.  Running
"t5616-*.sh --stress" locally on Debian (x86-64) did not help
isolate it very well.

>   - The leak-checking jobs fail quite regularly in t0003 with something
>     that feels like either a race caused by a leak or an issue with the
>     sanitizer itself [2]:

This one I am not aware of.

>   - Windows has been quite flaky since adding it to GitLab CI. No idea
>     whether it's the same for GitHub Actions.

Similar on GitHub CI front.  Not that I am playing favors between
GitHub and GitLab, but for historical reasons I've pushed to the
former myself but not to the latter, so I do not notice breakages on
the latter.


> The thing is, the less reliable it becomes the more likely it is that
> people are simply going to ignore its results.

Indeed.  Also, for macOS and Windows, I have no access to an
environment to let me debug, so it is really up to the platform
stakeholders to see what they can do to help.

Thanks.

racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Jeff King]

On Sat, Dec 28, 2024 at 09:27:41AM +0100, Patrick Steinhardt wrote:

>   - The leak-checking jobs fail quite regularly in t0003 with something
>     that feels like either a race caused by a leak or an issue with the
>     sanitizer itself [2]:
> 
>     ==git==17055==ERROR: LeakSanitizer: detected memory leaks
>     Direct leak of 32 byte(s) in 1 object(s) allocated from:
>     #0 0x7aa0d03c7713 in realloc ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:98
>     #1 0x7aa0d0221f69 in pthread_getattr_np (/lib/x86_64-linux-gnu/libc.so.6+0x9df69) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
>     #2 0x7aa0d03d9544 in __sanitizer::GetThreadStackTopAndBottom(bool, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:150
>     #3 0x7aa0d03d96fa in __sanitizer::GetThreadStackAndTls(bool, unsigned long*, unsigned long*, unsigned long*, unsigned long*) ../../../../src/libsanitizer/sanitizer_common/sanitizer_linux_libcdep.cpp:614
>     #4 0x7aa0d03cb2b9 in __lsan::ThreadStart(unsigned int, unsigned long long, __sanitizer::ThreadType) ../../../../src/libsanitizer/lsan/lsan_posix.cpp:53
>     #5 0x7aa0d03c756a in ThreadStartFunc<false> ../../../../src/libsanitizer/lsan/lsan_interceptors.cpp:431
>     #6 0x7aa0d0220a93  (/lib/x86_64-linux-gnu/libc.so.6+0x9ca93) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
>     #7 0x7aa0d02adc3b  (/lib/x86_64-linux-gnu/libc.so.6+0x129c3b) (BuildId: 6d64b17fbac799e68da7ebd9985ddf9b5cb375e6)
>     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>----
>     SUMMARY: LeakSanitizer: 32 byte(s) leaked in 1 allocation(s).

I did a bit of digging on this last week, but didn't come up with a very
satisfactory solution. You can reproduce easily by building with
SANITIZE=leak and running t0003 with --stress.

It's the same issue we tried to address in 993d38a066 (index-pack: spawn
threads atomically, 2024-01-05): one thread calls exit() and takes down
the process while other threads are still being spawned (so the leak
checker runs while those other threads are in a weird state where lsan
has allocated some memory but not yet set up the thread stack as a place
to look for reachable memory).

There we dealt with it by making sure no thread started work (and thus
hit the exit call) until all of them were sanitized. I tried doing
something similar here, like:

  diff --git a/builtin/grep.c b/builtin/grep.c
  index 98b85c7fca..866645e6f8 100644
  --- a/builtin/grep.c
  +++ b/builtin/grep.c
  @@ -233,18 +233,20 @@ static void start_threads(struct grep_opt *opt)
                  strbuf_init(&todo[i].out, 0);
          }

  +       grep_lock();
          CALLOC_ARRAY(threads, num_threads);
          for (i = 0; i < num_threads; i++) {
                  int err;
                  struct grep_opt *o = grep_opt_dup(opt);
                  o->output = strbuf_out;
                  compile_grep_patterns(o);
                  err = pthread_create(&threads[i], NULL, run, o);

                  if (err)
                          die(_("grep: failed to create thread: %s"),
                              strerror(err));
          }
  +       grep_unlock();
   }

   static int wait_all(void)

but it doesn't work. In fact, this does nothing at all because each
thread will start by looking for work to do via get_work(), and we do
not call add_work() to give them anything to do until all threads are
spawned.

So I suspect the race is actually trickier, and that the "weird state"
is not something that happens just while pthread_create() is being
called, but is actually running _in the thread itself_. So even though
pthread_create() has returned for each thread, they are still setting
themselves up before running.

It mostly worked in 993d38a066 because index-pack has to do more work to
get to the exit() call. So delaying the start of the threads was enough
to usually win the race. But here, the individual grep threads get to
the exit() call very quickly, and it's not enough.

Which would mean that 993d38a066 is not actually a full fix, either. And
indeed, I can get t5309 to fail even with that patch (which is weird,
because that was how I tested it originally; I wonder if anything on the
LSan side changed?).

So a full fix would actually require synchronization where we spawn each
thread, then wait for all of them to hit the barrier to declare
themselves ready, and then let them all start running. There is a
pthread_barrier type that would help with this, but we've never used it
before (so we'd probably need to at least provide a Windows compat
layer).

One quick workaround is this:

diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index 3c98b622f2..7ecaf8f4e3 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -443,7 +443,7 @@ test_expect_success 'diff without repository with attr source' '
 		cat >expect <<-EOF &&
 		fatal: cannot use --attr-source or GIT_ATTR_SOURCE without repo
 		EOF
-		test_must_fail env GIT_ATTR_SOURCE=HEAD git grep --no-index foo file 2>err &&
+		test_must_fail env GIT_ATTR_SOURCE=HEAD git grep --threads=1 --no-index foo file 2>err &&
 		test_cmp expect err
 	)
 '

Or you could even imagine automatically forcing online_cpus() to "1" for
LSan builds, which would fix it everywhere. But then we'd miss any leaks
that are specific to the threaded code.

Or of course we could try to engage with LSan folks about whether this
can be fixed there. I don't think we've reported it anywhere there.

-Peff

Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Jeff King]

On Sat, Dec 28, 2024 at 02:05:41PM -0500, Jeff King wrote:

> So I suspect the race is actually trickier, and that the "weird state"
> is not something that happens just while pthread_create() is being
> called, but is actually running _in the thread itself_. So even though
> pthread_create() has returned for each thread, they are still setting
> themselves up before running.
> [...]
> So a full fix would actually require synchronization where we spawn each
> thread, then wait for all of them to hit the barrier to declare
> themselves ready, and then let them all start running. There is a
> pthread_barrier type that would help with this, but we've never used it
> before (so we'd probably need to at least provide a Windows compat
> layer).

So here's a fix that seems to work, but doesn't address the portability
issues. One tweak is that the exit() call is actually not in a
sub-thread, but in the main thread itself. I don't think that really
changes the analysis too much, but it does mean we need to include the
main thread in the barrier wait.

diff --git a/builtin/grep.c b/builtin/grep.c
index d00ee76f24..933b4503b8 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -101,6 +101,9 @@ static pthread_cond_t cond_write;
 /* Signalled when we are finished with everything. */
 static pthread_cond_t cond_result;
 
+/* Synchronize the start of all threads */
+static pthread_barrier_t start_barrier;
+
 static int skip_first_line;
 
 static void add_work(struct grep_opt *opt, struct grep_source *gs)
@@ -198,6 +201,8 @@ static void *run(void *arg)
 	int hit = 0;
 	struct grep_opt *opt = arg;
 
+	pthread_barrier_wait(&start_barrier);
+
 	while (1) {
 		struct work_item *w = get_work();
 		if (!w)
@@ -229,6 +234,7 @@ static void start_threads(struct grep_opt *opt)
 	pthread_cond_init(&cond_add, NULL);
 	pthread_cond_init(&cond_write, NULL);
 	pthread_cond_init(&cond_result, NULL);
+	pthread_barrier_init(&start_barrier, NULL, num_threads + 1);
 	grep_use_locks = 1;
 	enable_obj_read_lock();
 
@@ -248,6 +254,7 @@ static void start_threads(struct grep_opt *opt)
 			die(_("grep: failed to create thread: %s"),
 			    strerror(err));
 	}
+	pthread_barrier_wait(&start_barrier);
 }
 
 static int wait_all(void)
@@ -284,6 +291,7 @@ static int wait_all(void)
 	pthread_cond_destroy(&cond_add);
 	pthread_cond_destroy(&cond_write);
 	pthread_cond_destroy(&cond_result);
+	pthread_barrier_destroy(&start_barrier);
 	grep_use_locks = 0;
 	disable_obj_read_lock();
 

It would probably be pretty easy to do something similar for index-pack,
though really this is something I guess we'd need to do manually
anywhere we spawn worker threads.

-Peff

Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Jeff King]

On Sat, Dec 28, 2024 at 02:23:07PM -0500, Jeff King wrote:

> It would probably be pretty easy to do something similar for index-pack,
> though really this is something I guess we'd need to do manually
> anywhere we spawn worker threads.

Here's what that would look like. Confirmed that this fixes the issue
running t5309 with --stress. Dropping the extra locks in
resolve_deltas() is just backing out that earlier insufficient attempt
to synchronize things.

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index d773809c4c..de1f5d8628 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -185,6 +185,8 @@ static pthread_mutex_t deepest_delta_mutex;
 
 static pthread_key_t key;
 
+static pthread_barrier_t start_barrier;
+
 static inline void lock_mutex(pthread_mutex_t *mutex)
 {
 	if (threads_active)
@@ -209,6 +211,7 @@ static void init_thread(void)
 	if (show_stat)
 		pthread_mutex_init(&deepest_delta_mutex, NULL);
 	pthread_key_create(&key, NULL);
+	pthread_barrier_init(&start_barrier, NULL, nr_threads);
 	CALLOC_ARRAY(thread_data, nr_threads);
 	for (i = 0; i < nr_threads; i++) {
 		thread_data[i].pack_fd = xopen(curr_pack, O_RDONLY);
@@ -231,6 +234,7 @@ static void cleanup_thread(void)
 	for (i = 0; i < nr_threads; i++)
 		close(thread_data[i].pack_fd);
 	pthread_key_delete(key);
+	pthread_barrier_destroy(&start_barrier);
 	free(thread_data);
 }
 
@@ -1100,6 +1104,8 @@ static int compare_ref_delta_entry(const void *a, const void *b)
 
 static void *threaded_second_pass(void *data)
 {
+	if (threads_active)
+		pthread_barrier_wait(&start_barrier);
 	if (data)
 		set_thread_data(data);
 	for (;;) {
@@ -1336,15 +1342,13 @@ static void resolve_deltas(struct pack_idx_option *opts)
 	base_cache_limit = opts->delta_base_cache_limit * nr_threads;
 	if (nr_threads > 1 || getenv("GIT_FORCE_THREADS")) {
 		init_thread();
-		work_lock();
 		for (i = 0; i < nr_threads; i++) {
 			int ret = pthread_create(&thread_data[i].thread, NULL,
 						 threaded_second_pass, thread_data + i);
 			if (ret)
 				die(_("unable to create thread: %s"),
 				    strerror(ret));
 		}
-		work_unlock();
 		for (i = 0; i < nr_threads; i++)
 			pthread_join(thread_data[i].thread, NULL);
 		cleanup_thread();

Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[René Scharfe]

Am 28.12.2024 um 20:23 schrieb Jeff King:
> On Sat, Dec 28, 2024 at 02:05:41PM -0500, Jeff King wrote:
>
>> So I suspect the race is actually trickier, and that the "weird state"
>> is not something that happens just while pthread_create() is being
>> called, but is actually running _in the thread itself_. So even though
>> pthread_create() has returned for each thread, they are still setting
>> themselves up before running.
>> [...]
>> So a full fix would actually require synchronization where we spawn each
>> thread, then wait for all of them to hit the barrier to declare
>> themselves ready, and then let them all start running. There is a
>> pthread_barrier type that would help with this, but we've never used it
>> before (so we'd probably need to at least provide a Windows compat
>> layer).
>
> So here's a fix that seems to work, but doesn't address the portability
> issues.

Windows has Synchronization Barriers.  Adding the following lines to
compat/win32/pthread.h at least lets your example compile and run:

#include <synchapi.h>
#define pthread_barrier_t SYNCHRONIZATION_BARRIER
#define PTHREAD_BARRIER_SERIAL_THREAD TRUE
#define pthread_barrier_init(b, a, c) return_0(InitializeSynchronizationBarrier((b), (c), -1))
#define pthread_barrier_destroy(b) return_0(DeleteSynchronizationBarrier(b))
#define pthread_barrier_wait(b) EnterSynchronizationBarrier((b), 0)

Catching a non-NULL second argument to pthread_barrier_init() would be a
good idea in a production version.  Error handling would be a good idea
in general, but callers would then actually have to check those errors.

Synchronization Barriers were added with Windows 8 and Windows Server
2012, Git for Windows requires higher versions, so this native
mechanism should be usable.  Relevant links:

https://learn.microsoft.com/en-us/windows/win32/sync/synchronization-barriers
https://github.com/git-for-windows/git/wiki/FAQ


However, macOS doesn't have pthread barriers.  Here's an implementation
that had to be fixed to satisfy Coverity, so it might be good now?

https://github.com/libusb/hidapi/blob/master/mac/hid.c

Perhaps that implementation could be used for Windows as well?  All
functions it uses are provided by compat/win32/pthread.h; not sure if
they are sufficiently fleshed out, though.

René

Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Jeff King]

On Sun, Dec 29, 2024 at 01:02:13PM +0100, René Scharfe wrote:

> Synchronization Barriers were added with Windows 8 and Windows Server
> 2012, Git for Windows requires higher versions, so this native
> mechanism should be usable.  Relevant links:
> [...]
> However, macOS doesn't have pthread barriers.  Here's an implementation
> that had to be fixed to satisfy Coverity, so it might be good now?

Yep, that matches what I'd found so far.

One of the reasons I hadn't sent anything is that I was waffling between
two approaches:

  - implement barriers everywhere and just use them. More work, but we'd
    have the tool if we wanted to use it later, and all builds behave
    the same.

  - make a "maybe_barrier" interface that might be a noop, and let most
    platforms compile without them. They are not needed for correct
    operation in most cases, but only to work around a sanitizer problem.
    And it is not even a problem that comes up frequently; it is a race
    that we occasionally see in CI. So enabling it only for our
    linux-leaks CI job would be enough to dull the pain.

    And there is no risk of any portability or run-time issues, because
    the code is a noop for most builds.

> https://github.com/libusb/hidapi/blob/master/mac/hid.c
> 
> Perhaps that implementation could be used for Windows as well?  All
> functions it uses are provided by compat/win32/pthread.h; not sure if
> they are sufficiently fleshed out, though.

Yeah, I found a similar one. I think it's an undergrad OS assignment to
implement barriers using semaphores, but probably building on a mutex
and cond is less horrible. ;)

-Peff

Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Jeff King]

On Sun, Dec 29, 2024 at 11:57:15AM -0500, Jeff King wrote:

> One of the reasons I hadn't sent anything is that I was waffling between
> two approaches:
> 
>   - implement barriers everywhere and just use them. More work, but we'd
>     have the tool if we wanted to use it later, and all builds behave
>     the same.
> 
>   - make a "maybe_barrier" interface that might be a noop, and let most
>     platforms compile without them. They are not needed for correct
>     operation in most cases, but only to work around a sanitizer problem.
>     And it is not even a problem that comes up frequently; it is a race
>     that we occasionally see in CI. So enabling it only for our
>     linux-leaks CI job would be enough to dull the pain.
> 
>     And there is no risk of any portability or run-time issues, because
>     the code is a noop for most builds.

I sent the "maybe" interface in this series if you want to take a look:

  https://lore.kernel.org/git/20241230042325.GA112439@coredump.intra.peff.net/

I'm not married to that approach, but it seemed like the easiest place
to start. And I especially wanted to see how much hand-waving was
required to justify it in the third patch. It's a fair bit. ;)

-Peff

Re: racy leak sanitizer builds, was Re: [PATCH 0/9] commit-reach: -Wsign-compare follow-ups

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> One of the reasons I hadn't sent anything is that I was waffling between
> two approaches:
>
>   - implement barriers everywhere and just use them. More work, but we'd
>     have the tool if we wanted to use it later, and all builds behave
>     the same.
>
>   - make a "maybe_barrier" interface that might be a noop, and let most
>     platforms compile without them. They are not needed for correct
>     operation in most cases, but only to work around a sanitizer problem.
>     And it is not even a problem that comes up frequently; it is a race
>     that we occasionally see in CI. So enabling it only for our
>     linux-leaks CI job would be enough to dull the pain.
>
>     And there is no risk of any portability or run-time issues, because
>     the code is a noop for most builds.

I love when people think before committing to an approach, and after
seeing these two cohices, I tend to have slight preference for the
latter over the former.  The work will not be wasted even if it
later turns out that we need a full-blown barrier implementation for
other platforms.

Thanks.