Re: [PATCH] Convert size datatype to size_t

[Junio C Hamano <gitster@pobox.com>]

Martin Koegler <martin.koegler@chello.at> writes:

> diff --git a/apply.c b/apply.c
> index f2d5991..ea97fd2 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -3085,7 +3085,7 @@ static int apply_binary_fragment(struct apply_state *state,
>  				 struct patch *patch)
>  {
>  	struct fragment *fragment = patch->fragments;
> -	unsigned long len;
> +	size_t len;
>  	void *dst;
>  
>  	if (!fragment)

This variable is made size_t because it receives the result size of
patch_delta().  And it later is assigned to img->len field, which
already is size_t before this patch.

Curiously, the patch_delta() invocation with this patch reads like
this:

		dst = patch_delta(img->buf, img->len, fragment->patch,
				  fragment->size, &len);

where patch_delta() is updated (correctly) to:

    void *patch_delta(const void *src_buf, size_t src_size,
                      const void *delta_buf, size_t delta_size,
                      size_t *dst_size)

with this patch.  But "size" field in "struct fragment" is still
"int".  So we'd need to update it as well, not necessarily in this
patch (which is already too big) but as part of a larger whole.

> @@ -3174,7 +3174,7 @@ static int apply_binary(struct apply_state *state,
>  	if (has_sha1_file(oid.hash)) {
>  		/* We already have the postimage */
>  		enum object_type type;
> -		unsigned long size;
> +		size_t size;
>  		char *result;
>  
>  		result = read_sha1_file(oid.hash, &type, &size);

This is to receive the resulting size from read_sha1_file().  It is
assigned to img->len, which is already size_t, so all is good here.

> @@ -3236,7 +3236,7 @@ static int read_blob_object(struct strbuf *buf, const struct object_id *oid, uns
>  		strbuf_addf(buf, "Subproject commit %s\n", oid_to_hex(oid));
>  	} else {
>  		enum object_type type;
> -		unsigned long sz;
> +		size_t sz;
>  		char *result;
>  
>  		result = read_sha1_file(oid->hash, &type, &sz);

By reading the remainder of this function, this conversion also is
good.  sz that is now size_t is used as the size attached to an
existing strbuf like so:

		result = read_sha1_file(oid->hash, &type, &sz);
		if (!result)
			return -1;
		/* XXX read_sha1_file NUL-terminates */
		strbuf_attach(buf, result, sz, sz + 1);

in the part beyond the post context of this hunk.  In the longer
term, sz+1 we see here may want to become the overflow-safe variant
st_add().

As you said in the comment after three-dashes in the patch, a lot
more work is needed and your patch is a good starting point.

I am not sure if we can split the patch somehow to make it easier to
review.  The deceptively small part of your patch, i.e.

 apply.c                  |  6 +++---

needs the above analysis to see if they are correct and what more
work is necessary, and there are 65 more files with ~190 lines
changed.