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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 27, 2024, 14:31 UTC
Message-ID: <xmqq5xn5urhv.fsf@gitster.g>
In-Reply-To: <e396131c-1bd3-46d0-bae6-cd97ca9710d8@gmail.com>

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

Show 19 quoted lines

>>>   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).

Show 9 quoted lines

> @@ -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.