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

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