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