Re: [PATCH] git: edit variable types to match what is assigned to them

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

"Rose via GitGitGadget" <gitgitgadget@gmail.com> writes:

> diff --git a/add-interactive.c b/add-interactive.c
> index ae1839c04a7..59ac88f8b5a 100644
> --- a/add-interactive.c
> +++ b/add-interactive.c
> @@ -469,7 +469,7 @@ static void collect_changes_cb(struct diff_queue_struct *q,
>  
>  	for (i = 0; i < stat.nr; i++) {
>  		const char *name = stat.files[i]->name;
> -		int hash = strhash(name);
> +		unsigned int hash = strhash(name);
>  		struct pathname_entry *entry;
>  		struct file_item *file_item;
>  		struct adddel *adddel, *other_adddel;

This is unquestionably correct.  strhash() returns unsigned, and
hash is passed to hashmap functions that expect unsigned.

> diff --git a/apply.c b/apply.c
> index bc338143134..ee5dcdb9133 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -443,7 +443,7 @@ static int name_terminate(int c, int terminate)
>  /* remove double slashes to make --index work with such filenames */
>  static char *squash_slash(char *name)
>  {
> -	int i = 0, j = 0;
> +	size_t i = 0, j = 0;
>  
>  	if (!name)
>  		return NULL;

These pointers are used to iterate over the same array from the
beginning to the end, so size_t is very much appropriate.

> @@ -654,7 +654,7 @@ static char *find_name_common(struct strbuf *root,
>  			      const char *end,
>  			      int terminate)
>  {
> -	int len;
> +	size_t len;
>  	const char *start = NULL;
>  
>  	if (p_value == 0)
> @@ -685,13 +685,13 @@ static char *find_name_common(struct strbuf *root,
>  	 * or "file~").
>  	 */
>  	if (def) {
> -		int deflen = strlen(def);
> +		size_t deflen = strlen(def);
>  		if (deflen < len && !strncmp(start, def, deflen))
>  			return squash_slash(xstrdup(def));
>  	}

The above look sensible as these two variables are to hold the
result from strlen().

>  	if (root->len) {
> -		char *ret = xstrfmt("%s%.*s", root->buf, len, start);
> +		char *ret = xstrfmt("%s%.*s", root->buf, (int)len, start);

This rewrite is correct, but having to do this makes the careful use
of (potentially larger) size_t more or less a moot point.  If the
length does not fit in "int", the returned pathname would not have
the correct contents.

> @@ -790,7 +790,7 @@ static int has_epoch_timestamp(const char *nameline)
>  	const char *timestamp = NULL, *cp, *colon;
>  	static regex_t *stamp;
>  	regmatch_t m[10];
> -	int zoneoffset, epoch_hour, hour, minute;
> +	long zoneoffset, epoch_hour, hour, minute;
>  	int status;

This are not wrong per-se, but is not necessary.

We use strtol() at the beginning of a string to read into hour, but
we clamp the source digit string with regexp that begins with
"^[0-2][0-9]:([0-5][0-9]):00(\\.0+)?" so unless your int is so small
that it cannot hold 29, the code should be safe.  The same holds for
the minute.  The latter part of the same regexp clamps the zone
offset in a similar fashion and we won't feed a string longer than 4
decimal digits to strtol() to read the zone offset.

> @@ -1083,7 +1083,7 @@ static int gitdiff_index(struct gitdiff_data *state,
>  	 * and optional space with octal mode.
>  	 */
>  	const char *ptr, *eol;
> -	int len;
> +	size_t len;
>  	const unsigned hexsz = the_hash_algo->hexsz;

Absolutely correct.  The variable is used to hold a ptrdiff and is
used to count number of bytes memcpy() should copy.

> @@ -2172,9 +2172,9 @@ static int parse_chunk(struct apply_state *state, char *buffer, unsigned long si
>  				"Files ",
>  				NULL,
>  			};
> -			int i;
> +			size_t i;
>  			for (i = 0; binhdr[i]; i++) {
> -				int len = strlen(binhdr[i]);
> +				size_t len = strlen(binhdr[i]);
>  				if (len < size - hd &&
>  				    !memcmp(binhdr[i], buffer + hd, len)) {
>  					state->linenr++;

OK.  But I think there are bigger fish in the same function to fry
around hdrsize and find_header(), both of which may want to become
size_t (I didn't look too carefully, though).

> diff --git a/base85.c b/base85.c
> index 5ca601ee14f..ad32ba1411a 100644
> --- a/base85.c
> +++ b/base85.c
> @@ -37,14 +37,15 @@ static void prep_base85(void)
>  	}
>  }
>  
> -int decode_85(char *dst, const char *buffer, int len)
> +int decode_85(char *dst, const char *buffer, size_t len)
>  {
>  	prep_base85();
>  
> -	say2("decode 85 <%.*s>", len / 4 * 5, buffer);
> +	say2("decode 85 <%.*s>", (int)(len / 4 * 5), buffer);
>  	while (len) {
>  		unsigned acc = 0;
> -		int de, cnt = 4;
> +		int de;
> +		size_t cnt = 4;
>  		unsigned char ch;
>  		do {
>  			ch = *buffer++;

Correct.  The debug-only bits around say2() spoils the careful
debugging the same way as the "len" in find_name_common() we saw
earlier, though

I'd stop here for now.

Thanks.