On Thu, Feb 17 2022, Jiang Xin wrote:

From: Jiang Xin <redacted>

On Sat, Feb 5, 2022 Ævar Arnfjörð Bjarmason wrote:

quoted

diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 2b2e28bad79..123df7d9a53 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c

@@ -937,8 +937,8 @@ static int store_object(
 	git_hash_ctx c;
 	git_zstream s;
 
-	hdrlen = xsnprintf((char *)hdr, sizeof(hdr), "%s %lu",
-			   type_name(type), (unsigned long)dat->len) + 1;
+	hdrlen = format_object_header((char *)hdr, sizeof(hdr), type,
+				      dat->len);

Type casting can be avoid if we use "void *" as the first parameter of
"format_object_header", and do type casting inside the helper function.
[...]
The return value of `type_name(type)` has not been checked for the original
implement, how about write a online inline-function in a header file like this:

	static inline int format_object_header(void *str, size_t size,
					       const char *type_name,
					       size_t objsize)
	{
		return xsnprintf((char *)str, size, "%s %"PRIuMAX, type_name,
				 (uintmax_t)objsize) + 1;
	}

I don't think the casting in the callers is bad, in that for the callers
that do use "char *" we get the compiler to help us with type checking.

Using a void * is something we really reserve only for callback-type
values, because it mens that now nobody gets any type checking.

I think if we wanted to avoid the casts it would make more sense to add
a trivial ucformat_object_header() wrapper or whatever, which would take
"unsigned chan *" and do the cast, or just tweak the relevant calling
code to change the type (IIRC some of it used unsigned v.s. signed for
no particular reason).

But I think just leaving this part as it is is better here...

[...]

quoted

+	if (!name)
+		BUG("could not get a type name for 'enum object_type' value %d", type);
+

The return value of `type_name(type)` has not been checked for the original
implement, how about write a online inline-function in a header file like this:

Yes, this part is not a faithful conversion on my part, but I think it
made sense when converting this to a library function.

The alternative is that we'd segfault on some platforms (not glibc,
since it's OK with null %s arguments), just checking it is cheap & I
think a good sanity check...