Re: [Patch size_t V3 19/19] Convert xdiff-interface to size_t

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

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

> diff --git a/combine-diff.c b/combine-diff.c
> index acf39ec..ad5d177 100644
> --- a/combine-diff.c
> +++ b/combine-diff.c
> @@ -343,7 +343,7 @@ struct combine_diff_state {
>  	struct sline *lost_bucket;
>  };
>  
> -static void consume_line(void *state_, char *line, unsigned long len)
> +static void consume_line(void *state_, char *line, size_t len)
>  {
>  	struct combine_diff_state *state = state_;
>  	if (5 < len && !memcmp("@@ -", line, 4)) {

This is a correct logical consequence of making consume_fn to take
size_t.

> diff --git a/diff.c b/diff.c
> index c12d062..f665f8d 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -406,7 +406,7 @@ static struct diff_tempfile {
>  	struct tempfile tempfile;
>  } diff_temp[2];
>  
> -typedef unsigned long (*sane_truncate_fn)(char *line, unsigned long len);
> +typedef size_t (*sane_truncate_fn)(char *line, size_t len);

It turns out that this type, the member of this type in ecb struct, and
a conditional call to the function when it is not NULL, are all unused.
If this were used, this conversion is correct ;-)

> @@ -4536,7 +4536,7 @@ struct patch_id_t {
>  	int patchlen;
>  };
>  
> -static int remove_space(char *line, int len)
> +static size_t remove_space(char *line, size_t len)
>  {
>  	int i;
>  	char *dst = line;

This function may also want to be rewritten.  The loop counter and
array index "i" is used this way:

	for (i = 0; i < len i++)
		if (!isspace((c = line[i]))
			*dst++ = c;
	return dst - line;

So you are still risking data loss (later parts of line[] may not be
scanned due to int possibly being narrower than size_t).

Turning "len" and the return type into size_t is absolutely the
right thing to do, and it is tempting to turn "i" also into size_t,
but we may just want to scan the thing with another pointer, i.e.

        size_t remove_space(char *line, size_t len)
        {
                char *src = line;
                char *dst = line;

                while (src < line + len) {
                        char c = *src++;
                        if (!isspace(c))
                                *dst++ = c;
                }
                return dst - line;
        }

Changes to size_t from types other than "ulong" is worth looking at
twice for a potential issue like this.

> diff --git a/xdiff-interface.c b/xdiff-interface.c
> index d82cd4a..f12285d 100644
> --- a/xdiff-interface.c
> +++ b/xdiff-interface.c
> @@ -26,7 +26,7 @@ static int parse_num(char **cp_p, int *num_p)
>  	return 0;
>  }
>  
> -int parse_hunk_header(char *line, int len,
> +int parse_hunk_header(char *line, size_t len,
>  		      int *ob, int *on,
>  		      int *nb, int *nn)
>  {

This is a correct conversion for the purpose of this series, but the
implementation of this function before (and after) this patch is
already incorrect.  It does not pay any attention to "len", and
neither the helper function this function uses, parse_num(), is
told how many bytes are remaining on the line to be parsed, so it
will happily go beyond "len".

The two things we may want to fix is obviously outside the scope of
this series, but they should be fixed (not necessarily by you) after
this series once the codebase solidifies, or before this series as
preliminary clean-up.

Thanks.