[PATCH 0/4] Fixes typemissmatch warinigs from msvc

[PATCH 0/4] Fixes typemissmatch warinigs from msvc

[Sören Krecker]

A smale series of patches to fix some typemissmatch warings from msvc 14.30.
Most of the missmatches a 64 to 32 bit conversion on a 64 bit Windows platform.

I use size_t where the variable values cannot become negative.

Sören Krecker (4):
  add-patch: Fix type missmatch rom msvc
  date.c: Fix type missmatch warings from msvc
  apply.c : Fix type missmatch warings from msvc
  commit.c: Fix type missmatch warings from msvc

 add-patch.c | 44 +++++++++++++++++++++++++-------------------
 apply.c     | 37 +++++++++++++++++++------------------
 apply.h     |  6 +++---
 commit.c    | 10 +++++-----
 date.c      |  6 +++---
 gettext.h   |  2 +-
 6 files changed, 56 insertions(+), 49 deletions(-)


base-commit: ff795a5c5ed2e2d07c688c217a615d89e3f5733b
-- 
2.39.5

Thanks

Sören Krecker

[PATCH 1/4] add-patch: Fix type missmatch rom msvc

[Sören Krecker]

Fix some compiler warings from msvw in add-patch.c for value truncation
form 64 bit to 32 bit integers.Change unsigned long to size_t for
correct variable size on linux and windows

Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
 add-patch.c | 44 +++++++++++++++++++++++++-------------------
 gettext.h   |  2 +-
 2 files changed, 26 insertions(+), 20 deletions(-)

diff --git a/add-patch.c b/add-patch.c
index 557903310d..1ea70ef988 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -241,7 +241,7 @@ static struct patch_mode patch_mode_worktree_nothead = {
 };
 
 struct hunk_header {
-	unsigned long old_offset, old_count, new_offset, new_count;
+	size_t old_offset, old_count, new_offset, new_count;
 	/*
 	 * Start/end offsets to the extra text after the second `@@` in the
 	 * hunk header, e.g. the function signature. This is expected to
@@ -321,7 +321,7 @@ static void setup_child_process(struct add_p_state *s,
 }
 
 static int parse_range(const char **p,
-		       unsigned long *offset, unsigned long *count)
+		       size_t *offset, size_t *count)
 {
 	char *pend;
 
@@ -672,8 +672,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		 */
 		const char *p;
 		size_t len;
-		unsigned long old_offset = header->old_offset;
-		unsigned long new_offset = header->new_offset;
+		size_t old_offset = header->old_offset;
+		size_t new_offset = header->new_offset;
 
 		if (!colored) {
 			p = s->plain.buf + header->extra_start;
@@ -699,12 +699,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
 		else
 			new_offset += delta;
 
-		strbuf_addf(out, "@@ -%lu", old_offset);
+		strbuf_addf(out, "@@ -%" PRIuMAX, (uintmax_t)old_offset);
 		if (header->old_count != 1)
-			strbuf_addf(out, ",%lu", header->old_count);
-		strbuf_addf(out, " +%lu", new_offset);
+			strbuf_addf(out, ",%" PRIuMAX,
+				    (uintmax_t)header->old_count);
+		strbuf_addf(out, " +%" PRIuMAX, (uintmax_t)new_offset);
 		if (header->new_count != 1)
-			strbuf_addf(out, ",%lu", header->new_count);
+			strbuf_addf(out, ",%" PRIuMAX,
+				    (uintmax_t)header->new_count);
 		strbuf_addstr(out, " @@");
 
 		if (len)
@@ -1065,11 +1067,13 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
 
 	/* last hunk simply gets the rest */
 	if (header->old_offset != remaining.old_offset)
-		BUG("miscounted old_offset: %lu != %lu",
-		    header->old_offset, remaining.old_offset);
+		BUG("miscounted old_offset: %"PRIuMAX" != %"PRIuMAX,
+		    (uintmax_t)header->old_offset,
+		    (uintmax_t)remaining.old_offset);
 	if (header->new_offset != remaining.new_offset)
-		BUG("miscounted new_offset: %lu != %lu",
-		    header->new_offset, remaining.new_offset);
+		BUG("miscounted new_offset: %"PRIuMAX" != %"PRIuMAX,
+		    (uintmax_t)header->new_offset,
+		    (uintmax_t)remaining.new_offset);
 	header->old_count = remaining.old_count;
 	header->new_count = remaining.new_count;
 	hunk->end = end;
@@ -1353,9 +1357,10 @@ static void summarize_hunk(struct add_p_state *s, struct hunk *hunk,
 	struct strbuf *plain = &s->plain;
 	size_t len = out->len, i;
 
-	strbuf_addf(out, " -%lu,%lu +%lu,%lu ",
-		    header->old_offset, header->old_count,
-		    header->new_offset, header->new_count);
+	strbuf_addf(out,
+		    " -%"PRIuMAX",%"PRIuMAX" +%"PRIuMAX",%"PRIuMAX" ",
+		    (uintmax_t)header->old_offset, (uintmax_t)header->old_count,
+		    (uintmax_t)header->new_offset, (uintmax_t)header->new_count);
 	if (out->len - len < SUMMARY_HEADER_WIDTH)
 		strbuf_addchars(out, ' ',
 				SUMMARY_HEADER_WIDTH + len - out->len);
@@ -1624,10 +1629,11 @@ 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.",
+				       (uintmax_t)file_diff->hunk_nr),
+				    (uintmax_t)file_diff->hunk_nr);
 		} else if (s->answer.buf[0] == '/') {
 			regex_t regex;
 			int ret;
diff --git a/gettext.h b/gettext.h
index 484cafa562..d36f5a7ade 100644
--- a/gettext.h
+++ b/gettext.h
@@ -53,7 +53,7 @@ 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 (!git_gettext_enabled)
 		return n == 1 ? msgid : plu;
-- 
2.39.5

Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc

[Junio C Hamano]

Sören Krecker <soekkle@freenet.de> writes:

> Fix some compiler warings from msvw in add-patch.c for value truncation
> form 64 bit to 32 bit integers.Change unsigned long to size_t for
> correct variable size on linux and windows
>
> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
>  add-patch.c | 44 +++++++++++++++++++++++++-------------------
>  gettext.h   |  2 +-
>  2 files changed, 26 insertions(+), 20 deletions(-)



>  struct hunk_header {
> -	unsigned long old_offset, old_count, new_offset, new_count;
> +	size_t old_offset, old_count, new_offset, new_count;

These are not "size"s in the traditional sense of what size_t is
(i.e. the number of bytes in a region of memory), but are more or
less proportional to that in that they count in number of lines.

If ulong is sufficient to count number of lines in an incoming
patch, then turning size_t may be excessive---are we sure that we
are not unnecessarily using wider-than-necessary size_t in some
places to hold these values for which ulong is sufficient, causing
compilers to emit unnecessary warning?

IOW, if we have variables of unsigned integer of various sizes, we
_could_ rewrite all of them to use uintmax_t and there won't be
truncation-upon-assignment warnings from a compiler, but such a
rewrite can be pointless.  We'd need to find where we stuff these
values, which are originally ulong, to size_t, and see if the use of
size_t is really sensible.

> @@ -321,7 +321,7 @@ static void setup_child_process(struct add_p_state *s,
>  }
>  
>  static int parse_range(const char **p,
> -		       unsigned long *offset, unsigned long *count)
> +		       size_t *offset, size_t *count)
>  {
>  	char *pend;
>  
> @@ -672,8 +672,8 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
>  		 */
>  		const char *p;
>  		size_t len;
> -		unsigned long old_offset = header->old_offset;
> -		unsigned long new_offset = header->new_offset;
> +		size_t old_offset = header->old_offset;
> +		size_t new_offset = header->new_offset;
>  
>  		if (!colored) {
>  			p = s->plain.buf + header->extra_start;
> @@ -699,12 +699,14 @@ static void render_hunk(struct add_p_state *s, struct hunk *hunk,
>  		else
>  			new_offset += delta;
>  
> -		strbuf_addf(out, "@@ -%lu", old_offset);
> +		strbuf_addf(out, "@@ -%" PRIuMAX, (uintmax_t)old_offset);
>  		if (header->old_count != 1)
> -			strbuf_addf(out, ",%lu", header->old_count);
> -		strbuf_addf(out, " +%lu", new_offset);
> +			strbuf_addf(out, ",%" PRIuMAX,
> +				    (uintmax_t)header->old_count);
> +		strbuf_addf(out, " +%" PRIuMAX, (uintmax_t)new_offset);
>  		if (header->new_count != 1)
> -			strbuf_addf(out, ",%lu", header->new_count);
> +			strbuf_addf(out, ",%" PRIuMAX,
> +				    (uintmax_t)header->new_count);
>  		strbuf_addstr(out, " @@");
>  
>  		if (len)

So far if we left the types of *_offset and count all ulong, I do
not see anything that causes trunation-upon-assignment.

> @@ -1065,11 +1067,13 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
>  
>  	/* last hunk simply gets the rest */
>  	if (header->old_offset != remaining.old_offset)
> -		BUG("miscounted old_offset: %lu != %lu",
> -		    header->old_offset, remaining.old_offset);
> +		BUG("miscounted old_offset: %"PRIuMAX" != %"PRIuMAX,
> +		    (uintmax_t)header->old_offset,
> +		    (uintmax_t)remaining.old_offset);
>  	if (header->new_offset != remaining.new_offset)
> -		BUG("miscounted new_offset: %lu != %lu",
> -		    header->new_offset, remaining.new_offset);
> +		BUG("miscounted new_offset: %"PRIuMAX" != %"PRIuMAX,
> +		    (uintmax_t)header->new_offset,
> +		    (uintmax_t)remaining.new_offset);
>  	header->old_count = remaining.old_count;
>  	header->new_count = remaining.new_count;
>  	hunk->end = end;

This hunk unfortunately may be needed because this function somehow
decided to use size_t for things like "hunk_index" and "end" when it
was added, even though the surrounding functions it interacts with
all used ulong.

> @@ -1353,9 +1357,10 @@ static void summarize_hunk(struct add_p_state *s, struct hunk *hunk,
>  	struct strbuf *plain = &s->plain;
>  	size_t len = out->len, i;
>  
> -	strbuf_addf(out, " -%lu,%lu +%lu,%lu ",
> -		    header->old_offset, header->old_count,
> -		    header->new_offset, header->new_count);
> +	strbuf_addf(out,
> +		    " -%"PRIuMAX",%"PRIuMAX" +%"PRIuMAX",%"PRIuMAX" ",
> +		    (uintmax_t)header->old_offset, (uintmax_t)header->old_count,
> +		    (uintmax_t)header->new_offset, (uintmax_t)header->new_count);
>  	if (out->len - len < SUMMARY_HEADER_WIDTH)
>  		strbuf_addchars(out, ' ',
>  				SUMMARY_HEADER_WIDTH + len - out->len);

Again, if we left the types of *_offset and count all ulong, I do
not see anything that causes trunation-upon-assignment.

> @@ -1624,10 +1629,11 @@ 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.",
> +				       (uintmax_t)file_diff->hunk_nr),
> +				    (uintmax_t)file_diff->hunk_nr);

Again, this hunk may be needed, as the "hunk_nr" uses size_t, which
probably is overly wide.

So, I do not mind too much to adjust the code around hunk_nr,
hunk_alloc and other things that are already size_t (but before
doing so, we probably should see if it makes more sense to use ulong
for these members instead of size_t), hbut I am not sure if it is a
sensible move to change old_offset, count, etc. that count in number
of lines and use ulong (not bytes) to use size_t instead.

size_t _might_ be wider than some other forms of unsigned integers,
but it is not necessarily the widest, so "because on msvc ulong is
merely 32-bit and I want wider integer like everybody else!" is not
a good excuse for such a change (if it were, we'd all coding with
nothing but uintmax_t).

So I dunno.

> diff --git a/gettext.h b/gettext.h
> index 484cafa562..d36f5a7ade 100644
> --- a/gettext.h
> +++ b/gettext.h
> @@ -53,7 +53,7 @@ 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 (!git_gettext_enabled)
>  		return n == 1 ? msgid : plu;

Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc

[Patrick Steinhardt]

On Thu, Dec 26, 2024 at 01:33:12PM -0800, Junio C Hamano wrote:
> Sören Krecker <soekkle@freenet.de> writes:
> > @@ -1624,10 +1629,11 @@ 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.",
> > +				       (uintmax_t)file_diff->hunk_nr),
> > +				    (uintmax_t)file_diff->hunk_nr);
> 
> Again, this hunk may be needed, as the "hunk_nr" uses size_t, which
> probably is overly wide.
> 
> So, I do not mind too much to adjust the code around hunk_nr,
> hunk_alloc and other things that are already size_t (but before
> doing so, we probably should see if it makes more sense to use ulong
> for these members instead of size_t), hbut I am not sure if it is a
> sensible move to change old_offset, count, etc. that count in number
> of lines and use ulong (not bytes) to use size_t instead.
> 
> size_t _might_ be wider than some other forms of unsigned integers,
> but it is not necessarily the widest, so "because on msvc ulong is
> merely 32-bit and I want wider integer like everybody else!" is not
> a good excuse for such a change (if it were, we'd all coding with
> nothing but uintmax_t).
> 
> So I dunno.

In practice, the number of bytes and number of lines _can_ be the same
when the file consists of newlines, only. That is of course unlikely to
be the case in any sane input, but may be the case when processing input
that was specifically crafted to trigger such an edge case. So using a
type that can store the maximum size of a theoretically possible object
feels like a sensible safeguard to me and requires us to worry less
about such weird edge cases.

I also doubt that widening the type to `size_t` would have a meaningful
impact on performance, so I don't see a strong reason not to go there.
I tend to think that using `size_t` in such size-like-fields should be
our default unless there is a good reason not to pick it.

Patrick

Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc

[Phillip Wood]

On 26/12/2024 21:33, Junio C Hamano wrote:
> Sören Krecker <soekkle@freenet.de> writes:
> 
>> Fix some compiler warings from msvw in add-patch.c for value truncation
>> form 64 bit to 32 bit integers.Change unsigned long to size_t for
>> correct variable size on linux and windows
>>
>> Signed-off-by: Sören Krecker <soekkle@freenet.de>
>> ---
>>   add-patch.c | 44 +++++++++++++++++++++++++-------------------
>>   gettext.h   |  2 +-
>>   2 files changed, 26 insertions(+), 20 deletions(-)
> 
> 
> 
>>   struct hunk_header {
>> -	unsigned long old_offset, old_count, new_offset, new_count;
>> +	size_t old_offset, old_count, new_offset, new_count;
> 
> These are not "size"s in the traditional sense of what size_t is
> (i.e. the number of bytes in a region of memory), but are more or
> less proportional to that in that they count in number of lines.
> 
> If ulong is sufficient to count number of lines in an incoming
> patch, then turning size_t may be excessive---are we sure that we
> are not unnecessarily using wider-than-necessary size_t in some
> places to hold these values for which ulong is sufficient, causing
> compilers to emit unnecessary warning?

That's my thought too - I think something like the diff below should
fix the warnings by using more appropriate types in expressions
involving the hunk header offset and count. Our internal diff
implementation will not generate diffs for blobs greater than ~1GB
and I don't think "git apply" can handle diff headers that contain
numbers greater that ULONG_MAX so switching to size_t here seems
unnecessary.

Best Wishes

Phillip

---- >8 ----

diff --git a/add-patch.c b/add-patch.c
index 557903310de..2c439b83665 100644
--- a/add-patch.c
+++ b/add-patch.c
@@ -253,7 +253,7 @@ struct hunk_header {
  
  struct hunk {
  	size_t start, end, colored_start, colored_end, splittable_into;
-	ssize_t delta;
+	long delta;
  	enum { UNDECIDED_HUNK = 0, SKIP_HUNK, USE_HUNK } use;
  	struct hunk_header header;
  };
@@ -760,7 +760,8 @@ static void render_diff_header(struct add_p_state *s,
  static int merge_hunks(struct add_p_state *s, struct file_diff *file_diff,
  		       size_t *hunk_index, int use_all, struct hunk *merged)
  {
-	size_t i = *hunk_index, delta;
+	size_t i = *hunk_index;
+	long delta;
  	struct hunk *hunk = file_diff->hunk + i;
  	/* `header` corresponds to the merged hunk */
  	struct hunk_header *header = &merged->header, *next;
@@ -890,7 +891,7 @@ static void reassemble_patch(struct add_p_state *s,
  {
  	struct hunk *hunk;
  	size_t save_len = s->plain.len, i;
-	ssize_t delta = 0;
+	long delta = 0;
  
  	render_diff_header(s, file_diff, 0, out);
  
@@ -926,7 +927,8 @@ static int split_hunk(struct add_p_state *s, struct file_diff *file_diff,
  	int colored = !!s->colored.len, first = 1;
  	struct hunk *hunk = file_diff->hunk + hunk_index;
  	size_t splittable_into;
-	size_t end, colored_end, current, colored_current = 0, context_line_count;
+	size_t end, colored_end, current, colored_current = 0;
+	unsigned long context_line_count;
  	struct hunk_header remaining, *header;
  	char marker, ch;
  
@@ -1175,8 +1177,8 @@ static int edit_hunk_manually(struct add_p_state *s, struct hunk *hunk)
  	return 1;
  }
  
-static ssize_t recount_edited_hunk(struct add_p_state *s, struct hunk *hunk,
-				   size_t orig_old_count, size_t orig_new_count)
+static long recount_edited_hunk(struct add_p_state *s, struct hunk *hunk,
+				 unsigned long orig_old_count, unsigned long orig_new_count)
  {
  	struct hunk_header *header = &hunk->header;
  	size_t i;
@@ -1626,7 +1628,7 @@ static int patch_update_file(struct add_p_state *s,
  			else
  				err(s, Q_("Sorry, only %d hunk available.",
  					  "Sorry, only %d hunks available.",
-					  file_diff->hunk_nr),
+					  (int)file_diff->hunk_nr),
  				    (int)file_diff->hunk_nr);
  		} else if (s->answer.buf[0] == '/') {
  			regex_t regex;

Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> writes:

>>>   struct hunk_header {
>>> -	unsigned long old_offset, old_count, new_offset, new_count;
>>> +	size_t old_offset, old_count, new_offset, new_count;
>> These are not "size"s in the traditional sense of what size_t is
>> (i.e. the number of bytes in a region of memory), but are more or
>> less proportional to that in that they count in number of lines.
>> If ulong is sufficient to count number of lines in an incoming
>> patch, then turning size_t may be excessive---are we sure that we
>> are not unnecessarily using wider-than-necessary size_t in some
>> places to hold these values for which ulong is sufficient, causing
>> compilers to emit unnecessary warning?
>
> That's my thought too - I think something like the diff below should
> fix the warnings by using more appropriate types in expressions
> involving the hunk header offset and count. Our internal diff
> implementation will not generate diffs for blobs greater than ~1GB
> and I don't think "git apply" can handle diff headers that contain
> numbers greater that ULONG_MAX so switching to size_t here seems
> unnecessary.

Yes, exactly.

Of course, when filling old_offset and friends by parsing an input
line like this:

    @@ -253,7 +253,7 @@ struct hunk_header {

it would be a bug if we did not check if "253" overflows the type of
old_offset, etc.  And I would very much welcome patches to fix such
a careless input validation routine.  But replacing ulong with size_t
would not make such a problem go away.

Now, I would be a bit more sympathetic if the patch were to use
integers of exact sizes, in the name of "let's make sure that
regardless of the platforms we handle patches up to the same limit".
But size_t is not a type that is appropriate for that (and of course
ulong is not, either---but the original did not aim for such a uniform
limit to begin with).

> @@ -1626,7 +1628,7 @@ static int patch_update_file(struct add_p_state *s,
>  			else
>  				err(s, Q_("Sorry, only %d hunk available.",
>  					  "Sorry, only %d hunks available.",
> -					  file_diff->hunk_nr),
> +					  (int)file_diff->hunk_nr),
>  				    (int)file_diff->hunk_nr);
>  		} else if (s->answer.buf[0] == '/') {
>  			regex_t regex;

I skimmed your "how about going this way" illustration patch and
found all the hunks reasonable, but this one I am not sure.  Is
there a reason why hunk_nr has to be of type size_t?  

When queuing a hunk (and performing an operation that changes the
number of hunks, like splitting an existing one), the code should be
careful not to make too many hunks to overflow "int" (if that is the
more natural type to count them---and "int" being the most natural
integer type for the platform, I tend to think it should be fine),
again, that applies equally if the type of hunk_nr is "size_t".

Thanks.

Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc

[Sören Krecker]

Hi everyone,

If I understand your comments correctly, it would be preferably to 
switch to a data type like uint32_t or uint64_t so that the behavior is 
consisted on all platforms?
Also add a test if the input overflows the data type.

Best regards,

Sören Krecker

Junio C Hamano writes:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>>>>    struct hunk_header {
>>>> -	unsigned long old_offset, old_count, new_offset, new_count;
>>>> +	size_t old_offset, old_count, new_offset, new_count;
>>> These are not "size"s in the traditional sense of what size_t is
>>> (i.e. the number of bytes in a region of memory), but are more or
>>> less proportional to that in that they count in number of lines.
>>> If ulong is sufficient to count number of lines in an incoming
>>> patch, then turning size_t may be excessive---are we sure that we
>>> are not unnecessarily using wider-than-necessary size_t in some
>>> places to hold these values for which ulong is sufficient, causing
>>> compilers to emit unnecessary warning?
>>
>> That's my thought too - I think something like the diff below should
>> fix the warnings by using more appropriate types in expressions
>> involving the hunk header offset and count. Our internal diff
>> implementation will not generate diffs for blobs greater than ~1GB
>> and I don't think "git apply" can handle diff headers that contain
>> numbers greater that ULONG_MAX so switching to size_t here seems
>> unnecessary.
> 
> Yes, exactly.
> 
> Of course, when filling old_offset and friends by parsing an input
> line like this:
> 
>      @@ -253,7 +253,7 @@ struct hunk_header {
> 
> it would be a bug if we did not check if "253" overflows the type of
> old_offset, etc.  And I would very much welcome patches to fix such
> a careless input validation routine.  But replacing ulong with size_t
> would not make such a problem go away.
> 
> Now, I would be a bit more sympathetic if the patch were to use
> integers of exact sizes, in the name of "let's make sure that
> regardless of the platforms we handle patches up to the same limit".
> But size_t is not a type that is appropriate for that (and of course
> ulong is not, either---but the original did not aim for such a uniform
> limit to begin with).
> 
>> @@ -1626,7 +1628,7 @@ static int patch_update_file(struct add_p_state *s,
>>   			else
>>   				err(s, Q_("Sorry, only %d hunk available.",
>>   					  "Sorry, only %d hunks available.",
>> -					  file_diff->hunk_nr),
>> +					  (int)file_diff->hunk_nr),
>>   				    (int)file_diff->hunk_nr);
>>   		} else if (s->answer.buf[0] == '/') {
>>   			regex_t regex;
> 
> I skimmed your "how about going this way" illustration patch and
> found all the hunks reasonable, but this one I am not sure.  Is
> there a reason why hunk_nr has to be of type size_t?
> 
> When queuing a hunk (and performing an operation that changes the
> number of hunks, like splitting an existing one), the code should be
> careful not to make too many hunks to overflow "int" (if that is the
> more natural type to count them---and "int" being the most natural
> integer type for the platform, I tend to think it should be fine),
> again, that applies equally if the type of hunk_nr is "size_t".
> 
> Thanks.

Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc

[Junio C Hamano]

Sören Krecker <soekkle@freenet.de> writes:

> If I understand your comments correctly, it would be preferably to
> switch to a data type like uint32_t or uint64_t so that the behavior
> is consisted on all platforms?

I personally wouldn't prefer that.

I'd rather stick to some "natural" platform type like ulong.  I see
no strong need to say "we must behave identically on all platforms"
in this area.  It is preferrable to have every platform use the most
natural type on it, and make sure that we validate input that is too
large to fit on each platform correctly (i.e. it is OK to diagnose
"too big a line number" and die on 32-bit platform with much smaller
line number than on 64-bit platform).

Thanks.

Re: [PATCH 1/4] add-patch: Fix type missmatch rom msvc

[Phillip Wood]

On 27/12/2024 14:31, Junio C Hamano wrote:
> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> @@ -1626,7 +1628,7 @@ static int patch_update_file(struct add_p_state *s,
>>   			else
>>   				err(s, Q_("Sorry, only %d hunk available.",
>>   					  "Sorry, only %d hunks available.",
>> -					  file_diff->hunk_nr),
>> +					  (int)file_diff->hunk_nr),
>>   				    (int)file_diff->hunk_nr);
>>   		} else if (s->answer.buf[0] == '/') {
>>   			regex_t regex;
> 
> I skimmed your "how about going this way" illustration patch and
> found all the hunks reasonable, but this one I am not sure.  Is
> there a reason why hunk_nr has to be of type size_t?

We certainly don't need to be able to hold that many hunks but changing 
it to a narrower type generates a truncation warning in ALLOC_GROW_BY() 
that macro declares a local size_t variable to hold the new element 
count and then assigns that to hunk_nr

> When queuing a hunk (and performing an operation that changes the
> number of hunks, like splitting an existing one), the code should be
> careful not to make too many hunks to overflow "int" (if that is the
> more natural type to count them---and "int" being the most natural
> integer type for the platform, I tend to think it should be fine),

Yes it's hard to see anyone wanting to use "git add -p" on INT_MAX hunks

> again, that applies equally if the type of hunk_nr is "size_t".

If we cast to "unsigned long" rather than "int" here then we'd be sure 
that there was no overflow as we only support files with up to ULONG_MAX 
lines so there cannot be more than that number of hunks. "unsigned long" 
would also match the prototype of ngettext().

Best Wishes

Phillip

[PATCH 2/4] date.c: Fix type missmatch warings from msvc

[Sören Krecker]

Fix compiler warings from msvc in date.c for value truncation from 64
bit to 32 bit integers.

Also switch from int to size_t for all variables with result of strlen()
which cannot become negative.

Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
 date.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/date.c b/date.c
index bee9fe8f10..8ae19f9ecc 100644
--- a/date.c
+++ b/date.c
@@ -1242,7 +1242,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 	}
 
 	for (s = special; s->name; s++) {
-		int len = strlen(s->name);
+		size_t len = strlen(s->name);
 		if (match_string(date, s->name) == len) {
 			s->fn(tm, now, num);
 			*touched = 1;
@@ -1252,7 +1252,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 
 	if (!*num) {
 		for (i = 1; i < 11; i++) {
-			int len = strlen(number_name[i]);
+			size_t len = strlen(number_name[i]);
 			if (match_string(date, number_name[i]) == len) {
 				*num = i;
 				*touched = 1;
@@ -1268,7 +1268,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
 
 	tl = typelen;
 	while (tl->type) {
-		int len = strlen(tl->type);
+		size_t len = strlen(tl->type);
 		if (match_string(date, tl->type) >= len-1) {
 			update_tm(tm, now, tl->length * *num);
 			*num = 0;
-- 
2.39.5

Re: [PATCH 2/4] date.c: Fix type missmatch warings from msvc

[Junio C Hamano]

Sören Krecker <soekkle@freenet.de> writes:

> Fix compiler warings from msvc in date.c for value truncation from 64
> bit to 32 bit integers.
>
> Also switch from int to size_t for all variables with result of strlen()
> which cannot become negative.
>
> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
>  date.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/date.c b/date.c
> index bee9fe8f10..8ae19f9ecc 100644
> --- a/date.c
> +++ b/date.c
> @@ -1242,7 +1242,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
>  	}
>  
>  	for (s = special; s->name; s++) {
> -		int len = strlen(s->name);
> +		size_t len = strlen(s->name);
>  		if (match_string(date, s->name) == len) {
>  			s->fn(tm, now, num);
>  			*touched = 1;
> @@ -1252,7 +1252,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
>  
>  	if (!*num) {
>  		for (i = 1; i < 11; i++) {
> -			int len = strlen(number_name[i]);
> +			size_t len = strlen(number_name[i]);
>  			if (match_string(date, number_name[i]) == len) {
>  				*num = i;
>  				*touched = 1;
> @@ -1268,7 +1268,7 @@ static const char *approxidate_alpha(const char *date, struct tm *tm, struct tm
>  
>  	tl = typelen;
>  	while (tl->type) {
> -		int len = strlen(tl->type);
> +		size_t len = strlen(tl->type);
>  		if (match_string(date, tl->type) >= len-1) {
>  			update_tm(tm, now, tl->length * *num);
>  			*num = 0;

These are all good changes, unquestionably.  strlen() counts in
bytes and returns size_t; we should recieve the returned value in a
variable of type size_t.

Will queue.  Thanks.

[PATCH 3/4] apply.c : Fix type missmatch warings from msvc

[Sören Krecker]

Fix compiler warings from msvc in date.c for value truncation from 64
bit to 32 bit integers.

Also switch from int to size_t for all variables with result of strlen()
which cannot become negative.

Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
 apply.c | 37 +++++++++++++++++++------------------
 apply.h |  6 +++---
 2 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/apply.c b/apply.c
index a3fc2d5330..5bb0b0e78e 100644
--- a/apply.c
+++ b/apply.c
@@ -413,9 +413,9 @@ static int read_patch_file(struct strbuf *sb, int fd)
 	return 0;
 }
 
-static unsigned long linelen(const char *buffer, unsigned long size)
+static size_t linelen(const char *buffer, size_t size)
 {
-	unsigned long len = 0;
+	size_t len = 0;
 	while (size--) {
 		len++;
 		if (*buffer++ == '\n')
@@ -687,7 +687,7 @@ static char *find_name_common(struct strbuf *root,
 	 * or "file~").
 	 */
 	if (def) {
-		int deflen = strlen(def);
+		size_t deflen = strlen(def);
 		if (deflen < len && !strncmp(start, def, deflen))
 			return squash_slash(xstrdup(def));
 	}
@@ -1087,7 +1087,7 @@ static int gitdiff_index(struct gitdiff_data *state,
 	 */
 	const char *ptr, *eol;
 	int len;
-	const unsigned hexsz = the_hash_algo->hexsz;
+	const size_t hexsz = the_hash_algo->hexsz;
 
 	ptr = strchr(line, '.');
 	if (!ptr || ptr[1] != '.' || hexsz < ptr - line)
@@ -1130,7 +1130,7 @@ static int gitdiff_unrecognized(struct gitdiff_data *state UNUSED,
  */
 static const char *skip_tree_prefix(int p_value,
 				    const char *line,
-				    int llen)
+				    size_t llen)
 {
 	int nslash;
 	int i;
@@ -1157,7 +1157,7 @@ static const char *skip_tree_prefix(int p_value,
  */
 static char *git_header_name(int p_value,
 			     const char *line,
-			     int llen)
+			     ssize_t llen)
 {
 	const char *name;
 	const char *second = NULL;
@@ -1312,15 +1312,15 @@ static int check_header_line(int linenr, struct patch *patch)
 	return 0;
 }
 
-int parse_git_diff_header(struct strbuf *root,
+size_t parse_git_diff_header(struct strbuf *root,
 			  int *linenr,
 			  int p_value,
 			  const char *line,
-			  int len,
-			  unsigned int size,
+			  size_t len,
+			  size_t size,
 			  struct patch *patch)
 {
-	unsigned long offset;
+	size_t offset;
 	struct gitdiff_data parse_hdr_state;
 
 	/* A git diff has explicit new/delete information, so we don't guess */
@@ -1377,7 +1377,7 @@ int parse_git_diff_header(struct strbuf *root,
 			break;
 		for (i = 0; i < ARRAY_SIZE(optable); i++) {
 			const struct opentry *p = optable + i;
-			int oplen = strlen(p->str);
+			size_t oplen = strlen(p->str);
 			int res;
 			if (len < oplen || memcmp(p->str, line, oplen))
 				continue;
@@ -1429,7 +1429,8 @@ static int parse_num(const char *line, unsigned long *p)
 static int parse_range(const char *line, int len, int offset, const char *expect,
 		       unsigned long *p1, unsigned long *p2)
 {
-	int digits, ex;
+	int digits;
+	size_t ex;
 
 	if (offset < 0 || offset >= len)
 		return -1;
@@ -1464,7 +1465,7 @@ static int parse_range(const char *line, int len, int offset, const char *expect
 	return offset + ex;
 }
 
-static void recount_diff(const char *line, int size, struct fragment *fragment)
+static void recount_diff(const char *line, size_t size, struct fragment *fragment)
 {
 	int oldlines = 0, newlines = 0, ret = 0;
 
@@ -1474,7 +1475,7 @@ static void recount_diff(const char *line, int size, struct fragment *fragment)
 	}
 
 	for (;;) {
-		int len = linelen(line, size);
+		size_t len = linelen(line, size);
 		size -= len;
 		line += len;
 
@@ -1542,11 +1543,11 @@ static int parse_fragment_header(const char *line, int len, struct fragment *fra
  */
 static int find_header(struct apply_state *state,
 		       const char *line,
-		       unsigned long size,
+		       size_t size,
 		       int *hdrsize,
 		       struct patch *patch)
 {
-	unsigned long offset, len;
+	size_t offset, len;
 
 	patch->is_toplevel_relative = 0;
 	patch->is_rename = patch->is_copy = 0;
@@ -2131,7 +2132,7 @@ static int use_patch(struct apply_state *state, struct patch *p)
  *   the number of bytes consumed otherwise,
  *     so that the caller can call us again for the next patch.
  */
-static int parse_chunk(struct apply_state *state, char *buffer, unsigned long size, struct patch *patch)
+static int parse_chunk(struct apply_state *state, char *buffer, size_t size, struct patch *patch)
 {
 	int hdrsize, patchsize;
 	int offset = find_header(state, buffer, size, &hdrsize, patch);
@@ -2490,7 +2491,7 @@ static int match_fragment(struct apply_state *state,
 	struct strbuf fixed = STRBUF_INIT;
 	char *fixed_buf;
 	size_t fixed_len;
-	int preimage_limit;
+	ssize_t preimage_limit;
 	int ret;
 
 	if (preimage->line_nr + current_lno <= img->line_nr) {
diff --git a/apply.h b/apply.h
index 90e887ec0e..bb01ce7dbc 100644
--- a/apply.h
+++ b/apply.h
@@ -166,12 +166,12 @@ int check_apply_state(struct apply_state *state, int force_apply);
  *
  * Returns -1 on failure, the length of the parsed header otherwise.
  */
-int parse_git_diff_header(struct strbuf *root,
+size_t parse_git_diff_header(struct strbuf *root,
 			  int *linenr,
 			  int p_value,
 			  const char *line,
-			  int len,
-			  unsigned int size,
+			  size_t len,
+			  size_t size,
 			  struct patch *patch);
 
 void release_patch(struct patch *patch);
-- 
2.39.5

[PATCH 4/4] commit.c: Fix type missmatch warings from msvc

[Sören Krecker]

Fix compiler warings from msvc in date.c for value truncation from 64
bit to 32 bit integers.

Also switch from int to size_t for all variables with result of strlen()
which cannot become negative.

Signed-off-by: Sören Krecker <soekkle@freenet.de>
---
 commit.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/commit.c b/commit.c
index 35ab9bead5..3d363260f3 100644
--- a/commit.c
+++ b/commit.c
@@ -466,8 +466,8 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
 	struct object_id parent;
 	struct commit_list **pptr;
 	struct commit_graft *graft;
-	const int tree_entry_len = the_hash_algo->hexsz + 5;
-	const int parent_entry_len = the_hash_algo->hexsz + 7;
+	const size_t tree_entry_len = the_hash_algo->hexsz + 5;
+	const size_t parent_entry_len = the_hash_algo->hexsz + 7;
 	struct tree *tree;
 
 	if (item->object.parsed)
@@ -1114,10 +1114,10 @@ static const char *gpg_sig_headers[] = {
 
 int add_header_signature(struct strbuf *buf, struct strbuf *sig, const struct git_hash_algo *algo)
 {
-	int inspos, copypos;
+	ssize_t inspos, copypos;
 	const char *eoh;
 	const char *gpg_sig_header = gpg_sig_headers[hash_algo_by_ptr(algo)];
-	int gpg_sig_header_len = strlen(gpg_sig_header);
+	size_t gpg_sig_header_len = strlen(gpg_sig_header);
 
 	/* find the end of the header */
 	eoh = strstr(buf->buf, "\n\n");
@@ -1530,7 +1530,7 @@ int commit_tree(const char *msg, size_t msg_len, const struct object_id *tree,
 	return result;
 }
 
-static int find_invalid_utf8(const char *buf, int len)
+static int find_invalid_utf8(const char *buf, size_t len)
 {
 	int offset = 0;
 	static const unsigned int max_codepoint[] = {
-- 
2.39.5

Re: [PATCH 4/4] commit.c: Fix type missmatch warings from msvc

[Junio C Hamano]

Sören Krecker <soekkle@freenet.de> writes:

> Fix compiler warings from msvc in date.c for value truncation from 64
> bit to 32 bit integers.
>
> Also switch from int to size_t for all variables with result of strlen()
> which cannot become negative.
>
> Signed-off-by: Sören Krecker <soekkle@freenet.de>
> ---
>  commit.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 35ab9bead5..3d363260f3 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -466,8 +466,8 @@ int parse_commit_buffer(struct repository *r, struct commit *item, const void *b
>  	struct object_id parent;
>  	struct commit_list **pptr;
>  	struct commit_graft *graft;
> -	const int tree_entry_len = the_hash_algo->hexsz + 5;
> -	const int parent_entry_len = the_hash_algo->hexsz + 7;
> +	const size_t tree_entry_len = the_hash_algo->hexsz + 5;
> +	const size_t parent_entry_len = the_hash_algo->hexsz + 7;

We saw another change around hexsz in this series, but I seriously
doubt that it is sensible to define .hexsz member of git_hash_algo
as type size_t.  The whole _point_ of hash function is so that it
can be represented by a handful of bytes, so insisting size_t and
forcing us to suffer code churning like we see here is simply crazy.

Would it work equally well, if not better, if you instead fixed the
type of the .hexsz member (and its friends) to something more
reasonable, like "int"?

Re: [PATCH 0/4] Fixes typemissmatch warinigs from msvc

[Junio C Hamano]

Sören Krecker <soekkle@freenet.de> writes:

> A smale series of patches to fix some typemissmatch warings from msvc 14.30.
> Most of the missmatches a 64 to 32 bit conversion on a 64 bit Windows platform.

Thanks for the patches.

I'll welcome other people to take a look, if they are inclined, but
it is not something I'd want to look at during a pre-release freeze.
Nobody sane would be running "git add -p" on a patch that exceeds
2GB, for example, so the only practical thing they fix are compiler
warnings.  They are worth fixing eventually, but not all that
urgent.

Thanks, again.

Re: [PATCH 0/4] Fixes typemissmatch warinigs from msvc

[Junio C Hamano]

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

> Sören Krecker <soekkle@freenet.de> writes:
>
>> A smale series of patches to fix some typemissmatch warings from msvc 14.30.
>> Most of the missmatches a 64 to 32 bit conversion on a 64 bit Windows platform.
>
> Thanks for the patches.
>
> I'll welcome other people to take a look, if they are inclined, but
> it is not something I'd want to look at during a pre-release freeze.
> Nobody sane would be running "git add -p" on a patch that exceeds
> 2GB, for example, so the only practical thing they fix are compiler
> warnings.  They are worth fixing eventually, but not all that
> urgent.
>
> Thanks, again.

Oops, sorry, this didn't come out quite right.  I didn't mean to say
that this contribution is unwelcome.  I'll get to it eventually
(like, after the upcoming release), but please do not expect them to
be merged before the upcoming release.

Re: [PATCH 0/4] Fixes typemissmatch warinigs from msvc

[Sören Krecker]

Hi Junio,

I think I undersand your first mail correct, that this is only a code 
quality change and not a bug in the code. And so it isn't critical for 
git 2.48.
In the future I will send some more commits for warings from msvc in 
git. MSVC print more the 1000 warings.

Best regards,

Sören Krecker


Am 23.12.24 um 17:52 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Sören Krecker <soekkle@freenet.de> writes:
>>
>>> A smale series of patches to fix some typemissmatch warings from msvc 14.30.
>>> Most of the missmatches a 64 to 32 bit conversion on a 64 bit Windows platform.
>>
>> Thanks for the patches.
>>
>> I'll welcome other people to take a look, if they are inclined, but
>> it is not something I'd want to look at during a pre-release freeze.
>> Nobody sane would be running "git add -p" on a patch that exceeds
>> 2GB, for example, so the only practical thing they fix are compiler
>> warnings.  They are worth fixing eventually, but not all that
>> urgent.
>>
>> Thanks, again.
> 
> Oops, sorry, this didn't come out quite right.  I didn't mean to say
> that this contribution is unwelcome.  I'll get to it eventually
> (like, after the upcoming release), but please do not expect them to
> be merged before the upcoming release.
>