Re: [PATCH v2 2/4] archive-tar: mark RECORDSIZE/BLOCKSIZE as unsigned

["René Scharfe" <l.s.r@web.de>]

Am 09.05.19 um 20:38 schrieb Jeff King:
> I do dream of a world where we do not have a bunch of implicit
> conversions (both signedness but also truncation) in our code base, and
> can compile cleanly with -Wconversion We know that this case is
> perfectly fine, but I am sure there are many that are not. However, I'm
> not sure if we'll ever get there, and in the meantime I don't think it's
> worth worrying too much about individual cases like this.

Here's a rough take on how to silence that warning for archive-tar.c using
GCC 8.3.  Some of the changes are worth polishing and submitting.  Some
are silly.  The one for regexec_buf() is scary; I don't see a clean way of
dealing with that size_t to int conversion.

---
 archive-tar.c     | 54 +++++++++++++++++++++++++++++++----------------
 cache.h           | 10 ++++-----
 git-compat-util.h | 21 +++++++++++++++---
 hash.h            |  2 +-
 strbuf.h          |  2 +-
 5 files changed, 61 insertions(+), 28 deletions(-)

diff --git a/archive-tar.c b/archive-tar.c
index 3e53aac1e6..bfd91782ab 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -15,7 +15,7 @@
 static char block[BLOCKSIZE];
 static unsigned long offset;

-static int tar_umask = 002;
+static mode_t tar_umask = 002;

 static int write_tar_filter_archive(const struct archiver *ar,
 				    struct archiver_args *args);
@@ -99,7 +99,7 @@ static void write_blocked(const void *data, unsigned long size)
  */
 static void write_trailer(void)
 {
-	int tail = BLOCKSIZE - offset;
+	size_t tail = BLOCKSIZE - offset;
 	memset(block + offset, 0, tail);
 	write_or_die(1, block, BLOCKSIZE);
 	if (tail < 2 * RECORDSIZE) {
@@ -127,12 +127,13 @@ static int stream_blocked(const struct object_id *oid)
 		readlen = read_istream(st, buf, sizeof(buf));
 		if (readlen <= 0)
 			break;
-		do_write_blocked(buf, readlen);
+		do_write_blocked(buf, (size_t)readlen);
 	}
 	close_istream(st);
-	if (!readlen)
-		finish_record();
-	return readlen;
+	if (readlen < 0)
+		return -1;
+	finish_record();
+	return 0;
 }

 /*
@@ -142,9 +143,9 @@ static int stream_blocked(const struct object_id *oid)
  * string and appends it to a struct strbuf.
  */
 static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,
-				     const char *value, unsigned int valuelen)
+				     const char *value, size_t valuelen)
 {
-	int len, tmp;
+	size_t len, tmp;

 	/* "%u %s=%s\n" */
 	len = 1 + 1 + strlen(keyword) + 1 + valuelen + 1;
@@ -152,7 +153,7 @@ static void strbuf_append_ext_header(struct strbuf *sb, const char *keyword,
 		len++;

 	strbuf_grow(sb, len);
-	strbuf_addf(sb, "%u %s=", len, keyword);
+	strbuf_addf(sb, "%"PRIuMAX" %s=", (uintmax_t)len, keyword);
 	strbuf_add(sb, value, valuelen);
 	strbuf_addch(sb, '\n');
 }
@@ -168,7 +169,9 @@ static void strbuf_append_ext_header_uint(struct strbuf *sb,
 	int len;

 	len = xsnprintf(buf, sizeof(buf), "%"PRIuMAX, value);
-	strbuf_append_ext_header(sb, keyword, buf, len);
+	if (len < 0)
+		BUG("unable to convert %"PRIuMAX" to decimal", value);
+	strbuf_append_ext_header(sb, keyword, buf, (size_t)len);
 }

 static unsigned int ustar_header_chksum(const struct ustar_header *header)
@@ -177,7 +180,7 @@ static unsigned int ustar_header_chksum(const struct ustar_header *header)
 	unsigned int chksum = 0;
 	while (p < (const unsigned char *)header->chksum)
 		chksum += *p++;
-	chksum += sizeof(header->chksum) * ' ';
+	chksum += (unsigned int)sizeof(header->chksum) * ' ';
 	p += sizeof(header->chksum);
 	while (p < (const unsigned char *)header + sizeof(struct ustar_header))
 		chksum += *p++;
@@ -355,12 +358,14 @@ static void write_global_extended_header(struct archiver_args *args)
 }

 static struct archiver **tar_filters;
-static int nr_tar_filters;
-static int alloc_tar_filters;
+static size_t nr_tar_filters;
+static size_t alloc_tar_filters;

-static struct archiver *find_tar_filter(const char *name, int len)
+static struct archiver *find_tar_filter(const char *name, size_t len)
 {
 	int i;
+	if (len < 1)
+		return NULL;
 	for (i = 0; i < nr_tar_filters; i++) {
 		struct archiver *ar = tar_filters[i];
 		if (!strncmp(ar->name, name, len) && !ar->name[len])
@@ -369,14 +374,27 @@ static struct archiver *find_tar_filter(const char *name, int len)
 	return NULL;
 }

+static int parse_config_key2(const char *var, const char *section,
+			     const char **subsection, size_t *subsection_len,
+			     const char **key)
+{
+	int rc, len;
+
+	rc = parse_config_key(var, section, subsection, &len, key);
+	if (!rc && len < 0)
+		return -1;
+	*subsection_len = (size_t)len;
+	return rc;
+}
+
 static int tar_filter_config(const char *var, const char *value, void *data)
 {
 	struct archiver *ar;
 	const char *name;
 	const char *type;
-	int namelen;
+	size_t namelen;

-	if (parse_config_key(var, "tar", &name, &namelen, &type) < 0 || !name)
+	if (parse_config_key2(var, "tar", &name, &namelen, &type) < 0 || !name)
 		return 0;

 	ar = find_tar_filter(name, namelen);
@@ -400,7 +418,7 @@ static int tar_filter_config(const char *var, const char *value, void *data)
 		if (git_config_bool(var, value))
 			ar->flags |= ARCHIVER_REMOTE;
 		else
-			ar->flags &= ~ARCHIVER_REMOTE;
+			ar->flags &= ~(unsigned int)ARCHIVER_REMOTE;
 		return 0;
 	}

@@ -414,7 +432,7 @@ static int git_tar_config(const char *var, const char *value, void *cb)
 			tar_umask = umask(0);
 			umask(tar_umask);
 		} else {
-			tar_umask = git_config_int(var, value);
+			tar_umask = (mode_t)git_config_ulong(var, value);
 		}
 		return 0;
 	}
diff --git a/cache.h b/cache.h
index 67cc2e1806..a791034260 100644
--- a/cache.h
+++ b/cache.h
@@ -241,7 +241,7 @@ static inline void copy_cache_entry(struct cache_entry *dst,
 				    const struct cache_entry *src)
 {
 	unsigned int state = dst->ce_flags & CE_HASHED;
-	int mem_pool_allocated = dst->mem_pool_allocated;
+	unsigned int mem_pool_allocated = dst->mem_pool_allocated;

 	/* Don't copy hash chain and name */
 	memcpy(&dst->ce_stat_data, &src->ce_stat_data,
@@ -249,7 +249,7 @@ static inline void copy_cache_entry(struct cache_entry *dst,
 			offsetof(struct cache_entry, ce_stat_data));

 	/* Restore the hash state */
-	dst->ce_flags = (dst->ce_flags & ~CE_HASHED) | state;
+	dst->ce_flags = (dst->ce_flags & ~(unsigned int)CE_HASHED) | state;

 	/* Restore the mem_pool_allocated flag */
 	dst->mem_pool_allocated = mem_pool_allocated;
@@ -1314,7 +1314,7 @@ extern int check_and_freshen_file(const char *fn, int freshen);
 extern const signed char hexval_table[256];
 static inline unsigned int hexval(unsigned char c)
 {
-	return hexval_table[c];
+	return (unsigned int)hexval_table[c];
 }

 /*
@@ -1323,8 +1323,8 @@ static inline unsigned int hexval(unsigned char c)
  */
 static inline int hex2chr(const char *s)
 {
-	unsigned int val = hexval(s[0]);
-	return (val & ~0xf) ? val : (val << 4) | hexval(s[1]);
+	int val = hexval_table[(unsigned char)s[0]];
+	return (val < 0) ? val : (val << 4) | hexval_table[(unsigned char)s[1]];
 }

 /* Convert to/from hex/sha1 representation */
diff --git a/git-compat-util.h b/git-compat-util.h
index 4386b3e1c8..cf33e84c96 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -1068,7 +1068,7 @@ static inline int strtoul_ui(char const *s, int base, unsigned int *result)
 	ul = strtoul(s, &p, base);
 	if (errno || *p || p == s || (unsigned int) ul != ul)
 		return -1;
-	*result = ul;
+	*result = (unsigned int)ul;
 	return 0;
 }

@@ -1081,7 +1081,7 @@ static inline int strtol_i(char const *s, int base, int *result)
 	ul = strtol(s, &p, base);
 	if (errno || *p || p == s || (int) ul != ul)
 		return -1;
-	*result = ul;
+	*result = (int)ul;
 	return 0;
 }

@@ -1119,7 +1119,22 @@ static inline int regexec_buf(const regex_t *preg, const char *buf, size_t size,
 {
 	assert(nmatch > 0 && pmatch);
 	pmatch[0].rm_so = 0;
-	pmatch[0].rm_eo = size;
+	pmatch[0].rm_eo = (regoff_t)size;
+	if (pmatch[0].rm_eo != size) {
+		if (((regoff_t)-1) < 0) {
+			if (sizeof(regoff_t) == sizeof(int))
+				pmatch[0].rm_eo = (regoff_t)INT_MAX;
+			else if (sizeof(regoff_t) == sizeof(long))
+				pmatch[0].rm_eo = (regoff_t)LONG_MAX;
+			else
+				die("unable to determine maximum value of regoff_t");
+		} else {
+			pmatch[0].rm_eo = (regoff_t)-1;
+		}
+		warning("buffer too big (%"PRIuMAX"), "
+			"will search only the first %"PRIuMAX" bytes",
+			(uintmax_t)size, (uintmax_t)pmatch[0].rm_eo);
+	}
 	return regexec(preg, buf, nmatch, pmatch, eflags | REG_STARTEND);
 }

diff --git a/hash.h b/hash.h
index 661c9f2281..7056f89eb4 100644
--- a/hash.h
+++ b/hash.h
@@ -134,7 +134,7 @@ int hash_algo_by_id(uint32_t format_id);
 /* Identical, except based on the length. */
 int hash_algo_by_length(int len);
 /* Identical, except for a pointer to struct git_hash_algo. */
-static inline int hash_algo_by_ptr(const struct git_hash_algo *p)
+static inline ptrdiff_t hash_algo_by_ptr(const struct git_hash_algo *p)
 {
 	return p - hash_algos;
 }
diff --git a/strbuf.h b/strbuf.h
index c8d98dfb95..30659f2d5d 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -225,7 +225,7 @@ int strbuf_cmp(const struct strbuf *first, const struct strbuf *second);
 /**
  * Add a single character to the buffer.
  */
-static inline void strbuf_addch(struct strbuf *sb, int c)
+static inline void strbuf_addch(struct strbuf *sb, char c)
 {
 	if (!strbuf_avail(sb))
 		strbuf_grow(sb, 1);
--
2.21.0