[PATCH 0/2] string-list.h: make "nr" and "alloc" a "size_t"

[PATCH 0/2] string-list.h: make "nr" and "alloc" a "size_t"

[Ævar Arnfjörð Bjarmason]

$subject most recently came up in
https://lore.kernel.org/git/220305.86pmn030ou.gmgdl@evledraar.gmail.com/. I.e. we
have code in-tree already that's possibly (although in that case
unlikely to in practice) keeping track of size_t-indexed with a
string_list's "unsigned int"-indexed data.

Now there are no textual or semantic conflicts between this change and
"seen", so having series (which is of course based on "master") now
would be a good thing, and help to future-proof the API.

The 1/2 here isn't strictly neccesary as part of this series, but
since a large part 2/2 is adjusting the same sort of Q_() patterns I
thought it made sense to both show existing PRIuMAX use in those
macros, and how we do those casts.

Ævar Arnfjörð Bjarmason (2):
  gettext API users: correct use of casts for Q_()
  string-list API: change "nr" and "alloc" to "size_t"

 add-interactive.c           | 15 +++++++++------
 add-patch.c                 |  8 ++++----
 builtin/index-pack.c        |  2 +-
 builtin/receive-pack.c      |  9 +++++----
 builtin/shortlog.c          | 10 +++++-----
 bundle.c                    | 12 ++++++------
 commit-graph.c              |  8 ++++----
 mailmap.c                   |  7 ++++---
 merge-ort.c                 |  4 ++--
 string-list.h               |  3 ++-
 t/helper/test-run-command.c |  7 ++++---
 wt-status.c                 | 16 ++++++++--------
 12 files changed, 54 insertions(+), 47 deletions(-)

-- 
2.35.1.1241.gd8d69a17521

[PATCH 1/2] gettext API users: correct use of casts for Q_()

[Ævar Arnfjörð Bjarmason]

Change users of the inline gettext.h Q_() function to cast its
argument to "unsigned long" instead of "int" or "unsigned int".

The ngettext() function (which Q_() resolves to) takes an "unsigned
long int", and so does our Q_() wrapper for it, see 0c9ea33b90f (i18n:
add stub Q_() wrapper for ngettext, 2011-03-09).

In a subsequent commit we'll be making more use of this pattern of:

    func(Q_(..%"PRIuMAX..., (unsigned long)x), (uintmax_t)x);

By making this change we ensure that this case isn't the odd one out
in that post-image.

This:

 * Corrects code added in 7171a0b0cf5 (index-pack: correct "len" type
   in unpack_data(), 2016-07-13) to cast the "off_t len" to an
   "unsigned long int" rather than an "unsigned int".

 * Does the same for code in add-interactive.c added in several
   commits starting with a8c45be939d (built-in add -i: implement the
   `update` command, 2019-11-29).

 * Likewise for a case in 9254bdfb4f9 (built-in add -p: implement the
   'g' ("goto") command, 2019-12-13) where only the err() argument had
   a cast, but not the same argument to Q_().

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 add-interactive.c    | 15 +++++++++------
 add-patch.c          |  8 ++++----
 builtin/index-pack.c |  2 +-
 3 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/add-interactive.c b/add-interactive.c
index e1ab39cce30..6da781004ad 100644
--- a/add-interactive.c
+++ b/add-interactive.c
@@ -707,8 +707,9 @@ static int run_update(struct add_i_state *s, const struct pathspec *ps,
 		res = error(_("could not write index"));
 
 	if (!res)
-		printf(Q_("updated %d path\n",
-			  "updated %d paths\n", count), (int)count);
+		printf(Q_("updated %"PRIuMAX" path\n",
+			  "updated %"PRIuMAX" paths\n", (unsigned long)count),
+		       (uintmax_t)count);
 
 	putchar('\n');
 	return res;
@@ -814,8 +815,9 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps,
 						   NULL, NULL, NULL);
 
 	if (!res)
-		printf(Q_("reverted %d path\n",
-			  "reverted %d paths\n", count), (int)count);
+		printf(Q_("reverted %"PRIuMAX" path\n",
+			  "reverted %"PRIuMAX" paths\n", (unsigned long)count),
+		       (uintmax_t)count);
 
 finish_revert:
 	putchar('\n');
@@ -896,8 +898,9 @@ static int run_add_untracked(struct add_i_state *s, const struct pathspec *ps,
 		res = error(_("could not write index"));
 
 	if (!res)
-		printf(Q_("added %d path\n",
-			  "added %d paths\n", count), (int)count);
+		printf(Q_("added %"PRIuMAX" path\n",
+			  "added %"PRIuMAX" paths\n", (unsigned long)count),
+		       (uintmax_t)count);
 
 finish_add_untracked:
 	putchar('\n');
diff --git a/add-patch.c b/add-patch.c
index 55d719f7845..dfef00e5680 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -1569,10 +1569,10 @@ static int patch_update_file(struct add_p_state *s,
 			else if (0 < response && response <= file_diff->hunk_nr)
 				hunk_index = response - 1;
 			else
-				err(s, Q_("Sorry, only %d hunk available.",
-					  "Sorry, only %d hunks available.",
-					  file_diff->hunk_nr),
-				    (int)file_diff->hunk_nr);
+				err(s, Q_("Sorry, only %"PRIuMAX" hunk available.",
+					  "Sorry, only %"PRIuMAX" hunks available.",
+					  (unsigned long)file_diff->hunk_nr),
+				    (uintmax_t)file_diff->hunk_nr);
 		} else if (s->answer.buf[0] == '/') {
 			regex_t regex;
 			int ret;
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3c2e6aee3cc..f15b59e22b0 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -579,7 +579,7 @@ static void *unpack_data(struct object_entry *obj,
 		if (!n)
 			die(Q_("premature end of pack file, %"PRIuMAX" byte missing",
 			       "premature end of pack file, %"PRIuMAX" bytes missing",
-			       (unsigned int)len),
+			       (unsigned long)len),
 			    (uintmax_t)len);
 		from += n;
 		len -= n;
-- 
2.35.1.1241.gd8d69a17521

[PATCH 2/2] string-list API: change "nr" and "alloc" to "size_t"

[Ævar Arnfjörð Bjarmason]

Change the "nr" and "alloc" members of "struct string_list" to use
"size_t" instead of "nr". On some platforms the size of an "unsigned
int" will be smaller than a "size_t", e.g. a 32 bit unsigned v.s. 64
bit unsigned. As "struct string_list" is a generic API we use in a lot
of places this might cause overflows.

As one example: code in "refs.c" keeps track of the number of refs
with a "size_t", and auxiliary code in builtin/remote.c in
get_ref_states() appends those to a "struct string_list".

While we're at it split the "nr" and "alloc" in string-list.h across
two lines, which is the case for most such struct member
declarations (e.g. in "strbuf.h" and "strvec.h").

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/receive-pack.c      |  9 +++++----
 builtin/shortlog.c          | 10 +++++-----
 bundle.c                    | 12 ++++++------
 commit-graph.c              |  8 ++++----
 mailmap.c                   |  7 ++++---
 merge-ort.c                 |  4 ++--
 string-list.h               |  3 ++-
 t/helper/test-run-command.c |  7 ++++---
 wt-status.c                 | 16 ++++++++--------
 9 files changed, 40 insertions(+), 36 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d10aeb7e78f..fc948a27c4f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -813,13 +813,14 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 	proc.trace2_hook_name = hook_name;
 
 	if (feed_state->push_options) {
-		int i;
+		size_t i;
 		for (i = 0; i < feed_state->push_options->nr; i++)
 			strvec_pushf(&proc.env_array,
-				     "GIT_PUSH_OPTION_%d=%s", i,
+				     "GIT_PUSH_OPTION_%"PRIuMAX"=%s",
+				     (uintmax_t)i,
 				     feed_state->push_options->items[i].string);
-		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%d",
-			     feed_state->push_options->nr);
+		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%"PRIuMAX"",
+			     (uintmax_t)feed_state->push_options->nr);
 	} else
 		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT");
 
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 228d782754a..26c5c0cf935 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -435,7 +435,7 @@ static void add_wrapped_shortlog_msg(struct strbuf *sb, const char *s,
 
 void shortlog_output(struct shortlog *log)
 {
-	int i, j;
+	size_t i, j;
 	struct strbuf sb = STRBUF_INIT;
 
 	if (log->sort_by_number)
@@ -448,10 +448,10 @@ void shortlog_output(struct shortlog *log)
 				(int)UTIL_TO_INT(item), item->string);
 		} else {
 			struct string_list *onelines = item->util;
-			fprintf(log->file, "%s (%d):\n",
-				item->string, onelines->nr);
-			for (j = onelines->nr - 1; j >= 0; j--) {
-				const char *msg = onelines->items[j].string;
+			fprintf(log->file, "%s (%"PRIuMAX"):\n",
+				item->string, (uintmax_t)onelines->nr);
+			for (j = onelines->nr; j >= 1; j--) {
+				const char *msg = onelines->items[j - 1].string;
 
 				if (log->wrap_lines) {
 					strbuf_reset(&sb);
diff --git a/bundle.c b/bundle.c
index a0bb687b0f4..0ebc8733679 100644
--- a/bundle.c
+++ b/bundle.c
@@ -255,18 +255,18 @@ int verify_bundle(struct repository *r,
 
 		r = &header->references;
 		printf_ln(Q_("The bundle contains this ref:",
-			     "The bundle contains these %d refs:",
-			     r->nr),
-			  r->nr);
+			     "The bundle contains these %"PRIuMAX" refs:",
+			     (unsigned long)r->nr),
+			  (uintmax_t)r->nr);
 		list_refs(r, 0, NULL);
 		r = &header->prerequisites;
 		if (!r->nr) {
 			printf_ln(_("The bundle records a complete history."));
 		} else {
 			printf_ln(Q_("The bundle requires this ref:",
-				     "The bundle requires these %d refs:",
-				     r->nr),
-				  r->nr);
+				     "The bundle requires these %"PRIuMAX" refs:",
+				     (unsigned long)r->nr),
+				  (uintmax_t)r->nr);
 			list_refs(r, 0, NULL);
 		}
 	}
diff --git a/commit-graph.c b/commit-graph.c
index 265c010122e..c7584f802b0 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1690,10 +1690,10 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
 	dirlen = packname.len;
 	if (ctx->report_progress) {
 		strbuf_addf(&progress_title,
-			    Q_("Finding commits for commit graph in %d pack",
-			       "Finding commits for commit graph in %d packs",
-			       pack_indexes->nr),
-			    pack_indexes->nr);
+			    Q_("Finding commits for commit graph in %"PRIuMAX" pack",
+			       "Finding commits for commit graph in %"PRIuMAX" packs",
+			       (unsigned long)pack_indexes->nr),
+			    (uintmax_t)pack_indexes->nr);
 		ctx->progress = start_delayed_progress(progress_title.buf, 0);
 		ctx->progress_done = 0;
 	}
diff --git a/mailmap.c b/mailmap.c
index 40ce152024d..7befdc5e483 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -43,8 +43,8 @@ static void free_mailmap_info(void *p, const char *s)
 static void free_mailmap_entry(void *p, const char *s)
 {
 	struct mailmap_entry *me = (struct mailmap_entry *)p;
-	debug_mm("mailmap: removing entries for <%s>, with %d sub-entries\n",
-		 s, me->namemap.nr);
+	debug_mm("mailmap: removing entries for <%s>, with %"PRIuMAX" sub-entries\n",
+		 s, (uintmax_t)me->namemap.nr);
 	debug_mm("mailmap: - simple: '%s' <%s>\n",
 		 debug_str(me->name), debug_str(me->email));
 
@@ -250,7 +250,8 @@ int read_mailmap(struct string_list *map)
 
 void clear_mailmap(struct string_list *map)
 {
-	debug_mm("mailmap: clearing %d entries...\n", map->nr);
+	debug_mm("mailmap: clearing %"PRIuMAX" entries...\n",
+		 (uintmax_t)map->nr);
 	map->strdup_strings = 1;
 	string_list_clear_func(map, free_mailmap_entry);
 	debug_mm("mailmap: cleared\n");
diff --git a/merge-ort.c b/merge-ort.c
index 55decb2587e..bb2be2a2758 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4064,8 +4064,8 @@ static void process_entries(struct merge_options *opt,
 	trace2_region_enter("merge", "process_entries cleanup", opt->repo);
 	if (dir_metadata.offsets.nr != 1 ||
 	    (uintptr_t)dir_metadata.offsets.items[0].util != 0) {
-		printf("dir_metadata.offsets.nr = %d (should be 1)\n",
-		       dir_metadata.offsets.nr);
+		printf("dir_metadata.offsets.nr = %"PRIuMAX" (should be 1)\n",
+		       (uintmax_t)dir_metadata.offsets.nr);
 		printf("dir_metadata.offsets.items[0].util = %u (should be 0)\n",
 		       (unsigned)(uintptr_t)dir_metadata.offsets.items[0].util);
 		fflush(stdout);
diff --git a/string-list.h b/string-list.h
index 267d6e5769d..d5a744e1438 100644
--- a/string-list.h
+++ b/string-list.h
@@ -86,7 +86,8 @@ typedef int (*compare_strings_fn)(const char *, const char *);
  */
 struct string_list {
 	struct string_list_item *items;
-	unsigned int nr, alloc;
+	size_t nr;
+	size_t alloc;
 	unsigned int strdup_strings:1;
 	compare_strings_fn cmp; /* NULL uses strcmp() */
 };
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 913775a14b7..eabd28defc5 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -180,15 +180,16 @@ static int testsuite(int argc, const char **argv)
 	if (max_jobs > suite.tests.nr)
 		max_jobs = suite.tests.nr;
 
-	fprintf(stderr, "Running %d tests (%d at a time)\n",
-		suite.tests.nr, max_jobs);
+	fprintf(stderr, "Running %"PRIuMAX" tests (%d at a time)\n",
+		(uintmax_t)suite.tests.nr, max_jobs);
 
 	ret = run_processes_parallel(max_jobs, next_test, test_failed,
 				     test_finished, &suite);
 
 	if (suite.failed.nr > 0) {
 		ret = 1;
-		fprintf(stderr, "%d tests failed:\n\n", suite.failed.nr);
+		fprintf(stderr, "%"PRIuMAX" tests failed:\n\n",
+			(uintmax_t)suite.failed.nr);
 		for (i = 0; i < suite.failed.nr; i++)
 			fprintf(stderr, "\t%s\n", suite.failed.items[i].string);
 	}
diff --git a/wt-status.c b/wt-status.c
index 335e723a71e..b3d398d7864 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1374,10 +1374,10 @@ static void show_rebase_information(struct wt_status *s,
 			status_printf_ln(s, color, _("No commands done."));
 		else {
 			status_printf_ln(s, color,
-				Q_("Last command done (%d command done):",
-					"Last commands done (%d commands done):",
-					have_done.nr),
-				have_done.nr);
+				Q_("Last command done (%"PRIuMAX" command done):",
+					"Last commands done (%"PRIuMAX" commands done):",
+					(unsigned long)have_done.nr),
+				(uintmax_t)have_done.nr);
 			for (i = (have_done.nr > nr_lines_to_show)
 				? have_done.nr - nr_lines_to_show : 0;
 				i < have_done.nr;
@@ -1393,10 +1393,10 @@ static void show_rebase_information(struct wt_status *s,
 					 _("No commands remaining."));
 		else {
 			status_printf_ln(s, color,
-				Q_("Next command to do (%d remaining command):",
-					"Next commands to do (%d remaining commands):",
-					yet_to_do.nr),
-				yet_to_do.nr);
+				Q_("Next command to do (%"PRIuMAX" remaining command):",
+					"Next commands to do (%"PRIuMAX" remaining commands):",
+					(unsigned long)yet_to_do.nr),
+				(uintmax_t)yet_to_do.nr);
 			for (i = 0; i < nr_lines_to_show && i < yet_to_do.nr; i++)
 				status_printf_ln(s, color, "   %s", yet_to_do.items[i].string);
 			if (s->hints)
-- 
2.35.1.1241.gd8d69a17521

Re: [PATCH 1/2] gettext API users: correct use of casts for Q_()

[Derrick Stolee]

On 3/7/2022 6:38 AM, Ævar Arnfjörð Bjarmason wrote:
> Change users of the inline gettext.h Q_() function to cast its
> argument to "unsigned long" instead of "int" or "unsigned int".
> 
> The ngettext() function (which Q_() resolves to) takes an "unsigned
> long int", and so does our Q_() wrapper for it, see 0c9ea33b90f (i18n:
> add stub Q_() wrapper for ngettext, 2011-03-09).
> 
> In a subsequent commit we'll be making more use of this pattern of:
> 
>     func(Q_(..%"PRIuMAX..., (unsigned long)x), (uintmax_t)x);
> 
> By making this change we ensure that this case isn't the odd one out
> in that post-image.


>  	if (!res)
> -		printf(Q_("updated %d path\n",
> -			  "updated %d paths\n", count), (int)count);
> +		printf(Q_("updated %"PRIuMAX" path\n",
> +			  "updated %"PRIuMAX" paths\n", (unsigned long)count),
> +		       (uintmax_t)count);

Why are we adding more uses of "unsigned long" which is not consistent
in its size across 64-bit Linux and 64-bit Windows? Specifically, on
Windows "unsigned long" is _not_ uintmax_t. Shouldn't we be using
uintmax_t everywhere instead?

Thanks,
-Stolee

Re: [PATCH 2/2] string-list API: change "nr" and "alloc" to "size_t"

[Derrick Stolee]

On 3/7/2022 6:38 AM, Ævar Arnfjörð Bjarmason wrote:
> Change the "nr" and "alloc" members of "struct string_list" to use
> "size_t" instead of "nr". On some platforms the size of an "unsigned
> int" will be smaller than a "size_t", e.g. a 32 bit unsigned v.s. 64
> bit unsigned. As "struct string_list" is a generic API we use in a lot
> of places this might cause overflows.
> 

>  			printf_ln(Q_("The bundle requires this ref:",
> -				     "The bundle requires these %d refs:",
> -				     r->nr),
> -				  r->nr);
> +				     "The bundle requires these %"PRIuMAX" refs:",
> +				     (unsigned long)r->nr),
> +				  (uintmax_t)r->nr);

There are more additions of unsigned long here, which will possibly
truncate the size_t of r->nr. I must be missing something here that
explains why you are making this choice.

Thanks,
-Stolee

Re: [PATCH 1/2] gettext API users: correct use of casts for Q_()

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 07 2022, Derrick Stolee wrote:

> On 3/7/2022 6:38 AM, Ævar Arnfjörð Bjarmason wrote:
>> Change users of the inline gettext.h Q_() function to cast its
>> argument to "unsigned long" instead of "int" or "unsigned int".
>> 
>> The ngettext() function (which Q_() resolves to) takes an "unsigned
>> long int", and so does our Q_() wrapper for it, see 0c9ea33b90f (i18n:
>> add stub Q_() wrapper for ngettext, 2011-03-09).
>> 
>> In a subsequent commit we'll be making more use of this pattern of:
>> 
>>     func(Q_(..%"PRIuMAX..., (unsigned long)x), (uintmax_t)x);
>> 
>> By making this change we ensure that this case isn't the odd one out
>> in that post-image.
>
>
>>  	if (!res)
>> -		printf(Q_("updated %d path\n",
>> -			  "updated %d paths\n", count), (int)count);
>> +		printf(Q_("updated %"PRIuMAX" path\n",
>> +			  "updated %"PRIuMAX" paths\n", (unsigned long)count),
>> +		       (uintmax_t)count);
>
> Why are we adding more uses of "unsigned long" which is not consistent
> in its size across 64-bit Linux and 64-bit Windows? Specifically, on
> Windows "unsigned long" is _not_ uintmax_t. Shouldn't we be using
> uintmax_t everywhere instead?

Whatever we do with "unsigned long" v.s. "size_t" or "uintmax_t" here
we'll need to call the ngettext() function, which takes "unsigned long".

Since you're quoting the part of the commit message that's explaining
that I'm not sure if you're meaning this as a suggestion that the
explanation should be clearer/more explicit, or just missed that
ngettext() isn't ours...

I did wonder if we should just skip the casts here and instead do:
	
	diff --git a/gettext.h b/gettext.h
	index d209911ebb8..095bf6b0e5e 100644
	--- a/gettext.h
	+++ b/gettext.h
	@@ -49,8 +49,10 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
	 }
	 
	 static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
	-const char *Q_(const char *msgid, const char *plu, unsigned long n)
	+const char *Q_(const char *msgid, const char *plu, size_t n)
	 {
	+	if (n > ULONG_MAX)
	+		n = ULONG_MAX;
	 	return ngettext(msgid, plu, n);
	 }

Which I suppose would be more correct than a cast, but the edge case
where we'd need that ULONG_MAX is so obscure that I don't think it's
worth worrying about it.

I think for this series it probably makes more sense to drop the casts
for the "n" argument entirely, what do you think?

Re: [PATCH 2/2] string-list API: change "nr" and "alloc" to "size_t"

[Ævar Arnfjörð Bjarmason]

On Mon, Mar 07 2022, Derrick Stolee wrote:

> On 3/7/2022 6:38 AM, Ævar Arnfjörð Bjarmason wrote:
>> Change the "nr" and "alloc" members of "struct string_list" to use
>> "size_t" instead of "nr". On some platforms the size of an "unsigned
>> int" will be smaller than a "size_t", e.g. a 32 bit unsigned v.s. 64
>> bit unsigned. As "struct string_list" is a generic API we use in a lot
>> of places this might cause overflows.
>> 
>
>>  			printf_ln(Q_("The bundle requires this ref:",
>> -				     "The bundle requires these %d refs:",
>> -				     r->nr),
>> -				  r->nr);
>> +				     "The bundle requires these %"PRIuMAX" refs:",
>> +				     (unsigned long)r->nr),
>> +				  (uintmax_t)r->nr);
>
> There are more additions of unsigned long here, which will possibly
> truncate the size_t of r->nr. I must be missing something here that
> explains why you are making this choice.

Replied to in the reply to your comment on 1/2.

[PATCH v2 0/2] string-list.h: make "nr" and "alloc" a "size_t"

[Ævar Arnfjörð Bjarmason]

Change the "struct string_list" to have a "size_t" for "nr" and
"alloc", ensuring we won't overflow on platforms where size_t is 64
bit, but "unsigned int" is 32 bit.

This replaces the v1 1/2 to get rid of the casts we use for Q_(), and
the 2/2's addition of casts is then consistent with those.

Ævar Arnfjörð Bjarmason (2):
  gettext API users: don't explicitly cast ngettext()'s "n"
  string-list API: change "nr" and "alloc" to "size_t"

 builtin/index-pack.c        |  2 +-
 builtin/merge-recursive.c   |  2 +-
 builtin/receive-pack.c      |  9 +++++----
 builtin/shortlog.c          | 10 +++++-----
 bundle.c                    |  8 ++++----
 commit-graph.c              |  6 +++---
 mailmap.c                   |  7 ++++---
 merge-ort.c                 |  4 ++--
 strbuf.c                    |  4 ++--
 string-list.h               |  3 ++-
 t/helper/test-run-command.c |  7 ++++---
 wt-status.c                 | 12 ++++++------
 12 files changed, 39 insertions(+), 35 deletions(-)

Range-diff against v1:
1:  83659fbc459 < -:  ----------- gettext API users: correct use of casts for Q_()
-:  ----------- > 1:  83fd21741ad gettext API users: don't explicitly cast ngettext()'s "n"
2:  398682c07aa ! 2:  6db8ab7a121 string-list API: change "nr" and "alloc" to "size_t"
    @@ Commit message
         two lines, which is the case for most such struct member
         declarations (e.g. in "strbuf.h" and "strvec.h").
     
    +    Changing e.g. "int i" to "size_t i" in run_and_feed_hook() isn't
    +    strictly necessary, and there are a lot more cases where we'll use a
    +    local "int", "unsigned int" etc. variable derived from the "nr" in the
    +    "struct string_list". But in that case as well as
    +    add_wrapped_shortlog_msg() in builtin/shortlog.c we need to adjust the
    +    printf format referring to "nr" anyway, so let's also change the other
    +    variables referring to it.
    +
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## builtin/receive-pack.c ##
    @@ bundle.c: int verify_bundle(struct repository *r,
      		r = &header->references;
      		printf_ln(Q_("The bundle contains this ref:",
     -			     "The bundle contains these %d refs:",
    --			     r->nr),
    --			  r->nr);
     +			     "The bundle contains these %"PRIuMAX" refs:",
    -+			     (unsigned long)r->nr),
    + 			     r->nr),
    +-			  r->nr);
     +			  (uintmax_t)r->nr);
      		list_refs(r, 0, NULL);
      		r = &header->prerequisites;
    @@ bundle.c: int verify_bundle(struct repository *r,
      		} else {
      			printf_ln(Q_("The bundle requires this ref:",
     -				     "The bundle requires these %d refs:",
    --				     r->nr),
    --				  r->nr);
     +				     "The bundle requires these %"PRIuMAX" refs:",
    -+				     (unsigned long)r->nr),
    + 				     r->nr),
    +-				  r->nr);
     +				  (uintmax_t)r->nr);
      			list_refs(r, 0, NULL);
      		}
    @@ commit-graph.c: static int fill_oids_from_packs(struct write_commit_graph_contex
      		strbuf_addf(&progress_title,
     -			    Q_("Finding commits for commit graph in %d pack",
     -			       "Finding commits for commit graph in %d packs",
    --			       pack_indexes->nr),
    --			    pack_indexes->nr);
     +			    Q_("Finding commits for commit graph in %"PRIuMAX" pack",
     +			       "Finding commits for commit graph in %"PRIuMAX" packs",
    -+			       (unsigned long)pack_indexes->nr),
    + 			       pack_indexes->nr),
    +-			    pack_indexes->nr);
     +			    (uintmax_t)pack_indexes->nr);
      		ctx->progress = start_delayed_progress(progress_title.buf, 0);
      		ctx->progress_done = 0;
    @@ wt-status.c: static void show_rebase_information(struct wt_status *s,
      			status_printf_ln(s, color,
     -				Q_("Last command done (%d command done):",
     -					"Last commands done (%d commands done):",
    --					have_done.nr),
    --				have_done.nr);
     +				Q_("Last command done (%"PRIuMAX" command done):",
     +					"Last commands done (%"PRIuMAX" commands done):",
    -+					(unsigned long)have_done.nr),
    + 					have_done.nr),
    +-				have_done.nr);
     +				(uintmax_t)have_done.nr);
      			for (i = (have_done.nr > nr_lines_to_show)
      				? have_done.nr - nr_lines_to_show : 0;
    @@ wt-status.c: static void show_rebase_information(struct wt_status *s,
      			status_printf_ln(s, color,
     -				Q_("Next command to do (%d remaining command):",
     -					"Next commands to do (%d remaining commands):",
    --					yet_to_do.nr),
    --				yet_to_do.nr);
     +				Q_("Next command to do (%"PRIuMAX" remaining command):",
     +					"Next commands to do (%"PRIuMAX" remaining commands):",
    -+					(unsigned long)yet_to_do.nr),
    + 					yet_to_do.nr),
    +-				yet_to_do.nr);
     +				(uintmax_t)yet_to_do.nr);
      			for (i = 0; i < nr_lines_to_show && i < yet_to_do.nr; i++)
      				status_printf_ln(s, color, "   %s", yet_to_do.items[i].string);
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 1/2] gettext API users: don't explicitly cast ngettext()'s "n"

[Ævar Arnfjörð Bjarmason]

Change a few stray users of the inline gettext.h Q_() function to stop
casting its "n" argument, the vast majority of the users of that
wrapper API use the implicit cast to "unsigned long".

The ngettext() function (which Q_() resolves to) takes an "unsigned
long int", and so does our Q_() wrapper for it, see 0c9ea33b90f (i18n:
add stub Q_() wrapper for ngettext, 2011-03-09). The function isn't
ours, but provided by e.g. GNU libintl.

This amends code added in added in 7171a0b0cf5 (index-pack: correct
"len" type in unpack_data(), 2016-07-13). The cast it added for the
printf format to die() was needed, but not the cast to Q_().

Likewise the casts in strbuf.c added in 8f354a1faed (l10n: localizable
upload progress messages, 2019-07-02) and for
builtin/merge-recursive.c in ccf7813139f (i18n: merge-recursive: mark
error messages for translation, 2016-09-15) weren't needed.

In the latter case the cast was copy/pasted from the argument to
warning() itself, added in b74d779bd90 (MinGW: Fix compiler warning in
merge-recursive, 2009-05-23). The cast for warning() is needed, but
not the one for ngettext()'s "n" argument.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/index-pack.c      | 2 +-
 builtin/merge-recursive.c | 2 +-
 strbuf.c                  | 4 ++--
 3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 3c2e6aee3cc..89339e83a0d 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -579,7 +579,7 @@ static void *unpack_data(struct object_entry *obj,
 		if (!n)
 			die(Q_("premature end of pack file, %"PRIuMAX" byte missing",
 			       "premature end of pack file, %"PRIuMAX" bytes missing",
-			       (unsigned int)len),
+			       len),
 			    (uintmax_t)len);
 		from += n;
 		len -= n;
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index a4bfd8fc51d..b9acbf5d342 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -58,7 +58,7 @@ int cmd_merge_recursive(int argc, const char **argv, const char *prefix)
 				   "Ignoring %s.",
 				   "cannot handle more than %d bases. "
 				   "Ignoring %s.",
-				    (int)ARRAY_SIZE(bases)-1),
+				    ARRAY_SIZE(bases)-1),
 				(int)ARRAY_SIZE(bases)-1, argv[i]);
 	}
 	if (argc - i != 3) /* "--" "<head>" "<remote>" */
diff --git a/strbuf.c b/strbuf.c
index 00abeb55afd..dd9eb85527a 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -875,9 +875,9 @@ static void strbuf_humanise(struct strbuf *buf, off_t bytes,
 		strbuf_addf(buf,
 				humanise_rate == 0 ?
 					/* TRANSLATORS: IEC 80000-13:2008 byte */
-					Q_("%u byte", "%u bytes", (unsigned)bytes) :
+					Q_("%u byte", "%u bytes", bytes) :
 					/* TRANSLATORS: IEC 80000-13:2008 byte/second */
-					Q_("%u byte/s", "%u bytes/s", (unsigned)bytes),
+					Q_("%u byte/s", "%u bytes/s", bytes),
 				(unsigned)bytes);
 	}
 }
-- 
2.35.1.1242.gfeba0eae32b

[PATCH v2 2/2] string-list API: change "nr" and "alloc" to "size_t"

[Ævar Arnfjörð Bjarmason]

Change the "nr" and "alloc" members of "struct string_list" to use
"size_t" instead of "nr". On some platforms the size of an "unsigned
int" will be smaller than a "size_t", e.g. a 32 bit unsigned v.s. 64
bit unsigned. As "struct string_list" is a generic API we use in a lot
of places this might cause overflows.

As one example: code in "refs.c" keeps track of the number of refs
with a "size_t", and auxiliary code in builtin/remote.c in
get_ref_states() appends those to a "struct string_list".

While we're at it split the "nr" and "alloc" in string-list.h across
two lines, which is the case for most such struct member
declarations (e.g. in "strbuf.h" and "strvec.h").

Changing e.g. "int i" to "size_t i" in run_and_feed_hook() isn't
strictly necessary, and there are a lot more cases where we'll use a
local "int", "unsigned int" etc. variable derived from the "nr" in the
"struct string_list". But in that case as well as
add_wrapped_shortlog_msg() in builtin/shortlog.c we need to adjust the
printf format referring to "nr" anyway, so let's also change the other
variables referring to it.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/receive-pack.c      |  9 +++++----
 builtin/shortlog.c          | 10 +++++-----
 bundle.c                    |  8 ++++----
 commit-graph.c              |  6 +++---
 mailmap.c                   |  7 ++++---
 merge-ort.c                 |  4 ++--
 string-list.h               |  3 ++-
 t/helper/test-run-command.c |  7 ++++---
 wt-status.c                 | 12 ++++++------
 9 files changed, 35 insertions(+), 31 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index d10aeb7e78f..fc948a27c4f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -813,13 +813,14 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
 	proc.trace2_hook_name = hook_name;
 
 	if (feed_state->push_options) {
-		int i;
+		size_t i;
 		for (i = 0; i < feed_state->push_options->nr; i++)
 			strvec_pushf(&proc.env_array,
-				     "GIT_PUSH_OPTION_%d=%s", i,
+				     "GIT_PUSH_OPTION_%"PRIuMAX"=%s",
+				     (uintmax_t)i,
 				     feed_state->push_options->items[i].string);
-		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%d",
-			     feed_state->push_options->nr);
+		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%"PRIuMAX"",
+			     (uintmax_t)feed_state->push_options->nr);
 	} else
 		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT");
 
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 228d782754a..26c5c0cf935 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -435,7 +435,7 @@ static void add_wrapped_shortlog_msg(struct strbuf *sb, const char *s,
 
 void shortlog_output(struct shortlog *log)
 {
-	int i, j;
+	size_t i, j;
 	struct strbuf sb = STRBUF_INIT;
 
 	if (log->sort_by_number)
@@ -448,10 +448,10 @@ void shortlog_output(struct shortlog *log)
 				(int)UTIL_TO_INT(item), item->string);
 		} else {
 			struct string_list *onelines = item->util;
-			fprintf(log->file, "%s (%d):\n",
-				item->string, onelines->nr);
-			for (j = onelines->nr - 1; j >= 0; j--) {
-				const char *msg = onelines->items[j].string;
+			fprintf(log->file, "%s (%"PRIuMAX"):\n",
+				item->string, (uintmax_t)onelines->nr);
+			for (j = onelines->nr; j >= 1; j--) {
+				const char *msg = onelines->items[j - 1].string;
 
 				if (log->wrap_lines) {
 					strbuf_reset(&sb);
diff --git a/bundle.c b/bundle.c
index a0bb687b0f4..7608701a51e 100644
--- a/bundle.c
+++ b/bundle.c
@@ -255,18 +255,18 @@ int verify_bundle(struct repository *r,
 
 		r = &header->references;
 		printf_ln(Q_("The bundle contains this ref:",
-			     "The bundle contains these %d refs:",
+			     "The bundle contains these %"PRIuMAX" refs:",
 			     r->nr),
-			  r->nr);
+			  (uintmax_t)r->nr);
 		list_refs(r, 0, NULL);
 		r = &header->prerequisites;
 		if (!r->nr) {
 			printf_ln(_("The bundle records a complete history."));
 		} else {
 			printf_ln(Q_("The bundle requires this ref:",
-				     "The bundle requires these %d refs:",
+				     "The bundle requires these %"PRIuMAX" refs:",
 				     r->nr),
-				  r->nr);
+				  (uintmax_t)r->nr);
 			list_refs(r, 0, NULL);
 		}
 	}
diff --git a/commit-graph.c b/commit-graph.c
index 265c010122e..e7731db8f40 100644
--- a/commit-graph.c
+++ b/commit-graph.c
@@ -1690,10 +1690,10 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
 	dirlen = packname.len;
 	if (ctx->report_progress) {
 		strbuf_addf(&progress_title,
-			    Q_("Finding commits for commit graph in %d pack",
-			       "Finding commits for commit graph in %d packs",
+			    Q_("Finding commits for commit graph in %"PRIuMAX" pack",
+			       "Finding commits for commit graph in %"PRIuMAX" packs",
 			       pack_indexes->nr),
-			    pack_indexes->nr);
+			    (uintmax_t)pack_indexes->nr);
 		ctx->progress = start_delayed_progress(progress_title.buf, 0);
 		ctx->progress_done = 0;
 	}
diff --git a/mailmap.c b/mailmap.c
index 40ce152024d..7befdc5e483 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -43,8 +43,8 @@ static void free_mailmap_info(void *p, const char *s)
 static void free_mailmap_entry(void *p, const char *s)
 {
 	struct mailmap_entry *me = (struct mailmap_entry *)p;
-	debug_mm("mailmap: removing entries for <%s>, with %d sub-entries\n",
-		 s, me->namemap.nr);
+	debug_mm("mailmap: removing entries for <%s>, with %"PRIuMAX" sub-entries\n",
+		 s, (uintmax_t)me->namemap.nr);
 	debug_mm("mailmap: - simple: '%s' <%s>\n",
 		 debug_str(me->name), debug_str(me->email));
 
@@ -250,7 +250,8 @@ int read_mailmap(struct string_list *map)
 
 void clear_mailmap(struct string_list *map)
 {
-	debug_mm("mailmap: clearing %d entries...\n", map->nr);
+	debug_mm("mailmap: clearing %"PRIuMAX" entries...\n",
+		 (uintmax_t)map->nr);
 	map->strdup_strings = 1;
 	string_list_clear_func(map, free_mailmap_entry);
 	debug_mm("mailmap: cleared\n");
diff --git a/merge-ort.c b/merge-ort.c
index 55decb2587e..bb2be2a2758 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -4064,8 +4064,8 @@ static void process_entries(struct merge_options *opt,
 	trace2_region_enter("merge", "process_entries cleanup", opt->repo);
 	if (dir_metadata.offsets.nr != 1 ||
 	    (uintptr_t)dir_metadata.offsets.items[0].util != 0) {
-		printf("dir_metadata.offsets.nr = %d (should be 1)\n",
-		       dir_metadata.offsets.nr);
+		printf("dir_metadata.offsets.nr = %"PRIuMAX" (should be 1)\n",
+		       (uintmax_t)dir_metadata.offsets.nr);
 		printf("dir_metadata.offsets.items[0].util = %u (should be 0)\n",
 		       (unsigned)(uintptr_t)dir_metadata.offsets.items[0].util);
 		fflush(stdout);
diff --git a/string-list.h b/string-list.h
index 267d6e5769d..d5a744e1438 100644
--- a/string-list.h
+++ b/string-list.h
@@ -86,7 +86,8 @@ typedef int (*compare_strings_fn)(const char *, const char *);
  */
 struct string_list {
 	struct string_list_item *items;
-	unsigned int nr, alloc;
+	size_t nr;
+	size_t alloc;
 	unsigned int strdup_strings:1;
 	compare_strings_fn cmp; /* NULL uses strcmp() */
 };
diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
index 913775a14b7..eabd28defc5 100644
--- a/t/helper/test-run-command.c
+++ b/t/helper/test-run-command.c
@@ -180,15 +180,16 @@ static int testsuite(int argc, const char **argv)
 	if (max_jobs > suite.tests.nr)
 		max_jobs = suite.tests.nr;
 
-	fprintf(stderr, "Running %d tests (%d at a time)\n",
-		suite.tests.nr, max_jobs);
+	fprintf(stderr, "Running %"PRIuMAX" tests (%d at a time)\n",
+		(uintmax_t)suite.tests.nr, max_jobs);
 
 	ret = run_processes_parallel(max_jobs, next_test, test_failed,
 				     test_finished, &suite);
 
 	if (suite.failed.nr > 0) {
 		ret = 1;
-		fprintf(stderr, "%d tests failed:\n\n", suite.failed.nr);
+		fprintf(stderr, "%"PRIuMAX" tests failed:\n\n",
+			(uintmax_t)suite.failed.nr);
 		for (i = 0; i < suite.failed.nr; i++)
 			fprintf(stderr, "\t%s\n", suite.failed.items[i].string);
 	}
diff --git a/wt-status.c b/wt-status.c
index 335e723a71e..bd54cf59894 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -1374,10 +1374,10 @@ static void show_rebase_information(struct wt_status *s,
 			status_printf_ln(s, color, _("No commands done."));
 		else {
 			status_printf_ln(s, color,
-				Q_("Last command done (%d command done):",
-					"Last commands done (%d commands done):",
+				Q_("Last command done (%"PRIuMAX" command done):",
+					"Last commands done (%"PRIuMAX" commands done):",
 					have_done.nr),
-				have_done.nr);
+				(uintmax_t)have_done.nr);
 			for (i = (have_done.nr > nr_lines_to_show)
 				? have_done.nr - nr_lines_to_show : 0;
 				i < have_done.nr;
@@ -1393,10 +1393,10 @@ static void show_rebase_information(struct wt_status *s,
 					 _("No commands remaining."));
 		else {
 			status_printf_ln(s, color,
-				Q_("Next command to do (%d remaining command):",
-					"Next commands to do (%d remaining commands):",
+				Q_("Next command to do (%"PRIuMAX" remaining command):",
+					"Next commands to do (%"PRIuMAX" remaining commands):",
 					yet_to_do.nr),
-				yet_to_do.nr);
+				(uintmax_t)yet_to_do.nr);
 			for (i = 0; i < nr_lines_to_show && i < yet_to_do.nr; i++)
 				status_printf_ln(s, color, "   %s", yet_to_do.items[i].string);
 			if (s->hints)
-- 
2.35.1.1242.gfeba0eae32b

Re: [PATCH 1/2] gettext API users: correct use of casts for Q_()

[Derrick Stolee]

On 3/7/2022 8:54 AM, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Mar 07 2022, Derrick Stolee wrote:
> 
>> On 3/7/2022 6:38 AM, Ævar Arnfjörð Bjarmason wrote:
>>> Change users of the inline gettext.h Q_() function to cast its
>>> argument to "unsigned long" instead of "int" or "unsigned int".
>>>
>>> The ngettext() function (which Q_() resolves to) takes an "unsigned
>>> long int", and so does our Q_() wrapper for it, see 0c9ea33b90f (i18n:
>>> add stub Q_() wrapper for ngettext, 2011-03-09).
>>>
>>> In a subsequent commit we'll be making more use of this pattern of:
>>>
>>>     func(Q_(..%"PRIuMAX..., (unsigned long)x), (uintmax_t)x);
>>>
>>> By making this change we ensure that this case isn't the odd one out
>>> in that post-image.
>>
>>
>>>  	if (!res)
>>> -		printf(Q_("updated %d path\n",
>>> -			  "updated %d paths\n", count), (int)count);
>>> +		printf(Q_("updated %"PRIuMAX" path\n",
>>> +			  "updated %"PRIuMAX" paths\n", (unsigned long)count),
>>> +		       (uintmax_t)count);
>>
>> Why are we adding more uses of "unsigned long" which is not consistent
>> in its size across 64-bit Linux and 64-bit Windows? Specifically, on
>> Windows "unsigned long" is _not_ uintmax_t. Shouldn't we be using
>> uintmax_t everywhere instead?
> 
> Whatever we do with "unsigned long" v.s. "size_t" or "uintmax_t" here
> we'll need to call the ngettext() function, which takes "unsigned long".
> 
> Since you're quoting the part of the commit message that's explaining
> that I'm not sure if you're meaning this as a suggestion that the
> explanation should be clearer/more explicit, or just missed that
> ngettext() isn't ours...

Mostly missed which parts we had control over or not.

> I did wonder if we should just skip the casts here and instead do:
> 	
> 	diff --git a/gettext.h b/gettext.h
> 	index d209911ebb8..095bf6b0e5e 100644
> 	--- a/gettext.h
> 	+++ b/gettext.h
> 	@@ -49,8 +49,10 @@ static inline FORMAT_PRESERVING(1) const char *_(const char *msgid)
> 	 }
> 	 
> 	 static inline FORMAT_PRESERVING(1) FORMAT_PRESERVING(2)
> 	-const char *Q_(const char *msgid, const char *plu, unsigned long n)
> 	+const char *Q_(const char *msgid, const char *plu, size_t n)
> 	 {
> 	+	if (n > ULONG_MAX)
> 	+		n = ULONG_MAX;
> 	 	return ngettext(msgid, plu, n);
> 	 }
> 
> Which I suppose would be more correct than a cast, but the edge case
> where we'd need that ULONG_MAX is so obscure that I don't think it's
> worth worrying about it.
> 
> I think for this series it probably makes more sense to drop the casts
> for the "n" argument entirely, what do you think?

I agree that this is a better approach. It avoids polluting all of
the callers with down-casts and instead puts it here. The only
difference is that we will truncate at ULONG_MAX instead of blindly
ignoring the significant bits, which is probably better.

Thanks,
-Stolee

Re: [PATCH v2 0/2] string-list.h: make "nr" and "alloc" a "size_t"

[Derrick Stolee]

On 3/7/2022 10:27 AM, Ævar Arnfjörð Bjarmason wrote:
> Change the "struct string_list" to have a "size_t" for "nr" and
> "alloc", ensuring we won't overflow on platforms where size_t is 64
> bit, but "unsigned int" is 32 bit.
> 
> This replaces the v1 1/2 to get rid of the casts we use for Q_(), and
> the 2/2's addition of casts is then consistent with those.

This version looks a lot better, localizing the need for understanding
cast implications into the implementation of Q_() instead of every
caller.

Thanks,
-Stolee

Re: [PATCH v2 2/2] string-list API: change "nr" and "alloc" to "size_t"

[Philip Oakley]

On 07/03/2022 15:27, Ævar Arnfjörð Bjarmason wrote:
> Change the "nr" and "alloc" members of "struct string_list" to use
> "size_t" instead of "nr". On some platforms the size of an "unsigned
> int" will be smaller than a "size_t", e.g. a 32 bit unsigned v.s. 64
> bit unsigned. As "struct string_list" is a generic API we use in a lot
> of places this might cause overflows.
>
> As one example: code in "refs.c" keeps track of the number of refs
> with a "size_t", and auxiliary code in builtin/remote.c in
> get_ref_states() appends those to a "struct string_list".
>
> While we're at it split the "nr" and "alloc" in string-list.h across
> two lines, which is the case for most such struct member
> declarations (e.g. in "strbuf.h" and "strvec.h").
>
> Changing e.g. "int i" to "size_t i" in run_and_feed_hook() isn't
> strictly necessary, and there are a lot more cases where we'll use a
> local "int", "unsigned int" etc. variable derived from the "nr" in the
> "struct string_list". But in that case as well as
> add_wrapped_shortlog_msg() in builtin/shortlog.c we need to adjust the
> printf format referring to "nr" anyway, so let's also change the other
> variables referring to it.
>
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>  builtin/receive-pack.c      |  9 +++++----
>  builtin/shortlog.c          | 10 +++++-----
>  bundle.c                    |  8 ++++----
>  commit-graph.c              |  6 +++---
>  mailmap.c                   |  7 ++++---
>  merge-ort.c                 |  4 ++--
>  string-list.h               |  3 ++-
>  t/helper/test-run-command.c |  7 ++++---
>  wt-status.c                 | 12 ++++++------
>  9 files changed, 35 insertions(+), 31 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index d10aeb7e78f..fc948a27c4f 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -813,13 +813,14 @@ static int run_and_feed_hook(const char *hook_name, feed_fn feed,
>  	proc.trace2_hook_name = hook_name;
>  
>  	if (feed_state->push_options) {
> -		int i;
> +		size_t i;
>  		for (i = 0; i < feed_state->push_options->nr; i++)
>  			strvec_pushf(&proc.env_array,
> -				     "GIT_PUSH_OPTION_%d=%s", i,
> +				     "GIT_PUSH_OPTION_%"PRIuMAX"=%s",
> +				     (uintmax_t)i,
>  				     feed_state->push_options->items[i].string);
> -		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%d",
> -			     feed_state->push_options->nr);
> +		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT=%"PRIuMAX"",
> +			     (uintmax_t)feed_state->push_options->nr);
>  	} else
>  		strvec_pushf(&proc.env_array, "GIT_PUSH_OPTION_COUNT");
>  
> diff --git a/builtin/shortlog.c b/builtin/shortlog.c
> index 228d782754a..26c5c0cf935 100644
> --- a/builtin/shortlog.c
> +++ b/builtin/shortlog.c
> @@ -435,7 +435,7 @@ static void add_wrapped_shortlog_msg(struct strbuf *sb, const char *s,
>  
>  void shortlog_output(struct shortlog *log)
>  {
> -	int i, j;
> +	size_t i, j;
>  	struct strbuf sb = STRBUF_INIT;
>  
>  	if (log->sort_by_number)
> @@ -448,10 +448,10 @@ void shortlog_output(struct shortlog *log)
>  				(int)UTIL_TO_INT(item), item->string);
>  		} else {
>  			struct string_list *onelines = item->util;
> -			fprintf(log->file, "%s (%d):\n",
> -				item->string, onelines->nr);
> -			for (j = onelines->nr - 1; j >= 0; j--) {
> -				const char *msg = onelines->items[j].string;
> +			fprintf(log->file, "%s (%"PRIuMAX"):\n",
> +				item->string, (uintmax_t)onelines->nr);
> +			for (j = onelines->nr; j >= 1; j--) {
> +				const char *msg = onelines->items[j - 1].string;
Does this change of iteration limits and j's offset  need a commit
message comment?

Philip

>  
>  				if (log->wrap_lines) {
>  					strbuf_reset(&sb);
> diff --git a/bundle.c b/bundle.c
> index a0bb687b0f4..7608701a51e 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -255,18 +255,18 @@ int verify_bundle(struct repository *r,
>  
>  		r = &header->references;
>  		printf_ln(Q_("The bundle contains this ref:",
> -			     "The bundle contains these %d refs:",
> +			     "The bundle contains these %"PRIuMAX" refs:",
>  			     r->nr),
> -			  r->nr);
> +			  (uintmax_t)r->nr);
>  		list_refs(r, 0, NULL);
>  		r = &header->prerequisites;
>  		if (!r->nr) {
>  			printf_ln(_("The bundle records a complete history."));
>  		} else {
>  			printf_ln(Q_("The bundle requires this ref:",
> -				     "The bundle requires these %d refs:",
> +				     "The bundle requires these %"PRIuMAX" refs:",
>  				     r->nr),
> -				  r->nr);
> +				  (uintmax_t)r->nr);
>  			list_refs(r, 0, NULL);
>  		}
>  	}
> diff --git a/commit-graph.c b/commit-graph.c
> index 265c010122e..e7731db8f40 100644
> --- a/commit-graph.c
> +++ b/commit-graph.c
> @@ -1690,10 +1690,10 @@ static int fill_oids_from_packs(struct write_commit_graph_context *ctx,
>  	dirlen = packname.len;
>  	if (ctx->report_progress) {
>  		strbuf_addf(&progress_title,
> -			    Q_("Finding commits for commit graph in %d pack",
> -			       "Finding commits for commit graph in %d packs",
> +			    Q_("Finding commits for commit graph in %"PRIuMAX" pack",
> +			       "Finding commits for commit graph in %"PRIuMAX" packs",
>  			       pack_indexes->nr),
> -			    pack_indexes->nr);
> +			    (uintmax_t)pack_indexes->nr);
>  		ctx->progress = start_delayed_progress(progress_title.buf, 0);
>  		ctx->progress_done = 0;
>  	}
> diff --git a/mailmap.c b/mailmap.c
> index 40ce152024d..7befdc5e483 100644
> --- a/mailmap.c
> +++ b/mailmap.c
> @@ -43,8 +43,8 @@ static void free_mailmap_info(void *p, const char *s)
>  static void free_mailmap_entry(void *p, const char *s)
>  {
>  	struct mailmap_entry *me = (struct mailmap_entry *)p;
> -	debug_mm("mailmap: removing entries for <%s>, with %d sub-entries\n",
> -		 s, me->namemap.nr);
> +	debug_mm("mailmap: removing entries for <%s>, with %"PRIuMAX" sub-entries\n",
> +		 s, (uintmax_t)me->namemap.nr);
>  	debug_mm("mailmap: - simple: '%s' <%s>\n",
>  		 debug_str(me->name), debug_str(me->email));
>  
> @@ -250,7 +250,8 @@ int read_mailmap(struct string_list *map)
>  
>  void clear_mailmap(struct string_list *map)
>  {
> -	debug_mm("mailmap: clearing %d entries...\n", map->nr);
> +	debug_mm("mailmap: clearing %"PRIuMAX" entries...\n",
> +		 (uintmax_t)map->nr);
>  	map->strdup_strings = 1;
>  	string_list_clear_func(map, free_mailmap_entry);
>  	debug_mm("mailmap: cleared\n");
> diff --git a/merge-ort.c b/merge-ort.c
> index 55decb2587e..bb2be2a2758 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -4064,8 +4064,8 @@ static void process_entries(struct merge_options *opt,
>  	trace2_region_enter("merge", "process_entries cleanup", opt->repo);
>  	if (dir_metadata.offsets.nr != 1 ||
>  	    (uintptr_t)dir_metadata.offsets.items[0].util != 0) {
> -		printf("dir_metadata.offsets.nr = %d (should be 1)\n",
> -		       dir_metadata.offsets.nr);
> +		printf("dir_metadata.offsets.nr = %"PRIuMAX" (should be 1)\n",
> +		       (uintmax_t)dir_metadata.offsets.nr);
>  		printf("dir_metadata.offsets.items[0].util = %u (should be 0)\n",
>  		       (unsigned)(uintptr_t)dir_metadata.offsets.items[0].util);
>  		fflush(stdout);
> diff --git a/string-list.h b/string-list.h
> index 267d6e5769d..d5a744e1438 100644
> --- a/string-list.h
> +++ b/string-list.h
> @@ -86,7 +86,8 @@ typedef int (*compare_strings_fn)(const char *, const char *);
>   */
>  struct string_list {
>  	struct string_list_item *items;
> -	unsigned int nr, alloc;
> +	size_t nr;
> +	size_t alloc;
>  	unsigned int strdup_strings:1;
>  	compare_strings_fn cmp; /* NULL uses strcmp() */
>  };
> diff --git a/t/helper/test-run-command.c b/t/helper/test-run-command.c
> index 913775a14b7..eabd28defc5 100644
> --- a/t/helper/test-run-command.c
> +++ b/t/helper/test-run-command.c
> @@ -180,15 +180,16 @@ static int testsuite(int argc, const char **argv)
>  	if (max_jobs > suite.tests.nr)
>  		max_jobs = suite.tests.nr;
>  
> -	fprintf(stderr, "Running %d tests (%d at a time)\n",
> -		suite.tests.nr, max_jobs);
> +	fprintf(stderr, "Running %"PRIuMAX" tests (%d at a time)\n",
> +		(uintmax_t)suite.tests.nr, max_jobs);
>  
>  	ret = run_processes_parallel(max_jobs, next_test, test_failed,
>  				     test_finished, &suite);
>  
>  	if (suite.failed.nr > 0) {
>  		ret = 1;
> -		fprintf(stderr, "%d tests failed:\n\n", suite.failed.nr);
> +		fprintf(stderr, "%"PRIuMAX" tests failed:\n\n",
> +			(uintmax_t)suite.failed.nr);
>  		for (i = 0; i < suite.failed.nr; i++)
>  			fprintf(stderr, "\t%s\n", suite.failed.items[i].string);
>  	}
> diff --git a/wt-status.c b/wt-status.c
> index 335e723a71e..bd54cf59894 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -1374,10 +1374,10 @@ static void show_rebase_information(struct wt_status *s,
>  			status_printf_ln(s, color, _("No commands done."));
>  		else {
>  			status_printf_ln(s, color,
> -				Q_("Last command done (%d command done):",
> -					"Last commands done (%d commands done):",
> +				Q_("Last command done (%"PRIuMAX" command done):",
> +					"Last commands done (%"PRIuMAX" commands done):",
>  					have_done.nr),
> -				have_done.nr);
> +				(uintmax_t)have_done.nr);
>  			for (i = (have_done.nr > nr_lines_to_show)
>  				? have_done.nr - nr_lines_to_show : 0;
>  				i < have_done.nr;
> @@ -1393,10 +1393,10 @@ static void show_rebase_information(struct wt_status *s,
>  					 _("No commands remaining."));
>  		else {
>  			status_printf_ln(s, color,
> -				Q_("Next command to do (%d remaining command):",
> -					"Next commands to do (%d remaining commands):",
> +				Q_("Next command to do (%"PRIuMAX" remaining command):",
> +					"Next commands to do (%"PRIuMAX" remaining commands):",
>  					yet_to_do.nr),
> -				yet_to_do.nr);
> +				(uintmax_t)yet_to_do.nr);
>  			for (i = 0; i < nr_lines_to_show && i < yet_to_do.nr; i++)
>  				status_printf_ln(s, color, "   %s", yet_to_do.items[i].string);
>  			if (s->hints)

Re: [PATCH v2 2/2] string-list API: change "nr" and "alloc" to "size_t"

[Junio C Hamano]

Philip Oakley <philipoakley@iee.email> writes:

>> @@ -448,10 +448,10 @@ void shortlog_output(struct shortlog *log)
>>  				(int)UTIL_TO_INT(item), item->string);
>>  		} else {
>>  			struct string_list *onelines = item->util;
>> -			fprintf(log->file, "%s (%d):\n",
>> -				item->string, onelines->nr);
>> -			for (j = onelines->nr - 1; j >= 0; j--) {
>> -				const char *msg = onelines->items[j].string;
>> +			fprintf(log->file, "%s (%"PRIuMAX"):\n",
>> +				item->string, (uintmax_t)onelines->nr);
>> +			for (j = onelines->nr; j >= 1; j--) {
>> +				const char *msg = onelines->items[j - 1].string;
> Does this change of iteration limits and j's offset need a commit
> message comment?

Good eyes.  I also tried to made sure this is the only loop that
counts toward zero that use a variable whose type has become size_t
with this patch, which is unsigned and cannot use "var >= 0" as the
loop termination condition, but what is not in the patch is hard to
know.  If there were loops that used to count down with the .nr or
the .alloc members of string_list structure, they would need to be
updated, but if such a code exists, it is likely a bug already.

I think many internal index string-list.c uses are still "int"
(cf. get_entry_index() and functions on the call graph that leads to
it, like add_entry() and string_list_remove()) that wants to be
updated to "ssize_t"; if the string_list has very many elements,
add_entry() may not find an existing string and add a duplicate at
the end, because the bisection search get_entry_index() uses cannot
move the upper-bound pointer beyond "int".

It would have been better if you culled the parts of the patch you
are not referring to, by the way.

Thanks.

Re: [PATCH v2 2/2] string-list API: change "nr" and "alloc" to "size_t"

[Philip Oakley]

On 07/03/2022 20:43, Junio C Hamano wrote:
> It would have been better if you culled the parts of the patch you
> are not referring to, by the way.
Oops sorry, I'm not that great at deciding which parts to keep/cull to
ensure sufficient context for other readers - it wasn't something that
was desired when I was working (too many 'Outlook' influences).

I'll work on it.

Philip