threads / patch / 57514

patch, 2 partsgettext API users: correct use of casts for Q_()

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

## tl;dr

15 messages between Mar 7, 2022 and Mar 7, 2022. Diffs are folded; open one to read it.

replies: 14people: 4as markdown or json

Ævar Arnfjörð Bjarmason· Mar 7, 2022, 11:37 UTC · lore

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

$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
Ævar Arnfjörð Bjarmason· Mar 7, 2022, 11:38 UTC · re: Ævar Arnfjörð Bjarmason · lore

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(-)
Show changes to 3 files +14 −11

add-interactive.c, add-patch.c, builtin/index-pack.c

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
Derrick Stolee· Mar 7, 2022, 13:41 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 3/7/2022 6:38 AM, Ævar Arnfjörð Bjarmason wrote:
Show 13 quoted lines
> 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.
Show 6 quoted lines
>  	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

Ævar Arnfjörð Bjarmason· Mar 7, 2022, 13:54 UTC · re: Derrick Stolee · lore

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

On Mon, Mar 07 2022, Derrick Stolee wrote:
Show 27 quoted lines
> 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?

Derrick Stolee· Mar 7, 2022, 15:53 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 3/7/2022 8:54 AM, Ævar Arnfjörð Bjarmason wrote:
Show 38 quoted lines
> 
> 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.
Show 24 quoted lines
> 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

Ævar Arnfjörð Bjarmason· Mar 7, 2022, 11:38 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 9 files +40 −36

builtin/receive-pack.c, builtin/shortlog.c, bundle.c, commit-graph.c, mailmap.c, merge-ort.c, string-list.h, t/helper/test-run-command.c, wt-status.c

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
Derrick Stolee· Mar 7, 2022, 13:43 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 3/7/2022 6:38 AM, Ævar Arnfjörð Bjarmason wrote:
Show 6 quoted lines
> 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.
> 
Show 7 quoted lines
>  			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

Ævar Arnfjörð Bjarmason· Mar 7, 2022, 14:10 UTC · re: Derrick Stolee · lore

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

On Mon, Mar 07 2022, Derrick Stolee wrote:
Show 19 quoted lines
> 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.
Ævar Arnfjörð Bjarmason· Mar 7, 2022, 15:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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
Ævar Arnfjörð Bjarmason· Mar 7, 2022, 15:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 3 files +4 −4

builtin/index-pack.c, builtin/merge-recursive.c, strbuf.c

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
Ævar Arnfjörð Bjarmason· Mar 7, 2022, 15:27 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

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(-)
Show changes to 9 files +35 −31

builtin/receive-pack.c, builtin/shortlog.c, bundle.c, commit-graph.c, mailmap.c, merge-ort.c, string-list.h, t/helper/test-run-command.c, wt-status.c

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
Philip Oakley· Mar 7, 2022, 16:23 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 07/03/2022 15:27, Ævar Arnfjörð Bjarmason wrote:
Show 83 quoted lines
> 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
Show 158 quoted lines
>  
>  				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)
Junio C Hamano· Mar 7, 2022, 20:43 UTC · re: Philip Oakley · lore

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

Philip Oakley <philipoakley@iee.email> writes:
Show 14 quoted lines
>> @@ -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.
Philip Oakley· Mar 7, 2022, 23:34 UTC · re: Junio C Hamano · lore

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

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
Derrick Stolee· Mar 7, 2022, 15:55 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On 3/7/2022 10:27 AM, Ævar Arnfjörð Bjarmason wrote:
Show 6 quoted lines
> 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

← back to recent threads